Cleanups, fix tests

qgis · Jan 23, 2017 · 2966a16 · 2966a16
1 parent 9522f31
commit 2966a16
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 69 deletions.
diff --git a/src/core/qgsproperty.cpp b/src/core/qgsproperty.cpp
@@ -236,31 +236,31 @@ void QgsProperty::setStaticValue( const QVariant& value )
 {
   d.detach();
   d->type = StaticProperty;
-  d->staticData.value = value;
+  d->staticValue = value;
 }
 
 QVariant QgsProperty::staticValue() const
 {
   if ( d->type != StaticProperty )
     return QVariant();
 
-  return d->staticData.value;
+  return d->staticValue;
 }
 
 void QgsProperty::setField( const QString& field )
 {
   d.detach();
   d->type = FieldBasedProperty;
-  d->fieldData.fieldName = field;
-  d->fieldData.cachedFieldIdx = -1;
+  d->fieldName = field;
+  d->cachedFieldIdx = -1;
 }
 
 QString QgsProperty::field() const
 {
   if ( d->type != FieldBasedProperty )
     return QString();
 
-  return d->fieldData.fieldName;
+  return d->fieldName;
 }
 
 QgsProperty::operator bool() const
@@ -272,17 +272,17 @@ void QgsProperty::setExpressionString( const QString& expression )
 {
   d.detach();
   d->type = ExpressionBasedProperty;
-  d->expressionData.expressionString = expression;
-  d->expressionData.expression = QgsExpression( expression );
-  d->expressionData.prepared = false;
+  d->expressionString = expression;
+  d->expression = QgsExpression( expression );
+  d->expressionPrepared = false;
 }
 
 QString QgsProperty::expressionString() const
 {
   if ( d->type != ExpressionBasedProperty )
     return QString();
 
-  return d->expressionData.expressionString;
+  return d->expressionString;
 }
 
 
@@ -291,13 +291,13 @@ QString QgsProperty::asExpression() const
   switch ( d->type )
   {
     case StaticProperty:
-      return QgsExpression::quotedValue( d->staticData.value );
+      return QgsExpression::quotedValue( d->staticValue );
 
     case FieldBasedProperty:
-      return QgsExpression::quotedColumnRef( d->fieldData.fieldName );
+      return QgsExpression::quotedColumnRef( d->fieldName );
 
     case ExpressionBasedProperty:
-      return d->expressionData.expressionString;
+      return d->expressionString;
 
     case InvalidProperty:
       return QString();
@@ -320,22 +320,22 @@ bool QgsProperty::prepare( const QgsExpressionContext& context ) const
       d.detach();
       // cache field index to avoid subsequent lookups
       QgsFields f = context.fields();
-      d->fieldData.cachedFieldIdx = f.lookupField( d->fieldData.fieldName );
+      d->cachedFieldIdx = f.lookupField( d->fieldName );
       return true;
     }
 
     case ExpressionBasedProperty:
     {
       d.detach();
-      if ( !d->expressionData.expression.prepare( &context ) )
+      if ( !d->expression.prepare( &context ) )
       {
-        d->expressionData.referencedCols.clear();
-        d->expressionData.prepared = false;
+        d->expressionReferencedCols.clear();
+        d->expressionPrepared = false;
         return false;
       }
 
-      d->expressionData.prepared = true;
-      d->expressionData.referencedCols = d->expressionData.expression.referencedColumns();
+      d->expressionPrepared = true;
+      d->expressionReferencedCols = d->expression.referencedColumns();
       return true;
     }
 
@@ -361,18 +361,18 @@ QSet<QString> QgsProperty::referencedFields( const QgsExpressionContext& context
     case FieldBasedProperty:
     {
       QSet< QString > fields;
-      if ( !d->fieldData.fieldName.isEmpty() )
-        fields.insert( d->fieldData.fieldName );
+      if ( !d->fieldName.isEmpty() )
+        fields.insert( d->fieldName );
       return fields;
     }
 
     case ExpressionBasedProperty:
     {
       d.detach();
-      if ( !d->expressionData.prepared && !prepare( context ) )
+      if ( !d->expressionPrepared && !prepare( context ) )
         return QSet< QString >();
 
-      return d->expressionData.referencedCols;
+      return d->expressionReferencedCols;
     }
 
   }
@@ -393,7 +393,7 @@ QVariant QgsProperty::propertyValue( const QgsExpressionContext& context, const
     {
       if ( ok )
         *ok = true;
-      return d->staticData.value;
+      return d->staticValue;
     }
 
     case FieldBasedProperty:
@@ -403,14 +403,14 @@ QVariant QgsProperty::propertyValue( const QgsExpressionContext& context, const
         return defaultValue;
 
       //shortcut the field lookup
-      if ( d->fieldData.cachedFieldIdx >= 0 )
+      if ( d->cachedFieldIdx >= 0 )
       {
         if ( ok )
           *ok = true;
-        return f.attribute( d->fieldData.cachedFieldIdx );
+        return f.attribute( d->cachedFieldIdx );
       }
 
-      int fieldIdx = f.fieldNameIndex( d->fieldData.fieldName );
+      int fieldIdx = f.fieldNameIndex( d->fieldName );
       if ( fieldIdx < 0 )
         return defaultValue;
 
@@ -422,10 +422,10 @@ QVariant QgsProperty::propertyValue( const QgsExpressionContext& context, const
     case ExpressionBasedProperty:
     {
       d.detach();
-      if ( !d->expressionData.prepared && !prepare( context ) )
+      if ( !d->expressionPrepared && !prepare( context ) )
         return defaultValue;
 
-      QVariant result = d->expressionData.expression.evaluate( &context );
+      QVariant result = d->expression.evaluate( &context );
       if ( result.isValid() )
       {
         if ( ok )
@@ -601,16 +601,16 @@ bool QgsProperty::writeXml( QDomElement &propertyElem, QDomDocument &doc ) const
   switch ( d->type )
   {
     case StaticProperty:
-      propertyElem.setAttribute( "valType", d->staticData.value.typeName() );
-      propertyElem.setAttribute( "val", d->staticData.value.toString() );
+      propertyElem.setAttribute( "valType", d->staticValue.typeName() );
+      propertyElem.setAttribute( "val", d->staticValue.toString() );
       break;
 
     case FieldBasedProperty:
-      propertyElem.setAttribute( "field", d->fieldData.fieldName );
+      propertyElem.setAttribute( "field", d->fieldName );
       break;
 
     case ExpressionBasedProperty:
-      propertyElem.setAttribute( "expression", d->expressionData.expressionString );
+      propertyElem.setAttribute( "expression", d->expressionString );
       break;
 
     case InvalidProperty:
@@ -637,24 +637,24 @@ bool QgsProperty::readXml( const QDomElement &propertyElem, const QDomDocument &
   switch ( d->type )
   {
     case StaticProperty:
-      d->staticData.value = QVariant( propertyElem.attribute( "val", "" ) );
-      d->staticData.value.convert( QVariant::nameToType( propertyElem.attribute( "valType", "QString" ).toLocal8Bit().constData() ) );
+      d->staticValue = QVariant( propertyElem.attribute( "val", "" ) );
+      d->staticValue.convert( QVariant::nameToType( propertyElem.attribute( "valType", "QString" ).toLocal8Bit().constData() ) );
       break;
 
     case FieldBasedProperty:
-      d->fieldData.fieldName = propertyElem.attribute( "field" );
-      if ( d->fieldData.fieldName.isEmpty() )
+      d->fieldName = propertyElem.attribute( "field" );
+      if ( d->fieldName.isEmpty() )
         d->active = false;
       break;
 
     case ExpressionBasedProperty:
-      d->expressionData.expressionString = propertyElem.attribute( "expression" );
-      if ( d->expressionData.expressionString.isEmpty() )
+      d->expressionString = propertyElem.attribute( "expression" );
+      if ( d->expressionString.isEmpty() )
         d->active = false;
 
-      d->expressionData.expression = QgsExpression( d->expressionData.expressionString );
-      d->expressionData.prepared = false;
-      d->expressionData.referencedCols.clear();
+      d->expression = QgsExpression( d->expressionString );
+      d->expressionPrepared = false;
+      d->expressionReferencedCols.clear();
       break;
 
     case InvalidProperty:

diff --git a/src/core/qgsproperty_p.h b/src/core/qgsproperty_p.h
@@ -36,16 +36,24 @@ class QgsPropertyPrivate : public QSharedData
 {
   public:
 
-    QgsPropertyPrivate() = default;
+    QgsPropertyPrivate()
+        : type( 0 )
+        , active( true )
+        , transformer( nullptr )
+    {}
 
     QgsPropertyPrivate( const QgsPropertyPrivate& other )
         : QSharedData( other )
         , type( other.type )
         , active( other.active )
         , transformer( other.transformer ? other.transformer->clone() : nullptr )
-        , staticData( other.staticData )
-        , fieldData( other.fieldData )
-        , expressionData( other.expressionData )
+        , staticValue( other.staticValue )
+        , fieldName( other.fieldName )
+        , cachedFieldIdx( other.cachedFieldIdx )
+        , expressionString( other.expressionString )
+        , expressionPrepared( other.expressionPrepared )
+        , expression( other.expression )
+        , expressionReferencedCols( other.expressionReferencedCols )
     {}
 
     ~QgsPropertyPrivate()
@@ -61,27 +69,19 @@ class QgsPropertyPrivate : public QSharedData
     //! Optional transfomer
     QgsPropertyTransformer* transformer = nullptr;
 
-    struct StaticData
-    {
-      QVariant value;
-    };
-    struct FieldData
-    {
-      QString fieldName;
-      mutable int cachedFieldIdx = -1;
-    };
-    struct ExpressionData
-    {
-      QString expressionString;
-      mutable bool prepared = false;
-      mutable QgsExpression expression;
-      //! Cached set of referenced columns
-      mutable QSet< QString > referencedCols;
-    };
-
-    StaticData staticData;
-    FieldData fieldData;
-    ExpressionData expressionData;
+    // StaticData
+    QVariant staticValue;
+
+    // FieldData
+    QString fieldName;
+    mutable int cachedFieldIdx = -1;
+
+    // ExpressionData
+    QString expressionString;
+    mutable bool expressionPrepared = false;
+    mutable QgsExpression expression;
+    //! Cached set of referenced columns
+    mutable QSet< QString > expressionReferencedCols;
 
 };
 

diff --git a/src/core/qgspropertycollection.cpp b/src/core/qgspropertycollection.cpp
@@ -129,7 +129,7 @@ void QgsPropertyCollection::clear()
 void QgsPropertyCollection::setProperty( int key, const QgsProperty& property )
 {
   if ( property )
-    mProperties[ key ] = property;
+    mProperties.insert( key, property );
   else
     mProperties.remove( key );
 

diff --git a/tests/src/core/testqgsproperty.cpp b/tests/src/core/testqgsproperty.cpp
@@ -73,6 +73,7 @@ class TestQgsProperty : public QObject
     void init();// will be called before each testfunction is executed.
     void cleanup();// will be called after every testfunction.
     void conversions(); //test QgsProperty static conversion methods
+    void invalid(); //test invalid properties
     void staticProperty(); //test for QgsStaticProperty
     void fieldBasedProperty(); //test for QgsFieldBasedProperty
     void expressionBasedProperty(); //test for QgsExpressionBasedProperty
@@ -243,6 +244,18 @@ void TestQgsProperty::conversions()
   QCOMPARE( collection.valueAsString( 4, context , "y" ), QString( "s" ) );
 }
 
+void TestQgsProperty::invalid()
+{
+  QgsProperty p; //invalid property
+  QCOMPARE( p.propertyType(), QgsProperty::InvalidProperty );
+  QgsProperty p2( p );
+  QCOMPARE( p2.propertyType(), QgsProperty::InvalidProperty );
+  QgsProperty p3 = QgsProperty::fromValue( 5 );
+  p3 = p;
+  QCOMPARE( p3.propertyType(), QgsProperty::InvalidProperty );
+
+}
+
 void TestQgsProperty::staticProperty()
 {
   QgsExpressionContext context;

diff --git a/tests/src/python/test_qgspallabeling_placement.py b/tests/src/python/test_qgspallabeling_placement.py
@@ -235,10 +235,10 @@ def test_point_dd_ordered_placement(self):
         self._TestMapSettings = self.cloneMapSettings(self._MapSettings)
         self.lyr.placement = QgsPalLayerSettings.OrderedPositionsAroundPoint
         self.lyr.dist = 2
-        self.lyr.properties().setProperty(QgsPalLayerSettings.PredefinedPositionOrder, QgsProperty.fromExpression("'T,B'"))
+        #self.lyr.properties().setProperty(QgsPalLayerSettings.PredefinedPositionOrder, QgsProperty.fromExpression("'T,B'"))
         self.checkTest()
         self.removeMapLayer(self.layer)
-        self.lyr.properties().setProperty(QgsPalLayerSettings.PredefinedPositionOrder, QgsProperty())
+        #self.lyr.properties().setProperty(QgsPalLayerSettings.PredefinedPositionOrder, QgsProperty())
         self.layer = None
 
     def test_point_dd_ordered_placement1(self):