Skip to content

Commit 7c0665a

Browse files
committedMar 15, 2018
Clearer ownership, docs for rule based renderer
1 parent 685adbf commit 7c0665a

File tree

4 files changed

+54
-47
lines changed

4 files changed

+54
-47
lines changed
 

‎python/core/symbology/qgsrulebasedrenderer.sip.in

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,12 @@ the rules and draws features with symbols from rules that match.
3939
struct RenderJob
4040
{
4141
RenderJob( QgsRuleBasedRenderer::FeatureToRender &_ftr, QgsSymbol *_s );
42+
4243
QgsRuleBasedRenderer::FeatureToRender &ftr;
44+
%Docstring
45+
Feature to render
46+
%End
47+
4348
QgsSymbol *symbol;
4449
};
4550

@@ -48,6 +53,7 @@ the rules and draws features with symbols from rules that match.
4853
explicit RenderLevel( int z );
4954
~RenderLevel();
5055
int zIndex;
56+
5157
QList<QgsRuleBasedRenderer::RenderJob *> jobs;
5258

5359

@@ -415,9 +421,6 @@ Check if this rule is an ELSE rule
415421
protected:
416422
void initFilter();
417423

418-
419-
420-
421424
private:
422425
Rule( const QgsRuleBasedRenderer::Rule &rh );
423426
};

‎src/core/symbology/qgsrulebasedrenderer.cpp

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,6 @@ QgsRuleBasedRenderer::Rule::Rule( QgsSymbol *symbol, int scaleMinDenom, int scal
5656

5757
QgsRuleBasedRenderer::Rule::~Rule()
5858
{
59-
delete mSymbol;
60-
delete mFilter;
6159
qDeleteAll( mChildren );
6260
// do NOT delete parent
6361
}
@@ -67,20 +65,17 @@ void QgsRuleBasedRenderer::Rule::initFilter()
6765
if ( mFilterExp.trimmed().compare( QLatin1String( "ELSE" ), Qt::CaseInsensitive ) == 0 )
6866
{
6967
mElseRule = true;
70-
delete mFilter;
71-
mFilter = nullptr;
68+
mFilter.reset();
7269
}
7370
else if ( mFilterExp.trimmed().isEmpty() )
7471
{
7572
mElseRule = false;
76-
delete mFilter;
77-
mFilter = nullptr;
73+
mFilter.reset();
7874
}
7975
else
8076
{
8177
mElseRule = false;
82-
delete mFilter;
83-
mFilter = new QgsExpression( mFilterExp );
78+
mFilter = qgis::make_unique< QgsExpression >( mFilterExp );
8479
}
8580
}
8681

@@ -157,8 +152,7 @@ void QgsRuleBasedRenderer::Rule::setIsElse( bool iselse )
157152
{
158153
mFilterExp = QStringLiteral( "ELSE" );
159154
mElseRule = iselse;
160-
delete mFilter;
161-
mFilter = nullptr;
155+
mFilter.reset();
162156
}
163157

164158

@@ -215,7 +209,7 @@ QgsSymbolList QgsRuleBasedRenderer::Rule::symbols( const QgsRenderContext &conte
215209
{
216210
QgsSymbolList lst;
217211
if ( mSymbol )
218-
lst.append( mSymbol );
212+
lst.append( mSymbol.get() );
219213

220214
Q_FOREACH ( Rule *rule, mChildren )
221215
{
@@ -226,8 +220,7 @@ QgsSymbolList QgsRuleBasedRenderer::Rule::symbols( const QgsRenderContext &conte
226220

227221
void QgsRuleBasedRenderer::Rule::setSymbol( QgsSymbol *sym )
228222
{
229-
delete mSymbol;
230-
mSymbol = sym;
223+
mSymbol.reset( sym );
231224
}
232225

233226
void QgsRuleBasedRenderer::Rule::setFilterExpression( const QString &filterExp )
@@ -241,7 +234,7 @@ QgsLegendSymbolList QgsRuleBasedRenderer::Rule::legendSymbolItems( int currentLe
241234
QgsLegendSymbolList lst;
242235
if ( currentLevel != -1 ) // root rule should not be shown
243236
{
244-
lst << QgsLegendSymbolItem( mSymbol, mLabel, mRuleKey, true, mMaximumScale, mMinimumScale, currentLevel, mParent ? mParent->mRuleKey : QString() );
237+
lst << QgsLegendSymbolItem( mSymbol.get(), mLabel, mRuleKey, true, mMaximumScale, mMinimumScale, currentLevel, mParent ? mParent->mRuleKey : QString() );
245238
}
246239

247240
for ( RuleList::const_iterator it = mChildren.constBegin(); it != mChildren.constEnd(); ++it )
@@ -294,7 +287,7 @@ QDomElement QgsRuleBasedRenderer::Rule::save( QDomDocument &doc, QgsSymbolMap &s
294287
if ( mSymbol )
295288
{
296289
int symbolIndex = symbolMap.size();
297-
symbolMap[QString::number( symbolIndex )] = mSymbol;
290+
symbolMap[QString::number( symbolIndex )] = mSymbol.get();
298291
ruleElem.setAttribute( QStringLiteral( "symbol" ), symbolIndex );
299292
}
300293
if ( !mFilterExp.isEmpty() )
@@ -508,7 +501,7 @@ QgsRuleBasedRenderer::Rule::RenderResult QgsRuleBasedRenderer::Rule::renderFeatu
508501
Q_FOREACH ( int normZLevel, mSymbolNormZLevels )
509502
{
510503
//QgsDebugMsg(QString("add job at level %1").arg(normZLevel));
511-
renderQueue[normZLevel].jobs.append( new RenderJob( featToRender, mSymbol ) );
504+
renderQueue[normZLevel].jobs.append( new RenderJob( featToRender, mSymbol.get() ) );
512505
rendered = true;
513506
}
514507
}
@@ -565,7 +558,7 @@ QgsSymbolList QgsRuleBasedRenderer::Rule::symbolsForFeature( QgsFeature &feat, Q
565558
if ( !isFilterOK( feat, context ) )
566559
return lst;
567560
if ( mSymbol )
568-
lst.append( mSymbol );
561+
lst.append( mSymbol.get() );
569562

570563
Q_FOREACH ( Rule *rule, mActiveChildren )
571564
{

‎src/core/symbology/qgsrulebasedrenderer.h

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer
4545
FeatDrawMarkers = 2
4646
};
4747

48-
// feature for rendering: QgsFeature and some flags
48+
/**
49+
* Feature for rendering by a QgsRuleBasedRenderer. Contains a QgsFeature and some flags.
50+
* \ingroup core
51+
*/
4952
struct FeatureToRender
5053
{
5154
FeatureToRender( QgsFeature &_f, int _flags )
@@ -56,25 +59,35 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer
5659
int flags; // selected and/or draw markers
5760
};
5861

59-
// rendering job: a feature to be rendered with a particular symbol
60-
// (both f, symbol are _not_ owned by this class)
62+
/**
63+
* A QgsRuleBasedRenderer rendering job, consisting of a feature to be rendered with a particular symbol.
64+
* \ingroup core
65+
*/
6166
struct RenderJob
6267
{
6368
RenderJob( QgsRuleBasedRenderer::FeatureToRender &_ftr, QgsSymbol *_s )
6469
: ftr( _ftr )
6570
, symbol( _s )
6671
{}
72+
73+
//! Feature to render
6774
QgsRuleBasedRenderer::FeatureToRender &ftr;
75+
76+
//! Symbol to render feature with (not owned by this object).
6877
QgsSymbol *symbol = nullptr;
6978
};
7079

71-
// render level: a list of jobs to be drawn at particular level
72-
// (jobs are owned by this class)
80+
/**
81+
* Render level: a list of jobs to be drawn at particular level for a QgsRuleBasedRenderer.
82+
* \ingroup core
83+
*/
7384
struct RenderLevel
7485
{
7586
explicit RenderLevel( int z ): zIndex( z ) {}
7687
~RenderLevel() { Q_FOREACH ( RenderJob *j, jobs ) delete j; }
7788
int zIndex;
89+
90+
//! List of jobs to render, owned by this object.
7891
QList<QgsRuleBasedRenderer::RenderJob *> jobs;
7992

8093
QgsRuleBasedRenderer::RenderLevel &operator=( const QgsRuleBasedRenderer::RenderLevel &rh )
@@ -100,7 +113,7 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer
100113

101114
};
102115

103-
// rendering queue: a list of rendering levels
116+
//! Rendering queue: a list of rendering levels
104117
typedef QList<QgsRuleBasedRenderer::RenderLevel> RenderQueue;
105118

106119
class Rule;
@@ -178,7 +191,7 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer
178191
*/
179192
bool isScaleOK( double scale ) const;
180193

181-
QgsSymbol *symbol() { return mSymbol; }
194+
QgsSymbol *symbol() { return mSymbol.get(); }
182195
QString label() const { return mLabel; }
183196
bool dependsOnScale() const { return mMaximumScale != 0 || mMinimumScale != 0; }
184197

@@ -206,7 +219,7 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer
206219
* A filter that will check if this rule applies
207220
* \returns An expression
208221
*/
209-
QgsExpression *filter() const { return mFilter; }
222+
QgsExpression *filter() const { return mFilter.get(); }
210223

211224
/**
212225
* A filter that will check if this rule applies
@@ -411,29 +424,29 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer
411424
protected:
412425
void initFilter();
413426

414-
Rule *mParent; // parent rule (NULL only for root rule)
415-
QgsSymbol *mSymbol = nullptr;
427+
private:
428+
#ifdef SIP_RUN
429+
Rule( const QgsRuleBasedRenderer::Rule &rh );
430+
#endif
431+
432+
Rule *mParent = nullptr; // parent rule (NULL only for root rule)
433+
std::unique_ptr< QgsSymbol > mSymbol;
416434
double mMaximumScale = 0;
417435
double mMinimumScale = 0;
418436
QString mFilterExp, mLabel, mDescription;
419-
bool mElseRule;
437+
bool mElseRule = false;
420438
RuleList mChildren;
421439
RuleList mElseRules;
422-
bool mIsActive; // whether it is enabled or not
440+
bool mIsActive = true; // whether it is enabled or not
423441

424442
QString mRuleKey; // string used for unique identification of rule within renderer
425443

426444
// temporary
427-
QgsExpression *mFilter = nullptr;
445+
std::unique_ptr< QgsExpression > mFilter;
428446
// temporary while rendering
429447
QSet<int> mSymbolNormZLevels;
430448
RuleList mActiveChildren;
431449

432-
private:
433-
#ifdef SIP_RUN
434-
Rule( const QgsRuleBasedRenderer::Rule &rh );
435-
#endif
436-
437450
/**
438451
* Check which child rules are else rules and update the internal list of else rules
439452
*

‎tests/src/python/test_qgsrulebasedrenderer.py

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
from qgis.PyQt.QtCore import QSize
3131

3232
from qgis.core import (QgsVectorLayer,
33+
QgsMapSettings,
3334
QgsProject,
3435
QgsRectangle,
3536
QgsMultiRenderChecker,
@@ -42,7 +43,6 @@
4243
QgsRendererRange
4344
)
4445
from qgis.testing import start_app, unittest
45-
from qgis.testing.mocked import get_iface
4646
from utilities import unitTestDataPath
4747

4848
# Convenience instances in case you may need them
@@ -54,7 +54,6 @@
5454
class TestQgsRulebasedRenderer(unittest.TestCase):
5555

5656
def setUp(self):
57-
self.iface = get_iface()
5857
myShpFile = os.path.join(TEST_DATA_DIR, 'rectangles.shp')
5958
layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
6059
QgsProject.instance().addMapLayer(layer)
@@ -68,14 +67,13 @@ def setUp(self):
6867
self.r2 = QgsRuleBasedRenderer.Rule(sym2, 0, 0, '"id" = 2')
6968
self.r3 = QgsRuleBasedRenderer.Rule(sym3, 0, 0, 'ELSE')
7069

71-
self.rootrule = QgsRuleBasedRenderer.Rule(None)
72-
self.rootrule.appendChild(self.r1)
73-
self.rootrule.appendChild(self.r2)
74-
self.rootrule.appendChild(self.r3)
70+
rootrule = QgsRuleBasedRenderer.Rule(None)
71+
rootrule.appendChild(self.r1)
72+
rootrule.appendChild(self.r2)
73+
rootrule.appendChild(self.r3)
7574

76-
self.renderer = QgsRuleBasedRenderer(self.rootrule)
77-
layer.setRenderer(self.renderer)
78-
self.mapsettings = self.iface.mapCanvas().mapSettings()
75+
layer.setRenderer(QgsRuleBasedRenderer(rootrule))
76+
self.mapsettings = QgsMapSettings()
7977
self.mapsettings.setOutputSize(QSize(400, 400))
8078
self.mapsettings.setOutputDpi(96)
8179
self.mapsettings.setExtent(QgsRectangle(-163, 22, -70, 52))

0 commit comments

Comments
 (0)
Please sign in to comment.