PDF views: jump to precise locations

PdfScrollingPageView and PdfMultiPageView were already jumping to the
right page; but if you have zoomed in very far, you need it to jump
to the right part of the page too.

This affects how it jumps to search results, link locations, and the
forward/back behavior.  All of those should be more precise and
repeatable now.  But we depend on some new features that are added to
TableView for Qt 6; in lieu of those, we use TableViewExtra for now.

Fixes: QTBUG-83679
Change-Id: Ie974205562fe7dbf93bae274cef6fefa768aaefb
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

7 files changed, 126 insertions, 53 deletions

diff --git a/src/pdf/api/qpdflinkmodel_p_p.h b/src/pdf/api/qpdflinkmodel_p_p.h
index 3e44f1651..0454d6755 100644
--- a/src/pdf/api/qpdflinkmodel_p_p.h
+++ b/src/pdf/api/qpdflinkmodel_p_p.h
@@ -74,7 +74,7 @@ public:
         // destination inside PDF
         int page = -1; // -1 means look at the url instead
         QPointF location;
-        qreal zoom = 1;
+        qreal zoom = 0; // 0 means no specified zoom: don't change when clicking
         // web destination
         QUrl url;
 
diff --git a/src/pdf/quick/qml/PdfMultiPageView.qml b/src/pdf/quick/qml/PdfMultiPageView.qml
index 14856b1e2..9de9e1767 100644
--- a/src/pdf/quick/qml/PdfMultiPageView.qml
+++ b/src/pdf/quick/qml/PdfMultiPageView.qml
@@ -48,15 +48,15 @@ Item {
 
     property string selectedText
     function selectAll() {
-        var currentItem = tableView.itemAtPos(0, tableView.contentY + root.height / 2)
-        if (currentItem !== null)
+        var currentItem = tableHelper.itemAtCell(tableHelper.cellAtPos(root.width / 2, root.height / 2))
+        if (currentItem)
             currentItem.selection.selectAll()
     }
     function copySelectionToClipboard() {
-        var currentItem = tableView.itemAtPos(0, tableView.contentY + root.height / 2)
+        var currentItem = tableHelper.itemAtCell(tableHelper.cellAtPos(root.width / 2, root.height / 2))
         if (debug)
             console.log("currentItem", currentItem, "sel", currentItem.selection.text)
-        if (currentItem !== null)
+        if (currentItem)
             currentItem.selection.copyToClipboard()
     }
 
@@ -69,14 +69,18 @@ Item {
     function goToPage(page) {
         if (page === navigationStack.currentPage)
             return
-        goToLocation(page, Qt.point(0, 0), 0)
+        goToLocation(page, Qt.point(-1, -1), 0)
     }
     function goToLocation(page, location, zoom) {
-        if (zoom > 0)
+        if (zoom > 0) {
+            navigationStack.jumping = true // don't call navigationStack.update() because we will push() instead
             root.renderScale = zoom
-        navigationStack.push(page, location, zoom)
-        searchModel.currentPage = page
+            tableView.forceLayout() // but do ensure that the table layout is correct before we try to jump
+            navigationStack.jumping = false
+        }
+        navigationStack.push(page, location, zoom) // actually jump
     }
+    property vector2d jumpLocationMargin: Qt.vector2d(10, 10)  // px from top-left corner
 
     // page scaling
     property real renderScale: 1
@@ -121,23 +125,14 @@ Item {
         property bool rot90: rotationNorm == 90 || rotationNorm == 270
         onRot90Changed: forceLayout()
         property size firstPagePointSize: document === undefined ? Qt.size(0, 0) : document.pagePointSize(0)
-        contentWidth: document === undefined ? 0 : (rot90 ? document.maxPageHeight : document.maxPageWidth) * root.renderScale + vscroll.width + 2
-        // workaround for missing function (see https://codereview.qt-project.org/c/qt/qtdeclarative/+/248464)
-        function itemAtPos(x, y, includeSpacing) {
-            // we don't care about x (assume col 0), and assume includeSpacing is true
-            var ret = null
-            for (var i = 0; i < contentItem.children.length; ++i) {
-                var child = contentItem.children[i];
-                if (root.debug)
-                    console.log(child, "@y", child.y)
-                if (child.y < y && (!ret || child.y > ret.y))
-                    ret = child
-            }
-            if (root.debug && ret !== null)
-                console.log("given y", y, "found", ret, "@", ret.y)
-            return ret // the delegate with the largest y that is less than the given y
-        }
+        property real pageHolderWidth: Math.max(root.width, document === undefined ? 0 :
+                                         (rot90 ? document.maxPageHeight : document.maxPageWidth) * root.renderScale)
+        contentWidth: document === undefined ? 0 : pageHolderWidth + vscroll.width + 2
         rowHeightProvider: function(row) { return (rot90 ? document.pagePointSize(row).width : document.pagePointSize(row).height) * root.renderScale }
+        TableViewExtra {
+            id: tableHelper
+            tableView: tableView
+        }
         delegate: Rectangle {
             id: pageHolder
             color: root.debug ? "beige" : "transparent"
@@ -147,11 +142,8 @@ Item {
                 rotation: -90; text: pageHolder.width.toFixed(1) + "x" + pageHolder.height.toFixed(1) + "\n" +
                                      image.width.toFixed(1) + "x" + image.height.toFixed(1)
             }
-            implicitWidth: Math.max(root.width, (tableView.rot90 ? document.maxPageHeight : document.maxPageWidth) * root.renderScale)
+            implicitWidth: tableView.pageHolderWidth
             implicitHeight: tableView.rot90 ? image.width : image.height
-            onImplicitWidthChanged: tableView.forceLayout()
-            objectName: "page " + index
-            property int delegateIndex: row // expose the context property for JS outside of the delegate
             property alias selection: selection
             Rectangle {
                 id: paper
@@ -347,42 +339,82 @@ Item {
             property bool moved: false
             onPositionChanged: moved = true
             onActiveChanged: {
-                var currentItem = tableView.itemAtPos(0, tableView.contentY + root.height / 2)
-                var currentPage = currentItem.delegateIndex
-                var currentLocation = Qt.point((tableView.contentX - currentItem.x + root.width / 2) / root.renderScale,
-                                               (tableView.contentY - currentItem.y + root.height / 2) / root.renderScale)
+                var cell = tableHelper.cellAtPos(root.width / 2, root.height / 2)
+                var currentItem = tableHelper.itemAtCell(cell)
+                var currentLocation = Qt.point(0, 0)
+                if (currentItem) { // maybe the delegate wasn't loaded yet
+                    currentLocation = Qt.point((tableView.contentX - currentItem.x + jumpLocationMargin.x) / root.renderScale,
+                                               (tableView.contentY - currentItem.y + jumpLocationMargin.y) / root.renderScale)
+                }
                 if (active) {
                     moved = false
-                    navigationStack.push(currentPage, currentLocation, root.renderScale)
+                    // emitJumped false to avoid interrupting a pinch if TableView thinks it should scroll at the same time
+                    navigationStack.push(cell.y, currentLocation, root.renderScale, false)
                 } else if (moved) {
-                    navigationStack.update(currentPage, currentLocation, root.renderScale)
+                    navigationStack.update(cell.y, currentLocation, root.renderScale)
                 }
             }
         }
         ScrollBar.horizontal: ScrollBar { }
     }
     onRenderScaleChanged: {
+        // if navigationStack.jumped changes the scale, don't turn around and update the stack again;
+        // and don't force layout either, because positionViewAtCell() will do that
+        if (navigationStack.jumping)
+            return
         tableView.forceLayout()
-        var currentItem = tableView.itemAtPos(tableView.contentX + root.width / 2, tableView.contentY + root.height / 2)
-        if (currentItem !== undefined)
-            navigationStack.update(currentItem.delegateIndex, Qt.point(currentItem.x / renderScale, currentItem.y / renderScale), renderScale)
+        var cell = tableHelper.cellAtPos(root.width / 2, root.height / 2)
+        var currentItem = tableHelper.itemAtCell(cell)
+        if (currentItem) {
+            var currentLocation = Qt.point((tableView.contentX - currentItem.x + jumpLocationMargin.x) / root.renderScale,
+                                           (tableView.contentY - currentItem.y + jumpLocationMargin.y) / root.renderScale)
+            navigationStack.update(cell.y, currentLocation, renderScale)
+        }
     }
     PdfNavigationStack {
         id: navigationStack
+        property bool jumping: false
+        property int previousPage: 0
         onJumped: {
+            jumping = true
             root.renderScale = zoom
-            tableView.contentX = Math.max(0, location.x - root.width / 2) * root.renderScale
-            tableView.contentY = tableView.originY + root.document.heightSumBeforePage(page, tableView.rowSpacing / root.renderScale) * root.renderScale
-            if (root.debug) {
-                console.log("going to page", page,
-                            "@y", root.document.heightSumBeforePage(page, tableView.rowSpacing / root.renderScale) * root.renderScale,
-                            "ended up @", tableView.contentY, "originY is", tableView.originY)
+            if (location.y < 0) {
+                // invalid to indicate that a specific location was not needed,
+                // so attempt to position the new page just as the current page is
+                var currentYOffset = 0
+                var previousPageDelegate = tableHelper.itemAtCell(0, previousPage)
+                if (previousPageDelegate)
+                    currentYOffset = tableView.contentY - previousPageDelegate.y
+                tableHelper.positionViewAtRow(page, Qt.AlignTop, currentYOffset)
+                if (root.debug) {
+                    console.log("going from page", previousPage, "to", page, "offset", currentYOffset,
+                                "ended up @", tableView.contentX.toFixed(1) + ", " + tableView.contentY.toFixed(1))
+                }
+            } else {
+                // jump to a page and position the given location relative to the top-left corner of the viewport
+                var pageSize = root.document.pagePointSize(page)
+                pageSize.width *= root.renderScale
+                pageSize.height *= root.renderScale
+                var xOffsetLimit = Math.max(0, pageSize.width - root.width) / 2
+                var offset = Qt.point(Math.max(-xOffsetLimit, Math.min(xOffsetLimit,
+                                        location.x * root.renderScale - jumpLocationMargin.x)),
+                                      Math.max(0, location.y * root.renderScale - jumpLocationMargin.y))
+                tableHelper.positionViewAtCell(0, page, Qt.AlignLeft | Qt.AlignTop, offset)
+                if (root.debug) {
+                    console.log("going to zoom", zoom, "loc", location, "on page", page,
+                                "ended up @", tableView.contentX.toFixed(1) + ", " + tableView.contentY.toFixed(1))
+                }
             }
+            jumping = false
+            previousPage = page
         }
+        onCurrentPageChanged: searchModel.currentPage = currentPage
     }
     PdfSearchModel {
         id: searchModel
         document: root.document === undefined ? null : root.document
-        onCurrentPageChanged: if (currentPage != navigationStack.currentPage) root.goToPage(currentPage)
+        // TODO maybe avoid jumping if the result is already fully visible in the viewport
+        onCurrentResultBoundingRectChanged: root.goToLocation(currentPage,
+            Qt.point(currentResultBoundingRect.x, currentResultBoundingRect.y), 0)
     }
 }
diff --git a/src/pdf/quick/qml/PdfScrollablePageView.qml b/src/pdf/quick/qml/PdfScrollablePageView.qml
index 3a3727cc4..51d9e530d 100644
--- a/src/pdf/quick/qml/PdfScrollablePageView.qml
+++ b/src/pdf/quick/qml/PdfScrollablePageView.qml
@@ -136,18 +136,26 @@ Flickable {
     PdfSearchModel {
         id: searchModel
         document: root.document === undefined ? null : root.document
-        onCurrentPageChanged: root.goToPage(currentPage)
+        // TODO maybe avoid jumping if the result is already fully visible in the viewport
+        onCurrentResultBoundingRectChanged: root.goToLocation(currentPage,
+            Qt.point(currentResultBoundingRect.x, currentResultBoundingRect.y), 0)
     }
 
     PdfNavigationStack {
         id: navigationStack
         onJumped: {
             root.renderScale = zoom
-            root.contentX = Math.max(0, location.x * root.renderScale - root.width / 2)
-            root.contentY = Math.max(0, location.y * root.renderScale - root.height / 2)
-            if (root.debug)
+            var dx = Math.max(0, location.x * root.renderScale - root.width / 2) - root.contentX
+            var dy = Math.max(0, location.y * root.renderScale - root.height / 2) - root.contentY
+            // don't jump if location is in the viewport already, i.e. if the "error" between desired and actual contentX/Y is small
+            if (Math.abs(dx) > root.width / 3)
+                root.contentX += dx
+            if (Math.abs(dy) > root.height / 3)
+                root.contentY += dy
+            if (root.debug) {
                 console.log("going to zoom", zoom, "loc", location,
                             "on page", page, "ended up @", root.contentX + ", " + root.contentY)
+            }
         }
         onCurrentPageChanged: searchModel.currentPage = currentPage
     }
diff --git a/src/pdf/quick/qquickpdfnavigationstack.cpp b/src/pdf/quick/qquickpdfnavigationstack.cpp
index 7ba317557..044023ef6 100644
--- a/src/pdf/quick/qquickpdfnavigationstack.cpp
+++ b/src/pdf/quick/qquickpdfnavigationstack.cpp
@@ -90,6 +90,8 @@ void QQuickPdfNavigationStack::forward()
     if (forwardAvailableWas != forwardAvailable())
         emit forwardAvailableChanged();
     m_changing = false;
+    qCDebug(qLcNav) << "forward: index" << m_currentHistoryIndex << "page" << currentPage()
+                    << "@" << currentLocation() << "zoom" << currentZoom();
 }
 
 /*!
@@ -120,6 +122,8 @@ void QQuickPdfNavigationStack::back()
     if (!forwardAvailableWas)
         emit forwardAvailableChanged();
     m_changing = false;
+    qCDebug(qLcNav) << "back: index" << m_currentHistoryIndex << "page" << currentPage()
+                    << "@" << currentLocation() << "zoom" << currentZoom();
 }
 
 /*!
@@ -163,13 +167,14 @@ qreal QQuickPdfNavigationStack::currentZoom() const
     \qmlmethod void PdfNavigationStack::push(int page, point location, qreal zoom)
 
     Adds the given destination, consisting of \a page, \a location and \a zoom,
-    to the history of visited locations.
+    to the history of visited locations.  If \a emitJumped is \c false, the
+    \l jumped() signal will not be emitted.
 
     If forwardAvailable is \c true, calling this function represents a branch
     in the timeline which causes the "future" to be lost, and therefore
     forwardAvailable will change to \c false.
 */
-void QQuickPdfNavigationStack::push(int page, QPointF location, qreal zoom)
+void QQuickPdfNavigationStack::push(int page, QPointF location, qreal zoom, bool emitJumped)
 {
     if (page == currentPage() && location == currentLocation() && zoom == currentZoom())
         return;
@@ -192,7 +197,8 @@ void QQuickPdfNavigationStack::push(int page, QPointF location, qreal zoom)
         emit backAvailableChanged();
     if (forwardAvailableWas)
         emit forwardAvailableChanged();
-    emit jumped(page, location, zoom);
+    if (emitJumped)
+        emit jumped(page, location, zoom);
     qCDebug(qLcNav) << "push: index" << m_currentHistoryIndex << "page" << page
                     << "@" << location << "zoom" << zoom << "-> history" <<
         [this]() {
diff --git a/src/pdf/quick/qquickpdfnavigationstack_p.h b/src/pdf/quick/qquickpdfnavigationstack_p.h
index 8d7102fb1..0d88d62fd 100644
--- a/src/pdf/quick/qquickpdfnavigationstack_p.h
+++ b/src/pdf/quick/qquickpdfnavigationstack_p.h
@@ -67,7 +67,7 @@ class QQuickPdfNavigationStack : public QObject
 public:
     explicit QQuickPdfNavigationStack(QObject *parent = nullptr);
 
-    Q_INVOKABLE void push(int page, QPointF location, qreal zoom);
+    Q_INVOKABLE void push(int page, QPointF location, qreal zoom, bool emitJumped = true);
     Q_INVOKABLE void update(int page, QPointF location, qreal zoom);
     Q_INVOKABLE void forward();
     Q_INVOKABLE void back();
diff --git a/src/pdf/quick/qquickpdfsearchmodel.cpp b/src/pdf/quick/qquickpdfsearchmodel.cpp
index a4b457841..1f62fbad0 100644
--- a/src/pdf/quick/qquickpdfsearchmodel.cpp
+++ b/src/pdf/quick/qquickpdfsearchmodel.cpp
@@ -116,6 +116,29 @@ QVector<QPolygonF> QQuickPdfSearchModel::currentResultBoundingPolygons() const
     return ret;
 }
 
+/*!
+    \qmlproperty point PdfSearchModel::currentResultBoundingRect
+
+    The bounding box containing all \l currentResultBoundingPolygons.
+
+    When this property changes, a scrollable view should automatically scroll
+    itself in such a way as to ensure that this region is visible; for example,
+    it could try to position the upper-left corner near the upper-left of its
+    own viewport, subject to the constraints of the scrollable area.
+*/
+QRectF QQuickPdfSearchModel::currentResultBoundingRect() const
+{
+    QRectF ret;
+    const auto &results = const_cast<QQuickPdfSearchModel *>(this)->resultsOnPage(m_currentPage);
+    if (m_currentResult < 0 || m_currentResult >= results.count())
+        return ret;
+    auto rects = results[m_currentResult].rectangles();
+    ret = rects.takeFirst();
+    for (auto rect : rects)
+        ret = ret.united(rect);
+    return ret;
+}
+
 void QQuickPdfSearchModel::onResultsChanged()
 {
     emit currentPageBoundingPolygonsChanged();
@@ -266,6 +289,7 @@ void QQuickPdfSearchModel::setCurrentResult(int currentResult)
     m_currentResult = currentResult;
     emit currentResultChanged();
     emit currentResultBoundingPolygonsChanged();
+    emit currentResultBoundingRectChanged();
 }
 
 /*!
diff --git a/src/pdf/quick/qquickpdfsearchmodel_p.h b/src/pdf/quick/qquickpdfsearchmodel_p.h
index 3e05f80e3..66fc583d9 100644
--- a/src/pdf/quick/qquickpdfsearchmodel_p.h
+++ b/src/pdf/quick/qquickpdfsearchmodel_p.h
@@ -64,6 +64,7 @@ class QQuickPdfSearchModel : public QPdfSearchModel
     Q_PROPERTY(int currentResult READ currentResult WRITE setCurrentResult NOTIFY currentResultChanged)
     Q_PROPERTY(QVector<QPolygonF> currentPageBoundingPolygons READ currentPageBoundingPolygons NOTIFY currentPageBoundingPolygonsChanged)
     Q_PROPERTY(QVector<QPolygonF> currentResultBoundingPolygons READ currentResultBoundingPolygons NOTIFY currentResultBoundingPolygonsChanged)
+    Q_PROPERTY(QRectF currentResultBoundingRect READ currentResultBoundingRect NOTIFY currentResultBoundingRectChanged)
 
 public:
     explicit QQuickPdfSearchModel(QObject *parent = nullptr);
@@ -81,6 +82,7 @@ public:
 
     QVector<QPolygonF> currentPageBoundingPolygons() const;
     QVector<QPolygonF> currentResultBoundingPolygons() const;
+    QRectF currentResultBoundingRect() const;
 
 signals:
     void documentChanged();
@@ -88,6 +90,7 @@ signals:
     void currentResultChanged();
     void currentPageBoundingPolygonsChanged();
     void currentResultBoundingPolygonsChanged();
+    void currentResultBoundingRectChanged();
 
 private:
     void updateResults();