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

Offer an option to reset local settings if loading them fails at launch #917

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

anttimaki
Copy link
Collaborator

If SettingsLoader fails to load the settings, a button for clearing out the IndexedDB containing local settings is shown. Also offers different suggestions for solutions based on what went wrong.

Refs TS-1384

@anttimaki
Copy link
Collaborator Author

If loading the info of default game (RoR2) fails:
image

If loading the settings fails the first time (this was achieved by putting a non-JSON string in the storage):
image

If user clicks the reset button, but the resetting itself fails:
image

I'm not sure if we want to use link here. It's easier for the user than copy-pasting a long URL, but it's opened in another window spawned by the mod manager rather than user's default browser.

If resetting IndexedDB succeeds, but the settings still can't be loaded:
image

Any suggestions for better wording etc. is welcome.

@anttimaki
Copy link
Collaborator Author

Here's a video of how things should ideally work. The modal is trickered by reloading the app via dev tools (because it was easier for me to record the video this way) and having the code intentionally corrupt the storage on first load attempt.

r2modman_reset.mp4

@MythicManiac
Copy link
Collaborator

Not a full review yet, but could the button use the styling that's used elsewhere in the app? I'd assume some fairly easy to re-use classes exist for that.

Copy link
Collaborator

@MythicManiac MythicManiac left a comment

Choose a reason for hiding this comment

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

Main question here, didn't review the smaller details yet

src/r2mm/manager/ManagerSettings.ts Outdated Show resolved Hide resolved
@anttimaki
Copy link
Collaborator Author

@MythicManiac

  • The button now has rounded corners and padding that's consistent with the rest of the buttons. The buttons offered by Bulma mostly looked off when set on the red background, so the button is still white.
  • The settings database is now dropped using IndexedDB API directly. For now I chose to not require a restart, since it didn't seem necessary. Thoughts?

Copy link
Collaborator

@MythicManiac MythicManiac left a comment

Choose a reason for hiding this comment

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

Looks good enough to me, left a comment but I don't consider it a blocker

src/components/SettingsLoader.vue Outdated Show resolved Hide resolved
Copy link
Owner

@ebkr ebkr left a comment

Choose a reason for hiding this comment

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

Added comments primarily regarding wording

can use the button below to reset the
settings, but note that all settings for all
games will be lost and this can't be undone.
This won't affect the loaded mods or mod profiles.
Copy link
Owner

Choose a reason for hiding this comment

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

This will if a custom data directory was set. I need to work on a way to re-link easily as currently it expects an empty folder.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed that line.

</div>

<p v-else-if="phase === PHASES.RESET_FAILED">
Well, that's awkward. Resetting of the settings
Copy link
Owner

Choose a reason for hiding this comment

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

Preference to avoid things like "Well, that's awkard" as it adds no value and is just clutter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

Well, that's awkward. Resetting of the settings
failed as well. You can still try to reset the
settings manually by following these
<a target="_blank" rel="noopener noreferrer"
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for noopener / noreferrer?

Can we bind the Link component provider earlier (if it needs binding?) as that can open in an actual browser

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for noopener / noreferrer?

Generally good practice, there are some security concerns with links that don't use these.

Not sure about the Link component suggestion, leaving that up to @anttimaki

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea was that the SettingsLoader mounts before any providers etc. to reduce points of failure, and to ensure that any problem encountered is actually related to loading the settings. If having the link open in a browser is considered worth it to introduce the LinkingProvider earlier, and the LinkingProvider is seen as a stable and tested implementation, I see no reason why it couldn't be done. Personally I don't see it fitting into the idea (that I had) of the SettingsLoader, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense to me at least, given that the goal was to minimize the points of failure for this component.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it’d be good to do, and it’s stable as in it only depends on external libraries for opening, it’s not reliant on settings or anything user accessible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now passed to SettingsLoader as a prop to keep things consistent with logError. Also skipped the provider and used the implementation directly so that all the providers are initiated in the same place (App.vue).

</p>

<p v-else-if="phase === PHASES.RETRY_FAILED">
That didn't do much, it seems. Locally stored
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment about wording, I think we want to avoid "friendly" wording and to keep it more professional and straight to the point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

ebkr
ebkr previously requested changes Dec 18, 2022
Copy link
Owner

@ebkr ebkr left a comment

Choose a reason for hiding this comment

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

Left it as comment by accident 👀

Base automatically changed from settings-loader to develop December 19, 2022 15:37
If SettingsLoader fails to load the settings, a button for clearing out
the IndexedDB containing local settings is shown. Also offers different
suggestions for solutions based on what went wrong.

Refs TS-1384
@anttimaki anttimaki dismissed ebkr’s stale review December 20, 2022 13:58

Requested changes have been addressed and approved by MythicManiac

@anttimaki anttimaki merged commit b79dc98 into develop Dec 20, 2022
@MythicManiac MythicManiac deleted the settings-reset branch December 20, 2022 13:59
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