bug#43519: 28.0.50; Overlay at end of minibuf hides minibuf's real content

[Stefan Monnier <monnier@iro.umontreal.ca>]

>> Indeed, then we'll just rely on the "generic" behavior, which is
>> admittedly not focused on single-line (or few lines) windows, but at
>> least in the current case it works better (and simplifies the code
>> slightly).
> I believe the "works better" part is only guaranteed if
> max-mini-window-height is 1.

You're more optimistic than I am: I only said it works better "in the
current case".

> Something like that.  More generally, assume the text to be displayed
> in the mini-window is
>
>    111111111111111111
>    2222222222222222222222
>    33333333333333333333
>    444444444444444444444
>
> With point in the 4th line and max-mini-window-height = 4, there's no
> guarantee that we will see all the 4 lines, we could see just 3 and an
> empty 4th one.

You mean because of something like recentering?  IOW the kind of
situation that (in normal buffer) can (sometimes) be avoided by the
likes of `comint-scroll-show-maximum-output` and
`scroll-conservatively`?

Aah.... OK thanks, so now I did manage to reproduce a test case where
the current behavior is better than with my patch:

Set max-mini-window-height to 4, then do:

    M-: 1 ^Q ^J 2 ^Q ^J 3 ^Q ^J 4 ^Q ^J

With my patch this last C-j causes a recentering, so we can't see line
2 anymore.  Note that this recentering will still happen without my
patch if we add

    M-< M->

since our ad-hoc scrolling only occurs when the buffer's content
is changed.

And of course, we could set `scroll-conservatively` in the (mini)buffers
in which case my patch would thus recover the same behavior as the
current ad-hoc scroll (or even better since now even the `M-< M->`
avoids the recentering).

I'm not sure what would be the best way to "set `scroll-conservatively`
in the (mini)buffers", but the patch below does it "well enough" for my
tests to work.  It's probably not "good enough" for master, OTOH.

>> AFAICT our ad-hoc scrolling code gives the same result as the generic
>> scrolling code in that case.
> Not sure what ad-hoc scrolling code you allude to here.  If you mean
> what resize_mini_window does,

Yes, that's what I called "ad-hoc scroll".

> then it doesn't really scroll, it just instructs redisplay to use
> a particular buffer position as the window-start;

For me, "scrolling" is the act of changing `window-start`, so it does
seem like the right word to describe what this code does.  I called it
"ad-hoc" because it's special cased for that one particular use, as
opposed to the "generic" scroll code which is used in general.

>> I've been trying out the patch below and haven't bumped into any
>> surprising behavior yet, but admittedly, I probably lack creativity.
> IOW, you leave it entirely to the generic window-display code to
> select window-start based just on the value of point?

Yes.  It seems to work very well.
Even the corner case regression above doesn't seem very serious and can
be addressed using the scroll_conservatively code.

> And only when the mini-window cannot be enlarged enough?  I wouldn't.

We could dispense with this special case, but this is the case where
I think it's clearly at least as good a choice as whatever the generic
scrolling will do.


        Stefan


diff --git a/src/xdisp.c b/src/xdisp.c
index dfcb1d73e4..7738ac5380 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -11885,14 +11885,18 @@ resize_mini_window (struct window *w, bool exact_p)
       if (height > max_height)
 	{
 	  height = (max_height / unit) * unit;
-	  init_iterator (&it, w, ZV, ZV_BYTE, NULL, DEFAULT_FACE_ID);
-	  move_it_vertically_backward (&it, height - unit);
-	  start = it.current.pos;
+	  /* bug#43519: Let the redisplay choose the window start!
+           *
+           * init_iterator (&it, w, ZV, ZV_BYTE, NULL, DEFAULT_FACE_ID);
+	   * move_it_vertically_backward (&it, height - unit);
+	   * start = it.current.pos; */
 	}
       else
-	SET_TEXT_POS (start, BEGV, BEGV_BYTE);
+	{
+	  SET_TEXT_POS (start, BEGV, BEGV_BYTE);
 
-      SET_MARKER_FROM_TEXT_POS (w->start, start);
+          SET_MARKER_FROM_TEXT_POS (w->start, start);
+        }
 
       if (EQ (Vresize_mini_windows, Qgrow_only))
 	{
@@ -18913,6 +18917,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))
@@ -18923,7 +18928,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)