bug#67650: [PATCH] ; Hide completion preview when switching windows

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

This patch ensures that we dismiss the completion preview when users
switch to another window.  Previously, the completion preview would
remain in a window after switching to another window showing another
buffer, since the `post-command-hook` that Completion Preview mode sets
up locally doesn't run after a command switches to another buffer.


Thanks,

Eshel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Hide-completion-preview-when-switching-windows.patch --]
[-- Type: text/patch, Size: 2078 bytes --]

From 1edb324d5c47fab96eb8131393b1fd38f7dee7ce Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Tue, 5 Dec 2023 21:04:43 +0100
Subject: [PATCH] ; Hide completion preview when switching windows

* lisp/completion-preview.el
(completion-preview--window-selection-change): New function.
(completion-preview-mode): Add it to
'window-selection-change-functions'.
---
 lisp/completion-preview.el | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
index 1d5f1253702..b4f35d436b3 100644
--- a/lisp/completion-preview.el
+++ b/lisp/completion-preview.el
@@ -377,6 +377,15 @@ completion-preview--active-p
                completion-preview-next-candidate))
   (put cmd 'completion-predicate #'completion-preview--active-p))
 
+(defun completion-preview--window-selection-change (window)
+  "Hide completion preview in WINDOW after switching to another window.
+Completion Preview mode adds this function to
+`window-selection-change-functions', which see."
+  (unless (or (eq window (selected-window))
+              (eq window (minibuffer-selected-window)))
+    (with-current-buffer (window-buffer window)
+      (completion-preview-active-mode -1))))
+
 ;;;###autoload
 (define-minor-mode completion-preview-mode
   "Show in-buffer completion suggestions in a preview as you type.
@@ -391,8 +400,13 @@ completion-preview-mode
 cycles backward."
   :lighter " CP"
   (if completion-preview-mode
-      (add-hook 'post-command-hook #'completion-preview--post-command nil t)
+      (progn
+        (add-hook 'post-command-hook #'completion-preview--post-command nil t)
+        (add-hook 'window-selection-change-functions
+                  #'completion-preview--window-selection-change nil t))
     (remove-hook 'post-command-hook #'completion-preview--post-command t)
+    (remove-hook 'window-selection-change-functions
+                 #'completion-preview--window-selection-change t)
     (completion-preview-active-mode -1)))
 
 (provide 'completion-preview)
-- 
2.42.0

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eli Zaretskii]

> Date: Tue, 05 Dec 2023 21:26:27 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This patch ensures that we dismiss the completion preview when users
> switch to another window.  Previously, the completion preview would
> remain in a window after switching to another window showing another
> buffer, since the `post-command-hook` that Completion Preview mode sets
> up locally doesn't run after a command switches to another buffer.

Sorry, I don't understand: why does it matter whether
post-command-hook runs _after_ switching to another buffer?  When and
why would a command that caused post-command-hook invocation NOT need
to dismiss the preview?  If post-command-hook should always dismiss
the preview, then we shouldn't need another hook, right?

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 05 Dec 2023 21:26:27 +0100
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> This patch ensures that we dismiss the completion preview when users
>> switch to another window.  Previously, the completion preview would
>> remain in a window after switching to another window showing another
>> buffer, since the `post-command-hook` that Completion Preview mode sets
>> up locally doesn't run after a command switches to another buffer.
>
> Sorry, I don't understand:

No problem, let's clarify.

> why does it matter whether post-command-hook runs _after_ switching to
> another buffer?

Since then the value of `post-command-hook` is no longer the
buffer-local value you had in the buffer where you invoked the command,
and in our case that buffer-local value is the one that includes
`completion-preview--post-command`, where we would update (or dismiss)
the completion preview for that (original) buffer.

> When and why would a command that caused post-command-hook invocation
> NOT need to dismiss the preview?

What do you mean?  All commands "cause `post-command-hook`", no?
We show/hide/update the preview during `post-command-hook`.  So for
example, when `post-command-hook` runs after `self-insert-command`, we
don't dismiss the preview, in fact we show it.  Does that make sense?

> If post-command-hook should always dismiss the preview

That's not the case, see above.

> then we shouldn't need another hook, right?

Even if it were the case, we would need to handle the case when
`post-command-hook` runs in a different buffer, with a different
buffer-local value, after switching.  And indeed we need to handle that
case, which is what this patch does.


Here's a recipe that you can try to see this patch in action:

1. emacs -Q
2. M-x completion-preview-mode RET
3. Type `(def` and see the preview appears
4. C-x 4 b foo RET

Without this patch, you'd now see the preview linger in the window
showing the *scratch* buffer.  With this patch, the preview disappears
as soon as you switch to the other window.


Best,

Eshel

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 67650@debbugs.gnu.org
> Date: Wed, 06 Dec 2023 14:33:01 +0100
> 
> > why does it matter whether post-command-hook runs _after_ switching to
> > another buffer?
> 
> Since then the value of `post-command-hook` is no longer the
> buffer-local value you had in the buffer where you invoked the command,
> and in our case that buffer-local value is the one that includes
> `completion-preview--post-command`, where we would update (or dismiss)
> the completion preview for that (original) buffer.

OK, I understand.  However, it means that now we will not only slow
down every command in the buffer that has the completion-preview mode
turned on, but we will also slow down every redisplay cycle, even if
the buffer was not switched.  I think we should find a better way of
doing this.  How about that idle timer idea we discussed earlier?  I'm
beginning to like it more and more, and I have a reason to believe
this is not the end of more and more hooks you'd need to add if we
stay with this design.

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> OK, I understand.  However, it means that now we will not only slow
> down every command in the buffer that has the completion-preview mode
> turned on, but we will also slow down every redisplay cycle, even if
> the buffer was not switched.

How so?  The docstring of `window-selection-change-functions` says:

  Functions specified buffer-locally are called for each window showing
  the corresponding buffer if and only if that window has been selected
  or deselected since the last redisplay.

And indeed I see that the function this patch adds to that hook is only
called in those circumstances.  What performance impact do you envision
for other redisplay cycles?

> I think we should find a better way of doing this.

I was also a bit uneasy with extending another hook at first, so I
appreciate your scrutiny, and I'd love to consider different ways to
achieve what we want here.  But so far this is the best way I came up
with, and after testing it and examining the specifics of the case I
feel that it's not such a bad solution.

> How about that idle timer idea we discussed earlier?

I'm not sure I see how that would solve this issue, because we want to
dismiss the preview as soon as you switch windows, and I imagine that an
ideal timer would instead be less prompt to react to such a change.
What do you have in mind?

I also feel that we shouldn't underrate the ability of the current
approach to display the preview immediately.  In fact, one user said
that Completion Preview mode "seems more smooth and efficient" then the
package he was using before, which I attribute to this exact property of
showing the preview without delay.

> I'm beginning to like it more and more, and I have a reason to believe
> this is not the end of more and more hooks you'd need to add if we
> stay with this design.

I share the concern that you're expressing towards excessive use of
hooks, but I can't currently think of further cases in which we'll
/need/ any hook but `post-command-hook` for showing/hiding the preview,
and I think that we're still in the safe zone with this patch.

Perhaps we should wait a few days to see if other suggestions come up?


Thanks,

Eshel

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 67650@debbugs.gnu.org
> Date: Wed, 06 Dec 2023 19:29:32 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > OK, I understand.  However, it means that now we will not only slow
> > down every command in the buffer that has the completion-preview mode
> > turned on, but we will also slow down every redisplay cycle, even if
> > the buffer was not switched.
> 
> How so?  The docstring of `window-selection-change-functions` says:
> 
>   Functions specified buffer-locally are called for each window showing
>   the corresponding buffer if and only if that window has been selected
>   or deselected since the last redisplay.
> 
> And indeed I see that the function this patch adds to that hook is only
> called in those circumstances.  What performance impact do you envision
> for other redisplay cycles?

Look at the implementation.  Each redisplay cycle we call
run_window_change_functions, which then needs to decide whether to run
any of the window-change related hooks, and for which windows.  IOW,
before we determine whether a window was selected or deselected, we
need to examine them all.  This wastes CPU cycles.

I'm also worried about code that runs from with-selected-window and
similar macros, about code that selects and then deselects the
mini-window, and other similar "temporary changes" of the selected
window.

> > I think we should find a better way of doing this.
> 
> I was also a bit uneasy with extending another hook at first, so I
> appreciate your scrutiny, and I'd love to consider different ways to
> achieve what we want here.  But so far this is the best way I came up
> with, and after testing it and examining the specifics of the case I
> feel that it's not such a bad solution.
> 
> > How about that idle timer idea we discussed earlier?
> 
> I'm not sure I see how that would solve this issue, because we want to
> dismiss the preview as soon as you switch windows, and I imagine that an
> ideal timer would instead be less prompt to react to such a change.
> What do you have in mind?

Whether a timer will be prompt or not is determined by the time
interval programmed into the timer.  A small enough interval is
indistinguishable from "as soon as".

We pop down other features using timers, for example tooltips.  It
doesn't look bad in practice.

Also, window-selection-change-functions cannot perform redisplay
(because they are called in the middle of redisplay), so the effect
will be seen only upon the next redisplay cycle -- not immediately
anyway.

> I also feel that we shouldn't underrate the ability of the current
> approach to display the preview immediately.  In fact, one user said
> that Completion Preview mode "seems more smooth and efficient" then the
> package he was using before, which I attribute to this exact property of
> showing the preview without delay.

Maybe so, but adding too much stuff to these hooks is inelegant, to
say the least, and basically I'm reluctant to admit more and more of
such features.  We already have too many features on these hooks.  And
users rightfully complain that Emacs feels sluggish, except if you
invoke "emacs -Q".

> I share the concern that you're expressing towards excessive use of
> hooks, but I can't currently think of further cases in which we'll
> /need/ any hook but `post-command-hook` for showing/hiding the preview,
> and I think that we're still in the safe zone with this patch.

Well, please try to find alternative implementation ideas, as I'm very
unhappy about going this way.  Again, I suspect this is not the last
hook you'd want to employ for this feature, we are just starting down
that road.

> Perhaps we should wait a few days to see if other suggestions come up?

Fine by me.

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > ...now we will not only slow down every command in the buffer that
>> > has the completion-preview mode turned on, but we will also slow
>> > down every redisplay cycle, even if the buffer was not switched.
>>
>> How so?  The docstring of `window-selection-change-functions` says:
>>
>>   Functions specified buffer-locally are called for each window showing
>>   the corresponding buffer if and only if that window has been selected
>>   or deselected since the last redisplay.
>>
>> And indeed I see that the function this patch adds to that hook is only
>> called in those circumstances.  What performance impact do you envision
>> for other redisplay cycles?
>
> Look at the implementation.  Each redisplay cycle we call
> run_window_change_functions, which then needs to decide whether to run
> any of the window-change related hooks, and for which windows.  IOW,
> before we determine whether a window was selected or deselected, we
> need to examine them all.  This wastes CPU cycles.

I have, and ISTM that the buffer-local value of
`window-selection-change-functions` is only ever consulted in
`run_window_change_functions_1`, which calls this functions in turn.
That means that whenever redisplay happened without invoking our hook
function, our hook function did not incur any performance penalty.  Is
there some fast path for when this hook is nil that I'm missing?

> I'm also worried about code that runs from with-selected-window and
> similar macros, about code that selects and then deselects the
> mini-window, and other similar "temporary changes" of the selected
> window.

You'll find none of those here.  My patch only says
`with-current-buffer`, it do any temporary window selections.

>> > How about that idle timer idea we discussed earlier?
>>
>> I'm not sure I see how that would solve this issue, because we want to
>> dismiss the preview as soon as you switch windows, and I imagine that an
>> ideal timer would instead be less prompt to react to such a change.
>> What do you have in mind?
>
> Whether a timer will be prompt or not is determined by the time
> interval programmed into the timer.  A small enough interval is
> indistinguishable from "as soon as".
>
> We pop down other features using timers, for example tooltips.  It
> doesn't look bad in practice.
>
> Also, window-selection-change-functions cannot perform redisplay
> (because they are called in the middle of redisplay), so the effect
> will be seen only upon the next redisplay cycle -- not immediately
> anyway.

Well, I don't see any delay.

>> I also feel that we shouldn't underrate the ability of the current
>> approach to display the preview immediately.  In fact, one user said
>> that Completion Preview mode "seems more smooth and efficient" then the
>> package he was using before, which I attribute to this exact property of
>> showing the preview without delay.
>
> Maybe so, but adding too much stuff to these hooks is inelegant, to
> say the least, and basically I'm reluctant to admit more and more of
> such features.  We already have too many features on these hooks.  And
> users rightfully complain that Emacs feels sluggish, except if you
> invoke "emacs -Q".
>
>> I share the concern that you're expressing towards excessive use of
>> hooks, but I can't currently think of further cases in which we'll
>> /need/ any hook but `post-command-hook` for showing/hiding the preview,
>> and I think that we're still in the safe zone with this patch.
>
> Well, please try to find alternative implementation ideas, as I'm very
> unhappy about going this way.

Here's one.  I'm attaching an updated patch (v2) that does the same
thing, except now we only extend `window-selection-change-functions`
when the preview is shown, and we remove our function from the hook
immediately when hiding the preview.  So you can have ten windows
showing buffers with Completion Preview mode and only the selected
buffer will have any `window-selection-change-functions`, and only when
it's showing the preview.

> Again, I suspect this is not the last hook you'd want to employ for
> this feature, we are just starting down that road.

Noted.

>> Perhaps we should wait a few days to see if other suggestions come up?
>
> Fine by me.

In the meantime, here's the updated patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-Hide-completion-preview-when-switching-windows.patch --]
[-- Type: text/x-patch, Size: 1919 bytes --]

From ff0d83513f17d4f15ba46a8354dfb67507da1824 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Tue, 5 Dec 2023 21:04:43 +0100
Subject: [PATCH v2] ; Hide completion preview when switching windows

* lisp/completion-preview.el
(completion-preview--window-selection-change): New function.
(completion-preview-active-mode): Add it to
'window-selection-change-functions'.  (Bug#67650)
---
 lisp/completion-preview.el | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
index 1d5f1253702..2ed2e5dd001 100644
--- a/lisp/completion-preview.el
+++ b/lisp/completion-preview.el
@@ -189,10 +189,24 @@ completion-preview--get
   "Return property PROP of the completion preview overlay."
   (overlay-get completion-preview--overlay prop))
 
+(defun completion-preview--window-selection-change (window)
+  "Hide completion preview in WINDOW after switching to another window.
+Completion Preview mode adds this function to
+`window-selection-change-functions', which see."
+  (unless (or (eq window (selected-window))
+              (eq window (minibuffer-selected-window)))
+    (with-current-buffer (window-buffer window)
+      (completion-preview-active-mode -1))))
+
 (define-minor-mode completion-preview-active-mode
   "Mode for when the completion preview is shown."
   :interactive nil
-  (unless completion-preview-active-mode (completion-preview-hide)))
+  (if completion-preview-active-mode
+      (add-hook 'window-selection-change-functions
+                #'completion-preview--window-selection-change nil t)
+    (remove-hook 'window-selection-change-functions
+                 #'completion-preview--window-selection-change t)
+    (completion-preview-hide)))
 
 (defun completion-preview--try-table (table beg end props)
   "Check TABLE for a completion matching the text between BEG and END.
-- 
2.42.0

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 67650@debbugs.gnu.org
> Date: Wed, 06 Dec 2023 22:16:28 +0100
> 
> In the meantime, here's the updated patch:

Thanks, installed on the master branch, and closing the bug.

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Dmitry Gutov]

On 05/12/2023 22:26, Eshel Yaron via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> This patch ensures that we dismiss the completion preview when users
> switch to another window.  Previously, the completion preview would
> remain in a window after switching to another window showing another
> buffer, since the `post-command-hook` that Completion Preview mode sets
> up locally doesn't run after a command switches to another buffer.

Another way to do this would look like this:

diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
index 1d5f1253702..f2560a98b23 100644
--- a/lisp/completion-preview.el
+++ b/lisp/completion-preview.el
@@ -182,6 +182,7 @@ completion-preview--make-overlay
                   (eq (get-text-property 0 'face previous)
                       (get-text-property 0 'face string)))
        (add-text-properties 0 1 '(cursor 1) string)
+      (overlay-put completion-preview--overlay 'window (selected-window))
        (overlay-put completion-preview--overlay 'after-string string))
      completion-preview--overlay))


Although it depends on whether the overlay itself staying around is a 
good thing for this feature.

You could also move some cleanup logic to pre-command-hook, which should 
be fine with the buffer-locality of the value.

bug#67650: [PATCH] ; Hide completion preview when switching windows

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 05/12/2023 22:26, Eshel Yaron via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> This patch ensures that we dismiss the completion preview when users
>> switch to another window.  Previously, the completion preview would
>> remain in a window after switching to another window showing another
>> buffer, since the `post-command-hook` that Completion Preview mode sets
>> up locally doesn't run after a command switches to another buffer.
>
> Another way to do this would look like this:
>
> diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
> index 1d5f1253702..f2560a98b23 100644
> --- a/lisp/completion-preview.el
> +++ b/lisp/completion-preview.el
> @@ -182,6 +182,7 @@ completion-preview--make-overlay
>                   (eq (get-text-property 0 'face previous)
>                       (get-text-property 0 'face string)))
>        (add-text-properties 0 1 '(cursor 1) string)
> +      (overlay-put completion-preview--overlay 'window (selected-window))
>        (overlay-put completion-preview--overlay 'after-string string))
>      completion-preview--overlay))
>

We actually have that already (a few lines above in this function),
but it doesn't do the trick.  This makes the overlay appear only in
the selected window when you're editing a buffer that's displayed
in multiple windows, but it doesn't make the overlay go away when
you switch to another window.

> You could also move some cleanup logic to pre-command-hook, which
> should be fine with the buffer-locality of the value.

Yes, I'm not sure if that's better than
`window-selection-change-functions` though in terms of performance,
and we'd need to put in some extra work to know if the command is
going to select another window.