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

Previous Next

Full log

View this message in rfc822 format

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading query-replace string, etc.
Date: Sat, 09 Apr 2022 13:06:25 +0200

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

On Fri,  8 Apr 2022 at 10:32, Juri Linkov <juri <at> 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 (text/x-patch, attachment)]

[0002-Add-lazy-highlight-when-reading-query-replace-argume.patch (text/x-patch, attachment)]

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

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

Previous Next

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