Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce lsp-completion-passthrough-try-completion #4544

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Introduce lsp-completion-passthrough-try-completion #4544

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion lsp-completion.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,20 @@ The CLEANUP-FN will be called to cleanup."
"Disable LSP completion support."
(lsp-completion-mode -1))

(defun lsp-completion-passthrough-try-completion (string table pred point)
(let* ((completion-ignore-case t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to assign this to t? I think we should just use the user's preference value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS lsp-mode doesn't use this variable when matching: the list of completions is constructed case-insensitively, at least with the language server that I'm currently testing with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's right. lsp-mode is using string-match, so this should be bound to case-fold-search instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Well, when you do string matching in completion, I think you're expected to bind case-fold-search to completion-ignore-case around that logic.

Or just go with the value t, like I think happens now most of the time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I can bind the case-fold-search to completion-ignore-case in another PR, or if possible, can you help to bind it in this same PR if possible :).
I think there's only two places in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that case-fold-search is t by default, whereas completion-ignore-case is nil by default.

Obeying the user-set completion-ignore-case would make lsp-mode's completion case-sensitive. This is not bad (Eglot currently does that), but it's a significant change in behavior, I think, and it'll make it less "passthrough".

So, are you sure you want that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we should change that in other places to make it conforms with completion-ignore-case. In this passthrough function, just returns (cons string point) should be okay. It will be similar to VsCode.
@yyoncho, let me know if you have any concern

Copy link
Contributor Author

@dgutov dgutov Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this passthrough function, just returns (cons string point) should be okay.

Like mentioned, I'd like to keep the change in behavior in this PR to a minimum. After the merge, this code would be equivalent to the latest Eglot in Emacs master where I had the same goal.

Except Eglot does case matching for prefix (by default) - and lsp-mode ignores case. Not sure which is better but both of the corresponding behaviors seem fit the corresponding package' goals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might very well do that change in the next edit, though (if there is agreement among the maintainers).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I signed off for this change. Let's get another signoff and we can merge

(try (completion-basic-try-completion string table pred point))
(newstr (car try))
(newpoint (cdr try))
(beforepoint (and try (substring newstr 0 newpoint))))
(if (and beforepoint
(string-prefix-p
beforepoint
(try-completion "" table pred)
t))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of t, can we use completion-ignore-case here?

try
(cons string point))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a passthrough try completion, can we just return the (cons string point) instead of using try-completion here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible - then "expand common" will always just keep the current input.

That seems like a bit of a loss from my POV, but VS Code never supported this anyway, so 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, the basic-try-completion is also matching prefix anyway, and in case of advanced language server that do the fuzzy match by themselves, I don't think we want to be smart and step in to decide where is the common prefix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my main feedback, for other comments, feel free to won't fix them.


(defun lsp-completion-passthrough-all-completions (_string table pred _point)
"Passthrough all completions from TABLE with PRED."
(defvar completion-lazy-hilit-fn)
Expand Down Expand Up @@ -790,7 +804,7 @@ The CLEANUP-FN will be called to cleanup."
(setf (alist-get 'lsp-capf completion-category-defaults) '((styles . (lsp-passthrough))))
(make-local-variable 'completion-styles-alist)
(setf (alist-get 'lsp-passthrough completion-styles-alist)
'(completion-basic-try-completion
'(lsp-completion-passthrough-try-completion
lsp-completion-passthrough-all-completions
"Passthrough completion."))

Expand Down
Loading