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

Limit completions to top 40 #1218

Merged
merged 11 commits into from
Jan 18, 2021
Merged

Limit completions to top 40 #1218

merged 11 commits into from
Jan 18, 2021

Conversation

pepeiborra
Copy link
Collaborator

We are overwhelming the LSP client by sending 100s of completions after the first character. In VSCode this is noticeable - occasionally the UI freezes for a second or so while displaying the completions popup.

Instead, let's send 20 at a time and refresh for more when the user types another word - VSCode no longer freezes after this change.

We are overwhelming the LSP client by sending 100s of completions after the
first character. Instead, let's send 20 at a time and refresh for more when the
user types another word
@jneira
Copy link
Member

jneira commented Jan 17, 2021

could we do some sorting to send the most used ones?

@pepeiborra
Copy link
Collaborator Author

We don't have any usage statistics, so I don't see how

@Ailrun
Copy link
Member

Ailrun commented Jan 17, 2021

It would be really frustrating if autocompletion does not display what I want just because there are 20 similarly named functions... (say I want getXXXX, then there are probably many functions starting with get, especially in an import-heavy module...)

@ndmitchell
Copy link
Collaborator

It would be really frustrating if autocompletion does not display what I want just because there are 20 similarly named functions...

If its not in the top 20, are you going to scroll down the list repeatedly (unlikely) or keep typing characters until the one you want gets to the top (almost certainly).

I would ask why 20 though - where does VS Code start to get noticably slow? 20 feels relatively low to me (sometimes I do X. to export the exports of X), whereas 50 seems more than anyone would need.

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Jan 17, 2021

Note that as soon as you type an additional character the completions are refreshed. I picked 20 because it seems unreasonable to scroll more than 20 lines, but given that completions can also serve as hints I think it's reasonable to increase this to 50 or even 100.

EDIT: Actually, VSCode popup only ever shows 12 lines, so I think 20 is reasonable

Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Should we have a test for this?

ghcide/src/Development/IDE/Plugin/Completions.hs Outdated Show resolved Hide resolved
@Ailrun
Copy link
Member

Ailrun commented Jan 17, 2021

If its not in the top 20, are you going to scroll down the list repeatedly

Or wrapping arround to directly go to the bottom of the list. I often do such a wrapping move. (I don't know this is applicable to VSCode or other editors, though)

@pepeiborra
Copy link
Collaborator Author

If its not in the top 20, are you going to scroll down the list repeatedly

Or wrapping arround to directly go to the bottom of the list. I often do such a wrapping move. (I don't know this is applicable to VSCode or other editors, though)

Just type another character and will get better suggestions. I've increased the limit to 40, maybe you can contribute a change to make it user customisable?

A freezing UI is a much worse experience than a limited amount of suggestions, so I think the current tradeoff is justified

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Jan 17, 2021
Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the adjustment!

@ndmitchell
Copy link
Collaborator

Do we have documentation of all our configs, what they do, what they mean etc? If yes, you probably need to note it there. If not, should we add a ticket to create such a document?

@pepeiborra
Copy link
Collaborator Author

I don't think that such a document exists, no. /cc @jneira

@pepeiborra pepeiborra changed the title Limit completions to top 20 Limit completions to top 40 Jan 17, 2021
@Anrock
Copy link
Contributor

Anrock commented Jan 17, 2021

It's documented in readme. Not sure how complete and correct documentation is, tho.

@ndmitchell
Copy link
Collaborator

@pepeiborra - would be good to document this new flag in the readme.

@Ailrun
Copy link
Member

Ailrun commented Jan 18, 2021

It looks like eval plugin test is really unstable... Maybe it's the case that we should replace eval parser with the actual GHC parser.

@mergify mergify bot merged commit b1d912a into master Jan 18, 2021
@michaelpj
Copy link
Collaborator

I wonder if we used the streaming results support whether that would help with the freezing problem?

@pepeiborra
Copy link
Collaborator Author

I wonder if we used the streaming results support whether that would help with the freezing problem?

what's the streaming results support?

@michaelpj
Copy link
Collaborator

Returning partial results. I became aware of it from this lsp-mode issue.

However, looking at it a bit more, I take it back. I think the intention is more to allow servers to return results faster, rather than to allow sending smaller batches to avoid overloading the client.

@pepeiborra
Copy link
Collaborator Author

It's not very widely implemented either, not even by VSCode

@mithrandi
Copy link

Note that as soon as you type an additional character the completions are refreshed.

My experience with emacs is that they are only refreshed once the list of matching completions becomes empty. The limiting also interacts poorly with some matching mechanisms; eg. to match getSystem I would normally type something like gsym but this doesn't often work now due to getSystem being too far down the results for g.

@pepeiborra
Copy link
Collaborator Author

Your experience with previous versions of HLS/ghcide? Then that's consistent with the expected behaviour, since we were telling the LSP client that the list of completions provided was "complete" and didn't need refreshing.

Re your example with getSystem, that's probably explained by fuzzy matching now happening in the server (ghcide) rather than in the client (Emacs). You should be able to tweak the fuzzy matching we use (in ghcide/src/Development/IDE/Plugin/Completions/Logic.hs) to behave how you'd like it, or to expose some config options, and send a PR.

Finally, since the maxCompletions value is a setting, you can override the default to whatever you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants