Differential D14093

Toggle between zoom options "Fit"/"Fill" and "100%" on multiple activation
ClosedPublic
Actions

Authored by muhlenpfordt on Jul 13 2018, 9:03 AM.

Details

Reviewers
rkflx
Group Reviewers
Gwenview
Commits
R260:945c8a7a8d0b: Toggle between zoom options "Fit"/"Fill" and "100%" on multiple activation
Summary

In previous versions of Gwenview it was possible to toggle between
Fit and 100% zoom factors using the shortcut F.
After 3e10699ac37c added the Fit Width zoom option this feature
did not work any more.
This patch adds toggling for both Fit and Fill zoom
options if the shortcut F or one of the buttons is pressed
multiple times. Zooming to 100% is centered to the current cursor
position like middle click works.

BUG: 396360

Test Plan
  1. Open Gwenview in View Mode
  2. Use zoom buttons Fit/Fill or F multiple times
  3. The zoom should toggle between Fit and 100%, respectively between Fill and 100%

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Jul 13 2018, 9:03 AM
muhlenpfordt created this revision.
muhlenpfordt added a reviewer: Gwenview.Jul 13 2018, 9:08 AM
muhlenpfordt added inline comments.
lib/documentview/abstractimageview.cpp
271

If the two flags are not exclusive, toggling the zoom states results in some strange effects sometimes.

lib/documentview/documentviewcontroller.cpp
191–193

Listening to triggered is needed to receive signals for the already checked actions.

249

setChecked() does not emit the triggered signal (only toggled), so we don't need this any more.

muhlenpfordt updated this revision to Diff 37691.Jul 13 2018, 9:56 AM

Removed SignalBlocker include

rkflx added a subscriber: rkflx.Jul 13 2018, 3:24 PM

Works great, code mostly LGTM (but I have not yet understood DocumentView::setZoom*, see inline comment).

And for middle-clicking, it could work like this

  • (to be added to the docbook)
  • Shift + middle-click: Toggle between Fill and 100%

To warrant a proper BUG:, I'll sort out both points (code for the second one done already ;) from the bug in a follow-up patch.

Two more ideas I had when testing your patch (up for discussion and possibly followup-patches):

  • Instead of switching to 100%, switch to the zoom level which was applied just before toggling Fit or Fill. Only if there is none, fall back to 100%. Just an idea, not sure if such a behaviour would make sense at all.
  • Ctrl-zooming respects the cursor position. This could also be done when pressing any keyboard shortcut which would result in a zoom action (does not make sense for Fit, of course), instead of simply zooming relative to the center of the image.
lib/documentview/documentview.cpp
615

This looks really suspicious, because simply discarding the boolean parameter of a function is not what is normally expected.

Could not look into this in detail yet, but as far as I can see those zooming functions are all over the place. Do you have a good understanding of the situation to untangle them a bit? (See also my comment about the enum in T8220#151117, which might help with making 100% work similarly).

muhlenpfordt planned changes to this revision.Jul 13 2018, 4:18 PM
muhlenpfordt added inline comments.
lib/documentview/documentview.cpp
615

Maybe this parameter is really needed. I found an issue with compare mode and Synchronize option enabled. Clicking on one of the images switches to Fill zoom every time.
In DocumentViewSynchronizer these functions are called for all DocumentViews.
I'll take a closer look on this...

rkflx added inline comments.Jul 13 2018, 10:25 PM
lib/documentview/documentview.cpp
615

I'm sure you can solve it! Lots of users are asking for the toggling, BTW: https://forum.kde.org/viewtopic.php?f=213&t=141631

muhlenpfordt updated this revision to Diff 37731.Jul 14 2018, 9:25 AM

Move toggling code to separate slots

muhlenpfordt added a comment.Jul 14 2018, 9:25 AM

I think there are two reasons that make it necessary to move the toggling code to separate slots:

  • DocumentView::setZoomToFit/Fill() is used from various locations where the on parameter is important
  • The parameter checked of the triggered signal and the checked state of the zoom actions are unusable since they're always true when the triggered slot is called - this seems to be an effect of the QActionGroup in ZoomWidget linking all zoom buttons.
rkflx accepted this revision.Jul 14 2018, 10:56 PM

Thanks, Synchronize is working great now.

I guess adding the extra slots is not all that bad, at least the names now hint at what the slots are for.

This revision is now accepted and ready to land.Jul 14 2018, 10:56 PM
muhlenpfordt updated this revision to Diff 37845.Jul 16 2018, 7:27 AM
muhlenpfordt edited the summary of this revision. (Show Details)

Center 100% zoom to current cursor position

muhlenpfordt added inline comments.Jul 16 2018, 7:28 AM
lib/documentview/documentview.cpp
427

This way any cursor position inside the Gwenview window is used to center.
We could move this inside a if (view->underMouse()) { ... } to center on cursor position inside the image view only.
Both ways are ok for me. What do you think?

rkflx requested changes to this revision.Jul 16 2018, 10:47 AM

Nice! Tested it:

  • Working great for Fit.
  • We should really add the fallback for !view->underMouse(), otherwise it's just too confusing.
  • In Compare mode it does not work right for every document. Adding mapFromScene fixes it for me.
  • For Fill, clicking on the bottom of a tall and completely visible image fits to width, but centers instead of panning to the bottom parts. (Not a blocker to prevent the patch from making it into the Beta on Thursday, but nonetheless maybe you can find the issue, which might be in the zooming code for filling.)
In D14103#292792, @muhlenpfordt wrote:

Interesting. Is this really working for you without the patches? For me, even middle-clicking with a KDE4-based Gwenview centers instead of respecting the mouse position.

I never saw it not working. No matter what Kubuntu version I used, up to 18.04 (Frameworks 5.44 / Qt 5.9.5).

You are right (again). You added it in D11336: Keep click position in focus on middle-click zoom for 18.04 which I totally forgot about. So of course KDE4 did not have it, and somehow I tested 17.12 instead of master leading to my incorrect conclusion.

Anyway, everything cleared up now ;)

It's not easy to map the current mouse position from global QCursor::pos() in a QGraphicsWidget.

Ah, I've come across mapFromGlobal in Gwenview before when trying to fix bugs in the hover UI: PreviewItemDelegate::slotRowsChanged.

lib/documentview/documentview.cpp
423–426

I first thought about writing it like this (as I'd assume there'll always be a view at this point):

const auto view = q->scene()->views().first();

…but your for loop is probably safer.

This revision now requires changes to proceed.Jul 16 2018, 10:47 AM
muhlenpfordt added a comment.Jul 16 2018, 11:44 AM
In D14093#292918, @rkflx wrote:
  • In Compare mode it does not work right for every document. Adding mapFromScene fixes it for me.

Ahh, once again I forgot about the compare mode. Thanks for the hint - mapFromScene() corrects it.

  • For Fill, clicking on the bottom of a tall and completely visible image fits to width, but centers instead of panning to the bottom parts. (Not a blocker to prevent the patch from making it into the Beta on Thursday, but nonetheless maybe you can find the issue, which might be in the zooming code for filling.)

Maybe I don't see the problem...
Switching to Fill mode never respects the cursor position, only when going to 100%. The only difference I see is when Fill and 100% zoom factors are identical and display will not change on clicking.

muhlenpfordt updated this revision to Diff 37867.Jul 16 2018, 11:44 AM

Map cursor position to current image
Limit cursor centering to image view

rkflx accepted this revision.Jul 16 2018, 1:05 PM

Let's land what we have, and continue working on the rest after that.

In D14093#292980, @muhlenpfordt wrote:
  • For Fill, clicking on the bottom of a tall and completely visible image fits to width, but centers instead of panning to the bottom parts. (Not a blocker to prevent the patch from making it into the Beta on Thursday, but nonetheless maybe you can find the issue, which might be in the zooming code for filling.)

Maybe I don't see the problem...
Switching to Fill mode never respects the cursor position, only when going to 100%. The only difference I see is when Fill and 100% zoom factors are identical and display will not change on clicking.

Yeah, for Fit centering the complete document and centering to the cursor position are equivalent, as there's nothing to pan once the document fits. However for Fill there is a decision to be made where to pan to. For some cases (e.g. zooming out) this already works okay, but for zooming in from the minimum zoom level it does not:

In other words, imagine a document is displayed in Fit mode, and the user wants to see more details for a particular part of the image:

  • Middle-click will zoom to 100% and show the part the user is interested in.
  • ⇧-middle-click will zoom to Fill (which is good enough in this case), but center instead of panning correctly.

Note that the same behaviour as for Fill can also be observed for 100% once you add a shortcut for it for toggling from the keyboard.

Example document: F6110894

This revision is now accepted and ready to land.Jul 16 2018, 1:05 PM
muhlenpfordt added a comment.Jul 16 2018, 3:13 PM
In D14093#293037, @rkflx wrote:

However for Fill there is a decision to be made where to pan to. For some cases (e.g. zooming out) this already works okay, but for zooming in from the minimum zoom level it does not:

Ok, I thought there was something implemented but broken, it's more a todo. ;)
I'm working on it for another patch.

Closed by commit R260:945c8a7a8d0b: Toggle between zoom options "Fit"/"Fill" and "100%" on multiple activation (authored by muhlenpfordt). · Explain WhyJul 16 2018, 3:19 PM
This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Jul 16 2018, 10:13 PM
In D14093#293088, @muhlenpfordt wrote:

Ok, I thought there was something implemented but broken, it's more a todo. ;)
I'm working on it for another patch.

Yup, originally I also did not know whether this was a bug or more of a todo ;) Now we know better!

Also, thanks for approving my patch.

Last off-topic comment for today: Maybe you already noticed, but Applications/18.08 has now been created by Albert, so make sure to use it if applicable…

Revision Contents
Changeset List

Diff 37880

View Options

lib/documentview/abstractimageview.cpp

Loading...
View Options

lib/documentview/documentview.h

Loading...
View Options

lib/documentview/documentview.cpp

Loading...
View Options

lib/documentview/documentviewcontroller.cpp

Loading...