Bug 414701 - Kinetic scrolling breaks Back and Forward navigation
Summary: Kinetic scrolling breaks Back and Forward navigation
Status: RESOLVED FIXED
Alias: None
Product: okular
Classification: Applications
Component: general (show other bugs)
Version: 1.11.3
Platform: Compiled Sources Linux
: VHI normal
Target Milestone: ---
Assignee: Okular developers
URL:
Keywords: regression
: 427788 427903 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-11-30 21:01 UTC by Rodolfo Panerai
Modified: 2020-11-23 21:36 UTC (History)
11 users (show)

See Also:
Latest Commit:
Version Fixed In: 20.12


Attachments
An open access article with hyperlinks which demonstrate the bug (914.73 KB, application/pdf)
2020-10-18 19:37 UTC, Yuri Chornoivan
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rodolfo Panerai 2019-11-30 21:01:51 UTC
SUMMARY
Git commit 51b90ecd73e37b9646d8a4bbb51e4fa815942912 (implementing Kinetic Scrolling) breaks Back and Forward navigation.

STEPS TO REPRODUCE
Open any PDF with hyperlinks and use them to jump to different locations in the document. Then use Back (Alt+Shift+Left) and Forward (Alt+Shift+Right) to navigate through the history.

OBSERVED RESULT
Back (Alt+Shift+Left) just scrolls up a few lines. Forward (Alt+Shift+Right) stays mostly disabled.


SOFTWARE/OS VERSIONS
Linux: Arch Linux (up to date)
KDE Plasma Version: 5.17.3
KDE Frameworks Version: 5.64.0
Qt Version: 5.13.2
Comment 1 Albert Astals Cid 2019-11-30 21:39:44 UTC
Oliver, ¿Kezi? more things that your patch regressed.

Please fix ASAP.
Comment 2 Keziolio 2019-11-30 22:54:19 UTC
This seems to be the offending commit

https://invent.kde.org/kezik/okular/commit/6516c91302b24d8e48545393edb67f2a6c0e599b
Comment 3 Keziolio 2019-12-01 15:16:13 UTC
diff --git a/ui/pageview.cpp b/ui/pageview.cpp
index e0401826d..b97f05a62 100644
--- a/ui/pageview.cpp
+++ b/ui/pageview.cpp
@@ -1372,7 +1372,7 @@ void PageView::slotRealNotifyViewportChanged( bool smoothMove )
     const QPoint centerCoord = viewportToContentArea( vp );
 
     // if smooth movement requested, setup parameters and start it
-    center( centerCoord.x(), centerCoord.y(), smoothMove );
+    center( centerCoord.x(), centerCoord.y(), false );
 
     d->blockPixmapsRequest = false;




This works around the problem disabling the smooth scrolling, I don't see what side effect that would have on the navigation
Comment 4 Oliver Sander 2019-12-02 16:37:06 UTC
Kezi, do you think you can find the cause for the problem in short time?  Otherwise we may have to revert your patch until we got the regressions sorted out.  Thanks!
Comment 5 Keziolio 2019-12-02 22:30:55 UTC
I'll have more time later this week
Comment 6 Oliver Sander 2020-01-11 18:26:53 UTC
Fixed by Kezo Olio in 09cb524f1753ca1c9272618f9ceaded4c8ed7cb4 .

See https://invent.kde.org/kde/okular/merge_requests/77
Comment 7 Rodolfo Panerai 2020-10-14 15:26:59 UTC
The bug seems to be back (Okular 1.11.2).

Unfortunately, at the moment I don't have the time to bisect again to find the commit that reintroduced the bug.

SOFTWARE/OS VERSIONS
Linux: Arch Linux (up to date)
KDE Plasma Version: 5.20.0
KDE Frameworks Version: 5.75.0
Qt Version: 5.15.1
Comment 8 Rodolfo Panerai 2020-10-14 20:08:30 UTC
Well, I ended up bisecting it.

The offending commit is bdb2df773d774c15cf0a17fcae198d3f2452cdfa (Fix forms when inertially scrolling).

https://invent.kde.org/graphics/okular/commit/bdb2df773d774c15cf0a17fcae198d3f2452cdfa

Curiously, it reverts the change to the same line in ui/pageview.cpp that was fixed by commit 09cb524f1753ca1c9272618f9ceaded4c8ed7cb4 mentioned earlier in this bugreport (same author).
Comment 9 Albert Astals Cid 2020-10-17 22:56:54 UTC
Nate, another one for your lists of scrolling related things that need fixing.
Comment 10 Albert Astals Cid 2020-10-18 18:00:25 UTC
*** Bug 427788 has been marked as a duplicate of this bug. ***
Comment 11 Albert Astals Cid 2020-10-18 18:01:29 UTC
*** Bug 427903 has been marked as a duplicate of this bug. ***
Comment 12 Nate Graham 2020-10-18 18:07:36 UTC
Whoops, I'll look into that today.
Comment 13 Nate Graham 2020-10-18 19:23:18 UTC
Can anyone attach a PDF with hyperlinks to different sections of the document that I can test with?
Comment 14 Yuri Chornoivan 2020-10-18 19:37:19 UTC
Created attachment 132535 [details]
An open access article with hyperlinks which demonstrate the bug
Comment 15 Nate Graham 2020-10-18 19:37:47 UTC
Thanks so much Yuri!
Comment 16 Nate Graham 2020-10-18 19:41:52 UTC
Can confirm the issue.

It looks like the back/forward navigation calculates the prior position in the document based on scroll position and was not designed for animated scrolling. So the animated scrolling effect effectively populates the "location history" with multiple locations in the document when it should not.

I'll see what I can do. Worst-case scenario, we revert animated scrolling when clicking on links until we can fix the root cause and support both.
Comment 17 Oliver Sander 2020-10-18 19:59:36 UTC
I think I remember Kezi complaining about the ViewPort setting location history events itself, too.  Wouldn't it make more sense to make the code that triggers the location change request explicitly that the old location be recorded?  Something like

  // an internal link has been clicked
  recordLocation(here);
  scrollTo(LinkDestination)
Comment 18 Nate Graham 2020-10-18 20:15:14 UTC
Yeah that's what would make sense to me. Need to see if it's possible given the existing implementation. A targeted bugfix will probably be needed for 1.11.3 if we can make it in time.
Comment 19 Albert Astals Cid 2020-10-18 20:18:16 UTC
(In reply to Oliver Sander from comment #17)
> I think I remember Kezi complaining about the ViewPort setting location
> history events itself, too.  Wouldn't it make more sense to make the code
> that triggers the location change request explicitly that the old location
> be recorded?  Something like
> 
>   // an internal link has been clicked
>   recordLocation(here);
>   scrollTo(LinkDestination)

He complained because he broke everything and it's easier to complain that the old code is wrong than to accept that your new code is not as good as you think it is.

Let's be honest, the old code was perfectly working, going back and forth in history would animate the viewport (i still don't understand why we needed a new animation method while we had an existing one in place) and it did this animation without destroying history, so if the new code even with him introducing a new setViewportWithHistory method is not working, shows how bad the new code is.
Comment 20 Keziolio 2020-10-18 20:20:35 UTC
The old code was updating history inside the request visible pixmap function, those two things are semantically not the same, they were put in the same place for reasons unknown to me, my new implementation just exposed this original bad design decision
Comment 21 David Hurka 2020-10-18 21:34:23 UTC
> Can anyone attach a PDF with hyperlinks to different sections of
> the document that I can test with?
What happened to your LM555? ;)

> I think I remember Kezi complaining about the ViewPort setting location history
> events itself, too.  Wouldn't it make more sense to make the code that triggers
> the location change request explicitly that the old location be recorded? [...]
>     recordLocation(here);
That sounds like the natural solution to me. Then scrolling doesn’t need to know *why* it scrolls right now.

Besides that, I can’t reproduce the problem as described (as I understand it). If I enable smooth scrolling, there is no history updated when I press e. g. Arrow Down. Instead, the history is updated when I press Back and Forward, so I circle between the last two viewports only.
Comment 22 Kishore Gopalakrishnan 2020-10-19 04:48:20 UTC
(In reply to Nate Graham from comment #16)
> Can confirm the issue.
> 
> It looks like the back/forward navigation calculates the prior position in
> the document based on scroll position and was not designed for animated
> scrolling. So the animated scrolling effect effectively populates the
> "location history" with multiple locations in the document when it should
> not.
> 
> I'll see what I can do. Worst-case scenario, we revert animated scrolling
> when clicking on links until we can fix the root cause and support both.

The same history bug also appears when you use the 'go to page' option with animated scrolling enabled, so in case you do this, you would also have to disable animated scrolling for the 'go to page' feature.
Comment 23 Michael D 2020-10-22 10:15:03 UTC
Is it possible that this bugs also affects the "go to first/last page" commands? I've noticed that invoking those commands sometimes do not actually navigate to to the beginning/end but somewhere before them. Hard to reproduce though.
Comment 24 Nate Graham 2020-11-03 02:58:41 UTC
Git commit 585340cef5ca4dd1e26fe3baea1cf396f8de6469 by Nate Graham, on behalf of Kezi Olio.
Committed on 03/11/2020 at 02:58.
Pushed by ngraham into branch 'master'.

Fix back/forward navigation after clicking on a link

The condition was incorrect; we were ignoring history when not in the
middle of scrolling when we wanted to do the reverse.
FIXED-IN: 20.12

M  +1    -1    ui/pageview.cpp

https://invent.kde.org/graphics/okular/commit/585340cef5ca4dd1e26fe3baea1cf396f8de6469
Comment 25 Rodolfo Panerai 2020-11-05 23:39:49 UTC
Still broken.

The only thing that works now is moving back to the latest point in the navigation history. In other words, the first click on "Back" brings you to the correct place, any further click on it just moves a few lines up or down.

"Forward" is also completely broken (most of the time not even enabled).
Comment 26 Rodolfo Panerai 2020-11-05 23:54:48 UTC
My bad. I assumed that, being a regression, this was meant to be fixed in the new minor release but I see now that the patch is not in the 20.08 branch. It is scheduled for the 20.12 version.

Closing again.
Comment 27 Rajeesh K V 2020-11-09 11:51:21 UTC
Would it be possible to backport this fix to 20.08.y minor release (if any is planned)?
This issue affects a number of users in my organization.
Comment 28 Nate Graham 2020-11-09 14:48:18 UTC
I'm afraid not, as the last bugfix release for the current major Okular version was already released and there won't be any more before the next major version. However there's nothing stopping distros from backporting the fix for their users. You might want to bring that up with your distro's packagers.
Comment 29 Nate Graham 2020-11-10 18:16:40 UTC
*** Bug 428922 has been marked as a duplicate of this bug. ***
Comment 30 Rajeesh K V 2020-11-11 03:21:02 UTC
Thanks to Rex Dieter, this fix will be carried in Fedora.
https://bugzilla.redhat.com/show_bug.cgi?id=1896246