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

ATLAS: LEGEND support clipping #54692

Merged

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Sep 21, 2023

Fix #54654

Working on the tests...

@Geoscope19 please test the artifact when ready.

@elpaso elpaso added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging 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 backport queued_ltr_backports Queued Backports labels Sep 21, 2023
@github-actions github-actions bot added this to the 3.34.0 milestone Sep 21, 2023
@elpaso elpaso force-pushed the bugfix-gh8155-atlas-legend-support-clipping branch from c6855de to d156fba Compare September 21, 2023 15:37
@Geoscope19
Copy link

I’m a new, I don’t understand what to do. Could you explain to me how to proceed?

@elpaso
Copy link
Contributor Author

elpaso commented Sep 22, 2023

I’m a new, I don’t understand what to do. Could you explain to me how to proceed?

Sure, you couldn't possibly know, it is well hidden.

Download this package on windows, unzip it twice in a folder of your choice and run the qgis exe.

https://github.com/qgis/QGIS/suites/16418208318/artifacts/938243476

This is a QGIS build automatically created with the changes in this PR so you can test the changes.

@Geoscope19
Copy link

@elpaso Thank you.
Impeccable, the bug is fixed :-)

@elpaso
Copy link
Contributor Author

elpaso commented Sep 22, 2023

@elpaso Thank you. Impeccable, the bug is fixed :-)

Thanks for checking!

@Geoscope19
Copy link

It is quite normal, it is you who are struggling to improve Qgis, it is rather for me to thank you for all the work

@elpaso elpaso removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Sep 22, 2023
Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit I'm no expert in this part of the code, sorry if I failed to understand the cause of some of this modifications.

src/core/layout/qgslayoutitemlegend.cpp Show resolved Hide resolved
Comment on lines +1143 to +1144
layersToClip = mMap->atlasClippingSettings()->layersToClip();
for ( QgsMapLayer *layer : std::as_const( layersToClip ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
layersToClip = mMap->atlasClippingSettings()->layersToClip();
for ( QgsMapLayer *layer : std::as_const( layersToClip ) )
const QList<QgsMapLayer *> layersToClip = mMap->atlasClippingSettings()->layersToClip();
for ( QgsMapLayer *layer : layersToClip)

We can reduce the scope of layersToClip here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the std::as_const should stay though even if it is redundant: I was told several times from other developers that it makes things clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: isn't my original version slightly faster? I don't know if the compiler can optimize it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can keep the std::as_const indeed, it doesn't harm.

Not sure to understand why it would be slighly faster. Anyway it is purely cosmetic and non blocking for merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can keep the std::as_const indeed, it doesn't harm.

Not sure to understand why it would be slighly faster. Anyway it is purely cosmetic and non blocking for merge

because the assignment down inside the loop is done multiple times instead of once before the loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my comment was just to move the declaration at the same line of the assignment, not inside the loop. But nevermind that's not a big deal.

src/core/layout/qgslayoutitemlegend.cpp Show resolved Hide resolved
@elpaso elpaso merged commit 096d741 into qgis:master Sep 26, 2023
30 checks passed
@qgis-bot
Copy link
Collaborator

The backport to queued_ltr_backports failed:

The process '/usr/bin/git' failed with exit code 1
stderr
error: could not apply d156fbabfc... ATLAS: LEGEND support clipping
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/core/layout/qgslayoutitemlegend.cpp
CONFLICT (content): Merge conflict in src/core/layout/qgslayoutitemlegend.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-54692-to-queued_ltr_backports
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick d156fbabfcdd5bf632f2643d933a507d6885f96b,bbfefcfc81ef81f70df5b2e8d7ca1c79395859f0
# Push it to GitHub
git push --set-upstream origin backport-54692-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-54692-to-queued_ltr_backports.

@qgis-bot qgis-bot added the failed backport The automated backport attempt failed, needs a manual backport label Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter! failed backport The automated backport attempt failed, needs a manual backport Print Layouts Related to QGIS Print Layouts, Atlas or Reporting frameworks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

taxon legend from map whis atlas
4 participants