Oliver, ¿Kezi? more things that your patch regressed.

Please fix ASAP.

This seems to be the offending commit

https://invent.kde.org/kezik/okular/commit/6516c91302b24d8e48545393edb67f2a6c0e599b

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

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!

I'll have more time later this week

Fixed by Kezo Olio in 09cb524f1753ca1c9272618f9ceaded4c8ed7cb4 .

See https://invent.kde.org/kde/okular/merge_requests/77

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

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).

Nate, another one for your lists of scrolling related things that need fixing.

*** Bug 427788 has been marked as a duplicate of this bug. ***

*** Bug 427903 has been marked as a duplicate of this bug. ***

Whoops, I'll look into that today.

Can anyone attach a PDF with hyperlinks to different sections of the document that I can test with?

Created attachment 132535 [details]
An open access article with hyperlinks which demonstrate the bug

Thanks so much Yuri!

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.

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)

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.

(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.

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

> 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.

(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.

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.

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

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).

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.

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.

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.

*** Bug 428922 has been marked as a duplicate of this bug. ***

Thanks to Rex Dieter, this fix will be carried in Fedora.
https://bugzilla.redhat.com/show_bug.cgi?id=1896246