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

[Jognt]

When CKAN uninstalls a mod, it won't touch the mod folder itself if it still contains files. This can cause problems with ModuleManager since it'll consider that mod present.

I understand the rationale, and suggest renaming the mod folder to something that indicates the modName while still distinguishable from an installed version. Or move the whole folder elsewhere. That way any files/settings stay present for future use, but ModuleManager won't consider modName as being present.

[politas]

There are two reasons for not deleting files that CKAN installs.

1. Files remaining in a folder may belong to mods that were installed manually, so deleting them would break that mod. Renaming the folder would have the same effect as deleting the file as far as this reason.
2. Mods may create a config file according to user input. CKAN upgrades a mod by uninstalling the current version and then installing the new version, so leaving the config file behind maintains those configs. Renaming the folder and restoring it on re-install would be effective in this case.

Should we change our policy and possibly break manually installed mods because ModuleManager cannot properly detect an installed mod?

[HebaruSan]

ModuleManager cannot properly detect an installed mod

To be fair, nobody can do that. CKAN's method of scanning for DLLs is just as janky (and completely ineffective for mods that don't ship DLLs).

But this suggests the idea of CKAN → MM integration. We could dump a list of installed mods, or MM could parse CKAN/registry.json, and then the :[NEEDS] logic could use that instead of the folders in GameData. I wonder how many commonly checked-for mods have identifiers that match their folders...

[politas]

It's a great idea, though wouldn't catch manually installed mods, so MM would still need to go searching for other installed mods and find the empty directories.

[HebaruSan]

though wouldn't catch manually installed mods

Wouldn't it? What stops us from including the "AD" mods in the list?

[linuxgurugamer]

What about proving an option when deleting a mod, to delete the entire mod's directory?

This can be important because MM patches look for a directory existing, even if it's empty it can then assume that a particular mod is installed even when it isn't

[HebaruSan]

What about mods that have multiple directories?
What about mods that install into another mod's directory?
What about mods that install straight into GameData?
What about mods that install into a "deep" hierarchy, like Modder/ModFamily/Mod?

[Jognt]

Hmm. Yes it is indeed not as straightforward as I initially thought it would be.

Regardless, I think it would be nice if CKAN and MM had a way to agree on what is actually ‘installed’, preferably without relying on mods adding something as that would only ‘solve’ for new mods.

Maybe in the meantime a message/listing of CKAN-uninstalled mods whose folders were not removed? I’m currently manually checking for remaining folders after uninstall, even a slight reminder could go a long way to letting the end-user figure it out.

[politas]

Could we grab a copy of ModuleManager's code for detecting installed mods and use it to compare to our list and add a new MMD tag like our AD tag?

And then maybe a right-click Open Folder option for both AD and MMD mods so users can review the files that triggered the discovery.

[HebaruSan]

I had an idea about this. Instead of basing MM's logic on directories, mods could register themselves in some well-known cfg object that exists solely for this purpose:

INSTALLEDMODULES
{
    MODULE
    {
        name = Kopernicus // for example
    }
}

Mods could patch this object from their own cfg files like so:

@INSTALLEDMODULES
{
    MODULE
    {
        name = Astrogator
    }
}

That way it would be up to the mods themselves to declare what is installed, independent of their file structure, using a syntax that's already well understood by modders, and removing the cfg file would remove its node from the list.

Then instead of the directory-based logic, you could make individual MM patches depend on the presence or absence of those nodes:

PART:NEEDS[@INSTALLEDMODULES[MODULE[Astrogator]]]
{
    // Changes to make when Astrogator is installed
}

This last part is the only thing I think would require a change to ModuleManager; as far as I know there isn't a syntax for checking for an object in the global namespace in a filter clause.

[HebaruSan]

Looking at implementation options, it looks like the :FOR[] syntax adds the given mod to the installed mods list:

https://github.com/sarbian/ModuleManager/blob/4b7319c9da475e6d7598723151264b9de7112e06/ModuleManager/ModListGenerator.cs#L95-L116

So it's already possible for mods to self-register:

ASTROGATOR:FOR[Astrogator]
{
    // Does it need code?
}

If all relevant mods had such a clause, the problem could be solved simply by deleting the directory-scanning logic:

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

[Jognt]

Yeah I thought about such an option before as well, though not as in-depth as you have.
I’m not really a fan of removing the MM legacy pass and/or removing the folder scanning since many mods either haven’t updated to specify a FOR or outright refused to use it.
It would break a great amount of mods that rely on the legacy pass/folder scanning being present, both old and new.

Combine that with how widespread knowledge of how MM currently works is and I think it would be a nightmare to inform angry people why their workflow has to change.

Since the problem is folders being left after uninstalling a mod via CKAN and the cause is CKAN not wanting to ditch user-settings: How about moving non-empty uninstalled mods into a different folder where they won’t trip up Module Manager while still being accessible?

Could use the CKAN cache folder or specify a new one. As long as CKAN notifies or asks the user if he wants to backup the folder/settings that would work well IMO.

[HebaruSan]

the cause is CKAN not wanting to ditch user-settings

That's part of it, but there's also manually installed mods in general.
Consider a user who manually installs EVE (into GameData/BoulderCo), then uses CKAN to install KolyphemusSystemContinued (which installs its EVE configs into GameData/BoulderCo).

Now suppose this user changes his mind and uninstalls KolyphemusSystemContinued. As I understand your proposal (keep in mind that CKAN can't know the difference between manually installed mod files and mod-generated configs!), this would cause CKAN to move the GameData/BoulderCo folder to some other location, inappropriately uninstalling the user's manually installed EVE. I think that's pretty clearly a non-starter in terms of disruptions for the user.

[linuxgurugamer]

I can accept that, but what about having CKAN present a list of all the directories that it removed mods from. That way at least a user could do it manually without having to look at all directories.

Reason for this is that some mods are inside other directories, so trying to find it is sometimes difficult.

[Jognt]

@HebaruSan perhaps CKAN can remember which folders it created and which were present and only remove folders it created?

Though it won’t make the process idiot-proof, it still covers the usecase example you noted.

Or just a message like “CKAN detected remaining files and has not removed the folder for [modName] please manually review the remaining files before starting KSP. [Click to open folder in explorer/filemanager]”.

Keeping the user informed is more valuable than trying to automate a process which can involve many variables that are out of CKAN’s control.

[linuxgurugamer]

This is good. If the directory is empty after removing the mod, then remove the directory.
Else
provide a list of mod directories which have files in them along with a message like what @Jognt said above

[HebaruSan]

If the directory is empty after removing the mod, then remove the directory.

CKAN already does this part, for what it's worth.

[HebaruSan]

Though it won’t make the process idiot-proof, it still covers the usecase example you noted.

Until you flip the order. Install KolyphemusSystemContinued via CKAN, then install EVE manually. Now since CKAN created GameData/BoulderCo, we have the same problem again.

[Jognt]

Yes. But that’s a different use-case which I’d personally solve with something like this:

“CKAN automates the installation and deinstallation of KSP (Kerbal Space Program) modifications. Please note that modifying or adding files in/to CKAN-managed installations may result in data loss if the modified modification is later deinstalled using CKAN. Please follow modding best-practices by saving customizations in custom folders.”

Like I said, you can’t be reasonably expected to take into account every single variable in an open system where the end user can do everything. After all, if I modify a CKAN-installed file then it will still be removed anyway. (As would be expected)

If CKAN were to meticulously index each and every file and remove only the ones that were identical to when they were installed then you’d be left with a program that needs constant user intervention.

Having said that, I’m sure it’s technically possible to write something that takes every single use-case into account and I’d give a definite thumbs-up while thinking you went far above and beyond what could be reasonably expected of the system.

Edit: Or in less legalese I’d go for a JPG of Emperor Kuzco with the caption “No touchy!”. (Movie: Emperor’s New Groove)

[politas]

If all relevant mods had such a clause, the problem could be solved simply by deleting the directory-scanning logic

Ok, so a simple case of getting those cats herded! :-)

[HebaruSan]

Can get to this point without too much pain or suffering:

However, this is going to be a very power-user oriented UI. The newbie does not know why removing directories matters or how to choose correctly.

[DasSkelett]

Maybe it's clearer if there's another sentence at the end:

It is generally recommended to remove the directory, otherwise Module Manager might think the mod is still installed. However, custom mod settings that may have been created will be lost.

[Jognt]

Looks much better than having to check after each uninstall to check.

The important parts are:

• letting the user know the folder wasn’t removed (yet);
• letting the user know that removal is recommended;

The former being useful to everyone and the latter helping casual users get over the “remove sounds bad?” hurdle.

Additionally it may be nice to list multiple mods/folders in case of mass-uninstalling so there isn’t a pop-up for each separate mod.

If there is one thing I’ve learned in my UX studies it’s that the user will always manage to do something unintended, so the best thing you can do is give the info needed to decide.

[HebaruSan]

Additionally it may be nice to list multiple mods/folders in case of mass-uninstalling so there isn’t a pop-up for each separate mod.

Oh, that's what it does, I just didn't show it:

But this is really a mock-up, I think a final UI would have to have checkboxes so you could choose which ones to delete, as well as a way to view the remaining files (either in the UI or by opening a file browser window).

[Jognt]

Oh cool. Personally I’d prefer a “Show in explorer” type deal, but I’m guessing the average joe would appreciate a “I just have to click OK right?” approach.

Edit: I should be more like average Joe.

[linuxgurugamer]

Can you add a check to CKAN for the following files, and if found, delete them when removing a mod:

MiniAVC.dll.pruned
MiniAVC-v2..dll.pruned
MiniAVC.v2-Updated.dll
MiniAVC.v2-Updated.dll.old

This would be to account for ZeroMiniAVC pruning the MiniAVC.dll file. The other three are for a new mod I'm working on to replace all MiniAVC.dll files with an updated one, this because of a issue with MiniAVC and KSP 1.8 which causes the game to crash badly

[HebaruSan]

No, we're not going to add hacks for specific files.

[HebaruSan]

Got it to this point:

That long text label is wordy and confusing. I asked for help on Discord, but if anyone has suggestions here, please do share them.