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

Don't warn about incompatible DLCs, fix conflict highlighting with DLC installed #3304

Merged
merged 3 commits into from
Feb 25, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Feb 23, 2021

Problems

popup

Similarly, this can appear for a mod that was incompatible when you installed it but has since become compatible (because it's on SpaceDock and someone put in a ksp_version_min override).

Also if you have any DLC installed and you click to install two conflicting mods, they aren't highlighted red.

Cause

SQUAD didn't update the MH version number correctly (flashbacks to the extra whitespace for the other DLC in #3243). Current GameData/SquadExpansion/MakingHistory/readme.txt:

*****************************************************************************************************
      __ _______ ____     __  ___      __   _               __  ___      __ 
     / //_/ ___// __ \   /  |/  /___ _/ /__(_)___  ___  _  / / / (_)____/ / ____ 
    / ,<  \__ \/ /_/ /  / /|_/ / __ `/ //_/ / __ \/ __ `/ / /_/ / / ___/ __/ __ \/ ___/ / / /
   / /| |___/ / ____/  / /  / / /_/ / ,< / / / / / /_/ / / __  / (__ ) /_/  /_/ / /  / /_/ /
  /_/ |_/____/_/      /_/  /_/\__,_/_/|_/_/_/ /_/\__, / /_/ /_/_/____/\__/\____/_/   \__, /  
                                                /____/                              /____/ 
 
*****************************************************************************************************

Thank you for downloading the Making History Expansion for Kerbal Space Program!

Version 1.11.0

...

ChangeLog:
=================================== v1.11.1 - Requires KSP v1.11.1 ================================
+++ Bugfixes
* Fix the servicemodules shielding antennae on launch when the shrouds are turned off.
* Fix KAL being unselectable in action pane in the mission builder.
* Fix the missing Robotics icon category in the action pane.
* Fix surface attachment issues with the LV-Tx87 Bobcat.

Since the Version 1.11.0 line is what CKAN uses to detect DLC versions, CKAN thinks everyone using MH on 1.11.1 has an incompatible DLC.

When selecting conflicting rows, ModuleIsDLCKraken is thrown here, because the resolver is being told to install DLC:

CKAN/GUI/Model/ModList.cs

Lines 483 to 488 in b748670

var resolver = new RelationshipResolver(
mods_to_check,
change_set.Where(ch => ch.ChangeType == GUIModChangeType.Remove)
.Select(ch => ch.Mod),
options, registry, ksp_version
);

Changes

  • Now the incompatible modules warning excludes DLCs.
  • Now the incompatible modules warning excludes mods with compatible metadata in the available modules list but incompatible metadata in the installed modules list.
  • Now the main mod list grid conflict detector doesn't pass installed modules to the resolver, since it gets them from the registry itself anyway. This means that DLC will never be in its to-install list.

Fixes #3302. Fixes #3269.

@HebaruSan HebaruSan added Easy This is easy to fix GUI Issues affecting the interactive GUI labels Feb 23, 2021
@HebaruSan HebaruSan requested a review from DasSkelett February 23, 2021 20:56
@HebaruSan HebaruSan added the Core (ckan.dll) Issues affecting the core part of CKAN label Feb 23, 2021
@HebaruSan
Copy link
Member Author

@SliceofLie, if you want to try this out, there's a test build at the bottom of this page:

https://github.com/KSP-CKAN/CKAN/actions/runs/593969600

@HebaruSan HebaruSan changed the title Don't warn about incompatible DLCs Don't warn about incompatible DLCs, fix conflict highlighting with DLC installed Feb 24, 2021
@DasSkelett
Copy link
Member

DasSkelett commented Feb 24, 2021

Hm, why is installed even contained in modules_to_check->modulesToInstall? The RelationshipResolver already considers them when checking for conflicts, using registry.InstalledModules in the end as well:

//Need to check against installed mods and those to install.
var mods = modlist.Values.Concat(installed_modules).Where(listed_mod => listed_mod.ConflictsWith(module));

installed_modules = new HashSet<CkanModule>(registry.InstalledModules.Select(i_module => i_module.Module));

@HebaruSan
Copy link
Member Author

I suspect that our assumptions about handling of installed modules changed over time in the resolver, and ModList.ComputeConflictsFromModList wasn't updated to reflect those changes. My goal was to find the smallest change to minimize the risk of unexpectedly breaking something else, but we can try something more drastic if you think it's a good idea.

@DasSkelett
Copy link
Member

I think the impact of a slightly misbehaving ComputeConflictsFromModList would be limited.

But testing it everything seems to work as expected, conflicts with already installed modules get shown, as well as conflicts between two modules in the changeset about to be installed.
The RelationshipResolver of the actual install flow seems to work the same, with only the new modules in moduleToInstall.

So we should be fine removing installed from modules_to_check here.

@HebaruSan
Copy link
Member Author

OK, will do. There's also some unnecessary conversion from CkanModule to identifier back to CkanModule here that could cause its own problems down the line, so might as well clean that up too.

@HebaruSan
Copy link
Member Author

Done; now this looks very similar a block in MainInstall.cs, maybe we can consolidate them at some point...

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix GUI Issues affecting the interactive GUI
Projects
None yet
2 participants