Skip to content

Delete module folders on clearing settings#865

Open
Tharylia wants to merge 1 commit into
blish-hud:devfrom
Tharylia:delete-settings-folders
Open

Delete module folders on clearing settings#865
Tharylia wants to merge 1 commit into
blish-hud:devfrom
Tharylia:delete-settings-folders

Conversation

@Tharylia

Copy link
Copy Markdown
Contributor

This PR aims to clear all files of modules that currently remain when deleting the module settings.

Some modules use external json files to save some states.
These states currently do not get deleted when clicking "Delete module settings" sometimes causing problems.

This PR will break module settings from certain developers choosing to use bundled folders:
image

@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dlamkins

Copy link
Copy Markdown
Member

Due to the existence of bundled settings, I don't think I'll be able to move forward with this particular implementation. I'm going to keep this open, though, as it would be nice if we find a suitable alternative.

@Flyga-M

Flyga-M commented Apr 21, 2025

Copy link
Copy Markdown
Contributor

It might be possible to add a new virtual method to the Module class called ClearManualModuleData() or something similar. It would be called when the "Clear Settings" option is clicked.

This way module devs could handle themselves what data, states, etc. (in the registered directories) should be reset to default (and which should be kept).

We could even add a boolean property DeleteModuleFoldersOnClearSettings that defaults to false, or provide a protected method that could just be called in the ClearManualModuleData() override if deleting the whole folder (as originally proposed by this PR) works well for their specific module. This would keep the workload very low for smaller modules.

I guess adding new methods or properties on a class that is supposed to be inherited from could potentially introduce breaking changes, if any module already uses a method/property with this exact name. I feel like the chance for this could be kept low enough to still make this change.

@Tharylia

Tharylia commented Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

I have thought about that exact solution as well.
I didn't move forward with that as it basically has no effect until implemented by all modules.
And knowing the release cycle of blish core (new nuget needed) it would probably take months/years until this PR shows any noticable effects.

@Flyga-M

Flyga-M commented Apr 21, 2025

Copy link
Copy Markdown
Contributor

It might still be worth to implement. At least I don't see any better solution.

Forcing the deletion and making it opt-out (even just for modules that were developed for v1.3.0 or later) would probably be to drastic of a change I would assume?

And having the mentioned protected method or opt-in would make it pretty easy to adapt for most modules.

@Tharylia

Copy link
Copy Markdown
Contributor Author

I'm not against the solution with a protected virtual method. In fact I would prefer something like this so that the devs can handle their own stuff accordingly.

I just think that with the time it takes to get a new blish release out and modules getting updates (if ever) the "clear settings" function will have a lot of different effects.
But as Freesnöw said, because of bundled settings the original idea is not really going to work as expected either.

A protected virtual CleanupExternalSettings() function will be the best choice in that case I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants