-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Masking] Fix copy/paste of symbol or converting renderer #60325
base: master
Are you sure you want to change the base?
Conversation
Both symbol layer ids and masks need to be removed from symbol layers which can be inherited from a converted renderer
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
{ | ||
if ( QgsSymbolLayer *sl = symbol->symbolLayer( idx ) ) | ||
{ | ||
sl->clearMasks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lean toward a dynamic_cast to QgsMaskMarkerSymbolLayer here and then directly calling QgsMaskMarkerSymbolLayer::clearMasks to avoid the virtual clearMasks method in QgsSymbolLayer.
If later on we find other symbol layer types which need this handled then we can safely resurrect the virtual method without any API breakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this solution but preferred to have clearMasks() method for 2 reasons:
- to be consistent with masks() method (this is not the case of setMasks() but maybe this one should also be defined as a virtual method of QgsSymbolLayer)
- dynamic_cast is a bad design and should be avoided
Even if we never get a symbol layer types implementing masks, why do you think it is better with dynamic_cast ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you think it is better with dynamic_cast ?
It keeps the base class API smaller and avoids cluttering it with methods which have no use for all but one subclass. And in this case the dynamic_cast is only in one place in a static function from a utils class, so I think it's fine for that to have the little bit of added complexity.
Properly reset symbol layer ids when copy/pasting symbol or when converting a rule based renderer into a categorized renderer.
Fixes #46402 where issues comes from symbol layers having identical symbol layers ids, coming from I presume, actions described just above