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

feat: Improved completion sorting that takes into account the current word and prefix (strict then camel-hump) #705

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SCWells72
Copy link
Contributor

@SCWells72 SCWells72 commented Dec 14, 2024

Opening this initially as a draft PR, but this brings over some of the completion ordering enhancements that I've added to the completion contributors in my own plugin. The idea is that the initial offered completions should be as close to what the user is wanting as possible given the available information at the time that completions are triggered.

First, completion sorting now takes into account configured language case sensitivity.

Next, if completions are triggered in the context of an existing identifier, any matches for that identifier should be listed first with exact matches (again, taking into account case-sensitivity) before starts-with matches:

image

Next, the prefix is considered with exact prefix matches listed before camel-hump prefix matches:

Exact match
image

Camel-hump match
image

Here's another example from CSS since all of the above are from TypeScript:

image

If this looks good -- taking into account feedback, of course -- I'll update CompletionItemComparatorTest with comprehensive unit tests for the new comparator logic.

@SCWells72 SCWells72 marked this pull request as draft December 14, 2024 16:17
…ng completion offerings. Removed current word "contains" checks because they could short-circuit more accurate prefix checks.
@SCWells72
Copy link
Contributor Author

SCWells72 commented Dec 14, 2024

@angelozerr mentioned that sorting should be using sortText, so it's worth providing context on why the new contextual aspects of sorting use label instead:

You expressly don't want to do that because it defeats the entire point of the changes I made.

The problem you're solving is that you're trying to insert some word of some sort, typically a keyword. If you sort by the sort text -- which is often something like an integer value as shown in the image below (being sorted as text which has its own issues) -- the resulting sort order doesn't take into account insertion context at all, e..g:

image

If you look at how IntelliJ IDEA and other JetBrains IDEs sort completions, they take into account the same things I am here, but also things like assignability and declaration proximity which we don't have (easily) from LSP, and of course also machine learning statistics around previously-selected completions in a given context.

With this change it now tries to sort by the keyword/identifier you're trying to insert by looking at the word that already exists at the point of insertion and/or the partial word prefix that's already been typed. And if none of that applies, it degrades gracefully to the previous sorting logic, albeit now with proper consideration of language grammar case-sensitivity.

The best way to get a feel for what this change does is to try it. Try completions with and without this change and see the difference in accuracy in terms of what you're actually trying to insert at the caret. I think you'll find that, with these changes, it's quite accurate, and without, you have to type more characters before the PrefixMatcher filter kicks in enough to narrow the offered completions down to what you originally wanted.

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

Successfully merging this pull request may close these issues.

1 participant