-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add Last & Next zoom actions in layouts #58789
base: master
Are you sure you want to change the base?
Conversation
be685de
to
a6bf1bf
Compare
@YoannQDQ got a screencast or screenshot of this in action? (It's great to see you back! 😁 🥳) |
a6bf1bf
to
8f896a5
Compare
Done! Also fixed pan handling. By the way @nyalldawson I noticed I can no longer edit the label(s) on the issues I create. Could I maybe get the privilege back? Thanks. |
dc9abb6
to
bbbf7ce
Compare
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
bbbf7ce
to
ec5b8ec
Compare
ec5b8ec
to
5184972
Compare
5184972
to
ff61225
Compare
ff61225
to
0192669
Compare
src/gui/layout/qgslayoutview.cpp
Outdated
disconnect( this, &QgsLayoutView::zoomLevelChanged, this, &QgsLayoutView::extentChanged ); | ||
emit zoomLevelChanged(); | ||
connect( this, &QgsLayoutView::zoomLevelChanged, this, &QgsLayoutView::extentChanged ); |
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.
This looks wrong -- won't a resize generally cause a different extent? (eg when resizing to a different aspect ratio).
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.
Yes, but actually, the term extentChanged
is a bit misleading. What matters is only the center coordinates and the transform, and those won't change if the view is resized.
The problem I had was calling zoomNext or zoomLast can make the scrollbar appear or disappear, which triggers a resizeEvent, creating zoom history wipe or loops.
Also, not sure what is the emit zoomLevelChanged();
purpose here ; should the cache really be invalidated when the view is resized? If not, we can just get rid off those three lines.
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.
The problem I had was calling zoomNext or zoomLast can make the scrollbar appear or disappear, which triggers a resizeEvent, creating zoom history wipe or loops.
I'd handle that by adding a timer in the code which adds an entry to the zoom history. If the last entry was added < say 1 second ago, then we ignore the new one (or even better, replace the previous one!). That would also help with the case of say zooming in using a mouse wheel + ctrl key, which will fire many zoom level changes in a rapid period of time.
Also, not sure what is the emit zoomLevelChanged(); purpose here ; should the cache really be invalidated when the view is resized? If not, we can just get rid off those three lines.
Yes, it should. It triggers re-updating layout items at the new zoom level, because otherwise we'd draw them in a pixelated resized version of their previous render.
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.
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.
You're right, sorry! We don't need the zoomElevelChanged signal there.
connect( this, &QgsLayoutView::zoomLevelChanged, this, &QgsLayoutView::extentChanged ); | ||
|
||
verticalScrollBar()->installEventFilter( this ); | ||
horizontalScrollBar()->installEventFilter( this ); |
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.
Could all this logic be moved into the existing QgsLayoutView::viewChanged method? That should already be getting called whenever the view extent is changed, and already handles all the different ways this can happen.
That should also avoid the emit view()->extentChanged();
calls from other classes, which is rather fragile.
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.
The problem is viewChanged
is called every time the scrollbar moves. So dragging the scrollbar handle will trigger many MANY viewChanged calls, and we dont want them to clutter the zoom level history. Similarly, we do not want to add entries in the history while dragging the view with the pan tool.
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.
See above -- I'd handle this by using a timer to "collapse" extents which were used very close to each other.
For bonus points you could handle Qt::BackButton and Qt::ForwardButton in QgsLayoutView::mouseReleaseEvent, just like we do in QgsMapCanvas. 🥳 |
0192669
to
f1805b3
Compare
f1805b3
to
0353c3f
Compare
Description
Adapt QgsMapCanvas zoomToLastExtent / zoomToNextExtent to the QgsLayoutView.