Add more context to the legend filter expressions in layouts

[elpaso]

Fix #53229

[elpaso]

@Djedouas I would be grateful if you could test this patch with your layout use case.

[elpaso]

@nyalldawson that was fast :) Thank you.

[Djedouas]

Hi @elpaso and thanks for taking this issue 🙂

I have a complete QGIS crash when testing these changes.

The only error messages are:

src/app/layout/qgslayoutdesignerdialog.cpp:4049 : (restoreWindowState) [2743ms] restore of layout UI geometry failed
Fatal: ASSERT: "!isEmpty()" in file /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h, line 363

[Djedouas]

The line

src/app/layout/qgslayoutdesignerdialog.cpp:4049 : (restoreWindowState) [2743ms] restore of layout UI geometry failed

is not related to our problem.

The crash occurs in QgsLegendModel::data(index, role), in src/core/layout/qgslayoutitemlegend.cpp line 1395

[Djedouas]

Example project

1. Open project in the gpkg
2. Open Layouts manager
3. Open layout "Mise en page 2"
4. Select legend
5. Click on the expression filter (expression already written)
6. Crash

[elpaso]

@Djedouas I can reproduce the crash, looking into it.

[elpaso]

@Djedouas I can reproduce the crash, looking into it.

@Djedouas the crash is unrelated to this PR: I can reproduce it on master.

[Djedouas]

Ooh!

[elpaso]

@Djedouas crash is gone with a simple check, but looking at your sample file I see what you are trying to do and I realize that there is still a missing part: the expression context with the atlas variables must be passed down to the legend model because it is required by the hit test in order to evaluate the expression. There is no API to do that currently.

I guess this was really a feature request more than a bug, but I'll see what I can do since I've already spent some time on this topic.

[Djedouas]

All right, I was indeed not sure if this was missing by intention, hence it requires a feature request, or my mistake.

Thanks a lot 🙂

[elpaso]

All right, I was indeed not sure if this was missing by intention, hence it requires a feature request, or my mistake.

@Djedouas

It was harder than I thought and the test part was the most time consuming but I finally got it.

I'd appreciate your feedback.

[Djedouas]

Wow that's a big work indeed! Thanks, I knew I don't have enough QGIS development experience yet for this one 😄

That works for me 👍🏼

I only noticed this, I don't know if it's related, it was already the case before:
when going from an atlas page to another, I need to click on the legend for the text to be updated (not working with the refresh button):

Not up to date

Updated on click

It is alway up to date when going from an atlas page to another if the legend is (stays) selected.

---

I understand if you ask me to fill in a new issue for this, no problem 😉

[elpaso]

Yes, please file a separate issue.

[nyalldawson]

@elpaso

Couldn't we avoid a lot of the added complexity from the "pass the expression context to the legend renderer and to the hit test" commit by just setting the expression context for the QgsMapSettings object created in QgsLayoutItemLegend::doUpdateFilterByMap() directly?

[elpaso]

@elpaso

Couldn't we avoid a lot of the added complexity from the "pass the expression context to the legend renderer and to the hit test" commit by just setting the expression context for the QgsMapSettings object created in QgsLayoutItemLegend::doUpdateFilterByMap() directly?

I'll check if that allows for a simpler approach, thanks for the hint.

[elpaso]

@elpaso

Couldn't we avoid a lot of the added complexity from the "pass the expression context to the legend renderer and to the hit test" commit by just setting the expression context for the QgsMapSettings object created in QgsLayoutItemLegend::doUpdateFilterByMap() directly?

It worked, I removed the new API calls.

[qgis-bot]

The backport to queued_ltr_backports failed:

The process '/usr/bin/git' failed with exit code 1

stderr

error: could not apply e3196484e1... Add more context to the legend filter expressions in layouts
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout

Auto-merging src/gui/layout/qgslayoutlegendwidget.cpp
CONFLICT (content): Merge conflict in src/gui/layout/qgslayoutlegendwidget.cpp

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-queued_ltr_backports queued_ltr_backports
# Navigate to the new working tree
cd .worktrees/backport-queued_ltr_backports
# Create a new branch
git switch --create backport-53296-to-queued_ltr_backports
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick e3196484e15d0552ca44b8ab736e0ffb3e65cd77,ec2a6a013a0a780f029681ca12e417635789d4a2,7e9147c7125d50c9edb3ba2d6c631561713f9b95,e7ebb8bc1b5dd38aefa8e7dcb7faeb20ff93d2f9,d2185bdfa9e02df2bc391bcb6cfd3b623fad9395,b68ff1cda216757c2854191c86517e0c0b25bdcd
# Push it to GitHub
git push --set-upstream origin backport-53296-to-queued_ltr_backports
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-queued_ltr_backports

Then, create a pull request where the base branch is queued_ltr_backports and the compare/head branch is backport-53296-to-queued_ltr_backports.