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

LSP Completion preselect and preview doc #2705

Closed

Conversation

lazytanuki
Copy link
Contributor

Hi !

This PR implements two completion features that are present in other LSP enabled editors, regarding preselection :

Support LSP preselect completion items

In addition to suggesting completion, some LSP servers can tell the client which suggestions would be the most probable fit. For instance, if you want to assign a value to a variable named position, RA will preselect a method that is also named position() if it is part of the suggested items. The LSP server can mark multiple completion items as preselect. This PR makes these preselected items appear at the top of the completion menu.

This behavior is configurable with the editor.lsp.preselect field, which defaults to true.

Preview the documentation of the first completion item without selecting it

When using preselect, having a live preview of the documentation can be nice to some users. This PR introduces an option to have the documentation of the first completion item show up while typing, without necessarily selecting it.

This behavior is configurable with the editor.completion-doc-preview field, which defaults to false.

The first item can also be highlighted, otherwise it can look confusing. The corresponding highlight is ui.menu.highlighted.

To implement this, I had to add a highlighted feature to the Menu component, distinct from the selected one, which could be useful for other stuff later.

Here is a little demo that shows both of the features enabled on top, and disabled below:

Peek 2022-06-08 01-15

I can add the highlight to the default themes if the PR goes through.

Thanks and have a good evening !

@lazytanuki lazytanuki changed the title Completion preselect LSP Completion preselect and preview doc Jun 8, 2022
@archseer
Copy link
Member

Do we need to color the item differently? I was thinking we could just sort it as first.

Also not sure if it's necessary to open the document ahead of time, is this something VSCode does?

@lazytanuki
Copy link
Contributor Author

These are actually two separate features that I put in this PR (sorry about the confusion):

The first one, which is on by default, only retrieves and puts the LSP preselected item first, as you suggested.

The second one, which is off by default, previews the documentation of the first item. I found it necessary to highlight the item for which the documentation is shown because it more clearly shows that the documentation shown is the one of the first item. It felt odd without it.
This second feature is purely opt-in, I don't know if VS code does that but I find it nice because it allows to view the doc of what you type without needing to press Tab. I've been using this feature in neovim (using nvim-cmp) and grew to really like it.

@lazytanuki
Copy link
Contributor Author

Hi !

Just rebased this on master, could this get a review or some feedback ?

Thanks !

@lazytanuki
Copy link
Contributor Author

Just rebased after all the changes made by #2814

Comment on lines +18 to +22
pub enum SortStrategy {
Text,
Score,
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit wary of adding more strategies. How about Item implements fn score(matcher: &...) and the default implementation just calls matcher.fuzzy_match? Then to override for preselected items, the Item trait implementation for CompletionItem could return 0 if preselected.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also remove the preselected feature and just always pre-select.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the preselect config option to always use the LSP preselected item. Is this what you meant by your second comment ?

However, I am not quite sure I follow with the first one ! I don't see how having Item implement fn score(matcher: &...) would help get rid of the SortStrategy enum.

I use the SortStrategy enum at the menu level. It was necessary because as of now, the Menu only sorts alphabetically (the code to sort by score is commented out in master). This makes sense for code actions (the only other instance of Menu in use), but not for completion items that need to be sorted according to their fuzzy match score. As of now, completions items are just filtered (matches the pattern or not) and are then sorted alphabetically.
Moreover, the score given to an item depends of the prompt pattern being empty or not, so it cannot be contained in the Item trait implementation. That way, when the completion menu pops open, the user sees the preselected items first, but then when they type anything, they see the items sorted by score.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, so this is a change unrelated to preselect/preview doc (and is there to fix code action sorting)? Can you split it out into a separate PR to discuss there instead?

Moreover, the score given to an item depends of the prompt pattern being empty or not, so it cannot be contained in the Item trait implementation.

You could pass in both the matcher and the current pattern into the scoring function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I just opened #3215 for the sorting strategy. I'll rebase this one on master once merged 😉

Copy link
Member

Choose a reason for hiding this comment

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

Oh I mean I hoped to merge this PR without requiring to land the sort strategy. If preselect depends on it then yeah it's OK to review together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least it'll make a clearer diff once rebased, because with rustfmt reindenting big portions of the code, the few modifications to add sort-strategies made a huge diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants