Re: Stop frames stealing eachothers' minibuffers!

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan, Gregory, and Martin.

On Sat, Nov 28, 2020 at 12:02:34 -0500, Stefan Monnier wrote:
> >> A: type C-x C-f on frame F1, switch to frame F2
> >> B: type C-x C-f on frame F1, switch to frame F2, type M-:

> >> [1] There is also a severe regression in this case.  Type C-x C-f on frame 
> >> F1, switch to frame F2, type M-:.  "Find file" is still visible in the 
> >> miniwindow on frame F1; switch to frame F1.

> >> Experiment 1: Type the name of a file and RET.  You'll get the error 
> >> message "End of file during parsing", and MB2 on frame F2 will be left. 
> >> MB1 is now unuseable, and impossible to leave, it will stay on F1 whatever 
> >> you do.

> >> Experiment 2: Type C-g.  MB2 on frame F2 will be left, and "Find file" 
> >> will stay in MB1 on frame F1.  However you cannot use it anymore, the 
> >> keymap of MB1 is now minibuffer-inactive-mode-map.  And like in experiment 
> >> 1, you cannot leave it.

> > The abstract cause of this situation would appear to be using F1's
> > minibuffer while a more deeply nested minibuffer is still active.

> More specifically, it's the act of leaving MB1 when there's a deeper MB2
> active: the code for leaving a minibuffer (e.g. `exit-minibuffer` or
> `abort-recursive-edit`) doesn't actually pay attention to which
> minibuffer is currently being used: while it's run from MB1 it actually
> exits MB2.  I'm not completely sure why we end up with a broken state,
> but I guess it's because some of the code that "deactivates" the
> minibuffer upon exit in run in the minibuffer that the users thought they
> were about to exit rather than in the one that is actually exited.

> I expect that this is the core origin of the problem.

> One way to address it might be to make every minibuffer use a different exit
> tag (instead of the constant `exit` symbol), so that the `throw` will
> not be caught by some unrelated `catch`.  Additionally, we may want to
> tweak `exit-minibuffer` and `abort-recursive-edit` so that the user is
> warned/prompted before "silently" canceling that other (deeper) minibuffer.

> [ Another way to attack the problem would be to arrange it so that every
>   minibuffer runs in its own thread, so you can exit one without
>   affecting the other.  I think it might be an interesting direction,
>   but it's probably not trivial.  In any case, cmpletely out of scope
>   of the present problem.  ]

I haven't paid too much attention to the above.  But I do have a patch
which addresses the problem by removing the minibuffer key map from
outer level minibuffers, and giving them minibuffer-inactive-mode-map
instead.

Gregory, the patch is intended to restore at least part of the former
behaviour.  Set minibuffer-follows-selected-frame to a non-nil, not-t
value (such as 'hybrid), and minibuffers will move onto a selected frame
when the user invokes a minibuffer on that frame.  I think that's what
you wanted.

The patch might also fix your (Gregory's) problem with three frames and
three minibuffers.  I'm not sure.

Also, the patch is incomplete - the customsation entry in cus-start.el
needs amending, as do the Emacs manual and (probably) the NEWS entry.

Any testing you could do would be most appreciated.  Thanks!



diff --git a/src/minibuf.c b/src/minibuf.c
index fc3fd92a88..9db95c8381 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -63,9 +63,30 @@ static Lisp_Object minibuf_prompt;
 
 static ptrdiff_t minibuf_prompt_width;
 
+static Lisp_Object nth_minibuffer (EMACS_INT depth);
+
 \f
+/* Return TRUE when a frame switch causes a minibuffer on the old
+   frame to move onto the new one. */
 static bool
 minibuf_follows_frame (void)
+{
+  return EQ (Fdefault_toplevel_value (Qminibuffer_follows_selected_frame),
+             Qt);
+}
+
+/* Return TRUE when a minibuffer always remains on the frame where it
+   was first invoked. */
+static bool
+minibuf_stays_put (void)
+{
+  return NILP (Fdefault_toplevel_value (Qminibuffer_follows_selected_frame));
+}
+
+/* Return TRUE when opening a (recursive) minibuffer causes
+   minibuffers on other frames to move to the selected frame.  */
+static bool
+minibuf_moves_frame_when_opened (void)
 {
   return !NILP (Fdefault_toplevel_value (Qminibuffer_follows_selected_frame));
 }
@@ -90,7 +111,7 @@ choose_minibuf_frame (void)
       minibuf_window = sf->minibuffer_window;
       /* If we've still got another minibuffer open, use its mini-window
          instead.  */
-      if (minibuf_level && !minibuf_follows_frame ())
+      if (minibuf_level > 1 && minibuf_stays_put ())
         {
           Lisp_Object buffer = get_minibuffer (minibuf_level);
           Lisp_Object tail, frame;
@@ -105,26 +126,38 @@ choose_minibuf_frame (void)
         }
     }
 
-  if (minibuf_follows_frame ())
+  if (minibuf_moves_frame_when_opened ())
     /* Make sure no other frame has a minibuffer as its selected window,
        because the text would not be displayed in it, and that would be
        confusing.  Only allow the selected frame to do this,
        and that only if the minibuffer is active.  */
     {
       Lisp_Object tail, frame;
+      Lisp_Object buffer = nth_minibuffer (minibuf_level - 1);
+      struct frame *sf = XFRAME (selected_frame);
 
       FOR_EACH_FRAME (tail, frame)
-        if (MINI_WINDOW_P (XWINDOW (FRAME_SELECTED_WINDOW (XFRAME (frame))))
-            && !(EQ (frame, selected_frame)
-                 && minibuf_level > 0))
-          Fset_frame_selected_window (frame, Fframe_first_window (frame),
-                                      Qnil);
+        if (!EQ (frame, selected_frame)
+            && minibuf_level > 1)
+        {
+          if (MINI_WINDOW_P (XWINDOW (FRAME_SELECTED_WINDOW (XFRAME (frame)))))
+            Fset_frame_selected_window (frame, Fframe_first_window (frame),
+                                        Qnil);
+          if (EQ (XWINDOW (XFRAME (frame)->minibuffer_window)->contents,
+                  buffer))
+            {
+              set_window_buffer (sf->minibuffer_window, buffer, 0, 0);
+              set_window_buffer (XFRAME (frame)->minibuffer_window,
+                                 get_minibuffer (0), 0, 0);
+            }
+        }
     }
 }
 
-/* If `minibuffer_follows_selected_frame' and we have a minibuffer, move it
-   from its current frame to the selected frame.  This function is
-   intended to be called from `do_switch_frame' in frame.c.  */
+/* If `minibuffer_follows_selected_frame' is t and we have a
+   minibuffer, move it from its current frame to the selected frame.
+   This function is intended to be called from `do_switch_frame' in
+   frame.c.  */
 void move_minibuffer_onto_frame (void)
 {
   if (!minibuf_level)
@@ -411,6 +444,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   Lisp_Object val;
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object mini_frame, ambient_dir, minibuffer, input_method;
+  Lisp_Object calling_frame = selected_frame;
   Lisp_Object enable_multibyte;
   EMACS_INT pos = 0;
   /* String to add to the history.  */
@@ -532,7 +566,9 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   minibuf_save_list
     = Fcons (Voverriding_local_map,
 	     Fcons (minibuf_window,
-		    minibuf_save_list));
+                    Fcons (BVAR (XBUFFER (nth_minibuffer (minibuf_level - 1)),
+                                 keymap),
+                           minibuf_save_list)));
   minibuf_save_list
     = Fcons (minibuf_prompt,
 	     Fcons (make_fixnum (minibuf_prompt_width),
@@ -727,8 +763,33 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   /* Don't allow the user to undo past this point.  */
   bset_undo_list (current_buffer, Qnil);
 
+  /* Prevent the user manipulating outer levels of recursive minibuffers.  */
+  if (minibuf_level > 1)
+    {
+      Lisp_Object inactive_map;
+      if ((inactive_map =
+           find_symbol_value (intern ("minibuffer-inactive-mode-map")))
+          != Qunbound)
+        bset_keymap (XBUFFER (nth_minibuffer (minibuf_level - 1)),
+                     inactive_map);
+    }
+
   recursive_edit_1 ();
 
+  /* We've exited the recursive edit without an error, so switch the
+     frame back to the calling frame.  Also switch the current window
+     away from the expired minibuffer window.  */
+  if (!EQ (selected_frame, calling_frame)
+      && FRAMEP (calling_frame)
+      && FRAME_LIVE_P (XFRAME (calling_frame)))
+    {
+      Fset_frame_selected_window (selected_frame,
+                                  Fprevious_window (minibuf_window,
+                                                    Qnil, Qnil),
+                                  Qnil);
+      do_switch_frame (calling_frame, 1, 0, Qnil);
+    }
+
   /* If cursor is on the minibuffer line,
      show the user we have exited by putting it in column 0.  */
   if (XWINDOW (minibuf_window)->cursor.vpos >= 0
@@ -790,6 +851,14 @@ is_minibuffer (EMACS_INT depth, Lisp_Object buf)
     && EQ (Fcar (tail), buf);
 }
 
+/* Return the DEPTHth minibuffer, or nil if such does not yet exist.  */
+static Lisp_Object
+nth_minibuffer (EMACS_INT depth)
+{
+  Lisp_Object tail = Fnthcdr (make_fixnum (depth), Vminibuffer_list);
+  return XCAR (tail);
+}
+
 /* Return a buffer to be used as the minibuffer at depth `depth'.
    depth = 0 is the lowest allowed argument, and that is the value
    used for nonrecursive minibuffer invocations.  */
@@ -852,6 +921,7 @@ read_minibuf_unwind (void)
   Lisp_Object old_deactivate_mark;
   Lisp_Object window;
   Lisp_Object future_mini_window;
+  Lisp_Object map;
 
   /* If this was a recursive minibuffer,
      tie the minibuffer window back to the outer level minibuffer buffer.  */
@@ -888,6 +958,8 @@ read_minibuf_unwind (void)
 #endif
   future_mini_window = Fcar (minibuf_save_list);
   minibuf_save_list = Fcdr (minibuf_save_list);
+  map = Fcar (minibuf_save_list);
+  minibuf_save_list = Fcdr (minibuf_save_list);
 
   /* Erase the minibuffer we were using at this level.  */
   {
@@ -901,6 +973,10 @@ read_minibuf_unwind (void)
     unbind_to (count, Qnil);
   }
 
+  /* Restore the keymap of any outer level recursive minibuffer. */
+  if (minibuf_level > 0)
+    bset_keymap (XBUFFER (nth_minibuffer (minibuf_level)), map);
+
   /* When we get to the outmost level, make sure we resize the
      mini-window back to its normal size.  */
   if (minibuf_level == 0
@@ -2035,13 +2111,15 @@ For example, `eval-expression' uses this.  */);
 The function is called with the arguments passed to `read-buffer'.  */);
   Vread_buffer_function = Qnil;
 
-  DEFVAR_BOOL ("minibuffer-follows-selected-frame", minibuffer_follows_selected_frame,
-               doc: /* Non-nil means the active minibuffer always displays on the selected frame.
+  DEFVAR_LISP ("minibuffer-follows-selected-frame", minibuffer_follows_selected_frame,
+               doc: /* t means the active minibuffer always displays on the selected frame.
 Nil means that a minibuffer will appear only in the frame which created it.
+Any other value means the minibuffer will move onto another frame, but
+only when the user starts using a minniffer.
 
 Any buffer local or dynamic binding of this variable is ignored.  Only the
 default top level value is used.  */);
-  minibuffer_follows_selected_frame = 1;
+  minibuffer_follows_selected_frame = Qt;
 
   DEFVAR_BOOL ("read-buffer-completion-ignore-case",
 	       read_buffer_completion_ignore_case,



>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).