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

Handle Reset options gracefully. #924

Merged
merged 1 commit into from
May 20, 2020
Merged

Conversation

manavmehta
Copy link
Collaborator

@manavmehta manavmehta commented Apr 7, 2020

What's this PR do?
Fixes: #903

  • Transport Factory Reset from menu bar to Settings page
  • Remove Reset App Data option as it does not do something much useful

Screenshots?
Reset
P.S. The ? is added in the messageBox heading, just after screenshot

You have tested this PR on:

  • macOS Catalina 10.15.2
  • Linux/Ubuntu 18.04

@manavmehta
Copy link
Collaborator Author

@akashnimare Can you please have a look?

@manavmehta
Copy link
Collaborator Author

manavmehta commented Apr 15, 2020

@akashnimare @timabbott please have a look if it seems concise

.
Reset App Data
reset-data

.
Factory Reset
factory-reset

@timabbott
Copy link
Member

The messages in the popups messages are still kinda confusing. E.g. "you are starting Zulip for the first time" makes it sound like it might delete your message history. The first part but ending with "... as if you just downloaded the Zulip desktop app for the first time" would be clear on that front.

And for the first one, I think "Reset all the Zulip desktop app's configuration settings to the defaults." would be clear.

Otherwise lgtm.

@akashnimare
Copy link
Member

I think "Factory Reset" setting should go somewhere in the Menu. Two settings for wiping out the app data doesn't look good UX wise and it may confuse users.

On second thought we should rethink our decision of adding the "Reset app data" in the setting. As far as I remember the main reason behind adding this was to handle the case where users weren't able to restore the window position of the app and I think we have not received any issue related to the window restoring feature. So I think the best way would be -

  • Remove the "Reset app data" setting completely
  • Keep the "Factory reset" in the setting option

open for discussion.

cc - @timabbott @abhigyank

@manavmehta
Copy link
Collaborator Author

@akashnimare as you said, One option indeed looks better
Regarding

handle the case where users weren't able to restore the window position of the app
we have not received any issue related to the window restoring feature

Issue #28 is solved but I can;t check for #231 as I don;t have an external display. If it's solved and window-state does the necessary, then we might keep only Factory Reset
If #231 still persists, we may solve that separately and then only merge this.
@timabbott @abhigyank if it sounds fair ?

@akashnimare
Copy link
Member

Folks have not reported the window position error in the last two years so we can assume it works well.

@manavmehta
Copy link
Collaborator Author

@akashnimare Should we wait for their reply on the thread and then go on with

  • Removing Reset App Data
  • Keep Factory Reset as the only reset

@akashnimare
Copy link
Member

Fix the merge conflicts. I think we can remove the one which deletes the app data. It doesn't seem to serve much of a purpose. So for now, let's keep the factory one. We can revisit this later based on more feedbacks etc.

@manavmehta manavmehta force-pushed the factory-reset branch 2 times, most recently from 2ba7114 to 951d70a Compare May 19, 2020 19:26
@manavmehta manavmehta changed the title Redefinition and Replacement of Factory Reset and Reset App Data options Handle Reset options gracefully May 19, 2020
* Transport `Factory Reset` from menu bar to Settings page, configuring it to remove the application data instead of just config/ files
* Remove the `Reset App Data` option as it doesn't do anything much useful
* remove redundant ipcRenderer call in renderer/js/main.ts

Fixes: zulip#903
@akashnimare
Copy link
Member

LGTM. Merging.

@akashnimare akashnimare changed the title Handle Reset options gracefully Handle Reset options gracefully. May 20, 2020
@akashnimare akashnimare merged commit f55570f into zulip:master May 20, 2020
@akashnimare
Copy link
Member

@manavmehta as a follow-up, can you send a PR updating the translating file since some strings are being updated in this PR.

@manavmehta
Copy link
Collaborator Author

@akashnimare Sure. The context-menu would be having new strings too. So I'll look into them also at once :)

@manavmehta manavmehta deleted the factory-reset branch May 20, 2020 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

“Factory Reset” and “Reset App Data” use two completely separate code paths
5 participants