Fix inconsistent viewport positioning in PageView

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.

Reviewers: #okular, sander

Reviewed By: sander

Subscribers: ngraham, sander, aacid, okular-devel

Tags: #okular

Differential Revision: https://phabricator.kde.org/D16941

autotests/data/simple-multipage.tex

@@ -0,0 +1,82 @@
+\documentclass[12pt, a4paper]{article}
+\begin{document}
+Page 1
+\newpage
+Page 2
+\newpage
+Page 3
+\newpage
+Page 4
+\newpage
+Page 5
+\newpage
+Page 6
+\newpage
+Page 7
+\newpage
+Page 8
+\newpage
+Page 9
+\newpage
+Page 10
+\newpage
+Page 11
+\newpage
+Page 12
+\newpage
+Page 13
+\newpage
+Page 14
+\newpage
+Page 15
+\newpage
+Page 16
+\newpage
+Page 17
+\newpage
+Page 18
+\newpage
+Page 19
+\newpage
+Page 20
+\newpage
+Page 21
+\newpage
+Page 22
+\newpage
+Page 23
+\newpage
+Page 24
+\newpage
+Page 25
+\newpage
+Page 26
+\newpage
+Page 27
+\newpage
+Page 28
+\newpage
+Page 29
+\newpage
+Page 30
+\newpage
+Page 31
+\newpage
+Page 32
+\newpage
+Page 33
+\newpage
+Page 34
+\newpage
+Page 35
+\newpage
+Page 36
+\newpage
+Page 37
+\newpage
+Page 38
+\newpage
+Page 39
+\newpage
+Page 40
+\end{document}

autotests/parttest.cpp

@@ -118,6 +118,7 @@ class PartTest
         void testCrashTextEditDestroy();
         void testAnnotWindow();
         void testAdditionalActionTriggers();
+        void testJumpToPage();
 
     private:
         void simulateMouseSelection(double startX, double startY, double endX, double endY, QWidget *target);
@@ -1751,6 +1752,35 @@ void PartTest::testAdditionalActionTriggers()
     verifyTargetStates( QStringLiteral( "pb" ), fields, false, false, false, __LINE__ );
 }
 
+void PartTest::testJumpToPage() {
+    const QString testFile = QStringLiteral( KDESRCDIR "data/simple-multipage.pdf" );
+    const int targetPage = 25;
+    Okular::Part part( nullptr, nullptr, QVariantList() );
+    part.openDocument( testFile );
+    part.widget()->resize(800, 600);
+    part.widget()->show();
+    QVERIFY( QTest::qWaitForWindowExposed( part.widget() ) );
+
+    part.m_document->pages();
+    part.m_document->setViewportPage( targetPage );
+
+    /* Document::setViewportPage triggers pixmap rendering in another thread.
+     * We want to test how things look AFTER finished signal arrives back,
+     * because PageView::slotRelayoutPages may displace the viewport again.
+     */
+    QTRY_VERIFY( part.m_document->page( targetPage )->hasPixmap( part.m_pageView ) );
+
+    const int contentAreaHeight = part.m_pageView->verticalScrollBar()->maximum() + part.m_pageView->viewport()->height();
+    const int pageWithSpaceTop = contentAreaHeight / part.m_document->pages() * targetPage;
+
+    /*
+     * This is a test for a "known by trial" displacement.
+     * We'd need access to part.m_pageView->d->items[targetPage]->croppedGeometry().top(),
+     * to determine the expected viewport position, but we don't have access.
+     */
+    QCOMPARE(part.m_pageView->verticalScrollBar()->value(), pageWithSpaceTop - 4);
+}
+
 } // namespace Okular
 
 int main(int argc, char *argv[])

ui/pageview.cpp

@@ -1331,36 +1331,15 @@ void PageView::slotRealNotifyViewportChanged( bool smoothMove )
         slotRelayoutPages();
 
     // restore viewport center or use default {x-center,v-top} alignment
-    const QRect & r = item->croppedGeometry();
-    int newCenterX = r.left(),
-        newCenterY = r.top();
-    if ( vp.rePos.enabled )
-    {
-        if ( vp.rePos.pos == Okular::DocumentViewport::Center )
-        {
-            newCenterX += (int)( normClamp( vp.rePos.normalizedX, 0.5 ) * (double)r.width() );
-            newCenterY += (int)( normClamp( vp.rePos.normalizedY, 0.0 ) * (double)r.height() );
-        }
-        else
-        {
-            // TopLeft
-            newCenterX += (int)( normClamp( vp.rePos.normalizedX, 0.0 ) * (double)r.width() + viewport()->width() / 2 );
-            newCenterY += (int)( normClamp( vp.rePos.normalizedY, 0.0 ) * (double)r.height() + viewport()->height() / 2 );
-        }
-    }
-    else
-    {
-        newCenterX += r.width() / 2;
-        newCenterY += viewport()->height() / 2 - 10;
-    }
+    const QPoint centerCoord = viewportToContentArea( vp );
 
     // if smooth movement requested, setup parameters and start it
     if ( smoothMove )
     {
         d->viewportMoveActive = true;
         d->viewportMoveTime.start();
-        d->viewportMoveDest.setX( newCenterX );
-        d->viewportMoveDest.setY( newCenterY );
+        d->viewportMoveDest.setX( centerCoord.x() );
+        d->viewportMoveDest.setY( centerCoord.y() );
         if ( !d->viewportMoveTimer )
         {
             d->viewportMoveTimer = new QTimer( this );
@@ -1372,7 +1351,7 @@ void PageView::slotRealNotifyViewportChanged( bool smoothMove )
         horizontalScrollBar()->setEnabled( false );
     }
     else
-        center( newCenterX, newCenterY );
+        center( centerCoord.x(), centerCoord.y() );
     d->blockPixmapsRequest = false;
 
     // request visible pixmaps in the current viewport and recompute it
@@ -3805,6 +3784,35 @@ void PageView::scrollPosIntoView( const QPoint & pos )
     else d->dragScrollTimer.stop();
 }
 
+QPoint PageView::viewportToContentArea( const Okular::DocumentViewport & vp ) const {
+    Q_ASSERT( vp.pageNumber >= 0 );
+
+    const QRect & r = d->items[ vp.pageNumber ]->croppedGeometry();
+    QPoint c { r.left(), r.top() };
+
+    if ( vp.rePos.enabled )
+    {
+        if ( vp.rePos.pos == Okular::DocumentViewport::Center )
+        {
+            c.rx() += qRound( normClamp( vp.rePos.normalizedX, 0.5 ) * (double)r.width() );
+            c.ry() += qRound( normClamp( vp.rePos.normalizedY, 0.0 ) * (double)r.height() );
+        }
+        else
+        {
+            // TopLeft
+            c.rx() += qRound( normClamp( vp.rePos.normalizedX, 0.0 ) * (double)r.width() + viewport()->width() / 2 );
+            c.ry() += qRound( normClamp( vp.rePos.normalizedY, 0.0 ) * (double)r.height() + viewport()->height() / 2 );
+        }
+    }
+    else
+    {
+        // exact repositioning disabled, align page top margin with viewport top border by default
+        c.rx() += r.width() / 2;
+        c.ry() += viewport()->height() / 2 - 10;
+    }
+    return c;
+}
+
 void PageView::updateSelection( const QPoint & pos )
 {
     if ( d->mouseSelecting )
@@ -4644,11 +4652,10 @@ void PageView::slotRelayoutPages()
             {
                 int prevX = horizontalScrollBar()->value(),
                     prevY = verticalScrollBar()->value();
-                const QRect & geometry = d->items[ vp.pageNumber ]->croppedGeometry();
-                double nX = vp.rePos.enabled ? normClamp( vp.rePos.normalizedX, 0.5 ) : 0.5,
-                       nY = vp.rePos.enabled ? normClamp( vp.rePos.normalizedY, 0.0 ) : 0.0;
-                center( geometry.left() + qRound( nX * (double)geometry.width() ),
-                        geometry.top() + qRound( nY * (double)geometry.height() ) );
+
+                const QPoint centerPos = viewportToContentArea( vp );
+                center( centerPos.x(), centerPos.y() );
+
                 // center() usually moves the viewport, that requests pixmaps too.
                 // if that doesn't happen we have to request them by hand
                 if ( prevX == horizontalScrollBar()->value() && prevY == verticalScrollBar()->value() )

ui/pageview.h

@@ -198,6 +198,7 @@ Q_OBJECT
         QMenu* createProcessLinkMenu( PageViewItem *item, const QPoint & eventPos );
         // used when selecting stuff, makes the view scroll as necessary to keep the mouse inside the view
         void scrollPosIntoView( const QPoint & pos );
+        QPoint viewportToContentArea( const Okular::DocumentViewport & vp ) const;
 
         // called from slots to turn off trim modes mutually exclusive to id
         void updateTrimMode( int except_id );