Re: Stop frames stealing eachothers' minibuffers!

[Alan Mackenzie <acm@muc.de>]

Hello, jakanakaevangeli.

On Mon, Feb 08, 2021 at 13:53:43 +0100, jakanakaevangeli wrote:
> Alan Mackenzie <acm@muc.de> writes:

> Hello

> > I spent some time boring into the doc string and the description in the
> > Elisp manual.  The only conclusion I could come to was that C-] is meant
> > to abort EXACTLY ONE level.

> > So I have tweaked the C sources once more and come up with the following
> > patch.  It should apply to the Emacs state after yesterday's patch.
> > Please remove the patch from earlier today before you try to apply the
> > new patch.  Thanks!

> During testing, I didn't encounter any problems related to your latest
> patch so you hard work paid off.

> Sadly, I did encounter 2 additional minibuffer issues which aren't
> related to your latest two patches, that is, they are present regardless
> if these patches are applied or not. I'm posting them here since they
> are still possibly related to the new minibuffer frame-following
> functionality.

Thanks!

> 1) With minibuffer-follows-selected-frame set to t (the default value):
> - press M-x on frame A
> - select frame B (the minibuffer will move to this frame)
> - C-x o, to select the minibuffer
> - C-g to quit it
> Miniwindow stays selected and its buffer is *Minibuf-1* instead of
> *Minibuf-0*. You can check this by evaluating (minibuffer-window).

This one was somewhat tricky to resolve, my fixes uncovering several
other inconsistencies in the minibuffer handling, which I hope I have
now fixed.

> 2) With minibuffer-follows-selected-frame set to nil:
> - (setq set enable-recursive-minibuffers t)
> - (minibuffer-depth-indicate-mode 1)
> - select frame A and press M-x
> - select frame B and press M-x
> - select frame A and close it
> - select frame B and quit its minibuffer with C-g.
> This doesn't quit the outer minibuffer, as expected, but this minibuffer
> isn't shown anywhere and the only reasonable way to quit it is with C-],
> which, I believe, a lot of users don't know about (at least I personally
> didn't until quite recently). You can check this with
> (minibuffer-depth).

This is more involved.  What do we want to happen when a frame with open
minibuffers is deleted?  I would say that these minibuffers should,
except in the (eq minibuffer-follows-selected-frame t) case, be aborted,
together with any others in other frames whose nesting level makes this
necessary.  Then there're complications with recursive edits.  But I
haven't looked at implementing this, yet.  Alternatively, maybe open
minibuffers should be moved to the "previous" frame.  Perhaps I should
open another thread on emacs-devel for this problem.

Anyhow, I have a patch for bug 1.  It is a full diff from master, rather
than a difference from "yesterday".  I would be very grateful indeed if
you could try it out, and let me know again how well it's working.  I
would really like to commit this to master.  Thanks again!



diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index a899a943d4..aacb8ab00b 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2116,18 +2116,19 @@ minibuffer-hide-completions
 (defun exit-minibuffer ()
   "Terminate this minibuffer argument."
   (interactive)
-  (when (or
-         (innermost-minibuffer-p)
-         (not (minibufferp)))
+  (when (minibufferp)
+    (when (not (minibuffer-innermost-command-loop-p))
+      (error "%s" "Not in most nested command loop"))
+    (when (not (innermost-minibuffer-p))
+      (error "%s" "Not in most nested minibuffer")))
   ;; If the command that uses this has made modifications in the minibuffer,
   ;; we don't want them to cause deactivation of the mark in the original
   ;; buffer.
   ;; A better solution would be to make deactivate-mark buffer-local
   ;; (or to turn it into a list of buffers, ...), but in the mean time,
   ;; this should do the trick in most cases.
-    (setq deactivate-mark nil)
-    (throw 'exit nil))
-  (error "%s" "Not in most nested minibuffer"))
+  (setq deactivate-mark nil)
+  (throw 'exit nil))
 
 (defun self-insert-and-exit ()
   "Terminate minibuffer input."
diff --git a/src/eval.c b/src/eval.c
index 3aff3b56d5..91fc4e6837 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1165,21 +1165,23 @@ usage: (catch TAG BODY...)  */)
    FUNC should return a Lisp_Object.
    This is how catches are done from within C code.  */
 
+/* MINIBUFFER_QUIT_LEVEL is to handle quitting from nested minibuffers by
+   throwing t to tag `exit'.
+   0 means there is no (throw 'exit t) in progress, or it wasn't from
+     a minibuffer which isn't the most nested;
+   N > 0 means the `throw' was done from the minibuffer at level N which
+     wasn't the most nested.  */
+EMACS_INT minibuffer_quit_level = 0;
+
 Lisp_Object
 internal_catch (Lisp_Object tag,
 		Lisp_Object (*func) (Lisp_Object), Lisp_Object arg)
 {
-  /* MINIBUFFER_QUIT_LEVEL is to handle quitting from nested minibuffers by
-     throwing t to tag `exit'.
-     Value -1 means there is no (throw 'exit t) in progress;
-     0 means the `throw' wasn't done from an active minibuffer;
-     N > 0 means the `throw' was done from the minibuffer at level N.  */
-  static EMACS_INT minibuffer_quit_level = -1;
   /* This structure is made part of the chain `catchlist'.  */
   struct handler *c = push_handler (tag, CATCHER);
 
   if (EQ (tag, Qexit))
-    minibuffer_quit_level = -1;
+    minibuffer_quit_level = 0;
 
   /* Call FUNC.  */
   if (! sys_setjmp (c->jmp))
@@ -1194,22 +1196,16 @@ internal_catch (Lisp_Object tag,
       Lisp_Object val = handlerlist->val;
       clobbered_eassert (handlerlist == c);
       handlerlist = handlerlist->next;
-      if (EQ (tag, Qexit) && EQ (val, Qt))
+      if (EQ (tag, Qexit) && EQ (val, Qt) && minibuffer_quit_level > 0)
 	/* If we've thrown t to tag `exit' from within a minibuffer, we
 	   exit all minibuffers more deeply nested than the current
 	   one.  */
 	{
-	  EMACS_INT mini_depth = this_minibuffer_depth (Qnil);
-	  if (mini_depth && mini_depth != minibuffer_quit_level)
-	    {
-	      if (minibuffer_quit_level == -1)
-		minibuffer_quit_level = mini_depth;
-	      if (minibuffer_quit_level
-		  && (minibuf_level > minibuffer_quit_level))
-		Fthrow (Qexit, Qt);
-	    }
+	  if (minibuf_level > minibuffer_quit_level
+	      && !NILP (Fminibuffer_innermost_command_loop_p (Qnil)))
+            Fthrow (Qexit, Qt);
 	  else
-	    minibuffer_quit_level = -1;
+	    minibuffer_quit_level = 0;
 	}
       return val;
     }
diff --git a/src/lisp.h b/src/lisp.h
index 409a1e7060..0847324d1f 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4091,6 +4091,7 @@ intern_c_string (const char *str)
 }
 
 /* Defined in eval.c.  */
+extern EMACS_INT minibuffer_quit_level;
 extern Lisp_Object Vautoload_queue;
 extern Lisp_Object Vrun_hooks;
 extern Lisp_Object Vsignaling_function;
@@ -4369,6 +4370,7 @@ extern void syms_of_casetab (void);
 
 /* Defined in keyboard.c.  */
 
+extern EMACS_INT command_loop_level;
 extern Lisp_Object echo_message_buffer;
 extern struct kboard *echo_kboard;
 extern void cancel_echoing (void);
diff --git a/src/minibuf.c b/src/minibuf.c
index 949c3d989d..4b1f4b1ff7 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -41,6 +41,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
    minibuffer recursions are encountered.  */
 
 Lisp_Object Vminibuffer_list;
+Lisp_Object Vcommand_loop_level_list;
 
 /* Data to remember during recursive minibuffer invocations.  */
 
@@ -64,6 +65,8 @@ static Lisp_Object minibuf_prompt;
 static ptrdiff_t minibuf_prompt_width;
 
 static Lisp_Object nth_minibuffer (EMACS_INT depth);
+static EMACS_INT minibuf_c_loop_level (EMACS_INT depth);
+static void set_minibuffer_mode (Lisp_Object buf, EMACS_INT depth);
 
 \f
 /* Return TRUE when a frame switch causes a minibuffer on the old
@@ -181,7 +184,12 @@ void move_minibuffer_onto_frame (void)
 	set_window_buffer (sf->minibuffer_window, nth_minibuffer (i), 0, 0);
       minibuf_window = sf->minibuffer_window;
       if (of != sf)
-	set_window_buffer (of->minibuffer_window, get_minibuffer (0), 0, 0);
+	{
+	  Lisp_Object temp = get_minibuffer (0);
+
+	  set_window_buffer (of->minibuffer_window, temp, 0, 0);
+	  set_minibuffer_mode (temp, 0);
+	}
     }
 }
 
@@ -389,6 +397,21 @@ No argument or nil as argument means use the current buffer as BUFFER.  */)
     : Qnil;
 }
 
+DEFUN ("minibuffer-innermost-command-loop-p", Fminibuffer_innermost_command_loop_p,
+       Sminibuffer_innermost_command_loop_p, 0, 1, 0,
+       doc: /* Return t if BUFFER is a minibuffer at the current command loop level.
+No argument or nil as argument means use the current buffer as BUFFER.  */)
+  (Lisp_Object buffer)
+{
+  EMACS_INT depth;
+  if (NILP (buffer))
+    buffer = Fcurrent_buffer ();
+  depth = this_minibuffer_depth (buffer);
+  return depth && minibuf_c_loop_level (depth) == command_loop_level
+    ? Qt
+    : Qnil;
+}
+
 /* Return the nesting depth of the active minibuffer BUFFER, or 0 if
    BUFFER isn't such a thing.  If BUFFER is nil, this means use the current
    buffer.  */
@@ -420,12 +443,17 @@ confirm the aborting of the current minibuffer and all contained ones.  */)
 
   if (!minibuf_depth)
     error ("Not in a minibuffer");
+  if (NILP (Fminibuffer_innermost_command_loop_p (Qnil)))
+    error ("Not in most nested command loop");
   if (minibuf_depth < minibuf_level)
     {
       array[0] = fmt;
       array[1] = make_fixnum (minibuf_level - minibuf_depth + 1);
       if (!NILP (Fyes_or_no_p (Fformat (2, array))))
-	Fthrow (Qexit, Qt);
+	{
+	  minibuffer_quit_level = minibuf_depth;
+	  Fthrow (Qexit, Qt);
+	}
     }
   else
     Fthrow (Qexit, Qt);
@@ -508,6 +536,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object mini_frame, ambient_dir, minibuffer, input_method;
   Lisp_Object calling_frame = selected_frame;
+  Lisp_Object calling_window = selected_window;
   Lisp_Object enable_multibyte;
   EMACS_INT pos = 0;
   /* String to add to the history.  */
@@ -598,7 +627,8 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 
   if (minibuf_level > 1
       && minibuf_moves_frame_when_opened ()
-      && !minibuf_follows_frame ())
+      && (!minibuf_follows_frame ()
+	  || (!EQ (mini_frame, selected_frame))))
     {
       EMACS_INT i;
 
@@ -607,8 +637,6 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 	set_window_buffer (minibuf_window, nth_minibuffer (i), 0, 0);
     }
 
-  record_unwind_protect_void (choose_minibuf_frame);
-
   record_unwind_protect (restore_window_configuration,
                          Fcons (Qt, Fcurrent_window_configuration (Qnil)));
 
@@ -640,7 +668,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 (calling_frame,
+			   Fcons (calling_window,
+				  minibuf_save_list))));
   minibuf_save_list
     = Fcons (minibuf_prompt,
 	     Fcons (make_fixnum (minibuf_prompt_width),
@@ -694,6 +724,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   /* Switch to the minibuffer.  */
 
   minibuffer = get_minibuffer (minibuf_level);
+  set_minibuffer_mode (minibuffer, minibuf_level);
   Fset_buffer (minibuffer);
 
   /* Defeat (setq-default truncate-lines t), since truncated lines do
@@ -738,6 +769,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
      where there is an active minibuffer.
      Set them to point to ` *Minibuf-0*', which is always empty.  */
   empty_minibuf = get_minibuffer (0);
+  set_minibuffer_mode (empty_minibuf, 0);
 
   FOR_EACH_FRAME (dummy, frame)
     {
@@ -837,20 +869,6 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 
   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);
-  }
-
   /* 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
@@ -959,11 +977,16 @@ Lisp_Object
 get_minibuffer (EMACS_INT depth)
 {
   Lisp_Object tail = Fnthcdr (make_fixnum (depth), Vminibuffer_list);
+  Lisp_Object cll_tail = Fnthcdr (make_fixnum (depth),
+				  Vcommand_loop_level_list);
   if (NILP (tail))
     {
       tail = list1 (Qnil);
       Vminibuffer_list = nconc2 (Vminibuffer_list, tail);
+      cll_tail = list1 (Qnil);
+      Vcommand_loop_level_list = nconc2 (Vcommand_loop_level_list, cll_tail);
     }
+  XSETCAR (cll_tail, make_fixnum (depth ? command_loop_level : 0));
   Lisp_Object buf = Fcar (tail);
   if (NILP (buf) || !BUFFER_LIVE_P (XBUFFER (buf)))
     {
@@ -973,7 +996,6 @@ get_minibuffer (EMACS_INT depth)
       buf = Fget_buffer_create (lname, Qnil);
       /* Do this before set_minibuffer_mode.  */
       XSETCAR (tail, buf);
-      set_minibuffer_mode (buf, depth);
       /* Although the buffer's name starts with a space, undo should be
 	 enabled in it.  */
       Fbuffer_enable_undo (buf);
@@ -985,12 +1007,19 @@ get_minibuffer (EMACS_INT depth)
 	 while the buffer doesn't know about them any more.  */
       delete_all_overlays (XBUFFER (buf));
       reset_buffer (XBUFFER (buf));
-      set_minibuffer_mode (buf, depth);
     }
 
   return buf;
 }
 
+static EMACS_INT minibuf_c_loop_level (EMACS_INT depth)
+{
+  Lisp_Object cll = Fnth (make_fixnum (depth), Vcommand_loop_level_list);
+  if (FIXNUMP (cll))
+    return XFIXNUM (cll);
+  return 0;
+}
+
 static void
 run_exit_minibuf_hook (void)
 {
@@ -1004,17 +1033,16 @@ static void
 read_minibuf_unwind (void)
 {
   Lisp_Object old_deactivate_mark;
-  Lisp_Object window;
+  Lisp_Object calling_frame;
+  Lisp_Object calling_window;
   Lisp_Object future_mini_window;
 
-  /* If this was a recursive minibuffer,
-     tie the minibuffer window back to the outer level minibuffer buffer.  */
-  minibuf_level--;
-
-  window = minibuf_window;
   /* To keep things predictable, in case it matters, let's be in the
-     minibuffer when we reset the relevant variables.  */
-  Fset_buffer (XWINDOW (window)->contents);
+     minibuffer when we reset the relevant variables.  Don't depend on
+     `minibuf_window' here.  This could by now be the mini-window of any
+     frame.  */
+  Fset_buffer (nth_minibuffer (minibuf_level));
+  minibuf_level--;
 
   /* Restore prompt, etc, from outer minibuffer level.  */
   Lisp_Object key_vec = Fcar (minibuf_save_list);
@@ -1042,6 +1070,10 @@ read_minibuf_unwind (void)
 #endif
   future_mini_window = Fcar (minibuf_save_list);
   minibuf_save_list = Fcdr (minibuf_save_list);
+  calling_frame = Fcar (minibuf_save_list);
+  minibuf_save_list = Fcdr (minibuf_save_list);
+  calling_window = Fcar (minibuf_save_list);
+  minibuf_save_list = Fcdr (minibuf_save_list);
 
   /* Erase the minibuffer we were using at this level.  */
   {
@@ -1059,7 +1091,7 @@ read_minibuf_unwind (void)
      mini-window back to its normal size.  */
   if (minibuf_level == 0
       || !EQ (selected_frame, WINDOW_FRAME (XWINDOW (future_mini_window))))
-    resize_mini_window (XWINDOW (window), 0);
+    resize_mini_window (XWINDOW (minibuf_window), 0);
 
   /* Deal with frames that should be removed when exiting the
      minibuffer.  */
@@ -1090,6 +1122,24 @@ read_minibuf_unwind (void)
      to make sure we don't leave around bindings and stuff which only
      made sense during the read_minibuf invocation.  */
   call0 (intern ("minibuffer-inactive-mode"));
+
+  /* We've exited the recursive edit, so switch the current windows
+     away from the expired minibuffer window, both in the current
+     minibuffer's frame and the original calling frame.  */
+  choose_minibuf_frame ();
+  if (!EQ (WINDOW_FRAME (XWINDOW (minibuf_window)), calling_frame))
+  {
+    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.  */
+    if (!EQ (prev, minibuf_window)
+	&& EQ (WINDOW_FRAME (XWINDOW (prev)),
+	       WINDOW_FRAME (XWINDOW (minibuf_window))))
+      Fset_frame_selected_window (selected_frame, prev, Qnil);
+  }
+  else
+    Fset_frame_selected_window (calling_frame, calling_window, Qnil);
 }
 \f
 
@@ -2137,6 +2187,7 @@ void
 init_minibuf_once (void)
 {
   staticpro (&Vminibuffer_list);
+  staticpro (&Vcommand_loop_level_list);
   pdumper_do_now_and_after_load (init_minibuf_once_for_pdumper);
 }
 
@@ -2150,6 +2201,7 @@ init_minibuf_once_for_pdumper (void)
      restore from a dump file.  pdumper doesn't try to preserve
      frames, windows, and so on, so reset everything related here.  */
   Vminibuffer_list = Qnil;
+  Vcommand_loop_level_list = Qnil;
   minibuf_level = 0;
   minibuf_prompt = Qnil;
   minibuf_save_list = Qnil;
@@ -2380,6 +2432,7 @@ instead. */);
 
   defsubr (&Sminibufferp);
   defsubr (&Sinnermost_minibuffer_p);
+  defsubr (&Sminibuffer_innermost_command_loop_p);
   defsubr (&Sabort_minibuffers);
   defsubr (&Sminibuffer_prompt_end);
   defsubr (&Sminibuffer_contents);
diff --git a/src/window.h b/src/window.h
index 79eb44e7a3..b6f88e8f55 100644
--- a/src/window.h
+++ b/src/window.h
@@ -1120,10 +1120,6 @@ void set_window_buffer (Lisp_Object window, Lisp_Object buffer,
 
 extern Lisp_Object echo_area_window;
 
-/* Depth in recursive edits.  */
-
-extern EMACS_INT command_loop_level;
-
 /* Non-zero if we should redraw the mode lines on the next redisplay.
    Usually set to a unique small integer so we can track the main causes of
    full redisplays in `redisplay--mode-lines-cause'.  */



-- 
Alan Mackenzie (Nuremberg, Germany).