fix: use a transient map to avoid overlay keymaps issues

[kassick]

When some other overlay is active at point with a keymap property, even when
its priority is lower than the inline completion overlay's, this foreign keymap
may take precedence over our inline completion keymap.

This happens, for example, when the inline completion is shown inside a pair
inserted by smartparens. For example, when the user types [, smartparens will insert the closing ] pair and leave the cursor inside the pair -- [|]. The user then presses <return>.

The result is the buffer with the following state (cursor at |:

list = [
    |
]

When Smartparens inserts the closing pair, it creates an overlay from [ to up ]. This overlay has a
keymap property mapping C-g to a function that removes the overlay.

When an inline completion is shown (either on idle or by user request), the
keymap from smartparens overlay is active despite the inline completion
overlay's higher priority.

As a result, pressing C-<return> will likely display a message complaining
the key is not bound.

If the user presses C-g once, then they gain access to the inline completion
keymap.

This caused a weird bug in which a user gets a suggestion, but can't accept
it. Pressing C-g only once would not cancel the completion, but pressing it
again would indeed hide the completion overlay. Explicitly asking for a
suggestion at the same point would then display the completion overlay and the
keymap would work as expected, since the first C-g removed the smartparens
overlay.

This commit fixes the issue using overriding-terminal-local-map. This ensures that the inline completion
keymap is active when the overlay is shown (see
https://www.gnu.org/software/emacs/manual/html_node/elisp/Searching-Keymaps.html).

Since the inline completion keymap binds [t] to a "hide and execute whatever
command was bound before", we end up with the expected behavior of the
overlay keymap.

[kassick]

This PR hopefully fixes the issue reported here . The error reported by @farazshaikh seems to be the same one I was seeing, so if they could provide some clarification or maybe test these changes, it'd be great.

[kassick]

It also fixes the issue reported in this other comment by @thecsw.

The client now obeys lsp-copilot-enabled by default -- but the user is still free to customize the applicable-fn.

[kassick]

@kiennq FYI

[kiennq]

This has a default handler in lsp--default-notification-handlers already, we might not need this here

[kassick]

The lsp-progress-function is called in this copilot-specific handler.

I added it because of panel completions (see here ).

Whether we decide that panel completions should be part of lsp-mode or of some other package, we need some copilot-specific handling of the progress notification.

I'd rather not advise the global progress handler to be able to collect panel completions, so I believe a copilot-specific function is a better strategy.

If lsp-mode already has a way of tapping into progress notifications for extra handling that is specific to some client , let me know!

[kassick]

I've opened a draft MR with the panel changes so you can better understand why panel completions require copilot-ls handling $/progress notifications.

[kiennq]

This commit fixes the issue by using a transient map when displaying the
overlay. The transient map will take priority over any active overlay map, so
we do not run into this issue from smartparens or any other mode that may be
placing overlays with active keymaps.

Instead of using transient map and cleaning up of it. Would using lsp-define-conditional-key work here?? The condition can be when the overlay visible for example.

[kassick]

would using lsp-define-conditional-key work here?? The condition can be when the overlay visible for example.

I'll try that and come back to you. I did not like how this fix makes state management a bit messy, so I'd be happy with a simpler solution ;)

(Though I'm not sure what would happen in the case where the smartparens overlay is visible (binding C-g to its hide-overlay function). Probably the overlay keymap would still have higher priority and we (as users) would have to waltz around other minor modes that may be interfering with the inline-completion-keymap.)

[kassick]

Hey @kiennq I've tried:

• lsp-define-conditional-key conditioning the binding to the visibility of the overlay (I've used lsp-mode-map for testing)
• Defining a minor mode to hold a lsp-inline-completions-active-map with the keys that should be active

The results were not great. The issue is keymap priority. C-<return> and C-g, for example, worked fine, but C-n would not be bound to our -next function. Instead, it would always be bound to evil's evil-complete-next, since I have evil-mode active in my setup.

I tried to force the minor-mode keymap to be the first one in the minor-mode-map-alist, but still (describe-key (kbd "C-n")) would give me the evil keybinding instead of the inline completion one.

I did not find much documentation on how to handle these conflicting bindings from different minor-mode maps, except texts pointing to overriding-terminal-local-map -- which in the end is what transient keymaps use.

I've pushed here the changes needed to use overriding local map directly (ignore the changes in lsp-copilot.el and the ones regarding inhibition) .

See if it looks better this way -- if so, I'll cherry pick the necessary changes and update this MR.

[fnussbaum]

I tried to force the minor-mode keymap to be the first one in the minor-mode-map-alist, but still (describe-key (kbd "C-n")) would give me the evil keybinding instead of the inline completion one.

FWIW evil uses emulation-mode-map-alists, see also https://www.gnu.org/software/emacs/manual/html_node/elisp/Searching-Keymaps.html.

[kassick]

I tried to force the minor-mode keymap to be the first one in the minor-mode-map-alist, but still (describe-key (kbd "C-n")) would give me the evil keybinding instead of the inline completion one.

FWIW evil uses emulation-mode-map-alists, see also https://www.gnu.org/software/emacs/manual/html_node/elisp/Searching-Keymaps.html.

Thanks @fnussbaum! That explains a lot!

So IMHO using overriding-terminal-local-map is the less change-haeavy, more robust strategy here ;)

I'll cherry pick the changes and update the PR

[kassick]

@kiennq that should do it.

We're now just replacing the overlay keymap by a overriding-terminal-local-map associated with the overlay and keys message. This has been working just fine here for me ;)

(On a side note, that's similar to what hydra does)