-
-
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
Handle conditional visibility of data defined size legend nodes #57461
Conversation
Tests failed for Qt 6One or more tests failed using the build from commit 108aa23 legend_data_defined_size_separated (testDataDefinedSizeSeparated)legend_data_defined_size_separatedTest failed at _verifyImage at tests/src/core/testqgslegendrenderer.cpp:225 Rendered image did not match tests/testdata/control_images/legend/expected_legend_data_defined_size_separated/expected_legend_data_defined_size_separated.png (found 3540 pixels different) legend_data_defined_size_filter_by_map (testDataDefinedSizeCollapsedFilterByMap)legend_data_defined_size_filter_by_mapTest failed at _verifyImage at tests/src/core/testqgslegendrenderer.cpp:225 Rendered image did not match tests/testdata/control_images/legend/expected_legend_data_defined_size_filter_by_map/expected_legend_data_defined_size_filter_by_map.png (found 347 pixels different) legend_data_defined_size_filter_by_map (testDataDefinedSizeSeparatedFilterByMap)legend_data_defined_size_filter_by_mapTest failed at _verifyImage at tests/src/core/testqgslegendrenderer.cpp:225 Rendered image did not match tests/testdata/control_images/legend/expected_legend_data_defined_size_filter_by_map/expected_legend_data_defined_size_filter_by_map.png (found 347 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
@@ -126,7 +126,8 @@ QgsLegendSymbolList QgsDataDefinedSizeLegend::legendSymbolList() const | |||
QgsLegendSymbolList lst; | |||
if ( !mTitleLabel.isEmpty() ) | |||
{ | |||
QgsLegendSymbolItem title( nullptr, mTitleLabel, QString() ); | |||
// we're abusing the ruleKey field here so we can later identify the resulting legend nodes as data defined size related | |||
QgsLegendSymbolItem title( nullptr, mTitleLabel, QStringLiteral( "data-defined-size" ) ); |
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.
instead of the abuse, how about we add a QMap< int, QVariant > userData to QgsLegendSymbolItem, and adapt QgsSymbolLegendNode::data to return those values for unknown roles.
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.
We could have a new role in QgsLayerTreeModelLegendNode::CustomRole
for that, instead of handling all unknown roles.
But, why a QMap< int, QVariant >
? What would you store there (in this case and/or in future uses of it)?
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.
We could have a new role in QgsLayerTreeModelLegendNode::CustomRole for that, instead of handling all unknown roles.
That also works. I was just thinking ahead if allowing custom roles in QgsLegendSymbolItem would also be useful in future.
But, why a QMap< int, QVariant >? What would you store there (in this case and/or in future uses of it)?
The int keys would correspond to a role from QgsLayerTreeModelLegendNode (either public or private). I'd find a QMap approach slightly preferable to having separate members for every property we decide to add in future.
Tests failed for Qt 5One or more tests failed using the build from commit a31dfb5 layout_export_markerline_masked_geometrylayout_export_markerline_masked_geometryTest failed at test_markerline_masked at tests/src/python/test_selective_masking.py:1376 Rendered image did not match tests/testdata/control_images/selective_masking/layout_export_markerline_masked/layout_export_markerline_masked.png (found 124 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
The backport to
stderr
stdout
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_38 release-3_38
# Navigate to the new working tree
cd .worktrees/backport-release-3_38
# Create a new branch
git switch --create backport-57461-to-release-3_38
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 016cfe60a0411761bbc0f0971b7cd22d88820689,108aa23f67b9f43a77fffaf3ce46e6806c9c9d87,cb491ea9baa466a260943edfb0135ad932f180c0,ac292ac6fdebb5f852dc99ba75a440f0d621dde2,3bb06c107118f62812404bce1390da43877de10d,b44b51cc109cd9954dc92d8f8b5fa3f65d695ba2,2bc7bc57547215da53a7e4dab7884057e060bb40,a31dfb5c2be8af26cf173286fd183c746fd557f4
# Push it to GitHub
git push --set-upstream origin backport-57461-to-release-3_38
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_38 Then, create a pull request where the |
Description
This PR Fixes #50301
The solution is a little hacky.Would it make more sense to haveQgsDataDefinedSizeLegendNode
class also handle the rendering of the data defined size label and also individual classes, instead of having separateQgsSymbolLegendNode
s for them?Solution is no longer hacky, #57461 (comment) was implemented