Inconsistent Check behaviour when using DependsOnKeys

[Herobrine24]

Issue:
When setting up an install option with multiple negative DependsOnKeys values and ACTION_ALLOW_SELECT as the NotMetAction, the install option will remain selected when NotMetAction is true due to first having selected the first install option as ordered in DependsOnKeys. If NotMetAction is true and the non-first option is selected, this conditional option will become unchecked.

Expected behaviour:
The selection of the first OptionKey causes the conditional install option to become unchecked.

Initial state:

First OptionKey is selected:

Non-first OptionKey is selected:

Example Mod:
RemoveEnemyGrenades_1.0.zip

Example Mod with workaround:
RemoveEnemyGrenades_1.0.zip
A hidden OP_NOTHING option is used as the first OptionKey in DependsOnKeys to achieve the desired behaviour.

[Mgamerz]

I'm able to repro this. The options system internally is wildly complicated so this will take time to figure out.

[Mgamerz]

I think I am narrowing on the issue.

The reason this affects the first option (cerberus), but not others, is because the dependsonkeys are enumerated in order for this specific mod. The code exhibiting the issue is here: https://github.com/ME3Tweaks/ME3TweaksModManager/blob/backport-92/MassEffectModManagerCore/modmanager/objects/alternates/AlternateOption.cs#L284

The issue, I think, is due to how I short circuit the loop when the condition is met or not. If if it not met, it immediately fires notmet() and stops checking the further items. This means that when you pick Cerberus, the first loop iteration fires notmet() and nothing fires again. When applying the dependson action, if the option is selectable (not disabled checkbox), AND the target state is also selectable (not disabled), the checkbox state isn't changed, in order to not change the user selection.

When you choose the second option, it loops over the options, and Cerberus is not checked. There is code that is applying DependsOnMet(), which I think is a bug, as this should only be run after all other options have been checked:
https://github.com/ME3Tweaks/ME3TweaksModManager/blob/backport-92/MassEffectModManagerCore/modmanager/objects/alternates/AlternateOption.cs#L313

This application of dependsOnMet() is causing the checkbox of the all grenades option to change, so when it runs on the reaper option on the next iteration of the loop, the underlying state has changed, and it triggers DependsOnNotMet() (it should trigger this state, as it is in-fact, not met). The code to prevent changing user options here is not applicable due to the first call to DependsOnMet(). Which as a side effect, is causing the UI to behave how it should, though it's due to a bug and might not actually work as intended when fixed.

Regardless, once I figure out how to handle this, the logic will be gated to 9.2 and higher to prevent breaking existing setups.

[Mgamerz]

I think this has been fixed in my local code. There was an additional issue where the state of being able to be chosen by the user or not was being automatically reset due to legacy code predating dependsonkeys (only checking for COND_MANUAL). This would internally cause the comparison code to not run as expected - as the user you wouldn't see this as it happened all at once.

In moddesc 9.2 and above the state should be preserved before checking the dependson keys for comparing. It also will now only apply the dependsonmet if dependsonnotmet was not run on the option.

Both of these fixes are not applied if the moddesc is 9.1 or lower. This means that mods updating moddesc that use the dependsonsystem will need to verify they continue to work properly.

In your demo mod posted, there is still selection oddity with being able to choose Remove all and others - but as they cannot really be mutually exclusive (which would prevent user from selecting option), or dependent on each other (may cause circular dependency or prevent user from selecting anything), I don't think there is much you can do about that, unless you pivot on hidden options.

This can be tested in the next build which I hope to publish to beta tomorrow.