Add the ability to support custom config files #2390

lucydodo · 2020-09-04T16:47:34Z

This patch changes to support custom configuration file. This PR is related to issue #2317

Two options are added for the command line:

-c [config_file]
--config [config_file]

We can also use environment variables without using options on the command line.
export DB4S_CONFIG_FILE=[config_file]

If there is an environment variable and use the -c or --config option, the environment variable is ignored.

Usage

$ ./DB\ Browser\ for\ SQLite -c db4sconfig
$ ./DB\ Browser\ for\ SQLite --config db4sconfig

$ export DB4S_CONFIG_FILE=db4sconfig
$ ./DB\ Browser\ for\ SQLite

Possible problem

The database file may be overwritten if the user enters only the value of the database file to be open without specifying a configuration file parameter when using this option. -> $ ./DB\ Browser\ for\ SQLite --config veryImportantDB.db
Solved through this commit(60ddbdd).

Known Bugs

If you specify an environment variable on macOS and run it with the following command, the config file is not saved anywhere.
open ./DB\ Browser\ for\ SQLite.app/
However, if you specify as follows, it is saved normally.
./DB\ Browser\ for\ SQLite.app/Contents/MacOS/DB\ Browser\ for\ SQLite

Test Environment

macOS Catalina (10.15.6, 19G2021)

lucydodo · 2020-09-04T16:47:42Z

@mgrojo I have permission to merge, but I haven't finished studying and analyzing the project source tree yet.
May I ask for a review to you due to I am worried that it will cause unexpected problems to project via my code. 👀

mgrojo

A good start! Thanks for letting me review it.

src/Application.cpp

src/Settings.cpp

src/Settings.h

@mgrojo

Implement setter(setUserConfigurationFile) to store user configuration file. Move the code that print error when the argument for the option is not provided. Print detailed information when the value of an environment variable is substituted. Replaced with 'nullptr' instead of 'NULL' according to Modern C++ standard. Add code to check if provided file is a normal configuration file. Change the QSetting class object to be created only once(setSettingsObject). Using the '->' operator. Thank you so much for reviewing! @mgrojo

lucydodo · 2020-09-05T19:21:56Z

@mgrojo Thanks for the review! 😄 I modified the code according to your review.
In addition, the following modifications were made too:

Ignore the option if user does NOT have permission to the configuration file.
Fixed that the code for 'clearValue()' and 'restoreDefaults()' was not modified, so it did not work for user configuration file.

I added a comment, but it seems that the code is like 'spaghetti code'. I'll try think about neat code continue. 👀

mgrojo

From a user point of view would be ready for merging, but I've made some comments regarding the code which you might want to consider.

src/Settings.cpp

src/Application.cpp

src/Settings.cpp

Make QSettings* object(settings) as a static member variable. Compress error message when no arguments. Change the behavior of the 'setSettingsObject()` method to be similar to the existing interface. Simplify the normal file check code.

lucydodo · 2020-09-06T12:03:39Z

@mgrojo Thanks for the second review! 😄 I modified the code according to your review.
and thank you for always catching something I never thought of. thanks to that, I feel myself growing with our project. 😄

However, calling setSettingsObject() every time in method seems to slow down the program operation.

src/Settings.cpp

Change to abort 'setSettingsObject()' if QSettings object has already been created

mgrojo · 2020-09-06T18:10:30Z

Thanks, @lucydodo. It's now merged!

After merging, I've seen something that I think could be improved. If the file doesn't exist, it should be allowed and passed to QSettings so an initial file can be populated. Another option could be having an Export Settings option from the GUI, but although we had that, I think it is better to allow to create a new file from the argument. Application will run with default values and the file will be initialized with whatever setting the user changes, and they will be able to reuse it in the future. What do you think?

lucydodo · 2020-09-06T18:19:55Z

@mgrojo Sorry I didn't understand what you say because my English is not very well.
You mean that the user give the uncreated file as an argument?

mgrojo · 2020-09-06T18:21:49Z

Sorry, I was executing another branch 😄 It works as expected, forget my comment.

Add the ability to support custom config files

814d73f

lucydodo added the enhancement Feature requests. label Sep 4, 2020

lucydodo requested a review from mgrojo September 4, 2020 16:47

lucydodo self-assigned this Sep 4, 2020

mgrojo requested changes Sep 4, 2020

View reviewed changes

mgrojo reviewed Sep 6, 2020

View reviewed changes

src/Settings.cpp Outdated Show resolved Hide resolved

src/Application.cpp Outdated Show resolved Hide resolved

src/Settings.cpp Show resolved Hide resolved

src/Settings.cpp Outdated Show resolved Hide resolved

SeongTae Jeong added 2 commits September 6, 2020 20:54

Modify code according to second review

4db8e84

Make QSettings* object(settings) as a static member variable. Compress error message when no arguments. Change the behavior of the 'setSettingsObject()` method to be similar to the existing interface. Simplify the normal file check code.

Fix a bug that could cause NULL pointer access

96482e3

mgrojo reviewed Sep 6, 2020

View reviewed changes

src/Settings.cpp Show resolved Hide resolved

mgrojo approved these changes Sep 6, 2020

View reviewed changes

Change code to avoid unnecessary operation

4e45bce

Change to abort 'setSettingsObject()' if QSettings object has already been created

mgrojo merged commit 0644bc0 into sqlitebrowser:master Sep 6, 2020

lucydodo deleted the dev-2317 branch October 29, 2020 17:42

MatejKafka mentioned this pull request Oct 13, 2021

Newest release does not include code for portable config file #2870

Open

lucydodo mentioned this pull request May 27, 2022

[Feature]: Save settings to directory #3045

Open

Uh oh!

Add the ability to support custom config files #2390

Add the ability to support custom config files #2390

Uh oh!

Conversation

lucydodo commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage

Possible problem

Known Bugs

Test Environment

Uh oh!

lucydodo commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgrojo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucydodo commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgrojo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucydodo commented Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mgrojo commented Sep 6, 2020

Uh oh!

lucydodo commented Sep 6, 2020

Uh oh!

mgrojo commented Sep 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lucydodo commented Sep 4, 2020 •

edited

Loading

lucydodo commented Sep 4, 2020 •

edited

Loading

lucydodo commented Sep 5, 2020 •

edited

Loading

lucydodo commented Sep 6, 2020 •

edited

Loading