bug#44070: 28.0.50; Minibuffer display "jumps" upon minor edit

[Stefan Monnier]

>> +     (recenter (if (window-minibuffer-p) -1 -3)))))
> This should have a comment that explains the reason for the
> difference.

Good point.  And applies to the other changes as well.
I believe the addition of a config vars takes care of it in the patch below.

> (Btw, does this DTRT when the text in the minibuffer has
> a newline at the end?)

It does, yes.

>>    /* Try to scroll by specified few lines.  */
>>    if ((0 < scroll_conservatively
>> +       || MINI_WINDOW_P (w)
>>         || 0 < emacs_scroll_step
[...]
>>        int ss = try_scrolling (window, just_this_one_p,
>> -                          scroll_conservatively,
>> +                          (MINI_WINDOW_P (w)
>> +                           ? SCROLL_LIMIT + 1
>> +                           : scroll_conservatively),
>>                            emacs_scroll_step,
>
> If we want the minibuffer behave as if scroll-conservatively was set,
> why not simply set scroll-conservatively in each minibuffer?

That was my initial thought as well, but when I tried to implement it,
it quickly turned into a scavenge hunt trying to find all the places
where it needs to be set (and re-set after a kill-all-local-variables).

So in the end, the "simply" qualifier didn't apply at all.
Another option I considered was to do it directly inside
`reset_buffer_local_variables`, but then we need to pass the info about
"this is a buffer meant for the mini-windows" through several layers (or
worse: do it based on the buffer's name), which is again unworthy of the
"simply" qualifier.

At this point I stopped and realized that my motivation was to change
the behavior in some particular *windows* rather than in some particular
*buffers*, so I think setting it buffer-locally in those buffers used as
minibuffers, while being a valid option, is not better than the simpler
patch I sent.

> We could then have a user option, by default on, to do that, and let
> users who like the current (mis)behavior continue having that.

We could add such an option, indeed.

> As a nice bonus, we will then be sure the change doesn't affect
> echo-area messages, only editing in the minibuffer.

Indeed, it makes it easier to test whether a change is due to
this modification.

How 'bout the patch below, then?


        Stefan


diff --git a/lisp/simple.el b/lisp/simple.el
index 2e40e3261c..8c1761797b 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1134,7 +1134,11 @@ end-of-buffer
         ;; If the end of the buffer is not already on the screen,
         ;; then scroll specially to put it near, but not at, the bottom.
         (overlay-recenter (point))
-        (recenter -3))))
+        ;; FIXME: Arguably if `scroll-conservatively' is set, then
+         ;; we should always pass -1 to `recenter'.
+        (recenter (if (and minibuffer-scroll-conservatively
+                           (window-minibuffer-p))
+                      -1 -3)))))
 
 (defcustom delete-active-region t
   "Whether single-char deletion commands delete an active region.
diff --git a/src/xdisp.c b/src/xdisp.c
index 5c80e37581..fb8719628b 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -18820,6 +18820,7 @@ redisplay_window (Lisp_Object window, bool 
just_this_one_p)
 
   /* Try to scroll by specified few lines.  */
   if ((0 < scroll_conservatively
+       || (minibuffer_scroll_conservatively && MINI_WINDOW_P (w))
        || 0 < emacs_scroll_step
        || temp_scroll_step
        || NUMBERP (BVAR (current_buffer, scroll_up_aggressively))
@@ -18830,7 +18831,10 @@ redisplay_window (Lisp_Object window, bool 
just_this_one_p)
       /* The function returns -1 if new fonts were loaded, 1 if
         successful, 0 if not successful.  */
       int ss = try_scrolling (window, just_this_one_p,
-                             scroll_conservatively,
+                             ((minibuffer_scroll_conservatively
+                               && MINI_WINDOW_P (w))
+                              ? SCROLL_LIMIT + 1
+                              : scroll_conservatively),
                              emacs_scroll_step,
                              temp_scroll_step, last_line_misfit);
       switch (ss)
@@ -34538,7 +34542,14 @@ syms_of_xdisp (void)
 
   DEFSYM (Qredisplay_internal_xC_functionx, "redisplay_internal (C function)");
 
-  DEFVAR_BOOL("inhibit-message", inhibit_message,
+  DEFVAR_BOOL ("minibuffer-scroll-conservatively",
+               minibuffer_scroll_conservatively,
+               doc: /* Non-nil means scroll conservatively in minibuffer 
windows.
+When the value is nil, scrolling in minibuffer windows obeys the
+settings of `scroll-conservatively'.  */);
+  minibuffer_scroll_conservatively = true; /* bug#44070 */
+
+  DEFVAR_BOOL ("inhibit-message", inhibit_message,
               doc:  /* Non-nil means calls to `message' are not displayed.
 They are still logged to the *Messages* buffer.
 
@@ -34546,7 +34557,7 @@ syms_of_xdisp (void)
 disable messages everywhere, including in I-search and other
 places where they are necessary.  This variable is intended to
 be let-bound around code that needs to disable messages temporarily. */);
-  inhibit_message = 0;
+  inhibit_message = false;
 
   message_dolog_marker1 = Fmake_marker ();
   staticpro (&message_dolog_marker1);
diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index 95c39dacc3..fad90fad53 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -21,34 +21,55 @@
 
 (require 'ert)
 
+(defmacro xdisp-tests--in-minibuffer (&rest body)
+  (declare (debug t) (indent 0))
+  `(catch 'result
+     (minibuffer-with-setup-hook
+         (lambda ()
+           (let ((redisplay-skip-initial-frame nil)
+                 (executing-kbd-macro nil)) ;Don't skip redisplay
+             (throw 'result (progn . ,body))))
+       (let ((executing-kbd-macro t)) ;Force real minibuffer in `read-string'.
+         (read-string "toto: ")))))
+
 (ert-deftest xdisp-tests--minibuffer-resizing () ;; bug#43519
-  ;; FIXME: This test returns success when run in batch but
-  ;; it's only a lucky accident: it also returned success
-  ;; when bug#43519 was not fixed.
   (should
    (equal
     t
-    (catch 'result
-      (minibuffer-with-setup-hook
-          (lambda ()
-            (insert "hello")
-            (let ((ol (make-overlay (point) (point)))
-                  (redisplay-skip-initial-frame nil)
-                  (max-mini-window-height 1)
-                  (text 
"askdjfhaklsjdfhlkasjdfhklasdhflkasdhflkajsdhflkashdfkljahsdlfkjahsdlfkjhasldkfhalskdjfhalskdfhlaksdhfklasdhflkasdhflkasdhflkajsdhklajsdgh"))
-              ;; (save-excursion (insert text))
-              ;; (sit-for 2)
-              ;; (delete-region (point) (point-max))
-              (put-text-property 0 1 'cursor t text)
-              (overlay-put ol 'after-string text)
-              (let ((executing-kbd-macro nil)) ;Don't skip redisplay
-                (redisplay 'force))
-              (throw 'result
-                     ;; Make sure we do the see "hello" text.
-                     (prog1 (equal (window-start) (point-min))
-                       ;; (list (window-start) (window-end) (window-width))
-                       (delete-overlay ol)))))
-        (let ((executing-kbd-macro t)) ;Force real minibuffer in `read-string'.
-          (read-string "toto: ")))))))
+    (xdisp-tests--in-minibuffer
+      (insert "hello")
+      (let ((ol (make-overlay (point) (point)))
+            (max-mini-window-height 1)
+            (text 
"askdjfhaklsjdfhlkasjdfhklasdhflkasdhflkajsdhflkashdfkljahsdlfkjahsdlfkjhasldkfhalskdjfhalskdfhlaksdhfklasdhflkasdhflkasdhflkajsdhklajsdgh"))
+        ;; (save-excursion (insert text))
+        ;; (sit-for 2)
+        ;; (delete-region (point) (point-max))
+        (put-text-property 0 1 'cursor t text)
+        (overlay-put ol 'after-string text)
+        (redisplay 'force)
+        ;; Make sure we do the see "hello" text.
+        (prog1 (equal (window-start) (point-min))
+          ;; (list (window-start) (window-end) (window-width))
+          (delete-overlay ol)))))))
+
+(ert-deftest xdisp-tests--minibuffer-scroll () ;; bug#44070
+  (let ((posns
+         (xdisp-tests--in-minibuffer
+           (let ((max-mini-window-height 4))
+             (dotimes (_ 80) (insert "\nhello"))
+             (beginning-of-buffer)
+             (redisplay 'force)
+             (end-of-buffer)
+             ;; A simple edit like removing the last `o' shouldn't cause
+             ;; the rest of the minibuffer's text to move.
+             (list
+              (progn (redisplay 'force) (window-start))
+              (progn (delete-char -1)
+                     (redisplay 'force) (window-start))
+              (progn (goto-char (point-min)) (redisplay 'force)
+                     (goto-char (point-max)) (redisplay 'force)
+                     (window-start)))))))
+    (should (equal (nth 0 posns) (nth 1 posns)))
+    (should (equal (nth 1 posns) (nth 2 posns)))))
 
 ;;; xdisp-tests.el ends here