Fix auto removal when installing w/ deps

[HebaruSan]

Background

If you install a mod with dependencies, by default they are marked as auto-installed, and if you later uninstall the original mod, the auto-installed dependencies are supposed to be uninstalled as well automatically, see #2753.

Problems

While investigating #1268, I noticed that sometimes my auto-removable modules would not show up in the changeset. It happened when I was both removing and installing mods, but only if the installing mods had some dependencies of their own. The auto-removable modules were left in place, and after the install finished, the Apply changes button was still enabled on the main mod list, with a changeset to remove them.

Distantly relatedly (this is the deleted TODO in ModList.cs), if you remove the only module that depends on an auto-installed mod X and also install a new module that depends on X in the same changeset, then X will be removed in the uninstall step and re-installed in the install step. It should just be left alone.

Causes

I found this bug particularly complicated and obscure, so I will try to spell it out as carefully as possible.

The auto-removal logic works by first calculating an auto-installed module's reverse dependencies within {the changeset combined with the installed modules}, i.e., the mods that depend on this mod, directly or indirectly, after the changeset is performed. Then it checks to see whether those reverse dependencies are a subset of the auto-installed mods; if so, then the module can be removed. If any user-selected mod depends on an auto-installed mod, then it is not auto-removed. This algorithm is sound (but there was a problem with the implementation; keep reading).

To calculate the reverse dependencies of a mod, we take a copy of the set of mods and remove that mod from it. Then we check whether any of the remaining mods have unsatisfied dependencies, and if they do, we remove them as well. The logic then repeats until none of the remaining mods have unsatisfied dependencies; at that point, the mods that have been removed up to that point are considered the reverse dependencies.

The unstated assumption here is that the changeset used to build the set of mods is the full changeset. Otherwise, you could have a set of mods that had unsatisfied dependencies before any mods were removed from it!

That unstated assumption was not satisfied by Registry.FindRemovableAutoInstalled. The list of mods that it used for finding reverse dependencies was the installed modules combined with the user-selected changeset, not including dependencies. This explains why installing a mod with dependencies broke the auto-remove feature: the mod with dependencies (which were always unsatisfied unless the user selected them manually) was erroneously counted as a reverse dependency of every auto-installed mod, and it wasn't auto-installed, so we determined that nothing could be auto-removed in that pass.

Changes

• Now Registry.FindRemovableAutoInstalled calculates the full changeset associated with its inputs, which it then checks for auto-removable modules
  • To do this requires RelationshipResolver, which shouldn't be used inside of Registry because Registry is supposed to be a data object that doesn't need to know the complexities of relationship resolution. (It might also create a circular reference, I didn't check that.) Luckily we already have a class that acts as a thin app logic layer around Registry, and in fact already uses RelationshipResolver: IRegistryQuerier; hence Registry.FindRemovableAutoInstalled has been moved to IRegistryQuerier.FindRemovableAutoInstalled
  • RelationshipResolver needs a GameVersionCriteria, so now IRegistryQuerier.FindRemovableAutoInstalled does as well
• Now all references to GUI.Main.Instance within GUI.Main itself are changed to this (or simply omitted, when referencing a class member)
  • GUI.Main.FetchMasterRepositoryList is no longer static because its logic depends on using an instance of Main, which is better expressed as a member function
• IEnumerable<> is nice because it's capable of holding a reference to an enumerable collection of any underlying type, but it has the pitfall of also being C#'s mechanism for implementing generators (see Eliminate duplicate network calls in Netkan #2928 and Memoize lazily evaluated sequences #2953). For example, if I set IEnumerable<string> s = someArray.Select(x => x.ToString()), and then save a reference to s or pass it to other functions, that expression might be evaluated multiple times, repeating logic and slowing down the app. Each function that uses such a value has to be written carefully to avoid this, which requires a fairly deep understanding of C#.
  The ideal use of IEnumerable<> is for a function that legitimately can/should be a generator, possibly using another generator as input, on which the calling function can then call .ToList(), .ToHashSet(), .ToDictionary(), etc., depending on which type it needs.
  Now many of our uses of IEnumerable<> for member variables and function parameters are changed to List or HashSet (or changed between those types where appropriate), to ensure that we hold a reference to a real object rather than a generator that might be lazily evaluated at some point in the future.
• GUI.MainTabControl was a class that existed just to set focus after the main tabstrip fired OnSelectedIndexChanged. This logic is now moved to Main.MainTabControl_OnSelectedIndexChanged, and the class is deleted and replaced by ThemedTabControl, its parent class.
• Now all the compiler warnings are fixed
  • Main.ManageMods_OnRefresh is renamed to Main.RefreshModList because ManageMods wasn't actually using it
• Some debug statements are added or improved because this was challenging to investigate
  (I had an issue with DLCs breaking the resolver, which I couldn't figure out because my log.Debug messages were accidentally suppressed by my log4net.xml configuration file. By the time I fixed that, I ended up with messages that more clearly illuminated the proceedings.)

[techman83]

That was a curly one, well done 🎉