GNU bug report logs - #66394
29.1; Make register-read-with-preview more useful

Previous Next

Full log

Message #309 received at 66394 <at> debbugs.gnu.org (full text, mbox):

From: Thierry Volpiatto <thievol <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: michael_heerdegen <at> web.de, dmitry <at> gutov.dev, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefankangas <at> gmail.com>, 66394 <at> debbugs.gnu.org
Subject: Re: bug#66394: 29.1; Make register-read-with-preview more useful
Date: Fri, 15 Dec 2023 18:36:26 +0000

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> @@ -429,7 +429,12 @@ 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."
>> -  (funcall register--read-with-preview-function prompt))
>> +  (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)))
>
> Questions/comments:
>
> - 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.

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

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

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. 

(cl-defmethod register-command-info ((_command (eql increment-register)))
  (make-register-preview-info
   :types '(all)
   :msg "Increment to register `%s'"
   :act 'set
   :noconfirm nil))


Thanks Stefan for reviewing this.

-- 
Thierry

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.