Re: Stop frames stealing eachothers' minibuffers!

[Alan Mackenzie <acm@muc.de>]

Hello, Martin, Andrii, Gregory, and everybody else.

I have been working on this topic over the last few days, and think the
time is almost ripe for another commit to master.  In particular,
several bugs have been fixed.

I would be grateful to anybody who tests out the patch below.  Otherwise
I think the patch is pretty much ready for committing.

On Fri, Nov 27, 2020 at 11:36:47 +0100, martin rudalics wrote:
>  >> If, with Emacs 28, I set 'minibuffer-follows-selected-frame' to non-nil,
>  >
>  > Do you mean "to nil", here?  That variable is non-nil by default.

> Right.  I meant "to nil" here.

>  >> the behavior does not entirely match that of Emacs 27 because the second
>  >> RET must be typed in the first frame.  So if some application relies on
>  >> the exact replication of the behavior of Emacs 27, we have a regression.
>  >
>  > Well the new behaviour is explicitly not wholly compatible with the old.
>  > I'm not sure that counts as a regression.

> Having a customizable variable like 'minibuffer-follows-selected-frame'
> whose purpose is to get back the old behavior, should also provide that
> old behavior as faithfully as possible IMHO.

Due to protests, minibuffer-follows-selected-frame can now also take a
non-nil, non-t value which provides an approximation of the old
behaviour.

The bugs which have been fixed are these:


1/- "Madhu's bug".  Set pop-up-frames to 'graphic-only, do M-!, type
g TAB, which opens *Completions* in a new frame.  Select something.  The
*Completions* frame should be deleted, but isn't.

This was a problem in window-deleteable-p (window.el), where if an
active minibuffer is in a frame, that frame is regarded as not to be
deleted.  The new minibuffer mechanisms move minibuffers to the new
frame on a frame change, so this obviously(?) clashed.  I have amended
window-deleteable-p to take account of
minibuffer-follows-selected-frame and the new mechanisms.


2/- From Martin: Start emacs -Q -l foo.el, where foo.el is:

(setq default-frame-alist '((minibuffer . nil)))

(defun foo ()
  (interactive)
  (read-from-minibuffer "...?")
  (insert (format "%s" (selected-frame))))

(global-set-key [(control meta +)] 'foo)

(setq enable-recursive-minibuffers t)

Do C-M-+, type something into the minibuffer, and either selected-frame
announced *Minibuffer-1*, or there was an error about "Window not being
in Frame".  Both of these problems are now fixed.


3/- From Gregory: Start emacs -Q, and set enable-recursive-minibuffers
to t.   Do C-x C-f  C-x 5 o twice, then C-x C-f a third time.  It was
possible to enter filenames for and visit files for the innermost two
minibuffers, but not the outermost one.  This has (I believe) been
fixed.


4/- With minibuffer-follows-selected-frame nil, and
enable-recursive-minibuffers t, there were problems caused by editing
outer level minibuffers whilst an inner level buffer was still active.
I've tried to fix this by giving outer level MBs the keymap
minibuffer-inactive-mode-map temporariliy whilst a recursive MB is
active.


One bug which I haven't fixed, and doesn't appear to be to do with these
changes, is:


5/- emacs -Q, enter the following into *Scratch*:

(defun bar ()
  (interactive)
  (read-from-minibuffer "...?")
  (insert "window: %s ... frame: %s"
          (selected-window) (selected-frame)))

, evaluate it, then evaluate (bar).  The window announced under "window:"
is *Scratch*, that under "frame:" is *Minibuf-1*.  It seems they should
match.

I think the problem is that frame->name doesn't get appear to get set on
a set-frame-selected-window call.  I think the command loop sets
frame->name, and the recursive command loop in read_minibuf sets it to
*Minibuf-1*, and nothing else changes it until the next iteration of the
main command loop.

Also, this bug was in the version of master just before I made my first
commit in this area.


OK, that's enough talking.  Here's the current version of the patch,
which might well be ready to commit.  As already said, I'd be grateful
for anybody who tests it.  Thanks!



diff --git a/doc/emacs/mini.texi b/doc/emacs/mini.texi
index c7c8fb30ac..f81e64bdf9 100644
--- a/doc/emacs/mini.texi
+++ b/doc/emacs/mini.texi
@@ -76,9 +76,13 @@ Basic Minibuffer
 the user option @code{minibuffer-follows-selected-frame} to
 @code{nil}, then the minibuffer stays in the frame where you opened
 it, and you must switch back to that frame in order to complete (or
-abort) the current command.  Note that the effect of the command, when
-you finally finish using the minibuffer, always takes place in the
-frame where you first opened it.
+abort) the current command.  If you set that option to a value which
+is neither @code{nil} nor @code{t}, the minibuffer moves frame only
+after a recursive minibuffer has been opened in the current command
+(@pxref{Recursive Mini,,, elisp}).  This option is mainly to retain
+(approximately) the behavior prior to Emacs 28.1.  Note that the
+effect of the command, when you finally finish using the minibuffer,
+always takes place in the frame where you first opened it.
 
 @node Minibuffer File
 @section Minibuffers for File Names
diff --git a/etc/NEWS b/etc/NEWS
index b294ff1d23..bd707cb047 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -102,12 +102,13 @@ effect should be negligible in the vast majority of cases anyway.
 By default, when you switch to another frame, an active minibuffer now
 moves to the newly selected frame.  Nevertheless, the effect of what
 you type in the minibuffer happens in the frame where the minibuffer
-was first activated, even if it moved to another frame.  An
-alternative behavior is available by customizing
-'minibuffer-follows-selected-frame' to nil.  Here, the minibuffer
-stays in the frame where you first opened it, and you must switch back
-to this frame to continue or abort its command.  The old, somewhat
-unsystematic behavior, which mixed these two is no longer available.
+was first activated.  An alternative behavior is available by
+customizing 'minibuffer-follows-selected-frame' to nil.  Here, the
+minibuffer stays in the frame where you first opened it, and you must
+switch back to this frame to continue or abort its command.  The old
+behavior, which mixed these two, can be approximated by customizing
+'minibuffer-follows-selected-frame' to a value which is neither nil
+nor t.
 
 +++
 ** New system for displaying documentation for groups of functions.
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index 85dd14f628..0293d34d1c 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -394,7 +394,11 @@ minibuffer-prompt-properties--setter
 	     ;;    			(directory :format "%v"))))
 	     (load-prefer-newer lisp boolean "24.4")
 	     ;; minibuf.c
-             (minibuffer-follows-selected-frame minibuffer boolean "28.1")
+	     (minibuffer-follows-selected-frame
+              minibuffer (choice (const :tag "Always" t)
+                                 (const :tag "When used" hybrid)
+                                 (const :tag "Never" nil))
+              "28.1")
 	     (enable-recursive-minibuffers minibuffer boolean)
 	     (history-length minibuffer
 			     (choice (const :tag "Infinite" t) integer)
diff --git a/lisp/window.el b/lisp/window.el
index cd13e6603a..4b7d2c4677 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -4116,7 +4116,10 @@ window-deletable-p
 				     frame))
 			(throw 'other t))))
 		  (let ((minibuf (active-minibuffer-window)))
-		    (and minibuf (eq frame (window-frame minibuf)))))
+		    (and minibuf (eq frame (window-frame minibuf))
+                         (not (eq (default-toplevel-value
+                                    minibuffer-follows-selected-frame)
+                                  t)))))
 	'frame))
      ((window-minibuffer-p window)
       ;; If WINDOW is the minibuffer window of a non-minibuffer-only
diff --git a/src/minibuf.c b/src/minibuf.c
index 8b23569019..be4ce9d321 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,37 @@ 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;
-
-      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);
-    }
+  {
+    Lisp_Object tail, frame;
+    struct frame *sf = XFRAME (selected_frame);
+    struct frame *of;
+
+    FOR_EACH_FRAME (tail, frame)
+      if (!EQ (frame, selected_frame)
+          && minibuf_level > 1)
+        {
+          of = XFRAME (frame);
+          if (MINI_WINDOW_P (XWINDOW (FRAME_SELECTED_WINDOW (of))))
+            Fset_frame_selected_window (frame, Fframe_first_window (frame),
+                                        Qnil);
+
+          if (!EQ (XWINDOW (of->minibuffer_window)->contents,
+                   nth_minibuffer (0)))
+            set_window_buffer (of->minibuffer_window,
+                               nth_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 +443,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 +565,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),
@@ -648,6 +683,17 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
         }
     }
 
+  if (minibuf_moves_frame_when_opened ())
+  {
+    EMACS_INT i;
+
+    /* Stack up all the (recursively) open minibuffers on the selected
+       mini_window.  */
+    for (i = 1; i < minibuf_level; i++)
+      set_window_buffer (XFRAME (mini_frame)->minibuffer_window,
+                         nth_minibuffer (i), 0, 0);
+  }
+
   /* Display this minibuffer in the proper window.  */
   /* Use set_window_buffer instead of Fset_window_buffer (see
      discussion of bug#11984, bug#12025, bug#12026).  */
@@ -727,8 +773,39 @@ 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
+     current window away from the expired minibuffer window.  */
+  {
+    Lisp_Object prev = Fprevious_window (minibuf_window, Qnil, Qnil);
+    /* PREV can be on a different frame when we have a minibuffer only
+       frame, the other frame's minibuffer window is MINIBUF_WINDOW,
+       and its "focus window" is also MINIBUF_WINDOW.  */
+    while (!EQ (prev, minibuf_window)
+	   && !EQ (selected_frame, WINDOW_FRAME (XWINDOW (prev))))
+      prev = Fprevious_window (prev, Qnil, Qnil);
+    if (!EQ (prev, minibuf_window))
+      Fset_frame_selected_window (selected_frame, prev, Qnil);
+  }
+
+  /* Switch the frame back to the calling frame.  */
+  if (!EQ (selected_frame, calling_frame)
+      && FRAMEP (calling_frame)
+      && FRAME_LIVE_P (XFRAME (calling_frame)))
+    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 +867,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 +937,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 +974,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 +989,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 +2127,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 there.
 
 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,



> martin

-- 
Alan Mackenzie (Nuremberg, Germany).