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

API: CompletionItem: add filterText field #6510

Open
ryuukk opened this issue Oct 7, 2024 · 8 comments
Open

API: CompletionItem: add filterText field #6510

ryuukk opened this issue Oct 7, 2024 · 8 comments

Comments

@ryuukk
Copy link

ryuukk commented Oct 7, 2024

Problem description

Right now, to support LSP's label detail, SublimeLSP puts the detail inside the trigger, wich means SublimeText will use garbage data during filtering

This is not good

image

You can see, the perfect match is always at the bottom, because other fields contains useless data

Same server using vscode:

image

Preferred solution

 completion = sublime.CompletionItem(
         trigger,
         annotation,
         sublime.COMPLETION_FORMAT_COMMAND,
         kind,
         details=" | ".join(details),
+        filterText= filter,
     )

Alternatives

I can't think of any

Additional Information

No response

@rchl
Copy link

rchl commented Oct 7, 2024

To give some more context from the LSP spec:

	/**
	 * A string that should be used when filtering a set of
	 * completion items. When omitted the label is used as the
	 * filter text for this item.
	 */
	filterText?: string;

@jwortmann
Copy link

jwortmann commented Oct 7, 2024

I think there is a misunderstanding here.

What your shreenshot indicates is missing support for the CompletionItemLabelDetails from the LSP specs, in particular CompletionItemLabelDetails.detail:

	/**
	 * An optional string which is rendered less prominently directly after
	 * {@link CompletionItem.label label}, without any spacing. Should be
	 * used for function signatures or type annotations.
	 */
	detail?: string;

SublimeLSP currently uses heuristics to decide whether it can append CompletionItemLabelDetails.detail to the sublime.CompletionItem.trigger or not: https://github.com/sublimelsp/LSP/blob/d71e11a9d428462a5994508f7891914c3fa74258/plugin/completion.py#L69-L92
In that case it is indeed used for filtering, but with the current abilities of the CompletionItem API there is nothing we can do about it. The alternative would be to never show the CompletionItemLabelDetails.detail in Sublime's completion popup.

The re-sorting of the filtered items is a different issue. I would expect ecs_id_t to be given higher priority after filtering than the other items from your screenshot; all of them have the same number of matched characters, but ecs_id_t has the best locality of the matched characters. Perhaps the sublime.INHIBIT_REORDER which is used at https://github.com/sublimelsp/LSP/blob/d71e11a9d428462a5994508f7891914c3fa74258/plugin/completion.py#L248-L249 has an influence here.


As far as I understand, the filterText from the LSP specs has a different purpose; its purpose seems to be to prevent items from being filtered out even if they don't contain the already typed characters. See the example and screenshot at microsoft/language-server-protocol#898 (comment) where the user typed b and the item with foo-text-range-replace is still shown in the completion popup despite not containing any b.

Completion Item with text edits: in this mode the server tells the client that it actually knows what it is doing. If you create a completion item with a text edit at the current cursor position no word guessing takes place and no filtering should happen. This mode can be combined with a sort text and filter text to customize two things. If the text edit can is a replace edit then the range denotes the word used for filtering. If the replace changes the text it most likely makes sense to specify a filter text to be used.

Maybe this is not the best example because the other item foo-text-range-insert is also still shown in the completion popup. That one has a TextEdit but without any filterText, which means that it is intended to always and unconditionally be shown in the popup (I can't think of any real-world use case for that).

Anyways, since Sublime doesn't have the ability to distinguish between what should be displayed (label and labelDetails.detail) and what should be used to filter (filterText if available, otherwise label), SublimeLSP uses the filterText as the trigger if it exists but the label is not compatible (in the sense that it doesn't start) with filterText: https://github.com/sublimelsp/LSP/blob/d71e11a9d428462a5994508f7891914c3fa74258/plugin/completion.py#L85

The heuristics are probably not perfect and there is more context to read about in e.g. sublimelsp/LSP#2189 ... but it is complicated and even not all language servers can comply how to use the various fields in a consistent way.

But I think even having direct support for a filterText in Sublime Text might in general not help you with your example from the screenshot. (Perhaps it could be (ab)used to be filled with the label if the LSP filterText is not given - then it could work)

@ryuukk
Copy link
Author

ryuukk commented Oct 7, 2024

You are the one misunderstanding the issue here, don't try to minimize the issue

What your shreenshot indicates is missing support for the CompletionItemLabelDetails

That's the whole point, to dissociate what's being displayed from what's being used to decide what to filter

Because ST doesn't have a way to render extra detail next to the label, SublimeLSP is merging both, here, this is a HACK, we are feeding undesired data to the CompletionItem, only to be able to display the extra information from the outside world, and as a result filtering looks bad

The heuristics are probably not perfect and there is more context to read about

The heuristics is not the problem here, it is good, we just feed him undesired data

labelDetails can contain all kind of data, including non-code, so a filterText is needed, or a new field to display extra info next to the label, or perhaps both

@jwortmann
Copy link

You are the one misunderstanding the issue here, don't try to minimize the issue

I'm not minimizing the issue and I'm not the one who is misunderstanding it. You're using a language server which uses CompletionItemLabelDetails.detail, but you are asking for filterText. And in your preferred solution you put filter as filterText:

 completion = sublime.CompletionItem(
         trigger,
         annotation,
         sublime.COMPLETION_FORMAT_COMMAND,
         kind,
         details=" | ".join(details),
+        filterText= filter,
     )

But what is filter? If you would use the filterText from the LSP specs, which would be the obvious approach that comes into mind first, then it won't help you if your language server doesn't provide it (sublimelsp/LSP#1924 (comment)). As I wrote above it might work if we put the label instead as a workaround then, but this would only be a special case if LSP's filterText is not given, because in general filterText can be anything (only useful to be defined if it is different from label).

Because ST doesn't have a way to render extra detail next to the label, SublimeLSP is merging both, here, this is a HACK, we are feeding undesired data to the CompletionItem, only to be able to display the extra information from the outside world, and as a result filtering looks bad

This was done primarily because of a request from you: sublimelsp/LSP#1924

Regarding sorting you could try to remove the lines at https://github.com/sublimelsp/LSP/blob/d71e11a9d428462a5994508f7891914c3fa74258/plugin/completion.py#L248-L249 and test whether it works better then. I don't know the purpose why this is used and I think I've never touched those lines.

@deathaxe
Copy link
Collaborator

deathaxe commented Oct 7, 2024

To be fair filterText was indirectly proposed by @rchl on discord, so please keep polite to keep the discussion constructive.

Both proposals would be a possible solution, more or less.

I like the details approach to display function signatures only for selected completion items as it significantly reduces noise.

I wonder how it would cooperate with annotations.

I also see possible benefits of a filterText, though I wonder if it on the other hand might cause some confusion if items stay in the list which absolutely do not share any character visually with typed content.

@predragnikolic
Copy link

Exposing an API to LSP-* packages to specify what should be displayed in the details section or annotation section could also address the reported issue.

Related sublimelsp/LSP#2467

@deathaxe
Copy link
Collaborator

deathaxe commented Oct 7, 2024

The point however is whole trigger being subject of fuzzy search and in combination with some sorting flags or settings causing high chances for just garbage to be filtered/selected.

I've also been hit by ST absolutely refusing to select items I want for no obvious reason.

@ryuukk
Copy link
Author

ryuukk commented Oct 10, 2024

This was done primarily because of a request from you: sublimelsp/LSP#1924

I didn't request anything, i reported an issue saying that it doesn't support LSP's latest spec

But glad to see that since 2021, ST has done nothing to improve the situation

Regarding sorting you could try to remove the lines at https://github.com/sublimelsp/LSP/blob/d71e11a9d428462a5994508f7891914c3fa74258/plugin/completion.py#L248-L249 and test whether it works better then. I don't know the purpose why this is used and I think I've never touched those lines.

Removing that puts ecs_id_t first, but now it completely ignores server sorting, there is no better solution than proper filter text or the ability to display extra information

But considering nothing was done since 2021, i'd suggest them to do the minimum, the filter text, so we'll get an improvement sooner rather than later, or am i asking too much already?

Anyways, i get annoyed with tech, when it's not the language, it's the tools, something wrong with me perhaps

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

No branches or pull requests

5 participants