Re: Stop frames stealing eachothers' minibuffers!

[Alan Mackenzie <acm@muc.de>]

Hello, jakanakaevangeli.

Thank you very much indeed for your post, which I have treated as a bug
report.

On Wed, Feb 03, 2021 at 16:20:50 +0100, jakanakaevangeli wrote:
> I am reporting three technical problems with the current minibuffer
> quitting behaviour (on commit 20e48b6fd6cade60e468140a66127d326abfb8ff,
> after your patch was applied):

> 1) If you enter a (non-minibuffer) M-x recursive-edit inside a
>    minibuffer, abort-minibuffers (bound to C-g) will quit the inner
>    recursive edit but not the minibuffer, which is what the command's
>    doc-string suggests. This is only a minor inconvenience. In fact,
>    this behaviour is consistent with previous Emacs versions.

Indeed, in my patch I failed completely to account for recursive edits.
The behaviour you report is indeed a bug.

> 2) If you
>    - set minibuffer-follows-selected-frame to nil,
>    - open a minibuffer on frame A,
>    - open a new inner minibuffer on frame B,
>    - enter a M-x recursive-edit in this minibuffer,
>    - select frame A's outer minibuffer, press C-g and answer yes,
>    abort-minibuffers will fail to quit the outer minibuffer, it will not
>    be visible in the mini-window, but you can check by evaluating
>    (minibuffer-depth).

Yes.  This is a bug closely related to the above.

> 3) This one is a bit opinionated: internal_catch now has undocumented
>    special handling for Qexit.

Yes, also.  I'll be thinking about this in the coming days.  It would
probably be a good idea to put a comment describing the special handling
for (throw 'exit t) before the function.

I notice also, on the page "Text from Minibuffer" in the Elisp manual, I
neglected to change the description of the binding of C-g from the
former `abort-recursive-edit' to the now current `abort-minibuffers'.

> I have come up with an idea to fix all of the above problems:
> - revert the special handling of Qexit in internal_catch
> - in read_minibuf, wrap the call to recursive_edit_1 with a catch for
>   symbol exit-read-minibuffer
> - throwing to this symbol with nil or t shall have the same effect as
>   throwing nil or t to 'exit
> - throwing to this symbol with a natural number N shall re-throw to this
>   same symbol with N-1 (or quit if N<=1), effectively quitting out of N
>   minibuffers
> - make exit-minibuffer throw to 'exit-read-minibuffer
> - make quit-minibuffers throw to 'exit-read-minibuffer with
>   minibuf_level - this_minibuf_depth () + 1

> Please let me know if you find this idea worthy of implementing and if
> you want me to try implementing it and posting a patch.

It's an ingenious idea, but to be honest I don't think it's the best way
to solve the problems.  The handling you're proposing for
exit-read-minibuffer is no less complicated than what's already there
for exit.  I can't help feeling that there would be nasty interactions
between the two catch tags exit and exit-read-minibuffer.  I think it's
documented in the Elisp manual that (throw 'exit t) is the way to abort
a minibuffer and its calling function.

Also, this part of the Emacs C code is horrendous to adapt and maintain,
as I've discovered since November.  ;-)  It is full of special cases
that one is scarcely aware of (a minibuffer in its own frame, for
example).  To be honest, I've become tired of trying to make this
section of the code work properly, but having started on it, I've got to
finish it.

Anyhow, I've attempted to solve the problems you reported, and come up
with the following patch, which should apply cleanly to the savannah
master branch.  I'd be grateful if you could try it out, and report back
as to whether the bugs you noticed are actually fixed by it, or what is
still not quite right.



diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 03cc70c0d4..f8143feedd 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2116,16 +2116,18 @@ minibuffer-hide-completions
 (defun exit-minibuffer ()
   "Terminate this minibuffer argument."
   (interactive)
+  (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.
-  (when (innermost-minibuffer-p)
-    (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 5bf3faebc8..d4456d8b46 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1197,15 +1197,12 @@ internal_catch (Lisp_Object tag,
 	   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 (minibuffer_quit_level == -1)
+            minibuffer_quit_level = this_minibuffer_depth (Qnil);
+          if (minibuf_level > minibuffer_quit_level
+              || command_loop_level
+              > minibuf_c_loop_level (minibuffer_quit_level))
+            Fthrow (Qexit, Qt);
 	  else
 	    minibuffer_quit_level = -1;
 	}
diff --git a/src/lisp.h b/src/lisp.h
index f658868544..035d7b37fc 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4349,6 +4349,7 @@ extern bool is_minibuffer (EMACS_INT, Lisp_Object);
 extern EMACS_INT this_minibuffer_depth (Lisp_Object);
 extern EMACS_INT minibuf_level;
 extern Lisp_Object get_minibuffer (EMACS_INT);
+extern EMACS_INT minibuf_c_loop_level (EMACS_INT depth);
 extern void init_minibuf_once (void);
 extern void syms_of_minibuf (void);
 extern void barf_if_interaction_inhibited (void);
@@ -4368,6 +4369,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..0745c095bb 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,7 @@ static Lisp_Object minibuf_prompt;
 static ptrdiff_t minibuf_prompt_width;
 
 static Lisp_Object nth_minibuffer (EMACS_INT depth);
+static void restore_minibuf_window (Lisp_Object mw);
 
 \f
 /* Return TRUE when a frame switch causes a minibuffer on the old
@@ -389,6 +391,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.  */
@@ -598,7 +615,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;
 
@@ -835,7 +853,18 @@ 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);
 
-  recursive_edit_1 ();
+  {
+    /* Note that here it doesn't suffice to record MINIBUF_WINDOW in a local
+       variable.  It must be correct in `read_minibuf_unwind' in the event of
+       a non-local exit.  */
+    int count2 = SPECPDL_INDEX ();
+
+    record_unwind_protect (restore_minibuf_window, minibuf_window);
+
+    recursive_edit_1 ();
+
+    unbind_to (count2, Qnil);
+  }
 
   /* We've exited the recursive edit without an error, so switch the
      current window away from the expired minibuffer window.  */
@@ -908,6 +937,11 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   return val;
 }
 
+static void restore_minibuf_window (Lisp_Object mw)
+{
+  minibuf_window = mw;
+}
+
 /* Return true if BUF is a particular existing minibuffer.  */
 bool
 is_minibuffer (EMACS_INT depth, Lisp_Object buf)
@@ -959,11 +993,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)))
     {
@@ -991,6 +1030,14 @@ get_minibuffer (EMACS_INT depth)
   return buf;
 }
 
+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)
 {
@@ -2137,6 +2184,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 +2198,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 +2429,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).