Remove expression evaluation in layout legend widget

[Djedouas]

Description

This PR comes from trying to fix #53442.

I jumped into parts of code that are very strange.

The evaluation of expressions for the QgsLayoutLegendItem in the composer, vs the evaluation of expressions in the QTreeView in the composer legend widget are somewhat correlated but not completely...

I will explain my observations, and why I suggest the changes in this PR.

User observations

• The layout item always evaluates the legend label, expression or not being present (in QgsLayerTreeModelLegendNode::draw)

• The legend tree view does not always evaluate the legend labels
  
  

• The legend tree view does not evaluate expressions when refreshing the layout () or when moving to the next atlas page (except if the filtering for items - in linked map or atlas - is enabled)

Technical observations

Technically, what is very strange, is that the label evaluation occurs in the getData method of the legend model.

That means that the model is changed inside a method that should not change the model (and the model is not aware of this change).

Also, the legend layout item evaluates the label of the model, whereas the legend tree view evaluates the UserLabel (edit role) to change and update the model label...

The way it is implemented is also strange (evaluating children symbols nodes but not layer nodes, only if the layer node has an expression... !).

After a lot of time digging into the code, trying to update the labels at the right place/moment, I ended with this conclusion:

the expression evaluation is very situational (symbol expressions that can be added for every child, atlas page number, layer feature-count enabled, user-defined variables, filters, etc.) and should not be part of the model. It is a representation, by definition, the real data being the raw expression. Furthermore, the expressions are today evaluated at different places with possibly different contexts (for the legend layout item and for the legend tree view).

Suggested changes

I think that the model should contain the label, with the textual expression (not evaluated), and that it is the responsibility of the view to evaluate the expression, like it is done with the legend layout item.

As a result, in this PR I removed the label evaluation taking place in the getData method of the model.

The proposition is to always see the expressions in the legend tree view, and leave the layout legend item be the only one evaluating the label.

It is technically easier because evaluating the label in every situation is difficult to handle (with expressions being able to be in the layer style label, in the legend node widget, with the entity count, atlas page, etc.), so having the "total" expression, not evaluated, in the model label makes sense, and it is evaluated at the very end, for the rendering.

From a user point of view it can be argued that it is also more convenient to see his expression in the legend tree widget, and the evaluation on his composer page.

[roya0045]

Thanks for the detail, personally as long as the end user can view aggregate expression properly evaluated for each symbol entry in the layout I don't mind too much. The only worry that I have is having long expressions creating noise in the Item menu.

The reason for some strangeness was inexperience and trying to consolidate differing mode of input and vilusation to have something uniform.

[Djedouas]

I just added an adaptation for handling symbol label expressions, and a correction regarding the scope of this very expression builder dialog.

QgsSymbolLegendNode::createSymbolScope() is now public for removing duplicate code and have always a coherent expression scope for symbols.

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

[Djedouas]

Hi,

It would be great if someone could review/discuss this PR please. Issue https://github.com/qgis/QGIS/issues/53442 is still here...

Thanks!

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[DelazJ]

From a user point of view it can be argued that it is also more convenient to see his expression in the legend tree widget, and the evaluation on his composer page.

I can't speak for the code and technical considerations but I very much +1 the above statement.

[Djedouas]

@elpaso @nyalldawson can I ask for a review here please? Or maybe you can redirect to some contributor who is familiar with this part of QGIS?

I know it's not an easy one, and first of all I need feedbacks on the proposal I made in the PR description, with the prerequisite to read the related issue first.

(Then I can see if there is a need for a rebase due to the long time since the PR).

Thanks

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[Guts]

I and some end-users are waiting for this fix. Any chance to see it reviewed in the next weeks?

[elpaso]

@Djedouas LGTM

just a couple of questions:

does this PR also impact the label display outside of the layout context (for example in the main legend)?

A question about the context creation, have you considered to use QgsExpressionContextGenerator interface?

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in a week.

[github-actions[bot]]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.