Fix incorrect symbols appearing for categorized and graduated renderers

This was triggered by 18614e1, which revealed a deeper bug in the handling of symbols by these renderers. For both renderers a const method returns a non-const pointer to a symbol and is used to modify this symbol in place. Because both renderers store their classes in a QList (implicitly shared) this means that all shared class lists were being updated when a clone of a renderer has its symbols altered in this way. Prior to 18614e these class QLists were being detached and copied in random ways. Work around this by forcing a deep copy of the class lists when a renderer is cloned.
qgis · Dec 18, 2015 · e9cd829 · e9cd829
1 parent 6a973d6
commit e9cd829
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 8 deletions.
diff --git a/src/core/symbology-ng/qgscategorizedsymbolrendererv2.cpp b/src/core/symbology-ng/qgscategorizedsymbolrendererv2.cpp
@@ -149,20 +149,21 @@ void QgsRendererCategoryV2::toSld( QDomDocument &doc, QDomElement &element, QgsS
 QgsCategorizedSymbolRendererV2::QgsCategorizedSymbolRendererV2( const QString& attrName, const QgsCategoryList& categories )
     : QgsFeatureRendererV2( "categorizedSymbol" )
     , mAttrName( attrName )
-    , mCategories( categories )
     , mInvertedColorRamp( false )
     , mScaleMethod( DEFAULT_SCALE_METHOD )
     , mAttrNum( -1 )
     , mCounting( false )
 {
-  for ( int i = 0; i < mCategories.count(); ++i )
+  //important - we need a deep copy of the categories list, not a shared copy. This is required because
+  //QgsRendererCategoryV2::symbol() is marked const, and so retrieving the symbol via this method does not
+  //trigger a detachment and copy of mCategories BUT that same method CAN be used to modify a symbol in place
+  Q_FOREACH ( const QgsRendererCategoryV2& cat, categories )
   {
-    if ( !mCategories.at( i ).symbol() )
+    if ( cat.symbol() )
     {
       QgsDebugMsg( "invalid symbol in a category! ignoring..." );
-      mCategories.removeAt( i-- );
     }
-    //mCategories.insert(cat.value().toString(), cat);
+    mCategories << cat;
   }
 }
 

diff --git a/src/core/symbology-ng/qgsgraduatedsymbolrendererv2.cpp b/src/core/symbology-ng/qgsgraduatedsymbolrendererv2.cpp
@@ -284,7 +284,6 @@ void QgsRendererRangeV2LabelFormat::saveToDomElement( QDomElement &element )
 QgsGraduatedSymbolRendererV2::QgsGraduatedSymbolRendererV2( const QString& attrName, const QgsRangeList& ranges )
     : QgsFeatureRendererV2( "graduatedSymbol" )
     , mAttrName( attrName )
-    , mRanges( ranges )
     , mMode( Custom )
     , mInvertedColorRamp( false )
     , mScaleMethod( DEFAULT_SCALE_METHOD )
@@ -294,6 +293,15 @@ QgsGraduatedSymbolRendererV2::QgsGraduatedSymbolRendererV2( const QString& attrN
 
 {
   // TODO: check ranges for sanity (NULL symbols, invalid ranges)
+
+  //important - we need a deep copy of the ranges list, not a shared copy. This is required because
+  //QgsRendererRangeV2::symbol() is marked const, and so retrieving the symbol via this method does not
+  //trigger a detachment and copy of mRanges BUT that same method CAN be used to modify a symbol in place
+  Q_FOREACH ( const QgsRendererRangeV2& range, ranges )
+  {
+    mRanges << range;
+  }
+
 }
 
 QgsGraduatedSymbolRendererV2::~QgsGraduatedSymbolRendererV2()

diff --git a/src/core/symbology-ng/qgsgraduatedsymbolrendererv2.h b/src/core/symbology-ng/qgsgraduatedsymbolrendererv2.h
@@ -116,7 +116,6 @@ class CORE_EXPORT QgsGraduatedSymbolRendererV2 : public QgsFeatureRendererV2
   public:
 
     QgsGraduatedSymbolRendererV2( const QString& attrName = QString(), const QgsRangeList& ranges = QgsRangeList() );
-    QgsGraduatedSymbolRendererV2( const QgsGraduatedSymbolRendererV2 & other );
 
     virtual ~QgsGraduatedSymbolRendererV2();
 

diff --git a/tests/src/python/test_qgsrulebasedrenderer.py b/tests/src/python/test_qgsrulebasedrenderer.py
@@ -138,7 +138,6 @@ def testRefineWithCategories(self):
         assert self.r3.children()[0].filterExpression() == '"id" = 1'
         assert self.r3.children()[1].filterExpression() == '"id" = 2'
 
-    @unittest.skip("temporarily disabled")
     def testRefineWithRanges(self):
         # Test refining rule with ranges (refs #10815)