Fix inconsistent viewport positioning in PageView
ClosedPublic

Authored by tobiasdeiminger on Nov 17 2018, 9:03 AM.

Details

Summary

This diff unifies the calculation of the viewport position from a given DocumentViewport. PageView::notifyViewportChanged and PageView::slotRelayoutPages used to handle it differntly, which resulted in viewport jumps for no reason.

It happened in various situations, e.g. when jumping to a page using the footer page navigation, or when reloading the document after presentation mode left, or when resizing the main window after presentation mode left.

The diff selects the notifyViewportChanged way (align viewport top border with page top margin) as golden behavior in case of rePos.enabled == false.

BUGS: 357958
CCBUG: 341939
CCBUG: 400890

341939 and 400890 are fixed partially. These two still suffer from a minor displacement that happens when finished signal arrives from pixmap generation thread.

Test Plan
  • When using the footer page navigation to jump to different pages, new page top is always algined with viewport top.
  • After changing page with footer page navigation, press F5 to reload. Page top stays aligned with viewport top.
  • When exiting presentation mode, and touching the file, page top stays aligned with viewport top.
  • When exiting presentation mode, and changing main window size, page top stays aligned with viewport top.

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Okular. · View Herald TranscriptNov 17 2018, 9:03 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
tobiasdeiminger requested review of this revision.Nov 17 2018, 9:03 AM
aacid added a subscriber: aacid.Nov 17 2018, 9:12 AM

Can we have an autotest?

Can we have an autotest?

Yes will try, but probably won't get to it today.

Assert correctly.

sander added a subscriber: sander.Nov 17 2018, 8:18 PM

This looks like a definite improvement. It is still not perfect -- see my description at https://bugs.kde.org/show_bug.cgi?id=400890#c3 .

As you seem to have acquired quite some understanding of that viewport positioning logic, could you maybe add some extra inline documentation for future generations?

It is still not perfect

I can reproduce everything you describe there. After touching the file once, a cascade of several notifyViewportChanged and slotRelayoutPages breaks off, where page->croppedGeometry().top(), pageView->viewport()->height() and rePos.enabled wildly change values throughout (this happens with and without the patch applied). It's quite messy to debug, I only have vague ideas what's going on.

tobiasdeiminger edited the summary of this revision. (Show Details)Nov 18 2018, 7:00 AM
tobiasdeiminger edited the test plan for this revision. (Show Details)

@aacid When is the last chance to get something into 18.12 release? Already passed?

I'm asking because we have two interfering issues here, where the first should be reasonably fixed with this diff (test still missing), but the second may take longer. Maybe we can split it, and get the first in?

  1. inconsistent calculation of viewport placement (fixed now)
  2. viewport displaced a bit again, when finished signals arrive from pixmap generation thread asynchronously, causing notifyPageChanged( page, DocumentObserver::BoundingBox ) and PageView::slotRelayoutPages

If there's a chance to get 1 in, I'll focus on it (i.e., implement a test).

Add test to check PageView position after Document::setViewportPage

It basically checks the expected scrollbar displacement, because that's some public information accessible by the test method.

However few pixel depend on how big the empty grey space between pages is. That space is not easy to pre-calculate. So there is some "known-by-trial" portion in the expected value. I wonder if there's a better way?

How hard would be to extend the test to do a F5/refresh and check position is the same? That was also a bug and this fixes it, right?

How hard would be to extend the test to do a F5/refresh and check position is the same? That was also a bug and this fixes it, right?

It wouldn't be too hard. But the F5 bug is only fixed partially with this diff (see D16941#362806 regarding finished signal from pixmap thread), so I'd rather leave that test out for now. Hence I'm going to remove 341939 and 400890 from the BUGS list again. Only 357958 is completely fixed, the others are improved but not fixed.

Has anybody found some time yet to try the jump-to-page test? I'm a bit concerned it might work only in my environment, but not in others (because of the "known by trial" portion).

Has anybody found some time yet to try the jump-to-page test?

It passes on my Debian Testing laptop.

tobiasdeiminger edited the summary of this revision. (Show Details)Nov 26 2018, 8:54 PM

It passes on my Debian Testing laptop.

Thanks for trying. Then the diff is only one accept away from landing...

Also fixes 341939 and 400890 partially

maybe mention those using CCBUG?

CCBUG: 341939
CCBUG: 400890
tobiasdeiminger edited the summary of this revision. (Show Details)Nov 28 2018, 8:33 AM
sander accepted this revision.Nov 28 2018, 8:23 PM
This revision is now accepted and ready to land.Nov 28 2018, 8:23 PM
aacid added inline comments.Nov 28 2018, 10:15 PM
ui/pageview.cpp
1334

const

4656

const

  • Add test to check PageView position after Document::setViewportPage
  • Fix review issues
tobiasdeiminger marked 2 inline comments as done.Nov 29 2018, 8:41 PM
This revision was automatically updated to reflect the committed changes.