bug#66394: 29.1; Make register-read-with-preview more useful

[Stefan Monnier]

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