Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

[Stefan Monnier]

>          (when filter
> +          (make-local-variable 'isearch-filter-predicate)
>            (add-function :after-while isearch-filter-predicate filter))

`add-function` is modeled after `add-hook`, so the above
`make-local-variable` would be better replaced with

    (add-function :after-while (local 'isearch-filter-predicate) filter)

[ Of course, that's pure theory.  You'd better test it first.  ]


        Stefan

Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

[Augusto Stoffel]

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

On Sun, 15 May 2022 at 16:16, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>>          (when filter
>> +          (make-local-variable 'isearch-filter-predicate)
>>            (add-function :after-while isearch-filter-predicate filter))
>
> `add-function` is modeled after `add-hook`, so the above
> `make-local-variable` would be better replaced with
>
>     (add-function :after-while (local 'isearch-filter-predicate) filter)
>
> [ Of course, that's pure theory.  You'd better test it first.  ]

Yes, I guess that's the right way to approach this.  The attached patch
implements it.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-last-change-in-minibuffer-lazy-highlight-setup.patch --]
[-- Type: text/x-patch, Size: 2173 bytes --]

From 6699f0b1313f475e83757dc954ee59c8171e9500 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sun, 15 May 2022 22:48:50 +0200
Subject: [PATCH] Fix last change in minibuffer-lazy-highlight-setup

* lisp/isearch.el (minibuffer-lazy-highlight-setup): Apply advices
buffer-locally.
---
 lisp/isearch.el | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 3e1dab4d15..31fbdf01bf 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -4410,14 +4410,17 @@ minibuffer-lazy-highlight-setup
     (let ((unwind (make-symbol "minibuffer-lazy-highlight--unwind"))
           (after-change (make-symbol "minibuffer-lazy-highlight--after-change"))
           (display-count (make-symbol "minibuffer-lazy-highlight--display-count"))
+          (buffer (current-buffer))
           overlay)
       (fset unwind
             (lambda ()
-              (remove-function isearch-filter-predicate filter)
+              (when filter
+                (with-current-buffer buffer
+                  (remove-function (local 'isearch-filter-predicate) filter)))
               (remove-hook 'lazy-count-update-hook display-count)
               (when overlay (delete-overlay overlay))
-              (remove-hook 'after-change-functions after-change)
-              (remove-hook 'minibuffer-exit-hook unwind)
+              (remove-hook 'after-change-functions after-change t)
+              (remove-hook 'minibuffer-exit-hook unwind t)
               (let ((lazy-highlight-cleanup cleanup))
                 (lazy-highlight-cleanup))))
       (fset after-change
@@ -4447,8 +4450,8 @@ minibuffer-lazy-highlight-setup
           (setq overlay (make-overlay (point-min) (point-min) (current-buffer) t))
           (add-hook 'lazy-count-update-hook display-count))
         (when filter
-          (make-local-variable 'isearch-filter-predicate)
-          (add-function :after-while isearch-filter-predicate filter))
+          (with-current-buffer buffer
+            (add-function :after-while (local 'isearch-filter-predicate) filter)))
         (funcall after-change nil nil nil)))))
 
 \f
-- 
2.36.1


[-- Attachment #3: Type: text/plain, Size: 435 bytes --]


Now, I had tried it earlier and noticed something looked fishy.  Now I
can reproduce the problem:

1. With an active region, call C-M-%
2. Switch from the minibuffer back to the original buffer
3. Call keyboard-escape-quit (maybe twice) to quit the minibuffer
   prompt.

Then the buffer-local value of `isearch-filter-predicate' is not cleaned
up.  Is it by any chance intentional that minibuffer-exit-hook doesn't
run in this case?

Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

[Juri Linkov]

>> `add-function` is modeled after `add-hook`, so the above
>> `make-local-variable` would be better replaced with
>>
>>     (add-function :after-while (local 'isearch-filter-predicate) filter)
>>
>> [ Of course, that's pure theory.  You'd better test it first.  ]
>
> Yes, I guess that's the right way to approach this.  The attached patch
> implements it.

Thanks, pushed.

> Now, I had tried it earlier and noticed something looked fishy.  Now I
> can reproduce the problem:
>
> 1. With an active region, call C-M-%
> 2. Switch from the minibuffer back to the original buffer
> 3. Call keyboard-escape-quit (maybe twice) to quit the minibuffer
>    prompt.
>
> Then the buffer-local value of `isearch-filter-predicate' is not cleaned
> up.  Is it by any chance intentional that minibuffer-exit-hook doesn't
> run in this case?

Indeed, a buffer-local minibuffer-exit-hook is not called
when the minibuffer is exited outside of the minibuffer.

This problem exists for all existing uses of buffer-local
minibuffer-exit-hook.  For example, in 'string-rectangle':

1. activate a rectangular region
2. type 'C-x r t'
3. enter a string in the minibuffer
4. switch to the original buffer
5. call keyboard-escape-quit twice

The temporary review content of the rectangle is not erased
by rectangle--string-erase-preview from minibuffer-exit-hook.

Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

[Stefan Monnier]

Juri Linkov [2022-05-18 21:59:04] wrote:

>>> `add-function` is modeled after `add-hook`, so the above
>>> `make-local-variable` would be better replaced with
>>>
>>>     (add-function :after-while (local 'isearch-filter-predicate) filter)
>>>
>>> [ Of course, that's pure theory.  You'd better test it first.  ]
>>
>> Yes, I guess that's the right way to approach this.  The attached patch
>> implements it.
>
> Thanks, pushed.
>
>> Now, I had tried it earlier and noticed something looked fishy.  Now I
>> can reproduce the problem:
>>
>> 1. With an active region, call C-M-%
>> 2. Switch from the minibuffer back to the original buffer
>> 3. Call keyboard-escape-quit (maybe twice) to quit the minibuffer
>>    prompt.
>>
>> Then the buffer-local value of `isearch-filter-predicate' is not cleaned
>> up.  Is it by any chance intentional that minibuffer-exit-hook doesn't
>> run in this case?
>
> Indeed, a buffer-local minibuffer-exit-hook is not called
> when the minibuffer is exited outside of the minibuffer.

Looks like a bug.
How 'bout the patch below?


        Stefan


diff --git a/src/minibuf.c b/src/minibuf.c
index df82bcb121a..ac48d0f4714 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -265,7 +265,7 @@ DEFUN ("set-minibuffer-window", Fset_minibuffer_window,
 
 static void read_minibuf_unwind (void);
 static void minibuffer_unwind (void);
-static void run_exit_minibuf_hook (void);
+static void run_exit_minibuf_hook (Lisp_Object minibuf);
 
 
 /* Read a Lisp object from VAL and return it.  If VAL is an empty
@@ -749,7 +749,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
      separately from read_minibuf_unwind because we need to make sure that
      read_minibuf_unwind is fully executed even if exit-minibuffer-hook
      signals an error.  --Stef  */
-  record_unwind_protect_void (run_exit_minibuf_hook);
+  record_unwind_protect (run_exit_minibuf_hook, Fcurrent_buffer ());
 
   /* Now that we can restore all those variables, start changing them.  */
 
@@ -1076,9 +1076,13 @@ get_minibuffer (EMACS_INT depth)
 }
 
 static void
-run_exit_minibuf_hook (void)
+run_exit_minibuf_hook (Lisp_Object minibuf)
 {
+  specpdl_ref count = SPECPDL_INDEX ();
+  record_unwind_current_buffer ();
+  Fset_buffer (minibuf);
   safe_run_hooks (Qminibuffer_exit_hook);
+  unbind_to (count, Qnil);
 }
 
 /* This variable records the expired minibuffer's frame between the

Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

[Juri Linkov]

>>> 1. With an active region, call C-M-%
>>> 2. Switch from the minibuffer back to the original buffer
>>> 3. Call keyboard-escape-quit (maybe twice) to quit the minibuffer
>>>    prompt.
>>>
>>> Then the buffer-local value of `isearch-filter-predicate' is not cleaned
>>> up.  Is it by any chance intentional that minibuffer-exit-hook doesn't
>>> run in this case?
>>
>> Indeed, a buffer-local minibuffer-exit-hook is not called
>> when the minibuffer is exited outside of the minibuffer.
>
> Looks like a bug.
> How 'bout the patch below?

Alas, it even breaks exiting the minibuffer directly from the minibuffer.

> -  record_unwind_protect_void (run_exit_minibuf_hook);
> +  record_unwind_protect (run_exit_minibuf_hook, Fcurrent_buffer ());

After a little debugging I see that everything is fixed after replacing
'Fcurrent_buffer ()' with 'minibuffer':

     record_unwind_protect (run_exit_minibuf_hook, minibuffer);

Re: master 2b3f3d421a: Make minibuffer lazy highlight setup buffer-local where appropriate

[Stefan Monnier]

>> How 'bout the patch below?
> Alas, it even breaks exiting the minibuffer directly from the minibuffer.
>> -  record_unwind_protect_void (run_exit_minibuf_hook);
>> +  record_unwind_protect (run_exit_minibuf_hook, Fcurrent_buffer ());
> After a little debugging I see that everything is fixed after replacing
> 'Fcurrent_buffer ()' with 'minibuffer':
>      record_unwind_protect (run_exit_minibuf_hook, minibuffer);

Duh, indeed, I misread the code, thanks for doing my debugging for me.
Pushed to `master`.


        Stefan