bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Juri Linkov]

X-Debbugs-Cc: Augusto Stoffel <arstoffel@gmail.com>

0. emacs -Q
1. (setq enable-recursive-minibuffers t)
1. M-! is C-a
2. M-% is

Matches are lazy-highlighted in the original buffer,
but the replacement is going to be used to replace
matches in the minibuffer.

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Juri Linkov]

> 1. (setq enable-recursive-minibuffers t)
> 2. M-! is C-a
> 3. M-% is
>
> Matches are lazy-highlighted in the original buffer,
> but the replacement is going to be used to replace
> matches in the minibuffer.

Unfortunately, this is how minibuffer-selected-window behaves:

1. M-!
2. M-x
3. M-: (minibuffer-selected-window)

returns *scratch*, but not the minibuffer where it was invoked.

So lazy-highlighting could be disabled if (minibuffer-depth) > 1,
if we are sure that not such highlighting would be preferable.

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sat, 10 Dec 2022 19:20:14 +0200
> 
> > 1. (setq enable-recursive-minibuffers t)
> > 2. M-! is C-a
> > 3. M-% is
> >
> > Matches are lazy-highlighted in the original buffer,
> > but the replacement is going to be used to replace
> > matches in the minibuffer.
> 
> Unfortunately, this is how minibuffer-selected-window behaves:
> 
> 1. M-!
> 2. M-x
> 3. M-: (minibuffer-selected-window)
> 
> returns *scratch*, but not the minibuffer where it was invoked.
> 
> So lazy-highlighting could be disabled if (minibuffer-depth) > 1,
> if we are sure that not such highlighting would be preferable.

If you know that you are in the minibuffer window, you can get that
window by calling minibuffer-window, right?

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Augusto Stoffel]

On Fri,  9 Dec 2022 at 09:42, Juri Linkov wrote:

> X-Debbugs-Cc: Augusto Stoffel <arstoffel@gmail.com>
>
> 0. emacs -Q
> 1. (setq enable-recursive-minibuffers t)
> 1. M-! is C-a
> 2. M-% is
>
> Matches are lazy-highlighted in the original buffer,
> but the replacement is going to be used to replace
> matches in the minibuffer.

I guess this happens because minibuffer-selected-window returns the
original buffer.  I think this patch does the trick?

--8<---------------cut here---------------start------------->8---
modified   lisp/isearch.el
@@ -4435,7 +4435,9 @@ minibuffer-lazy-highlight-setup
 highlighting.
 LAX-WHITESPACE: The value of `isearch-lax-whitespace' and
 `isearch-regexp-lax-whitespace' to use for lazy highlighting."
-  (if (not highlight)
+  (if (not (and highlight
+                (eq (current-buffer) (window-buffer
+                                      (minibuffer-selected-window)))))
       #'ignore
     (let ((unwind (make-symbol "minibuffer-lazy-highlight--unwind"))
           (after-change (make-symbol "minibuffer-lazy-highlight--after-change"))
--8<---------------cut here---------------end--------------->8---

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Juri Linkov]

>> 1. (setq enable-recursive-minibuffers t)
>> 1. M-! is C-a
>> 2. M-% is
>>
>> Matches are lazy-highlighted in the original buffer,
>> but the replacement is going to be used to replace
>> matches in the minibuffer.
>
> I guess this happens because minibuffer-selected-window returns the
> original buffer.  I think this patch does the trick?

We need to wait until Eli decides whether to install this
to emacs-29 or master.

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Eli Zaretskii]

> Cc: 59918@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Mon, 12 Dec 2022 19:43:25 +0200
> 
> >> 1. (setq enable-recursive-minibuffers t)
> >> 1. M-! is C-a
> >> 2. M-% is
> >>
> >> Matches are lazy-highlighted in the original buffer,
> >> but the replacement is going to be used to replace
> >> matches in the minibuffer.
> >
> > I guess this happens because minibuffer-selected-window returns the
> > original buffer.  I think this patch does the trick?
> 
> We need to wait until Eli decides whether to install this
> to emacs-29 or master.

I admit that I don't understand the patch.  minibuffer-selected-window
returns a window, not a buffer, and it returns the window that you
didn't want, AFAIU.

Or maybe I don't understand the root cause -- could one of you please
elaborate on what happens and why this is the right patch?

Also, was this code introduced in Emacs 28/29 or earlier?

Thanks.

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Augusto Stoffel]

On Mon, 12 Dec 2022 at 20:07, Eli Zaretskii wrote:

>> Cc: 59918@debbugs.gnu.org
>> From: Juri Linkov <juri@linkov.net>
>> Date: Mon, 12 Dec 2022 19:43:25 +0200
>> 
>> >> 1. (setq enable-recursive-minibuffers t)
>> >> 1. M-! is C-a
>> >> 2. M-% is
>> >>
>> >> Matches are lazy-highlighted in the original buffer,
>> >> but the replacement is going to be used to replace
>> >> matches in the minibuffer.
>> >
>> > I guess this happens because minibuffer-selected-window returns the
>> > original buffer.  I think this patch does the trick?
>> 
>> We need to wait until Eli decides whether to install this
>> to emacs-29 or master.
>
> I admit that I don't understand the patch.  minibuffer-selected-window
> returns a window, not a buffer, and it returns the window that you
> didn't want, AFAIU.

The check in the patch is whether the buffer of the
minibuffer-selected-window is the current buffer.  It's meant to fail
when you are about to start a recursive minibuffer.

> Or maybe I don't understand the root cause -- could one of you please
> elaborate on what happens and why this is the right patch?

The original code assumed that when you are about to activate a
minibuffer, the current buffer will become the (window-buffer
(minibuffer-selected-window)).  This is not true if you are entering a
recursive minibuffer, hence the check.

It may or may not be the best patch, but I would say it's the safe one
if were to fix this in Emacs 29.  I would also say the issue rare and
basically cosmetic.

> Also, was this code introduced in Emacs 28/29 or earlier?

New in Emacs 29.

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Juri Linkov <juri@linkov.net>,  59918@debbugs.gnu.org
> Date: Mon, 12 Dec 2022 20:05:52 +0100
> 
> On Mon, 12 Dec 2022 at 20:07, Eli Zaretskii wrote:
> 
> > I admit that I don't understand the patch.  minibuffer-selected-window
> > returns a window, not a buffer, and it returns the window that you
> > didn't want, AFAIU.
> 
> The check in the patch is whether the buffer of the
> minibuffer-selected-window is the current buffer.  It's meant to fail
> when you are about to start a recursive minibuffer.

Isn't that test fragile as well?  Which buffer is considered "current"
and which window is considered "selected" when the minibuffer is
active is always a tricky question.  Wouldn't it be better to test
instead that the we are in the minibuffer, and if so to use
minibuffer-window instead of minibuffer-selected-window?

> It may or may not be the best patch, but I would say it's the safe one
> if were to fix this in Emacs 29.

So is the method I suggest -- assuming that it works.

> > Also, was this code introduced in Emacs 28/29 or earlier?
> 
> New in Emacs 29.

OK, so we must fix it on the emacs-29 branch.  What remains is to
decide what fix to use.

Thanks.

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Augusto Stoffel]

[-- Attachment #1: Type: text/plain, Size: 161 bytes --]

On Mon, 12 Dec 2022 at 21:28, Eli Zaretskii wrote:

> OK, so we must fix it on the emacs-29 branch.  What remains is to
> decide what fix to use.

This one :-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-allow-lazy-highlight-from-recursive-minibuffer.patch --]
[-- Type: text/x-patch, Size: 989 bytes --]

From bcebf9622bc11cb36e0a56bc1874f24cfa634210 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Mon, 12 Dec 2022 23:07:28 +0100
Subject: [PATCH] Don't allow lazy highlight from recursive minibuffers

See bug#59918.

* lisp/isearch.el (minibuffer-lazy-highlight-setup): Don't activate
when starting a recursive minibuffer.
---
 lisp/isearch.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index bc3697deb0a..6a17d18c45e 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -4435,7 +4435,7 @@ minibuffer-lazy-highlight-setup
 highlighting.
 LAX-WHITESPACE: The value of `isearch-lax-whitespace' and
 `isearch-regexp-lax-whitespace' to use for lazy highlighting."
-  (if (not highlight)
+  (if (or (not highlight) (minibufferp))
       #'ignore
     (let ((unwind (make-symbol "minibuffer-lazy-highlight--unwind"))
           (after-change (make-symbol "minibuffer-lazy-highlight--after-change"))
-- 
2.38.1

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: juri@linkov.net,  59918@debbugs.gnu.org
> Date: Mon, 12 Dec 2022 23:42:46 +0100
> 
> On Mon, 12 Dec 2022 at 21:28, Eli Zaretskii wrote:
> 
> > OK, so we must fix it on the emacs-29 branch.  What remains is to
> > decide what fix to use.
> 
> This one :-)
> 
> >From bcebf9622bc11cb36e0a56bc1874f24cfa634210 Mon Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Mon, 12 Dec 2022 23:07:28 +0100
> Subject: [PATCH] Don't allow lazy highlight from recursive minibuffers
> 
> See bug#59918.
> 
> * lisp/isearch.el (minibuffer-lazy-highlight-setup): Don't activate
> when starting a recursive minibuffer.
> ---
>  lisp/isearch.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index bc3697deb0a..6a17d18c45e 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -4435,7 +4435,7 @@ minibuffer-lazy-highlight-setup
>  highlighting.
>  LAX-WHITESPACE: The value of `isearch-lax-whitespace' and
>  `isearch-regexp-lax-whitespace' to use for lazy highlighting."
> -  (if (not highlight)
> +  (if (or (not highlight) (minibufferp))
>        #'ignore
>      (let ((unwind (make-symbol "minibuffer-lazy-highlight--unwind"))
>            (after-change (make-symbol "minibuffer-lazy-highlight--after-change"))

This is fine with me, but AFAIU it means that replacements in the
minibuffer will never be highlighted, ever.  Is that what we want?  I
thought Juri wanted to see highlighting there?

bug#59918: 29.0.60; query-replace in the minibuffer lazy-highlights original buffer

[Juri Linkov]

close 59918 29.0.60
thanks

>> @@ -4435,7 +4435,7 @@ minibuffer-lazy-highlight-setup
>>  highlighting.
>>  LAX-WHITESPACE: The value of `isearch-lax-whitespace' and
>>  `isearch-regexp-lax-whitespace' to use for lazy highlighting."
>> -  (if (not highlight)
>> +  (if (or (not highlight) (minibufferp))
>>        #'ignore
>>      (let ((unwind (make-symbol "minibuffer-lazy-highlight--unwind"))
>>            (after-change (make-symbol "minibuffer-lazy-highlight--after-change"))

Thanks for the patch.

> This is fine with me, but AFAIU it means that replacements in the
> minibuffer will never be highlighted, ever.  Is that what we want?  I
> thought Juri wanted to see highlighting there?

Highlighting is not visible here because the query-replace prompt
replaces the original minibuffer.  So I pushed the patch to emacs-29.