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

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

[Stefan Monnier]

Package: Emacs
Version: 28.0.50


Step 1:

    % emacs -Q
    M-: a C-u 20 C-q C-j abc

You should now have "abc" on the last line of the miniwindow with the
prompt scrolled out of view.  So far so good.

Step 2:

    M-< M->

We scroll to the beginning and back to the end.  Now "abc" is not quite
on the last line of the miniwindow, more specifically there should be 2 empty
lines left after "abc".  The behavior is slightly different if instead of
`end-of-buffer` we use (goto-char (point-max)), but the problem at step
3 remains:

Step 3:

    DEL

This is the surprising part: this simple edit causes the minibuffer to
be "rescrolled" so that the resulting "ab" is now again placed on the
very last line of the miniwindow.

The same thing happens with any other simple edit because any buffer
modification triggers a call to `resize_mini_window`, which will always
try to arrange to see a maximum of text.

Not sure if it's better to fix this by changing step 2 or by changing
step 3, but the patch below fixes it by changing step 2.

WDYT?


        Stefan


diff --git a/lisp/simple.el b/lisp/simple.el
index d6fce922c4..41aba2ddc3 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1129,7 +1129,7 @@ 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))))
+	 (recenter (if (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 5a62cd6eb5..c1105df3fc 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
+       || MINI_WINDOW_P (w)
        || 0 < emacs_scroll_step
        || temp_scroll_step
        || NUMBERP (BVAR (current_buffer, scroll_up_aggressively))
@@ -18830,7 +18831,9 @@ 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,
+			      (MINI_WINDOW_P (w)
+			       ? SCROLL_LIMIT + 1
+			       : scroll_conservatively),
 			      emacs_scroll_step,
 			      temp_scroll_step, last_line_misfit);
       switch (ss)
diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index 95c39dacc3..d513f5beba 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#43519
+  (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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 18 Oct 2020 18:09:55 -0400
> 
> diff --git a/lisp/simple.el b/lisp/simple.el
> index d6fce922c4..41aba2ddc3 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -1129,7 +1129,7 @@ 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))))
> +	 (recenter (if (window-minibuffer-p) -1 -3)))))

This should have a comment that explains the reason for the
difference.  (Btw, does this DTRT when the text in the minibuffer has
a newline at the end?)

> --- 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
> +       || MINI_WINDOW_P (w)
>         || 0 < emacs_scroll_step
>         || temp_scroll_step
>         || NUMBERP (BVAR (current_buffer, scroll_up_aggressively))
> @@ -18830,7 +18831,9 @@ 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,
> +			      (MINI_WINDOW_P (w)
> +			       ? SCROLL_LIMIT + 1
> +			       : scroll_conservatively),
>  			      emacs_scroll_step,
>  			      temp_scroll_step, last_line_misfit);
>        switch (ss)

If we want the minibuffer behave as if scroll-conservatively was set,
why not simply set scroll-conservatively in each minibuffer?  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.  As a nice bonus,
we will then be sure the change doesn't affect echo-area messages,
only editing in the minibuffer.

WDYT?

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 44070@debbugs.gnu.org
> Date: Thu, 29 Oct 2020 13:54:01 -0400
> 
> > 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).

That's strange: don't we have a single place where we create the
minibuffer?

> How 'bout the patch below, then?

LGTM, modulo the NEWS and ELisp manual updates.

> +  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'.  */);

I'd say "behaves as if scroll-conservatively were set" instead of
"obeys the setting of scroll-conservatively", because the latter can
be interpreted as meaning one actually needs to set
scroll-conservatively.

> +  minibuffer_scroll_conservatively = true; /* bug#44070 */

It is IMO generally not useful to have such comments, since the Git
history will tell us that much (and then some), provided you don't
forget to mention the bug number in the log message.

Thanks.

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

[Stefan Monnier]

>> 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).
> That's strange: don't we have a single place where we create the
> minibuffer?

Yes, but the problem is that we reuse the minibuffers and that we need
to re-set the var after killing local vars, so the only sane place to
change this would be in `reset_buffer_local_variables`.

>> How 'bout the patch below, then?
> LGTM, modulo the NEWS and ELisp manual updates.

OK, thanks.  Done and pushed.

>> +  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'.  */);
>
> I'd say "behaves as if scroll-conservatively were set" instead of
> "obeys the setting of scroll-conservatively", because the latter can
> be interpreted as meaning one actually needs to set
> scroll-conservatively.

That's indeed what it means since it describes the behavior when the var
is nil (which is the old behavior).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 44070@debbugs.gnu.org
> Date: Sat, 31 Oct 2020 09:12:16 -0400
> 
> >> How 'bout the patch below, then?
> > LGTM, modulo the NEWS and ELisp manual updates.
> 
> OK, thanks.  Done and pushed.

Actually, I see now that your changes only test that a window is a
mini-window, but not that it displays a minibuffer.  Did I miss
something?  If not, I'd prefer to add the conditions for displaying a
minibuffer, okay?

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

[Stefan Monnier]

>> >> How 'bout the patch below, then?
>> > LGTM, modulo the NEWS and ELisp manual updates.
>> OK, thanks.  Done and pushed.
> Actually, I see now that your changes only test that a window is a
> mini-window, but not that it displays a minibuffer.  Did I miss
> something?  If not, I'd prefer to add the conditions for displaying a
> minibuffer, okay?

How would I do that?


        Stefan

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

[Stefan Monnier]

>>> >> How 'bout the patch below, then?
>>> > LGTM, modulo the NEWS and ELisp manual updates.
>>> OK, thanks.  Done and pushed.
>> Actually, I see now that your changes only test that a window is a
>> mini-window, but not that it displays a minibuffer.  Did I miss
>> something?  If not, I'd prefer to add the conditions for displaying a
>> minibuffer, okay?
> How would I do that?

Also, I'm not sure that's what we want to do: as I argued in my previous
message, this change is mostly meant to better fit the behavior of
resize_mini_window, and that behavior applies to all mini-windows,
regardless if they're used for a minibuffer or something else.

I suggest we keep the code as is for now, and we'll consider what to do
*if* someone encounters a regression (which, for all we know, could
also affect the minibuffer case).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 44070@debbugs.gnu.org
> Date: Sun, 01 Nov 2020 09:12:11 -0500
> 
> Also, I'm not sure that's what we want to do: as I argued in my previous
> message, this change is mostly meant to better fit the behavior of
> resize_mini_window, and that behavior applies to all mini-windows,
> regardless if they're used for a minibuffer or something else.

My line of thinking was that, while in minibuffers we usually want to
see the end of the text, that is not necessarily true in echo-area
messages.

> I suggest we keep the code as is for now, and we'll consider what to do
> *if* someone encounters a regression (which, for all we know, could
> also affect the minibuffer case).

Then let's leave a FIXME comment about this in those two places where
you use MINI_WINDOW_P, okay?

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

[Stefan Monnier]

>> Also, I'm not sure that's what we want to do: as I argued in my previous
>> message, this change is mostly meant to better fit the behavior of
>> resize_mini_window, and that behavior applies to all mini-windows,
>> regardless if they're used for a minibuffer or something else.
> My line of thinking was that, while in minibuffers we usually want to
> see the end of the text, that is not necessarily true in echo-area
> messages.

Yet, that's what `resize_mini_window` does for all mini-windows
regardless of it's an echo-area or not.

Also, my patch shouldn't affect whether we "see the end of the text", so
maybe I just don't understand what you're referring to.

>> I suggest we keep the code as is for now, and we'll consider what to do
>> *if* someone encounters a regression (which, for all we know, could
>> also affect the minibuffer case).
> Then let's leave a FIXME comment about this in those two places where
> you use MINI_WINDOW_P, okay?

I thought the test of `scroll_minibuffer_conservative` played this role.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 44070@debbugs.gnu.org
> Date: Sun, 01 Nov 2020 13:59:35 -0500
> 
> >> Also, I'm not sure that's what we want to do: as I argued in my previous
> >> message, this change is mostly meant to better fit the behavior of
> >> resize_mini_window, and that behavior applies to all mini-windows,
> >> regardless if they're used for a minibuffer or something else.
> > My line of thinking was that, while in minibuffers we usually want to
> > see the end of the text, that is not necessarily true in echo-area
> > messages.
> 
> Yet, that's what `resize_mini_window` does for all mini-windows
> regardless of it's an echo-area or not.

Resizing is one thing; which part of partially displayed text to show
is another.  They aren't the same decisions.

> Also, my patch shouldn't affect whether we "see the end of the text", so
> maybe I just don't understand what you're referring to.

Where do you think point is in an echo-area buffer?

> >> I suggest we keep the code as is for now, and we'll consider what to do
> >> *if* someone encounters a regression (which, for all we know, could
> >> also affect the minibuffer case).
> > Then let's leave a FIXME comment about this in those two places where
> > you use MINI_WINDOW_P, okay?
> 
> I thought the test of `scroll_minibuffer_conservative` played this role.

That's exactly why we need a FIXME: the variable says "minibuffer",
but the code checks for mini-window.

Anyway, I can add the comment myself; no need to argue.

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

[Stefan Monnier]

>> >> Also, I'm not sure that's what we want to do: as I argued in my previous
>> >> message, this change is mostly meant to better fit the behavior of
>> >> resize_mini_window, and that behavior applies to all mini-windows,
>> >> regardless if they're used for a minibuffer or something else.
>> > My line of thinking was that, while in minibuffers we usually want to
>> > see the end of the text, that is not necessarily true in echo-area
>> > messages.
>> Yet, that's what `resize_mini_window` does for all mini-windows
>> regardless of it's an echo-area or not.
> Resizing is one thing; which part of partially displayed text to show
> is another.  They aren't the same decisions.

Right, but `resize_mini_window` does do both: it resizes the window and
and sets the window-start according to its rules of which part should
be displayed.  My patch tries to make the non-resizing scrolls
follow the same logic as that of `resize_mini_window` to
avoid inconsistencies.

>> Also, my patch shouldn't affect whether we "see the end of the text", so
>> maybe I just don't understand what you're referring to.
> Where do you think point is in an echo-area buffer?

I must say that I don't know, but I expect it should be either at BOB or
at EOB: if it's a BOB then my patch won't make any difference because
`window-start` will be set to BOB regardless of the scroll being
conservative or not; if it's at EOB my patch will maximize the amount of
text actually displayed, which is also what `resize_mini_window` does,
but I suspect that for each-areas this won't make any difference because
I've never seen an echo-area messages "scrolled" such that there's white
space left below its end (probably because `resize_mini_window` already
did the job, so there's no scrolling involved).

>> I thought the test of `scroll_minibuffer_conservative` played this role.
> That's exactly why we need a FIXME: the variable says "minibuffer",
> but the code checks for mini-window.

I chose the word "minibuffer" because it seems those "mini windows" are
usually referred to as "minibuffer windows" in places like
`window-minibuffer-p`.  The docstring similarly uses "minibuffer
windows" to refer to those *windows* rather than to their specific use
for minibuffers as opposed to echo area messages, although I strongly
suspect that the variable has indeed no concrete effect in echo-areas.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 44070@debbugs.gnu.org
> Date: Sun, 01 Nov 2020 08:29:09 -0500
> 
> >> >> How 'bout the patch below, then?
> >> > LGTM, modulo the NEWS and ELisp manual updates.
> >> OK, thanks.  Done and pushed.
> > Actually, I see now that your changes only test that a window is a
> > mini-window, but not that it displays a minibuffer.  Did I miss
> > something?  If not, I'd prefer to add the conditions for displaying a
> > minibuffer, okay?
> 
> How would I do that?

I thought about testing equality between the window being redisplayed
and minibuf_window.  We could also look at the buffer displayed by the
window, and see if it appears in Vminibuffer_list.  Does that make
sense?

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

[Stefan Monnier]

> I thought about testing equality between the window being redisplayed
> and minibuf_window.  We could also look at the buffer displayed by the
> window, and see if it appears in Vminibuffer_list.  Does that make
> sense?

Sounds pretty ugly.  What do you think about the other argument, that it
should apply wherever resize_mini_window applies?


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 44070@debbugs.gnu.org
> Date: Sun, 01 Nov 2020 10:38:03 -0500
> 
> > I thought about testing equality between the window being redisplayed
> > and minibuf_window.  We could also look at the buffer displayed by the
> > window, and see if it appears in Vminibuffer_list.  Does that make
> > sense?
> 
> Sounds pretty ugly.

??? Why?  In any case, we do this stuff all over the place.  A random
example:

      else if ((w != XWINDOW (minibuf_window)
		|| minibuf_level == 0)
	       /* When buffer is nonempty, redisplay window normally.  */
	       && BUF_Z (XBUFFER (w->contents)) == BUF_BEG (XBUFFER (w->contents))
	       /* Quail displays non-mini buffers in minibuffer window.
		  In that case, redisplay the window normally.  */
	       && !NILP (Fmemq (w->contents, Vminibuffer_list)))