Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly clear AD upgrades from changeset #4037

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Feb 17, 2024

Problems

  • If you choose to upgrade an AD mod and then use the install-all checkbox to clear the changeset, the AD upgrade will only be fully removed if the mod matches the current search filters (and is therefore visible or could be made visible with scrolling). Otherwise the row's checkbox will be cleared but the install of the mod will remain in the changeset.
  • If you choose to upgrade an AD mod to have CKAN take ownership of it, the changeset removal icon from Ability to clear auto-installed flag from changeset tab #4033 will appear but not work for that entry.
  • If you use the Versions tab to replace one version of a mod with a different version, the changeset tab will say "Currently installed" for the new instead of the old version, and the changeset removal icon from Ability to clear auto-installed flag from changeset tab #4033 won't appear for that entry.
  • The install-all checkbox at the upper left above the Install column is supposed to uninstall all mods when unchecked, leaving the user with a clean empty install, but it doesn't void pending upgrades of AD mods, even though they would cause files to be installed rather than removed.

Causes

  • When an upgrading AD row is visible, GUIMod.SetUpgradeChecked is called twice, once to clear the checkbox and once to react to the changing of the checkbox. (I hate this and desperately want to change it, but I don't think I would be able to account for all the complexities well enough to avoid breaking something.) The first call sets GUIMod.SelectedMod (the property that drives the changeset) to its current value again, which is incorrect, and the second call sets it to null, which is correct. (There are complex interactions with the Versions tab here in order to ensure that switching to the latest version of a mod is always equivalent to upgrading and vice versa.)
    When such a row is invisible, the second call does not occur because ManageMods.ModGrid_CellValueChanged is not called by the grid.
  • An upgrade of an AD mod uses the upgrade checkbox in the grid, but internally it is represented as an Install action because there's no previous version to remove, so the changeset removal functionality was only clearing the Install checkbox, not Upgrade.
  • RelationshipResolver.reasons was using NameComparer, which makes it match keys only based on identifier and therefore could return the wrong reasons for a module.
  • When the install-all checkbox code was written, it wasn't possible to upgrade a mod that wasn't installed, so it didn't consider that case.

Changes

  • Now the first call to GUIMod.SetUpgradeChecked sets GUIMod.SelectedMod to null, properly removing the AD upgrade from the changeset.
  • Now ManageMods.RemoveChangesetItem unchecks the Upgrade checkbox for an Install action of an AD mod, so you can remove these actions from a changeset.
  • Now RelationshipResolver.reasons uses the normal comparer, which uses both identifier and version, so the right reason will be used for each version.
  • Now the install-all checkbox also unchecks the upgrade checkbox for AD mods.
  • Now the existing incipient ManageMods.freezeChangeSet mechanism to skip redundant refreshes of the changeset tab is refactored into a WithFrozenChangeset function, which is now used by the following to improve response times:
    • The Update all button
    • The Install-all checkbox in the upper left above the Install column
    • The Clear button on the progress tab (which also no longer separately calls UpdateChangesDialog)

Fixes #4035.

@HebaruSan HebaruSan added Bug Something is not working as intended GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN labels Feb 17, 2024
@HebaruSan HebaruSan merged commit e8cb3de into KSP-CKAN:master Feb 17, 2024
8 checks passed
@HebaruSan HebaruSan deleted the fix/clear-ad-upgr-changeset branch February 17, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN GUI Issues affecting the interactive GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clearing the changeset after selecting "update all" does not work when filters/search are active
1 participant