From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#67650: [PATCH] ; Hide completion preview when switching windows Date: Wed, 06 Dec 2023 22:16:28 +0100 Message-ID: References: <83r0jz4jvd.fsf@gnu.org> <837clr46y9.fsf@gnu.org> <83v89b2lyg.fsf@gnu.org> Reply-To: Eshel Yaron Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36934"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 67650@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Dec 06 22:17:31 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rAzGt-0009PK-Co for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 06 Dec 2023 22:17:31 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rAzGh-0003q0-F9; Wed, 06 Dec 2023 16:17:19 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rAzGE-0003n5-Ok for bug-gnu-emacs@gnu.org; Wed, 06 Dec 2023 16:16:54 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rAzGE-0006H5-A9 for bug-gnu-emacs@gnu.org; Wed, 06 Dec 2023 16:16:50 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rAzGQ-00029F-6U for bug-gnu-emacs@gnu.org; Wed, 06 Dec 2023 16:17:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eshel Yaron Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 06 Dec 2023 21:17:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 67650 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 67650-submit@debbugs.gnu.org id=B67650.17018974068233 (code B ref 67650); Wed, 06 Dec 2023 21:17:02 +0000 Original-Received: (at 67650) by debbugs.gnu.org; 6 Dec 2023 21:16:46 +0000 Original-Received: from localhost ([127.0.0.1]:40556 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rAzGA-00028j-82 for submit@debbugs.gnu.org; Wed, 06 Dec 2023 16:16:46 -0500 Original-Received: from mail.eshelyaron.com ([107.175.124.16]:58942 helo=eshelyaron.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rAzG7-00028X-D4 for 67650@debbugs.gnu.org; Wed, 06 Dec 2023 16:16:44 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=eshelyaron.com; s=mail; t=1701897390; bh=jrw/JdyCFXXPKXlJYSDwwzobQrW090lx4/9SpWpagxI=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=IaB4v9xfdNbns74/yJiEsRGa9l11z2JDuCiwCW45mYpQXkIY5tffc2QT9qnTT4SWC A1Z0ELTxCd0+qXpm3tWjlJug5VcSm9CbzmbGp6VYKnFSaE3s/N/fdvGxFt7gY9XarA LJJAo41uCX31cUWytVk1UVPx9L5TDF6eJck69s9UZTApvsfPq9l3Hl8m2SX7DCeT6u XQCYlKimtmv1EwXOSLOPiGlbdvu+2MfdN0+i+3Vjf4G3YsLVXaWuNMhsk5j+L+X3SE WxutUEK7Y3Dficf/9hXBUCtruIyXgG1jBAna1Ah34rXel30Jl9PQiLL4z7+5lGJjxY 88nAfPvJIxqQg== In-Reply-To: <83v89b2lyg.fsf@gnu.org> (Eli Zaretskii's message of "Wed, 06 Dec 2023 21:02:47 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:275627 Archived-At: --=-=-= Content-Type: text/plain Eli Zaretskii writes: >> From: Eshel Yaron >> >> Eli Zaretskii 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: --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=v2-0001-Hide-completion-preview-when-switching-windows.patch >From ff0d83513f17d4f15ba46a8354dfb67507da1824 Mon Sep 17 00:00:00 2001 From: Eshel Yaron 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 --=-=-=--