unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
@ 2021-07-22 23:05 miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-07-23  5:42 ` Eli Zaretskii
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-07-22 23:05 UTC (permalink / raw)
  To: 49700; +Cc: Alan Mackenzie


[-- Attachment #1.1: Type: text/plain, Size: 321 bytes --]

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.

It also exposes Vminibuffer_list to lisp through the new function
Fminibuffer_alist.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Refactor-minibuffer-aborting.patch --]
[-- Type: text/x-patch, Size: 11654 bytes --]

From 498dbfbd9e527183fce34e548b7362e5db1b25bf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Thu, 22 Jul 2021 17:49:45 +0200
Subject: [PATCH] Refactor minibuffer aborting

* src/minibuf.c (Finnermost_minibuffer_p): Remove.  Use the new
function `minibuffer-alist' instead.

(Fminibuffer_innermost_command_loop_p): Remove.

(minibuf_c_loop_level): Remove, not needed anymore.

(Fminibuffer_alist): New function.

(Fabort_minibuffers): Re-implement this function in lisp.  To quit
multiple recursive edit levels, use the mechanism of throwing a
function value to 'exit.

* lisp/minibuffer.el (exit-minibuffer): Use `minibuffer-alist'.

* doc/lispref/minibuf.texi (Recursive Mini):
* etc/NEWS: Document new function `minibuffer-alist'.

* src/eval.c (internal_catch): Remove special handling of 'exit tag.
---
 doc/lispref/minibuf.texi |  8 ++++
 etc/NEWS                 |  4 ++
 lisp/minibuffer.el       | 44 ++++++++++++++++++--
 src/eval.c               | 22 ----------
 src/lisp.h               |  1 -
 src/minibuf.c            | 89 ++++++++++------------------------------
 6 files changed, 74 insertions(+), 94 deletions(-)

diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index 196dd99076..90e738f3ea 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -2645,6 +2645,14 @@ Recursive Mini
 returns zero.
 @end defun
 
+@defun minibuffer-alist
+Return an alist of all minibuffers and their recursion depths.  The
+car of each element is a minibuffer and the cdr is what the function
+@code{recursion-depth} would have returned after this minibuffer
+activation.  The number of elements of the returned list is equal to
+the current minibuffer depth.
+@end defun
+
 @defopt enable-recursive-minibuffers
 If this variable is non-@code{nil}, you can invoke commands (such as
 @code{find-file}) that use minibuffers even while the minibuffer is
diff --git a/etc/NEWS b/etc/NEWS
index 95218faa1b..364c1b814b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -3094,6 +3094,10 @@ The former is now declared obsolete.
 \f
 * Lisp Changes in Emacs 28.1
 
++++
+** New function 'minibuffer-alist'
+Returns an alist of all minibuffers and their recursion depths.
+
 +++
 *** New function 'split-string-shell-command'.
 This splits a shell command string into separate components,
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 1578ab8e1e..702e7e105d 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2315,10 +2315,11 @@ exit-minibuffer
   "Terminate this minibuffer argument."
   (interactive)
   (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")))
+    (let ((minibufs (minibuffer-alist)))
+      (when (not (eq (current-buffer) (caar minibufs)))
+        (error "%s" "Not in most nested minibuffer"))
+      (when (/= (recursion-depth) (cdar minibufs))
+        (error "%s" "Not in most nested command loop"))))
   ;; 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.
@@ -2328,6 +2329,41 @@ exit-minibuffer
   (setq deactivate-mark nil)
   (throw 'exit nil))
 
+(defun abort-minibuffers ()
+  "Abort the current minibuffer.
+If we are not currently in the innermost minibuffer, prompt the user to
+confirm the aborting of the current minibuffer and all contained ones."
+  (interactive)
+  (let* ((minibufs (minibuffer-alist))
+         (buffer (current-buffer))
+         (found nil)
+         (minibuffer-level (recursion-depth))
+         (outermost-p t))
+    (while (and (not found) minibufs)
+      (when (/= minibuffer-level (cdar minibufs))
+        (error "Not in most nested command loop"))
+      (if (eq buffer (caar minibufs))
+          (setq found t)
+        (setq outermost-p nil)
+        (setq minibufs (cdr minibufs))
+        (cl-decf minibuffer-level)))
+    (unless found
+      (error "Not in a minibuffer"))
+    (if outermost-p
+        (minibuffer-quit-recursive-edit)
+      (when (yes-or-no-p
+             (format "Abort %s minibuffer levels? "
+                     (- (recursion-depth) minibuffer-level -1)))
+        (let (exit-fun)
+          (setq exit-fun
+                (lambda ()
+                  (if (> (recursion-depth) minibuffer-level)
+                      (throw 'exit exit-fun)
+                    (signal 'minibuffer-quit nil))))
+          ;; See Info node `(elisp)Recursive Editing' for an
+          ;; explanation of throwing a function to `exit'.
+          (throw 'exit exit-fun))))))
+
 (defun minibuffer-quit-recursive-edit ()
   "Quit the command that requested this recursive edit without error.
 Like `abort-recursive-edit' without aborting keyboard macro
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 80efd77113..fd89b464fc 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4111,7 +4111,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..d9e404b7b0 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -71,7 +71,6 @@ Copyright (C) 1985-1986, 1993-2021 Free Software Foundation, Inc.
 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);
 static bool live_minibuffer_p (Lisp_Object);
 
@@ -423,33 +422,28 @@ DEFUN ("minibufferp", Fminibufferp,
     ? Qt : Qnil;
 }
 
-DEFUN ("innermost-minibuffer-p", Finnermost_minibuffer_p,
-       Sinnermost_minibuffer_p, 0, 1, 0,
-       doc: /* Return t if BUFFER is the most nested active minibuffer.
-No argument or nil as argument means use the current buffer as BUFFER.  */)
-  (Lisp_Object buffer)
-{
-  if (NILP (buffer))
-    buffer = Fcurrent_buffer ();
-  return EQ (buffer, (Fcar (Fnthcdr (make_fixnum (minibuf_level),
-				     Vminibuffer_list))))
-    ? Qt
-    : 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)
+DEFUN ("minibuffer-alist", Fminibuffer_alist, Sminibuffer_alist, 0, 0, 0,
+       doc: /* Return an alist of minibuffers.
+Elements are of the form (MINIBUFFER . DEPTH), where depth is the
+recursion depth the MINIBUFFER.  The returned minibuffers are sorted
+in the order from the innermost minibuffer to the outermost one.  */)
+  (void)
 {
-  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;
+  Lisp_Object bufs_tail = Fcdr (Vminibuffer_list);
+  Lisp_Object cll_tail = Fcdr (Vcommand_loop_level_list);
+  Lisp_Object ret = Qnil;
+  for (int i = 1;
+       i <= minibuf_level && !NILP (bufs_tail) && !NILP (cll_tail);
+       i++)
+    {
+      EMACS_INT depth = XFIXNUM (Fcar (cll_tail)) + i;
+      Lisp_Object pair = Fcons (Fcar (bufs_tail),
+				make_fixnum (depth));
+      ret = Fcons (pair, ret);
+      cll_tail = Fcdr (cll_tail);
+      bufs_tail = Fcdr (bufs_tail);
+    }
+  return ret;
 }
 
 /* Return the nesting depth of the active minibuffer BUFFER, or 0 if
@@ -471,35 +465,6 @@ this_minibuffer_depth (Lisp_Object buffer)
   return 0;
 }
 
-DEFUN ("abort-minibuffers", Fabort_minibuffers, Sabort_minibuffers, 0, 0, "",
-       doc: /* Abort the current minibuffer.
-If we are not currently in the innermost minibuffer, prompt the user to
-confirm the aborting of the current minibuffer and all contained ones.  */)
-  (void)
-{
-  EMACS_INT minibuf_depth = this_minibuffer_depth (Qnil);
-  Lisp_Object array[2];
-  AUTO_STRING (fmt, "Abort %s minibuffer levels? ");
-
-  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))))
-	{
-	  minibuffer_quit_level = minibuf_depth;
-	  Fthrow (Qexit, Qt);
-	}
-    }
-  else
-    CALLN (Ffuncall, intern ("minibuffer-quit-recursive-edit"));
-  return Qnil;
-}
-
 DEFUN ("minibuffer-prompt-end", Fminibuffer_prompt_end,
        Sminibuffer_prompt_end, 0, 0, 0,
        doc: /* Return the buffer position of the end of the minibuffer prompt.
@@ -1045,14 +1010,6 @@ get_minibuffer (EMACS_INT 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)
 {
@@ -2539,9 +2496,7 @@ syms_of_minibuf (void)
   defsubr (&Sminibuffer_prompt);
 
   defsubr (&Sminibufferp);
-  defsubr (&Sinnermost_minibuffer_p);
-  defsubr (&Sminibuffer_innermost_command_loop_p);
-  defsubr (&Sabort_minibuffers);
+  defsubr (&Sminibuffer_alist);
   defsubr (&Sminibuffer_prompt_end);
   defsubr (&Sminibuffer_contents);
   defsubr (&Sminibuffer_contents_no_properties);
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-22 23:05 bug#49700: 27.2; [PATCH] Refactor minibuffer aborting miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-07-23  5:42 ` Eli Zaretskii
  2021-07-23  7:26   ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-07-23 21:03 ` Alan Mackenzie
       [not found] ` <YPsnLZa5vmDYIpxX@ACM>
  2 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2021-07-23  5:42 UTC (permalink / raw)
  To: miha; +Cc: acm, 49700

> Cc: Alan Mackenzie <acm@muc.de>
> Date: Fri, 23 Jul 2021 01:05:41 +0200
> From: miha--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 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.
> 
> It also exposes Vminibuffer_list to lisp through the new function
> Fminibuffer_alist.

Thanks, but could you please explain the rationale and the motivation
for these changes?





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-23  5:42 ` Eli Zaretskii
@ 2021-07-23  7:26   ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-07-23  7:32     ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-07-23  7:26 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: acm, 49700

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Alan Mackenzie <acm@muc.de>
>> Date: Fri, 23 Jul 2021 01:05:41 +0200
>> From: miha--- via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> 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.
>> 
>> It also exposes Vminibuffer_list to lisp through the new function
>> Fminibuffer_alist.
>
> Thanks, but could you please explain the rationale and the motivation
> for these changes?

Refactoring to have cleaner code.

Right now, without applying this patch, quitting multiple recursive
edits (in minibuffer-exit) is achieved by extra special handling in
internal_catch.  In my opinion, it's cleaner to avoid adding such code
into a core function like internal_catch if possible.  This patch moves
this code into the function minibuffer-exit, and by using closures, it
achieves the same effect without a global variable
(minibuffer_quit_level).

In other words, without this patch, Fminibuffer_exit cooperates with
internal_catch through a global variable.  And with this patch,
Fminibuffer_exit cooperates with command_loop by passing it a closure.

Fminibuffer_exit was moved to lisp because its easier to make closures
in lisp.

minibuffer-alist was introduced because it's needed by minibuffer-exit.
I also think that it's nice to expose the list of minibuffers to lisp.

The two minibuffer-innermost-[command-loop]-p functions were removed
because minibuffer-alist can be used instead.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-23  7:26   ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-07-23  7:32     ` Eli Zaretskii
  2021-07-23  8:34       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2021-07-23  7:32 UTC (permalink / raw)
  To: miha; +Cc: acm, 49700

> From: <miha@kamnitnik.top>
> Cc: acm@muc.de, 49700@debbugs.gnu.org
> Date: Fri, 23 Jul 2021 09:26:21 +0200
> 
> > Thanks, but could you please explain the rationale and the motivation
> > for these changes?
> 
> Refactoring to have cleaner code.
> 
> Right now, without applying this patch, quitting multiple recursive
> edits (in minibuffer-exit) is achieved by extra special handling in
> internal_catch.  In my opinion, it's cleaner to avoid adding such code
> into a core function like internal_catch if possible.  This patch moves
> this code into the function minibuffer-exit, and by using closures, it
> achieves the same effect without a global variable
> (minibuffer_quit_level).
> 
> In other words, without this patch, Fminibuffer_exit cooperates with
> internal_catch through a global variable.  And with this patch,
> Fminibuffer_exit cooperates with command_loop by passing it a closure.
> 
> Fminibuffer_exit was moved to lisp because its easier to make closures
> in lisp.
> 
> minibuffer-alist was introduced because it's needed by minibuffer-exit.
> I also think that it's nice to expose the list of minibuffers to lisp.

Thanks.

I'd prefer not to expose minibuffer-alist to Lisp if it can be
avoided.  This is a tricky area of Emacs, and exposing it to Lisp IMO
gives Lisp programmers too much rope to hang themselves.  Is it
feasible to make these changes without exposing the alist?





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-23  7:32     ` Eli Zaretskii
@ 2021-07-23  8:34       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-07-23 10:31         ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-07-23  8:34 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: acm, 49700

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: <miha@kamnitnik.top>
>> Cc: acm@muc.de, 49700@debbugs.gnu.org
>> Date: Fri, 23 Jul 2021 09:26:21 +0200
>> 
>> > Thanks, but could you please explain the rationale and the motivation
>> > for these changes?
>> 
>> Refactoring to have cleaner code.
>> 
>> Right now, without applying this patch, quitting multiple recursive
>> edits (in minibuffer-exit) is achieved by extra special handling in
>> internal_catch.  In my opinion, it's cleaner to avoid adding such code
>> into a core function like internal_catch if possible.  This patch moves
>> this code into the function minibuffer-exit, and by using closures, it
>> achieves the same effect without a global variable
>> (minibuffer_quit_level).
>> 
>> In other words, without this patch, Fminibuffer_exit cooperates with
>> internal_catch through a global variable.  And with this patch,
>> Fminibuffer_exit cooperates with command_loop by passing it a closure.
>> 
>> Fminibuffer_exit was moved to lisp because its easier to make closures
>> in lisp.
>> 
>> minibuffer-alist was introduced because it's needed by minibuffer-exit.
>> I also think that it's nice to expose the list of minibuffers to lisp.
>
> Thanks.
>
> I'd prefer not to expose minibuffer-alist to Lisp if it can be
> avoided.  This is a tricky area of Emacs, and exposing it to Lisp IMO
> gives Lisp programmers too much rope to hang themselves.

Well, the minibuffer list is already kind of exposed to lisp, try:
(seq-filter #'minibufferp (buffer-list))

minibuffer-alist returns a newly constructed list, similar to
buffer-list, so modifying the list structure is safe.  What could be
unsafe is modifying the minibuffers themselves, renaming or killing
them.  I believe that, since such actions are possible without the use
of minibuffer-alist
(for example, by evaluating (kill-buffer " *Minibuf-1")), they should
not mess up Emacs internals and it should be treated as a bug if they
do.

> Is it feasible to make these changes without exposing the alist?

Yes it is feasible.  If the above didn't convince you, please send
another e-mail and I will try.  Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-23  8:34       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-07-23 10:31         ` Eli Zaretskii
  2021-07-23 11:13           ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2021-07-23 10:31 UTC (permalink / raw)
  To: miha; +Cc: acm, 49700

> From: <miha@kamnitnik.top>
> Cc: acm@muc.de, 49700@debbugs.gnu.org
> Date: Fri, 23 Jul 2021 10:34:45 +0200
> 
> > I'd prefer not to expose minibuffer-alist to Lisp if it can be
> > avoided.  This is a tricky area of Emacs, and exposing it to Lisp IMO
> > gives Lisp programmers too much rope to hang themselves.
> 
> Well, the minibuffer list is already kind of exposed to lisp, try:
> (seq-filter #'minibufferp (buffer-list))

Then why did you need to introduce a new function?  It's fine with me
to use the above if it does the job.

> minibuffer-alist returns a newly constructed list, similar to
> buffer-list, so modifying the list structure is safe.  What could be
> unsafe is modifying the minibuffers themselves, renaming or killing
> them.

Exactly.  And there are more atrocities that can be done with these
buffers.

> I believe that, since such actions are possible without the use
> of minibuffer-alist
> (for example, by evaluating (kill-buffer " *Minibuf-1")), they should
> not mess up Emacs internals and it should be treated as a bug if they
> do.

I'd like to make that as difficult as possible.  When someone reports
a bug, it could take some time and effort to discover that the code
does something it shouldn't, and that eats up our precious resources.
Also, if we expose the list of minibuffers explicitly, and with
auxiliary information on top of that, it is hard to defend the
position that Lisp programs should not do anything they want with that
information.

> > Is it feasible to make these changes without exposing the alist?
> 
> Yes it is feasible.  If the above didn't convince you, please send
> another e-mail and I will try.  Thanks.

Yes, please try, and thanks in advance.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-23 10:31         ` Eli Zaretskii
@ 2021-07-23 11:13           ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-07-23 11:41             ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-07-23 11:13 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: acm, 49700

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: <miha@kamnitnik.top>
>> Cc: acm@muc.de, 49700@debbugs.gnu.org
>> Date: Fri, 23 Jul 2021 10:34:45 +0200
>> 
>> > I'd prefer not to expose minibuffer-alist to Lisp if it can be
>> > avoided.  This is a tricky area of Emacs, and exposing it to Lisp IMO
>> > gives Lisp programmers too much rope to hang themselves.
>> 
>> Well, the minibuffer list is already kind of exposed to lisp, try:
>> (seq-filter #'minibufferp (buffer-list))
>
> Then why did you need to introduce a new function?  It's fine with me
> to use the above if it does the job.
>

Yes, the above returns an unsorted list of minibuffers and I don't know
of any way to sort them according to depth.  minibuffer-alist would
return a sorted list.

>> minibuffer-alist returns a newly constructed list, similar to
>> buffer-list, so modifying the list structure is safe.  What could be
>> unsafe is modifying the minibuffers themselves, renaming or killing
>> them.
>
> Exactly.  And there are more atrocities that can be done with these
> buffers.
>
>> I believe that, since such actions are possible without the use
>> of minibuffer-alist
>> (for example, by evaluating (kill-buffer " *Minibuf-1")), they should
>> not mess up Emacs internals and it should be treated as a bug if they
>> do.
>
> I'd like to make that as difficult as possible.  When someone reports
> a bug, it could take some time and effort to discover that the code
> does something it shouldn't, and that eats up our precious resources.
> Also, if we expose the list of minibuffers explicitly, and with
> auxiliary information on top of that, it is hard to defend the
> position that Lisp programs should not do anything they want with that
> information.
>

OK, I understand.

>> > Is it feasible to make these changes without exposing the alist?
>> 
>> Yes it is feasible.  If the above didn't convince you, please send
>> another e-mail and I will try.  Thanks.
>
> Yes, please try, and thanks in advance.

OK, I will, but it might take about a week as I'm currently away.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-23 11:13           ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-07-23 11:41             ` Eli Zaretskii
  0 siblings, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2021-07-23 11:41 UTC (permalink / raw)
  To: miha; +Cc: acm, 49700

> From: <miha@kamnitnik.top>
> Cc: acm@muc.de, 49700@debbugs.gnu.org
> Date: Fri, 23 Jul 2021 13:13:49 +0200
> 
> >> > Is it feasible to make these changes without exposing the alist?
> >> 
> >> Yes it is feasible.  If the above didn't convince you, please send
> >> another e-mail and I will try.  Thanks.
> >
> > Yes, please try, and thanks in advance.
> 
> OK, I will, but it might take about a week as I'm currently away.

Thanks.  There's no rush, so no worries.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-07-22 23:05 bug#49700: 27.2; [PATCH] Refactor minibuffer aborting miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-07-23  5:42 ` Eli Zaretskii
@ 2021-07-23 21:03 ` Alan Mackenzie
       [not found] ` <YPsnLZa5vmDYIpxX@ACM>
  2 siblings, 0 replies; 15+ messages in thread
From: Alan Mackenzie @ 2021-07-23 21:03 UTC (permalink / raw)
  To: 49700

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.

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.

I haven't yet spent enough time looking at your patch.  Perhaps I'll
manage it in the next week.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
       [not found] ` <YPsnLZa5vmDYIpxX@ACM>
@ 2021-08-01  1:23   ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
       [not found]   ` <87im0qrmxe.fsf@miha-pc>
  1 sibling, 0 replies; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-08-01  1:23 UTC (permalink / raw)
  To: 49700


[-- Attachment #1.1: Type: text/plain, Size: 3354 bytes --]

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).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Refactor-minibuffer-aborting.patch --]
[-- Type: text/x-patch, Size: 4424 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0001-Improve-documentation-of-exiting-recursive-editing.patch --]
[-- Type: text/x-patch, Size: 3765 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
       [not found]   ` <87im0qrmxe.fsf@miha-pc>
@ 2021-08-06 20:14     ` Alan Mackenzie
       [not found]     ` <YQ2XHG6k6olofEb/@ACM>
  1 sibling, 0 replies; 15+ messages in thread
From: Alan Mackenzie @ 2021-08-06 20:14 UTC (permalink / raw)
  To: 49700

Hello again, Miha.

On Sun, Aug 01, 2021 at 03:09:01 +0200, miha@kamnitnik.top wrote:
> 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.

I'm not sure it does.  It still looks unclear to me how you are
distinguishing recursive edit levels from minibuffer depth.  For example,
in Fabort_minibuffers (minibuf.c), the argument passed to
minibuffer-quit-recursive-edit is the number of minibuffer levels to be
aborted.  Yet the doc string of minibuffer-quit-recursive-edit refers to 
LEVELS as "the number of nested recursive edits".  Either the doc string
or the code is erroneous here.  Again, what's needed is "the number of
nested minibuffer calls".

I'm also a touch concerned about the "Like `abort-recursive-edit'" in the
doc string, since minibuffer-quit-recursive-edit is significantly
different from abort-recursive-edit.  It can also be aggravating for a
user to have to look somewhere else (here abort-recursive-edit) to
discover the semantics of a Lisp function.

> Overall, this patch is much simpler than the original
> patch I proposed and the closure passing should now be hopefully easier
> to understand.

I think closures are difficult to understand in any circumstances.  But
that's just my personal take on things.

> > 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.

Thanks, that's a lot better!

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
       [not found]     ` <YQ2XHG6k6olofEb/@ACM>
@ 2021-08-06 22:45       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-17 21:47         ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-08-06 22:45 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: Eli Zaretskii, 49700

[-- Attachment #1: Type: text/plain, Size: 7515 bytes --]

Alan Mackenzie <acm@muc.de> writes:

> Hello again, Miha.
>
> On Sun, Aug 01, 2021 at 03:09:01 +0200, miha@kamnitnik.top wrote:
>> 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.
>
> I'm not sure it does. It still looks unclear to me how you are
> distinguishing recursive edit levels from minibuffer depth. For
> example, in Fabort_minibuffers (minibuf.c), the argument passed to
> minibuffer-quit-recursive-edit is the number of minibuffer levels to
> be aborted. Yet the doc string of minibuffer-quit-recursive-edit
> refers to LEVELS as "the number of nested recursive edits". Either the
> doc string or the code is erroneous here. Again, what's needed is "the
> number of nested minibuffer calls".

True, my code would indeed be incorrect if the number of minibuffer
levels to be aborted weren't to match the number of recursive edits.
However, in such cases, my code isn't reached because an error is
signaled that the current minibuffer isn't in the innermost command
loop.

Sorry for not clarifying this earlier. Would it would be enough to
include the following comment in the code?:

/* Due to the above check, the current minibuffer is in the most nested
   command loop, which means that we don't have to abort any extra
   non-minibuffer recursive edits.  Thus, the number of recursive edits
   we have to abort equals the number of minibuffers we have to abort.
   */

I have also tested various sequences of minibuffers and M-x
recursive-edit with and without this patch and in both cases, behaviour
is the same: C-g will only abort minibuffers and will never let you
abort any M-x recursive-edit, even if it is hidden behind nested
minibuffers.

>
> I'm also a touch concerned about the "Like `abort-recursive-edit'" in the
> doc string, since minibuffer-quit-recursive-edit is significantly
> different from abort-recursive-edit.

Hmm... as I see it, they are pretty much equivalent: Code-wise,
abort-recursive-edit throws 't to 'exit, which is equivalent to throwing
(lambda () (signal 'quit)) to 'exit. This is the same as
minibuffer-quit-recursive-edit except for the error signal (and of
course, the new optional argument in this patch and the interactive
spec. Should the new function perhaps also be made interactive?).

Behaviour-wise, as described in the doc string, the only difference is
that abort-recursive-edit causes kmacro termination and the new function
doesn't.

> It can also be aggravating for a user to have to look somewhere else
> (here abort-recursive-edit) to discover the semantics of a Lisp
> function.

Perhaps the following adjustment to the doc string could be considered
(to be applied on top of the latest patch). It copies the beginning of
abort-recursive-edit's doc string and removes the link to it.

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 912e186b06..55886ac015 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2331,6 +2331,6 @@ exit-minibuffer
 (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.  LEVELS specifies the number of nested recursive edits
-to quit.  If nil, it defaults to 1."
+  "Quit the command that requested this recursive edit or minibuffer input.
+Do so without terminationg keyboard macro recording or execution.
+LEVELS specifies the number of nested recursive edits to quit.
+If nil, it defaults to 1."
   (unless levels

>
>> Overall, this patch is much simpler than the original
>> patch I proposed and the closure passing should now be hopefully easier
>> to understand.
>
> I think closures are difficult to understand in any circumstances.  But
> that's just my personal take on things.

In this case, both closures in minibuffer-quit-recursive-edit could be
rewritten in terms of `apply-partially' (which returns a closure,
obviously), but maybe partially applied functions could be easier to
reason about than closures?

>
>> > 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.
>
> Thanks, that's a lot better!
>
> [ .... ]
>
> -- 
> Alan Mackenzie (Nuremberg, Germany).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-08-06 22:45       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-17 21:47         ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-19 19:30           ` Alan Mackenzie
  0 siblings, 1 reply; 15+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-17 21:47 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: Eli Zaretskii, 49700


[-- Attachment #1.1: Type: text/plain, Size: 1433 bytes --]

miha--- via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> True, my code would indeed be incorrect if the number of minibuffer
> levels to be aborted weren't to match the number of recursive edits.
> However, in such cases, my code isn't reached because an error is
> signaled that the current minibuffer isn't in the innermost command
> loop.
>
> Sorry for not clarifying this earlier. Would it would be enough to
> include the following comment in the code?:
>
> /* Due to the above check, the current minibuffer is in the most nested
>    command loop, which means that we don't have to abort any extra
>    non-minibuffer recursive edits.  Thus, the number of recursive edits
>    we have to abort equals the number of minibuffers we have to abort.
>    */
>
> [...]


> Perhaps the following adjustment to the doc string could be considered
> (to be applied on top of the latest patch). It copies the beginning of
> abort-recursive-edit's doc string and removes the link to it.
>
> [...]

I realized that my proposals are kind of scattered over multiple e-mails
in this thread. So I'm sending them again, attached in this mail as
complete patches that apply to current master.

The first patch refactors abort-minibuffers
(and improves minibuffer-quit-recursive-edit's doc string as requested).

The second patch improves documentation of recursive editing.

Best regards.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Refactor-minibuffer-aborting.patch --]
[-- Type: text/x-patch, Size: 5029 bytes --]

From fd5be298c67157822b35cd0231c65691c48dc29e 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 | 20 ++++++++++++--------
 src/eval.c         | 22 ----------------------
 src/lisp.h         |  1 -
 src/minibuf.c      |  9 +++++++--
 4 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 9668e7c732..b5c0054a3c 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2349,14 +2349,18 @@ minibuffer-restore-windows
 
 (add-hook 'minibuffer-exit-hook 'minibuffer-restore-windows)
 
-(defun minibuffer-quit-recursive-edit ()
-  "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))))
+(defun minibuffer-quit-recursive-edit (&optional levels)
+  "Quit the command that requested this recursive edit or minibuffer input.
+Do so without terminating keyboard macro recording or 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 7bfc69b647..4e6298a101 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4113,7 +4113,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 0e7baf30dc..a4219d2a63 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -491,8 +491,13 @@ 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);
+	  /* Due to the above check, the current minibuffer is in the
+	     most nested command loop, which means that we don't have
+	     to abort any extra non-minibuffer recursive edits.  Thus,
+	     the number of recursive edits we have to abort equals the
+	     number of minibuffers we have to abort.  */
+	  CALLN (Ffuncall, intern ("minibuffer-quit-recursive-edit"),
+		 array[1]);
 	}
     }
   else
-- 
2.33.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0001-Improve-documentation-of-exiting-recursive-editing.patch --]
[-- Type: text/x-patch, Size: 3844 bytes --]

From 387baa2b42e8220b0aac36f6f23ec3547ad0811e 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 ddd74d1245..3425880fec 100644
--- a/doc/lispref/commands.texi
+++ b/doc/lispref/commands.texi
@@ -3585,17 +3585,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 63bf29a948..2d97429ade 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.33.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-09-17 21:47         ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-19 19:30           ` Alan Mackenzie
  2021-09-20  6:01             ` Lars Ingebrigtsen
  0 siblings, 1 reply; 15+ messages in thread
From: Alan Mackenzie @ 2021-09-19 19:30 UTC (permalink / raw)
  To: miha; +Cc: acm, 49700

Hello, Miha.

Apologies for loosing track of this thread a few weeks ago.

I spent quite some time trying to find things wrong with your latest
patch, but couldn't.  ;-)  So although I haven't fully tested it, I would
be in favour of merging it to master.

-- 
Alan Mackenzie (Nuremberg, Germany).


On Fri, Sep 17, 2021 at 23:47:35 +0200, miha@kamnitnik.top wrote:
> miha--- via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs@gnu.org> writes:

> > True, my code would indeed be incorrect if the number of minibuffer
> > levels to be aborted weren't to match the number of recursive edits.
> > However, in such cases, my code isn't reached because an error is
> > signaled that the current minibuffer isn't in the innermost command
> > loop.

> > Sorry for not clarifying this earlier. Would it would be enough to
> > include the following comment in the code?:

> > /* Due to the above check, the current minibuffer is in the most nested
> >    command loop, which means that we don't have to abort any extra
> >    non-minibuffer recursive edits.  Thus, the number of recursive edits
> >    we have to abort equals the number of minibuffers we have to abort.
> >    */

> > [...]


> > Perhaps the following adjustment to the doc string could be considered
> > (to be applied on top of the latest patch). It copies the beginning of
> > abort-recursive-edit's doc string and removes the link to it.

> > [...]

> I realized that my proposals are kind of scattered over multiple e-mails
> in this thread. So I'm sending them again, attached in this mail as
> complete patches that apply to current master.

> The first patch refactors abort-minibuffers
> (and improves minibuffer-quit-recursive-edit's doc string as requested).

> The second patch improves documentation of recursive editing.

> Best regards.


> From fd5be298c67157822b35cd0231c65691c48dc29e 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 | 20 ++++++++++++--------
>  src/eval.c         | 22 ----------------------
>  src/lisp.h         |  1 -
>  src/minibuf.c      |  9 +++++++--
>  4 files changed, 19 insertions(+), 33 deletions(-)

> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 9668e7c732..b5c0054a3c 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -2349,14 +2349,18 @@ minibuffer-restore-windows
>  
>  (add-hook 'minibuffer-exit-hook 'minibuffer-restore-windows)
>  
> -(defun minibuffer-quit-recursive-edit ()
> -  "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))))
> +(defun minibuffer-quit-recursive-edit (&optional levels)
> +  "Quit the command that requested this recursive edit or minibuffer input.
> +Do so without terminating keyboard macro recording or 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 7bfc69b647..4e6298a101 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -4113,7 +4113,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 0e7baf30dc..a4219d2a63 100644
> --- a/src/minibuf.c
> +++ b/src/minibuf.c
> @@ -491,8 +491,13 @@ 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);
> +	  /* Due to the above check, the current minibuffer is in the
> +	     most nested command loop, which means that we don't have
> +	     to abort any extra non-minibuffer recursive edits.  Thus,
> +	     the number of recursive edits we have to abort equals the
> +	     number of minibuffers we have to abort.  */
> +	  CALLN (Ffuncall, intern ("minibuffer-quit-recursive-edit"),
> +		 array[1]);
>  	}
>      }
>    else
> -- 
> 2.33.0


> From 387baa2b42e8220b0aac36f6f23ec3547ad0811e 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 ddd74d1245..3425880fec 100644
> --- a/doc/lispref/commands.texi
> +++ b/doc/lispref/commands.texi
> @@ -3585,17 +3585,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 63bf29a948..2d97429ade 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.33.0








^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#49700: 27.2; [PATCH] Refactor minibuffer aborting
  2021-09-19 19:30           ` Alan Mackenzie
@ 2021-09-20  6:01             ` Lars Ingebrigtsen
  0 siblings, 0 replies; 15+ messages in thread
From: Lars Ingebrigtsen @ 2021-09-20  6:01 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: miha, 49700

Alan Mackenzie <acm@muc.de> writes:

> Apologies for loosing track of this thread a few weeks ago.
>
> I spent quite some time trying to find things wrong with your latest
> patch, but couldn't.  ;-)  So although I haven't fully tested it, I would
> be in favour of merging it to master.

Thanks for doing the code review.  I've applied the patch and tested it
here, and it doesn't seem to lead to any regressions here, so I went
ahead and pushed the two patches to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-09-20  6:01 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 23:05 bug#49700: 27.2; [PATCH] Refactor minibuffer aborting miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-07-23  5:42 ` Eli Zaretskii
2021-07-23  7:26   ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-07-23  7:32     ` Eli Zaretskii
2021-07-23  8:34       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-07-23 10:31         ` Eli Zaretskii
2021-07-23 11:13           ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-07-23 11:41             ` Eli Zaretskii
2021-07-23 21:03 ` Alan Mackenzie
     [not found] ` <YPsnLZa5vmDYIpxX@ACM>
2021-08-01  1:23   ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
     [not found]   ` <87im0qrmxe.fsf@miha-pc>
2021-08-06 20:14     ` Alan Mackenzie
     [not found]     ` <YQ2XHG6k6olofEb/@ACM>
2021-08-06 22:45       ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-17 21:47         ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-19 19:30           ` Alan Mackenzie
2021-09-20  6:01             ` Lars Ingebrigtsen

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).