Hold zoom center below center/mouse position
Needs RevisionPublic

Authored by steffenh on Jun 12 2019, 9:00 AM.

Details

Reviewers
ngraham
aacid
kezik
Group Reviewers
Okular
Summary

This patch hold (if possibly) the zoom center below the mouse position (zoom with Ctrl + Mouse Wheel and Middle mouse button) or the center position from the pinch gesture.

BUG: 390707

Diff Detail

Repository
R223 Okular
Branch
holdZoomCenter
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13432
Build 13450: arc lint + arc unit
steffenh created this revision.Jun 12 2019, 9:00 AM
Restricted Application added a project: Okular. · View Herald TranscriptJun 12 2019, 9:00 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
steffenh requested review of this revision.Jun 12 2019, 9:00 AM
ngraham edited the summary of this revision. (Show Details)Jun 12 2019, 2:53 PM

Using cursor position for Ctrl+Scroll, but not for Ctrl+Plus/Minus makes sense to me.

Can this be extended to using the initial cursor position for middle-button-drag zooming?

steffenh updated this revision to Diff 59730.Jun 13 2019, 2:22 PM

Use initial cursor position for middle-button-drag zooming

rework the calculation for newScroll, because d->zoomFactor don't hold the exact zoom factor.

ngraham accepted this revision.Jun 14 2019, 4:33 PM
ngraham added a reviewer: aacid.
ngraham added a subscriber: ngraham.

Interactivity is great, a big improvement! Code looks sane to me too but I'd like a review from a Okular person before we land this.

This revision is now accepted and ready to land.Jun 14 2019, 4:33 PM

there's a few types

activ -> active

beginn -> begin
and please make centerBeing and centerEnd const

Also it doesn't work? or not how i would expect it?

I'm doing ctrl+wheel zoom here with the mouse over the a and ends up being over the p.

I was expecting it to end up being over the a too?

Hi @aacid,
thanks for testing,

Also it doesn't work? or not how i would expect it?

I'm doing ctrl+wheel zoom here with the mouse over the a and ends up being over the p.

I was expecting it to end up being over the a too?

to begin the width of the document is smaller than the viewport with, so we have no horizontal scrollbar and we can not make a horizontal scrolling.
You can see in your picture the zoom from 100 to 400 the mouse start and ends up being over the p, here we have a scrollbar and can make a horizontal scrolling.

steffenh updated this revision to Diff 60400.Jun 23 2019, 6:57 AM

fix typos and make centerBegin and centerEnd const

ngraham requested changes to this revision.Jun 23 2019, 9:56 AM

Hmm, now it doesn't work for me anymore. It zooms in to a location near the cursor/center pinch point, but not exactly there.

This revision now requires changes to proceed.Jun 23 2019, 9:56 AM

Hi @aacid,
thanks for testing,

Also it doesn't work? or not how i would expect it?

I'm doing ctrl+wheel zoom here with the mouse over the a and ends up being over the p.

I was expecting it to end up being over the a too?

to begin the width of the document is smaller than the viewport with, so we have no horizontal scrollbar and we can not make a horizontal scrolling.

Sure, but at some point there will be a scrollbar, and at that point you can make it scroll to the "correct", position, no?

Hi @aacid,
thanks for testing,

Also it doesn't work? or not how i would expect it?

I'm doing ctrl+wheel zoom here with the mouse over the a and ends up being over the p.

I was expecting it to end up being over the a too?

to begin the width of the document is smaller than the viewport with, so we have no horizontal scrollbar and we can not make a horizontal scrolling.

Sure, but at some point there will be a scrollbar, and at that point you can make it scroll to the "correct", position, no?

Do you mean it should always try to scroll where the zoom was triggered the first time after Ctrl was pressed the last time? I think it makes sense, but only marginally more than scolling to the position of the individual scroll event.

I just tried this patch, and zooming with the middle mouse button is unusable now. The view heavily jaggs arround all the time, you can’t see anything. Maybe make viewport->update() a queued connection in slotRelayoutPages()?

Otherwise it works like I expect.

Maybe it makes sense to stick the cursor position while zooming with the middle mouse button, so the zoom center is shown. (I know that only by Spore.)

aacid added a comment.Jun 23 2019, 3:35 PM

Do you mean it should always try to scroll where the zoom was triggered the first time after Ctrl was pressed the last time?

That is what i was expecting this to do yes, if it's supposed to center on the mouse position, it should do that regardless of if there were scrollbars or not

steffenh updated this revision to Diff 60870.Jun 30 2019, 4:00 PM
steffenh edited the summary of this revision. (Show Details)
  • lock mouse cursor for middle mouse button zooming
  • adjust for disappear and appear of scrollbars
  • try to recenter if zoom center moving below the mouse cursor

Oops, sorry I lost track of this patch. Can you rebase it please so we can continue the review?

Behaviorally this works great, but visually there's now a big regression: the content itself is no longer visible during the zoom operation; the view blanks out and displays the default white background.

Yes, I have found this, if okular is build / using new poppler versions, but I don't know why. (poppler version poppler-0.77.0 and poppler-0.62.0 is working)

aacid requested changes to this revision.Feb 21 2020, 6:53 PM

Please move as a Merge Request in https://invent.kde.org/kde/okular

We have pre-commit CI and lots of checks including clazy and clang-tidy there so it's a much better place for doing the review/approval/merge of the code.

This revision now requires changes to proceed.Feb 21 2020, 6:53 PM