Set completion trigger from label detail if present #2002

rchl · 2022-07-24T15:11:34Z

No description provided.

plugin/core/views.py

predragnikolic

👍

predragnikolic · 2022-07-25T04:49:23Z

Something I've noticed while testing this.
Tried it with TS 4.7.2 and the latest branch from theia ts ls.

Do you maybe know why I got 2 completion items for bar?

I wanted to check if I create function overloads, that I might see more completion items,
but that was not the case.

rchl · 2022-07-25T06:09:07Z

I believe it provides two completions on purpose since, depending on user's code, one might be preferred over another . I suppose it works the same in vs code.

Also it seems to be a pretty new feature so maybe it isn't supported in all possible cases in tsserver. Just guessing.

* main: Prevent failing to load all configs when one fails Refactor symbol and completion item kinds (#2000) Add Godot (GDScript) LSP instructions Add preview for resource files in LocationPicker

jwortmann · 2022-07-25T15:58:54Z

I'm not really convinced that this would be a good solution. There is already the CompletionItem.filterText (or CompletionItem.label if filterText doesn't exist) to be used for the "trigger" field of ST autocompletion items. I think the example with bar(x) is insufficient, because usually function signatures contain multiple parameters with longer names & types. So in practice the typed text will randomly fuzzy match the characters in the label details more often, so that filtering doesn't work as expected. Due to the lack of language servers with support for label details, I haven't tested it though, but I would expect bug reports for it when more servers start to support it. And it just feels wrong to me to misuse the "trigger" field for information which are not meant for filtering. Instead, we should adopt to the abilities of the ST autocompletion popup (either "annotation" or "details"), or create a feature request for it in the ST issue tracker, and as long as it isn't implemented use one of those existing fields.

And what if next time someone creates an issue like

deprecated CompletionItems not correctly implemented

CompletionItems with the "deprecated" property or with CompletionItemTag.Deprecated should be rendered using strike-through in the autocompletion popup. ST doesn't have the ability to do that, and as a workaround LSP currently uses a "!" as the icon letter and an additional mark in the "details" panel at the bottom for deprecated items. But instead, we can just add a dash after every other character in the "trigger" field, which would look a little bit like strike-through!

That would have even less to none impact for filtering, because usually completion items don't contain dashes.

Another solution to have the label details in the "trigger" field for a particular server, would be to use the new AbstractPlugin.on_server_response_async method to just replace the CompletionItem.filterText with label + label details for each completion item in the server response.

rchl · 2022-07-25T17:58:52Z

I'm not convinced yet that this is the proper solution (or that it's better than the old one) but if your main argument is that it affects filtering then it seems like it also does affect filtering in VSCode so maybe it's not that bad after all.

I'd need to see some specific case where it actually is noticeably worse before I'm convinced.

rchl · 2022-07-25T19:46:52Z

For the record here is another case. Two completions:

    {
      "commitCharacters": [
        ".",
        ",",
        "("
      ],
      "data": {
        "entryNames": [
          "banner"
        ],
        "file": "/usr/local/workspace/github/typescript-language-server/rollup.config.ts",
        "line": 13,
        "offset": 16
      },
      "filterText": "banner",
      "insertText": "banner",
      "kind": 5,
      "label": "banner?",
      "sortText": "12\u0000banner\u0000"
    },
    {
      "commitCharacters": [
        ".",
        ",",
        "("
      ],
      "data": {
        "entryNames": [
          {
            "name": "banner",
            "source": "ObjectLiteralMethodSnippet/"
          }
        ],
        "file": "/usr/local/workspace/github/typescript-language-server/rollup.config.ts",
        "line": 13,
        "offset": 16
      },
      "filterText": "banner",
      "insertText": "banner() {\n    $0\n},",
      "insertTextFormat": 2,
      "kind": 5,
      "label": "banner?",
      "labelDetails": {
        "detail": "()"
      },
      "sortText": "12\u0000banner\u00001"
    },

VSCode shows them as:

banner?
banner?()

ST with code in this PR as:

banner
banner

because it prefers filterText over label+label.details. We could use the latter but that would create a bit of inconsistency as we'd show ? in the second case but not the first one.

(Note that having ? in the trigger should not affect filtering as completion popup is canceled on pressing ? anyway but it's just this specific case that uses ?, other cases might use characters that ST can actually use for filtering)

jwortmann · 2022-07-26T15:23:00Z

but if your main argument is that it affects filtering then it seems like it also does affect filtering in VSCode so maybe it's not that bad after all.

Yeah, that is my main point and it surprises me that VSCode uses the label details for matching/filtering. Not sure if that is intended or could even be considered to be a bug in VSCode. Maybe we should ask in https://github.com/microsoft/language-server-protocol/issues to clarify whether the labelDetails.detail is meant to be used for matching/filtering.

At least I'm not the only one who is skeptical: #1924 (comment)

VSCode shows them as:

banner?

banner?()

ST with code in this PR as:

banner

banner

because it prefers filterText over label+label.details. We could use the latter but that would create a bit of inconsistency as we'd show ? in the second case but not the first one.

Hm.. maybe LSP should entirely ignore filterText, and always prefer label (possibly + label.details depending on the outcome of this PR). filterText seems to be used as an additional internal filter mechanism in VSCode, but isn't shown to the user in the completion item, as we do here.

I've also noticed an inconvenience with Pyright, that seems to be caused by using filterText for the "trigger": Pyright will show the parameter names of a function as keyword arguments in function calls, and they are always ordered at the top of the completion items. Let's say I have a function with parameter name param1, and also a variable with the same name that I want to use as argument when calling the function. Then Pyright will suggest two items with that name (and both have the same "variable" kind too), but we show the label param1= only in the details panel at the bottom. With this, I often end up to quickly select this first item, which unintendedly inserts the = suffix. Then I have to press backspace to remove it again. A bit annoying when it happens regularly. I guess that if LSP would show the full param1= in the "trigger", this situation would be improved.

As a Sidenote, I believe you can/should replace the _wrap_in_tags("p", lsp_label) by html.escape(lsp_label), because the p tag isn't supported in the completion item details anyway and will be ignored: https://www.sublimetext.com/docs/minihtml.html#inline-formatting

rchl · 2022-07-26T15:45:35Z

Yeah, that is my main point and it surprises me that VSCode uses the label details for matching/filtering.

I believe there might be something else going on [actually you've mentioned that later also...]. I think it actually uses a separate filterText completion property for matching. In the bar(x) case that is set to bar(x) {\n $0\n}, even.
(well, should clarify that this logic is for the typescript server - https://github.com/microsoft/vscode/blob/1a2c2b2916da6110ca2da0aad604a477a8aa03e9/extensions/typescript-language-features/src/languageFeatures/completions.ts#L99-L99)

At least I'm not the only one who is skeptical: #1924 (comment)

But that's two ST users who haven't even properly tried such change in practice for a decent amount of time so I'm not convinced yet. :)

Hm.. maybe LSP should entirely ignore filterText, and always prefer label (possibly + label.details depending on the outcome of this PR). filterText seems to be used as an additional internal filter mechanism in VSCode, but isn't shown to the user in the completion item, as we do here.

I suppose that we would have to see the effects of such change in practice while testing various servers.

I guess that if LSP would show the full param1= in the "trigger", this situation would be improved.

👍

As a Sidenote, I believe you can/should replace the _wrap_in_tags("p", lsp_label) by html.escape(lsp_label), because the p tag isn't supported in the completion item details anyway and will be ignored: sublimetext.com/docs/minihtml.html#inline-formatting

👍 👍

jwortmann · 2022-07-26T16:23:35Z

How about the following proposal:

If there are no label.details, we always use label as the "trigger" (i.e. filterText will be ignored)
If there are label.details, we use label + label.details as the "trigger", unless filterText exists and label+label.details in filterText returns False (in which case we would only use label)

rchl · 2022-07-27T21:28:14Z

Maybe I've lost some context but can you explain the reasoning for the filterText part of nr. 2?

jwortmann · 2022-07-27T22:21:52Z

Maybe I've lost some context but can you explain the reasoning for the filterText part of nr. 2?

I think that would be the closest to how VSCode handles it, and it would be a compromise between "show detailed information in the trigger" and "use sensible matching algorithm". VSCode indeed doesn't seem to use the label details for matching, but the bar(x) completion will still be shown when you only type x because it is included in the "filterText". So in ST, we would show the full bar(x) from the TypeScript server in the "trigger", and it would also work for the D server according to #1924 (comment), because that server doesn't use the "filterText".

In general a server would use "filterText", if it wants to give additional information how a particular completion item should be matched (otherwise it could just omit it and according to the specs the client can fallback to "label"). So if we know that the label+label.details is also included in the "filterText", then we know that it is intentional that the matching algorithm should also consider the label.details for matching, and we can safely put it in the "trigger". And the other way, if the server wants to restrict matching by using "filterText" and not including the label details therein, then we know that ST shouldn't use it for matching (then we should probably put it into the "details" instead, like it is the case now).

I'm a bit doubtful that it is a good way how the TypeScript server uses "filterText", by including the \n and tabstop, but well, it's up to them if they like to.

A more conservative (in regard to the current behavior) alternative could also be:

2. If there are label.details we use label + label.details as the "trigger", but only if also filterText exists and label+label.details in filterText returns True (and otherwise use only label).

But this one wouldn't show the label details from the D server in the "trigger".

rwols · 2022-07-28T09:45:03Z

For more context on this matter: see #990 (comment)

Also see: microsoft/language-server-protocol#898 (comment)

My takeaway from this is that Sublime is fundamentally different from VSCode in that it shows what the user is matching, while VSCode hides what the user is matching.

We can try using LSP-label as ST-trigger if the LSP-label starts with LSP-filterText.

I don't see how we can "dim" the LSP-label-details though.

jwortmann · 2022-07-28T17:52:32Z

We can try using LSP-label as ST-trigger if the LSP-label starts with LSP-filterText.

Hm, okay that could work. But then I think we could do the same with the LSP-label-details = item["labelDetails"]["detail"], i.e. if LSP-label + LSP-label-details starts with LSP-filterText, then we can use LSP-label + LSP-label-details as the "trigger".

I've made some tests with it and how I would implement it is at jwortmann@e8f6928.

In the mentioned case of LSP-label + LSP-label-details as "trigger", it will also use the LSP-label-description (if present) as the "annotation". This should yield better results with the D server #1924 (comment) and follows the specs in regard that it should be "rendered less prominently after" LSP-label-details.

It's quite annoying that various servers seem to use some fields differently or in unexpected ways. For example the Julia server doesn't use the labelDetails, but puts the whole function body into the regular item["detail"] for a function completion:

rchl · 2022-07-28T18:19:35Z

I do like how it works (after testing for 5 minutes, mind you).

rchl · 2022-07-31T12:43:40Z

@jwortmann do you want to create a PR with your changes?

Alternatively I could adopt those here.

Set completion trigger from label detail if present

ace0164

rchl mentioned this pull request Jul 24, 2022

detailedLabelSupport not correctly implemented #1924

Closed

document completion UI

a70e7f0

predragnikolic reviewed Jul 24, 2022

View reviewed changes

plugin/core/views.py Outdated Show resolved Hide resolved

rchl added 2 commits July 24, 2022 20:20

cleanup

b70e7b4

UI

d1ed4d2

predragnikolic approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'main' into fix/completion-label-detail

04e19fa

* main: Prevent failing to load all configs when one fails Refactor symbol and completion item kinds (#2000) Add Godot (GDScript) LSP instructions Add preview for resource files in LocationPicker

tweaks

aae2d63

jwortmann mentioned this pull request Jul 31, 2022

Completions overhaul #2010

Merged

rchl closed this Jul 31, 2022

rchl deleted the fix/completion-label-detail branch July 31, 2022 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set completion trigger from label detail if present #2002

Set completion trigger from label detail if present #2002

rchl commented Jul 24, 2022

predragnikolic left a comment

predragnikolic commented Jul 25, 2022 •

edited

Loading

rchl commented Jul 25, 2022 •

edited

Loading

jwortmann commented Jul 25, 2022

rchl commented Jul 25, 2022 •

edited

Loading

rchl commented Jul 25, 2022 •

edited

Loading

jwortmann commented Jul 26, 2022

rchl commented Jul 26, 2022 •

edited

Loading

jwortmann commented Jul 26, 2022 •

edited

Loading

rchl commented Jul 27, 2022 •

edited

Loading

jwortmann commented Jul 27, 2022

rwols commented Jul 28, 2022

jwortmann commented Jul 28, 2022

rchl commented Jul 28, 2022

rchl commented Jul 31, 2022

Set completion trigger from label detail if present #2002

Set completion trigger from label detail if present #2002

Conversation

rchl commented Jul 24, 2022

predragnikolic left a comment

Choose a reason for hiding this comment

predragnikolic commented Jul 25, 2022 • edited Loading

rchl commented Jul 25, 2022 • edited Loading

jwortmann commented Jul 25, 2022

rchl commented Jul 25, 2022 • edited Loading

rchl commented Jul 25, 2022 • edited Loading

jwortmann commented Jul 26, 2022

rchl commented Jul 26, 2022 • edited Loading

jwortmann commented Jul 26, 2022 • edited Loading

rchl commented Jul 27, 2022 • edited Loading

jwortmann commented Jul 27, 2022

rwols commented Jul 28, 2022

jwortmann commented Jul 28, 2022

rchl commented Jul 28, 2022

rchl commented Jul 31, 2022

predragnikolic commented Jul 25, 2022 •

edited

Loading

rchl commented Jul 25, 2022 •

edited

Loading

rchl commented Jul 25, 2022 •

edited

Loading

rchl commented Jul 25, 2022 •

edited

Loading

rchl commented Jul 26, 2022 •

edited

Loading

jwortmann commented Jul 26, 2022 •

edited

Loading

rchl commented Jul 27, 2022 •

edited

Loading