From d6331a6d3d2e9b8a9075ca097176e361ccc8d701 Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Tue, 27 Mar 2018 09:13:38 +0100 Subject: [PATCH 1/7] [bugfix] Fixes willRenderFeature method for QgsRuledBasedRenderer --- src/core/symbology/qgsrulebasedrenderer.cpp | 32 ++++++++++++++++++--- src/core/symbology/qgsrulebasedrenderer.h | 4 +-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/core/symbology/qgsrulebasedrenderer.cpp b/src/core/symbology/qgsrulebasedrenderer.cpp index 136b7cae6d2..348ac285e03 100644 --- a/src/core/symbology/qgsrulebasedrenderer.cpp +++ b/src/core/symbology/qgsrulebasedrenderer.cpp @@ -581,7 +581,7 @@ QSet QgsRuleBasedRenderer::Rule::legendKeysForFeature( QgsFeature &feat return lst; } -QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( QgsFeature &feat, QgsRenderContext *context ) +QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( QgsFeature &feat, QgsRenderContext *context, bool withElse, bool onlyActive ) { RuleList lst; if ( !isFilterOK( feat, context ) ) @@ -590,9 +590,16 @@ QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( QgsF if ( mSymbol ) lst.append( this ); - Q_FOREACH ( Rule *rule, mActiveChildren ) + RuleList listChildren = children(); + if ( onlyActive ) + listChildren = mActiveChildren; + + Q_FOREACH ( Rule *rule, listChildren ) { - lst += rule->rulesForFeature( feat, context ); + if ( rule->isElse() && !withElse ) + continue; + + lst += rule->rulesForFeature( feat, context, withElse, onlyActive ); } return lst; } @@ -1141,7 +1148,24 @@ QString QgsRuleBasedRenderer::dump() const bool QgsRuleBasedRenderer::willRenderFeature( QgsFeature &feat, QgsRenderContext &context ) { - return mRootRule->willRenderFeature( feat, &context ); + for ( Rule *rule : mRootRule->children() ) + { + if ( ! rule->active() ) + continue; + + // a feature already rendered by another rule shouldn't be considered in a + // 'else' statement for rendering + if ( rule->isElse() && mRootRule->rulesForFeature( feat, &context, false, false ).empty() ) + { + return true; + } + else if ( !rule->isElse( ) && rule->willRenderFeature( feat, &context ) ) + { + return true; + } + } + + return false; } QgsSymbolList QgsRuleBasedRenderer::symbolsForFeature( QgsFeature &feat, QgsRenderContext &context ) diff --git a/src/core/symbology/qgsrulebasedrenderer.h b/src/core/symbology/qgsrulebasedrenderer.h index 6cc8202ef2b..387b59897ea 100644 --- a/src/core/symbology/qgsrulebasedrenderer.h +++ b/src/core/symbology/qgsrulebasedrenderer.h @@ -343,7 +343,7 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer QSet< QString > legendKeysForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr ); //! tell which rules will be used to render the feature - QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr ); + QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr, bool withElse = true, bool onlyActive = true ); /** * Stop a rendering process. Used to clean up the internal state of this rule @@ -419,7 +419,7 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer * * \returns True if this rule is an else rule */ - bool isElse() { return mElseRule; } + bool isElse() const { return mElseRule; } protected: void initFilter(); From 2cb1f43342694fe836ad6394ff272601d3656f00 Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Tue, 27 Mar 2018 09:14:02 +0100 Subject: [PATCH 2/7] Fix sip binding --- python/core/symbology/qgsrulebasedrenderer.sip.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/core/symbology/qgsrulebasedrenderer.sip.in b/python/core/symbology/qgsrulebasedrenderer.sip.in index bdeefe7ed49..88f6f0c3dad 100644 --- a/python/core/symbology/qgsrulebasedrenderer.sip.in +++ b/python/core/symbology/qgsrulebasedrenderer.sip.in @@ -324,7 +324,7 @@ Returns which legend keys match the feature .. versionadded:: 2.14 %End - QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = 0 ); + QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = 0, bool withElse = true, bool onlyActive = true ); %Docstring tell which rules will be used to render the feature %End @@ -411,7 +411,7 @@ Sets if this rule is an ELSE rule :param iselse: If true, this rule is an ELSE rule %End - bool isElse(); + bool isElse() const; %Docstring Check if this rule is an ELSE rule From dd6f98fe24e786ce45964074204abc070d7e5e41 Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Tue, 27 Mar 2018 09:14:18 +0100 Subject: [PATCH 3/7] Add unit tests --- tests/src/python/test_qgsrulebasedrenderer.py | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/src/python/test_qgsrulebasedrenderer.py b/tests/src/python/test_qgsrulebasedrenderer.py index d7197ab7fc4..9d807f45c10 100644 --- a/tests/src/python/test_qgsrulebasedrenderer.py +++ b/tests/src/python/test_qgsrulebasedrenderer.py @@ -40,7 +40,8 @@ from qgis.core import (QgsVectorLayer, QgsRendererCategory, QgsCategorizedSymbolRenderer, QgsGraduatedSymbolRenderer, - QgsRendererRange + QgsRendererRange, + QgsRenderContext ) from qgis.testing import start_app, unittest from utilities import unitTestDataPath @@ -101,6 +102,27 @@ class TestQgsRulebasedRenderer(unittest.TestCase): assert result + def testWillRenderFeature(self): + vl = self.mapsettings.layers()[0] + ft = vl.getFeature(0) # 'id' = 1 + renderer = vl.renderer() + + ctx = QgsRenderContext.fromMapSettings(self.mapsettings) + ctx.expressionContext().setFeature(ft) + + renderer.rootRule().children()[0].setActive(False) + renderer.rootRule().children()[1].setActive(True) + renderer.rootRule().children()[2].setActive(True) + + renderer.startRender(ctx, vl.fields()) # build mActiveChlidren + + rendered = renderer.willRenderFeature(ft, ctx) + renderer.rootRule().children()[0].setActive(True) + assert rendered == False + + rendered = renderer.willRenderFeature(ft, ctx) + assert rendered == True + def testRefineWithCategories(self): # Test refining rule with categories (refs #10815) From 0b5c2aeecc06061c80dabd48ffb700b697ac0141 Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Tue, 27 Mar 2018 11:00:20 +0100 Subject: [PATCH 4/7] Recursive fix --- .../symbology/qgsrulebasedrenderer.sip.in | 2 +- src/core/symbology/qgsrulebasedrenderer.cpp | 39 +++++++------------ src/core/symbology/qgsrulebasedrenderer.h | 2 +- tests/src/python/test_qgsrulebasedrenderer.py | 4 +- 4 files changed, 20 insertions(+), 27 deletions(-) diff --git a/python/core/symbology/qgsrulebasedrenderer.sip.in b/python/core/symbology/qgsrulebasedrenderer.sip.in index 88f6f0c3dad..ed9d06f402a 100644 --- a/python/core/symbology/qgsrulebasedrenderer.sip.in +++ b/python/core/symbology/qgsrulebasedrenderer.sip.in @@ -324,7 +324,7 @@ Returns which legend keys match the feature .. versionadded:: 2.14 %End - QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = 0, bool withElse = true, bool onlyActive = true ); + QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = 0, bool onlyActive = true ); %Docstring tell which rules will be used to render the feature %End diff --git a/src/core/symbology/qgsrulebasedrenderer.cpp b/src/core/symbology/qgsrulebasedrenderer.cpp index 348ac285e03..50210e2f0ab 100644 --- a/src/core/symbology/qgsrulebasedrenderer.cpp +++ b/src/core/symbology/qgsrulebasedrenderer.cpp @@ -541,13 +541,24 @@ bool QgsRuleBasedRenderer::Rule::willRenderFeature( QgsFeature &feat, QgsRenderC { if ( !isFilterOK( feat, context ) ) return false; + if ( mSymbol ) return true; Q_FOREACH ( Rule *rule, mActiveChildren ) { - if ( rule->willRenderFeature( feat, context ) ) + if ( rule->isElse() ) + { + RuleList lst = rulesForFeature( feat, context, false ); + lst.removeOne( rule ); + + if ( lst.empty() ) + return true; + } + else if ( !rule->isElse( ) && rule->willRenderFeature( feat, context ) ) + { return true; + } } return false; } @@ -581,7 +592,7 @@ QSet QgsRuleBasedRenderer::Rule::legendKeysForFeature( QgsFeature &feat return lst; } -QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( QgsFeature &feat, QgsRenderContext *context, bool withElse, bool onlyActive ) +QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( QgsFeature &feat, QgsRenderContext *context, bool onlyActive ) { RuleList lst; if ( !isFilterOK( feat, context ) ) @@ -596,10 +607,7 @@ QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( QgsF Q_FOREACH ( Rule *rule, listChildren ) { - if ( rule->isElse() && !withElse ) - continue; - - lst += rule->rulesForFeature( feat, context, withElse, onlyActive ); + lst += rule->rulesForFeature( feat, context, onlyActive ); } return lst; } @@ -1148,24 +1156,7 @@ QString QgsRuleBasedRenderer::dump() const bool QgsRuleBasedRenderer::willRenderFeature( QgsFeature &feat, QgsRenderContext &context ) { - for ( Rule *rule : mRootRule->children() ) - { - if ( ! rule->active() ) - continue; - - // a feature already rendered by another rule shouldn't be considered in a - // 'else' statement for rendering - if ( rule->isElse() && mRootRule->rulesForFeature( feat, &context, false, false ).empty() ) - { - return true; - } - else if ( !rule->isElse( ) && rule->willRenderFeature( feat, &context ) ) - { - return true; - } - } - - return false; + return mRootRule->willRenderFeature( feat, &context ); } QgsSymbolList QgsRuleBasedRenderer::symbolsForFeature( QgsFeature &feat, QgsRenderContext &context ) diff --git a/src/core/symbology/qgsrulebasedrenderer.h b/src/core/symbology/qgsrulebasedrenderer.h index 387b59897ea..33a1bb4da8b 100644 --- a/src/core/symbology/qgsrulebasedrenderer.h +++ b/src/core/symbology/qgsrulebasedrenderer.h @@ -343,7 +343,7 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer QSet< QString > legendKeysForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr ); //! tell which rules will be used to render the feature - QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr, bool withElse = true, bool onlyActive = true ); + QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr, bool onlyActive = true ); /** * Stop a rendering process. Used to clean up the internal state of this rule diff --git a/tests/src/python/test_qgsrulebasedrenderer.py b/tests/src/python/test_qgsrulebasedrenderer.py index 9d807f45c10..50053452e0a 100644 --- a/tests/src/python/test_qgsrulebasedrenderer.py +++ b/tests/src/python/test_qgsrulebasedrenderer.py @@ -115,12 +115,14 @@ class TestQgsRulebasedRenderer(unittest.TestCase): renderer.rootRule().children()[2].setActive(True) renderer.startRender(ctx, vl.fields()) # build mActiveChlidren - rendered = renderer.willRenderFeature(ft, ctx) + renderer.stopRender(ctx) renderer.rootRule().children()[0].setActive(True) assert rendered == False + renderer.startRender(ctx, vl.fields()) # build mActiveChlidren rendered = renderer.willRenderFeature(ft, ctx) + renderer.stopRender(ctx) assert rendered == True def testRefineWithCategories(self): From 58b3e2005444898aa02070ac4ebebe15d79b18a1 Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Tue, 27 Mar 2018 14:09:20 +0100 Subject: [PATCH 5/7] Fixes #13999: feature count on rule based renderer is valid for ELSE statement --- src/core/symbology/qgsrulebasedrenderer.cpp | 23 ++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/core/symbology/qgsrulebasedrenderer.cpp b/src/core/symbology/qgsrulebasedrenderer.cpp index 50210e2f0ab..31c52072cf2 100644 --- a/src/core/symbology/qgsrulebasedrenderer.cpp +++ b/src/core/symbology/qgsrulebasedrenderer.cpp @@ -553,7 +553,9 @@ bool QgsRuleBasedRenderer::Rule::willRenderFeature( QgsFeature &feat, QgsRenderC lst.removeOne( rule ); if ( lst.empty() ) + { return true; + } } else if ( !rule->isElse( ) && rule->willRenderFeature( feat, context ) ) { @@ -587,7 +589,26 @@ QSet QgsRuleBasedRenderer::Rule::legendKeysForFeature( QgsFeature &feat Q_FOREACH ( Rule *rule, mActiveChildren ) { - lst.unite( rule->legendKeysForFeature( feat, context ) ); + bool validKey = false; + if ( rule->isElse() ) + { + RuleList lst = rulesForFeature( feat, context, false ); + lst.removeOne( rule ); + + if ( lst.empty() ) + { + validKey = true; + } + } + else if ( !rule->isElse( ) && rule->willRenderFeature( feat, context ) ) + { + validKey = true; + } + + if ( validKey ) + { + lst.unite( rule->legendKeysForFeature( feat, context ) ); + } } return lst; } From 624e446b672a4e0bf4e3f0728f8dbc08872d0baa Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Tue, 27 Mar 2018 14:11:15 +0100 Subject: [PATCH 6/7] Add unit test --- tests/src/python/test_qgsrulebasedrenderer.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/src/python/test_qgsrulebasedrenderer.py b/tests/src/python/test_qgsrulebasedrenderer.py index 50053452e0a..8a3c3a9d240 100644 --- a/tests/src/python/test_qgsrulebasedrenderer.py +++ b/tests/src/python/test_qgsrulebasedrenderer.py @@ -125,6 +125,29 @@ class TestQgsRulebasedRenderer(unittest.TestCase): renderer.stopRender(ctx) assert rendered == True + def testFeatureCount(self): + vl = self.mapsettings.layers()[0] + ft = vl.getFeature(2) # 'id' = 3 => ELSE + renderer = vl.renderer() + + ctx = QgsRenderContext.fromMapSettings(self.mapsettings) + ctx.expressionContext().setFeature(ft) + + counter = vl.countSymbolFeatures() + counter.waitForFinished() + + renderer.startRender(ctx, vl.fields()) + + elseRule = None + for rule in renderer.rootRule().children(): + if rule.filterExpression() == 'ELSE': + elseRule = rule + + assert elseRule != None + + cnt = counter.featureCount(elseRule.ruleKey()) + assert cnt == 1 + def testRefineWithCategories(self): # Test refining rule with categories (refs #10815) From 810e531bfc487ddfd18e5649cc9d9fd09137ce1b Mon Sep 17 00:00:00 2001 From: Blottiere Paul Date: Thu, 29 Mar 2018 08:51:03 +0100 Subject: [PATCH 7/7] Add documentation for onlyActive parameter --- python/core/symbology/qgsrulebasedrenderer.sip.in | 7 ++++++- src/core/symbology/qgsrulebasedrenderer.h | 9 ++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/python/core/symbology/qgsrulebasedrenderer.sip.in b/python/core/symbology/qgsrulebasedrenderer.sip.in index ed9d06f402a..29c7777464f 100644 --- a/python/core/symbology/qgsrulebasedrenderer.sip.in +++ b/python/core/symbology/qgsrulebasedrenderer.sip.in @@ -326,7 +326,12 @@ Returns which legend keys match the feature QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = 0, bool onlyActive = true ); %Docstring -tell which rules will be used to render the feature +Returns the list of rules used to render the feature in a specific +context. + +:param feat: The feature for which rules have to be find +:param context: The rendering context +:param onlyActive: True to search for active rules only, false otherwise %End void stopRender( QgsRenderContext &context ); diff --git a/src/core/symbology/qgsrulebasedrenderer.h b/src/core/symbology/qgsrulebasedrenderer.h index 33a1bb4da8b..8d8a9e0ae29 100644 --- a/src/core/symbology/qgsrulebasedrenderer.h +++ b/src/core/symbology/qgsrulebasedrenderer.h @@ -342,7 +342,14 @@ class CORE_EXPORT QgsRuleBasedRenderer : public QgsFeatureRenderer */ QSet< QString > legendKeysForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr ); - //! tell which rules will be used to render the feature + /** + * Returns the list of rules used to render the feature in a specific + * context. + * + * \param feat The feature for which rules have to be find + * \param context The rendering context + * \param onlyActive True to search for active rules only, false otherwise + */ QgsRuleBasedRenderer::RuleList rulesForFeature( QgsFeature &feat, QgsRenderContext *context = nullptr, bool onlyActive = true ); /**