From mboxrd@z Thu Jan  1 00:00:00 1970
Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail
From: Stefan Monnier via "Bug reports for GNU Emacs,
 the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
Newsgroups: gmane.emacs.bugs
Subject: bug#66394: 29.1; Make register-read-with-preview more useful
Date: Fri, 15 Dec 2023 18:30:56 -0500
Message-ID: <jwvplz73vpg.fsf-monnier+emacs@gnu.org>
References: <87il7ib6cu.fsf@posteo.net> <8734wov2wv.fsf@posteo.net>
 <83v89j6arv.fsf@gnu.org> <87cyvpf8y6.fsf@posteo.net>
 <83plzp82mb.fsf@gnu.org> <87a5qhxf05.fsf@posteo.net>
 <83jzpkvs4z.fsf@gnu.org> <87v8947ulo.fsf@posteo.net>
 <871qbsk5le.fsf@posteo.net>
 <f7f749d1-3a2d-0ea0-a106-6c586f4faca7@gutov.dev>
 <87v894hr2e.fsf@posteo.net> <87cyvbepi0.fsf@posteo.net>
 <87bkavk9nv.fsf@posteo.net>
 <8eebbb30-9366-e869-a39a-8100638cb99a@gutov.dev>
 <87o7etgxeb.fsf@posteo.net> <83v891qlcn.fsf@gnu.org>
 <jwvfs04hjm9.fsf-monnier+emacs@gnu.org>
 <CADwFkmm3H9+-nrYi0JQM9YT0_ZYU3SYvcqfmEnEpJWWaZzHTGQ@mail.gmail.com>
 <87r0jn4j8i.fsf@posteo.net> <jwv8r5vfqo6.fsf-monnier+emacs@gnu.org>
 <87jzpf48k5.fsf@posteo.net>
Reply-To: Stefan Monnier <monnier@iro.umontreal.ca>
Mime-Version: 1.0
Content-Type: text/plain
Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214";
	logging-data="40522"; mail-complaints-to="usenet@ciao.gmane.io"
User-Agent: Gnus/5.13 (Gnus v5.13)
Cc: michael_heerdegen@web.de, dmitry@gutov.dev, Eli Zaretskii <eliz@gnu.org>,
 Stefan Kangas <stefankangas@gmail.com>, 66394@debbugs.gnu.org
To: Thierry Volpiatto <thievol@posteo.net>
Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Dec 16 00:32:25 2023
Return-path: <bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org>
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 <bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org>)
	id 1rEHfM-000AFw-Sp
	for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 16 Dec 2023 00:32:25 +0100
Original-Received: from localhost ([::1] helo=lists1p.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.90_1)
	(envelope-from <bug-gnu-emacs-bounces@gnu.org>)
	id 1rEHf1-00022h-Kp; Fri, 15 Dec 2023 18:32:03 -0500
Original-Received: from eggs.gnu.org ([2001:470:142:3::10])
 by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.90_1) (envelope-from <Debian-debbugs@debbugs.gnu.org>)
 id 1rEHf0-00020p-Cc
 for bug-gnu-emacs@gnu.org; Fri, 15 Dec 2023 18:32:02 -0500
Original-Received: from debbugs.gnu.org ([2001:470:142:5::43])
 by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128)
 (Exim 4.90_1) (envelope-from <Debian-debbugs@debbugs.gnu.org>)
 id 1rEHf0-0005Nv-45
 for bug-gnu-emacs@gnu.org; Fri, 15 Dec 2023 18:32:02 -0500
Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2)
 (envelope-from <Debian-debbugs@debbugs.gnu.org>) id 1rEHez-0007qk-VL
 for bug-gnu-emacs@gnu.org; Fri, 15 Dec 2023 18:32:01 -0500
X-Loop: help-debbugs@gnu.org
Resent-From: Stefan Monnier <monnier@iro.umontreal.ca>
Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
Resent-CC: bug-gnu-emacs@gnu.org
Resent-Date: Fri, 15 Dec 2023 23:32:01 +0000
Resent-Message-ID: <handler.66394.B66394.170268308230118@debbugs.gnu.org>
Resent-Sender: help-debbugs@gnu.org
X-GNU-PR-Message: followup 66394
X-GNU-PR-Package: emacs
Original-Received: via spool by 66394-submit@debbugs.gnu.org id=B66394.170268308230118
 (code B ref 66394); Fri, 15 Dec 2023 23:32:01 +0000
Original-Received: (at 66394) by debbugs.gnu.org; 15 Dec 2023 23:31:22 +0000
Original-Received: from localhost ([127.0.0.1]:53745 helo=debbugs.gnu.org)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
 id 1rEHe7-0007jO-9s
 for submit@debbugs.gnu.org; Fri, 15 Dec 2023 18:31:22 -0500
Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:62546)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <monnier@iro.umontreal.ca>) id 1rEHe4-0007Vm-Hr
 for 66394@debbugs.gnu.org; Fri, 15 Dec 2023 18:31:05 -0500
Original-Received: from pmg2.iro.umontreal.ca (localhost.localdomain [127.0.0.1])
 by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 84E6A8027C;
 Fri, 15 Dec 2023 18:30:58 -0500 (EST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca;
 s=mail; t=1702683057;
 bh=Nm7YdMYx/9zg4M0I6lvoEPhcLraEuOUwqYq4iUARAb0=;
 h=From:To:Cc:Subject:In-Reply-To:References:Date:From;
 b=mFJtbSUlLt5SWuKN+fa7kdjwSH06+UyEzv6QRQtmZfMHiz78E0a4CvSMvn6xSwgGF
 Dfv+pgUE/2KJGgWheGkUazIKZFAsyHARttzkjcpdOvU6OF75Srxc2gpoVQVMWfZ6/3
 PLIX1KscKUXARRoFW8Djr5Cx6tqMNr2OnruQTBAjIDzRA1fGKeYW8f1vdRwYApiz7d
 zKNtMDnGm441pL8kxYvZdDZyNRnf9KuOB7P+WHsDqEk5KgEZlOqnwpUSZ6YLhvkPdR
 wErmiF+kjj7FaRfgX49BQCBYO5XS1XHOAb6loJrLTmIpReNzESFZAzP0gC70I+4EUj
 F6r6CyfcFAUNg==
Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1])
 by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 16DD080AE9;
 Fri, 15 Dec 2023 18:30:57 -0500 (EST)
Original-Received: from pastel (65-110-221-238.cpe.pppoe.ca [65.110.221.238])
 by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id D224F1202EA;
 Fri, 15 Dec 2023 18:30:56 -0500 (EST)
In-Reply-To: <87jzpf48k5.fsf@posteo.net> (Thierry Volpiatto's message of "Fri, 
 15 Dec 2023 18:36:26 +0000")
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" <bug-gnu-emacs.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/bug-gnu-emacs>,
 <mailto:bug-gnu-emacs-request@gnu.org?subject=unsubscribe>
List-Archive: <https://lists.gnu.org/archive/html/bug-gnu-emacs>
List-Post: <mailto:bug-gnu-emacs@gnu.org>
List-Help: <mailto:bug-gnu-emacs-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/bug-gnu-emacs>,
 <mailto:bug-gnu-emacs-request@gnu.org?subject=subscribe>
Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org
Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org
Xref: news.gmane.io gmane.emacs.bugs:276294
Archived-At: <http://permalink.gmane.org/gmane.emacs.bugs/276294>

>> - Why did you change from using
>>   `register--read-with-preview-function` to using
>>   (default-value 'register--read-with-preview-function) ?
>>   [ The answer should presumably be in the commit message but
>>     I couldn't find it there.  ]
>>
>> - Why let-bind `register--read-with-preview-function`
>>   rather than using a local lexical var?
>>   [ The answer should probably be in a comment in the code.  ]
>
> To answer to your 1) and 2) questions, I guess what you
> suggest is something like this (indeed better):
>
>     diff --git a/lisp/register.el b/lisp/register.el
>     index 15ed5c0a53b..2444f88794e 100644
>     --- a/lisp/register.el
>     +++ b/lisp/register.el
>     @@ -429,12 +429,11 @@ Format of each entry is controlled by the variable `register-preview-function'."
>      Prompt with the string PROMPT.
>      If `help-char' (or a member of `help-event-list') is pressed,
>      display such a window regardless."
>     -  (let ((register--read-with-preview-function
>     -         (if (and executing-kbd-macro
>     -                  (memq register-use-preview '(nil never)))
>     -             #'register-read-with-preview-basic
>     -           (default-value 'register--read-with-preview-function))))
>     -    (funcall register--read-with-preview-function prompt)))
>     +  (let ((fn (if (and executing-kbd-macro
>     +                     (memq register-use-preview '(nil never)))
>     +                #'register-read-with-preview-basic
>     +              register--read-with-preview-function)))
>     +    (funcall fn prompt)))
>      
>      (defun register-read-with-preview-basic (prompt)
>        "Read and return a register name, possibly showing existing registers.

LGTM, thanks.

>> - Making the behavior dependent on `executing-kbd-macro` is generally
>>   undesirable, so it should be accompanied with a comment in the code
>>   explaining why we need it (with enough detail that someone
>>   sufficiently motivated could potentially "fix" the code to remove this
>>   dependency, or alternatively to convince that someone else that this
>>   dependency is actually desirable here).
>
> The explanation is in the commit message.

Then please move it into a comment in the code.

> To resume, when we are not
> using RET to exit minibuffer, we use `exit-minibuffer` from the timer
> function in minibuffer-setup-hook, BTW when you have a macro using
> e.g. "C-x r i, C-n, C-a, C-x r +", "C-n and C-a" are running
> immediately BEFORE the minibuffer is exited so they run in minibuffer
> and have no effect in your macro that run in current-buffer.
> Is such a comment sufficiently explicit? (will add in next patch if so).

Sounds good, yes.

> If you have a better fix for this I take ;-).

I haven't looked enough at the code to have a better suggestion.
>From what I see the only way to have a "better fix" would be to forego
the use of asynchronous code (i.e. of a timer).  I don't know why you're
using a timer, but usually people don't use timer just because they're
pretty, so I assume you've considered other options already (such as
using an `after-change-function` or a `post-command-hook` instead of
a timer).

[ FWIW, I suspect we have a similar problem in `read-key`.
  Maybe that's the reason why people refrain from using `read-key`?
  I can't see any comment in `read-key` mentioning that it doesn't work
  in kmacros, but indeed it uses a timer just like you do, so it
  probably fails in the exact same way.  ]

> The problem with such a fix (as I did) is that we can't have an hybrid
> version of preview i.e. one that use RET to confirm overwrite and no RET
> for other things.
> For example if one add a configuration like below to modify behavior
> with *-use-preview == nil the macro will fail to execute properly.

You might want to add a short hint about that problem in the comment.

> Thanks Stefan for reviewing this.

I looked only at the kmacro part (because I thought maybe it had to do
with kmacro's use of OClosures), sorry.


        Stefan