[bugfix] Fixes identify action on deactivated rules for QgsRuleBasedRenderer #6679
Conversation
|
@nyalldawson Can you give me your opinion on this PR if you have some time? Thanks :) |
pblottiere
force-pushed
the
bugfix_will_render_feature
branch
from
8b0a4f2
to
aaac8f5
Compare
|
@pblottiere I'm not sure but how close is this issue to https://issues.qgis.org/issues/18143? |
|
@pblottiere Tested and it's ok for me. Thanks! This is not yours, but for other reviewers, a comment on QgsNullSymbolRenderer should be always false IMHO. @DelazJ 👍 |
pblottiere
force-pushed
the
bugfix_will_render_feature
branch
from
aaac8f5
to
dd6f98f
Compare
|
Hi Etienne, I think this PR fixes an issue you raised some time ago : https://issues.qgis.org/issues/13999 Before:
Now:
|
Mmm I'm not sure either. But this issue leads me to another one : https://issues.qgis.org/issues/13999 Thank you for that! |
Yes, also saw it when looking for the report. Wanted to point you to it but as it was mentioned in the first link, i just crossed my fingers and hoped you tackle both of them ;). Good it's gone; this was an annoying issue when willing to show feature count in legend. Thanks Paul. |
| @@ -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 | |||
There was a problem hiding this comment.
Maybe add an explanation of the onlyActive argument here too
Can we discuss that elsewhere? I personally think it's nice to be able to see selected features even with the null renderer. |
|
Travis is green, I'll merge this PR tomorrow if I have no more comments in the meantime. |
|
Great work @pblottiere! |
|
This causes a regression, I guess related to nested
In such a scenario, the identify action will not discover visible features which are rendered by one of the nested rules within the |
| rendered = renderer.willRenderFeature(ft, ctx) | ||
| renderer.stopRender(ctx) | ||
| renderer.rootRule().children()[0].setActive(True) | ||
| assert rendered == False |
There was a problem hiding this comment.
As a general input, better use the self.assertFalse() etc methods, they give a much better output if they fail.
Description
This PR fixes an issue raised by @lbartoletti during its work on #6657 (comment).
Actually, if a
ELSErule is defined and activated within the rule based renderer, then all others rules are valid even if they're deactivated. Then, an identify action can be done on invisible features.A video is available, allowing to highlight the issue: https://github.com/lbartoletti/lbartoletti.github.io/tree/master/willRenderFeature.
A unit test has been added to test the underlying
willRenderFeaturemethod.Checklist
fixes #11111in the commit message next to the description[FEATURE]in the commit message[needs-docs]in the commit message and containt sufficient information in the commit message to be documentedscripts/prepare-commit.shscript before each commit