bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[martin rudalics]

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

The unbinding order in x_consider_frame_title is botched.  Hence,
while do_switch_frame will not resize the minibuffer window when the
frame of the title bar gets selected, it will still resize when the
old frame gets reselected in the unbind form.

The behavior can be seen with emacs -Q evaluating the following two
forms in row:

(make-frame '((minibuffer . nil)))
(y-or-n-p "\n")

The attached trivial patch cures it.  Obviously, there's no guarantee
that some other client relies on the wrong order.

martin

[-- Attachment #2: x_consider_frame_title.diff~ --]
[-- Type: text/plain, Size: 1487 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 274ab8d..0d084f8 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12002,12 +12002,6 @@ static void ATTRIBUTE_FORMAT_PRINTF (1, 0)
       /* Set global variable indicating that multiple frames exist.  */
       multiple_frames = CONSP (tail);
 
-      /* Switch to the buffer of selected window of the frame.  Set up
-	 mode_line_target so that display_mode_element will output into
-	 mode_line_noprop_buf; then display the title.  */
-      record_unwind_protect (unwind_format_mode_line,
-			     format_mode_line_unwind_data
-			       (f, current_buffer, selected_window, false));
       /* select-frame calls resize_mini_window, which could resize the
 	 mini-window and by that undo the effect of this redisplay
 	 cycle wrt minibuffer and echo-area display.  Binding
@@ -12015,6 +12009,12 @@ static void ATTRIBUTE_FORMAT_PRINTF (1, 0)
 	 no-op, thus avoiding the adverse side effects.  */
       specbind (Qinhibit_redisplay, Qt);
 
+      /* Switch to the buffer of selected window of the frame.  Set up
+	 mode_line_target so that display_mode_element will output into
+	 mode_line_noprop_buf; then display the title.  */
+      record_unwind_protect (unwind_format_mode_line,
+			     format_mode_line_unwind_data
+			       (f, current_buffer, selected_window, false));
       Fselect_window (f->selected_window, Qt);
       set_buffer_internal_1
 	(XBUFFER (XWINDOW (f->selected_window)->contents));

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[martin rudalics]

 > The unbinding order in x_consider_frame_title is botched.  Hence,
 > while do_switch_frame will not resize the minibuffer window when the
 > frame of the title bar gets selected, it will still resize when the
 > old frame gets reselected in the unbind form.
 >
 > The behavior can be seen with emacs -Q evaluating the following two
 > forms in row:
 >
 > (make-frame '((minibuffer . nil)))
 > (y-or-n-p "\n")

Maybe it's not entirely clear from this description.  The

(y-or-n-p "\n")

form must be evaluated in the new, minibuffer-less frame.  Then the
minibuffer window in the first, minibuffer-equipped frame appears
empty.

 > The attached trivial patch cures it.  Obviously, there's no guarantee
 > that some other client relies on the wrong order.

Any suggestions on how to proceed?  This is a regression wrt Emacs 25.

Thanks, martin

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[Eli Zaretskii]

> Date: Tue, 19 Feb 2019 10:07:36 +0100
> From: martin rudalics <rudalics@gmx.at>
> 
>  > The attached trivial patch cures it.  Obviously, there's no guarantee
>  > that some other client relies on the wrong order.
> 
> Any suggestions on how to proceed?  This is a regression wrt Emacs 25.

How about installing this on master?  Just please mention in the
comments the convoluted use case that caused the change in order, so
that we don't forget.

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[martin rudalics]

 > How about installing this on master?  Just please mention in the
 > comments the convoluted use case that caused the change in order, so
 > that we don't forget.

The use case is by no means convoluted.  Try 'y-or-n-p' with a single
line prompt that exceeds the minibuffer window width.  No feedback is
visible which is IMO a quite dangerous effect.  Missing a question is
bad.

Anyway.  I doubt that installing my change without accompanying
measures will work out of the box.  But first of all I'd like to know
whether your change was intentionally asymmetric (if so, why?),
experimentally asymmetric (which experiments did you conduct?), or
accidentally so (it's rather atypical of you to install asymmetric
behavior and not document it).

And obviously, all this evil has two roots: The fact that redisplay
may call 'select-frame' and the one that do_switch_frame always
shrinks the minibuffer window of the frame it leaves.  Together these
make debugging resize_mini_window calls a nightmare.

martin

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[Eli Zaretskii]

> Date: Sat, 23 Feb 2019 15:00:31 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 34317@debbugs.gnu.org
> 
>  > How about installing this on master?  Just please mention in the
>  > comments the convoluted use case that caused the change in order, so
>  > that we don't forget.
> 
> The use case is by no means convoluted.

That single word was hardly the main part of what I wrote...

> first of all I'd like to know whether your change was intentionally
> asymmetric (if so, why?), experimentally asymmetric (which
> experiments did you conduct?), or accidentally so (it's rather
> atypical of you to install asymmetric behavior and not document it).

I didn't realize we were discussing my change.  Which change was that?

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[martin rudalics]

 >> first of all I'd like to know whether your change was intentionally
 >> asymmetric (if so, why?), experimentally asymmetric (which
 >> experiments did you conduct?), or accidentally so (it's rather
 >> atypical of you to install asymmetric behavior and not document it).
 >
 > I didn't realize we were discussing my change.  Which change was that?

commit 821ea144bd446268fbe4a4a4775a06da52dea8cb

Display mini-window resized even when there are several frames

* src/xdisp.c (x_consider_frame_title): Bind inhibit-redisplay to
t to avoid resizing back the mini-window as result of considering
the title of other frames.  (Bug#24285)
(redisplay_window): No need to bind inhibit-redisplay here.


This means that in x_consider_frame_title we now do

       ptrdiff_t count = SPECPDL_INDEX ();

       ...

       record_unwind_protect (unwind_format_mode_line,
                              format_mode_line_unwind_data
                                (f, current_buffer, selected_window, false));
       ...

       specbind (Qinhibit_redisplay, Qt);

       Fselect_window (f->selected_window, Qt);

      ...

       unbind_to (count, Qnil);

where unwind_format_mode_line does

       Fselect_window (old_window, Qt);

The asymmetry I mentioned is that while binding Qinhibit_redisplay
covers the Fselect_window call in x_consider_frame_title, it does not
cover the Fselect_window in unwind_format_mode_line since that is
performed _after_ the special binding of Qinhibit_redisplay has been
abolished.  Which means that we call resize_mini_window for the first
call when the corresponding windows are on different frames but not
for the second.

martin

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[Eli Zaretskii]

> Date: Sun, 24 Feb 2019 09:43:09 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 34317@debbugs.gnu.org
> 
> * src/xdisp.c (x_consider_frame_title): Bind inhibit-redisplay to
> t to avoid resizing back the mini-window as result of considering
> the title of other frames.  (Bug#24285)
> (redisplay_window): No need to bind inhibit-redisplay here.
> 
> 
> This means that in x_consider_frame_title we now do
> 
>        ptrdiff_t count = SPECPDL_INDEX ();
> 
>        ...
> 
>        record_unwind_protect (unwind_format_mode_line,
>                               format_mode_line_unwind_data
>                                 (f, current_buffer, selected_window, false));
>        ...
> 
>        specbind (Qinhibit_redisplay, Qt);
> 
>        Fselect_window (f->selected_window, Qt);
> 
>       ...
> 
>        unbind_to (count, Qnil);
> 
> where unwind_format_mode_line does
> 
>        Fselect_window (old_window, Qt);
> 
> The asymmetry I mentioned is that while binding Qinhibit_redisplay
> covers the Fselect_window call in x_consider_frame_title, it does not
> cover the Fselect_window in unwind_format_mode_line since that is
> performed _after_ the special binding of Qinhibit_redisplay has been
> abolished.  Which means that we call resize_mini_window for the first
> call when the corresponding windows are on different frames but not
> for the second.

I cannot imagine that was on purpose.

But your proposed change fixes that, so I'm unsure why you said it
won't work OOTB.  What did I miss?

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[martin rudalics]

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

 > I cannot imagine that was on purpose.

OK.

 > But your proposed change fixes that, so I'm unsure why you said it
 > won't work OOTB.  What did I miss?

Because I encountered problems in another context but do not remember
the details.  All I can tell is that here I additionally had to install
the attached.

martin

[-- Attachment #2: keyboard.c.diff --]
[-- Type: text/plain, Size: 486 bytes --]

--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1477,7 +1477,9 @@ static int read_key_sequence (Lisp_Object *, Lisp_Object,
 
       /* If displaying a message, resize the echo area window to fit
 	 that message's size exactly.  */
-      if (!NILP (echo_area_buffer[0]))
+      if (!NILP (echo_area_buffer[0])
+	  && (EQ (echo_area_window,
+		  FRAME_MINIBUF_WINDOW (XFRAME (selected_frame)))))
 	resize_echo_area_exactly ();
 
       /* If there are warnings waiting, process them.  */

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[Eli Zaretskii]

> Date: Sun, 24 Feb 2019 19:30:42 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: 34317@debbugs.gnu.org
> 
>  > But your proposed change fixes that, so I'm unsure why you said it
>  > won't work OOTB.  What did I miss?
> 
> Because I encountered problems in another context but do not remember
> the details.  All I can tell is that here I additionally had to install
> the attached.

This addition makes sense regardless, so why not install both and see
what breaks?

Thanks.

bug#34317: 26.1.90; Wrong unbinding order in x_consider_frame_title

[martin rudalics]

fixed 34317 27.1
quit

 > This addition makes sense regardless, so why not install both and see
 > what breaks?

Installed.  Marking bug as done.

Thanks, martin