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

[miha]

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

signature.asc
Description: PGP signature