sailfishos / sailfish-office

Sailfish Office
GNU General Public License v2.0
73 stars 26 forks source link

[office] Add a back button after an internal link has been taped. #96

Closed dcaliste closed 7 years ago

dcaliste commented 7 years ago

Add a back button in the toolbar when tapped on an internal goto link, see #89. Also add smooth scroll to the link location.

When tapping on an internal goto link, the view scrolls to the link target location and the toolbar is shown. The toolbar is kept visible, up to the moment the back button is either tapped, the view is scrolled more than the height of a screen, or the zoom changes. Going back to the previous location is also smoothly animated.

pvuorela commented 7 years ago

Seems a nice feature. Couple details, though:

pvuorela commented 7 years ago

As last resort for text fitting can also use on Label: width: Math.min(parent.width - Theme.paddingSmall, implicitWidth) fontSizeMode: Text.HorizontalFit

Seems good enough when there is no back button. Could create a pull request to handle current situation with that already. For back button shown, maybe could keep page number item same width as normally, but scale all the others?

dcaliste commented 7 years ago

I agree that scroll (either for goto links or search results) should follow the QuickScroll style when going further than one or two pages. I didn't think about it but it's a very good suggestion. I'll do what I can achieve with what is under BSD in Sailfish/Silica and report here later.

dcaliste commented 7 years ago

I've added a commit trying to mimick what happen in private/FastScrollAnimator.qml. It's not as sophisticated as implemented there, but it should keep the spirit and durations. What do you think ?

pvuorela commented 7 years ago

Didn't yet go to code side, but scrolling looks so much nicer now. As small detail the page counter can briefly show start+1 and end-1 while animating. Could consider avoiding those intermediate page numbers?

Having this on top of my text fitting, can see how the text size changes if the document is long enough. Could maybe use minimum width of Screen.width / 4 on that? (Extra points for omitting on screen.sizeCategory > Screen.Medium which likely just has enough space)

dcaliste commented 7 years ago

As small detail the page counter can briefly show start+1 and end-1 while animating. Could consider avoiding those intermediate page numbers?

That's true, but I have no idea how to prevent this easily (signal being emitted by PDF.Canvas object). But, I propose an alternative solution : show the toolbar only after the scroll is complete and hide the toolbar as soon as the scroll back is started (instead of waiting for the timeout). Like that, it's not obvious that the page numbers are changing.

Could maybe use minimum width of Screen.width / 4 on that?

I've added a simple commit implementing this, separated from the back button commits. The commit may go by itself if you think so, or together with the back button PR.

pvuorela commented 7 years ago

That's true, but I have no idea how to prevent this easily (signal being emitted by PDF.Canvas object). But, I propose an alternative solution : show the toolbar only after the scroll is complete and hide the toolbar as soon as the scroll back is started (instead of waiting for the timeout). Like that, it's not obvious that the page numbers are changing.

For example something like PDFView: property int currentPage: _transitioningTo >= 0 ? _transitioningTo : pdfCanvas.currentPage, transition to set overriding page number when starting and setting -1 when done.

Temporary hiding doesn't sound good to me, intermediate page number could be lesser evil.

dcaliste commented 7 years ago

For example something like PDFView: property int currentPage

Of course, silly me. I don't know why I was focussing on the currentPage not being emitted in such a scheme when transition finishes, which is wrong of course. Anyway, I've updated the last commit (the one implementing quick scroll) to display directly the destination page in case of quick jump.

I've also reestablished the toolbar being shown as soon as link is activated, but I keep the automatic hiding as soon as back button is clicked. If you prefer, I can put it back to the previous behaviour with the toolbar being hidden after timeout after going back.

pvuorela commented 7 years ago

LGTM.

But still nitpicking a little: second commit includes a merge conflict. If you want, I can also squash last two commits and push that.

dcaliste commented 7 years ago

second commit includes a merge conflict.

Arf, really poorly done rebase… It's corrected.