[feature] QgsSettings QGIS QSettings replacement #4160

elpaso · 2017-02-18T16:55:31Z

As discussed in qgis/QGIS-Enhancement-Proposals#82 from
qgis/QGIS-Enhancement-Proposals#82 (comment)

This is the new QgsSettings class that adds an (optional)
Global Settings additional ini file where a system administrator
can store default values for the settings and provide
pre-configuration.

If an ini file named qgis_global_settings.ini is found
in the pkgDataPath directory it is automatically loaded,
this path can be overriden by a command line argument
( --globalsettingsfile path ) and through an environment
variable (QGIS_GLOBAL_SETTINGS_FILE).

For now, the new class is used in main.cpp
and WMS configuration (for testing and demonstration).

When this PR will be merged, the new QgsSettings
class will be applied to all QGIS code base.

NathanW2 · 2017-02-19T07:50:14Z

src/app/main.cpp

+  }
+  if ( globalsettingsfile.isEmpty( ) )
+  {
+    QString default_globalsettingsfile = QgsApplication::pkgDataPath( ) + "/qgis_global_settings.ini";


I might change this if this is ok based on the QEP. It will just look in a QgsApplication::pkgDataPath()/gloabl/QGIS/QGIS.ini file. As per the QEP just makes it easier and allows use to include other things in the "global config" as it's just a copy of a user config but in the install folder. e.g we might want to include extra plugins, symbols, etc in there so we can setup a user config and then copy to install folder.

Leave for now though and I will rework with my QEP work this week.

NathanW2 · 2017-02-19T07:52:27Z

@elpaso this looks really great. I will pull down and test this week. I think I will end up moving most of the settings loading logic that is found in main.cpp into a static method on QgsSettings in order to make it easier to find. The user config stuff will also live in there.

nyalldawson

Posted a couple of small code fixes.

Something I'm wondering- could we have an instance of this attached to QgsApplication? There's various parts of code which would benefit from being able to connect to a "changed" signal from the qsettings. At the moment this code relies on ugly "something->emitChanged()" type workarounds. It'd be much cleaner if this could be avoided and that code could just connect to a QgsApplication::settings changed signal.

nyalldawson · 2017-02-20T00:32:49Z

src/core/qgssettings.h

+     * called application from the organization called organization, and with parent parent.
+    */
+    explicit QgsSettings( const QString &organization,
+                          const QString &application = QString(), QObject *parent = 0 );


0 -> nullptr

nyalldawson · 2017-02-20T00:33:45Z

src/core/qgssettings.h

+
+    void init( );
+    QString sanitizeKey( QString key ) const;
+    QSettings* mGlobalSettings;


please initialize pointer variables at declaration - see d19e707

nyalldawson · 2017-02-20T00:34:07Z

src/core/qgssettings.h

+    QString sanitizeKey( QString key ) const;
+    QSettings* mGlobalSettings;
+    static QString sGlobalSettingsPath;
+    bool mUsingGlobalArray;


ditto with bools - safer to initialize here (c++11 style)

3nids · 2017-02-20T08:39:55Z

I like very much the idea. +1 on @nyalldawson 's idea for signals, I wanted to have something like that for a while. Going further, what do you think about adding project settings to it so we would have signals for them too.
We could use a QgsSettingScope (global vs project) to deal with them.

elpaso · 2017-02-20T09:01:20Z

@nyalldawson , @m-kuhn travis is apparently failing on unrelated tests (I restarted the build but it's now failing on another test, Travis sometimes really looks like a good candidate for a slow entropy generator 😄 ), can you please have a look? https://travis-ci.org/qgis/QGIS/builds/203345828#L1481

Something I'm wondering- could we have an instance of this attached to QgsApplication? There's various parts of code which would benefit from being able to connect to a "changed" signal from the qsettings.

I also like the idea, but in this case it would not be a drop-in QSettings replacement any more, right? All QgsSettings consumers would then use the instance provided by the app or loose the listeners attached to the signal.

Are you thinking to emit the signal whenever a setValue (& friends) is called?

BTW, I feel that maybe worth leaving QgsSettings duty to pure QSettings handling (as I originally designed it) and wrap it into a settings manager class, that would take care of the signals, of the QgsSettings inital configuration and all other interactions with QgsApplication.

3nids · 2017-02-20T09:04:38Z

@elpaso instead of inheriting QSettings, you can just use a private member. That would allow:

creating your own setValue / value which would emit signal
handle project settings in the same class
have a greater flexibility

m-kuhn · 2017-02-20T09:08:05Z

travis is apparently failing on unrelated tests

I was a bit fast on the merge button and fixed it 20 minutes later, you probably just caught a very bad time. Rebase and push ;)

NathanW2 · 2017-02-20T09:08:55Z

I also like the idea, but in this case it would not be a drop-in QSettings replacement any more, right? All QgsSettings consumers would then use the instance provided by the app or loose the listeners attached to the signal.

@elpaso @nyalldawson and I talked about this and it will still work with the current model. QSetings is a QObject. So we can just assign the settings object that we make in main to QgsApplication. If someone needs to listen to signals they can do

QgsApplication::settings().valueChanged.connect()

if they don't care and just want to read values you can just do

QgsSettings s;
s.value(...)

both work here and QgsApplication::settings() is just a shortcut to the instance we make in main.cpp

elpaso · 2017-02-20T15:35:08Z

@3nids we can always override setValue and emit the signal, I'd prefer to not create a separate class unless it's really needed or it has a real advantage over a plain old inheritance.
The original idea was to have a QSettings replacement with QGIS superpowers and the inheritance makes it explicit that there is a main QSettings class (the parent) with the parent's API, that uses another dependant and completely transparent/optional QSettings instance as a provider for default values. I guess that in the future we will probably abstract the defaults provider to support sources for default values different from QSettings such as DBs etc., but the main mission of the class will still be a QSettings replacement.

m-kuhn · 2017-02-20T15:38:43Z

@3nids we can always override setValue and emit the signal, I'd prefer to not create a separate class unless it's really needed or it has a real advantage over a plain old inheritance.

AFAIK QSettings::setValue is not virtual.

elpaso · 2017-02-20T16:29:17Z

@m-kuhn you are correct, but I don't see the problem here. Calling setValue from a QgsSettings instance would result in a call to QgsSettings::setValue as expected.

m-kuhn · 2017-02-20T16:36:27Z

True, sounds like it's fine in this scenario here.

NathanW2 · 2017-02-21T07:31:26Z

Ok to merge?

3nids · 2017-02-21T07:31:51Z

I just don't see any advantage of inheritance, only drawbacks.
Not inheriting like you suggest would allow to use other means of saving settings while still using the same class.
Moreover, how would you emit signal if you subclass? (this is something really important as far as I see it)

NathanW2 · 2017-02-21T07:33:18Z

@3nids emit the signal in setValue() and connect to it via QgsApplication::settings().settingChanged

3nids · 2017-02-21T07:34:35Z

but you cannot override setValue, it's not virtual. Or am I missing something?

And again, is there any advantage of inheritance? I'm just seeing a constraint for the future.

elpaso · 2017-02-21T07:40:56Z

@3nids See my previous comment: #4160 (comment)
As I said, the original idea of the QgsSettings class was to provide additional (optional and transparent) behaviour to QSettings maintaining the original API (with the addition of a few convenience methods).
QgsSettings is a specialized QSettings, I believe that inheritance is the correct pattern here.

NathanW2 · 2017-02-21T07:41:41Z

@3nids thinking about it now I'm considering the same thing. And some google searching has turned up the same things, better to just wrap QSettings and not inherit.

Would that break anything @elpaso? I think the main thing was that you would then have to use QgsSettings because QSettings isn't going to be set like it used to be. However, I'm wondering if that isn't a bad thing now to force people to the right class.

@elpaso I'm happy to take this work on if you can't. We will have to do a blank replace though out the code base but that is OK.

NathanW2 · 2017-02-21T07:44:32Z

It should be mainly just having the same methods as QSettings and then just forwarding the calls though right?

3nids · 2017-02-21T07:45:59Z

@NathanW2 yep!

elpaso · 2017-02-21T07:46:45Z

@NathanW2 I still don't see a valid reason for doing it. But there are also no particular reasons for not doing it, beside that it's more complicated that it could be, and I tend to prefer simpler solutions.

It should be mainly just having the same methods as QSettings and then just forwarding the calls though right?

Yes, that's it.

I can do the implementation.

NathanW2 · 2017-02-21T07:49:02Z

@elpaso I think it will be easier to go from composition to inheritance later on rather than the other way. So if you are happy to make the change now that would be cool. My offer is still open if you need a hand.

wonder-sk

I would be also in favor of composition - always a safer option especially if the class is not explicitly designed to be inherited (i.e. having virtual methods)

I am also wondering if we could combine QgsSettings with QgsProject somehow - in QgsProject we have those project properties that try to work the same way as QSettings, just on project level...

It seems that the global settings file is always newly opened and parsed - I think we should probably cache the global settings to avoid this extra overhead every time QgsSettings is used.

Could we make the API leaner by avoiding the use of getters/setters like guiValue(), setGuiValue() ? It seems to me that the concept of sections is not necessary if there is already support for groups in the API? In case we need some shortcuts for often used groups, why not just using constructor like this: QgsSettings s(QgsSettings.Server); (where QgsSettings.Server would be a string literal)

elpaso · 2017-02-21T08:21:55Z

@NathanW2 do you think the signal valueChanged should be emitted on remove(key) ? Or probably better a different signal. And what about a valueAdded() ?

@wonder-sk I'm already refactoring to a composition.
The rationale for the section getters/setters is to try to stop the current anarchy on the settings by providing an API that helps to classify the settings into the main sections of the code (libraries) and adding Plugins and Misc.

3nids · 2017-02-21T17:18:43Z

src/core/qgssettings.h

+    //! Removes the setting key and any sub-settings of key.
+    void remove( const QString &key );
+    //! Overloaded getter that accepts an additional Section argument
+    QVariant sectionValue( const QString &key, const Section section, const QVariant &defaultValue = QVariant() ) const;


I would not create a new method here, but rather use value and setValue overloads.

Thanks for the tip, I'll add the Section as an optional last argument to value getter/setter.

3nids · 2017-02-21T19:27:05Z

Thanks a lot for this @elpaso it will bring new opportunities!

NathanW2 · 2017-02-21T23:00:22Z

@elpaso once you have removed sectionValue I will merge this.

NathanW2 · 2017-02-21T23:00:38Z

@elpaso thanks again for changing the design based on feedback.

This is the new QgsSettings class that adds an (optional) Global Settings additional ini file where a system administrator can store default values for the settings and provide pre-configuration. If an ini file named qgis_global_settings.ini is found in the pkgDataPath directory it is automatically loaded, this path can be overriden by a command line argument ( --globalsettingsfile path ) and through an environment variable (QGIS_GLOBAL_SETTINGS_FILE). For now, the new class is used in main.cpp and WMS configuration (for testing and demonstration). When this PR will be merged, the new QgsSettings class will be applied to all QGIS code base.

... and 0 -> nullptr

NathanW2 · 2017-02-22T10:22:47Z

Travis was passing unsure why GitHub was saying otherwise. Thanks to all for the review and feedback and @elpaso for all the work.

nirvn · 2017-02-23T10:21:28Z

@elpaso , @NathanW2 , on my machine, the QGIS startup tips window keep showing up, even though I check the [x] I've had enough -- could this PR have caused this regression?

NathanW2 · 2017-02-23T10:23:51Z

I don't suspect so but unsure. It's working fine on my machine at the moment.

…

On Thu, Feb 23, 2017 at 8:21 PM, Mathieu Pellerin ***@***.***> wrote: @elpaso <https://github.com/elpaso> , @NathanW2 <https://github.com/NathanW2> , on my machine, the QGIS startup tips window keep showing up, even though I check the [x] I've had enough -- could this PR have caused this regression? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4160 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXS3E3TxwVQ1FcfbOhVqHp7E6ewGVppks5rfV2pgaJpZM4MFLR4> .

elpaso · 2017-02-23T10:46:45Z

@nirvn I'll have a look

elpaso · 2017-02-23T12:42:40Z

@nirvn @NathanW2 , yes it is a (temporary) regression: the new QgsSettings keys are not case sensititive, but QgsSettings is not yet used in all QGIS code, this means that when all QSettings istances will be replaced by QgsSettings this issue will go away.
There will be other issues like this until the migration from QSettings to QgsSettings will be completed.

I'm hesitant to do it now, we should first decide if we want to implement @nyalldawson proposal of a QgsSettings QgsApplication()::settings() implementation (that looks good to me) or if choose a plain search and replace from QSettings to QgsSettings.

NathanW2 · 2017-02-23T12:45:16Z

You can leave it for now. I am reworking the loading order etc of the settings and will add it to QgsApplication as part of my QEP work or maybe before.

…

On Thu, Feb 23, 2017 at 10:42 PM, Alessandro Pasotti < ***@***.***> wrote: @nirvn <https://github.com/nirvn> @NathanW2 <https://github.com/NathanW2> , yes it is a (temporary) regression: the new QgsSettings keys are *not* case sensititive, but QgsSettings is not yet used in all QGIS code, this means that when all QSettings istances will be replaced by QgsSettings this issue will go away. There will be other issues like this until the migration from QSettings to QgsSettings will be completed. I'm hesitant to do it now, we should first decide if we want to implement @nyalldawson <https://github.com/nyalldawson> proposal of a QgsSettings QgsApplication()::settings() implementation (that looks good to me) or if choose a plain search and replace from QSettings to QgsSettings. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4160 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXS3AL8aHvbOF_Hcq_W7-OhHIxS846Gks5rfX7BgaJpZM4MFLR4> .

nirvn · 2017-02-26T04:06:58Z

@elpaso , @NathanW2 , here's another QgsSettings regression (which crashes QGIS): http://hub.qgis.org/issues/16233

Also, adding a WM(T)S layer fails now with the following error message: Failed to download capabilities: Download of capabilities failed: Protocol "" is unknown. I'm also assuming it's linked to this QgsSettings PR.

The welcome tips regression wasn't that serious, but this is rather more consequential 😄 , should we temporarily revert?

nirvn · 2017-02-26T05:13:19Z

One more: http://hub.qgis.org/issues/16234 -- here, an option setting (icon size) appears to be properly written, but not read upon QGIS launch.

elpaso · 2017-02-27T15:33:51Z

src/core/qgssettings.cpp

+
+void QgsSettings::remove( const QString &key )
+{
+  mGlobalSettings->remove( key );


@nirvn @NathanW2 the SEGFAULT error is here: is should be mUserSettings instead. I'm on holiday with limited connection and without my dev PC, I can fix it later this week.

elpaso · 2017-02-27T15:35:01Z

@nirvn there will be more: every time a mixed/upper case key is created by QSettings and read through QgsSettings. As I said the problem is temporary and will go away when QsgSettings will be used to write and read the settings.

As a temporary solution, you can revert the QgsSettings use in WMS classes (that is the one I used to show off the class usage), and in qgisapp.cpp.

Regarding http://hub.qgis.org/issues/16233 , see my comment here https://github.com/qgis/QGIS/pull/4160/files#r103233230

* [feature] QgsSettings QGIS QSettings replacement This is the new QgsSettings class that adds an (optional) Global Settings additional ini file where a system administrator can store default values for the settings and provide pre-configuration. If an ini file named qgis_global_settings.ini is found in the pkgDataPath directory it is automatically loaded, this path can be overriden by a command line argument ( --globalsettingsfile path ) and through an environment variable (QGIS_GLOBAL_SETTINGS_FILE). Cherry pick from e1ede70

* [feature] QgsSettings QGIS QSettings replacement (qgis#4160) This is the new QgsSettings class that adds an (optional) Global Settings additional ini file where a system administrator can store default values for the settings and provide pre-configuration. If an ini file named qgis_global_settings.ini is found in the pkgDataPath directory it is automatically loaded, this path can be overriden by a command line argument ( --globalsettingsfile path ) and through an environment variable (QGIS_GLOBAL_SETTINGS_FILE). Cherry pick from e1ede70, plus commits from Jurgen Fischer and Mathieu Pellerin

elpaso force-pushed the qgssettings-prototype branch 2 times, most recently from 5f4633c to c1f8ea9 Compare February 18, 2017 20:43

NathanW2 self-assigned this Feb 19, 2017

NathanW2 added the For Review label Feb 19, 2017

NathanW2 added this to the QGIS 3 milestone Feb 19, 2017

NathanW2 reviewed Feb 19, 2017

View reviewed changes

nyalldawson reviewed Feb 20, 2017

View reviewed changes

elpaso force-pushed the qgssettings-prototype branch from 8a3892d to 72cf11f Compare February 20, 2017 09:27

wonder-sk reviewed Feb 21, 2017

View reviewed changes

3nids self-requested a review February 21, 2017 17:16

3nids reviewed Feb 21, 2017

View reviewed changes

elpaso added 4 commits February 22, 2017 08:17

Initialize QgsSettings members at declaration

e1ab177

... and 0 -> nullptr

Removed getter/setters and refactored to composition

2e5ec20

Moved Section getter/setter to an optional additional argument

3aaceac

elpaso force-pushed the qgssettings-prototype branch from f7f1316 to 3aaceac Compare February 22, 2017 07:19

NathanW2 merged commit e1ede70 into qgis:master Feb 22, 2017

elpaso deleted the qgssettings-prototype branch February 22, 2017 17:07

jef-n mentioned this pull request Feb 26, 2017

Migrate to QgsSettings and python cleanups #4190

Merged

elpaso commented Feb 27, 2017

View reviewed changes

qgib mentioned this pull request May 25, 2019

Editing raster pseudocolor classification seems to trigger unwanted auto-classification after validation and by the same clearing all modifications #25001

Closed

This was referenced May 5, 2020

global settings file lutraconsulting/qgis-discovery-plugin#72

Closed

migrate to QgsSettings to use global settings file MarcoDuiker/QGIS_Map_library#2

Merged

[feature] QgsSettings QGIS QSettings replacement #4160

[feature] QgsSettings QGIS QSettings replacement #4160

Conversation

elpaso commented Feb 18, 2017

NathanW2 Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

NathanW2 commented Feb 19, 2017

nyalldawson left a comment

Choose a reason for hiding this comment

nyalldawson Feb 20, 2017

Choose a reason for hiding this comment

nyalldawson Feb 20, 2017

Choose a reason for hiding this comment

nyalldawson Feb 20, 2017

Choose a reason for hiding this comment

nyalldawson Feb 20, 2017

Choose a reason for hiding this comment

3nids commented Feb 20, 2017

elpaso commented Feb 20, 2017

3nids commented Feb 20, 2017

m-kuhn commented Feb 20, 2017

NathanW2 commented Feb 20, 2017 • edited Loading

elpaso commented Feb 20, 2017

m-kuhn commented Feb 20, 2017

elpaso commented Feb 20, 2017

m-kuhn commented Feb 20, 2017

NathanW2 commented Feb 21, 2017

3nids commented Feb 21, 2017

NathanW2 commented Feb 21, 2017

3nids commented Feb 21, 2017 • edited Loading

elpaso commented Feb 21, 2017

NathanW2 commented Feb 21, 2017

NathanW2 commented Feb 21, 2017

3nids commented Feb 21, 2017

elpaso commented Feb 21, 2017

NathanW2 commented Feb 21, 2017

wonder-sk left a comment

Choose a reason for hiding this comment

elpaso commented Feb 21, 2017

3nids Feb 21, 2017

Choose a reason for hiding this comment

elpaso Feb 21, 2017

Choose a reason for hiding this comment

3nids commented Feb 21, 2017

NathanW2 commented Feb 21, 2017

NathanW2 commented Feb 21, 2017

NathanW2 commented Feb 22, 2017

nirvn commented Feb 23, 2017

NathanW2 commented Feb 23, 2017 via email

elpaso commented Feb 23, 2017

elpaso commented Feb 23, 2017

NathanW2 commented Feb 23, 2017 via email

nirvn commented Feb 26, 2017

nirvn commented Feb 26, 2017

elpaso Feb 27, 2017

Choose a reason for hiding this comment

elpaso commented Feb 27, 2017

NathanW2 Feb 19, 2017 •

edited

Loading

NathanW2 commented Feb 20, 2017 •

edited

Loading

3nids commented Feb 21, 2017 •

edited

Loading