Skip to content
Permalink
Browse files

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.
  • Loading branch information
nyalldawson committed Dec 18, 2015
1 parent 6a973d6 commit e9cd82902b035881af9df87a468753a7cb26e5a6
@@ -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;
}
}

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

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

0 comments on commit e9cd829

Please sign in to comment.
You can’t perform that action at this time.