bug#49700: 27.2; [PATCH] Refactor minibuffer aborting

[miha]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Miha.
>
> On Fri, Jul 23, 2021 at 01:05:41 +0200, miha@kamnitnik.top wrote:
>> The attached patch removes special handling of the 'exit tag from
>> internal_catch.  This special handling was introduced by Alan in commit
>> Sun Jan 10 20:32:40 2021 +0000
>> (c7c154bb5756e0ae71d342c5d8aabf725877f186), hence me CC-ing him.
>
> Thanks, that's appreciated.
>
> I'm not sure I'm in favour of the change as a whole, since the proposed
> code contains complexities (as does the code it would replace).  I find
> the use of the closures difficult to understand.  But then again, I wrote
> the old code, so I'm not in a position to judge whether the old or the
> new is "better".
>
>> It also exposes Vminibuffer_list to lisp through the new function
>> Fminibuffer_alist.
>
> Like Eli, I'm against this.  Indeed, when I was modifying the minibuffer
> code, I took great care to avoid Vminibuffer_list becoming visible to
> Lisp.  As a result, some of the current code is less elegant than it
> might have been.  The idea of some Lisp looping through all existing
> minibuffers doing something destructive didn't help me sleep well.
>
> As a general point, I'm a bit worried you might not be distinguishing
> between (minibuffer-depth) and (recursive-depth).  They are only the same
> most of the time.  When (recursive-edit) gets called outside of the
> minibuffer code, then these two values are different.  For example, in 
> abort-minibuffers, you've got
>
>> +      (when (yes-or-no-p
>> +             (format "Abort %s minibuffer levels? "
>> +                     (- (recursion-depth) minibuffer-level -1)))
>
> ..  minibuffer-level is confusingly a result of (recursion-depth), not
> (minibuffer-depth), so the code isn't prompting with the number of
> minibuffer levels to be aborted, but the number of recursive edits.
>
> As a small point, the use of cl-decf:
>
>> +        (cl-decf minibuffer-level)))
>
> might be unwise.  Have you checked that it works in a bootstrap build?
> My fear is that in a bootstrap, minibuffer.el might be compiled before
> the CL files, and then cl-decf would be wrongly compiled as a function
> call rather than a macro expansion.  But I haven't checked it myself.

Thanks for feedback.  Attached is a patch that should address all of
these issues.  Overall, this patch is much simpler than the original
patch I proposed and the closure passing should now be hopefully easier
to understand.

> I've also had a look a part of your patch from Tuesday (2021-07-20), and
> am unhappy about some aspects of the change to the documentation on the
> Elisp manual page Recursive Editing.  For example, the text no longer
> says what happens on throwing a random value to 'exit (but it used to).
> Also this text is generally a bit unclear; what does "a function value"
> mean?  I would normally understand "the value returned by a function",
> but here it just means the function.  But I think it would be better for
> me to raise these issues in a different thread.
>
Please take a look at the second patch, attached to this message.  I
tried to improve the documentation of exiting a recursive edit in
lispref.  I also adjusted the doc string of the function
`recursive-edit', which I forgot to do in my older patch from
2021-07-20.
>
> -- 
> Alan Mackenzie (Nuremberg, Germany).

>From 91276c2485b518850b2d0d02be1823439571a3f9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Sat, 31 Jul 2021 13:44:21 +0200
Subject: [PATCH] Refactor minibuffer aborting

* lisp/minibuffer.el (minibuffer-quit-recursive-edit): New optional
argument to specify how many levels of recursion to quit.

* src/minibuf.c (Fabort_minibuffers): Use
minibuffer-quit-recursive-edit to quit multiple levels of minibuffer
recursion.

* src/eval.c (internal_catch): Remove special handling of 'exit tag.
---
 lisp/minibuffer.el | 16 ++++++++++------
 src/eval.c         | 22 ----------------------
 src/lisp.h         |  1 -
 src/minibuf.c      |  4 ++--
 4 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 3751ba80e0..912e186b06 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2328,14 +2328,18 @@ exit-minibuffer
   (setq deactivate-mark nil)
   (throw 'exit nil))
 
-(defun minibuffer-quit-recursive-edit ()
+(defun minibuffer-quit-recursive-edit (&optional levels)
   "Quit the command that requested this recursive edit without error.
 Like `abort-recursive-edit' without aborting keyboard macro
-execution."
-  ;; See Info node `(elisp)Recursive Editing' for an explanation of
-  ;; throwing a function to `exit'.
-  (throw 'exit (lambda ()
-                 (signal 'minibuffer-quit nil))))
+execution.  LEVELS specifies the number of nested recursive edits
+to quit.  If nil, it defaults to 1."
+  (unless levels
+    (setq levels 1))
+  (if (> levels 1)
+      ;; See Info node `(elisp)Recursive Editing' for an explanation
+      ;; of throwing a function to `exit'.
+      (throw 'exit (lambda () (minibuffer-quit-recursive-edit (1- levels))))
+    (throw 'exit (lambda () (signal 'minibuffer-quit nil)))))
 
 (defun self-insert-and-exit ()
   "Terminate minibuffer input."
diff --git a/src/eval.c b/src/eval.c
index 48104bd0f4..76fe671b6d 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1174,14 +1174,6 @@ #define clobbered_eassert(E) verify (sizeof (E) != 0)
    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)
@@ -1189,9 +1181,6 @@ internal_catch (Lisp_Object tag,
   /* This structure is made part of the chain `catchlist'.  */
   struct handler *c = push_handler (tag, CATCHER);
 
-  if (EQ (tag, Qexit))
-    minibuffer_quit_level = 0;
-
   /* Call FUNC.  */
   if (! sys_setjmp (c->jmp))
     {
@@ -1205,17 +1194,6 @@ internal_catch (Lisp_Object tag,
       Lisp_Object val = handlerlist->val;
       clobbered_eassert (handlerlist == c);
       handlerlist = handlerlist->next;
-      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.  */
-       {
-         if (minibuf_level > minibuffer_quit_level
-             && !NILP (Fminibuffer_innermost_command_loop_p (Qnil)))
-            Fthrow (Qexit, Qt);
-         else
-           minibuffer_quit_level = 0;
-       }
       return val;
     }
 }
diff --git a/src/lisp.h b/src/lisp.h
index 15a42a4456..4fdee6c280 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4112,7 +4112,6 @@ 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;
diff --git a/src/minibuf.c b/src/minibuf.c
index 0f4349e70b..f7cd2c5fcc 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -491,8 +491,8 @@ DEFUN ("abort-minibuffers", Fabort_minibuffers, 
Sabort_minibuffers, 0, 0, "",
       array[1] = make_fixnum (minibuf_level - minibuf_depth + 1);
       if (!NILP (Fyes_or_no_p (Fformat (2, array))))
        {
-         minibuffer_quit_level = minibuf_depth;
-         Fthrow (Qexit, Qt);
+         CALLN (Ffuncall, intern ("minibuffer-quit-recursive-edit"),
+                array[1]);
        }
     }
   else
-- 
2.32.0

>From bdccb4d9399090ffe08cbdde289b5a1afd0c310d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Sun, 1 Aug 2021 02:41:37 +0200
Subject: [PATCH] Improve documentation of exiting recursive editing

* doc/lispref/commands.texi (Recursive Editing): Mention what happens
when throwing a string or any other value to 'exit.
* src/keyboard.c (Frecursive_edit): Document throwing a function to 'exit.
---
 doc/lispref/commands.texi | 22 ++++++++++++----------
 src/keyboard.c            | 18 ++++++++++++++----
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
index b4a8b733a0..e0982b14bb 100644
--- a/doc/lispref/commands.texi
+++ b/doc/lispref/commands.texi
@@ -3568,17 +3568,19 @@ Recursive Editing
 @cindex exit recursive editing
 @cindex aborting
   To invoke a recursive editing level, call the function
-@code{recursive-edit}.  This function contains the command loop; it also
-contains a call to @code{catch} with tag @code{exit}, which makes it
-possible to exit the recursive editing level by throwing to @code{exit}
-(@pxref{Catch and Throw}).  If you throw a @code{nil} value, then
-@code{recursive-edit} returns normally to the function that called it.
-The command @kbd{C-M-c} (@code{exit-recursive-edit}) does this.
-Throwing a @code{t} value causes @code{recursive-edit} to quit, so that
-control returns to the command loop one level up.  This is called
-@dfn{aborting}, and is done by @kbd{C-]} (@code{abort-recursive-edit}).
-You can also throw a function value.  In that case,
+@code{recursive-edit}.  This function contains the command loop; it
+also contains a call to @code{catch} with tag @code{exit}, which makes
+it possible to exit the recursive editing level by throwing to
+@code{exit} (@pxref{Catch and Throw}).  Throwing a @code{t} value
+causes @code{recursive-edit} to quit, so that control returns to the
+command loop one level up.  This is called @dfn{aborting}, and is done
+by @kbd{C-]} (@code{abort-recursive-edit}).  Similarly, you can throw
+a string value to make @code{recursive-edit} signal an error, printing
+this string as the message.  If you throw a function,
 @code{recursive-edit} will call it without arguments before returning.
+Throwing any other value, will make @code{recursive-edit} return
+normally to the function that called it.  The command @kbd{C-M-c}
+(@code{exit-recursive-edit}) does this.
 
   Most applications should not use recursive editing, except as part of
 using the minibuffer.  Usually it is more convenient for the user if you
diff --git a/src/keyboard.c b/src/keyboard.c
index 820229cf8f..06509bcb72 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -753,10 +753,20 @@ DEFUN ("recursive-edit", Frecursive_edit, 
Srecursive_edit, 0, 0, "",
        doc: /* Invoke the editor command loop recursively.
 To get out of the recursive edit, a command can throw to `exit' -- for
 instance (throw \\='exit nil).
-If you throw a value other than t, `recursive-edit' returns normally
-to the function that called it.  Throwing a t value causes
-`recursive-edit' to quit, so that control returns to the command loop
-one level up.
+
+The following values can be thrown to 'exit:
+
+- t causes `recursive-edit' to quit, so that control returns to the
+  command loop one level up.
+
+- A string causes `recursive-edit' to signal an error, printing this
+  string as the message.
+
+- A function causes `recursive-edit' to call this function without
+  arguments before returning normally.
+
+- Any other value causes `recursive-edit' to return normally to the
+  function that called it.
 
 This function is called by the editor initialization to begin editing.  */)
   (void)
-- 
2.32.0

signature.asc
Description: PGP signature