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.

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

Remove keywords from completions #259

Merged
merged 5 commits into from
May 31, 2020
Merged

Remove keywords from completions #259

merged 5 commits into from
May 31, 2020

Conversation

axelson
Copy link
Member

@axelson axelson commented May 25, 2020

They were being returned regardless of context which made them very noisy. Additionally the most important of these ("do/end") is typically inserted by an editor plugin.

Related:

They were being returned regardless of context which made them very noisy.

Related:
* elixir-lsp/elixir_sense#99
* #251
@axelson
Copy link
Member Author

axelson commented May 25, 2020

@msaraiva one downside to removing this is that now since there is no do completion if a user has "editor.acceptSuggestionOnEnter": "on" (which is the default), then they'll end up with bad text after typing do<enter>:

screenshot-do-autocomplete

Do you have any thoughts on how to work around that? Setting "editor.acceptSuggestionOnEnter": "off" is already in the README but I'm sure most users have not tweaked that setting. One solution is to remove most of the keyword but leave do.

cc: @lukaszsamson

@msaraiva
Copy link
Collaborator

Do you have any thoughts on how to work around that?

The simplest way I see is to keep adding do but only if the user has already written exactly "do". I wouldn't show if the hint is "d" or "".

@axelson
Copy link
Member Author

axelson commented May 26, 2020

Hmmm, in my testing that probably won't exactly work. When I type "do" I only see a completion request for "d" and not for "do", it seems like VSCode does client side filtering after the initial request. So instead perhaps we could include "do" if the user has typed "d" or "do".

@msaraiva
Copy link
Collaborator

You're right. I believe VS Code does client-side filtering in this case.

perhaps we could include "do" if the user has typed "d" or "do".

Yeah. We can try that.

@lukaszsamson
Copy link
Collaborator

it seems like VSCode does client side filtering after the initial request

Yes, it does. Currently any further keystrokes only filter the initial list. But I think we can change this with setting

isIncomplete: true

on CompletionList

From the spec

This list it not complete. Further typing should result in recomputing this list.

@msaraiva
Copy link
Collaborator

That's very interesting. I'm not sure how bad the performance could be impacted by setting isIncomplete: true, however, I believe it would solve the issue of the search being non-deterministic. Example:

Typing "t" and "e" gives us:

image

Hiting ctrl + space after "te" gives us:

image

The server uses a strict approach using starts_with. The client uses something closed to a fuzzy search.

So I guess we have 3 options, at least:

  1. Change the server to use the same flexible search of the client
  2. Set isIncomplete: true and force re-computing the list on every type
  3. Find a way to configure the client to also use start_with

Performance-wise, #3 would be perfect but I don't know if we can configure that.

@axelson
Copy link
Member Author

axelson commented May 30, 2020

Man, investigating the auto-complete filtering behavior in LSP was quite the rabbit hole. Here are the two most related issues:

A short summary of those two issues is that we should use isIncomplete: true if we want the client to continue fetching completions (it's a bit poorly named). Also we should be using editText instead of insertText (which is being tracked in #28).

1. Change the server to use the same flexible search of the client
2. Set `isIncomplete: true` and force re-computing the list on every type
3. Find a way to configure the client to also use `start_with`

I think 1 isn't really viable since different clients have different behavior, and similarly 3 isn't viable because we can't control that behavior via the protocol.

I think I'd like to investigate moving forward with isIncomplete: true. If the performance is too poor than we can try a new approach.

@axelson
Copy link
Member Author

axelson commented May 30, 2020

Okay, after some testing with isIncomplete = true, it is looking promising and since the client is no longer doing the fuzzy matching on the completions from d it's not using a completion like defoverridable.

@lukaszsamson
Copy link
Collaborator

LSP v3.16 introduces some improvements we can look into

Add support for insert and replace ranges on CompletionItem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants