Use filterText as trigger, if it's present

[rwols]

The LSP-filterText is truly the suitable thing for the ST-trigger.

If an LSP-filterText is present, we have a bit of a problem on where
to stuff all of the interesting things.
I decided to put the LSP-label into the ST-annotation field.
That leaves us with only the ST-details to fill in. If there's an LSP-detail,
use that. Otherwise, use the LSP-documentation for the ST-details.

In case there's no LSP-filterText, we are supposed to use the LSP-label
for the ST-trigger. In that case there are enough fields: the LSP-detail
can go into the ST-annotation, and the LSP-documentation and go into the
ST-details.

[rwols]

I know we had some discussion about this in the past, and we ultimately decided to keep using the LSP-label for ST-trigger. We went back to the LSP-label because the completions looked too "bare". But now we can stuff the LSP-label in the ST-annotation field.

This fixes the clangd problem of decorated labels.

@ayoub-benali, could you try out this branch and see if metals is working better with this, now that we have an ST4 AC widget?

This PR is also inspired by findings and investigations in https://gist.github.com/jskinner/4dfa6d86f848e1e1e583883507369b67

[rwols]

Some screenshots:

pyls:

• Has no filterText, so the label is used as trigger.
• LSP-detail field is put into ST-annotation field.
• LSP-documentation field is put into ST-details field.
  

clangd:

• Uses filterText.
• LSP-label is put into ST-annotation field.
• LSP-detail is put into ST-details field.
  

lemminx: (XML server)

• uses filterText.
• LSP-label is put into ST-annotation field. This is absolutely necessary as "end with" does not match the prefix at all.
  

[rwols]

I don't know Scala, but I'm guessing this works pretty well for metals, too:

What we could do in addition to this diff, is to check whether lsp_label.startswith(lsp_filter_text). If it does, then st_trigger=lsp_label is safe to do.

[predragnikolic]

@rwols, can you check Implement all members for metals?

#771
based what I saw in the issue for "Implement all members",
"filterText": "e" and the "label": "Implement all members".

 "label": "Implement all members",
  "kind": 12,
  "sortText": "00002",
  "filterText": "e",
  "insertTextFormat": 2,
  "textEdit": {
    "range": {
      "start": {
        "line": 9,
        "character": 3
      },
      "end": {
        "line": 9,
        "character": 4
      }
    },
    "newText": "def foo: Int \u003d ${0:???}\n   def boo: Int \u003d ${0:???}"
  },
  "data": {
    "target": "file:/Users/ckipp/Documents/scala-workspace/test-project/?id\u003droot",
    "symbol": "local6"
  }

[predragnikolic]

Instead of if/else maybe just one or?

st_details = html.escape(lsp_detail) or self.normalized_documentation(item)

[rwols]

So I'm pretty sure we came to the wrong conclusion in #771.
The filterText is supposed to be used for prefix matching. Read microsoft/language-server-protocol#898 (comment) very carefully.

In that comment:

• filterText: b
• label: foo-text-range-replace

When the user types a b, the completion item with foo-text-range-replace appears.

In #771:

• filterText e
• label: Implement all members

When the user types an e, the completion item with Implement all members should appear.

[predragnikolic]

Is this function expensive?
I tried the PR locally,
When I trigger the AC in LSP-elm it takes >20s for this line finish. The AC doesn't show up at the end.

items = list(map(self.format_completion, response_items))

maybe the reason it is the combination of normalized_documentation and transform_region being applied to all items?

[predragnikolic]

transform_region is not that expensive,
but normalized_documentation is. If I comment out the normalized_documentation calls, the AC show up

[rwols]

These are some good points. Waiting potentially 20 seconds before completions show up is of course unacceptable. Performance from mdpopups.md2html is apparently not good enough (perhaps @facelessuser is willing to improve it, if that it even possible). I think the problem is ultimately on our side since we're rendering potentially >200 markdown strings to html.

Ideally, documentation would only be rendered once the user highlights a particular completion item (lazy rendering). But right now we're forced to render all markdown docs to html before presenting completions (cc @jskinner)

That said, from some initial experience with this PR I think putting the documentation into the ST-details is not a good fit. This is because we don't know how many lines/sentences a server will put into the docs. And there is only place for at most one sentence in the ST-details field.

So either we'll have to come up with a way to extract the "most important sentence" from the returned markdown string, or we have to think of something else.

One idea that's been floating in my head is as follows:

Use the new subl:// scheme in the ST-details to make a href to a command (let's call it LspShowDocumentationCommand). So that href will appear in the ST-details field at the bottom of the completion widget. The user can click it. The LspShowDocumentationCommand would then show the docs in a popup with mdpopups.show_popup and with the flag sublime.COOPERATE_WITH_AUTO_COMPLETE. This would allow us to do the lazy rendering as we can store the LSP-documentations of all items somewhere.

[predragnikolic]

Because we currently don't know the what the highlighted completion item is in the AC,
Just to chain on your idea.
Instead of just calling LspShowDocumentationCommand,
the documentation field may not be there unless that completion item is resolved.
I would just suggest first calling the completionItem/resolve before showing the documentation.

[ayoub-benali]

It is the same for Metals, client is expected to call completionItem/resolve while scrolling through the completion list because the documentation is expensive to compute.
Is it possible to request completionItem/resolve via the new AC API ?

[rwols]

We can't do that, because we don't know which completion item is highlighted in the AC widget.

[rwols]

Yes, we know up-front whether server can resolve docs through the resolveProvider server capability. So if that's not present or set to false then we won't show the hyperlink.

[rchl]

But if that's a server capability and not per-completion then I suppose some completions might still return nothing.

[ayoub-benali]

we don't know which completion item is highlighted in the AC widget.

I got misled by this
https://discordapp.com/channels/280102180189634562/280102180189634562/704475737088065566

[rwols]

But if that's a server capability and not per-completion then I suppose some completions might still return nothing.

Yes.

[rchl]

I've tried on lsp-typescript and lsp-vue and couldn't see anything out of place. It was a bit better generally.

LSP-typescript

Before:

After:

• there is some duplication here so the startsWith optimization might help here

LSP-vue

Before:

After:

• I like extra documentation field

[ayoub-benali]

@rwols @predragnikolic I tested #771

EDIT:
prefix completion is still working

[FichteFoll]

I've tried on lsp-typescript and lsp-vue and couldn't see anything out of place.

The LSP-typescript example confuses me a bit, tbh. Should the ? not be included in the trigger? Does it make sense to show almost the same text as the trigger in the annotation but with a ? appended to represent what is being inserted? Sounds like a lot of duplication.

[rchl]

I agree it's kinda unnecessary and busy. I believe that duplication would be gone if the startsWith "optimization" would be applied (#990 (comment)).

[rwols]

It's not guaranteed that if one LSP-label starts with its associated LSP-filterText, then all LSP-labels start with their associated LSP-filterText.

For instance look at the metals language server. We have a funky e as filterText, but the label is Implement all members, something totally different.

However from some testing I found that most completion items from metals do have the "starts with" property.

But I fear that it might be confusing for users. We could definitely give it a try though. Also, carefully look at the behavior of ST's fuzzy string matching and see if it doesn't deteriorate when using the LSP-label for the trigger.

[rwols]

See also: sublimehq/sublime_text#819

Using filterText is safer to use as it should contain only alphanumeric characters (PowershellEditorServices language server seems to have a different opinion on this).

[FichteFoll]

Indeed, I was reminded about sublimehq/sublime_text#819 earlier today and this is definitely a show stopper for non-alphanumerics in the trigger. Trying to find a general algorhithm to remove redundant duplication when it makes sense, but leave it when it wouldn't sounds like a doable but tough endeavour.