Skip to content
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

Hide fields from layout legend expr filter #53434

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Jun 12, 2023

Fix #53244

@elpaso elpaso added Bug Either a bug report, or a bug fix. Let's hope for the latter! Print Layouts Related to QGIS Print Layouts, Atlas or Reporting frameworks labels Jun 12, 2023
@github-actions github-actions bot added this to the 3.32.0 milestone Jun 12, 2023
@roya0045
Copy link
Contributor

I don't think that hiding information that could be useful is a good fix. IMO the solution would be to have the preview operate on the symbols instead of the features. The fields can still be useful for aggregate operations.

@elpaso
Copy link
Contributor Author

elpaso commented Jun 13, 2023

I don't think that hiding information that could be useful is a good fix. IMO the solution would be to have the preview operate on the symbols instead of the features. The fields can still be useful for aggregate operations.

I agree with you, a proper fix would require significant changes to the expression dialog. Unfortunately I don't think I have the time and resources for that implementation now but I would be glad to assist and review your code if you are willing to do it.

The question is if this small PR is a step forward or not.

If it isn't I'm happy to close it and wait for a better solution.

@roya0045
Copy link
Contributor

roya0045 commented Jun 13, 2023

Yes I came to the same conclusion that altering the expression builder would require a significant refactoring to solve this corner case. What could be helpful is to get #48375 merged, this would help point to user a robust formulation to perform aggregate calculations.

I don't know if that's possible, but displaying the field from the layer and having the feature be the atlas feature might give a more reliable preview. Though it might confuse users.

But as it stands, I'm afraid that simply hiding the fields will be a shortfix that will become permanent and will greatly impede the usage the of tool. I will try to see if I can come up with something to help shortly. Otherwise my opinion can be ignored if Nyall agree to move on with this quickfix for the moment.

The builder is at https://github.com/qgis/QGIS/blob/678cee7533ba9902a6e8483d8c35c96219a3e968/src/gui/layout/qgslayoutlegendwidget.cpp#LL1193C3-L1193C29

Maybe riging a way to use the layer's fields in

void QgsExpressionBuilderWidget::initWithFields( const QgsFields &fields, const QgsExpressionContext &context, const QString &recentCollection, QgsExpressionBuilderWidget::Flags flags )

And then setting the layer to the atlas layer would make it work without too much refactoring.

@nyalldawson
Copy link
Collaborator

I'm happy with @elpaso's approach here -- the field names are still available for easy access from the 'Map Layers' group, it's just hiding the not-available "field" accessors here. That's a good approach for now.

@nyalldawson nyalldawson merged commit 4eb38af into qgis:master Jun 14, 2023
36 checks passed
@elpaso elpaso deleted the bugfix-gh53244-hide-fields-from-layout-legend-filter-expression-context branch June 19, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Print Layouts Related to QGIS Print Layouts, Atlas or Reporting frameworks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map Layout composer - Legend item expression not displaying correctly
3 participants