Feature: inline completions #4623

kassick · 2024-11-25T14:07:31Z

As discussed in #4581, this PR adds support for Inline Completions

Using some server with inlineCompletionProvider capability (I'm using the copilot-node-server with this implementation), the inline completion is triggered automatically if lsp-inline-completion-enable is set to t. The user can also call lsp-inline-completion-display explicitly to have the completion displayed.

Optionally, the user can add lsp-inline-completion-show-keys to the lsp-inline-completion-shown-hook and the number of available completions and the keys used to interact with the tooltips are shown in the minibuffer.

When the inline completion tooltip is being displayed, pressing any key that is not mapped in lsp-inline-completion-active-map causes the tooltip to be hidden and the key is processed with the current active keymap. This way, if the completion is shown automatically, the user can continue typing to have the completion ignored.

…e buffer When a overlay is displayed at the end of the buffer, (window-end) may return a value that is past the end of the buffer. Calling line-number-at-pos with a value outside of the buffer bounds raises an error.

Only the parser

…tion

…displayed

Check if company is active before displaying the overlay -- hide it if so

kassick · 2024-11-26T18:25:19Z

A note about company mode and the inline completion overlay: Since the completions are not provided by company, we may end up with the company popup displayed alongside (or over) the inline completion overlay (as it happened already with copilot.el).

To workaround this issue, I've added code to cancel company before showing the overlay and then restore it on cancel.

Kooha-2024-11-26-14-40-17.webm

Switching between the two completion UIs can be achieved by binding company-manual-begin and lsp-inline-completion-display.

Nonetheless, I've added a variable that can inhibit inline completion mode on certain conditions, such as company being active. For example, I have the following snippet in my user settings:

  (defun lsp-inline-completion-inhibit-if-company-active ()
    (and (bound-and-true-p company-mode) (company--active-p)))

  (push 'lsp-inline-completion-inhibit-if-company-active lsp-inline-completion-inhibit-predicates)

Add a list of predicates that can inhibit triggering inline completions -- e.g.: ```elisp (defun lsp-inline-completion-inhibit-if-company-active () (and (bound-and-true-p company-mode) (company--active-p))) (push 'lsp-inline-completion-inhibit-if-company-active lsp-inline-completion-inhibit-predicates) ```

kiennq · 2024-11-27T06:09:48Z

lsp-protocol.el

@@ -624,6 +626,8 @@ See `-let' for a description of the destructuring mechanism."
 (CompletionItemKindCapabilities nil (:valueSet))
 (CompletionItemTagSupportCapabilities (:valueSet) nil)
 (CompletionOptions nil (:resolveProvider :triggerCharacters :allCommitCharacters))
+ (InlineCompletionItem (:insertText) (:filterText :range :command))


Please sort this

I've added the relevant interfaces to the end of the list with the version of the protocol that supports them.

Let me know if you want to have the whole interface definition sorted.

kiennq · 2024-11-27T06:14:52Z

lsp-inline-completion.el

+                               lsp/inline-completion-trigger-automatic
+                             lsp/inline-completion-trigger-invoked)))))
+
+;;;###autoload


I don't think we need to make this function public and have autoload. Shouldn't it be used for internal function only?

Yeah, I was trying to make the test suite happy when I have the minor mode defined in lsp-mode.el; forgot to remove it later

kiennq · 2024-11-27T06:16:20Z

lsp-inline-completion.el

+    ;; Kludge to workaround multiple backends responding -- it may
+    ;; come as a list (multiple servers) or as a single ht (single
+    ;; server). Promote it to list and move on
+    (if (ht-p it) (list it) it)


The type can be ht or plist depend on lsp-use-plists so it would be better to just check the lsp-inline-completion-list? and then merge or extract and merge items.

Good call!
Though we must check if it is a completion-list or a completion-list-items

result: InlineCompletionItem[] | InlineCompletionList | null defined as follows:

I've refactored the parse function, should not depend on ht anymore

kiennq · 2024-11-27T06:19:25Z

lsp-inline-completion.el

+    map)
+  "Keymap active when showing inline code suggestions")
+
+;;;###autoload


nit: I think we don't need to make this autoload

kiennq · 2024-11-27T06:21:17Z

lsp-inline-completion.el

+  lsp-inline-completion--overlay)
+
+
+;;;###autoload


Same here. If this is not an interactive command, then we don't need to make it autoload as if it's called from another package, we can just (require 'lsp-inline-completion) instead

kiennq · 2024-11-27T06:28:50Z

lsp-inline-completion.el

+
+  (lsp-inline-completion--clear-overlay)
+
+  (setq lsp-inline-completion--showing-company


I wonder if we want to include company-mode integration here as we can have multiple alternative package provide the same functionality as company-mode, corfu for example.

What will happen if we don't cancel company mode here? The overlay from company-mode will be displayed over the inline completion? Can that be mitigate by making the overlay from inline completion topmost or bottommost?

Also, we can put it as a tip for the user (a simple function that the user can put to their init file) instead? Or even suggest the user to use a different company-mode's front end?
I've been using copilot.el with company-mode and haven't really notice issue so far though.

I wonder if we want to include company-mode integration here as we can have multiple alternative package provide the same functionality as company-mode, corfu for example.

Hmmm I agree that having no way of plugging other completion frontends may be an issue -- corfu users would have to advise the inline completion functions to have a similar level of integration, so not very ideal.

What will happen if we don't cancel company mode here?

It depends on what frontends will be active at the time we display the overlay. With the default company tooltip frontend active, I only managed to get one-liners such as in the video below. It does not obsure the company completion list, but pressing tab or whatever uses the inline completion overlay active map, so this would cause some confusion on the user side.

Kooha-2024-11-27-15-58-38.webm

What happens if company-preview frontend is active? Not sure, probably the inline completion would obscure the preview completely.

If the user uses company-box or company-posframe (my case), then overlay priority makes no difference because company displays completions in a child frame :/

a tip for the user (a simple function that the user can put to their init file) instead?

Good one!

I've refactored the company integration into a minor mode. The user can then set up their code such as in the following snippet:

(require 'lsp-inline-completion) ;; Show the current/total plus keys in the (add-hook 'lsp-inline-completion-shown-hook #'lsp-inline-completion-show-keys) ;; Make inline completions play nice with company mode (add-hook 'lsp-inline-completion-mode-hook #'lsp-inline-completion-company-integration-mode)

but pressing tab or whatever uses the inline completion overlay active map, so this would cause some confusion on the user side.

Maybe we should bind to other keys for that? I think Vs Code use other key bindings to accept inline completion item compare to the normal completion items as well?
For example, I binded something like this for copilot.el.

;; http://endlessparentheses.com/define-context-aware-keys-in-emacs.html (defmacro define-conditional-key (keymap key def cond &rest bindings) "In KEYMAP, define key sequence KEY as DEF conditionally. This is like `define-key', except the definition disappears whenever COND evaluates to nil. BINDINGS is a list of (key def cond)." (declare (indent defun) (debug (form form form form &rest sexp))) (cl-labels ((partition (n seq) (seq-partition seq n))) (thread-last bindings (append `(,key ,def ,cond)) (partition 3) (mapcar (pcase-lambda (`(,key ,def ,cond)) `(define-key ,keymap ,key '(menu-item ,(format "maybe-%s" (or (car (cdr-safe def)) def)) nil :filter (lambda (&optional _) (if ,cond ,def)))))) macroexp-progn))) (define-conditional-key copilot-completion-map (kbd "C-<return>") #'copilot-accept-completion (copilot--overlay-visible) (kbd "C-j") #'copilot-accept-completion (copilot--overlay-visible) (kbd "M-<return>") #'copilot-accept-completion-by-line (copilot--overlay-visible) (kbd "C-n") #'copilot-next-completion (copilot--overlay-visible) (kbd "C-p") #'copilot-previous-completion (copilot--overlay-visible))

kiennq · 2024-11-27T06:40:59Z

lsp-mode.el

@@ -9934,6 +9957,43 @@ string."
    (setf window-scroll-functions
          (delete #'lsp--update-inlay-hints-scroll-function window-scroll-functions)))))

+
+;; Inline Completions


nit: If possible we should move this to lsp-inline-completion.el instead.
An auto hook can be added to lsp-inline-completion to autoload this feature if it's on.
Example:

;;;###autoload (add-hook 'lsp-configure-hook (lambda () (when (and lsp-auto-configure lsp-completion-enable) (lsp-completion--enable))))

Oh, that's a neat trick!

kiennq · 2024-11-27T06:44:44Z

lsp-mode.el

+  "Mode automatically displaying inline completions."
+  :lighter nil
+  (cond
+   ((and lsp-inline-completion-mode lsp--buffer-workspaces)


Can we use lsp-on-idle-hook instead? The lsp-inlay-hints-mode is using that and it has been working well so far. It also should fire less frequently compared to on-change-hook as when typing, we're likely don't want to request inline completion anyway.

Hmm, I don't think that's a good idea.

I've tested here, and triggering it via lsp-idle-hook means we get suggestions after moving the cursor around -- I don't think a user would expect that (I, for one, don't).

Regarding the change hook being more aggressive than idle: Since lsp's idle timer is set both on post-command-hook and on after-change-hook, I don't see how that could be.

In my implementation, what actually happens in after-change is a cancel-timer and run-with-timer (i.e. timer reschedule). We don't ping the server after each change -- we ping the server after lsp-inline-completion-idle-delay seconds have elapsed without any change in the buffer.

Ah okay. The Visual Studio will trigger inline completion on mouse move as well while the VS Code seems to only trigger it on buffer modification. Whichever as default is fine for me :)

kiennq · 2024-11-27T06:45:53Z

Thank you @kassick for your contribution. I've left some comments in the PR. The feature looks good to me :)

…Context

Also, refactor how we parse the response, using pcase for a more explicit approach

Users can enable the lsp-inline-completion-company-integration-mode if they see fit. Other completion frontends may be customized via hooks, just as this minor mode does

…ginal buffer

kiennq · 2024-11-28T03:58:58Z

lsp-inline-completion.el

+      (cancel-timer lsp-inline-completion--idle-timer))
+    (let ((buffer (current-buffer)))
+      (setq lsp-inline-completion--idle-timer
+            (run-with-timer lsp-inline-completion-idle-delay


nit: run-with-idle-timer?

kiennq · 2024-11-28T04:06:52Z

lsp-inline-completion.el

+
+(defun lsp-inline-completion--after-change (&rest _)
+  (when (and lsp-inline-completion-mode lsp--buffer-workspaces)
+    (when lsp-inline-completion--idle-timer


perhaps we can move this outside of the when check to make sure lsp-inline-completion--idle-timer always be cancelled?

kiennq · 2024-11-28T04:10:07Z

lsp-inline-completion.el

+(defvar-local lsp-inline-completion--idle-timer nil
+  "The idle timer used by lsp-inline-completion-mode")
+
+(define-minor-mode lsp-inline-completion-mode


I think we need to mark this as autoload as well so the add-hook autoload below, when called will know how to trigger this.

kiennq · 2024-11-28T04:11:35Z

lsp-mode.el

-		 (mapconcat 'string-trim-right
-			    (split-string (lsp--render-on-hover-content contents t) "\n")
-			    "\n"))))
+         (mapconcat 'string-trim-right


Is the indentation here correct?

kiennq

I think this looks good now, just a few nit comments you might want to consider for a smooth load of this mode.

kiennq · 2024-11-28T04:17:59Z

lsp-inline-completion.el

+  lsp-inline-completion--overlay)
+
+
+(defun lsp-inline-completion-show-keys ()


nit: is this called from somewhere? Should we make this a command instead of just a function?

kiennq · 2024-11-28T04:23:08Z

lsp-inline-completion.el

+            (setq lsp-inline-completion--start-point (point))
+            (lsp-inline-completion-show-overlay))
+        (unless implicit
+          (message "No Suggestions!")))


nit: lsp--info or lsp--warn?

kassick added 7 commits November 19, 2024 16:08

feat: Inline Completion support

5caf44c

Only the parser

feat: Inline Completion default UI

7227d67

feat: do not consume the key used to cancel the current inline comple…

33025a0

…tion

feat: better positioning of the cursor when the inline completion is …

9c5fc21

…displayed

chore: do not warn of no suggestions when implicit

2b9d47d

feat: provide a minor mode for auto activating inline completions

5ee78c1

kassick force-pushed the feat/inline-completions branch 5 times, most recently from bf4ab0a to 71c81dd Compare November 25, 2024 14:45

kassick added 2 commits November 25, 2024 12:45

chore: avoid linter errors

b4f0c7c

feat: better cursor positioning on accept

f5b572b

kassick force-pushed the feat/inline-completions branch from 71c81dd to f5b572b Compare November 25, 2024 15:49

fix: forward prefix arg

f9783fd

kassick force-pushed the feat/inline-completions branch from 9dea79c to f9783fd Compare November 25, 2024 16:00

fix: wrong function name

828875b

kassick force-pushed the feat/inline-completions branch from c32fa58 to 828875b Compare November 25, 2024 16:07

kassick added 3 commits November 25, 2024 13:13

fix: wrong number of arguments

d3ff223

chore: use cl-map to avoid linting issues

8f13826

fix: avoid weird interactions with company-mode

1d4b7c8

Check if company is active before displaying the overlay -- hide it if so

kassick added 4 commits November 26, 2024 15:49

refactor: split parse and request -- may be handy in async

063fd89

fix: use current active map instead of global map

5f67185

fix: add external references

3bab211

kassick force-pushed the feat/inline-completions branch from 04fa88a to da2d8c2 Compare November 26, 2024 18:50

chore: remove debug

64e6a77

kiennq reviewed Nov 27, 2024

View reviewed changes

kassick added 15 commits November 27, 2024 11:40

chore: keep interfaces sorted in lsp-protocol.el

a1e7d7d

refactor: declare and use InlineCompletionParams and InlineCompletion…

92281ed

…Context

fix: do not assume that the server response is a hash table

55d086a

Also, refactor how we parse the response, using pcase for a more explicit approach

refactor: move lsp-inline-completion-mode outside of lsp-mode.el

0e81d72

refactor: extract company integration to a minor mode

a74edd4

Users can enable the lsp-inline-completion-company-integration-mode if they see fit. Other completion frontends may be customized via hooks, just as this minor mode does

refactor: use autoload to configure inline completions mode

80e210a

fix: ensure the callback is still executing in the context of the ori…

3c43167

…ginal buffer

chore: use local variables for the company integration minor mode

612e90e

chore: remove ht as dependency

b19a8f1

fix: variable name

231b867

fix: define minor mode before the functions that need to check for it

49895b7

chore: add missing type to custom variable

312626e

fix: declare company--begin-inhibit-commands as a variable

79dd07c

chore: do not use make-variable-buffer-local

e20df27

fix: use cl-map to make the compiler happy

f2b38e0

kiennq reviewed Nov 28, 2024

View reviewed changes

kiennq approved these changes Nov 28, 2024

View reviewed changes

kiennq reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: inline completions #4623

Feature: inline completions #4623

kassick commented Nov 25, 2024

kassick commented Nov 26, 2024

kiennq Nov 27, 2024

kassick Nov 27, 2024 •

edited

Loading

kiennq Nov 27, 2024

kassick Nov 27, 2024

kiennq Nov 27, 2024

kassick Nov 27, 2024

kiennq Nov 27, 2024

kiennq Nov 27, 2024 •

edited

Loading

kiennq Nov 27, 2024

kassick Nov 27, 2024

kiennq Nov 28, 2024

kiennq Nov 27, 2024

kassick Nov 27, 2024

kiennq Nov 27, 2024

kassick Nov 27, 2024

kiennq Nov 28, 2024

kiennq commented Nov 27, 2024

kiennq Nov 28, 2024

kiennq Nov 28, 2024

kiennq Nov 28, 2024

kiennq Nov 28, 2024

kiennq left a comment

kiennq Nov 28, 2024

kiennq Nov 28, 2024


		(lsp-inline-completion--clear-overlay)

		(setq lsp-inline-completion--showing-company

		lsp-inline-completion--overlay)


		(defun lsp-inline-completion-show-keys ()

Feature: inline completions #4623

Are you sure you want to change the base?

Feature: inline completions #4623

Conversation

kassick commented Nov 25, 2024

kassick commented Nov 26, 2024

Choose a reason for hiding this comment

kassick Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiennq Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiennq commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiennq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kassick Nov 27, 2024 •

edited

Loading

kiennq Nov 27, 2024 •

edited

Loading