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

Previous Next

Package: emacs;

Reported by: Augusto Stoffel <arstoffel <at> gmail.com>

Date: Sat, 8 Jan 2022 13:25:01 UTC

Severity: normal

Tags: patch

Merged with 53341

Fixed in version 29.0.50

Done: Juri Linkov <juri <at> linkov.net>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 53126 in the body.
You can then email your comments to 53126 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sat, 08 Jan 2022 13:25:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Augusto Stoffel <arstoffel <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sat, 08 Jan 2022 13:25:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Lazy highlight/count when reading query-replace
 string, etc.
Date: Sat, 08 Jan 2022 14:24:33 +0100
[Message part 1 (text/plain, inline)]
The anzu package was mentioned recently in the mailing list.  Emacs now
includes the lazy-count feature, which is the main purpose of anzu, but
it's still missing one nice feature from that package, namely the
highlighting and count of matches as one types a regexp or string to
replace in `query-replace{-regexp}'.  See the below patch for a
(probably not yet mature) stab at this.

I've also added lazy highlighting to the `isearch-edit-string' command.

I didn't introduce any new customization variables.  Lazy highlighting
in both `query-replace{-regexp}' and `isearch-edit-string' is controlled
by `isearch-lazy-highlight' (therefore is on by default) and lazy count
next to the minibuffer prompt is controlled by `isearch-lazy-count'
(therefore off by default).

I believe the problems pointed out in a previous iteration of this in
https://lists.gnu.org/archive/html/emacs-devel/2021-05/msg01196.html are
now solved.

[0001-Allow-reading-from-minibuffer-with-lazy-highlight-an.patch (text/x-patch, attachment)]
[0002-Lazy-highlight-when-reading-query-replace-text.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sat, 08 Jan 2022 19:11:02 GMT) Full text and rfc822 format available.

Message #8 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sat, 08 Jan 2022 20:59:45 +0200
> The anzu package was mentioned recently in the mailing list.  Emacs now
> includes the lazy-count feature, which is the main purpose of anzu, but
> it's still missing one nice feature from that package, namely the
> highlighting and count of matches as one types a regexp or string to
> replace in `query-replace{-regexp}'.  See the below patch for a
> (probably not yet mature) stab at this.

Strange, I expected that the lazy-count feature applied to query-replace
should do a different thing - when querying about replacing the current match,
it should show the number of current match and the number of total matches.
The latter number should be updated after every replacement.

But your patches are intended for a different feature - highlighting
of matches in the buffer while entering an input string in the minibuffer.

I wonder how many users need this feature, when it's easy to construct
a query-replace string using highlighting/counting in isearch-mode, then type M-%
(isearch-query-replace) that invokes query-replace with the query-replace string.

Perhaps better to ask on emacs-devel if anyone needs this feature.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sat, 08 Jan 2022 19:36:02 GMT) Full text and rfc822 format available.

Message #11 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sat, 08 Jan 2022 20:35:36 +0100
On Sat,  8 Jan 2022 at 20:59, Juri Linkov <juri <at> linkov.net> wrote:

> Strange, I expected that the lazy-count feature applied to query-replace
> should do a different thing - when querying about replacing the current match,
> it should show the number of current match and the number of total matches.
> The latter number should be updated after every replacement.
>

That would be handy too.  I haven't checked how the `perform-replace'
loop works, but wouldn't this require a hook into the lazy count
procedure --- precisely what my patch introduces through the new
variable `isearch-lazy-count-display-function'?

> But your patches are intended for a different feature - highlighting
> of matches in the buffer while entering an input string in the minibuffer.
>
> I wonder how many users need this feature, when it's easy to construct
> a query-replace string using highlighting/counting in isearch-mode,
> then type M-%
> (isearch-query-replace) that invokes query-replace with the
> query-replace string.
>

Sure, this alternative method works.  But somehow it's not the way I
usually start a replace, and I think there's nothing wrong with that
preference :-)

> Perhaps better to ask on emacs-devel if anyone needs this feature.

We can ask.  But note that the bulk of the patch has a fairly general
purpose, namely to read a regexp with live preview of the matches.
There's probably a couple extra uses for this.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 09 Jan 2022 09:30:02 GMT) Full text and rfc822 format available.

Message #14 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 09 Jan 2022 11:10:13 +0200
>> Strange, I expected that the lazy-count feature applied to query-replace
>> should do a different thing - when querying about replacing the current match,
>> it should show the number of current match and the number of total matches.
>> The latter number should be updated after every replacement.
>
> That would be handy too.  I haven't checked how the `perform-replace'
> loop works, but wouldn't this require a hook into the lazy count
> procedure --- precisely what my patch introduces through the new
> variable `isearch-lazy-count-display-function'?

It seems the "Query replacing" prompt in perform-replace could just
display the values of isearch-lazy-count-current and
isearch-lazy-count-total.  Or for asynchronous lazy-count a new hook
needs to be run at the end of isearch-lazy-highlight-buffer-update
in addition to the call of isearch-message.

>> But your patches are intended for a different feature - highlighting
>> of matches in the buffer while entering an input string in the minibuffer.
>>
>> I wonder how many users need this feature, when it's easy to construct
>> a query-replace string using highlighting/counting in isearch-mode,
>> then type M-%
>> (isearch-query-replace) that invokes query-replace with the
>> query-replace string.
>
> Sure, this alternative method works.  But somehow it's not the way I
> usually start a replace, and I think there's nothing wrong with that
> preference :-)

But this method is more limiting - no keys to pull text from the buffer
like 'C-w' (isearch-yank-word-or-char) does in isearch-mode, no way
to navigate to the first match like it's possible in isearch-mode, etc.

>> Perhaps better to ask on emacs-devel if anyone needs this feature.
>
> We can ask.  But note that the bulk of the patch has a fairly general
> purpose, namely to read a regexp with live preview of the matches.
> There's probably a couple extra uses for this.

Maybe then this feature could be added to read-regexp or even
to read-from-minibuffer?  And activated by adding a setup function
to minibuffer-setup-hook like other minibuffer's features do, such as
icomplete-minibuffer-setup, minibuffer-depth-setup, rfn-eshadow-setup-minibuffer.

Then maybe a new feature could be named e.g. "lazy-minibuffer"?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 09 Jan 2022 10:03:01 GMT) Full text and rfc822 format available.

Message #17 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 09 Jan 2022 11:02:46 +0100
On Sun,  9 Jan 2022 at 11:10, Juri Linkov <juri <at> linkov.net> wrote:

> It seems the "Query replacing" prompt in perform-replace could just
> display the values of isearch-lazy-count-current and
> isearch-lazy-count-total.

Yes, I guess you are right.

> Or for asynchronous lazy-count a new hook needs to be run at the end
> of isearch-lazy-highlight-buffer-update in addition to the call of
> isearch-message.
>

That's what the patch does (but with a variable pointing to a function
instead of a hook, although this can be changed).  But note that it's
not enough to do something “in addition to the call of isearch-message”;
it's necessary to _suppress_ the call to isearch-message.

>>> But your patches are intended for a different feature - highlighting
>>> of matches in the buffer while entering an input string in the minibuffer.
>>>
>>> I wonder how many users need this feature, when it's easy to construct
>>> a query-replace string using highlighting/counting in isearch-mode,
>>> then type M-%
>>> (isearch-query-replace) that invokes query-replace with the
>>> query-replace string.
>>
>> Sure, this alternative method works.  But somehow it's not the way I
>> usually start a replace, and I think there's nothing wrong with that
>> preference :-)
>
> But this method is more limiting - no keys to pull text from the buffer
> like 'C-w' (isearch-yank-word-or-char) does in isearch-mode, no way
> to navigate to the first match like it's possible in isearch-mode, etc.
>

OTOH, you can't do a replace-in-region that way.

> Maybe then this feature could be added to read-regexp or even
> to read-from-minibuffer?  And activated by adding a setup function
> to minibuffer-setup-hook like other minibuffer's features do, such as
> icomplete-minibuffer-setup, minibuffer-depth-setup, rfn-eshadow-setup-minibuffer.
>
> Then maybe a new feature could be named e.g. "lazy-minibuffer"?

This is what the patch does, with code of this kind:

```
(let ((isearch-regexp t)
      ;; Whatever else isearch / lazy-highlight settings might be needed
      (isearch-lazy-count-display-function #'isearch-read-with-highlight-count))
  (minibuffer-with-setup-hook #'isearch-read-with-highlight-setup
    (read-from-minibuffer "Type a regexp with preview: ")))
```

There could be a convenience wrapper for this code, but I'm not sure
that makes much sense, because isearch and lazy-highlight have too many
parameters one might need to set so it's better to be explicit/flexible.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 09 Jan 2022 10:31:01 GMT) Full text and rfc822 format available.

Message #20 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 09 Jan 2022 11:30:31 +0100
On Sun,  9 Jan 2022 at 11:02, Augusto Stoffel <arstoffel <at> gmail.com> wrote:

> OTOH, you can't do a replace-in-region that way.

Or rather -- you can, but it's tricky to keep the desired region
unchanged.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 09 Jan 2022 19:05:02 GMT) Full text and rfc822 format available.

Message #23 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 09 Jan 2022 20:58:32 +0200
>> Or for asynchronous lazy-count a new hook needs to be run at the end
>> of isearch-lazy-highlight-buffer-update in addition to the call of
>> isearch-message.
>
> That's what the patch does (but with a variable pointing to a function
> instead of a hook, although this can be changed).  But note that it's
> not enough to do something “in addition to the call of isearch-message”;
> it's necessary to _suppress_ the call to isearch-message.

It would be great to use your new variable with a function
to show replacement counts in perform-replace.  IIUC,
let-binding isearch-lazy-count-display-function to
isearch-read-with-highlight-count will suppress isearch-message?

>> Maybe then this feature could be added to read-regexp or even
>> to read-from-minibuffer?  And activated by adding a setup function
>> to minibuffer-setup-hook like other minibuffer's features do, such as
>> icomplete-minibuffer-setup, minibuffer-depth-setup, rfn-eshadow-setup-minibuffer.
>
> This is what the patch does, with code of this kind:
>
> ```
> (let ((isearch-regexp t)
>       ;; Whatever else isearch / lazy-highlight settings might be needed
>       (isearch-lazy-count-display-function #'isearch-read-with-highlight-count))
>   (minibuffer-with-setup-hook #'isearch-read-with-highlight-setup
>     (read-from-minibuffer "Type a regexp with preview: ")))
> ```
>
> There could be a convenience wrapper for this code, but I'm not sure
> that makes much sense, because isearch and lazy-highlight have too many
> parameters one might need to set so it's better to be explicit/flexible.

I meant using simply

  (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)

But it seems isearch-read-with-highlight-setup doesn't set
isearch-lazy-count-display-function.

>> Then maybe a new feature could be named e.g. "lazy-minibuffer"?

This feature has little to do with isearch.  This is why there are
efforts to move away from the prefix isearch- for lazy-related
functions and variables, so we have now:

  lazy-count-prefix-format
  lazy-count-suffix-format
  lazy-highlight-buffer
  lazy-highlight-buffer-max-at-a-time
  lazy-highlight-cleanup
  lazy-highlight-initial-delay
  lazy-highlight-interval
  lazy-highlight-max-at-a-time
  ...

There are still isearch-specific names like isearch-lazy-count
that enables lazy-count in isearch-mode.  What would be a similar
name for the minibuffer?  Maybe minibuffer-lazy-count?

Then the prefix isearch- needs to be removed from other names too,
e.g. isearch-lazy-count-display-function -> lazy-count-display-function
isearch-read-with-highlight-setup maybe to minibuffer-lazy-highlight-setup,
etc.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Mon, 10 Jan 2022 17:35:02 GMT) Full text and rfc822 format available.

Message #26 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Mon, 10 Jan 2022 18:34:18 +0100
[Message part 1 (text/plain, inline)]
Hi Juri,

I attached a new patch (still a sketch) that requires no changes in
comint.el and simple.el.  Perhaps you will find this approach more
acceptable.

More comments below.

On Sun,  9 Jan 2022 at 20:58, Juri Linkov <juri <at> linkov.net> wrote:

> It would be great to use your new variable with a function
> to show replacement counts in perform-replace.  IIUC,
> let-binding isearch-lazy-count-display-function to
> isearch-read-with-highlight-count will suppress isearch-message?
>

I tried this and it's relatively simple to do, but there is a problem.
Suppose you want to replace all "a" with "z", and your buffer has 20
"a"s initially.  Then, as you keep hitting "y" to confirm a replacement
the count will be

   1/20, 1/19, ..., 1/1

since the number of "a"s decrease, and the point is always at the first
of the still-existing ones.  But probably one should count the number of
prompts, so

   1/20, 2/20, ..., 20/20

I think this means `perform-replace' has to implement its own way to
display a count.

> I meant using simply
>
>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>
> But it seems isearch-read-with-highlight-setup doesn't set
> isearch-lazy-count-display-function.
>

I guess this could be done.  But note that there are two possible types
of counts: a "current/total" counter or just a "total" counter.  Each
use case calls for a different count style.

>>> Then maybe a new feature could be named e.g. "lazy-minibuffer"?
>
> This feature has little to do with isearch.  This is why there are
> efforts to move away from the prefix isearch- for lazy-related
> functions and variables, so we have now:
>
>   lazy-count-prefix-format
>   lazy-count-suffix-format
>   lazy-highlight-buffer
>   lazy-highlight-buffer-max-at-a-time
>   lazy-highlight-cleanup
>   lazy-highlight-initial-delay
>   lazy-highlight-interval
>   lazy-highlight-max-at-a-time
>   ...
>
> There are still isearch-specific names like isearch-lazy-count
> that enables lazy-count in isearch-mode.  What would be a similar
> name for the minibuffer?  Maybe minibuffer-lazy-count?

I see, the lazy-* names are the new ones!

>
> Then the prefix isearch- needs to be removed from other names too,
> e.g. isearch-lazy-count-display-function -> lazy-count-display-function
> isearch-read-with-highlight-setup maybe to minibuffer-lazy-highlight-setup,
> etc.

Yes, I agree.  The names I came up with are horrible and
`lazy-minibuffer' is weird, but your current suggestion is nice.

By the way, I'm debating a bit whether
`isearch-lazy-count-display-function' should be:

1. Either nil or function, as it is right now,
2. #'ignore by default, so similar to 1) but a bit easier to use with
   `add-function'
3. a hook, the main inconvenience being that it can't be easily let-bound.

[0001-Allow-reading-from-minibuffer-with-lazy-highlight-an.patch (text/x-patch, attachment)]
[0002-Lazy-highlight-when-reading-query-replace-text.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Mon, 10 Jan 2022 19:12:01 GMT) Full text and rfc822 format available.

Message #29 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Mon, 10 Jan 2022 21:09:40 +0200
> I attached a new patch (still a sketch) that requires no changes in
> comint.el and simple.el.  Perhaps you will find this approach more
> acceptable.

Thanks, no changes in other files is certainly a big plus.

>> It would be great to use your new variable with a function
>> to show replacement counts in perform-replace.  IIUC,
>> let-binding isearch-lazy-count-display-function to
>> isearch-read-with-highlight-count will suppress isearch-message?
>
> I tried this and it's relatively simple to do, but there is a problem.
> Suppose you want to replace all "a" with "z", and your buffer has 20
> "a"s initially.  Then, as you keep hitting "y" to confirm a replacement
> the count will be
>
>    1/20, 1/19, ..., 1/1

This is an interesting question.  I tried in other editors,
and e.g. in the editor xed that is installed by default,
this is exactly what is displayed: 1/20, 1/19, ..., 1/1.

> since the number of "a"s decrease, and the point is always at the first
> of the still-existing ones.  But probably one should count the number of
> prompts, so
>
>    1/20, 2/20, ..., 20/20
>
> I think this means `perform-replace' has to implement its own way to
> display a count.

Maybe this makes more sense, when the users will learn
what do these numbers mean.

>> I meant using simply
>>
>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>
>> But it seems isearch-read-with-highlight-setup doesn't set
>> isearch-lazy-count-display-function.
>
> I guess this could be done.

Maybe two separate hooks could be defined?  One highlights like
lazy-highlight, and another counts like lazy-count does:

  (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
  (add-hook 'minibuffer-setup-hook 'isearch-read-with-count-setup)

> But note that there are two possible types
> of counts: a "current/total" counter or just a "total" counter.  Each
> use case calls for a different count style.

The format of the total counter needs to be defined in a separate variable.

> By the way, I'm debating a bit whether
> `isearch-lazy-count-display-function' should be:
>
> 1. Either nil or function, as it is right now,
> 2. #'ignore by default, so similar to 1) but a bit easier to use with
>    `add-function'
> 3. a hook, the main inconvenience being that it can't be easily let-bound.

This can be answered only by testing with all possible cases ;-)




Forcibly Merged 53126 53341. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Fri, 21 Jan 2022 11:21:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sat, 26 Feb 2022 16:14:01 GMT) Full text and rfc822 format available.

Message #34 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sat, 26 Feb 2022 17:13:31 +0100
[Message part 1 (text/plain, inline)]
Hi Juri,

Sorry for getting back to this after such a long time.  I've attached a
new patch that hopefully is good to merge, except for adding some NEWS
entry.  Let me know what you think.

Inlined below some further comments.

[0001-Display-lazy-highlight-and-match-count-in-when-readi.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
On Mon, 10 Jan 2022 at 21:09, Juri Linkov <juri <at> linkov.net> wrote:

>> I attached a new patch (still a sketch) that requires no changes in
>> comint.el and simple.el.  Perhaps you will find this approach more
>> acceptable.
>
> Thanks, no changes in other files is certainly a big plus.
>
>>> It would be great to use your new variable with a function
>>> to show replacement counts in perform-replace.  IIUC,
>>> let-binding isearch-lazy-count-display-function to
>>> isearch-read-with-highlight-count will suppress isearch-message?
>>
>> I tried this and it's relatively simple to do, but there is a problem.
>> Suppose you want to replace all "a" with "z", and your buffer has 20
>> "a"s initially.  Then, as you keep hitting "y" to confirm a replacement
>> the count will be
>>
>>    1/20, 1/19, ..., 1/1
>
> This is an interesting question.  I tried in other editors,
> and e.g. in the editor xed that is installed by default,
> this is exactly what is displayed: 1/20, 1/19, ..., 1/1.

Let's do this at a later point, to keep this patch smaller and more
focused.

Note that there is one further fancy feature from anzu that we could add
eventually, namely the preview of the replacement text during replace.
This was requested in some recent bug.  I think it's not so often that
such a thing would be useful, but it can be very handy occasionally.

>> since the number of "a"s decrease, and the point is always at the first
>> of the still-existing ones.  But probably one should count the number of
>> prompts, so
>>
>>    1/20, 2/20, ..., 20/20
>>
>> I think this means `perform-replace' has to implement its own way to
>> display a count.
>
> Maybe this makes more sense, when the users will learn
> what do these numbers mean.
>
>>> I meant using simply
>>>
>>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>>
>>> But it seems isearch-read-with-highlight-setup doesn't set
>>> isearch-lazy-count-display-function.

What used to be 'isearch-read-with-highlight-setup' is now
'minibuffer-lazy-highlight-setup'.

Just to make sure I understood you: your suggestion is for this function
to remove itself automagically from the minibuffer setup hook, to
dispense with the need of 'minibuffer-with-setup-hook'?  This seems
handy but unusual, hence my question.

>> I guess this could be done.
>
> Maybe two separate hooks could be defined?  One highlights like
> lazy-highlight, and another counts like lazy-count does:
>
>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-count-setup)

The highlight without counting can be achieved by binding a suitable new
variable.  Counting without highlight is not supported by isearch AFAIU.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Tue, 15 Mar 2022 17:28:01 GMT) Full text and rfc822 format available.

Message #37 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Tue, 15 Mar 2022 19:09:49 +0200
> Inlined below some further comments.
>
>>>> I meant using simply
>>>>
>>>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>>>
>>>> But it seems isearch-read-with-highlight-setup doesn't set
>>>> isearch-lazy-count-display-function.
>
> What used to be 'isearch-read-with-highlight-setup' is now
> 'minibuffer-lazy-highlight-setup'.
>
> Just to make sure I understood you: your suggestion is for this function
> to remove itself automagically from the minibuffer setup hook, to
> dispense with the need of 'minibuffer-with-setup-hook'?  This seems
> handy but unusual, hence my question.
> ...
> - In a previous message, you seemed to suggest making
>   `minibuffer-lazy-highlight-setup' self-cleaning form the
>   minibuffer-setup-hook, so as to dispense with the need for
>   `with-minibuffer-setup-hook'.  I did exactly that, but since I haven't
>   seen this convention before, I wanted to double check.

Actually, my comment was about having the line that you already added
in your latest patch in 'minibuffer-lazy-highlight-setup':

  (add-hook 'lazy-count-update-hook #'minibuffer-lazy-highlight--count)

So I don't see the need to have this line:

  (remove-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)

Also I noticed that you changed the function 'isearch-lazy-count-display-function'
to the hook 'lazy-count-update-hook', and this looks fine,
I see no problem with this.

> - Besides query-replace, I only added lazy highlight to
>   isearch-edit-string for now.

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?

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

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

> - As to the lazy count during `perform-replace': I would like to leave
>   this for later.  In fact, I think the lazy highlight has some issues
>   that need fixing beforehand.  For instance, if I replace "a" with
>   "aba", then the "a"'s from the replacement text also get lazy
>   highlighted.  We shouldn't refresh the lazy highlight during
>   `preform-replace'.  Then adding lazy count on top should be easy.

Patches welcome.

>>> I guess this could be done.
>>
>> Maybe two separate hooks could be defined?  One highlights like
>> lazy-highlight, and another counts like lazy-count does:
>>
>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-count-setup)
>
> The highlight without counting can be achieved by binding a suitable new
> variable.  Counting without highlight is not supported by isearch AFAIU.

Counting without highlighting is only possible by redefining the function
'isearch-lazy-highlight-match' to a no-op function.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Tue, 15 Mar 2022 17:28:02 GMT) Full text and rfc822 format available.

Message #40 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Tue, 15 Mar 2022 19:24:01 +0200
> Sorry for getting back to this after such a long time.  I've attached a
> new patch that hopefully is good to merge, except for adding some NEWS
> entry.  Let me know what you think.

Please see comments for your latest patch below:

> @@ -1812,6 +1812,8 @@ isearch-edit-string
>  	  (minibuffer-history-symbol)
>  	  ;; Search string might have meta information on text properties.
>  	  (minibuffer-allow-text-properties t))
> +     (when isearch-lazy-highlight
> +       (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup))

Since this does both highlighting and counting, shouldn't the condition be

        (when (and isearch-lazy-highlight isearch-lazy-count))

Or maybe new separate customizable options are needed, e.g.
'minibuffer-lazy-highlight' and 'minibuffer-lazy-count'?

> @@ -2350,7 +2352,9 @@ isearch-query-replace
>  	(isearch-recursive-edit nil)
>  	(isearch-string-propertized
>           (isearch-string-propertize isearch-string)))
> -    (isearch-done nil t)
> +    (let ((lazy-highlight-cleanup (and lazy-highlight-cleanup
> +                                       (not query-replace-lazy-highlight))))
> +      (isearch-done nil t))

Is this some optimization?  It seems it's intended to leave
some existing highlighting?  Is this to avoid double highlighting?

Also maybe this condition could use a new variable as well.

> @@ -4048,7 +4056,7 @@ isearch-lazy-highlight-new-loop
>  			         isearch-lazy-highlight-window-end))))))
>      ;; something important did indeed change
>      (lazy-highlight-cleanup t (not (equal isearch-string ""))) ;stop old timer
> -    (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
> +    (when isearch-lazy-count
> ...
> @@ -4067,7 +4075,10 @@ isearch-lazy-highlight-new-loop
>          (setq isearch-lazy-count-current nil
>                isearch-lazy-count-total nil)
>          ;; Delay updating the message if possible, to avoid flicker
> -        (when (string-equal isearch-string "") (isearch-message))))
> +        (when (string-equal isearch-string "")
> +          (when (and isearch-mode (null isearch-message-function))
> +            (isearch-message))
> ...
> @@ -4120,13 +4131,15 @@ isearch-lazy-highlight-new-loop
>                                   'isearch-lazy-highlight-start))))
>    ;; Update the current match number only in isearch-mode and
>    ;; unless isearch-mode is used specially with isearch-message-function
> -  (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
> +  (when isearch-lazy-count

The problem is that when these conditions 'isearch-mode (null isearch-message-function)'
are removed, now this shows wrong counts in the minibuffer history search
(e.g. 'M-! C-r s C-r C-r ...') and the shell history search
(e.g. 'M-x shell RET M-r s C-r C-r ...').  Before this change
counting was disabled in the history search because it shows wrong numbers.

> +(defun minibuffer-lazy-highlight--count ()
> +  "Display total match count in the minibuffer prompt."
> +  (when minibuffer-lazy-highlight--overlay
> +    (overlay-put minibuffer-lazy-highlight--overlay
> +                 'after-string
> +                 (and isearch-lazy-count-total
> +                      (not isearch-error)
> +                      (format minibuffer-lazy-count-format
> +                              isearch-lazy-count-total)))))
> ...
> +  (setq minibuffer-lazy-highlight--overlay
> +        (and minibuffer-lazy-count-format
> +             (make-overlay (point-min) (point-min) (current-buffer) t)))

For some reasons the package lisp/mb-depth.el uses 'after-string'
instead of 'before-string', and (make-overlay (point-min) (1+ (point-min)))
instead of (make-overlay (point-min) (point-min)),
so maybe better to do the same?

> @@ -365,14 +372,44 @@ query-replace-read-args
> +    (condition-case error
> +        (let (;; Variables controlling lazy highlighting while reading
> +              ;; FROM and TO.
> +              (lazy-highlight-cleanup nil)
> +              (isearch-lazy-highlight query-replace-lazy-highlight)
> +              (isearch-regexp regexp-flag)
> +              (isearch-regexp-function nil)

Highlighting is still incorrect for word replacement ('C-u M-%')
and for non-nil 'replace-char-fold'.  To handle these cases correctly,
'replace-highlight' uses:

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

> @@ -2857,22 +2914,8 @@ perform-replace
>      (when region-noncontiguous-p
> -      (let ((region-bounds
> -             (mapcar (lambda (position)
> -                       (cons (copy-marker (car position))
> -                             (copy-marker (cdr position))))
> -                     (funcall region-extract-function 'bounds))))
> -        (setq region-filter
> -              (lambda (start end)
> -                (delq nil (mapcar
> -                           (lambda (bounds)
> -                             (and
> -                              (>= start (car bounds))
> -                              (<= start (cdr bounds))
> -                              (>= end   (car bounds))
> -                              (<= end   (cdr bounds))))
> -                           region-bounds))))
> -        (add-function :after-while isearch-filter-predicate region-filter)))
> +      (setq region-filter (replace--region-filter
> +                           (funcall region-extract-function 'bounds))))

I wonder why (add-function :after-while isearch-filter-predicate region-filter)
is removed?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Tue, 15 Mar 2022 21:22:02 GMT) Full text and rfc822 format available.

Message #43 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Tue, 15 Mar 2022 22:21:12 +0100
[Message part 1 (text/plain, inline)]
On Tue, 15 Mar 2022 at 19:24, Juri Linkov <juri <at> linkov.net> wrote:

>> Sorry for getting back to this after such a long time.  I've attached a
>> new patch that hopefully is good to merge, except for adding some NEWS
>> entry.  Let me know what you think.
>
> Please see comments for your latest patch below:
>
>> @@ -1812,6 +1812,8 @@ isearch-edit-string
>>  	  (minibuffer-history-symbol)
>>  	  ;; Search string might have meta information on text properties.
>>  	  (minibuffer-allow-text-properties t))
>> +     (when isearch-lazy-highlight
>> +       (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup))
>
> Since this does both highlighting and counting, shouldn't the condition be
>
>         (when (and isearch-lazy-highlight isearch-lazy-count))
>
> Or maybe new separate customizable options are needed, e.g.
> 'minibuffer-lazy-highlight' and 'minibuffer-lazy-count'?

isearch-edit-string already has the behavior your expect: if
isearch-lazy-count is nil but isearch-lazy-highlight is t, then the
matches are highlighted but the count is not displayed.

This happens because the new lazy-count-update-hook is only run when
isearch-lazy-highlight is non-nil.

I see no need for a customization variable, since the user already gets
exactly what they asked for.

>> @@ -2350,7 +2352,9 @@ isearch-query-replace
>>  	(isearch-recursive-edit nil)
>>  	(isearch-string-propertized
>>           (isearch-string-propertize isearch-string)))
>> -    (isearch-done nil t)
>> +    (let ((lazy-highlight-cleanup (and lazy-highlight-cleanup
>> +                                       (not query-replace-lazy-highlight))))
>> +      (isearch-done nil t))
>
> Is this some optimization?  It seems it's intended to leave
> some existing highlighting?  Is this to avoid double highlighting?
>
> Also maybe this condition could use a new variable as well.

The weird let-binding for lazy-highlight-cleanup is indeed an
optimization.  Without it, you can see the lazy highlights briefly flash
off an on again after you hit RET to confirm the replacement string.

(Same remark explains why condition-case is used instead of a
unwind-protect in query-replace-read-args).

>> @@ -4048,7 +4056,7 @@ isearch-lazy-highlight-new-loop
>>  			         isearch-lazy-highlight-window-end))))))
>>      ;; something important did indeed change
>>      (lazy-highlight-cleanup t (not (equal isearch-string ""))) ;stop old timer
>> -    (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
>> +    (when isearch-lazy-count
>> ...
>> @@ -4067,7 +4075,10 @@ isearch-lazy-highlight-new-loop
>>          (setq isearch-lazy-count-current nil
>>                isearch-lazy-count-total nil)
>>          ;; Delay updating the message if possible, to avoid flicker
>> -        (when (string-equal isearch-string "") (isearch-message))))
>> +        (when (string-equal isearch-string "")
>> +          (when (and isearch-mode (null isearch-message-function))
>> +            (isearch-message))
>> ...
>> @@ -4120,13 +4131,15 @@ isearch-lazy-highlight-new-loop
>>                                   'isearch-lazy-highlight-start))))
>>    ;; Update the current match number only in isearch-mode and
>>    ;; unless isearch-mode is used specially with isearch-message-function
>> -  (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
>> +  (when isearch-lazy-count
> 
> The problem is that when these conditions 'isearch-mode (null isearch-message-function)'
> are removed, now this shows wrong counts in the minibuffer history search
> (e.g. 'M-! C-r s C-r C-r ...') and the shell history search
> (e.g. 'M-x shell RET M-r s C-r C-r ...').  Before this change
> counting was disabled in the history search because it shows wrong numbers.
>

Okay, so this means we should bind isearch-lazy-count to nil in these
commands, do you agree?  It has always looked like a hack to me to check
for (null isearch-message-function).

>> +(defun minibuffer-lazy-highlight--count ()
>> +  "Display total match count in the minibuffer prompt."
>> +  (when minibuffer-lazy-highlight--overlay
>> +    (overlay-put minibuffer-lazy-highlight--overlay
>> +                 'after-string
>> +                 (and isearch-lazy-count-total
>> +                      (not isearch-error)
>> +                      (format minibuffer-lazy-count-format
>> +                              isearch-lazy-count-total)))))
>> ...
>> +  (setq minibuffer-lazy-highlight--overlay
>> +        (and minibuffer-lazy-count-format
>> +             (make-overlay (point-min) (point-min) (current-buffer) t)))
>
> For some reasons the package lisp/mb-depth.el uses 'after-string'
> instead of 'before-string', and (make-overlay (point-min) (1+ (point-min)))
> instead of (make-overlay (point-min) (point-min)),
> so maybe better to do the same?
>

Okay, but do you know the reasons?  I've changed to before-string, but I
don't like to make the overlay 1 char longer than it has to be :-P

>> @@ -365,14 +372,44 @@ query-replace-read-args
>> +    (condition-case error
>> +        (let (;; Variables controlling lazy highlighting while reading
>> +              ;; FROM and TO.
>> +              (lazy-highlight-cleanup nil)
>> +              (isearch-lazy-highlight query-replace-lazy-highlight)
>> +              (isearch-regexp regexp-flag)
>> +              (isearch-regexp-function nil)
>
> Highlighting is still incorrect for word replacement ('C-u M-%')
> and for non-nil 'replace-char-fold'.  To handle these cases correctly,
> 'replace-highlight' uses:
>
> 	    (isearch-regexp-function (or replace-regexp-function
> 					 delimited-flag
> 					 (and replace-char-fold
> 					      (not regexp-flag)
> 					      #'char-fold-to-regexp)))

Okay, fixed this.  (BTW, where is replace-regexp-function used?  It's
not set anywhere in Emacs, and it's not a defcustom either.)

>> @@ -2857,22 +2914,8 @@ perform-replace
>>      (when region-noncontiguous-p
>> -      (let ((region-bounds
>> -             (mapcar (lambda (position)
>> -                       (cons (copy-marker (car position))
>> -                             (copy-marker (cdr position))))
>> -                     (funcall region-extract-function 'bounds))))
>> -        (setq region-filter
>> -              (lambda (start end)
>> -                (delq nil (mapcar
>> -                           (lambda (bounds)
>> -                             (and
>> -                              (>= start (car bounds))
>> -                              (<= start (cdr bounds))
>> -                              (>= end   (car bounds))
>> -                              (<= end   (cdr bounds))))
>> -                           region-bounds))))
>> -        (add-function :after-while isearch-filter-predicate region-filter)))
>> +      (setq region-filter (replace--region-filter
>> +                           (funcall region-extract-function 'bounds))))
>
> I wonder why (add-function :after-while isearch-filter-predicate region-filter)
> is removed?

Oops, that was a very serious typo.  I've fixed it now.

Here are the changes I applied on top of my previous patch:

[0001-Changes-after-Juri-s-comments.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
And this is the aggregated patch for the entire feature:

[0001-Display-lazy-highlight-and-match-count-in-when-readi.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Tue, 15 Mar 2022 21:34:02 GMT) Full text and rfc822 format available.

Message #46 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Tue, 15 Mar 2022 22:33:24 +0100
On Tue, 15 Mar 2022 at 19:09, Juri Linkov <juri <at> linkov.net> wrote:

>> Inlined below some further comments.
>>
>>>>> I meant using simply
>>>>>
>>>>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>>>>
>>>>> But it seems isearch-read-with-highlight-setup doesn't set
>>>>> isearch-lazy-count-display-function.
>>
>> What used to be 'isearch-read-with-highlight-setup' is now
>> 'minibuffer-lazy-highlight-setup'.
>>
>> Just to make sure I understood you: your suggestion is for this function
>> to remove itself automagically from the minibuffer setup hook, to
>> dispense with the need of 'minibuffer-with-setup-hook'?  This seems
>> handy but unusual, hence my question.
>> ...
>> - In a previous message, you seemed to suggest making
>>   `minibuffer-lazy-highlight-setup' self-cleaning form the
>>   minibuffer-setup-hook, so as to dispense with the need for
>>   `with-minibuffer-setup-hook'.  I did exactly that, but since I haven't
>>   seen this convention before, I wanted to double check.
>
> Actually, my comment was about having the line that you already added
> in your latest patch in 'minibuffer-lazy-highlight-setup':
>
>   (add-hook 'lazy-count-update-hook #'minibuffer-lazy-highlight--count)
>
> So I don't see the need to have this line:
>
>   (remove-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)

Okay, but if I remove this line, then all calls to
minibuffer-lazy-highlight-setup will require a with-minibuffer-setup
hook.  And this will be awkward:

     (minibuffer-with-setup-hook (if isearch-lazy-highlight
                                      #'minibuffer-lazy-highlight-setup
                                    #'ignore)
         (read-from-minibuffer "Something: "))


> Also I noticed that you changed the function 'isearch-lazy-count-display-function'
> to the hook 'lazy-count-update-hook', and this looks fine,
> I see no problem with this.
>
>> - Besides query-replace, I only added lazy highlight to
>>   isearch-edit-string for now.
>
> 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?

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

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

>> - As to the lazy count during `perform-replace': I would like to leave
>>   this for later.  In fact, I think the lazy highlight has some issues
>>   that need fixing beforehand.  For instance, if I replace "a" with
>>   "aba", then the "a"'s from the replacement text also get lazy
>>   highlighted.  We shouldn't refresh the lazy highlight during
>>   `preform-replace'.  Then adding lazy count on top should be easy.
>
> Patches welcome.
>
>>>> I guess this could be done.
>>>
>>> Maybe two separate hooks could be defined?  One highlights like
>>> lazy-highlight, and another counts like lazy-count does:
>>>
>>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-count-setup)
>>
>> The highlight without counting can be achieved by binding a suitable new
>> variable.  Counting without highlight is not supported by isearch AFAIU.
>
> Counting without highlighting is only possible by redefining the function
> 'isearch-lazy-highlight-match' to a no-op function.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 16 Mar 2022 19:05:01 GMT) Full text and rfc822 format available.

Message #49 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 16 Mar 2022 20:56:16 +0200
>> So I don't see the need to have this line:
>>
>>   (remove-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
>
> Okay, but if I remove this line, then all calls to
> minibuffer-lazy-highlight-setup will require a with-minibuffer-setup
> hook.  And this will be awkward:
>
>      (minibuffer-with-setup-hook (if isearch-lazy-highlight
>                                       #'minibuffer-lazy-highlight-setup
>                                     #'ignore)
>          (read-from-minibuffer "Something: "))

The answer depends on another question: how do you intend the users
would enable this feature?  When to enable it in all minibuffers
(like e.g. minibuffer-depth-indicate-mode does) the users will add
to their init files:

  (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)

Then removing the user's customization would not be a nice thing to do.
OTOH, minibuffer-with-setup-hook will remove only own hook, not the user's one.
So to allow enabling this feature selectively, minibuffer-with-setup-hook
is not quite awkward.

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

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

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 16 Mar 2022 19:05:02 GMT) Full text and rfc822 format available.

Message #52 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 16 Mar 2022 21:02:25 +0200
>>> @@ -2350,7 +2352,9 @@ isearch-query-replace
>>> +    (let ((lazy-highlight-cleanup (and lazy-highlight-cleanup
>>> +                                       (not query-replace-lazy-highlight))))
>>> +      (isearch-done nil t))
>>
>> Is this some optimization?  It seems it's intended to leave
>> some existing highlighting?  Is this to avoid double highlighting?
>
> The weird let-binding for lazy-highlight-cleanup is indeed an
> optimization.  Without it, you can see the lazy highlights briefly flash
> off an on again after you hit RET to confirm the replacement string.
>
> (Same remark explains why condition-case is used instead of a
> unwind-protect in query-replace-read-args).

When I tried your latest patch, it still flashes when it starts
the perform-replace loop.

>> The problem is that when these conditions 'isearch-mode (null isearch-message-function)'
>> are removed, now this shows wrong counts in the minibuffer history search
>> (e.g. 'M-! C-r s C-r C-r ...') and the shell history search
>> (e.g. 'M-x shell RET M-r s C-r C-r ...').  Before this change
>> counting was disabled in the history search because it shows wrong numbers.
>
> Okay, so this means we should bind isearch-lazy-count to nil in these
> commands, do you agree?  It has always looked like a hack to me to check
> for (null isearch-message-function).

Binding isearch-lazy-count to nil could serve as a temporary measure
until lazy-count will be supported in the history search.

>> For some reasons the package lisp/mb-depth.el uses 'after-string'
>> instead of 'before-string', and (make-overlay (point-min) (1+ (point-min)))
>> instead of (make-overlay (point-min) (point-min)),
>> so maybe better to do the same?
>
> Okay, but do you know the reasons?  I've changed to before-string, but I
> don't like to make the overlay 1 char longer than it has to be :-P

I don't know the reason.  Since it works in your patch, then it's ok.

>> Highlighting is still incorrect for word replacement ('C-u M-%')
>> and for non-nil 'replace-char-fold'.  To handle these cases correctly,
>> 'replace-highlight' uses:
>>
>> 	    (isearch-regexp-function (or replace-regexp-function
>> 					 delimited-flag
>> 					 (and replace-char-fold
>> 					      (not regexp-flag)
>> 					      #'char-fold-to-regexp)))
>
> Okay, fixed this.  (BTW, where is replace-regexp-function used?  It's
> not set anywhere in Emacs, and it's not a defcustom either.)

'replace-regexp-function' was added recently in bug#52558
to allow implementing more regexp types in bug#54017.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 16 Mar 2022 20:10:03 GMT) Full text and rfc822 format available.

Message #55 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 16 Mar 2022 21:09:16 +0100
On Wed, 16 Mar 2022 at 20:56, Juri Linkov <juri <at> linkov.net> wrote:

>>> So I don't see the need to have this line:
>>>
>>>   (remove-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
>>
>> Okay, but if I remove this line, then all calls to
>> minibuffer-lazy-highlight-setup will require a with-minibuffer-setup
>> hook.  And this will be awkward:
>>
>>      (minibuffer-with-setup-hook (if isearch-lazy-highlight
>>                                       #'minibuffer-lazy-highlight-setup
>>                                     #'ignore)
>>          (read-from-minibuffer "Something: "))
>
> The answer depends on another question: how do you intend the users
> would enable this feature?  When to enable it in all minibuffers
> (like e.g. minibuffer-depth-indicate-mode does) the users will add
> to their init files:
>
>   (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
>
> Then removing the user's customization would not be a nice thing to do.
> OTOH, minibuffer-with-setup-hook will remove only own hook, not the user's one.
> So to allow enabling this feature selectively, minibuffer-with-setup-hook
> is not quite awkward.

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?

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

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

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 16 Mar 2022 20:26:01 GMT) Full text and rfc822 format available.

Message #58 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 16 Mar 2022 21:25:01 +0100
On Wed, 16 Mar 2022 at 21:02, Juri Linkov <juri <at> linkov.net> wrote:

>>>> @@ -2350,7 +2352,9 @@ isearch-query-replace
>>>> +    (let ((lazy-highlight-cleanup (and lazy-highlight-cleanup
>>>> +                                       (not query-replace-lazy-highlight))))
>>>> +      (isearch-done nil t))
>>>
>>> Is this some optimization?  It seems it's intended to leave
>>> some existing highlighting?  Is this to avoid double highlighting?
>>
>> The weird let-binding for lazy-highlight-cleanup is indeed an
>> optimization.  Without it, you can see the lazy highlights briefly flash
>> off an on again after you hit RET to confirm the replacement string.
>>
>> (Same remark explains why condition-case is used instead of a
>> unwind-protect in query-replace-read-args).
>
> When I tried your latest patch, it still flashes when it starts
> the perform-replace loop.

Does it always happen for you?  I see this occasionally, and as far as I
can tell it's random.

>>> The problem is that when these conditions 'isearch-mode (null isearch-message-function)'
>>> are removed, now this shows wrong counts in the minibuffer history search
>>> (e.g. 'M-! C-r s C-r C-r ...') and the shell history search
>>> (e.g. 'M-x shell RET M-r s C-r C-r ...').  Before this change
>>> counting was disabled in the history search because it shows wrong numbers.
>>
>> Okay, so this means we should bind isearch-lazy-count to nil in these
>> commands, do you agree?  It has always looked like a hack to me to check
>> for (null isearch-message-function).
>
> Binding isearch-lazy-count to nil could serve as a temporary measure
> until lazy-count will be supported in the history search.

Okay, the patch from yesterday already does that.

>>> For some reasons the package lisp/mb-depth.el uses 'after-string'
>>> instead of 'before-string', and (make-overlay (point-min) (1+ (point-min)))
>>> instead of (make-overlay (point-min) (point-min)),
>>> so maybe better to do the same?
>>
>> Okay, but do you know the reasons?  I've changed to before-string, but I
>> don't like to make the overlay 1 char longer than it has to be :-P
>
> I don't know the reason.  Since it works in your patch, then it's ok.

Great.

>>> Highlighting is still incorrect for word replacement ('C-u M-%')
>>> and for non-nil 'replace-char-fold'.  To handle these cases correctly,
>>> 'replace-highlight' uses:
>>>
>>> 	    (isearch-regexp-function (or replace-regexp-function
>>> 					 delimited-flag
>>> 					 (and replace-char-fold
>>> 					      (not regexp-flag)
>>> 					      #'char-fold-to-regexp)))
>>
>> Okay, fixed this.  (BTW, where is replace-regexp-function used?  It's
>> not set anywhere in Emacs, and it's not a defcustom either.)
>
> 'replace-regexp-function' was added recently in bug#52558
> to allow implementing more regexp types in bug#54017.

Ah, I see.  I must say, in isearch.el I don't like this trichotomy of
the regexp/literal/with isearch-regexp-function cases.  Ideally, the
regexp and literal search cases would be handled by
isearch-regexp-function set to identity respectively regexp-quote.  But
it would be a large refactoring, perhaps with only aesthetic benefits.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 17:11:02 GMT) Full text and rfc822 format available.

Message #61 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 19:05:34 +0200
>> When I tried your latest patch, it still flashes when it starts
>> the perform-replace loop.
>
> Does it always happen for you?  I see this occasionally, and as far as I
> can tell it's random.

It happens every time when starting perform-replace from isearch.

>>>> Highlighting is still incorrect for word replacement ('C-u M-%')
>>>> and for non-nil 'replace-char-fold'.  To handle these cases correctly,
>>>> 'replace-highlight' uses:
>>>>
>>>> 	    (isearch-regexp-function (or replace-regexp-function
>>>> 					 delimited-flag
>>>> 					 (and replace-char-fold
>>>> 					      (not regexp-flag)
>>>> 					      #'char-fold-to-regexp)))
>>>
>>> Okay, fixed this.  (BTW, where is replace-regexp-function used?  It's
>>> not set anywhere in Emacs, and it's not a defcustom either.)
>>
>> 'replace-regexp-function' was added recently in bug#52558
>> to allow implementing more regexp types in bug#54017.
>
> Ah, I see.  I must say, in isearch.el I don't like this trichotomy of
> the regexp/literal/with isearch-regexp-function cases.  Ideally, the
> regexp and literal search cases would be handled by
> isearch-regexp-function set to identity respectively regexp-quote.  But
> it would be a large refactoring, perhaps with only aesthetic benefits.

Yep, too much changes for little benefit.

The biggest hunk in your patch are changes in query-replace-read-args
starting from 'condition-case error'.  Would it be possible to simplify
this part?  Maybe by some refactoring?

OTOH, your changes that add lazy-count-update-hook and remove
'(null isearch-message-function)' can be already pushed.
Could you please send a separate patch for pushing with these changes only?

Then the patch with minibuffer-lazy-count feature will be shorter.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 17:11:02 GMT) Full text and rfc822 format available.

Message #64 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 19:09:13 +0200
> 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?

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

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

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 19:08:02 GMT) Full text and rfc822 format available.

Message #67 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 20:06:54 +0100
[Message part 1 (text/plain, inline)]
On Thu, 17 Mar 2022 at 19:05, Juri Linkov <juri <at> linkov.net> wrote:

> OTOH, your changes that add lazy-count-update-hook and remove
> '(null isearch-message-function)' can be already pushed.
> Could you please send a separate patch for pushing with these changes only?
>
> Then the patch with minibuffer-lazy-count feature will be shorter.

There it is:

[0001-Allow-lazy-highlight-and-match-count-while-reading-f.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 19:11:01 GMT) Full text and rfc822 format available.

Message #70 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 20:10:34 +0100
On Thu, 17 Mar 2022 at 19:09, Juri Linkov <juri <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 19:47:02 GMT) Full text and rfc822 format available.

Message #73 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 20:45:57 +0100
On Thu, 17 Mar 2022 at 19:05, Juri Linkov <juri <at> linkov.net> wrote:

>>> When I tried your latest patch, it still flashes when it starts
>>> the perform-replace loop.
>>
>> Does it always happen for you?  I see this occasionally, and as far as I
>> can tell it's random.
>
> It happens every time when starting perform-replace from isearch.

Interesting, I can't see thing flash at that particular place.  But I've
replaced the unwind-protect by a more fine-grained condition-case.  Let
me know if it helps.

> The biggest hunk in your patch are changes in query-replace-read-args
> starting from 'condition-case error'.  Would it be possible to simplify
> this part?  Maybe by some refactoring?

The only refactoring opportunity I see it to define

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

which can be used in 3 places.  Should I do that?

Otherwise, I dont' see much room for improvement.  There are a lot of
moving parts there, so to me things already turned out surprisingly
short.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 21:16:01 GMT) Full text and rfc822 format available.

Message #76 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 22:40:19 +0200
>>> 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.

This means dozens of new options for every possible command that uses
the minibuffer: occur-lazy-highlight, keep-lines-lazy-highlight,
flush-lines-lazy-highlight, kill-matching-lines-lazy-highlight,
copy-matching-lines-lazy-highlight, how-many-lazy-highlight, ...




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 21:16:02 GMT) Full text and rfc822 format available.

Message #79 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 22:43:20 +0200
> On Thu, 17 Mar 2022 at 19:05, Juri Linkov <juri <at> linkov.net> wrote:
>
>>>> When I tried your latest patch, it still flashes when it starts
>>>> the perform-replace loop.
>>>
>>> Does it always happen for you?  I see this occasionally, and as far as I
>>> can tell it's random.
>>
>> It happens every time when starting perform-replace from isearch.
>
> Interesting, I can't see thing flash at that particular place.  But I've
> replaced the unwind-protect by a more fine-grained condition-case.  Let
> me know if it helps.
>
>> The biggest hunk in your patch are changes in query-replace-read-args
>> starting from 'condition-case error'.  Would it be possible to simplify
>> this part?  Maybe by some refactoring?
>
> The only refactoring opportunity I see it to define
>
>     (defun replace-regexp-function (delimited-flag)
>       (or replace-regexp-function
>           delimited-flag
>           (and replace-char-fold
>                (not regexp-flag)
>                #'char-fold-to-regexp)))
>
> which can be used in 3 places.  Should I do that?
>
> Otherwise, I dont' see much room for improvement.  There are a lot of
> moving parts there, so to me things already turned out surprisingly
> short.

I meant a function that translates arguments of query-replace/perform-replace
to search parameters.  They are currently used in replace-search and
replace-highlight.  But maybe could be reused by query-replace-read-args?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 17 Mar 2022 21:43:01 GMT) Full text and rfc822 format available.

Message #82 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 17 Mar 2022 22:42:46 +0100
On Thu, 17 Mar 2022 at 22:40, Juri Linkov <juri <at> linkov.net> wrote:

>> 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.
>
> This means dozens of new options for every possible command that uses
> the minibuffer: occur-lazy-highlight, keep-lines-lazy-highlight,
> flush-lines-lazy-highlight, kill-matching-lines-lazy-highlight,
> copy-matching-lines-lazy-highlight, how-many-lazy-highlight, ...

I'd say one option that applies to this whole group of commands that
work on lines.  If we can find a good name for this variable, it
probably means this a good idea :-)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 20 Mar 2022 09:39:02 GMT) Full text and rfc822 format available.

Message #85 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 20 Mar 2022 10:38:48 +0100
On Thu, 17 Mar 2022 at 22:40, Juri Linkov <juri <at> linkov.net> wrote:

> This means dozens of new options for every possible command that uses
> the minibuffer: occur-lazy-highlight, keep-lines-lazy-highlight,
> flush-lines-lazy-highlight, kill-matching-lines-lazy-highlight,
> copy-matching-lines-lazy-highlight, how-many-lazy-highlight, ...

I'm experimenting with adding lazy-highlight directly into
`read-regexp', controlled by a new option `read-regexp-lazy-highlight',
which, preferably, would be t by default.  Thus, in particular, all the
above commands would get lazy-highlight by default.

At first this felt somewhat intrusive, and third-party code might
require adaptation.  The advantage is that the said adaptation is very
easy.  Namely, a package author would have three options:

- Do nothing.  Then read-regexp will have lazy highlighting as dictated
  by read-regexp-lazy-highlight.
- If lazy-highlighting makes no sense at all in a given context, then
  let-bind read-regexp-lazy-highlight to nil.
- If customizability is desired, define `package-X-lazy-highlight' and
  let-bind read-regexp-lazy-highlighting to that.

What do you think?  (This is probably also the approach with the minimal
number of additional code/changed lines, which seems to be desirable.)

Dmitry -- I've CC'ed you because I noticed project.el makes a bunch of
calls to read-regexp, and also a call query-replace-read-args at one
point.  To summarize the story here: would you like to have lazy
highlight and lazy count (in “anzu” style) while reading regexps and
query-replace arguments in package.el?  How fine-grained would you like
the user options to be here, and what should the defaults be?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 20 Mar 2022 18:54:02 GMT) Full text and rfc822 format available.

Message #88 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 20 Mar 2022 20:51:09 +0200
>> This means dozens of new options for every possible command that uses
>> the minibuffer: occur-lazy-highlight, keep-lines-lazy-highlight,
>> flush-lines-lazy-highlight, kill-matching-lines-lazy-highlight,
>> copy-matching-lines-lazy-highlight, how-many-lazy-highlight, ...
>
> I'm experimenting with adding lazy-highlight directly into
> `read-regexp', controlled by a new option `read-regexp-lazy-highlight',
> which, preferably, would be t by default.  Thus, in particular, all the
> above commands would get lazy-highlight by default.
>
> At first this felt somewhat intrusive, and third-party code might
> require adaptation.  The advantage is that the said adaptation is very
> easy.  Namely, a package author would have three options:
>
> - Do nothing.  Then read-regexp will have lazy highlighting as dictated
>   by read-regexp-lazy-highlight.
> - If lazy-highlighting makes no sense at all in a given context, then
>   let-bind read-regexp-lazy-highlight to nil.
> - If customizability is desired, define `package-X-lazy-highlight' and
>   let-bind read-regexp-lazy-highlighting to that.
>
> What do you think?  (This is probably also the approach with the minimal
> number of additional code/changed lines, which seems to be desirable.)

Sorry, I have no idea who and how might want to use lazy-highlighting
in the minibuffer.  I'd just provide a hook that any user can add
to the minibuffer-setup-hook, or any package author can add
to minibuffer-with-setup-hook.  But in any case we need more opinions.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 20 Mar 2022 19:32:02 GMT) Full text and rfc822 format available.

Message #91 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 20 Mar 2022 21:24:43 +0200
>> OTOH, your changes that add lazy-count-update-hook and remove
>> '(null isearch-message-function)' can be already pushed.
>> Could you please send a separate patch for pushing with these changes only?
>>
>> Then the patch with minibuffer-lazy-count feature will be shorter.
>
> There it is:
>
> From 3de37cfec647cedd69032df072a1176970224ecb Mon Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel <at> gmail.com>
> Date: Thu, 17 Mar 2022 20:02:13 +0100
> Subject: [PATCH] Allow lazy highlight and match count while reading from
>  minibuffer

Could you please split it to more logically separate patches?

1. a patch that removes checks for 'isearch-mode (null isearch-message-function)'
   and adds '(setq-local isearch-lazy-count nil)'.
   It could also contain more changes in comint.el etc
   from your earlier patch from 14 May 2021.

2. a patch that adds and runs 'lazy-count-update-hook'.

3. a patch with minibuffer-lazy-highlight feature in isearch.el.

Then what will remain to do is to decide how it would be better to activate
the minibuffer-lazy-highlight feature.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 20 Mar 2022 20:00:02 GMT) Full text and rfc822 format available.

Message #94 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 20 Mar 2022 20:59:40 +0100
[Message part 1 (text/plain, inline)]
On Sun, 20 Mar 2022 at 21:24, Juri Linkov <juri <at> linkov.net> wrote:

> Could you please split it to more logically separate patches?
>
> 1. a patch that removes checks for 'isearch-mode (null isearch-message-function)'
>    and adds '(setq-local isearch-lazy-count nil)'.
>    It could also contain more changes in comint.el etc
>    from your earlier patch from 14 May 2021.
>
> 2. a patch that adds and runs 'lazy-count-update-hook'.
>
> 3. a patch with minibuffer-lazy-highlight feature in isearch.el.
>
> Then what will remain to do is to decide how it would be better to activate
> the minibuffer-lazy-highlight feature.

Okay.  Points 1 and 2 are hard to disentangle, and 1 strictly is only
necessary because of 2.  Also, separating things further risks
introducing bugs in code that has been tested quite a bit by now.

So my patch 00001 is for your points 1 and 2, patch 0002 is for point 3,
and patch 0003 adds lazy highlight to isearch-edit-string, where I guess
there is no controversy about what the configuration variables should
be.

Feel free to copy-edit my patches before merging if desired.

[0003-Add-lazy-highlight-to-isearch-edit-string.patch (text/x-patch, attachment)]
[0002-Allow-lazy-highlight-and-match-count-while-reading-f.patch (text/x-patch, attachment)]
[0001-New-hook-lazy-count-update-hook.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 20 Mar 2022 20:32:02 GMT) Full text and rfc822 format available.

Message #97 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 20 Mar 2022 22:29:51 +0200
>> Could you please split it to more logically separate patches?
>>
>> 1. a patch that removes checks for 'isearch-mode (null isearch-message-function)'
>>    and adds '(setq-local isearch-lazy-count nil)'.
>>    It could also contain more changes in comint.el etc
>>    from your earlier patch from 14 May 2021.
>>
>> 2. a patch that adds and runs 'lazy-count-update-hook'.
>>
>> 3. a patch with minibuffer-lazy-highlight feature in isearch.el.
>>
>> Then what will remain to do is to decide how it would be better to activate
>> the minibuffer-lazy-highlight feature.
>
> Okay.  Points 1 and 2 are hard to disentangle, and 1 strictly is only
> necessary because of 2.  Also, separating things further risks
> introducing bugs in code that has been tested quite a bit by now.
>
> So my patch 00001 is for your points 1 and 2, patch 0002 is for point 3,
> and patch 0003 adds lazy highlight to isearch-edit-string, where I guess
> there is no controversy about what the configuration variables should
> be.

Thanks, pushed now, with a NEWS entry.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 20 Mar 2022 20:57:02 GMT) Full text and rfc822 format available.

Message #100 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 20 Mar 2022 21:56:06 +0100
On Sun, 20 Mar 2022 at 22:29, Juri Linkov <juri <at> linkov.net> wrote:

> Thanks, pushed now, with a NEWS entry.

Thanks!




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 18:31:01 GMT) Full text and rfc822 format available.

Message #103 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 23 Mar 2022 20:20:10 +0200
> So my patch 00001 is for your points 1 and 2, patch 0002 is for point 3,
> and patch 0003 adds lazy highlight to isearch-edit-string, where I guess
> there is no controversy about what the configuration variables should
> be.

Unfortunately, there is a regression: isearch-yank-char-in-minibuffer
bound to C-f and [right] in the minibuffer of isearch-edit-string
doesn't work anymore.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 18:55:01 GMT) Full text and rfc822 format available.

Message #106 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 23 Mar 2022 19:54:14 +0100
[Message part 1 (text/plain, inline)]
On Wed, 23 Mar 2022 at 20:20, Juri Linkov <juri <at> linkov.net> wrote:

> Unfortunately, there is a regression: isearch-yank-char-in-minibuffer
> bound to C-f and [right] in the minibuffer of isearch-edit-string
> doesn't work anymore.

The following patch solves to problem for me.

The point doesn't move visually, so selecting the isearch window puts it
in the right place.  But what exactly happens to the point in the
searched buffer?  I find this rather confusing!

[0001-Fix-regression-in-isearch-yank-char-in-minibuffer.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 19:18:02 GMT) Full text and rfc822 format available.

Message #109 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#53126: 29.0.50;
 [PATCH] Lazy highlight/count when reading query-replace string, etc.
Date: Wed, 23 Mar 2022 21:17:16 +0200
> From: Augusto Stoffel <arstoffel <at> gmail.com>
> Date: Wed, 23 Mar 2022 19:54:14 +0100
> Cc: 53126 <at> debbugs.gnu.org
> 
> The point doesn't move visually, so selecting the isearch window puts it
> in the right place.  But what exactly happens to the point in the
> searched buffer?  I find this rather confusing!

Because of switch-to-buffer-preserve-window-point, perhaps?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 19:55:02 GMT) Full text and rfc822 format available.

Message #112 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 23 Mar 2022 21:53:41 +0200
>> Unfortunately, there is a regression: isearch-yank-char-in-minibuffer
>> bound to C-f and [right] in the minibuffer of isearch-edit-string
>> doesn't work anymore.
>
> The following patch solves to problem for me.
>
> The point doesn't move visually, so selecting the isearch window puts it
> in the right place.  But what exactly happens to the point in the
> searched buffer?  I find this rather confusing!
>
> @@ -2670,7 +2670,7 @@ isearch-yank-char-in-minibuffer
>    (interactive "p")
>    (if (eobp)
>        (insert
> -       (with-current-buffer (cadr (buffer-list))
> +       (with-minibuffer-selected-window

Thanks, I confirm your patch fixes the problem.
But before pushing it, I'd like to understand
how your previous patches changed the order
of the buffer list?  Before your changes,
(cadr (buffer-list)) returned the original buffer,
but now it returns the minibuffer.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 20:08:02 GMT) Full text and rfc822 format available.

Message #115 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 23 Mar 2022 22:06:38 +0200
>> +       (with-minibuffer-selected-window
>
> Thanks, I confirm your patch fixes the problem.
> But before pushing it, I'd like to understand
> how your previous patches changed the order
> of the buffer list?  Before your changes,
> (cadr (buffer-list)) returned the original buffer,
> but now it returns the minibuffer.

Probably the buffer order is changed by
with-minibuffer-selected-window in
minibuffer-lazy-highlight--after-change.
This is fine, so I pushed your patch.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 20:31:01 GMT) Full text and rfc822 format available.

Message #118 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 23 Mar 2022 21:30:24 +0100
On Wed, 23 Mar 2022 at 22:06, Juri Linkov <juri <at> linkov.net> wrote:

>>> +       (with-minibuffer-selected-window
>>
>> Thanks, I confirm your patch fixes the problem.
>> But before pushing it, I'd like to understand
>> how your previous patches changed the order
>> of the buffer list?  Before your changes,
>> (cadr (buffer-list)) returned the original buffer,
>> but now it returns the minibuffer.
>
> Probably the buffer order is changed by
> with-minibuffer-selected-window in
> minibuffer-lazy-highlight--after-change.
> This is fine, so I pushed your patch.

FTR, I just tested and the culprit is not the
with-minibuffer-selected-window from
minibuffer-lazy-highlight--after-change (which makes sense, since that
macro uses NORECORD internally), but rather the calls to `select-window'
without NORECORD argument in isearch.el.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Wed, 23 Mar 2022 20:46:02 GMT) Full text and rfc822 format available.

Message #121 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Wed, 23 Mar 2022 22:43:46 +0200
>> Probably the buffer order is changed by
>> with-minibuffer-selected-window in
>> minibuffer-lazy-highlight--after-change.
>> This is fine, so I pushed your patch.
>
> FTR, I just tested and the culprit is not the
> with-minibuffer-selected-window from
> minibuffer-lazy-highlight--after-change (which makes sense, since that
> macro uses NORECORD internally), but rather the calls to `select-window'
> without NORECORD argument in isearch.el.

I see that you use with-minibuffer-selected-window
to start a new loop isearch-lazy-highlight-new-loop,
but later every iteration of isearch-lazy-highlight-update
needs to select the window again.  Then adding NORECORD argument
to `select-window' in lazy-highlight functions makes sense indeed.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 24 Mar 2022 19:04:01 GMT) Full text and rfc822 format available.

Message #124 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 24 Mar 2022 20:03:33 +0100
[Message part 1 (text/plain, inline)]
On Sun, 20 Mar 2022 at 20:51, Juri Linkov <juri <at> linkov.net> wrote:

> Sorry, I have no idea who and how might want to use lazy-highlighting
> in the minibuffer.  I'd just provide a hook that any user can add
> to the minibuffer-setup-hook, or any package author can add
> to minibuffer-with-setup-hook.  But in any case we need more opinions.

All right, I think this brings us back to the original idea: we add lazy
highlight to query-replace now and decide later about the other commands
in replace.el.

I've attached an updated patch that applies over the already merged
changes.

Juri: you mentioned the idea of adding a new option
`query-replace-read-lazy-highlight'.  This is easier to add then remove
in the future, so my suggestion would be to first wait and see if anyone
actually needs that option.  For now lazy highlight when reading the
query-replace args is controlled by the good old
`query-replace-lazy-highlight'.

[0001-Display-lazy-highlight-and-match-count-in-query-repl.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 25 Mar 2022 09:11:01 GMT) Full text and rfc822 format available.

Message #127 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 25 Mar 2022 10:39:12 +0200
> @@ -365,14 +372,49 @@ query-replace-read-args
> -    (let* ((from (query-replace-read-from prompt regexp-flag))
> -           (to (if (consp from) (prog1 (cdr from) (setq from (car from)))
> -                 (query-replace-read-to from prompt regexp-flag))))
> -      (list from to
> -            (or (and current-prefix-arg (not (eq current-prefix-arg '-)))
> -                (and (plist-member (text-properties-at 0 from) 'isearch-regexp-function)
> -                     (get-text-property 0 'isearch-regexp-function from)))
> -            (and current-prefix-arg (eq current-prefix-arg '-))))))

The name of the function is `query-replace-read-args'.
And now most of the function is dealing with highlighting:

> +    (condition-case error
> +        (let (;; Variables controlling lazy highlighting while reading
> +              ;; FROM and TO.
> +              (isearch-case-fold-search case-fold-search)
> +              (isearch-lazy-highlight query-replace-lazy-highlight)
> +              (isearch-regexp regexp-flag)
> +              (isearch-regexp-function (or replace-regexp-function
> +                                           (and current-prefix-arg
> +                                                (not (eq current-prefix-arg '-)))
> +                                           (and replace-char-fold
> +                                                (not regexp-flag)
> +                                                #'char-fold-to-regexp)))
> +              (lazy-highlight-cleanup nil)
> +              (minibuffer-lazy-highlight-transform
> +               (lambda (string)
> +                 (let* ((split (query-replace--split-string string))
> +                        (from-string (if (consp split) (car split) split)))
> +                   (when (and case-fold-search search-upper-case)
> +	             (setq isearch-case-fold-search
> +                           (isearch-no-upper-case-p from-string regexp-flag)))
> +                   from-string)))
> +              from to)
> +          (when query-replace-lazy-highlight
> +            (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
> +            (when (use-region-p)
> +              (letrec ((region-filter (replace--region-filter
> +                                       (funcall region-extract-function 'bounds)))
> +                       (cleanup (lambda ()
> +                                  (remove-function isearch-filter-predicate region-filter)
> +                                  (remove-hook 'minibuffer-exit-hook cleanup))))
> +                (add-function :after-while isearch-filter-predicate region-filter)
> +                (add-hook 'minibuffer-exit-hook cleanup))))
> +          (setq from (query-replace-read-from prompt regexp-flag))
> +          (setq to (if (consp from)
> +                       (prog1 (cdr from) (setq from (car from)))
> +                     (query-replace-read-to from prompt regexp-flag)))
> +          (list from to
> +                (or (and current-prefix-arg (not (eq current-prefix-arg '-)))
> +                    (and (plist-member (text-properties-at 0 from) 'isearch-regexp-function)
> +                         (get-text-property 0 'isearch-regexp-function from)))
> +                (and current-prefix-arg (eq current-prefix-arg '-))))
> +      (t (lazy-highlight-cleanup)
> +         (signal (car error) (cdr error))))))

This highlighting needs to be refactored from `query-replace-read-args'
into a separate highlighting function.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 25 Mar 2022 09:44:05 GMT) Full text and rfc822 format available.

Message #130 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 25 Mar 2022 10:43:21 +0100
On Fri, 25 Mar 2022 at 10:39, Juri Linkov <juri <at> linkov.net> wrote:

> The name of the function is `query-replace-read-args'.
> And now most of the function is dealing with highlighting:

The good thing is that it was a rather short function before, so now
it's still reasonably sized.  Also, the lazy highlight for query-replace
is essentially taken care of in one single place.

> This highlighting needs to be refactored from `query-replace-read-args'
> into a separate highlighting function.

Okay, you've said something along these lines before but I don't
understand what you mean.

Lazy highlight is controlled by lots of variables.  These variables need
to be set for the duration of the minibuffer session that reads the
replacement strings.  If we move this to a separate function, we can't
use 'let' anymore, so we basically need to reinvent the dynamic scoping
feature of Elisp (i.e., save the current values, set the variables to
new values, then restore the old values in some hook).  And an even ugly
workaround would be needed to replace the necessary condition-case.  Or
do I misunderstand you altogether?

The above accounts for most of the additions to query-replace-read-args.
What could move elsewhere are the 10 lines starting from

     (when query-replace-lazy-highlight

but this will not change the fact that most of query-replace-read-args
now takes care of setting up lazy highlight, so I don't see any real
benefits to this.

Or maybe the issue is with this line:

    (add-hook 'minibuffer-exit-hook cleanup)

which adds a closure rather than a symbol to the hook?  That I could be
changed.  I think it would require to add a defvar just to store
'region-filter', among other things I don't like, but I can do it.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 27 Mar 2022 07:50:02 GMT) Full text and rfc822 format available.

Message #133 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 27 Mar 2022 10:46:01 +0300
> Lazy highlight is controlled by lots of variables.  These variables need
> to be set for the duration of the minibuffer session that reads the
> replacement strings.  If we move this to a separate function, we can't
> use 'let' anymore, so we basically need to reinvent the dynamic scoping
> feature of Elisp (i.e., save the current values, set the variables to
> new values, then restore the old values in some hook).  And an even ugly
> workaround would be needed to replace the necessary condition-case.  Or
> do I misunderstand you altogether?

Do you think a macro could help to make this function short again?

Then when you would want to add highlighting to the minibuffer
reading the TO part of replacement too or to other minibuffers,
it would be easy to just add a single line with that macro.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 01 Apr 2022 09:07:01 GMT) Full text and rfc822 format available.

Message #136 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 01 Apr 2022 11:06:46 +0200
On Sun, 27 Mar 2022 at 10:46, Juri Linkov <juri <at> linkov.net> wrote:

>> Lazy highlight is controlled by lots of variables.  These variables need
>> to be set for the duration of the minibuffer session that reads the
>> replacement strings.  If we move this to a separate function, we can't
>> use 'let' anymore, so we basically need to reinvent the dynamic scoping
>> feature of Elisp (i.e., save the current values, set the variables to
>> new values, then restore the old values in some hook).  And an even ugly
>> workaround would be needed to replace the necessary condition-case.  Or
>> do I misunderstand you altogether?
>
> Do you think a macro could help to make this function short again?

That could be done, of course, but I'm pretty sure that this macro would
be used exactly once.  I don't think we should have a general-purpose
macro to set up the lazy highlighting for arbitrary minibuffer commands,
because complex cases like query-replace will be rare, and in any case
we can't anticipate all special requirements.

Anyway, should I refactor my patch to use a macro?

Something else that could make 'query-replace-read-args' slightly
shorter is to define

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

This function could be used in 3 different places in replace.el.

> Then when you would want to add highlighting to the minibuffer
> reading the TO part of replacement too or to other minibuffers,
> it would be easy to just add a single line with that macro.

Right, a preview of the replacement text will also need some extra work.
This will probably involve some extension to
'isearch-lazy-highlight-update', making it even more complicated...




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 01 Apr 2022 16:45:02 GMT) Full text and rfc822 format available.

Message #139 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 01 Apr 2022 19:35:38 +0300
>>> Lazy highlight is controlled by lots of variables.  These variables need
>>> to be set for the duration of the minibuffer session that reads the
>>> replacement strings.  If we move this to a separate function, we can't
>>> use 'let' anymore, so we basically need to reinvent the dynamic scoping
>>> feature of Elisp (i.e., save the current values, set the variables to
>>> new values, then restore the old values in some hook).  And an even ugly
>>> workaround would be needed to replace the necessary condition-case.  Or
>>> do I misunderstand you altogether?
>>
>> Do you think a macro could help to make this function short again?
>
> That could be done, of course, but I'm pretty sure that this macro would
> be used exactly once.  I don't think we should have a general-purpose
> macro to set up the lazy highlighting for arbitrary minibuffer commands,
> because complex cases like query-replace will be rare,

Why do you think query-replace is more complex than other minibuffer commands?
Other minibuffer commands need to set all the same a dozen of variables that
control lazy highlighting.  Otherwise, they will highlight some random matches
that happen to reuse values left from a previous isearch.

> and in any case we can't anticipate all special requirements.

To anticipate all requirements, I recommend to try adding this feature
at least to occur.  Then we will see what code needs to be shared.

> Anyway, should I refactor my patch to use a macro?

As long as experimentation will prove at least occur can use this macro.

> Something else that could make 'query-replace-read-args' slightly
> shorter is to define
>
>     (defun replace-regexp-function (delimited-flag)
>       (or replace-regexp-function
>           delimited-flag
>           (and replace-char-fold
>                (not regexp-flag)
>                #'char-fold-to-regexp)))
>
> This function could be used in 3 different places in replace.el.

This would be nice, maybe to be used as an optional argument of the macro.

>> Then when you would want to add highlighting to the minibuffer
>> reading the TO part of replacement too or to other minibuffers,
>> it would be easy to just add a single line with that macro.
>
> Right, a preview of the replacement text will also need some extra work.
> This will probably involve some extension to
> 'isearch-lazy-highlight-update', making it even more complicated...

Why?  Shouldn't it just highlight the same way the text
entered into the minibuffer that reads the replacement?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 01 Apr 2022 18:13:02 GMT) Full text and rfc822 format available.

Message #142 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 01 Apr 2022 20:12:32 +0200
[Message part 1 (text/plain, inline)]
On Fri,  1 Apr 2022 at 19:35, Juri Linkov <juri <at> linkov.net> wrote:

> Why do you think query-replace is more complex than other minibuffer commands?
> Other minibuffer commands need to set all the same a dozen of variables that
> control lazy highlighting.  Otherwise, they will highlight some random matches
> that happen to reuse values left from a previous isearch.

Without having tested, the lazy highlight feature for occur would only
require the addition of 4 lines:

[occur.patch (text/x-patch, inline)]
diff --git a/lisp/replace.el b/lisp/replace.el
index a56e493d99..cd1bf9057f 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1741,7 +1741,11 @@ occur-excluded-properties
   :version "22.1")
 
 (defun occur-read-primary-args ()
-  (let* ((perform-collect (consp current-prefix-arg))
+  (when isearch-lazy-highlight
+    (add-hook 'minibuffer-setup-hook 'minibuffer-lazy-highlight-setup))
+  (let* ((isearch-regexp t)
+         (isearch-case-fold-search case-fold-search)
+         (perform-collect (consp current-prefix-arg))
          (regexp (read-regexp (if perform-collect
                                   "Collect strings matching regexp"
                                 "List lines matching regexp")
[Message part 3 (text/plain, inline)]
So yes, I'd say query-replace is very likely to be most complex case
there will be.  And it has several unique features that make it quite
different from other potential uses of lazy highlight:

- The splitting of the TO and FROM strings at "->".
- The value of case-fold-search can change on the fly
- We must clean up the highlight only if the user quits, to avoid
  flickering at the beginning of the perform-replace stage.

Okay, as I write this I realize occur would require some special stuff
if the region is active.  This indeed should be factored out.  But
hopefully you will agree with the 3 points above :-).

>> Right, a preview of the replacement text will also need some extra work.
>> This will probably involve some extension to
>> 'isearch-lazy-highlight-update', making it even more complicated...
>
> Why?  Shouldn't it just highlight the same way the text
> entered into the minibuffer that reads the replacement?

I'm referring to the anzu feature whereby the replacement text is shown
next to each match, like this:

[anzu.png (image/png, inline)]
[Message part 5 (text/plain, inline)]
This will not be totally trivial to implement, right?  And it will add
some extra stuff to query-replace-read-args that is very much
query-replace specific.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sat, 02 Apr 2022 18:32:02 GMT) Full text and rfc822 format available.

Message #145 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sat, 02 Apr 2022 21:23:10 +0300
>> Why do you think query-replace is more complex than other minibuffer commands?
>> Other minibuffer commands need to set all the same a dozen of variables that
>> control lazy highlighting.  Otherwise, they will highlight some random matches
>> that happen to reuse values left from a previous isearch.
>
> Without having tested, the lazy highlight feature for occur would only
> require the addition of 4 lines:

I could test it, but I'm quite sure 4 lines would not be sufficient,
because lazy-highlight relies on much more isearch variables.

> So yes, I'd say query-replace is very likely to be most complex case
> there will be.  And it has several unique features that make it quite
> different from other potential uses of lazy highlight:
>
> - The splitting of the TO and FROM strings at "->".

Agreed.

> - The value of case-fold-search can change on the fly

Please clarify when it can be changed on the fly.

> - We must clean up the highlight only if the user quits, to avoid
>   flickering at the beginning of the perform-replace stage.

I don't know how important is to avoid flickering at the beginning,
when it flickers after every replacement anyway.

> Okay, as I write this I realize occur would require some special stuff
> if the region is active.  This indeed should be factored out.  But
> hopefully you will agree with the 3 points above :-).

I agreed on 1.5 points :-)

>>> Right, a preview of the replacement text will also need some extra work.
>>> This will probably involve some extension to
>>> 'isearch-lazy-highlight-update', making it even more complicated...
>>
>> Why?  Shouldn't it just highlight the same way the text
>> entered into the minibuffer that reads the replacement?
>
> I'm referring to the anzu feature whereby the replacement text is shown
> next to each match, like this:
>
> This will not be totally trivial to implement, right?  And it will add
> some extra stuff to query-replace-read-args that is very much
> query-replace specific.

I very much doubt in usefulness of such a feature.  I thought it could
simply highlight the replacement text to show the places in the buffer
that already contain such replacement text.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 03 Apr 2022 08:33:02 GMT) Full text and rfc822 format available.

Message #148 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 03 Apr 2022 10:32:12 +0200
[Message part 1 (text/plain, inline)]
I've attached a sketch of a macro to help activating the minibuffer lazy
highlight.  It doesn't make query-replace-read-args exactly short, but
if you like this, I can prepare the patch.

Note that I haven't tested this code at all.  It's for impressionistic
purposes only.

[0001-Display-lazy-highlight-and-match-count-in-query-repl.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Below a response to your last message.

On Sat,  2 Apr 2022 at 21:23, Juri Linkov <juri <at> linkov.net> wrote:

>> - The value of case-fold-search can change on the fly
>
> Please clarify when it can be changed on the fly.

I refer to the following lines of my patch, which are necessary.

    (when (and case-fold-search search-upper-case)
     (setq isearch-case-fold-search
           (isearch-no-upper-case-p from-string regexp-flag)))

(Incidentally, I like better the ripgrep smart case, where an uppercase
character only matches itself, and lowercase characters match both upper
and lower case.)

>> I'm referring to the anzu feature whereby the replacement text is shown
>> next to each match, like this:
>>
>> This will not be totally trivial to implement, right?  And it will add
>> some extra stuff to query-replace-read-args that is very much
>> query-replace specific.
>
> I very much doubt in usefulness of such a feature.  I thought it could
> simply highlight the replacement text to show the places in the buffer
> that already contain such replacement text.

The anzu thing is only really meaningful if you are doing some complex
replacements with \, and whatnot.  But in that case, I guess it could
help a lot?

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 03 Apr 2022 17:20:01 GMT) Full text and rfc822 format available.

Message #151 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 03 Apr 2022 20:06:37 +0300
> I've attached a sketch of a macro to help activating the minibuffer lazy
> highlight.  It doesn't make query-replace-read-args exactly short,

I guess it will be much shorter for other less complex uses like occur?
So the complexity of this feature activation will be hidden inside the macro.

> Note that I haven't tested this code at all.  It's for impressionistic
> purposes only.

This code doesn't look bad when other uses will have just one line with
with-minibuffer-lazy-highlight.  But then the macro
with-minibuffer-lazy-highlight needs to set some reasonable
default values to isearch variables, such as setting
isearch-forward to t, etc.

> but if you like this, I can prepare the patch.

Currently it fails with

  Eager macro-expansion failure: (error
  "Malformed argument list ends with: ...

so it seems the macro complexity makes it too fragile.
OTOH, spreading this complexity in every feature use
doesn't look better.

> The anzu thing is only really meaningful if you are doing some complex
> replacements with \, and whatnot.  But in that case, I guess it could
> help a lot?

I can see how it could help during replacements, but not during entering
the replacement string.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Mon, 04 Apr 2022 16:39:01 GMT) Full text and rfc822 format available.

Message #154 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Mon, 04 Apr 2022 19:37:10 +0300
> I've attached a sketch of a macro to help activating the minibuffer lazy
> highlight.  It doesn't make query-replace-read-args exactly short, but
> if you like this, I can prepare the patch.

I realized we already have the macro minibuffer-with-setup-hook,
so we don't need more macros.

The purpose of minibuffer-with-setup-hook is to set up the minibuffer
to the initial state with buffer-local variables.  In case of
lazy-highlighting, we can't set isearch variables directly,
but all isearch variables can be replaced by one variable
that contains a set of lazy-highlight parameters.  For example:

  (minibuffer-with-setup-hook
      (lambda ()
        (setq-local lazy-highlight-params
                    `((case-fold ,case-fold-search)
                      (regexp ,regexp-flag)
                      (regexp-function ,(or replace-regexp-function
                                            (and current-prefix-arg
                                                 (not (eq current-prefix-arg '-)))
                                            (and replace-char-fold
                                                 (not regexp-flag)
                                                 #'char-fold-to-regexp)))))
        (minibuffer-lazy-highlight-init))
    (query-replace-read-from prompt regexp-flag))

where minibuffer-lazy-highlight-init does a little more than
minibuffer-lazy-highlight-setup.  Or maybe it's not needed,
and minibuffer-lazy-highlight-setup should be enough.
Or maybe instead of setq-local is can let-bind variables.

Please also note that condition-case can be replaced by
a hook in minibuffer-exit-hook that can remove highlighting
after exiting the minibuffer.

Alternatively, the same lambda above could be added to

  (add-hook 'minibuffer-setup-hook (lambda () ...))




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Tue, 05 Apr 2022 16:40:01 GMT) Full text and rfc822 format available.

Message #157 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Tue, 05 Apr 2022 18:38:52 +0200
On Mon,  4 Apr 2022 at 19:37, Juri Linkov <juri <at> linkov.net> wrote:

>> I've attached a sketch of a macro to help activating the minibuffer lazy
>> highlight.  It doesn't make query-replace-read-args exactly short, but
>> if you like this, I can prepare the patch.
>
> I realized we already have the macro minibuffer-with-setup-hook,
> so we don't need more macros.
>
> The purpose of minibuffer-with-setup-hook is to set up the minibuffer
> to the initial state with buffer-local variables.  In case of
> lazy-highlighting, we can't set isearch variables directly,
> but all isearch variables can be replaced by one variable
> that contains a set of lazy-highlight parameters.  For example:
>
>   (minibuffer-with-setup-hook
>       (lambda ()
>         (setq-local lazy-highlight-params
>                     `((case-fold ,case-fold-search)
>                       (regexp ,regexp-flag)
>                       (regexp-function ,(or replace-regexp-function
>                                             (and current-prefix-arg
>                                                  (not (eq current-prefix-arg '-)))
>                                             (and replace-char-fold
>                                                  (not regexp-flag)
>                                                  #'char-fold-to-regexp)))))
>         (minibuffer-lazy-highlight-init))
>     (query-replace-read-from prompt regexp-flag))

With this code, case-fold-search and all following lazy highlight params
are evaluated with the minibuffer as current buffer, which is not the
intended behavior.

But that's a good point, we don't need a macro.  Among several
variations, we could make the setup code look like this:

     (minibuffer-with-setup-hook
           (minibuffer-lazy-highlight-init :case-fold case-fold-search
                                           :regexp regexp-flag
                                           ...)
       (query-replace-read-from prompt regexp-flag))

where now `minibuffer-lazy-highlight-init' is not the function that
initializes stuff, but rather a function that returns a closure that
initializes stuff.

> where minibuffer-lazy-highlight-init does a little more than
> minibuffer-lazy-highlight-setup.  Or maybe it's not needed,
> and minibuffer-lazy-highlight-setup should be enough.
> Or maybe instead of setq-local is can let-bind variables.
>
> Please also note that condition-case can be replaced by
> a hook in minibuffer-exit-hook that can remove highlighting
> after exiting the minibuffer.

If it was a `unwind-protect', I would agree.  But I don't know how to
simulate a `condition-case'.  Specifically, how can we determine if some
hook (the minibuffer-exit-hook in this case) is being run "normally" or
as part of the recovery from a signaled error?

> Alternatively, the same lambda above could be added to
>
>   (add-hook 'minibuffer-setup-hook (lambda () ...))

Why was it again that we want to avoid saying something like this?

    (let ((case-fold-search whatever)
          (isearch-regexp regexp-flag))
       (minibuffer-with-setup-hook #'minibuffer-lazy-highlight-init
         (query-replace-read-from prompt regexp-flag)))





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Tue, 05 Apr 2022 17:41:02 GMT) Full text and rfc822 format available.

Message #160 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Tue, 05 Apr 2022 20:12:16 +0300
> But that's a good point, we don't need a macro.  Among several
> variations, we could make the setup code look like this:
>
>      (minibuffer-with-setup-hook
>            (minibuffer-lazy-highlight-init :case-fold case-fold-search
>                                            :regexp regexp-flag
>                                            ...)
>        (query-replace-read-from prompt regexp-flag))
>
> where now `minibuffer-lazy-highlight-init' is not the function that
> initializes stuff, but rather a function that returns a closure that
> initializes stuff.

Looks good.

>> Please also note that condition-case can be replaced by
>> a hook in minibuffer-exit-hook that can remove highlighting
>> after exiting the minibuffer.
>
> If it was a `unwind-protect', I would agree.  But I don't know how to
> simulate a `condition-case'.  Specifically, how can we determine if some
> hook (the minibuffer-exit-hook in this case) is being run "normally" or
> as part of the recovery from a signaled error?

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.

>> Alternatively, the same lambda above could be added to
>>
>>   (add-hook 'minibuffer-setup-hook (lambda () ...))
>
> Why was it again that we want to avoid saying something like this?
>
>     (let ((case-fold-search whatever)
>           (isearch-regexp regexp-flag))
>        (minibuffer-with-setup-hook #'minibuffer-lazy-highlight-init
>          (query-replace-read-from prompt regexp-flag)))

4 lines look nice, unlike 20 lines in one of your patches ;-)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Thu, 07 Apr 2022 19:33:01 GMT) Full text and rfc822 format available.

Message #163 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Thu, 07 Apr 2022 21:32:21 +0200
[Message part 1 (text/plain, inline)]
On Tue,  5 Apr 2022 at 20:12, Juri Linkov <juri <at> linkov.net> wrote:

>> But that's a good point, we don't need a macro.  Among several
>> variations, we could make the setup code look like this:
>>
>>      (minibuffer-with-setup-hook
>>            (minibuffer-lazy-highlight-init :case-fold case-fold-search
>>                                            :regexp regexp-flag
>>                                            ...)
>>        (query-replace-read-from prompt regexp-flag))
>>
>> where now `minibuffer-lazy-highlight-init' is not the function that
>> initializes stuff, but rather a function that returns a closure that
>> initializes stuff.
>
> 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.)

[0001-Display-lazy-highlight-and-match-count-in-query-repl.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]

>>> Please also note that condition-case can be replaced by
>>> a hook in minibuffer-exit-hook that can remove highlighting
>>> after exiting the minibuffer.
>>
>> If it was a `unwind-protect', I would agree.  But I don't know how to
>> simulate a `condition-case'.  Specifically, how can we determine if some
>> hook (the minibuffer-exit-hook in this case) is being run "normally" or
>> as part of the recovery from a signaled error?
>
> 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.

>>> Alternatively, the same lambda above could be added to
>>>
>>>   (add-hook 'minibuffer-setup-hook (lambda () ...))
>>
>> Why was it again that we want to avoid saying something like this?
>>
>>     (let ((case-fold-search whatever)
>>           (isearch-regexp regexp-flag))
>>        (minibuffer-with-setup-hook #'minibuffer-lazy-highlight-init
>>          (query-replace-read-from prompt regexp-flag)))
>
> 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.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 08 Apr 2022 07:50:01 GMT) Full text and rfc822 format available.

Message #166 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 08 Apr 2022 10:32:41 +0300
>> 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!

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.

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

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Fri, 08 Apr 2022 07:54:01 GMT) Full text and rfc822 format available.

Message #169 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Fri, 08 Apr 2022 09:53:42 +0200
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!

Great, then I'll test it some more and prepare properly formatted
patches.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sat, 09 Apr 2022 11:07:01 GMT) Full text and rfc822 format available.

Message #172 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: 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.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#53126; Package emacs. (Sun, 10 Apr 2022 19:40:02 GMT) Full text and rfc822 format available.

Message #175 received at 53126 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 53126 <at> debbugs.gnu.org
Subject: Re: bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading
 query-replace string, etc.
Date: Sun, 10 Apr 2022 22:38:19 +0300
close 53126 29.0.50
thanks

> Please find attached the patches.

Thanks for working on this feature.
Now your patch provides a quote elegant code
with an easy-to-use function with simple parameters.
So now pushed to master.

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

Please create a new request when you want to add more features.




bug marked as fixed in version 29.0.50, send any further explanations to 53126 <at> debbugs.gnu.org and Augusto Stoffel <arstoffel <at> gmail.com> Request was from Juri Linkov <juri <at> linkov.net> to control <at> debbugs.gnu.org. (Sun, 10 Apr 2022 19:40:03 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Mon, 09 May 2022 11:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 346 days ago.

Previous Next


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