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

Settings registry (QEP 124) #42597

Merged
merged 20 commits into from
Apr 15, 2021
Merged

Conversation

domi4484
Copy link
Contributor

As of Enhancement proposal #124 and as followup to closed pr #41830 the goals of this pull request are to make the usage of settings less prone to errors.
Sorry @elpaso and @nyalldawson I screwed up my git branch and had to close #42239

In this proposed solution a new class QgsSettingsEntry is introduced. One instance will correspond to a single settings entry (or key) in QSettings.

The idea is to declare settings as static members in the class of appartenance like this for QgsLayout.h:

struct Settings
{
  static const inline QgsSettingsEntryStringList searchPathForTemplates = QgsSettingsEntryStringList( QStringLiteral( "Layout/searchPathsForTemplates" ), QgsSettings::Core, QStringList(), QObject::tr( "Search path for templates" ) );
};

and used like this:

QgsLayout::Settings::SearchPathForTemplates().setValue( QStringList() << "/path1" << "/path2" );

For settings with a dynamic key a placeholder can be inserted in the key:

struct Settings
{
  static const inline QgsSettingsEntryBool locatorFilterEnabled = QgsSettingsEntryBool( QStringLiteral( "locator_filters/enabled_%" ), QgsSettings::Gui, true, "Enabled" );
  static const inline QgsSettingsEntryBool locatorFilterDefault = QgsSettingsEntryBool( QStringLiteral( "locator_filters/default_%" ), QgsSettings::Gui, false, "Default value" );
  static const inline QgsSettingsEntryString locatorFilterPrefix = QgsSettingsEntryString( QStringLiteral( "locator_filters/prefix_%" ), QgsSettings::Gui, QString(), "Locator filter prefix" );
};

and can be accessed providing the dynamic part of the key as argument:

QgsLocator::Settings::locatorFilterEnabled.setValue( it.value(), filter->name() );

The settings registry in this solution would be needed only for introspection and it is simply implemented as a list of QgsSettingsEntry instances. Each module or plugin would have his own registry and add the settings to the main registry on load/startup.

This approach is more flexible as the one proposed in pr #41830 as it permit to keep the key and section as they are now. No migration will be needed as result of this work. Nonetheless it addresses all the issues exposed in the QEP.

@domi4484
Copy link
Contributor Author

@elpaso I removed the macro in favor of defining the settings as static members. The macro was also causing troubles on the sip part.

@github-actions github-actions bot added this to the 3.20.0 milestone Mar 31, 2021
@domi4484 domi4484 force-pushed the settingsRegistryStaticInline branch from 6007d47 to d16702d Compare April 1, 2021 08:17
@3nids 3nids changed the title Settings registry static inline Settings registry (QEP 124) Apr 2, 2021
@3nids
Copy link
Member

3nids commented Apr 2, 2021

We propose to merge this PR as a first step, as it is much easier to review.
Remains for a follow-up PR:

  • adding Python tests
  • migrate settings in core (maybe not all)
  • registry introspection
  • add API to register registries, so that the core registry would be able to list all settings
  • adding a short doc to the coding guidelines

@nyalldawson
Copy link
Collaborator

I will review over this weekend...

src/app/locator/qgslocatoroptionswidget.cpp Show resolved Hide resolved
src/core/layout/qgslayout.h Outdated Show resolved Hide resolved
bool byDefault = settings.value( QStringLiteral( "locator_filters/default_%1" ).arg( filter->name() ), filter->useWithoutPrefix(), QgsSettings::Section::Gui ).toBool();
QString prefix = settings.value( QStringLiteral( "locator_filters/prefix_%1" ).arg( filter->name() ), filter->prefix(), QgsSettings::Section::Gui ).toString();
bool enabled = true;
if ( QgsLocator::Settings::locatorFilterEnabled.exists( filter->name() ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we skip all the exists checks here by instead returning the default value when value() is called and the setting doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the enable yes. For the other two i think it is not possible because for example filter->prefix() would return a different value for each subclass of QgsLocatorFilter. QgsSettingsEntry support only one default value passed to the constructor.
Maybe it would be possible to add a second paramete to value( dynamicKey, defaultValueOverride ) or similar. defaultValueOverride would be taken in place of the "normal" default value when is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameters to override the default value added in commit abb8e80

src/core/locator/qgslocator.h Outdated Show resolved Hide resolved
src/core/settings/qgssettingsregistrycore.h Show resolved Hide resolved
Copy link
Member

@3nids 3nids 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, this looks really good.
some minor comments.

python/core/additions/qgssettingsentry.py Outdated Show resolved Hide resolved
src/app/options/qgsoptions.cpp Outdated Show resolved Hide resolved
src/core/layout/qgslayout.h Outdated Show resolved Hide resolved
src/core/locator/qgslocator.h Outdated Show resolved Hide resolved
{
QStringList dynamicKeyPartList;
if ( !dynamicKeyPart.isEmpty() )
dynamicKeyPartList.append( dynamicKeyPart );
Copy link
Member

Choose a reason for hiding this comment

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

isn't an empty string already a value?
this will not cause an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be an issue if someone wants to just remove the dynamic part by replacing it with an empty string. I will change the check with isNull()

@3nids
Copy link
Member

3nids commented Apr 15, 2021

Finally there :) Thanks for the amazing job and great inputs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants