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

[Augusto Stoffel]

On Thu, 17 Mar 2022 at 19:09, Juri Linkov <juri@linkov.net> wrote:

>> My idea is:
>>
>> - The users of the feature are Elisp programmers / package authors.
>> - I don't think end users can meaningfully do anything directly with
>>   this new minibuffer hook.
>> - If package X wants to take advantage of the feature, then it will
>>   either add minibuffer-lazy-highlight-setup to the
>>   minibuffer-setup-hook unconditionally, or it will define an
>>   X-lazy-highlight customization option to control this.
>>
>> So I think the conclusion is that the current approach in my patch is an
>> good way to proceed here?
>
> So do you think end users should not be able to decide where they want
> to use this feature?  For example, if one user will want it for 'occur',
> but another user doesn't want it in the 'occur' regexp-reading minibuffer,
> they should have no choice?

Of course there should an option, maybe occur-lazy-highlight.  This is
why isearch.el itself should _not_ contain a customization option called
`minibuffer-lazy-highlight': each use (or group of uses) of this
functionality should define their own customization option.

>>>>> BTW, what is the relation between the minibuffer-lazy-highlight feature
>>>>> and another proposed feature that immediately updates the search in
>>>>> the buffer while editing the string in the minibuffer by 
>>>>> isearch-edit-string?
>>>>> Can minibuffer-lazy-highlight be considered as a lightweight version of
>>>>> the buffer search from the minibuffer?
>>>>
>>>> Well, there's a package for that on ELPA (isearch-mb), so extending
>>>> isearch-edit-string to do that seems superfluous now?
>>>
>>> It's still possible to add this feature to isearch-edit-string,
>>> when the change would not be too enormous.  I recall squeezing
>>> it into a small patch, but unfortunately it requires changes
>>> in keymap priorities.
>>
>> I would suggest taking a look at isearch-mb.  I think the code is pretty
>> tight, and I would be unable to shorten the implementation other than by
>> deleting comment :-)
>
> I already looked at isearch-mb.  Adding the same to isearch.el
> will take only 52 lines.

Okay, I can give my opinion about these changes if you wish.

>>>>>> There are a few more we could add   (perhaps later),
>>>>>> such as `occur' and `keep-lines'.
>>>>>
>>>>> I tried (add-hook 'minibuffer-setup-hook 
>>>>> #'minibuffer-lazy-highlight-setup)
>>>>> in the minibuffer of 'occur' and others, and it works nicely.
>>>>> Maybe it could even semi-deprecate the package re-builder.el.
>>>>>
>>>>> Thanks for this generally usable feature.
>>>>
>>>> By the way, this is a byproduct of that long discussion that led to
>>>> isearch-mb, so it was not all in vain :-).
>>>
>>> Are you sure these features can't be combined?  One feature basically
>>> runs isearch-search-and-update in the buffer from the minibuffer,
>>> and this feature runs isearch-lazy-highlight-new-loop.
>>
>> For one thing, isearch-mode has 2 essential commands (repeat forward and
>> backwards), a couple more necessary ones (quit, abort, scroll,
>> beginning/end of buffer, mode toggles), and then a number of commands
>> that end the search with a special action (query-replace, etc.).
>>
>> These little details add up to the 283 lines in isearch-mb.el currently
>> has.
>
> I wonder how this is affected by scroll, beginning/end of buffer, mode 
> toggles?
> These commands don't use the minibuffer.

I probably misunderstood you then.  I thought you wanted to allow C-s
and C-r in isearch-edit-string to go back and forth in the search
buffer, but apparently this is not the case.  (Then I need to see your
52-line patch to understand what exactly you mean...)

>>>>>> - There's no customization variable to enable the minibuffer lazy
>>>>>>   highlight.  The rationale is that each command that will use it should
>>>>>>   define its own user option (or use an existing one).  For
>>>>>>   `isearch-edit-string' it's `isearch-lazy-highlight'; for
>>>>>>   `query-replace' it's `query-replace-lazy-highlight'; and so on.
>>>>>
>>>>> A common customizable option to enable this everywhere would be nice too.
>>>>> Maybe disabling is already possible by customizing
>>>>> 'minibuffer-lazy-count-format' to nil?  Then the checks for
>>>>> non-nil 'minibuffer-lazy-count-format' could be added to
>>>>> more places, such as to wrap the whole '(condition-case error'
>>>>> in query-replace-read-args with the 'when' condition, etc.
>>>>
>>>> Yes, the user can set minibuffer-lazy-count-format to nil to get rid of
>>>> the lazy count.
>>>>
>>>> Concerning query-replace, why would anyone want to have lazy highlight
>>>> during the perform-replace loop, but not earlier?  I'm not a fan of
>>>> adding a custom option here, not because it would be hard, but because
>>>> it seems totally unnecessary.
>>>
>>> Maybe a new option would make sense for the same reason why there is
>>> the option isearch-lazy-count?
>>
>> Okay, I'm not against this, but let's think about the names of these user
>> options.  The existing option is named query-replace-lazy-highlight,
>> which seems to exactly describe the new feature.  The existing feature
>> would more specifically be called perform-replace-lazy highlight.
>
> Do you mean lazy-highlight in the minibuffer that reads a string to replace?
> Then it could be named query-replace-read-lazy-highlight.

I personally find it a bit unnecessary to have separate options for
query-replace-read-lazy-highlight and query-replace-lazy-highlight.  Of
course it's nice to have fine-grained control, but at some point it just
becomes too difficult to configure things.  But I don't mind adding it.