render completion options by language server priority #7349

estin · 2023-06-15T19:10:50Z

Hi!
On multi ls with completion items rendered without any priority.
This PR groups completion items by language server id.
And in future allows to do some visualization to separate groups

For my use case (on python) with follow config

[language-server]
simple-completion-language-server = { command = "simple-completion-language-server" }

[[language]]
name = "python"
language-servers = [ "pylsp", "simple-completion-language-server" ]

on master, I expects items from pylsp on top of the list, for me it primary ls for completion

on this PR, all as I expected, on top items from pylsp and on bottom from simple-completion-language-server

Don't sure about it's nature behavior for other users.
Feel free to close this PR.
Sorry for my poor English.

poliorcetics · 2023-06-16T13:58:24Z

LGTM, though what is simple-completion-language-server ? A simple text search doesn't give me much results in duckduckgo or github

poliorcetics · 2023-06-16T13:59:21Z

It may have to be a config option, but I'm unsure about that, let's wait for maintainers :)

estin · 2023-06-16T14:11:34Z

@poliorcetics it's https://github.com/estin/simple-completion-language-server
simple and buggy solution for lexical completion #3328

poliorcetics · 2023-06-16T14:13:28Z

Thank you very much, I'll have a look!

the-mikedavis · 2023-06-16T14:47:18Z

helix-term/src/ui/completion.rs

+    #[inline]
+    fn group(&self, _data: &Self::Data) -> usize {
+        self.language_server_id
+    }


I can't think of other places we would want to re-use this group concept. Ideally I'd like to see this built in to the sort_text function: something like fn sort_key(&self, data: &Self::Data) -> impl Ord and then CompletionItem's Item implementation would contain the logic for using self.language_server_id. That depends on either rust-lang/rust#91611 to return an impl Ord, or rust-lang/rust#29661 so that we wouldn't have to explicitly implement sort_key in every Item:

pub trait Item { type Data; type SortKey: Ord = Cow<str>; // ... fn sort_key(&self, data: &Self::Data) -> Self::SortKey { self.filter_text() } // ... }

Maybe there's a clever way around the language features here that I'm not seeing though?

There was a similar thread of this idea in #2705 (comment).

While making this an assoc type would be annoying I think it's not the worst thing having to explicitly specify a type

removed group concept.

introduced Item.score function to control score and sorting

helix-term/src/ui/completion.rs

pascalkuthe · 2023-06-16T19:23:53Z

helix-term/src/ui/completion.rs

+    #[inline]
+    fn group(&self, _data: &Self::Data) -> usize {
+        self.language_server_id
+    }


While making this an assoc type would be annoying I think it's not the worst thing having to explicitly specify a type

pascalkuthe · 2023-06-17T22:20:47Z

I dont think this needs a config option since it only acts as a tie breaker where the order is semi random right now

helix-term/src/ui/menu.rs

helix-term/src/ui/completion.rs

archseer · 2023-11-18T01:32:36Z

helix-term/src/ui/menu.rs

+    /// Second ordering rule on equals items score
+    type TieBreaker;
+


I think you should be able to add a default value here so we don't have to define it in every single implementation.

archseer · 2023-11-18T01:35:52Z

helix-term/src/ui/menu.rs

@@ -29,30 +32,36 @@ pub trait Item: Sync + Send + 'static {
        let label: String = self.format(data).cell_text().collect();
        label.into()
    }
+
+    fn tie_breaker(&self, _data: &Self::Data) -> Self::TieBreaker;


But I don't like having this extra function, it should be unnecessary because this is the intent of sort_text (should really be renamed as sort_key).

Let's try modifying the return value of sort_text as K: Ord, and use a default that uses Cow<str>. Then for your one specific implementation you can instead return (label, language_server_id) ((Cow<str>, usize))

I agree we don't want an extra method. I am not sure what the right API is here though.

A couple points:

for the picker the concept of sort text does not make sense IMO. In general I think the needs of the picker and of the menu keep diverging (some of mikes colum related work for example) so I think we will likely just endup with two traits (or even something totally different for the picker)

for the menu I am not sure how much sense all of this actually makes. It feels wrong to me to sort by language server first since fuzzy matching is not very selective so you would see all the "bad" completions from one server before those from a second server. A score bonus is not a good solution because it's hard to implement (how large should the bonuss shpuld be is subjctive and would depend on word length/content) and ultimately not feel predictable. I proposed making it a tie breaker but exactly identicsl fuzzy scores are not as common as you would think fue to the fairly complex bpnus system. It's probably the least bad option right now.

all of this doesn't even take into account yet that we do not actually want to sort for a lot of servers. With incomplete completion request support we have to rerequest completions on every keypress and are not supposed to do filtering and scoring ourself. The question then becomes how to mix those servers with other completion options. The server sorts internslly according to their own criteria and scoring system which we are unaware of and also not comparable. This feature has a really large impact and is very commonly used (and the capability is not respected, for example by RA) so I see it as fairly high priority so such an obvious conflict here is a concern for me.

This makes it hard for me to review since I dont understand the full problem space yet. We may endup needing a very different solution and changing the trait before we have figured that out feels premature. Perhaphs sorting by server first works after all if we filter items with a score below 50% of the top score or something. Ideally we should study what other ides (primarily vscode) do here. There is also a whole can of worms of super long LSP issue trackers where it was discussed how filtering and sorting should work exactly (which can sometimes not have a clear co conclusion, I need to read trough all that carefully again).

One thing I am fairly sure about is that we want this to be (language_server, label) and not the other way around. Score ties are already rare (which is the only situation where sort_text matters) and when they do occur we want to group by language sever first (otherwise the language sever comparison basically never matters anyway).

One thing I am fairly sure about is that we want this to be (language_server, label)

I was assuming that too, but that probably doesn't make sense either since you could have very weak matches from your priority server that would now appear before better matches from a second one.

Ah no that actually would not happen (currently). Sort text is not used for fuzzy matching. It's only used as a tie breaker if the fuzzy match score is equal.

So sorting would be (fuzzy_score, language_server, label) so the order of these two only takes effect when the score is the same (which is already pretty rare)

estin · 2024-01-14T12:31:28Z

Now PR makes next changes:

populate completion menu item by lsp definition order
remove duplicated code

estin · 2024-02-10T14:13:08Z

rethink on #9590

the-mikedavis reviewed Jun 16, 2023

View reviewed changes

pascalkuthe reviewed Jun 17, 2023

View reviewed changes

helix-term/src/ui/menu.rs Outdated Show resolved Hide resolved

estin force-pushed the menu-item-ls-priority branch from a29de2c to 08b39a2 Compare June 25, 2023 21:01

kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 30, 2023

estin force-pushed the menu-item-ls-priority branch from bb8c355 to 7a85d52 Compare August 5, 2023 18:51

estin force-pushed the menu-item-ls-priority branch from 7a85d52 to 754b66b Compare August 30, 2023 11:55

estin requested a review from pascalkuthe August 30, 2023 12:13

pascalkuthe reviewed Aug 31, 2023

View reviewed changes

helix-term/src/ui/completion.rs Outdated Show resolved Hide resolved

estin force-pushed the menu-item-ls-priority branch from a87489a to 3b4aa5b Compare September 7, 2023 06:10

estin requested a review from pascalkuthe September 11, 2023 05:58

estin force-pushed the menu-item-ls-priority branch from 3b4aa5b to 90ae281 Compare October 3, 2023 14:40

estin force-pushed the menu-item-ls-priority branch from 90ae281 to 411a655 Compare October 17, 2023 06:26

estin force-pushed the menu-item-ls-priority branch 2 times, most recently from 0397c6b to c6525d0 Compare November 13, 2023 08:14

estin force-pushed the menu-item-ls-priority branch from c6525d0 to 56e4302 Compare November 17, 2023 12:45

archseer requested a review from the-mikedavis November 18, 2023 01:28

archseer reviewed Nov 18, 2023

View reviewed changes

estin force-pushed the menu-item-ls-priority branch from 56e4302 to 59bc4ad Compare December 19, 2023 06:43

respect lsp definition order for completion menu

83242b7

estin force-pushed the menu-item-ls-priority branch from 59bc4ad to 83242b7 Compare January 14, 2024 12:25

estin closed this Feb 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

render completion options by language server priority #7349

render completion options by language server priority #7349

estin commented Jun 15, 2023

poliorcetics commented Jun 16, 2023 •

edited

Loading

poliorcetics commented Jun 16, 2023

estin commented Jun 16, 2023

poliorcetics commented Jun 16, 2023 •

edited

Loading

the-mikedavis Jun 16, 2023

pascalkuthe Jun 16, 2023

estin Jun 26, 2023

pascalkuthe Jun 16, 2023

pascalkuthe commented Jun 17, 2023 •

edited

Loading

archseer Nov 18, 2023

archseer Nov 18, 2023

pascalkuthe Nov 18, 2023

archseer Nov 18, 2023

pascalkuthe Nov 18, 2023

estin commented Jan 14, 2024

estin commented Feb 10, 2024

		/// Second ordering rule on equals items score
		type TieBreaker;

render completion options by language server priority #7349

render completion options by language server priority #7349

Conversation

estin commented Jun 15, 2023

poliorcetics commented Jun 16, 2023 • edited Loading

poliorcetics commented Jun 16, 2023

estin commented Jun 16, 2023

poliorcetics commented Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe commented Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estin commented Jan 14, 2024

estin commented Feb 10, 2024

poliorcetics commented Jun 16, 2023 •

edited

Loading

poliorcetics commented Jun 16, 2023 •

edited

Loading

pascalkuthe commented Jun 17, 2023 •

edited

Loading