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

Auto-uninstall auto-installed modules #2753

Merged
merged 1 commit into from
May 7, 2019

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 6, 2019

Motivation

Installing a module pulls in its dependencies automatically. If the user later uninstalls that module, they may wish to remove the dependencies as well, but there's no easy way to identify them.

Debian's APT package management system has a concept of "automatically installed packages," which handles this situation nicely:

To install one package, it is often necessary to install several others (to fulfill its dependencies). For instance, if you wish to install the clanbomber package, you must also install the package libclanlib2. If you remove clanbomber again, you probably no longer need the libclanlib2 package; aptitude will attempt to detect this and automatically remove the libclanlib2 package.

It works like this: when you install a package, aptitude will automatically install any other packages on which it depends. These packages are marked as having been “automatically installed”; aptitude will monitor them and remove them when they are no longer depended upon by any manually installed package [10] . They will appear in the preview as “packages being removed because they are no longer used.”

As with any automatic process, there is a potential for things to go haywire. For instance, even if a package was automatically installed to start with, it might turn out to be useful in its own right. You can cancel the “automatic” flag at any time by pressing m; if the package is already being removed, you can use Package → Install (+) to cancel the removal and clear the “automatic” flag.

https://www.debian.org/doc/manuals/aptitude/ch02s02s06.en.html

Changes

Now CKAN supports a concept of automatically installed modules.

Fixes KSP-CKAN/NetKAN#6825 (because the motivation behind that issue was cleaning up dependencies that are no longer needed).

GUI

When you install a mod:

image

image

image

image

... a new checkbox column appears called "Auto-installed":

image

This will be checked for mods that were installed due to dependencies, and unchecked for mods the user chose to install. Recommendations and suggestions are considered as user-chosen. Any modules installed before the user upgraded to this version of CKAN will be treated as user-chosen.

When you remove a mod, we check for any auto-installed modules that will no longer be needed, and automatically add them to the change set:

image

The user can toggle the checkboxes to control whether a module should be considered auto-installed:

image

If a module was previously considered auto-installed, unchecking the box will "anchor" it so that it can no longer be auto-removed. If the module was previously not considered auto-installed, checking the box will allow the module to be auto-removed; if it currently has no depending mods, it will be added to the changeset for removal immediately:

image

You can hide the new column, since it may not be useful to some users:

image

image

To make this work, a new property InstalledModule.auto_installed is added that will be saved to registry.json. I don't think this requires an update of the schema or the spec, because InstalledModule is a private object that CKAN creates and consumes internally.

To make the checkbox column work, we now only reference columns by name instead of by index.

CmdLine

CmdLine is updated to print "+" as the symbol for auto installed:

> _build\ckan list

KSP found at C:/Program Files (x86)/Steam/steamapps/common/Kerbal Space Program

KSP Version: 1.7.1

Installed Modules:

+ ContractConfigurator 1.27.1
- CustomBarnKit 1.1.19.0
- FinalFrontier 1.5.3-3465
- GPP 1:1.6.3.1
+ GPPTextures 4.2.1
- KerbalKonstructs 1.4.5.64
+ Kopernicus 2:release-1.7.0-1
A MakingHistory-DLC 1.7.1 (unmanaged)
+ ModularFlightIntegrator 1.2.6.0
+ ModuleManager 4.0.2
- Strategia 1.7.3
- WaypointManager 2.7.5

Legend: -: Up to date. +:Auto-installed. X: Incompatible. ^: Upgradable. >: Replaceable
        A: Autodetected. ?: Unknown. *: Broken.

You can mark modules as auto-installed via CmdLine:

> ckan mark auto gpp astrogator
Marking GPP as auto-installed...
Astrogator is not installed.
Changes made!

Or unmark them:

> ckan mark user gpp astrogator
Marking GPP as user-selected...
Astrogator is not installed.
Changes made!

ConsoleUI

ConsoleUI is updated along the same lines:

image

  • Auto-installed mods are represented with a floating 'a' character with an underline, which codepage 437 charmingly calls the "feminine ordinal" symbol
  • The F8 key toggles auto install
  • The help screen explains what's up:
    image

@HebaruSan HebaruSan added Enhancement GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Pull request Registry Issues affecting the registry Relationships Issues affecting depends, recommends, etc. labels May 6, 2019
@HebaruSan HebaruSan force-pushed the feature/auto-remove-auto-inst branch from 7df5118 to 398d01b Compare May 6, 2019 04:31
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.

If I remove a mod (6CSL) with auto-installed dependencies (MM), the reason for action for the auto-removed mod is wrong:
grafik

If you clear the changeset on the changeset tab (doesn't matter if there's an "auto-remove" in the changeset or not), ckan crashes:

System.NullReferenceException: Object reference not set to an instance of an object
at System.Windows.Forms.DataGridViewCellCollection.get_Item (System.String columnName) [0x0002e] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at (wrapper remoting-invoke-with-check) System.Windows.Forms.DataGridViewCellCollection.get_Item(string)
at CKAN.GUIMod.SetUpgradeChecked (System.Windows.Forms.DataGridViewRow row, System.Nullable`1[T] set_value_to) [0x00007] in <c787e8ddab794c269a4e41cd1bd477ae>:0
at CKAN.Main.ClearChangeSet () [0x00061] in <c787e8ddab794c269a4e41cd1bd477ae>:0
at CKAN.Main.CancelChangesButton_Click (System.Object sender, System.EventArgs e) [0x00001] in <c787e8ddab794c269a4e41cd1bd477ae>:0
at System.Windows.Forms.Control.OnClick (System.EventArgs e) [0x00019] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Button.OnClick (System.EventArgs e) [0x0001e] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.ButtonBase.OnMouseUp (System.Windows.Forms.MouseEventArgs mevent) [0x00069] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Button.OnMouseUp (System.Windows.Forms.MouseEventArgs mevent) [0x00000] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Control.WmLButtonUp (System.Windows.Forms.Message& m) [0x00078] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Control.WndProc (System.Windows.Forms.Message& m) [0x001b4] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.ButtonBase.WndProc (System.Windows.Forms.Message& m) [0x00037] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Button.WndProc (System.Windows.Forms.Message& m) [0x00000] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Control+ControlWindowTarget.OnMessage (System.Windows.Forms.Message& m) [0x00000] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.Control+ControlNativeWindow.WndProc (System.Windows.Forms.Message& m) [0x0000b] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0
at System.Windows.Forms.NativeWindow.WndProc (System.IntPtr hWnd, System.Windows.Forms.Msg msg, System.IntPtr wParam, System.IntPtr lParam) [0x00085] in <8aef12e05d8341c2ba53ae50a7bb2ab8>:0

GUI/GUIMod.cs Outdated Show resolved Hide resolved
@HebaruSan
Copy link
Member Author

Can you be more specific about the steps to crash? If I cancel out of uninstalling 6 Crew Science Lab, it just goes back to the mod list for me, no crash.

@DasSkelett
Copy link
Member

DasSkelett commented May 6, 2019

  1. Be on Kubuntu (may or may not be relevant), have 6CSL installed
  2. Unselect 6CSL in the modlist (so it deinstalls)
  3. Click Apply
  4. Changeset tab opens. You see two mods, MM and 6CSL.
  5. Click clear in the lower right corner.
  6. CKAN hard-crashes with the given error.

Let me try to find the exact line with some debugging.

@HebaruSan
Copy link
Member Author

The reason for the auto removal action should be fixed now.

@HebaruSan
Copy link
Member Author

As for the crash, maybe Mono doesn't support indexing a row's .Cells collection by name? There are other options...

@DasSkelett
Copy link
Member

Selecting Add available updates works, and if I'm not mistaken, that uses reference by column name too?

@HebaruSan
Copy link
Member Author

That calls into the exact same function that's crashing, so if it's behaving differently, then I have no idea.

CKAN/GUI/Main.cs

Lines 569 to 574 in df8fb70

var mod = (GUIMod)row.Tag;
if (mod.HasUpdate)
{
MarkModForUpdate(mod.Identifier);
}
}

CKAN/GUI/MainModList.cs

Lines 318 to 328 in df8fb70

public void MarkModForUpdate(string identifier)
{
Util.Invoke(this, () => _MarkModForUpdate(identifier));
}
public void _MarkModForUpdate(string identifier)
{
DataGridViewRow row = mainModList.full_list_of_mod_rows[identifier];
var mod = (GUIMod)row.Tag;
mod.SetUpgradeChecked(row, true);
}

@DasSkelett
Copy link
Member

One more thing: If you downgrade a mod, the auto-installed tag is lost.
Is it intended this way? I'm honestly not sure how it should be handled.
On one side, the user definitely does "manual", non-default changes to a mod, so he might want to manage it himself.
On the other side, it's just a simple version change, like updating a mod, that doesn't per se mean "Want to keep that mod even if the dependents are removed", and he could always unset the checkbox himself.

@HebaruSan
Copy link
Member Author

I didn't consciously consider downgrading, but it doesn't sound broken to me. When in doubt, there's a column where the user can change it.

@DasSkelett
Copy link
Member

Setting the "UpdateCol" back to 2 fixes it, yes. Confusing that it doesn't crash when coming via MarkModForUpdate()

@HebaruSan
Copy link
Member Author

Well, here's the mono code that's throwing that exception:

				if (columnName == null)
					throw new ArgumentNullException ("columnName");


				foreach (DataGridViewCell cell in base.List) {
					if (string.Compare (cell.OwningColumn.Name, columnName, true) == 0)
						return cell;
				}


				throw new ArgumentException (string.Format (
					"Column name {0} cannot be found.",
					columnName), "columnName");

https://github.com/mono/mono/blob/a41be288719e56df153158faba0ad3c27af2e896/mcs/class/System.Windows.Forms/System.Windows.Forms/DataGridViewCellCollection.cs#L67-L77

Looks like an NRE would mean that either cell or cell.OwningColumn is null. I'm going to switch it to a safer version of numeric indices, since that just uses a regular array lookup.

https://github.com/mono/mono/blob/a41be288719e56df153158faba0ad3c27af2e896/mcs/class/System.Windows.Forms/System.Windows.Forms/DataGridViewCellCollection.cs#L45-L58

@DasSkelett
Copy link
Member

Yep works!
Last thing, probably for a future PR:
Would be nice to be able to change the auto-installed flag at least in the command line UI too, and also consoleUI wouldn't be that bad I think.

@HebaruSan
Copy link
Member Author

Yeah, that's needed of course. I'm not sure how it should look, though. I guess another status symbol representing auto installed? And then a command / keystroke to toggle.

@DasSkelett
Copy link
Member

The toggle option / keystroke could be "hidden" inside the details view, to avoid cluttering the "main" modlist view.
For the symbol, well, a simple 'a'? Or an '@' if it should look a bit fancy?

@HebaruSan
Copy link
Member Author

For the symbol, well, a simple 'a'? Or an '@' if it should look a bit fancy?

I'm thinking it might be nice to adopt the same symbol in both CmdLine and ConsoleUI, and CmdLine already uses "A" for manually installed mods. So "a" might be a bit confusing.

@HebaruSan
Copy link
Member Author

HebaruSan commented May 6, 2019

The other wrinkle is that unlike the existing status symbols in both UIs, this one would need to work alongside the existing ones, since an auto installed mod can also be upgradeable, broken, etc. So e.g. if we used "@", then CmdLine would show an upgradeable auto installed mod as "^@", a normal auto installed mod as "-@", etc.

EDIT: Or, maybe treating it as an alternate form of "-" is sufficient. If a mod is upgradeable, do I really care that it's also auto installed?

@HebaruSan
Copy link
Member Author

@DasSkelett, I'm going to add the CmdLine and ConsoleUI changes here. Would you mind un-approving so we can track the review of those changes?

@DasSkelett DasSkelett requested review from DasSkelett and removed request for DasSkelett May 7, 2019 05:44
@DasSkelett
Copy link
Member

Lemme figure out how to do that....

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.

Can't remove my previous approval, but I can approve the new commits!
Work fine.

@politas
Copy link
Member

politas commented May 8, 2019

Oooh, excellent! I've been meaning to raise an issue for this.

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 GUI Issues affecting the interactive GUI Registry Issues affecting the registry Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Suggestion] Highlight dependents or dependencies on click
4 participants