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

[Thierry Volpiatto]

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

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

Done.

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

It should be possible to use post-command-hook, I didn't use it because
it makes harder the communication between the minibuffer and the preview
buffer.


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

Ok, will add this in next patch.

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

Anyway your comments were useful, as always, thanks.

-- 
Thierry

signature.asc
Description: PGP signature