Skip to content

Allow users to export or import user-settings file in a GUI environment#2394

Merged
mgrojo merged 8 commits into
sqlitebrowser:masterfrom
lucydodo:dev-2393
Sep 19, 2020
Merged

Allow users to export or import user-settings file in a GUI environment#2394
mgrojo merged 8 commits into
sqlitebrowser:masterfrom
lucydodo:dev-2393

Conversation

@lucydodo

@lucydodo lucydodo commented Sep 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

This patch changes to allow users to export or import user-settings file in a GUI environment.
This PR is related to issue #2393.

Detailed Changes

  1. There was an opinion that the expression 'Settings' is more appropriate than 'Configuration' or 'Preferences'
    So it is be changed to 'Settings' and also, the following two command line options change accordingly:
    • -p [preferences_file] -> -S [settings_file]
    • --preferences [preferences_file] -> --settings [settings_file]
  2. Add the following two buttons to the 'PreferencesDialog'
    • Export Settings
    • Import Settings

Need Help

Screenshot from 2020-09-13 20-44-11
In issue #2393, @chrisjlocke showed a prototype of a new buttons to the left of the existing 'Restore Defaults` button.
I thought this is better, so I tried but failed due to my lack of ability. May I get any advice on this part?
This section has been patched with the following commit: b5a0d53

Test Environment

  • macOS Catalina (10.15.6, 19G2021)
  • Ubuntu Desktop 20.04.1

Thank you.

@lucydodo lucydodo added enhancement Feature requests. help wanted labels Sep 13, 2020
@lucydodo lucydodo self-assigned this Sep 13, 2020
@lucydodo lucydodo linked an issue Sep 13, 2020 that may be closed by this pull request
[Suggestion] Load or export user configuration file in a GUI environment #2393
Closed
mgrojo
mgrojo requested changes Sep 19, 2020
View reviewed changes

@mgrojo mgrojo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice feature and good implementation. Please, take into account just a pair of minor changes requested.

Comment thread src/Settings.cpp Outdated
Comment thread src/Settings.h Outdated
Comment thread src/Settings.cpp Outdated
Modify code according to review
304cf10 
Use range-base for loops instead of foreach
Rename verifyUserSettingsFile() to isVaildSettingsFile() for readability
@lucydodo

lucydodo commented Sep 19, 2020

Copy link
Copy Markdown
Member Author

Thanks for the review. I just modified and committed it. Could you take a look?

@mgrojo mgrojo merged commit 27c2f64 into sqlitebrowser:master Sep 19, 2020
@mgrojo

mgrojo commented Sep 19, 2020

Copy link
Copy Markdown
Member

Awesome! Another feature in development version. 👍

@mgrojo

mgrojo commented Sep 20, 2020

Copy link
Copy Markdown
Member

By the way, there is room for improvement about having to restart the application to apply the settings. Have you tried to make that work? We have the same problem when changing the language, but I suppose there could be problems due to Qt limitations (I haven't investigated it myself though). But maybe in this case it's just a matter of calling Settings::setValue() instead of invoking QSettings directly, and then get MainWindow to call reloadSettings(). Maybe a call to accept() at the end of PreferencesDialog::importSettings() would make the trick? I suppose that would do it, since it is what "restore defaults" is doing.

@lucydodo

lucydodo commented Sep 20, 2020

Copy link
Copy Markdown
Member Author

Yes, that's a good point.

It can be applied immediately by
call accept() in PreferencesDialog.cpp:importSettings(),
and call m_hCache.clear() in Settings.cpp:importSettings()

However, since the 'language' is also included in the settings file, I decided to request a restart.
If necessary, we can detect if there is a language change and only require a restart.

@mgrojo

mgrojo commented Sep 20, 2020

Copy link
Copy Markdown
Member

Yes, that makes sense.

@lucydodo

lucydodo commented Sep 20, 2020

Copy link
Copy Markdown
Member Author

Which method do you think is better?

  1. Keep the old way
  2. If the language settings is not changed, the settings is reflect immediately(work like 'Restore Defaults')
  3. When the language settings is changed, a warning indicating to restart
  4. When the language settings is changed, restart via code after warning(with QApplication, QProcess)

@mgrojo

mgrojo commented Sep 20, 2020

Copy link
Copy Markdown
Member

I think 2 and 3 is the best approach. 3 should reuse the current message dialog for notifying the user.

@lucydodo

lucydodo commented Sep 20, 2020 via email

Copy link
Copy Markdown
Member Author

@lucydodo

lucydodo commented Sep 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

One more thing, I think we need save settings before export settings.
What do you reckon? @mgrojo

However, since this closes the PreferencesDialog and opens the FileDialog, it may be a little unnatural.
I have prepared a screencast to help you understand. https://youtu.be/ZM6wYA4II-g

@mgrojo

mgrojo commented Sep 20, 2020

Copy link
Copy Markdown
Member

I see. Having these buttons in the Preferences dialog complicates the implementation. Maybe you have to split (or add a parameter to) saveSettings so you can save the data without calling accept, and call accept once the file has been chosen and saved.

@lucydodo

lucydodo commented Sep 21, 2020

Copy link
Copy Markdown
Member Author

Cool, this is a simple fix so I just committed it to the master branch without pr. 0031d9a

@lucydodo lucydodo deleted the dev-2393 branch October 29, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgrojo mgrojo mgrojo approved these changes

Assignees

@lucydodo lucydodo

Labels

enhancement Feature requests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Suggestion] Load or export user configuration file in a GUI environment

2 participants

@lucydodo @mgrojo