Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Applied suggestions from code review
  • Loading branch information
domi4484 committed Apr 20, 2021
1 parent a693ebc commit e99b92a
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 49 deletions.
14 changes: 9 additions & 5 deletions python/core/auto_generated/settings/qgssettingsentry.sip.in
Expand Up @@ -84,9 +84,13 @@ Get settings entry key.
The ``dynamicKeyParts`` argument specifies the list of dynamic parts of the settings key.
%End

bool checkKey( const QString &key ) const;
bool keyIsValid( const QString &key ) const;
%Docstring
Returns true if the provided key match the settings entry
Returns ``True`` if the provided key match the settings entry.

This is useful for settings with dynamic keys. For example this permits to check that
the settings key "NewsFeed/httpsfeedqgisorg/27/content" is valid for the settings entry
defined with the key "NewsFeed/%1/%2/content"

The ``key`` to check
%End
Expand All @@ -100,19 +104,19 @@ included. For non-dynamic settings returns the same as :py:func:`~QgsSettingsEnt

bool hasDynamicKey() const;
%Docstring
Returns true if a part of the settings key is built dynamically.
Returns ``True`` if a part of the settings key is built dynamically.
%End

bool exists( const QString &dynamicKeyPart = QString() ) const;
%Docstring
Returns true if the settings is contained in the underlying QSettings.
Returns ``True`` if the settings is contained in the underlying QSettings.

The ``dynamicKeyPart`` argument specifies the dynamic part of the settings key.
%End

bool exists( const QStringList &dynamicKeyPartList ) const;
%Docstring
Returns true if the settings is contained in the underlying QSettings.
Returns ``True`` if the settings is contained in the underlying QSettings.

The ``dynamicKeyParts`` argument specifies the list of dynamic parts of the settings key.
%End
Expand Down
12 changes: 7 additions & 5 deletions python/core/auto_generated/settings/qgssettingsregistry.sip.in
Expand Up @@ -31,11 +31,6 @@ Constructor for QgsSettingsRegistry.

virtual ~QgsSettingsRegistry();

void addSettingsEntry( const QgsSettingsEntryBase *settingsEntry );
%Docstring
Add ``settingsEntry`` to the register.
%End

QList<const QgsSettingsEntryBase *> getChildSettingsEntries() const;
%Docstring
Returns the list of registered :py:class:`QgsSettingsEntryBase`.
Expand All @@ -56,6 +51,13 @@ Add a child ``settingsRegistry`` to the register.
QList<const QgsSettingsRegistry *> getChildSettingsRegistries() const;
%Docstring
Returns the list of registered child QgsSettingsRegistry.
%End

protected:

void addSettingsEntry( const QgsSettingsEntryBase *settingsEntry );
%Docstring
Add ``settingsEntry`` to the register.
%End

};
Expand Down
34 changes: 17 additions & 17 deletions src/core/settings/qgssettingsentry.cpp
Expand Up @@ -55,69 +55,69 @@ QString QgsSettingsEntryBase::key( const QStringList &dynamicKeyPartList ) const
QString completeKey = mKey;
if ( !mPluginName.isEmpty() )
{
if ( !completeKey.startsWith( "/" ) )
completeKey.prepend( "/" );
if ( !completeKey.startsWith( '/' ) )
completeKey.prepend( '/' );
completeKey.prepend( mPluginName );
}

if ( dynamicKeyPartList.isEmpty() )
{
if ( hasDynamicKey() )
QgsLogger::warning( QStringLiteral( "Settings '%1' have a dynamic key but the dynamic key part was not provided" ).arg( completeKey ) );
QgsDebugMsg( QStringLiteral( "Settings '%1' have a dynamic key but the dynamic key part was not provided" ).arg( completeKey ) );

return completeKey;
}
else
{
if ( !hasDynamicKey() )
{
QgsLogger::warning( QStringLiteral( "Settings '%1' don't have a dynamic key, the provided dynamic key part will be ignored" ).arg( completeKey ) );
QgsDebugMsg( QStringLiteral( "Settings '%1' don't have a dynamic key, the provided dynamic key part will be ignored" ).arg( completeKey ) );
return completeKey;
}

for ( int i = 0; i < dynamicKeyPartList.size(); i++ )
{
completeKey.replace( QString( "%%1" ).arg( QString::number( i + 1 ) ), dynamicKeyPartList.at( i ) );
completeKey.replace( QStringLiteral( "%%1" ).arg( QString::number( i + 1 ) ), dynamicKeyPartList.at( i ) );
}
}
return completeKey;
}

bool QgsSettingsEntryBase::checkKey( const QString &key ) const
bool QgsSettingsEntryBase::keyIsValid( const QString &key ) const
{
// Key to check
QString completeKeyToCheck = key;

QString settingsPrefix = QgsSettings().prefixedKey( "", section() );
QString settingsPrefix = QgsSettings().prefixedKey( QString(), section() );
settingsPrefix.chop( 1 );
if ( !completeKeyToCheck.startsWith( settingsPrefix ) )
{
if ( !mPluginName.isEmpty()
&& !completeKeyToCheck.startsWith( mPluginName ) )
{
if ( !completeKeyToCheck.startsWith( "/" ) )
completeKeyToCheck.prepend( "/" );
if ( !completeKeyToCheck.startsWith( '/' ) )
completeKeyToCheck.prepend( '/' );
completeKeyToCheck.prepend( mPluginName );
}

if ( !completeKeyToCheck.startsWith( "/" ) )
completeKeyToCheck.prepend( "/" );
if ( !completeKeyToCheck.startsWith( '/' ) )
completeKeyToCheck.prepend( '/' );
completeKeyToCheck.prepend( settingsPrefix );
}

// Prefixed settings key
QString prefixedSettingsKey = definitionKey();
if ( !prefixedSettingsKey.startsWith( settingsPrefix ) )
{
if ( !prefixedSettingsKey.startsWith( "/" ) )
prefixedSettingsKey.prepend( "/" );
if ( !prefixedSettingsKey.startsWith( '/' ) )
prefixedSettingsKey.prepend( '/' );
prefixedSettingsKey.prepend( settingsPrefix );
}

if ( !hasDynamicKey() )
return completeKeyToCheck == prefixedSettingsKey;

QRegularExpression regularExpression( prefixedSettingsKey.replace( QRegularExpression( "%\\d+" ), ".*" ) );
QRegularExpression regularExpression( prefixedSettingsKey.replace( QRegularExpression( QStringLiteral( "%\\d+" ) ), QStringLiteral( ".*" ) ) );
QRegularExpressionMatch regularExpresisonMatch = regularExpression.match( completeKeyToCheck );
return regularExpresisonMatch.hasMatch();
}
Expand All @@ -127,8 +127,8 @@ QString QgsSettingsEntryBase::definitionKey() const
QString completeKey = mKey;
if ( !mPluginName.isEmpty() )
{
if ( !completeKey.startsWith( "/" ) )
completeKey.prepend( "/" );
if ( !completeKey.startsWith( '/' ) )
completeKey.prepend( '/' );
completeKey.prepend( mPluginName );
}

Expand All @@ -137,7 +137,7 @@ QString QgsSettingsEntryBase::definitionKey() const

bool QgsSettingsEntryBase::hasDynamicKey() const
{
static const QRegularExpression regularExpression( "%\\d+" );
static const QRegularExpression regularExpression( QStringLiteral( "%\\d+" ) );
return mKey.contains( regularExpression );
}

Expand Down
14 changes: 9 additions & 5 deletions src/core/settings/qgssettingsentry.h
Expand Up @@ -120,11 +120,15 @@ class CORE_EXPORT QgsSettingsEntryBase
QString key( const QStringList &dynamicKeyPartList ) const;

/**
* Returns true if the provided key match the settings entry
* Returns TRUE if the provided key match the settings entry.
*
* This is useful for settings with dynamic keys. For example this permits to check that
* the settings key "NewsFeed/httpsfeedqgisorg/27/content" is valid for the settings entry
* defined with the key "NewsFeed/%1/%2/content"
*
* The \a key to check
*/
bool checkKey( const QString &key ) const;
bool keyIsValid( const QString &key ) const;

/**
* Returns settings entry defining key.
Expand All @@ -134,19 +138,19 @@ class CORE_EXPORT QgsSettingsEntryBase
QString definitionKey() const;

/**
* Returns true if a part of the settings key is built dynamically.
* Returns TRUE if a part of the settings key is built dynamically.
*/
bool hasDynamicKey() const;

/**
* Returns true if the settings is contained in the underlying QSettings.
* Returns TRUE if the settings is contained in the underlying QSettings.
*
* The \a dynamicKeyPart argument specifies the dynamic part of the settings key.
*/
bool exists( const QString &dynamicKeyPart = QString() ) const;

/**
* Returns true if the settings is contained in the underlying QSettings.
* Returns TRUE if the settings is contained in the underlying QSettings.
*
* The \a dynamicKeyParts argument specifies the list of dynamic parts of the settings key.
*/
Expand Down
16 changes: 8 additions & 8 deletions src/core/settings/qgssettingsregistry.cpp
Expand Up @@ -37,15 +37,15 @@ QgsSettingsRegistry::~QgsSettingsRegistry()

void QgsSettingsRegistry::addSettingsEntry( const QgsSettingsEntryBase *settingsEntry )
{
if ( settingsEntry == nullptr )
if ( !settingsEntry )
{
QgsLogger::warning( QStringLiteral( "Trying to register a nullptr settings entry." ) );
QgsDebugMsg( QStringLiteral( "Trying to register a nullptr settings entry." ) );
return;
}

if ( mSettingsEntriesMap.contains( settingsEntry->definitionKey() ) )
{
QgsLogger::warning( QStringLiteral( "Settings with key '%1' is already registered." ).arg( settingsEntry->definitionKey() ) );
QgsDebugMsg( QStringLiteral( "Settings with key '%1' is already registered." ).arg( settingsEntry->definitionKey() ) );
return;
}

Expand All @@ -63,14 +63,14 @@ const QgsSettingsEntryBase *QgsSettingsRegistry::getSettingsEntry( const QString
const QMap<QString, const QgsSettingsEntryBase *> settingsEntriesMap = mSettingsEntriesMap;
for ( const QgsSettingsEntryBase *settingsEntry : settingsEntriesMap )
{
if ( settingsEntry->checkKey( key ) )
if ( settingsEntry->keyIsValid( key ) )
return settingsEntry;
}

// Search in child registries
if ( searchChildRegistries )
{
for ( const QgsSettingsRegistry *settingsRegistry : mSettingsRegistryChildList )
for ( const QgsSettingsRegistry *settingsRegistry : std::as_const( mSettingsRegistryChildList ) )
{
const QgsSettingsEntryBase *settingsEntry = settingsRegistry->getSettingsEntry( key, true );
if ( settingsEntry != nullptr )
Expand All @@ -83,15 +83,15 @@ const QgsSettingsEntryBase *QgsSettingsRegistry::getSettingsEntry( const QString

void QgsSettingsRegistry::addChildSettingsRegistry( const QgsSettingsRegistry *settingsRegistry )
{
if ( settingsRegistry == nullptr )
if ( !settingsRegistry )
{
QgsLogger::warning( QStringLiteral( "Trying to register a nullptr child settings registry." ) );
QgsDebugMsg( QStringLiteral( "Trying to register a nullptr child settings registry." ) );
return;
}

if ( mSettingsRegistryChildList.contains( settingsRegistry ) )
{
QgsLogger::warning( QStringLiteral( "Child register is already registered." ) );
QgsDebugMsg( QStringLiteral( "Child register is already registered." ) );
return;
}

Expand Down
12 changes: 7 additions & 5 deletions src/core/settings/qgssettingsregistry.h
Expand Up @@ -45,11 +45,6 @@ class CORE_EXPORT QgsSettingsRegistry
*/
virtual ~QgsSettingsRegistry();

/**
* Add \a settingsEntry to the register.
*/
void addSettingsEntry( const QgsSettingsEntryBase *settingsEntry );

/**
* Returns the list of registered QgsSettingsEntryBase.
*/
Expand All @@ -72,6 +67,13 @@ class CORE_EXPORT QgsSettingsRegistry
*/
QList<const QgsSettingsRegistry *> getChildSettingsRegistries() const;

protected:

/**
* Add \a settingsEntry to the register.
*/
void addSettingsEntry( const QgsSettingsEntryBase *settingsEntry );

private:

QMap<QString, const QgsSettingsEntryBase *> mSettingsEntriesMap;
Expand Down
20 changes: 16 additions & 4 deletions tests/src/core/testqgssettingsregistry.cpp
Expand Up @@ -20,6 +20,18 @@
#include "qgsmaplayerproxymodel.h"
#include "qgstest.h"

/**
* This is a helper class to test protected methods of QgsSettingsRegistry
*/
class SettingsRegistryTest : public QgsSettingsRegistry
{
public:

void addSettingsEntry( const QgsSettingsEntryBase *settingsEntry )
{
QgsSettingsRegistry::addSettingsEntry( settingsEntry );
}
};

/**
* \ingroup UnitTests
Expand All @@ -44,7 +56,7 @@ void TestQgsSettingsRegistry::getSettingsEntries()

QString settingsEntryInexisting( "/qgis/testing/settingsEntryInexisting" );

QgsSettingsRegistry settingsRegistry;
SettingsRegistryTest settingsRegistry;
settingsRegistry.addSettingsEntry( nullptr ); // should not crash
settingsRegistry.addSettingsEntry( &settingsEntryBool );
settingsRegistry.addSettingsEntry( &settingsEntryInteger );
Expand All @@ -65,7 +77,7 @@ void TestQgsSettingsRegistry::getSettingsEntriesWithDynamicKeys()

QString settingsEntryInexisting( "/qgis/testing/settingsEntryInexisting%1" );

QgsSettingsRegistry settingsRegistry;
SettingsRegistryTest settingsRegistry;
settingsRegistry.addSettingsEntry( &settingsEntryBool );
settingsRegistry.addSettingsEntry( &settingsEntryInteger );
settingsRegistry.addSettingsEntry( &settingsEntryDouble );
Expand All @@ -86,10 +98,10 @@ void TestQgsSettingsRegistry::childRegistry()
QString settingsEntryIntegerKey( "/qgis/testing/settingsEntryInteger" );
QgsSettingsEntryInteger settingsEntryInteger( settingsEntryIntegerKey, QgsSettings::NoSection, 123 );

QgsSettingsRegistry settingsRegistryChild;
SettingsRegistryTest settingsRegistryChild;
settingsRegistryChild.addSettingsEntry( &settingsEntryInteger );

QgsSettingsRegistry settingsRegistry;
SettingsRegistryTest settingsRegistry;
settingsRegistry.addSettingsEntry( &settingsEntryBool );
settingsRegistry.addChildSettingsRegistry( nullptr ); // should not crash
settingsRegistry.addChildSettingsRegistry( &settingsRegistryChild );
Expand Down

0 comments on commit e99b92a

Please sign in to comment.