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.

Sign up for GitHub

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

Fixes for GUI and .ckan-installed modules #3307

Merged
merged 3 commits into from
Mar 5, 2021
Merged

Fixes for GUI and .ckan-installed modules #3307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
667bb41
Fixes for .ckan-installed mods and other GUI fixes
DasSkelett Mar 5, 2021
38b9cf3
Don't offer mod upgrade if it would violate another mod's dependencies
DasSkelett Mar 2, 2021
91c2071
Refresh registry when loading an instance with empty registry in Cons…
DasSkelett Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ConsoleUI/ConsoleCKAN.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public class ConsoleCKAN {
/// </summary>
public ConsoleCKAN(GameInstanceManager mgr, string themeName, bool debug)
{
if (ConsoleTheme.Themes.TryGetValue(themeName, out ConsoleTheme theme))
if (ConsoleTheme.Themes.TryGetValue(themeName ?? "default", out ConsoleTheme theme))
{
// GameInstanceManager only uses its IUser object to construct game instance objects,
// which only use it to inform the user about the creation of the CKAN/ folder.
Expand All @@ -40,7 +40,7 @@ public ConsoleCKAN(GameInstanceManager mgr, string themeName, bool debug)
}
}
if (manager.CurrentInstance != null) {
new ModListScreen(manager, debug).Run(theme);
new ModListScreen(manager, debug, theme).Run(theme);
}

new ExitScreen().Run(theme);
Expand Down
42 changes: 24 additions & 18 deletions ConsoleUI/ModListScreen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ public class ModListScreen : ConsoleScreen {
/// </summary>
/// <param name="mgr">Game instance manager object containing the current instance</param>
/// <param name="dbg">True if debug options should be available, false otherwise</param>
public ModListScreen(GameInstanceManager mgr, bool dbg)
public ModListScreen(GameInstanceManager mgr, bool dbg, ConsoleTheme _theme)
{
debug = dbg;
manager = mgr;
registry = RegistryManager.Instance(manager.CurrentInstance).registry;

moduleList = new ConsoleListBox<CkanModule>(
1, 4, -1, -2,
GetAllMods(),
GetAllMods(_theme),
new List<ConsoleListBoxColumn<CkanModule>>() {
new ConsoleListBoxColumn<CkanModule>() {
Header = "",
Expand Down Expand Up @@ -205,7 +205,7 @@ public ModListScreen(GameInstanceManager mgr, bool dbg)
}
return true;
});

moduleList.AddTip("F8", "Mark auto-installed",
() => moduleList.Selection != null && !moduleList.Selection.IsDLC
&& (!registry.InstalledModule(moduleList.Selection.identifier)?.AutoInstalled ?? false)
Expand Down Expand Up @@ -265,7 +265,7 @@ public ModListScreen(GameInstanceManager mgr, bool dbg)
null,
new ConsoleMenuOption("Refresh mod list", "F5, Ctrl+R",
"Refresh the list of mods",
true, UpdateRegistry),
true, (ConsoleTheme th) => UpdateRegistry(th)),
new ConsoleMenuOption("Upgrade all", "Ctrl+U",
"Mark all available updates for installation",
true, UpgradeAll, null, null, HasAnyUpgradeable()),
Expand Down Expand Up @@ -328,7 +328,7 @@ protected override string CenterHeader()
private bool ImportDownloads(ConsoleTheme theme)
{
DownloadImportDialog.ImportDownloads(theme, manager.CurrentInstance, manager.Cache, plan);
RefreshList();
RefreshList(theme);
return true;
}

Expand Down Expand Up @@ -393,7 +393,7 @@ private bool ViewSuggestions(ConsoleTheme theme)
}
}
if (needRefresh) {
RefreshList();
RefreshList(theme);
}
} else {
RaiseError("Installed mods have no unsatisfied recommendations or suggestions.");
Expand All @@ -414,7 +414,7 @@ private int daysSinceUpdated(string filename)
return (DateTime.Now - File.GetLastWriteTime(filename)).Days;
}

private bool UpdateRegistry(ConsoleTheme theme)
private bool UpdateRegistry(ConsoleTheme theme, bool showNewModsPrompt = true)
{
ProgressScreen ps = new ProgressScreen("Updating Registry", "Checking for updates");
LaunchSubScreen(theme, ps, (ConsoleTheme th) => {
Expand Down Expand Up @@ -447,11 +447,11 @@ private bool UpdateRegistry(ConsoleTheme theme)
}
}
});
if (recent.Count > 0 && RaiseYesNoDialog(newModPrompt(recent.Count))) {
if (showNewModsPrompt && recent.Count > 0 && RaiseYesNoDialog(newModPrompt(recent.Count))) {
searchBox.Clear();
moduleList.FilterString = searchBox.Value = "~n";
}
RefreshList();
RefreshList(theme);
return true;
}

Expand Down Expand Up @@ -481,7 +481,7 @@ private bool SelectInstall(ConsoleTheme theme)
if (!prevInst.Equals(manager.CurrentInstance)) {
plan.Reset();
registry = RegistryManager.Instance(manager.CurrentInstance).registry;
RefreshList();
RefreshList(theme);
}
return true;
}
Expand All @@ -492,17 +492,23 @@ private bool EditAuthTokens(ConsoleTheme theme)
return true;
}

private void RefreshList()
private void RefreshList(ConsoleTheme theme)
{
moduleList.SetData(GetAllMods(true));
// In the constructor this is called while moduleList is being populated, just do nothing in this case.
// ModListScreen -> moduleList = (GetAllMods ...) -> UpdateRegistry -> RefreshList
moduleList?.SetData(GetAllMods(theme,true));
}

private List<CkanModule> allMods = null;

private List<CkanModule> GetAllMods(bool force = false)
private List<CkanModule> GetAllMods(ConsoleTheme theme, bool force = false)
{
ScanForMods();
if (allMods == null || force) {
if (!registry?.HasAnyAvailable() ?? false)
{
UpdateRegistry(theme, false);
}
allMods = new List<CkanModule>(registry.CompatibleModules(manager.CurrentInstance.VersionCriteria()));
foreach (InstalledModule im in registry.InstalledModules) {
CkanModule m = null;
Expand Down Expand Up @@ -546,7 +552,7 @@ private bool Help(ConsoleTheme theme)
private bool ApplyChanges(ConsoleTheme theme)
{
LaunchSubScreen(theme, new InstallScreen(manager, plan, debug));
RefreshList();
RefreshList(theme);
return true;
}

Expand Down Expand Up @@ -598,9 +604,9 @@ private long totalInstalledDownloadSize()
return total;
}

private GameInstanceManager manager;
private IRegistryQuerier registry;
private bool debug;
private GameInstanceManager manager;
private Registry registry;
private bool debug;

private ConsoleField searchBox;
private ConsoleListBox<CkanModule> moduleList;
Expand Down Expand Up @@ -839,7 +845,7 @@ public enum InstallStatus {
/// This mod is installed and not upgradeable or planned to be removed
/// </summary>
Installed,

/// <summary>
/// Like Installed, but can be auto-removed if depending mods are removed
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion Core/ModuleInstaller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1347,7 +1347,7 @@ IRegistryQuerier registry
{
RelationshipResolver resolver = new RelationshipResolver(
toInstall,
null,
toInstall.Select(m => registry.InstalledModule(m.identifier)?.Module).Where(m => m != null),
opts, registry, ksp.VersionCriteria()
);

Expand Down
5 changes: 5 additions & 0 deletions Core/Net/NetAsyncDownloader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,8 @@ private void FileProgressReport(int index, int percent, long bytesDownloaded, lo

foreach (NetAsyncDownloaderDownloadPart t in downloads.ToList())
{
if (t == null)
continue;
if (t.bytesLeft > 0)
{
totalBytesPerSecond += t.bytesPerSecond;
Expand All @@ -366,6 +368,9 @@ private void FileProgressReport(int index, int percent, long bytesDownloaded, lo
}
foreach (Net.DownloadTarget t in queuedDownloads.ToList())
{
// Somehow managed to get a NullRef for t here
if (t == null)
continue;
totalBytesLeft += t.size;
totalSize += t.size;
}
Expand Down
2 changes: 1 addition & 1 deletion Core/Registry/IRegistryQuerier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public static bool HasUpdate(this IRegistryQuerier querier, string identifier, G
{
RelationshipResolver resolver = new RelationshipResolver(
new CkanModule[] { newest_version },
null,
new CkanModule[] { querier.InstalledModule(identifier).Module },
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This one broke upgrading AD mods

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

Copy link
Member

Choose a reason for hiding this comment

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

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"?

Copy link
Member Author

Choose a reason for hiding this comment

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

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"...

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@DasSkelett DasSkelett Mar 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

new RelationshipResolverOptions()
{
with_recommends = false,
Expand Down
6 changes: 3 additions & 3 deletions Core/Relationships/RelationshipResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,9 @@ private void AddModulesToInstall(IEnumerable<CkanModule> modules)
{
log.DebugFormat("Preparing to resolve relationships for {0} {1}", module.identifier, module.version);

//Need to check against installed mods and those to install.
var mods = modlist.Values.Concat(installed_modules).Where(listed_mod => listed_mod.ConflictsWith(module));
foreach (CkanModule listed_mod in mods)
// Need to check against installed mods and those to install.
var conflictingModules = modlist.Values.Concat(installed_modules).Where(listed_mod => listed_mod.ConflictsWith(module));
foreach (CkanModule listed_mod in conflictingModules)
{
if (options.proceed_with_inconsistencies)
{
Expand Down
21 changes: 19 additions & 2 deletions GUI/Controls/AllModVersions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ public GUIMod SelectedModule
visibleGuiModule.PropertyChanged += visibleGuiModule_PropertyChanged;
}
}
VersionsListView.Items.Clear();
if (value == null)
{
return;
}

// Get all the data; can put this in bg if slow
GameInstance currentInstance = Main.Instance.Manager.CurrentInstance;
Expand All @@ -139,16 +144,28 @@ public GUIMod SelectedModule
}
catch (ModuleNotFoundKraken)
{
// No versions to be shown, abort and hope an auto refresh happens
// Identifier unknown to registry, maybe installed from local .ckan
allAvailableVersions = new Dictionary<CkanModule, bool>();
}

// Take the module associated with GUIMod, if any, and append it to the list if it's not already there.
var installedModule = value.InstalledMod?.Module;
if (installedModule != null && !allAvailableVersions.ContainsKey(installedModule))
{
allAvailableVersions.Add(installedModule, installable(installer, installedModule, registry));
}

if (!allAvailableVersions.Any())
{
return;
}

ModuleVersion installedVersion = registry.InstalledVersion(value.Identifier);

// Update UI; must be in fg
ignoreItemCheck = true;
bool latestCompatibleVersionAlreadyFound = false;

VersionsListView.Items.Clear();
// Only show checkboxes for non-DLC modules
VersionsListView.CheckBoxes = !value.ToModule().IsDLC;

Expand Down
12 changes: 11 additions & 1 deletion GUI/Controls/ManageMods.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions GUI/Controls/ManageMods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,8 +786,21 @@ public void ClearChangeSet()
{
mod.SetInstallChecked(row, Installed, mod.IsInstalled);
}
else if (mod.SelectedMod != mod.InstalledMod?.Module)
{
mod.SelectedMod = mod.InstalledMod?.Module;
}
mod.SetUpgradeChecked(row, UpdateCol, false);
mod.SetReplaceChecked(row, ReplaceCol, false);
// Marking a mod as AutoInstalled can immediately queue it for removal if there is no dependent mod.
// Reset the state of the AutoInstalled checkbox for these by deducing it from the changeset.
if (mod.InstalledMod != null &&
ChangeSet.Contains(new ModChange(mod.InstalledMod?.Module, GUIModChangeType.Remove,
new SelectionReason.NoLongerUsed()))
)
{
mod.SetAutoInstallChecked(row, AutoInstalled, false);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion GUI/Controls/ModInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ private void UpdateModInfo(GUIMod gui_module)
Util.Invoke(MetadataModuleAuthorTextBox, () => MetadataModuleAuthorTextBox.Text = gui_module.Authors);
Util.Invoke(MetadataIdentifierTextBox, () => MetadataIdentifierTextBox.Text = module.identifier);

Util.Invoke(MetadataModuleReleaseStatusTextBox, () =>
Util.Invoke(MetadataModuleReleaseStatusTextBox, () =>
{
if (module.release_status == null)
{
Expand Down
Loading