Skip to content

Added export/import options for settings #6542

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 31, 2021
Merged

Added export/import options for settings #6542

merged 7 commits into from
Oct 31, 2021

Conversation

d2dyno1
Copy link
Member

@d2dyno1 d2dyno1 commented Oct 17, 2021 •
edited
Loading

Resolved / Related Issues

Details of Changes

  • Adds options for exporting and importing settings file + fixes an issue where settings weren't respecting AppCenter.

Known issues

  • Sidebar won't update shown sections after importing configuration. (Even though OnSettingChangedEvent is raised)

Validation
How did you test these changes?

  • Built and ran the app

@d2dyno1 d2dyno1 marked this pull request as ready for review October 17, 2021 18:14
@d2dyno1 d2dyno1 requested a review from yaira2 October 17, 2021 18:14
@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 17, 2021

Translations are pending - need to know if the proposed design is good 👍

@yaira2
Copy link
Member

yaira2 commented Oct 17, 2021

Translations are pending - need to know if the proposed design is good 👍

Can you share a screenshot?

@yaira2 yaira2 requested a review from winston-de October 17, 2021 19:12
@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 17, 2021

Can you share a screenshot?

image

@yaira2
Copy link
Member

yaira2 commented Oct 17, 2021

@d2dyno1 can you change it to say manage settings?

yaira2
yaira2 requested changes Oct 18, 2021
View reviewed changes
@d2dyno1 @yaira2
Apply suggestions from code review
c477a0b 
Co-authored-by: Yair Aichenbaum <39923744+yaichenbaum@users.noreply.github.com>
@d2dyno1 d2dyno1 requested a review from yaira2 October 18, 2021 04:26
@yaira2
Copy link
Member

yaira2 commented Oct 18, 2021

@d2dyno1 have you tested your changes for accessibility?

@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 18, 2021

@yaichenbaum No issues!

image

@yaira2
Copy link
Member

yaira2 commented Oct 18, 2021

@d2dyno1 can you add tooltips to the button?

yaira2
yaira2 previously approved these changes Oct 18, 2021
View reviewed changes
@winston-de
Copy link
Contributor

winston-de commented Oct 18, 2021 •
edited
Loading

Just a thought: do settings (though not really settings) such as LastSessionTabList and ContinueLastSessionOnStartUp belong in settings.json? They aren't exposed to the user, and are just used internally, so it doesn't really make sense to expose it in the import/export.

@winston-de
Copy link
Contributor

winston-de commented Oct 18, 2021 •
edited
Loading

It looks like the app will crash on startup if the imported settings file is invalid. So for example, if you replace a boolean in the settings file with a string, then import it, it will crash on startup. I think there should be some validation of the settings to avoid a crash.

@yaira2 yaira2 added changes requested Changes are needed for this pull request and removed needs - code review labels Oct 26, 2021
@d2dyno1 d2dyno1 mentioned this pull request Oct 26, 2021
Closed
4 tasks
@yaira2
Copy link
Member

yaira2 commented Oct 31, 2021

@d2dyno1 can you make the changes Winston requested?

@d2dyno1 d2dyno1 requested a review from yaira2 October 31, 2021 11:04
@d2dyno1 d2dyno1 removed the changes requested Changes are needed for this pull request label Oct 31, 2021
yaira2
yaira2 approved these changes Oct 31, 2021
View reviewed changes
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Nice work!

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Oct 31, 2021
@yaira2
Copy link
Member

yaira2 commented Oct 31, 2021

Sidebar won't update shown sections after importing configuration. (Even though OnSettingChangedEvent is raised)

@d2dyno1 I'm going to hold off merging this PR for now as I don't want to introduce new issues.

@yaira2 yaira2 added needs - code review and removed ready to merge Pull requests that are approved and ready to merge labels Oct 31, 2021
@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 31, 2021

Sidebar won't update shown sections after importing configuration. (Even though OnSettingChangedEvent is raised)

@d2dyno1 I'm going to hold off merging this PR for now as I don't want to introduce new issues.

Well, it's enough if user restarts the app. It works as expected + this PR fixes an issue with some UI areas not being updated after a setting has changed.

@yaira2
Copy link
Member

yaira2 commented Oct 31, 2021 •
edited
Loading

Well, it's enough if user restarts the app. It works as expected + this PR fixes an issue with some UI areas not being updated after a setting has changed.

@d2dyno1 can you elaborate on what the issue with the sidebar actually is? Guess it's okay if it's only when importing settings. Can you open an issue so we don't lose track of this?

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Oct 31, 2021
@yaira2 yaira2 merged commit 871b923 into files-community:main Oct 31, 2021
@yaira2 yaira2 mentioned this pull request Oct 31, 2021
Closed
@d2dyno1 d2dyno1 mentioned this pull request Oct 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 yaira2 approved these changes

@winston-de winston-de Awaiting requested review from winston-de

Assignees
No one assigned
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to export\import settings
3 participants
@d2dyno1 @yaira2 @winston-de