From e9cd82902b035881af9df87a468753a7cb26e5a6 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Fri, 18 Dec 2015 16:02:49 +1100 Subject: [PATCH] Fix incorrect symbols appearing for categorized and graduated renderers This was triggered by 18614e11, 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. --- .../symbology-ng/qgscategorizedsymbolrendererv2.cpp | 11 ++++++----- .../symbology-ng/qgsgraduatedsymbolrendererv2.cpp | 10 +++++++++- src/core/symbology-ng/qgsgraduatedsymbolrendererv2.h | 1 - tests/src/python/test_qgsrulebasedrenderer.py | 1 - 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/core/symbology-ng/qgscategorizedsymbolrendererv2.cpp b/src/core/symbology-ng/qgscategorizedsymbolrendererv2.cpp index c7f95134e09..fc9af983ea1 100644 --- 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 index c14a1a02529..772a8874fe3 100644 --- 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 index 85ec3f0fa84..adc0e7a7505 100644 --- 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 index 91d37f75029..d5df0247c73 100644 --- a/tests/src/python/test_qgsrulebasedrenderer.py +++ b/tests/src/python/test_qgsrulebasedrenderer.py @@ -138,7 +138,6 @@ class TestQgsRulebasedRenderer(TestCase): 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)