bug#72323: 31.0.50; line-move unconditionally resets vscroll to 0

[Steven Allen via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: "Kim F. Storm" <storm@cua.dk>
>> Date: Sat, 27 Jul 2024 10:57:44 -0700
>> From:  Steven Allen via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Fixing home/end (beginning of line, end of line) case seems trivial:
>> don't call `line-move' in these cases (or have `line-move' short-circuit
>> when `arg' is 0). However, even after reading all the comments about
>> scrolling images, I'm still not sure why it's necessary to reset vscroll
>> to 0.
>
> Because otherwise what line-move does cannot be described in sensible
> terms.  If vscroll is not reset, then what would be the reference for
> such a "line move"?  By its very definition, line-move moves N screen
> lines wrt the line which was its starting point, but with vscroll
> non-zero that starting point could be anywhere.

I'm a bit confused because vscroll is about scrolling the window and
`line-move' is about moving point (only incidentally scrolling the
window if the point leaves it). Clearly `set-window-start' needs to
reset `vscroll', but I'm not sure I understand why `line-move' does.

Is this about detecting the correct column?

> In addition, when we move to another screen line, the value of vscroll
> cannot be meaningfully kept, because its meaning is tightly coupled to
> the screen line where it was applied.  In essence, vscroll is a trick
> to allow display of a tall display element (image, text using a large
> fonts, etc.) in a window whose height is too small to show all of the
> display element at once.
>
>> After commenting this line out, I can't tell a difference, even
>> when scrolling images with and without `auto-window-vscroll' and
>> `try-vscroll'.
>
> line-move is not just for scrolling across images, it is also about
> scrolling across tall text lines and other display elements.  In any
> case, asking for removal of that reset is a non-starter, for the
> reasons explained above, so it isn't going to happen.  The solution
> for any Lisp program that doesn't want vscroll to be rest is not to
> call line-move.

Now that I do more testing, I can see how removing that line breaks
`line-move' although I'm still not sure why.

Would it be acceptable to restore the vertical scroll position as long
as `line-move' hasn't otherwise scrolled the screen? See attached patch.

>> I was hoping Kim could shed some light on this.
>
> I'd be thrilled to hear from Kim, but I'm as guilty of the code in
> line-move as he is, so "blame" me if you want.

Ah, sorry, should have gone back further in the blame history.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-vertical-scroll-offset-after-line-move.patch --]
[-- Type: text/x-patch, Size: 5336 bytes --]

From 2f028e30b2e5ba3a3cd9fd2ecaeaff7c3d02b62f Mon Sep 17 00:00:00 2001
From: Steven Allen <steven@stebalien.com>
Date: Sat, 27 Jul 2024 12:54:31 -0700
Subject: [PATCH] Restore vertical-scroll offset after line-move

Restore the vertical-scroll offset after moving lines unless the window
was otherwise scrolled and/or altering the vertical scroll would move
the point off-screen.

* lisp/simple.el (line-move): restore `window-vscroll' when
appropriate (Bug#72323).
---
 lisp/simple.el | 85 ++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 37 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index a9f8b5845d8..71ae175d198 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7930,43 +7930,54 @@ line-move
                    ;; doesn't have very long lines.
                    (long-line-optimizations-p)))
 		 (line-move-partial arg noerror))
-      (set-window-vscroll nil 0 t)
-      (if (and line-move-visual
-	       ;; Display-based column are incompatible with goal-column.
-	       (not goal-column)
-               ;; Lines aren't truncated.
-               (not
-                (and
-                 (or truncate-lines (truncated-partial-width-window-p))
-                 (long-line-optimizations-p)))
-	       ;; When the text in the window is scrolled to the left,
-	       ;; display-based motion doesn't make sense (because each
-	       ;; logical line occupies exactly one screen line).
-	       (not (> (window-hscroll) 0))
-	       ;; Likewise when the text _was_ scrolled to the left
-	       ;; when the current run of vertical motion commands
-	       ;; started.
-	       (not (and (memq last-command
-			       `(next-line previous-line ,this-command))
-			 auto-hscroll-mode
-			 (numberp temporary-goal-column)
-			 (>= temporary-goal-column
-			    (- (window-width) hscroll-margin)))))
-	  (prog1 (line-move-visual arg noerror)
-	    ;; If we moved into a tall line, set vscroll to make
-	    ;; scrolling through tall images more smooth.
-	    (let ((lh (line-pixel-height))
-		  (edges (window-inside-pixel-edges))
-		  (dlh (default-line-height))
-		  winh)
-	      (setq winh (- (nth 3 edges) (nth 1 edges) 1))
-	      (if (and (< arg 0)
-		       (< (point) (window-start))
-		       (> lh winh))
-		  (set-window-vscroll
-		   nil
-		   (- lh dlh) t))))
-	(line-move-1 arg noerror)))))
+      (let ((initial-vscroll (window-vscroll nil t))
+            (initial-window-start (window-start)))
+            (set-window-vscroll nil 0 t)
+            (prog1
+                (if (and line-move-visual
+                         ;; Display-based column are incompatible with goal-column.
+                         (not goal-column)
+                         ;; Lines aren't truncated.
+                         (not
+                          (and
+                           (or truncate-lines (truncated-partial-width-window-p))
+                           (long-line-optimizations-p)))
+                         ;; When the text in the window is scrolled to the left,
+                         ;; display-based motion doesn't make sense (because each
+                         ;; logical line occupies exactly one screen line).
+                         (not (> (window-hscroll) 0))
+                         ;; Likewise when the text _was_ scrolled to the left
+                         ;; when the current run of vertical motion commands
+                         ;; started.
+                         (not (and (memq last-command
+                                         `(next-line previous-line ,this-command))
+                                   auto-hscroll-mode
+                                   (numberp temporary-goal-column)
+                                   (>= temporary-goal-column
+                                       (- (window-width) hscroll-margin)))))
+                    (prog1 (line-move-visual arg noerror)
+                      ;; If we moved into a tall line, set vscroll to make
+                      ;; scrolling through tall images more smooth.
+                      (let ((lh (line-pixel-height))
+                            (edges (window-inside-pixel-edges))
+                            (dlh (default-line-height))
+                            winh)
+                        (setq winh (- (nth 3 edges) (nth 1 edges) 1))
+                        (if (and (< arg 0)
+                                 (< (point) (window-start))
+                                 (> lh winh))
+                            (set-window-vscroll
+                             nil
+                             (- lh dlh) t))))
+                  (line-move-1 arg noerror))
+              ;; Restore the vscroll position, if any.
+              (when (and (not (zerop initial-vscroll))
+                         ;; But not if scrolling would hide the point.
+                         (< initial-vscroll (cdr (posn-x-y (posn-at-point))))
+                         ;; Or if line-move otherwise scrolled the window.
+                         (= initial-window-start (window-start))
+                         (zerop (window-vscroll nil t)))
+                (set-window-vscroll nil initial-vscroll t)))))))
 
 ;; Display-based alternative to line-move-1.
 ;; Arg says how many lines to move.  The value is t if we can move the
-- 
2.45.2