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

Add additional warning to plugin config reset modal #2005

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xpahtalo
Copy link

Adds a little warning to the config reset modal if you are subscribed to the testing version. It also further explains that this will delete plugin settings, not something related to Dalamud's config about the plugin. Here's how it should look if you are subscribed to testing.
image
We can't stop anyone from spreading bad info, but hopefully having an extra warning might get people to stop and ask more questions.

@Xpahtalo Xpahtalo requested a review from a team as a code owner August 11, 2024 06:42
Copy link
Member

@KazWolfe KazWolfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking in general:

  • Might it be clearer to call this a "factory reset" of a given plugin? Does that convey the same intent in a clearer fashion?
  • I'm not particularly a fan of "this won't remove you from testing" since users shouldn't be clicking in to this modal for that anyways, but that's I suppose why we got here in the first place...

Something to note though: for some plugins, you will need to reset configs to downgrade to stable. If the new config version is incompatible with the stable version of a plugin, there could very well be load errors and other annoyances.

@reiichi001
Copy link
Contributor

Something to note though: for some plugins, you will need to reset configs to downgrade to stable. If the new config version is incompatible with the stable version of a plugin, there could very well be load errors and other annoyances.

In this case, wouldn't serialization typically fail and reset the config anyways? While it's possible to craft up edge case and unique scenarios where a field kept the same name, but has an invalid value for an older version of the plugin, that would be reasonably easy for support to catch and notify a user if they report an issue, given it'd throw exceptions. (And is another reason plugin devs are encouraged to version config or note breakages, but aren't exactly expected to support downgrades, and we shouldn't really require that either. A user opting into testing is already consenting to those risks, whether they read the warning or forgot it.)

We do also have a separate modal that shows when a user is switching from a test version to a lower stable version, warning them about config. (Whether or not the user reads this or understands the ramifications is outside our control).
https://github.com/goatcorp/Dalamud/blob/master/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs#L2881
https://github.com/goatcorp/Dalamud/blob/master/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs#L942
https://github.com/goatcorp/Dalamud/blob/master/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs#L4258

@Xpahtalo
Copy link
Author

Xpahtalo commented Aug 14, 2024

Might it be clearer to call this a "factory reset" of a given plugin? Does that convey the same intent in a clearer fashion?

I agree that renaming this is probably worth it. Either something like "factory reset" which is well understood to be destructive, or something like "delete all plugin settings" to be more obvious that you are getting rid of something.

I'm not particularly a fan of "this won't remove you from testing" since users shouldn't be clicking in to this modal for that anyways, but that's I suppose why we got here in the first place...

It's a bit ugly and means the code has to reference things that are unrelated to the current action, but we clearly have lots of users that could benefit from having explicit explanations.

Something to note though: for some plugins, you will need to reset configs to downgrade to stable. If the new config version is incompatible with the stable version of a plugin, there could very well be load errors and other annoyances.

The modal for disabling testing somewhat mentions this, but we could add "unless told to by the plugin author" to the warning. I'm of the opinion it's unnecessary, though. From what I've seen, the devs that heavily utilize the testing branch to request early feedback are also active in Discord to support those situations.

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