-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
Use shared installer code in GUI and fix reinstall problems #2233
Conversation
In my Replaced_by branch, I want to move from the "Upgrade" code to my "Replace" code, so that the same technique is used for both, which can then also be used for "Reinstall" and "Switch to version [x]". Ultimately, we may be able to fold the "install" and "remove" into the same method. My idea is that you pass a "Change" object, which consists of two CkanModules - ChangeFrom and ChangeTo. If ChangeFrom is null, just install ChangeTo. If ChangeTo is null, just uninstall ChangeFrom. If neither is null, Uninstall ChangeFrom and install ChangeTo. The really tricky part is cleaning up unneeded dependencies. We don't currently maintain a "reason for installation" in our registry, which I think we need. Any module that is installed to fill a dependency, but now has nothing depending on it, should be flagged to the user for possible removal. |
I fear this has been broken by #2202, so I'm re-running Travis' checks I get these errors:
|
13792f1
to
4fa0dc3
Compare
Nice catch; I thought this might conflict with some of my other changes, but I didn't think of that one. I did notice one problem with reinstall (both versions of it): if the remove step removes a dependent mod, the install step doesn't reinstall it. So for example if I install a bunch of stuff and then reinstall ModuleManager, many things will be missing afterwards. That will probably annoy users. I'll submit an issue for it unless I can think of a simple fix. |
OK, I think that last commit covers the missing dependent mods problem. It will now uninstall the selected mod and any mods that depend on it, as normal, and then re-install all of them. The one remaining wrinkle is that it'll also prompt you for suggestions and recommendations from those mods. I think that's better than users having mods removed unexpectedly though. |
Hmm. I installed All Y'all, which installed ModuleManager. I reinstalled ModuleManager, which removed and then installed both All Y'all and ModuleManager. When that finished, the Installed checkbox for ModuleManager was no longer checked. If I tick the checkbox, it doesn't come up as a change to apply |
Good catch! |
Install messages in
ModuleInstaller.InstallList
The same changes are visible in ConsoleUI and GUI.
Code style/refactoring
In addition, some improvements are made to the organization of the code. This should have no visible impact.
MainInstaller
removedModuleInstaller
(modeled after similar function inConsoleUI
)ModuleInstaller.ResolveModules
andModuleInstaller.EnsureCache
removed because they were only used by the removed redundant GUI codeModuleInstaller.Upgrade
functions acceptIDownloader
reference instead ofNetAsyncModulesDodwnloader
, to make it easier to create alternate downloaders in the futureUser-Agent
header properly capitalized