Skip to content
Permalink
Browse files
Applied suggestions from code review
  • Loading branch information
domi4484 committed Apr 20, 2021
1 parent a693ebc commit e99b92a92cb2194ef90e2e115cedf6c78793cc09
@@ -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
@@ -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
@@ -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`.
@@ -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

};
@@ -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();
}
@@ -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 );
}

@@ -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 );
}

@@ -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.
@@ -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.
*/
@@ -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;
}

@@ -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 )
@@ -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;
}

@@ -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.
*/
@@ -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;
@@ -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
@@ -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 );
@@ -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 );
@@ -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 );

0 comments on commit e99b92a

Please sign in to comment.