What is restore_window_configuration in read_minibuf (minibuf.c) for?

What is restore_window_configuration in read_minibuf (minibuf.c) for?

[Alan Mackenzie]

Hello, Emacs.

Emacs 28, master branch.

In src/minibuf.c, function read_minibuf, there's a
restore_window_configuration set up to be called on minibuffer
termination.

When this call of restore_window_configuration happens, the restored
frame sometimes includes a minibuffer which has since moved to another
frame (reproduction recipe below).  Having the minibuffer displayed on
two frames causes trouble.

What is the purpose of this restore_window_configuration in
read_minibuf?  Does anybody know?  Removing it appears to fix the above
bug, but what problems might this cause?  I would like to remove it.

Recipe to reproduce the bug:
(i) emacs -Q --eval "(setq enable-recursive-minibuffers t
  minibuffer-follows-selected-frame 'hybrid)"
(ii) C-x 5 2, giving two frames F1, F2.
(iii) With four existing files file1, file2, file3, file4:
(iv) In F1, C-x C-f file1.
  In F2 C-x C-f file2.
  In F1 C-x C-f file3.
  In F2 C-x C-f file4.
(v) Currently the minibuffers are stacked up in F2.
(vi) In F2 type RET, visiting file4.  The MB for file3 is visible in
  F2's mini window.
(vii) In F2's mini window, type RET.  This visits file3 in F1, leaving
  the stack of minibuffers in F2.  However ....
(viii) The minibuffer for file2 is also visible in F1.  This is a bug
  (see above).
(ix) Typing RET in F1's mini window visits file2, but "loses" the
  minibuffer for file1, which now needs to be aborted with C-].

Just as a matter of interest, the above sequence on Emacs-27 doesn't
even get that far - it "loses" the minibuffers for file2 and file1 at an
early stage.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[martin rudalics]

 > In src/minibuf.c, function read_minibuf, there's a
 > restore_window_configuration set up to be called on minibuffer
 > termination.
[...]
 > What is the purpose of this restore_window_configuration in
 > read_minibuf?  Does anybody know?  Removing it appears to fix the above
 > bug, but what problems might this cause?  I would like to remove it.

See

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=45072

the option I proposed in that thread and Juri's remarks at its end.

martin

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[Alan Mackenzie]

Hello, Martin.

On Mon, Apr 19, 2021 at 14:08:38 +0200, martin rudalics wrote:
>  > In src/minibuf.c, function read_minibuf, there's a
>  > restore_window_configuration set up to be called on minibuffer
>  > termination.
> [...]
>  > What is the purpose of this restore_window_configuration in
>  > read_minibuf?  Does anybody know?  Removing it appears to fix the above
>  > bug, but what problems might this cause?  I would like to remove it.

> See

> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=45072

Whew!  That's a long thread.

The impression I get is that the restore_window_configuration is a user
facility, and we can't just remove it, much though I'd like to.

> the option I proposed in that thread and Juri's remarks at its end.

Juri's remarks from the end of that thread.  Thanks for drawing my
attention to them:

>> It seems that problem is that read_minibuf messes up the windows so
>> much, that at the end currently the only way to fix this mess is by
>> restoring the previous window configuration.

So we can't just remove it.

>> This means that there is a need to fix read_minibuf to restore all
>> previous window states without using restore_window_configuration.
>> Only then it will be possible to add a user option to disable using
>> restore_window_configuration.

I don't think it is read_minibuf itself that is causing the problems;
rather it is the commands used in read_minibuf's recursive edit.  That
will mean fixing a large part of lisp/minibuffer.el and possibly the
latter part of src/minibuf.c.  That will be quite a lot of work.

Anyhow, back to my bug: It is not restore_window_configuration as a
whole which is bugging the mini-windows, it is specifically restoring
the mini-window itself to an out of date state.

So what seems needed is an extra &optional parameter to
set-window-configuration, DONT-SET-MINIWINDOW which would inhibit the
restoration of the mini-window.  The call from unwinding a minibuffer
would pass a non-nil value for this argument.  Messy, but I haven't got
any better ideas.

What do you think?

> martin

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[Daniel Mendler]

On 4/19/21 4:56 PM, Alan Mackenzie wrote:
> The impression I get is that the restore_window_configuration is a user
> facility, and we can't just remove it, much though I'd like to.

FWIW I am using this feature in one of my packages 
(https://github.com/minad/consult/). I expect the window configuration 
to be restored when the minibuffer session is left. One of my commands 
previews buffers when selecting them during completing-read. However 
packages which do something like this can also be adjusted accordingly 
to save and restore the configuration manually if the decision is made 
to change this behavior.

Daniel Mendler

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[martin rudalics]

 > FWIW I am using this feature in one of my packages
 > (https://github.com/minad/consult/). I expect the window configuration
 > to be restored when the minibuffer session is left. One of my commands
 > previews buffers when selecting them during completing-read. However
 > packages which do something like this can also be adjusted accordingly
 > to save and restore the configuration manually if the decision is made
 > to change this behavior.

If a user wanted to do such previews on another, maybe even new frame,
restoring the window configuration won't be of any help.

martin

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[Stefan Monnier]

> The impression I get is that the restore_window_configuration is a user
> facility, and we can't just remove it, much though I'd like to.

Indeed.  I think we should remove it, but for that we need to replace it
with something that does the part we want to keep (e.g. making sure
that the buffer from which the minibuffer was "called" is
selected&visible again).

> So what seems needed is an extra &optional parameter to
> set-window-configuration, DONT-SET-MINIWINDOW which would inhibit the
> restoration of the mini-window.  The call from unwinding a minibuffer
> would pass a non-nil value for this argument.  Messy, but I haven't got
> any better ideas.
> What do you think?

Maybe the restoration of some previous minibuffer is OK (e.g. when you
use `M-x` from within `M-:` you do want the outer minibuffer to be
restored when you leave the inner one), and you just need to adjust it
after the fact in the rare cases where it results in an undesirable
state (e.g. if that minibuffer was already displayed elsewhere).



        Stefan

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[martin rudalics]

 > So what seems needed is an extra &optional parameter to
 > set-window-configuration, DONT-SET-MINIWINDOW which would inhibit the
 > restoration of the mini-window.  The call from unwinding a minibuffer
 > would pass a non-nil value for this argument.  Messy, but I haven't got
 > any better ideas.
 >
 > What do you think?

You could probably also set the minibuffer window(s) separately to
whatever you want after the configuration has been restored.  It's messy
in either case.

martin

Re: What is restore_window_configuration in read_minibuf (minibuf.c) for?

[Alan Mackenzie]

Hello, Martin.

On Mon, Apr 19, 2021 at 18:02:48 +0200, martin rudalics wrote:
>  > So what seems needed is an extra &optional parameter to
>  > set-window-configuration, DONT-SET-MINIWINDOW which would inhibit the
>  > restoration of the mini-window.  The call from unwinding a minibuffer
>  > would pass a non-nil value for this argument.  Messy, but I haven't got
>  > any better ideas.

>  > What do you think?

> You could probably also set the minibuffer window(s) separately to
> whatever you want after the configuration has been restored.  It's messy
> in either case.

I've gone ahead with my original suggestion.

Also, I've added some proper error handling for a "can't happen" case in
read_minibuf_unwind, at that place where the loop around frames caused a
crash when there were tooltip frames involved.  The error occurs if that
loop doesn't find the expired minibuffer in any frame.

If there are no objections, I intend to commit the following soon, which
fixes the bug:



diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index c32d711f12..82d2ce4757 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -5877,7 +5877,7 @@ Window Configurations
 @xref{Window Parameters}.
 @end defun
 
-@defun set-window-configuration configuration &optional dont-set-frame
+@defun set-window-configuration configuration &optional dont-set-frame dont-set-miniwindow
 This function restores the configuration of windows and buffers as
 specified by @var{configuration}, for the frame that
 @var{configuration} was created for, regardless of whether that frame
@@ -5885,8 +5885,12 @@ Window Configurations
 that was previously returned by @code{current-window-configuration}
 for that frame.  Normally the function also selects the frame which is
 recorded in the configuration, but if @var{dont-set-frame} is
-non-@code{nil}, it leaves selected the frame which was current at the
-start of the function.
+non-@code{nil}, it leaves selected the frame which was already
+selected at the start of the function.
+
+Normally the function restores the saved minibuffer (if any), but if
+@var{dont-set-miniwindow} is non-@code{nil}, the minibuffer current
+at the start of the function (if any) remains in the mini-window.
 
 If the frame from which @var{configuration} was saved is dead, all
 this function does is to restore the value of the variable
diff --git a/etc/NEWS b/etc/NEWS
index 3e5767c953..6cf1a98e5d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2660,9 +2660,11 @@ one to another in the init file.  The same user option also controls
 whether the function 'read-answer' accepts short answers.
 
 +++
-** 'set-window-configuration' now takes an optional 'dont-set-frame'
-parameter which, when non-nil, instructs the function not to select
-the frame recorded in the configuration.
+** 'set-window-configuration' now takes two optional parameters,
+'dont-set-frame' and 'dont-set-miniwindow'.  The first of these, when
+non-nil, instructs the function not to select the frame recorded in
+the configuration.  The second prevents the current minibuffer being
+replaced by the one stored in the configuration.
 
 +++
 ** 'define-globalized-minor-mode' now takes a ':predicate' parameter.
diff --git a/src/keyboard.c b/src/keyboard.c
index 266ebaa5fd..5db45ce8e5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2121,7 +2121,7 @@ read_char_help_form_unwind (void)
   Lisp_Object window_config = XCAR (help_form_saved_window_configs);
   help_form_saved_window_configs = XCDR (help_form_saved_window_configs);
   if (!NILP (window_config))
-    Fset_window_configuration (window_config, Qnil);
+    Fset_window_configuration (window_config, Qnil, Qnil);
 }
 
 #define STOP_POLLING					\
diff --git a/src/minibuf.c b/src/minibuf.c
index b823224a5f..db62ae0eea 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -660,17 +660,14 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 
   record_unwind_protect_void (minibuffer_unwind);
   record_unwind_protect (restore_window_configuration,
-                         Fcons (Qt, Fcurrent_window_configuration (Qnil)));
+			 list3 (Fcurrent_window_configuration (Qnil), Qt, Qt));
 
   /* If the minibuffer window is on a different frame, save that
      frame's configuration too.  */
   if (!EQ (mini_frame, selected_frame))
     record_unwind_protect (restore_window_configuration,
-			   Fcons (/* Arrange for the frame later to be
-                                     switched back to the calling
-                                     frame. */
-                                  Qnil,
-                                  Fcurrent_window_configuration (mini_frame)));
+			   list3 (Fcurrent_window_configuration (mini_frame),
+				  Qnil, Qt));
 
   /* If the minibuffer is on an iconified or invisible frame,
      make it visible now.  */
@@ -1071,13 +1068,13 @@ read_minibuf_unwind (void)
 	    goto found;
 	}
     }
-  return; /* expired minibuffer not found.  Maybe we should output an
-	     error, here. */
+  exp_MB_frame = Qnil;		/* "Can't happen." */
 
  found:
-  if (!EQ (exp_MB_frame, saved_selected_frame))
+  if (!EQ (exp_MB_frame, saved_selected_frame)
+      && !NILP (exp_MB_frame))
     do_switch_frame (exp_MB_frame, 0, 0, Qt); /* This also sets
-					     minibuff_window */
+					     minibuf_window */
 
   /* To keep things predictable, in case it matters, let's be in the
      minibuffer when we reset the relevant variables.  Don't depend on
@@ -1187,7 +1184,8 @@ read_minibuf_unwind (void)
     }
 
   /* Restore the selected frame. */
-  if (!EQ (exp_MB_frame, saved_selected_frame))
+  if (!EQ (exp_MB_frame, saved_selected_frame)
+      && !NILP (exp_MB_frame))
     do_switch_frame (saved_selected_frame, 0, 0, Qt);
 }
 
@@ -1202,6 +1200,7 @@ minibuffer_unwind (void)
   Lisp_Object window;
   Lisp_Object entry;
 
+  if (NILP (exp_MB_frame)) return; /* "Can't happen." */
   f = XFRAME (exp_MB_frame);
   window = f->minibuffer_window;
   w = XWINDOW (window);
diff --git a/src/window.c b/src/window.c
index a22fab2444..5134c3df63 100644
--- a/src/window.c
+++ b/src/window.c
@@ -6881,19 +6881,22 @@ DEFUN ("window-configuration-frame", Fwindow_configuration_frame, Swindow_config
 }
 
 DEFUN ("set-window-configuration", Fset_window_configuration,
-       Sset_window_configuration, 1, 2, 0,
+       Sset_window_configuration, 1, 3, 0,
        doc: /* Set the configuration of windows and buffers as specified by CONFIGURATION.
 CONFIGURATION must be a value previously returned
 by `current-window-configuration' (which see).
 
 Normally, this function selects the frame of the CONFIGURATION, but if
 DONT-SET-FRAME is non-nil, it leaves selected the frame which was
-current at the start of the function.
+current at the start of the function.  If DONT-SET-MINIWINDOW is non-nil,
+the mini-window of the frame doesn't get set to the corresponding element
+of CONFIGURATION.
 
 If CONFIGURATION was made from a frame that is now deleted,
 only frame-independent values can be restored.  In this case,
 the return value is nil.  Otherwise the value is t.  */)
-  (Lisp_Object configuration, Lisp_Object dont_set_frame)
+  (Lisp_Object configuration, Lisp_Object dont_set_frame,
+   Lisp_Object dont_set_miniwindow)
 {
   register struct save_window_data *data;
   struct Lisp_Vector *saved_windows;
@@ -7104,8 +7107,10 @@ the return value is nil.  Otherwise the value is t.  */)
 		}
 	    }
 
-	  if (BUFFERP (p->buffer) && BUFFER_LIVE_P (XBUFFER (p->buffer)))
-	    /* If saved buffer is alive, install it.  */
+	  if ((NILP (dont_set_miniwindow) || !MINI_WINDOW_P (w))
+	      && BUFFERP (p->buffer) && BUFFER_LIVE_P (XBUFFER (p->buffer)))
+	    /* If saved buffer is alive, install it, unless it's a
+	       minibuffer we explicitly prohibit.  */
 	    {
 	      wset_buffer (w, p->buffer);
 	      w->start_at_line_beg = !NILP (p->start_at_line_beg);
@@ -7258,9 +7263,11 @@ void
 restore_window_configuration (Lisp_Object configuration)
 {
   if (CONSP (configuration))
-    Fset_window_configuration (XCDR (configuration), XCAR (configuration));
+    Fset_window_configuration (XCAR (configuration),
+			       XCAR (XCDR (configuration)),
+			       XCAR (XCDR (XCDR (configuration))));
   else
-    Fset_window_configuration (configuration, Qnil);
+    Fset_window_configuration (configuration, Qnil, Qnil);
 }
 
 


> martin

-- 
Alan Mackenzie (Nuremberg, Germany).