Fixes for GUI and .ckan-installed modules

[DasSkelett]

Problems:

GUI

• If you cancel an installation too late and can't be stopped anymore and finishes successfully, the changeset doesn't get cleared.
• Clearing the changeset on the changeset tab doesn't remove incompatible mods checked using the AllModVersiions tab. Neither does it remove mods queued for removal because they've been checked as auto-installed without any dependent mods.
• A Discord user from the r/KerbalSpaceProgram server who tends to find every bug and glitch in CKAN, likes to double-click the "Accept" button in the changeset tab. This throws an exception:

System.InvalidOperationException: This BackgroundWorker is currently busy and cannot run multiple tasks concurrently.
   at System.ComponentModel.BackgroundWorker.RunWorkerAsync(Object argument)
   at CKAN.Changeset.ConfirmChangesButton_Click(Object sender, EventArgs e)
   at System.Windows.Forms.Control.OnClick(EventArgs e)
   at System.Windows.Forms.Button.OnClick(EventArgs e)
   at System.Windows.Forms.Button.OnMouseUp(MouseEventArgs mevent)
   at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks)
   at System.Windows.Forms.Control.WndProc(Message& m)
   at System.Windows.Forms.ButtonBase.WndProc(Message& m)
   at System.Windows.Forms.Button.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

• If you install a mod from a .ckan file which isn't known to the registry (identifier-wise), the "Versions" tab doesn't update when you select it
• Furthermore, for those mods the dependencies tab always has the new STOP sign from Visual cues for incompatibility reasons #3271
• If you downsize the GUI, some buttons of the menu strip disappear if they don't fit in the window.

ConsoleUI

• When loading a new instance the modlist is empty at first (or only contains the DLCs). This tends to confuse users. The GUI does automatically refresh the registry in this case.
• If you launch the un-repacked CKAN-ConsoleUI.exe directly (e.g. via IDE), you get an exception because the themename passed to ConsoleCKAN() is null.

Core

• CKAN offers to upgrade modules even if it would violate another module's relationships (but the installation fortunately fails); related it shows all versions in the VersionsListView as installable.
• While downloading a huge set of mods I got a NullReferenceException in NetAsyncDownloader.FileProgressReport(), for the t variable in the queuedDownloads foreach-loop. Unfortunately KDE crashed some time later and I didn't save the stacktrace to disk yet.

Causes:

GUI

• We don't run UpdateModsList after the install process has been cancelled, thus the changeset doesn't get cleared. We can't run it immediately, because it would close the WaitTabPage and remove the useful log messages.

• ClearChangeSet() doesn't detect changes for incompatible mods since IsInstallChecked is false for them. GetModChanges() find it by comparing GUIMod.SelectedMod with GUIMod.InstalledMod.Module.

• And it doesn't know about the auto-installed modules because toggling the chexkbox isn't an action itself that gets added to the changeset – it's applied instantly by setting InstalledMod.AutoInstalled.

• If you manage to hit the "Accept" button again before the GUI switches to the WaitTabPage, it tries to call installWorker.RunWorkerAsync() a second time, throwing the exception.

• AllModVersions uses registry.AvailableByIdentifier(value.Identifier) to populate the VersionListView. If the registry can't find anything, we just return, even without clearing the existing version items from the previous mod.

• GUIMod.IsIncompatible is always true for unknown modules because registry.LatestAvailable() can't find anything.

• By default, menu items are not displayed if they cannot fit within the available space.

  https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.menustrip.canoverflow?view=net-5.0#System_Windows_Forms_MenuStrip_CanOverflow

Core

• We don't tell the RelationshipResolver the module version that gets removed, only the new one to be installed. Thus it considers dependencies still satisfied by the old version.
• I guess if a new download gets picked out of the queue while we're in the loop adding all the sizes together, the DownloadTarget can become null.

Changes

GUI

• Add OK button to wait tab page; it is enabled if the install process failed or the user cancelled too late. It gives time to read the log messages, but ensures UpdateModsList() runs at some point. The tabController stays locked and the top menu deactivated until the user hits "OK". This means we also need to unlock the tabController when cancelling earlier in the installation process like when selecting the recommendations/suggestions or provides, because we never show the FailWaitDialog in this case.
• ClearChangeSet() also compares SelectedMod and InstalledMod now, and does the reverse of GetModChanges(), i.e. setting SelectedMod back to InstalledMod if they differ.
• Additionally, it deduces the previous state of now-marked-as-auto-installed modules by checking if they're in the changeset with SelectionReason.NoLongerUsed, and if so, sets it to false again. Doesn't catch all mods whose checkbox got changed, but it should suffice for most cases, to clear the changeset.
• If installWorker is already running, don't run it a second time. This was easier than trying to disable the button and finding all the right places where we'd need to enable it aǵain.
• If the registry doesn't know a mod, we still add the CkanModule associated with the GUIModule to the list, if it exists.
• The GUIMod constructor that takes an InstalledModule as arg now overwrites IsIncompatible based on the compatibility of the installed module.
• Make menuStrip2 overflowable. Instead of just not drawing the buttons, there's now a small dropdown to list those that didn't fit anymore. Doesn't work on Mono (it behaves just like before), but on Windows:
  

ConsoleUI

• Update the registry if the loaded instance doesn't contain any modules, just like the GUI does. Should make initial setup easier for new users.
• If the themeName passed to ConsoleCKAN is null it falls pack to default.

Core

• The two RelationshipResolver instantiations in ModuleInstaller.CanInstall() and IRegistryQuerierHelper.HasUpdate() now pass the currently installed module (if any) as moduleToRemove, just like the resolver for the actual install process does.
• Do a quick null check in the two foreach-loops in NetAsyncDownloader.FileProgressReport() and skip if t is null.

[HebaruSan]

Heh, you really love your custom .ckan files, don't you? 😆
Good thing, as I practically never use them, so I'll miss issues related to them.

[HebaruSan]

(Wow; the example code in the documentation of IsBusy, instead of noting that it may be needed to avoid the world's dumbest System.InvalidOperationException exception, instead shows how to waste CPU cycles with a tight polling loop!)

[DasSkelett]

Heh, you really love your custom .ckan files, don't you? laughing
Good thing, as I practically never use them, so I'll miss issues related to them.

Guess so 😄
I also use them when indexing new modules to make sure they get installed correctly, all dependencies work out, give a second look to make sure I didn't forget anything and so on.
And it's useful to have a dummy module sitting around which you can quickly edit to simulate some relationship edge cases or whatever, comes in handy every now and then.

[HebaruSan]

thus the changeset doesn't get cleared

I noticed that the change set clearing logic also doesn't work for forced installs of incompatible modules, since ManageMods.ClearChangeSet is implemented in terms of the install column checkboxes, which are "-" for those rows, but every time I try to solve this I experience a GUI-related mind fog. Do you see a fix?

[DasSkelett]

I noticed that the change set clearing logic also doesn't work for forced installs of incompatible modules, since ManageMods.ClearChangeSet is implemented in terms of the install column checkboxes, which are "-" for those rows, but every time I try to solve this I experience a GUI-related mind fog. Do you see a fix?

I might have found a way to achieve this. ClearChangeSet is now comparing GUIMod.SelectedMod and GUIMod.InstalledMod, and if they differ it set the former to the later.

• If an incompatible mod has been marked for installation, SelectedMod is not null whereas InstalledMod still is. Setting SelectedMod to null again removes it from the changeset again and unchecks the entry in AllModVersions ( SelectedMod -> OnPropertyChanged()->visibleGuiModule_PropertyChanged()->UpdateSelection()`)
• Up/-downgrading an already force-installed module will also reset SelectedMod to the installed module.
• Unchecking an already force-installed module is handled correctly by the existing if case (though in theory the new else case should handle it just fine as well).

I also gave modules queued for removal because they have been marked as auto installed a stab. I've seen users checking this checkbox without knowing what it does, then despairing because CKAN wants to remove their mods.
Now if the changeset contains the mod with SelectionReason.NoLongerUsed(), we uncheck the box again if the changeset gets cleared. It doesn't necessarily reset it for all mods, if toggling it for a previous mod already removed it from the changeset.

This doesn't catch modules that still have dependent mods, but it's impossible to know their previous state. And they shouldn't be much of a problem anyway, since they aren't about to be removed.

[HebaruSan]

Nice! Good stuff all around.

[DasSkelett]

This one broke upgrading AD mods, InstalledModule() is null for those, thus we get an exception which caught below and returns false.

The easy fix is to pass no array if it's a DLL. However since DLLs are considered to satisfy all dependencies, it's still possible that an upgrade would fail if the new version violated a versioned dependency.

The cleaner way would be to track installed DLLs separately in RelationshipResolver, and give the constructor a new option to tell the DLLs that are about to removed.

[HebaruSan]

This one broke upgrading AD mods

Maybe we need some tests for that. It seems a bit brittle at the moment.

[HebaruSan]

By pure luck, adding a ? also works, because RelationshipResolver is conservative with nulls:

                    new CkanModule[] { querier.InstalledModule(identifier)?.Module },

The cleaner way would be to track installed DLLs separately in RelationshipResolver, and give the constructor a new option to tell the DLLs that are about to removed.

I wonder whether AD mods should have their own InstalledModule, possibly with a CkanModule with "kind": "dll"?

[DasSkelett]

I wonder whether AD mods should have their own InstalledModule, possibly with a CkanModule with "kind": "dll"?

Interesting idea. I'm a bit anxious that things might break if they expect InstalledModule() to only return CKAN-managed modules. And if InstalledModule() returns modules that aren't in InstalledModules, more things might get confused (or would we also include DLLs in InstalledModules?)
And remembering all the bugs we had to fix because we didn't filter out "kind": "dlc"...

[HebaruSan]

And remembering all the bugs we had to fix because we didn't filter out "kind": "dlc"...

Yes, it's definitely a high risk change. For now, I think I'm going to try writing that test and doing the easy fix.

[DasSkelett]

And I'm currently trying out tracking DLLs separately in the RelationshipResolver, so far it brought up some more places where we missed to pass all the data we should've to the resolver.
But since it basically includes the easy fix anyway, I think it's good that you do it as a separate PR, especially if it contains some tests that help catching any problems with more complex changes.

[HebaruSan]

especially if it contains some tests that help catching any problems with more complex changes

Heh, I was just going to do the one simple test of HasUpdate with an AD mod, but now I'm looking for a few more...

• CKAN offers to upgrade modules even if it would violate another module's relationships

Do you remember which relationships these were? Did another installed module have a depends with version?

[DasSkelett]

Do you remember which relationships these were? Did another installed module have a depends with version?

Yup, BeyondHome with Parallax: KSP-CKAN/NetKAN#8378
And this one is also interesting since I've read from a lot of people having Parallax installed manually, because for all they knew the installation via CKAN was broken, and sometimes because they didn't know of the downgrade functionality. So that got me into the whole AD and upgrade with unsatisfied dependency rabbit hole.