bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading query-replace string, etc.

[Augusto Stoffel]

On Fri,  8 Apr 2022 at 10:32, Juri Linkov <juri@linkov.net> wrote:

>>> Looks good.
>>
>> Okay, I've refactored my code like this.  I actually like it better that
>> way.  (As a downside, the stuff that was already merged to isearch.el is
>> completely changed.)
>
> Thanks, now it looks much better!

Please find attached the patches.

0001-Rewrite-the-minibuffer-lazy-highlight-feature.patch
Description: Text Data

0002-Add-lazy-highlight-when-reading-query-replace-argume.patch
Description: Text Data

> In `isearch-edit-string' you replaced:
>   (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
> with
>   (minibuffer-with-setup-hook (minibuffer-lazy-highlight-setup)
> but the docstring of `minibuffer-lazy-highlight-setup' in your patch is:
>
>   This function return a closure intended to be added to
>   `minibuffer-setup-hook'.
>
> Maybe either a typo that needs to mention `minibuffer-with-setup-hook',
> or `minibuffer-setup-hook' needs to evaluate such closure with something like
>
>   (add-hook 'minibuffer-setup-hook (funcall 'minibuffer-lazy-highlight-setup))
>
> But since this is more ugly, then using `minibuffer-with-setup-hook' is fine.
>

Well, as a matter of fact the closure is meant to run in the
'minibuffer-setup-hook'.  This is not to say it's a good idea to use
'add-hook' to do add it there.  One should always use
'minibuffer-with-setup-hook' for that kind of stuff, I guess.

Feel free to copy edit the docstring, but in light of the above I didn't
change it for now.

>>> Shouldn't both cases clean up highlight from the buffer?
>>> Then I see no need to distinguish each case.  Or if really needed,
>>> you can try to bind the cleanup to command-error-function.
>>
>> My previous patch had only one case: if the user quits, we clean up the
>> highlighting.
>>
>> I can only see one simpler alternative, which is to always
>> unconditionally clean up the highlight.  This is not as nice, but if
>> keeping the code as simple as possible is important here, then I guess
>> this is the way forward.  So that's what the current patch does.
>>
>> I suspect people will see this as a bug, but maybe discussing this issue
>> by itself later will be easier.
>
> Yep, let's do this later when people will ask for it.

All right, I've removed all the "clever business" related to delayed
clean up of the lazy highlight for now.

>>> 4 lines look nice, unlike 20 lines in one of your patches ;-)
>>
>> When you add all the bells and whistles, 4 lines just won't do it.
>
> Now the parameters of minibuffer-lazy-highlight-setup look nice,
> so line count doesn't matter when code keeps simplicity.

So, my sample code from the previous message defined a function for this
snippet which already repeats 3 times

   (or replace-regexp-function
     delimited-flag
     (and replace-char-fold
          (not regexp-flag)
          #'char-fold-to-regexp))

I've removed that function for now.  We can reintroduce the function if
you deem it helpful.