OcBootManagementLib: Add InstanceIdentifier, and ability to target .contentVisibility to specific instances

[mikebeaton]

Redoes #446 in the manner discussed towards the end of that thread, as mentioned by PM to @dakanji.

@dakanji, please can you try the build artefacts now that they're ready, and let me know if this covers your needs?

@vit9696, I believe this is as you discussed/intended, but if you can confirm that it looks about right, as and when you have a moment.

[dakanji]

@mikebeaton ... Thanks.

Will try the artefact at some point over the weekend.

One thing I had noticed earlier, as an aside, is that the available constant was not used in the Visibility = OcReadFile call.
I note you have maintained this, presumably for good reason.

[dakanji]

@mikebeaton ... I tried this and it does not appear to be working.

Both Disabled: TAG_NAME and Disabled:TAG_NAME do not hide the item.
Perhaps you can test in case I got something wrong.

Actually, I think I will reactivate the original PR and complete this based on my vision:

• Adds the InstanceIdentifier item (Will more or less use use your code here)
• Uses bounded items as search strings based on tildes (~) which seems more readable than @ and I think readability was the primary and real issue before.
  • Will obviously happily use the feature with a comma delimited list if decreed that is what it has to be.
  • Just want to present an alternative on that aspect for a call to be made and to complete what was started

[mikebeaton]

@mikebeaton ... I tried this and it does not appear to be working.

Both Disabled: TAG_NAME and Disabled:TAG_NAME do not hide the item. Perhaps you can test in case I got something wrong.

Actually, I think I will reactivate the original PR and complete this based on my vision:

• Adds the InstanceIdentifier item (Will more or less use use your code here)

• Uses bounded items as search strings based on tildes (~) which seems more readable than @ and I think readability was the primary and real issue before.

  • Will obviously happily use the feature with a comma delimited list if decreed that is what it has to be.
  • Just want to present an alternative on that aspect for a call to be made and to complete what was started

I have tested and it definitely works for me. I'll add some additional debug lines.

[dakanji]

I have tested and it definitely works for me. I'll add some additional debug lines.

Will test again when done

[mikebeaton]

I've added some additional debug messages in an extra commit. Let's confirm (e.g. initial debug log to compare to subsequent ones) that you've got a .contentVisibility file without the qualifier list in a location where it definitely works (reduces visibility) for all OC instances (so just Disabled or Auxiliary) in the same context as the test, and then take a DEBUG log with the new commit logging what happens when you add a qualifier (Disabled:TAG_NAME,TAG_NAME2,TAG_NAME3 is indeed the intended format, spaces are not trimmed and Disabled: TAG_NAME would not work as presumably intended).

[mikebeaton]

Actually, I suspect I know what the issue would be. Existing .contentVisibility files are lax about having or not having terminating \r\n or \n, and actually about having any old text as long as it starts with Disabled or Auxiliary, which may or may not have been intended. Let me just upgrade my changes to handle terminating linefeeds gracefully.

[mikebeaton]

@dakanji - Okay, this latest version:

• Allows "Disabled:FOO\n" as well as "Disabled:FOO" (i.e. file can be created with echo, or normal editor, does not required echo -n)
  • (FYI Still no space stripping other than handling the (first) terminating \r or \n)
• Allows both "Disabled" and "Disabled\n" (as previously)
• No longer allows "DisabledMess" or "DisabledMess\n" to work as if it was "Disabled" (previously did)

I suspect that should get it working for you.

[dakanji]

It is hiding the item now but has an issue that is not present in the #446 implementation in that when you go back to an older version of OC, that has the current existing implementation, they disable the item.

That is, they treat things as if you just entered Disabled or Auxiliary and every item with qualified instructions have the instructions applied without discrimination. So, cannot use an older known working instance as an example.

Will be away for long stretches and that presumably that can be overcome and a final decision made.
Will drop in as when able to

[mikebeaton]

It is hiding the item now but has an issue that is not present in the #446 implementation in that when you go back to an older version of OC, that has the current existing implementation, they disable the item.

That is, they treat things as if you just entered Disabled or Auxiliary and every item with qualified instructions have the instructions applied without discrimination. So, cannot use an older known working instance as an example.

Will be away for long stretches and that presumably that can be overcome and a final decision made. Will drop in as when able to

Okay, thanks for taking the time to confirm this version all works now. And for taking the time to convince those-who-make-the-decisions that the feature was worth having, in the first place. If Vit does go with this version of the feature, certainly due another thx to yourself in the Changelog, which I'll add.

I'll leave my PR as is, for now - I'm not sure there's anything to fix, in terms of the fact that the 'qualified' visibilities for this implementation of the idea will be treated as unqualified visibilities if seen by the old .contentVis code. (Actually, it's unclear to me how the proposed visibilities for #446 would not be the same? All that matters in the old code, I believe, is the word the file starts with.)

[dakanji]

Noted and appreciated although the credit thing is not so important.
Focus (on both sides) is making sure stuff works as best as possible ... just a largely academic/reference difference in approach at play.

All that matters in the old code, I believe, is the word the file starts with.

You are correct. I had started on an initial that used different instruction names for legacy compatibility but stopped as I thought (prejudging going on) it would trigger a push back and didn't cleanup and finish it but had the binary in my ESP.

When then swapping various binaries, I got mixed up as I had forgotten about that item as well as having that binary as the "446" binary.

Anyway, I now think handling legacy instances is important and will finish then push that code.

[mikebeaton]

I have updated the commit so that the .contentVisibility file format is OCA,OCB:Disabled, rather than Disabled:OCA,OCB.

As @dakanji correctly points out, it is preferable that the format of 'new style' content visibility should be such that older versions of OpenCore treat it as invalid and display the entry - however, older versions of OpenCore, since 7b7d6ea, will accept any characters after Auxiliary or Disabled, and so would always hide the entry if the new format was Disabled:OCA,OCB.