fix(pdf-roll): scroll from the page the last scroll reached

[alberti42]

What happens

With pdf-view-roll-minor-mode on, a command that scrolls more than once per invocation cannot reach page one. pdf-view-previous-line-or-previous-page with an argument is one, since it runs pdf-roll-scroll-backward in a dotimes; so is any key bound to several scrolls.

The view moves up to the top of a page, crosses into the previous page, and then moves back down to near the bottom of the page it started on. Pressing the key again repeats that, so the document stays on the same two pages.

A trace from a window on page 4 at vscroll 40, running five backward scrolls of one line in a single command (frame char height 23, page height 2144):

scroll 1   page 4   vscroll 17
scroll 2   page 3   vscroll 2127     <- crossed the boundary, correct
scroll 3   page 4   vscroll 2104     <- back on page 4
scroll 4   page 4   vscroll 2081
scroll 5   page 4   vscroll 2058

The command was asked for 115 pixels upward and produced about 2000 downward. pdf-view-next-line-or-next-page has the same problem in the other direction.

Scrolling with a wheel or a trackpad does not show it, because those deliver one scroll per event with a redisplay in between.

Why

pdf-roll-scroll-forward and pdf-roll-scroll-backward record the page they reach in pdf-view-current-page and set the vscroll, but they do not set window-start. pdf-roll-pre-redisplay does that, from pdf-view-current-page, and it runs once per redisplay. Both functions nevertheless start from (window-start window). Within one command there is no redisplay between the scrolls, so every scroll after the first starts from the page the command began on.

A scroll that stays inside one page is unaffected: it works from the vscroll, which the previous scroll did set, and window-start still names the right page. A scroll that crosses a page boundary leaves a vscroll belonging to the page it moved to — a large number, because it measures from the top of that page down to near its bottom. The next scroll combines that number with the stale window-start and reads it as a position near the bottom of the page the command began on.

pdf-roll-scroll-backward picks the same stale value up a second time through pos-visible-in-window-p, whose RTOP is measured against window-start.

The fix

pdf-roll-window-start returns pdf-roll-page-to-pos of pdf-view-current-page, which is the position the next redisplay will give window-start. Both scroll functions start from it. In pdf-roll-scroll-backward, the pixels already scrolled past come from window-vscroll instead of pos-visible-in-window-p, for the same reason.

The same trace after the change:

scroll 1   page 4   vscroll 17
scroll 2   page 3   vscroll 2127
scroll 3   page 3   vscroll 2104
scroll 4   page 3   vscroll 2081
scroll 5   page 3   vscroll 2058

Separately, pdf-roll-scroll-backward ended with (pdf-roll-set-vscroll (- n) window), and N is still positive when the walk stops at page one, so it stored a negative vscroll. set-window-vscroll clamps that to zero, but image-mode-window-put keeps the negative number and pdf-roll-pre-redisplay and pdf-roll-display-pages then pass it on. Now (max 0 (- n)).

Tests

One test in test/pdf-roll-test.el, for pdf-roll-window-start: with pdf-view-current-page on page 3 and window-start still on page 1, it has to report page 3's position. The scroll functions themselves need a live window and a running server, so they are not covered in batch, in keeping with the note at the top of that file.

emacs -Q --batch -L lisp -l ert -l test/pdf-roll-test.el -f ert-run-tests-batch-and-exit
Ran 9 tests, 9 results as expected, 0 unexpected

No new byte-compile warnings.

Relation to #361, #362 and #338

Independent of #361 and #362, and branched off master like both of them. The three change different functions; lisp/pdf-roll.el merges without a conflict. Only test/pdf-roll-test.el conflicts with #362, because both append a section at the end of the file.

#338 rewrites both scroll functions and does not fix this: its versions still begin with (goto-char (window-start window)), so the second scroll of a command still starts from the wrong page. Its pdf-roll-scroll-backward also combines that window-start with (pdf-view-image-size t window) and (window-vscroll nil t), and both of those report on the selected window rather than on WINDOW. It does already contain the (max 0 (- n)) clamp, so that part of this change is present there.