bug#74404: [PATCH] Give Completion Preview bindings higher precedence

bug#74404: [PATCH] Give Completion Preview bindings higher precedence

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

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

Tags: patch

Whenever Completion Preview mode shows a completion preview, it also
activates keybindings for commands that interact with that preview, most
notably TAB is bound to completion-preview-insert, which inserts the
suggested completion.  This is implemented via a minor mode
completion-preview-active-mode whose keymap provides these bindings, and
which is enabled when the preview is displayed. 

This all works well, except for when another minor mode, foo-mode, is
enabled, whose keybindings conflict with these active preview bindings.
If foo-mode is defined after completion-preview-active-mode, i.e. if
foo.el is loaded after completion-preview.el, then foo-mode comes first
in minor-mode-map-alist and takes precedence over the bindings in
completion-preview-active-mode-map.

This happens, for example, with eshell-cmpl-mode, a minor mode that is
enabled by default in Eshell, which binds TAB to completion-at-point.
So currently Completion Preview mode doesn't work as expected in Eshell:
when you try to accept the suggested completion (with TAB), you get
completion-at-point instead of completion-preview-insert.

To fix this issue, we need to give the active completion preview
bindings higher precedence than keymaps of minor modes that where
defined after loading completion-preview.el.  The patch below does that
by using minor-mode-overriding-map-alist.  Any thoughts?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Give-Completion-Preview-bindings-higher-precedence.patch --]
[-- Type: text/patch, Size: 1520 bytes --]

From 951df80d3749b2e82072f397dd7c7b5ce256f36d Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 17 Nov 2024 16:55:30 +0100
Subject: [PATCH] Give Completion Preview bindings higher precedence

* lisp/completion-preview.el
(completion-preview-active-mode): add keymap to
'minor-mode-overriding-map-alist' so it takes precedence
over other minor mode maps that bind TAB, such as
'eshell-cmpl-mode' in Eshell.
---
 lisp/completion-preview.el | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lisp/completion-preview.el b/lisp/completion-preview.el
index 4564812e8a9..e5be3197032 100644
--- a/lisp/completion-preview.el
+++ b/lisp/completion-preview.el
@@ -322,8 +322,13 @@ completion-preview-active-mode
   "Mode for when the completion preview is shown."
   :interactive nil
   (if completion-preview-active-mode
-      (add-hook 'window-selection-change-functions
-                #'completion-preview--window-selection-change nil t)
+      (progn
+        (add-hook 'window-selection-change-functions
+                  #'completion-preview--window-selection-change nil t)
+        ;; Give keymap precedence over other minor mode maps.
+        (setf (alist-get 'completion-preview-active-mode
+                         minor-mode-overriding-map-alist)
+              completion-preview-active-mode-map))
     (remove-hook 'window-selection-change-functions
                  #'completion-preview--window-selection-change t)
     (completion-preview-hide)))
-- 
2.46.2

bug#74404: [PATCH] Give Completion Preview bindings higher precedence

[Eli Zaretskii]

> Date: Sun, 17 Nov 2024 17:30:47 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Whenever Completion Preview mode shows a completion preview, it also
> activates keybindings for commands that interact with that preview, most
> notably TAB is bound to completion-preview-insert, which inserts the
> suggested completion.  This is implemented via a minor mode
> completion-preview-active-mode whose keymap provides these bindings, and
> which is enabled when the preview is displayed. 
> 
> This all works well, except for when another minor mode, foo-mode, is
> enabled, whose keybindings conflict with these active preview bindings.
> If foo-mode is defined after completion-preview-active-mode, i.e. if
> foo.el is loaded after completion-preview.el, then foo-mode comes first
> in minor-mode-map-alist and takes precedence over the bindings in
> completion-preview-active-mode-map.
> 
> This happens, for example, with eshell-cmpl-mode, a minor mode that is
> enabled by default in Eshell, which binds TAB to completion-at-point.
> So currently Completion Preview mode doesn't work as expected in Eshell:
> when you try to accept the suggested completion (with TAB), you get
> completion-at-point instead of completion-preview-insert.
> 
> To fix this issue, we need to give the active completion preview
> bindings higher precedence than keymaps of minor modes that where
> defined after loading completion-preview.el.  The patch below does that
> by using minor-mode-overriding-map-alist.  Any thoughts?

Can't you instead use the 'keymap' text or overlay property?  That
would avoid the "arms race" of precedences.

Adding Stefan, in case he has comments or suggestions.

bug#74404: [PATCH] Give Completion Preview bindings higher precedence

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

>> To fix this issue, we need to give the active completion preview
>> bindings higher precedence than keymaps of minor modes that where
>> defined after loading completion-preview.el.  The patch below does that
>> by using minor-mode-overriding-map-alist.  Any thoughts?
>
> Can't you instead use the 'keymap' text or overlay property?  That
> would avoid the "arms race" of precedences.
>
> Adding Stefan, in case he has comments or suggestions.

Assuming the minor mode is disabled as soon as point moves away from the
completion, I wouldn't have a clear preference between `keymap` and
`minor-mode-overriding-map-alist`.

BTW, maybe we should add some notion of minor mode precedence since such
problems are actually fairly common.  We could do something similar to
what we do with `add-hook`, so `add-minor-mode` takes care of obeying
the ordering constraints.


        Stefan

bug#74404: [PATCH] Give Completion Preview bindings higher precedence

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

Hi,

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> To fix this issue, we need to give the active completion preview
>>> bindings higher precedence than keymaps of minor modes that where
>>> defined after loading completion-preview.el.  The patch below does that
>>> by using minor-mode-overriding-map-alist.  Any thoughts?
>>
>> Can't you instead use the 'keymap' text or overlay property?  That
>> would avoid the "arms race" of precedences.

I thought about that, but no, we can't easily use that, since the
overlay is not necessarily at point: it is generally at the end of the
symbol, and point may be at the middle of the symbol.

>> Adding Stefan, in case he has comments or suggestions.
>
> Assuming the minor mode is disabled as soon as point moves away from the
> completion, I wouldn't have a clear preference between `keymap` and
> `minor-mode-overriding-map-alist`.

Indeed, the minor mode is disabled as soon as the completion preview is
dismissed for whatever reason, including moving point elsewhere.

> BTW, maybe we should add some notion of minor mode precedence since such
> problems are actually fairly common.  We could do something similar to
> what we do with `add-hook`, so `add-minor-mode` takes care of obeying
> the ordering constraints.

That'd be nice, I think.


Thanks,

Eshel