[office] Add support for internal Goto link in PDF. #43
Conversation
|
The second commit in this PR adds support for in-page shifts for goto links. In PDF Goto links, the destination is defined by :
I separated the implementation into two commits, because the first just add a functionality, without changing the code base, while the second commit needs to change some internal API of existing code. |
| return d->pages.value( index ).rect.y(); | ||
| const QRectF& rect = d->pages.value( index ).rect; | ||
| qreal minOffset = 0., maxOffset = 1.; | ||
| return rect.y() + rect.height() * qBound(minOffset, offset, maxOffset); |
There was a problem hiding this comment.
Minor detail, and fine as is too, but to me offset sounds like a discrete value, pixels for example. Also as qml side calculates contentX, maybe it could be simpler and more consistent, if this method would be called pageRectangle() and qml would decide position inside the page?
There was a problem hiding this comment.
I agree to change offset that's not sounding for what it is. Maybe something like "qreal inPage".
For the rest, I may have not understood your point. For me, this value is mandatory in the function because every single page height is not known from QML side and because of spacing between pages, it is a bit cumbersome to use pagePosition(i+1) - pagePosition(i). ContentX is different because it refers to the width which is known from the QML side.
There was a problem hiding this comment.
The later sugestion was replacing qreal pagePosition(int) with QRectF pageRectangle(int). That gets converted into qml rect type and code in PDFView.qml can decide what position between rect.y and (rect.y + rect.height) to set.
There was a problem hiding this comment.
Ah, finally I got your point, sorry being slow ! You're absolutely right. I like your suggestion, I'll amend the commit to change it for this.
|
Nice! Couple comments, but on general good to go from my side. |
|
I've amended the commits to:
|
| if (gotoLink->isExternal()) | ||
| break; | ||
| QRectF linkArea = link->linkArea(); | ||
| QUrl linkURL = QUrl("?"); |
There was a problem hiding this comment.
Minor detail, but I think "?" is unnecessary here.
There was a problem hiding this comment.
I agree, I remove it.
|
The case of scrolling beyond document end is still there. On the pdf I linked can be triggered by having zoom level at minimum, full document width shown. Then clicking "6" on first page (can be quite hard to hit as the link text is very small) -> document jumps to section, application background is shown on the right and bottom half of the screen. Press and small movement makes the document jump back inside screen. If you want, can already merge this as is and have above fixed separately. |
|
Oups, sorry, no don't merge. I forgot this remark. I'll correct it before merge. |
|
Ok, I've added tests to check that a link don't send the view beyond the limits. By the way, this issue was pre-existing, for instance with a PDF document with pages smaller that the screen, going to the last page (with the goto input field), put this page on top of the screen and thus leave much empty space below. So one minor issue less ! |
|
Concerning the usability issue, I've tested with a wiggleFactor = 15.f in pdfcanvas.cpp. It helps a bit but it's not very convincing. One could increase this value more, but I don't think it's a good idea, especially when links are close because the urlAtPoint() algorithm (see pdfcanvas.cpp) currently stops on the first expanded link rectangle that contains the tap. So if we expand link rectangles too much, the first overlapping rectangle will hide all other ones. Instead, I propose to change a bit the urlAtPoint() algorithm (see pdfcanvas.cpp) to use a distance between non-expanded link rectangles and tap point. We select the link as the min of these distances and also if it is below a (big) threshold given by Theme.itemSizeExtraSmall for instance (to be resolution independant). What do you think ? |
|
Yea, makes sense to use closest link instead of topmost overlapping. Also, this looks good to me now. Merging. |
[office] Add support for internal Goto link in PDF.
This adds partial support for internal links in PDF documents. It makes the view scrolls to the target page of the link. It should contribute to issue #42. Currently, the inside of the target page itself is not scrolled.