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

Prompt user to delete non-empty folders after uninstall #2962

Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jan 12, 2020

Problem

  1. Install a typical mod in CKAN
  2. Run KSP
  3. The mod generates one or more files in its GameData folder (many mods do this)
  4. Uninstall the mod in CKAN

The files generated in step 3 will not be deleted, which prevents the mod's folder from being deleted. Unfortunately, this causes ModuleManager to think that the mod is still installed:

https://github.com/sarbian/ModuleManager/blob/4b7319c9da475e6d7598723151264b9de7112e06/ModuleManager/ModListGenerator.cs#L119-L129

Cause

CKAN only knows the origin of files it installed; other files might be user-created cfg files (unsafe to remove), manually installed mods (unsafe to remove), mod-generated config files (probably safe to remove), etc. We have no programmatic way of knowing what they are or whether they are safe to remove.

Changes

Now ModuleInstaller will report back to its calling code whenever it finds a non-empty directory that only contains unregistered files. What happens next is up to the UI that is performing the install/uninstall.

In GUI, if any such directories are found when upgrading or removing a module, the user will be taken to a new screen at the end of the install flow:

image

  • The big label at the top explains what the screen is and why it matters. This text could probably be improved a lot, so suggestions are welcome.
  • The left listview shows the directories that we think the user might want to delete. They have checkboxes, all of which are checked by default.
  • If the user clicks a directory in the left side, its contents appear in the right side so the user can decide whether to delete it.
  • The Open Directory button launches the selected folder in the OS's file browser, in case the user wants to do some manual maintenance such as copying settings to a backup before deleting folders.
  • The Delete Checked button closes the dialog and deletes the directories that have their checkboxes checked.
  • The Keep All button closes the dialog and doesn't delete anything.

Note that GUI applies a change set in this order:

  1. Removals
  2. Upgrades
  3. Installs

It's possible that a folder that was a candidate for deletion after step 1 or 2 might have a CKAN-installed file added to it in step 2 or 3, so we accumulate the directories that might be deletable through the whole process and then check them again at the end.

I wasn't happy with this UI at first, but then I did some user testing with someone who doesn't use CKAN or play KSP, and they seemed to understand what it was for and how it worked. I'm still interested in ideas for improvements, but I no longer feel like it needs a total overhaul to be workable.

Fixes #2841.

Side changes

The code for prompting the user to select a "provides" module was split between MainInstall.cs and MainDialogs.cs. Now it's moved to MainProvides.cs so I can find it. (This was helpful while I was trying to use it as a model for the new DeleteDirectories functionality.)

Known limitations

CmdLine and ConsoleUI could have this added in the future, since much of it is in Core, but only GUI is updated so far.

@HebaruSan HebaruSan added Bug Something is not working as intended GUI Issues affecting the interactive GUI Pull request labels Jan 12, 2020
@HebaruSan HebaruSan force-pushed the feature/remove-nonempty-dirs-prompt branch 3 times, most recently from eb2e1d1 to c9ad146 Compare January 12, 2020 18:44
@DasSkelett
Copy link
Member

The header row of the left panel doesn't show up anymore after the first time (so if you remove mods twice or more without restarting CKAN):
grafik

@HebaruSan
Copy link
Member Author

Can you please help me imagine what this UI would look like if it was designed by Google or Apple or Facebook?

@DasSkelett
Copy link
Member

DasSkelett commented Jan 13, 2020

I don't think it looks too bad.

  • The message on the top could be a bit bigger (as in font size).

  • Maybe change the message to the following:

The below directories are leftover after removing some mods. They contain files that were not installed by CKAN, but probably generated by the corresponding mod. CKAN does not automatically delete files it did not install, but you can choose to remove them if it looks safe to do so (recommended). Note that if you decide not to remove a directory, ModuleManager may incorrectly consider that an indication that that mod is still installed.

  • Maybe write something like "Please select a directory on the left to view its contents here" in the right panel when nothing is selected.

  • I would switch the Delete Checked and Keep All buttons, the Delete Checked is more of a confirm button than the Keep All. We could even go ahead and remove the Keep All button entirely, deselecting all directories does the same. Although a user who often wants to keep everything would welcome it.

  • The Open Directory button should be deactivated as long as nothing is selected.

I can't come up with something more modern that wouldn't reduce the usability. It's a fairly stable design with a message that explains what's going on, users should be able to figure out what to do.

@HebaruSan HebaruSan force-pushed the feature/remove-nonempty-dirs-prompt branch from f0b9f7e to 52946a7 Compare January 13, 2020 21:34
@HebaruSan
Copy link
Member Author

I like all of those suggestions. The latest commit should address all of them.

@HebaruSan HebaruSan force-pushed the feature/remove-nonempty-dirs-prompt branch from 52946a7 to 8610827 Compare January 13, 2020 21:48
@HebaruSan
Copy link
Member Author

It freezes on Windows (of course it does). Investigation ongoing...

@DasSkelett
Copy link
Member

Just pushed some adjustments for the German translation. You might want to do a quick pull before amending your Windows fixes and force-pushing.

@HebaruSan
Copy link
Member Author

Thanks!!

The freeze happens here.

m_TabControl.TabPages.Insert(index, m_TabPages[name]);

No exception, the m_TabPages[name] reference is valid, index is correct... :rage3:

@HebaruSan
Copy link
Member Author

I'm only able to work around the freeze by removing this:

m_TabControl.TabPages.Clear();

... which has the downside of leaving all of the tabs in the tabstrip. Then the UI looks awful, the buttons are missing and the top font is crazy:

image

How embarrassing for Microsoft that a free implementation of the framework functions so much better than the reference implementation!

@HebaruSan HebaruSan added the In progress We're still working on this label Jan 14, 2020
@HebaruSan
Copy link
Member Author

Some progress: the freeze might be caused by trying to work with the control while its parent TabPage is removed from the TabControl. In other words, .NET doesn't want me to set up my UI before displaying it. 😢

@HebaruSan
Copy link
Member Author

Latest changes fix the freeze and bad layout on Windows, and still look OK on Linux.

@HebaruSan HebaruSan removed the In progress We're still working on this label Jan 14, 2020
@DasSkelett
Copy link
Member

Uhhm, I'm missing some buttons on Windows :(
image

@HebaruSan
Copy link
Member Author

I guess I just don't understand UserControls and Anchoring. I thought I could set the initial size of the UserControl in its Designer file and then its constituent controls would be moved based on the Anchor property, but that only works on Mono; Windows applies some kind of scaling factor that moves the constituent controls outside of the UserControl.

I guess I'll have to put the buttons in a Panel and switch to Docking instead...

@HebaruSan HebaruSan added the In progress We're still working on this label Jan 15, 2020
@HebaruSan HebaruSan force-pushed the feature/remove-nonempty-dirs-prompt branch from 71a4cef to 0838ee7 Compare January 15, 2020 18:58
@HebaruSan HebaruSan removed the In progress We're still working on this label Jan 15, 2020
@HebaruSan
Copy link
Member Author

The net-core builds are saying "Error: The assembly name is invalid.", but I don't think I changed anything like that, especially not in the latest update. Build system glitch...?

@DasSkelett
Copy link
Member

Hmm, powershell ./build.ps1 --configuration=Debug_NetCore seems to work fine on my Windows machine, let me try Linux...

@DasSkelett
Copy link
Member

Linux is fine, too. Can we somehow trigger rebuilds for Travis?

@HebaruSan
Copy link
Member Author

Yeah, you can log in at the details page and click to restart the builds, but I did that and it still gave the same error.

@DasSkelett
Copy link
Member

Can't find that button :/
It's probably just Travis being Travis again then.

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.

Works fine now, even on Windows ;)
I think it's a good solution for #2841, hopefully this makes ModuleManager happy.

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 GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leaving directories for uninstalled mods makes ModuleManager think they're still installed
2 participants