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

[Alan Mackenzie <acm@muc.de>]

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