From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: miha--- via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#49700: 27.2; [PATCH] Refactor minibuffer aborting Date: Sat, 07 Aug 2021 00:45:28 +0200 Message-ID: <87eeb6b3av.fsf@miha-pc> References: <87pmvaar0a.fsf@miha-pc> <87im0qrmxe.fsf@miha-pc> Reply-To: Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="38503"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Eli Zaretskii , 49700@debbugs.gnu.org To: Alan Mackenzie Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Aug 07 00:44:11 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 1mC8ZW-0009ld-BQ for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 07 Aug 2021 00:44:10 +0200 Original-Received: from localhost ([::1]:55100 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mC8ZU-0002JF-Oz for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 06 Aug 2021 18:44:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:51474) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mC8ZO-0002J5-RD for bug-gnu-emacs@gnu.org; Fri, 06 Aug 2021 18:44:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:39951) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mC8ZO-0007mg-Jy for bug-gnu-emacs@gnu.org; Fri, 06 Aug 2021 18:44:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mC8ZO-0001Ju-C0 for bug-gnu-emacs@gnu.org; Fri, 06 Aug 2021 18:44:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 06 Aug 2021 22:44:02 +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.16282897925010 (code B ref 49700); Fri, 06 Aug 2021 22:44:02 +0000 Original-Received: (at 49700) by debbugs.gnu.org; 6 Aug 2021 22:43:12 +0000 Original-Received: from localhost ([127.0.0.1]:51497 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mC8Ya-0001Ik-7o for submit@debbugs.gnu.org; Fri, 06 Aug 2021 18:43:12 -0400 Original-Received: from kamnitnik.top ([209.250.245.214]:39086 helo=mail.kamnitnik.top) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mC8YW-0001IZ-CT for 49700@debbugs.gnu.org; Fri, 06 Aug 2021 18:43:10 -0400 Original-Received: from localhost (unknown [IPv6:2a00:ee2:e04:9300:e609:6c46:d026:8c47]) by mail.kamnitnik.top (Postfix) with ESMTPSA id CC1BDBCF6B; Fri, 6 Aug 2021 22:43:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kamnitnik.top; s=mail; t=1628289786; bh=eQFqXZk60gJYb5+np0mSNZMgW8Pu+uv35T9zmg6+rw4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Q373kTZPjgB6nM9sOK0PU0Gc7TJRbAqmuebcQ9oKrF1YEJuzTYXeKowXdr8D5O/97 iiKhmnOjUvk5VwnSoPwLpCeignd4YN+77BEv9jMBTnSn1hGHll4x3oq3LzS8yhd/6V LYHIuQlHQii6aDytcNFnbA43jdw+7gb+luIgpxEGvWCEC8BL+bAV1uG03u8pWmEJEL Z+5VlMWUy75jkFNYnI2h7qJAUStjh4tt9ECT9S7/C84WSGHGcx0R9nj6XgPGbD0XfI EjUZ5zi5P3HxuGiRWcLdYK2UE8850axfRoLlmwq/9fFuMMOAAloI2Q3b4wB2TN5UWq zZnjccEmp3YHg== In-Reply-To: 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:211354 Archived-At: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Alan Mackenzie writes: > 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 comm= it >> >> 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 wr= ote >> > 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 minibuff= er >> > 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 s= ame >> > most of the time. When (recursive-edit) gets called outside of the >> > minibuffer code, then these two values are different. For example, in= =20 >> > 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 =2D-- a/lisp/minibuffer.el +++ b/lisp/minibuffer.el @@ -2331,6 +2331,6 @@ exit-minibuffer (defun minibuffer-quit-recursive-edit (&optional levels) =2D "Quit the command that requested this recursive edit without error. =2DLike `abort-recursive-edit' without aborting keyboard macro =2Dexecution. LEVELS specifies the number of nested recursive edits =2Dto 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), a= nd >> > 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 f= or >> > 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! > > [ .... ] > > --=20 > Alan Mackenzie (Nuremberg, Germany). --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQJHBAEBCAAxFiEEmxVnesoT5rQXvVXnswkaGpIVmT8FAmENu4gTHG1paGFAa2Ft bml0bmlrLnRvcAAKCRCzCRoakhWZPxtAD/934oox+/4txErHw/j5BcUEHtbU0RQ0 wC8g+ui5q+MuwyOYSEJr/RdEqq9aEskiKGOsygsaxg+2Mxo19I3spIGrosMny5cB m0/fy6DQETOrn7G1O4aBU7mCvuNfoPKEZdoCjCuEz3n1j2a93baH9wIUKs9vODnL xHCtoBO9z3WzC1dnDZY1U+zVfsRR/UbLsj9b7qCp9bdYqyIpFphKFo04UNyfcSsk KlN8BXHpEnfHnRp0noWxdbjoRiiaIL5z3/564dQnyGI1A48KL3ZGYvRJ/Vw4++Lm y6kZouiCuS1J3BrRbFgjq7B5C69eXaeZPTa3znrRTC/5w3PxN19wNp/pnclJy+QO bTePTc3clFQ4has6g8nLgaot7GZICya0GknpBRAo2oKkwvaZ1sld6mnxKLtZdt+H paVL8S5g4iWS9sgREvaFuCkOOpdIlz8dTBcSeZVv4phlhPa9To3sLxpXxEDy6zFr 6fgkwkihkrybi+hGh5GJ0tKwV1tnV7SB8ZJCRQdQPMEV1PaFEOqi7iEvEMetl7b8 3mZ5Eh94CdhQ2xGMHcHE3tOymWUtsBCCduIkCr0BxAg5DDBDOj6pSIYnplrsSpDx 8THhCD91FE3O6U/I4t9UfzYPppICc8nNdZTJB22RdksLvD09brMCdQjByFvhzFoU tR2QCj4YhYZRKw== =U9jN -----END PGP SIGNATURE----- --=-=-=--