From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#49700: 27.2; [PATCH] Refactor minibuffer aborting Date: Fri, 6 Aug 2021 20:14:51 +0000 Message-ID: References: <87pmvaar0a.fsf@miha-pc> <87im0qrmxe.fsf@miha-pc> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="9077"; mail-complaints-to="usenet@ciao.gmane.io" To: 49700@debbugs.gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Aug 06 22:16:10 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mC6GI-0002AW-Pg for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 06 Aug 2021 22:16:10 +0200 Original-Received: from localhost ([::1]:36240 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mC6GH-0006tF-C9 for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 06 Aug 2021 16:16:09 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:59292) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mC6GA-0006t7-C7 for bug-gnu-emacs@gnu.org; Fri, 06 Aug 2021 16:16:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:39870) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mC6GA-0007Kl-3D for bug-gnu-emacs@gnu.org; Fri, 06 Aug 2021 16:16:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mC6G9-00049E-T2 for bug-gnu-emacs@gnu.org; Fri, 06 Aug 2021 16:16:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 06 Aug 2021 20:16:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 49700 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 49700-submit@debbugs.gnu.org id=B49700.162828090315872 (code B ref 49700); Fri, 06 Aug 2021 20:16:01 +0000 Original-Received: (at 49700) by debbugs.gnu.org; 6 Aug 2021 20:15:03 +0000 Original-Received: from localhost ([127.0.0.1]:51416 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mC6FD-00047v-2p for submit@debbugs.gnu.org; Fri, 06 Aug 2021 16:15:03 -0400 Original-Received: from colin.muc.de ([193.149.48.1]:44433 helo=mail.muc.de) by debbugs.gnu.org with smtp (Exim 4.84_2) (envelope-from ) id 1mC6F8-000477-ND for 49700@debbugs.gnu.org; Fri, 06 Aug 2021 16:15:02 -0400 Original-Received: (qmail 73990 invoked by uid 3782); 6 Aug 2021 20:14:51 -0000 Original-Received: from acm.muc.de (p4fe156b7.dip0.t-ipconnect.de [79.225.86.183]) (using STARTTLS) by colin.muc.de (tmda-ofmipd) with ESMTP; Fri, 06 Aug 2021 22:14:51 +0200 Original-Received: (qmail 3133 invoked by uid 1000); 6 Aug 2021 20:14:51 -0000 Content-Disposition: inline In-Reply-To: <87im0qrmxe.fsf@miha-pc> X-Submission-Agent: TMDA/1.3.x (Ph3nix) X-Primary-Address: acm@muc.de X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:211350 Archived-At: Hello again, Miha. On Sun, Aug 01, 2021 at 03:09:01 +0200, miha@kamnitnik.top wrote: > Alan Mackenzie 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).