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.

Sign up for GitHub

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

Use filterText as trigger, if it's present #990

Merged
merged 5 commits into from
Apr 28, 2020
Merged

Use filterText as trigger, if it's present #990

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cfb2256
Use filterText as trigger, if it's present
rwols Apr 27, 2020
4f12d24
Update completion tests
rwols Apr 27, 2020
676d629
I somehow removed a test
rwols Apr 27, 2020
933db94
Don't present documentation, let's work on that later
rwols Apr 28, 2020
aae2fb6
Fix textEdit range in tests
rwols Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions plugin/completion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import html
import mdpopups
import sublime
import sublime_plugin

Expand Down Expand Up @@ -191,6 +193,19 @@ def on_query_completions(self, prefix: str, locations: List[int]) -> Optional[su
lambda res: self.handle_error(res, completion_list))
return completion_list

def normalized_documentation(self, lsp_item: Dict[str, Any]) -> str:
lsp_documentation = lsp_item.get("documentation")
Copy link
Member

Choose a reason for hiding this comment

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

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?

output

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

@rwols rwols Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@rchl rchl Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Yes.

if isinstance(lsp_documentation, str):
return html.escape(lsp_documentation.replace('\n', ' '))
elif isinstance(lsp_documentation, dict):
value = lsp_documentation.get("value", '')
if lsp_documentation.get("kind") == "markdown":
return mdpopups.md2html(self.view, value)
else:
return html.escape(value.replace('\n', ' '))
else:
return ''

def format_completion(self, item: dict) -> sublime.CompletionItem:
item_kind = item.get("kind")
if item_kind:
Expand Down Expand Up @@ -220,12 +235,29 @@ def format_completion(self, item: dict) -> sublime.CompletionItem:
convert = self.view.text_point_utf16
item["native_region"] = (convert(row, start_col_utf16), convert(row, end_col_utf16))

lsp_label = item["label"]
lsp_filter_text = item.get("filterText")
lsp_detail = item.get("detail", "").replace('\n', ' ')
if lsp_filter_text:
st_trigger = lsp_filter_text
st_annotation = lsp_label
# Try to keep fields non-empty. If there's no `detail` field, attempt to use the `documentation` field.
if lsp_detail:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of if/else maybe just one or?

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

st_details = html.escape(lsp_detail)
else:
st_details = self.normalized_documentation(item)
else:
st_trigger = lsp_label
st_annotation = lsp_detail
st_details = self.normalized_documentation(item)

return sublime.CompletionItem.command_completion(
trigger=item["label"],
trigger=st_trigger,
command="lsp_select_completion_item",
args=item,
annotation=item.get('detail') or "",
kind=kind
annotation=st_annotation,
kind=kind,
details=st_details
)

def handle_response(self, response: Optional[Union[dict, List]], completion_list: sublime.CompletionList) -> None:
Expand Down
3 changes: 2 additions & 1 deletion stubs/sublime.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@ class CompletionItem:
command: str,
args: dict = {},
annotation: str = "",
kind: Tuple[int, str, str] = KIND_AMBIGUOUS
kind: Tuple[int, str, str] = KIND_AMBIGUOUS,
details: str = ""
) -> 'CompletionItem':
...

Expand Down
49 changes: 30 additions & 19 deletions tests/test_completion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from setup import CI, SUPPORTED_SYNTAX, TextDocumentTestCase, add_config, remove_config, text_config
from unittesting import DeferrableTestCase
import sublime
from sublime_plugin import view_event_listeners, ViewEventListener


additional_edits = {
Expand Down Expand Up @@ -292,31 +291,43 @@ def test_edit_after_nonword(self) -> 'Generator':
insert_text="List.",
expected_text='List.apply()')

def test_implement_all_members_quirk(self) -> 'Generator':
def test_filter_text_is_not_a_prefix_of_label(self) -> 'Generator':
"""
Metals: "Implement all members" should just select the newText.
Metals: "Implement all members"

The filterText is 'e', so when the user types 'e', one of the completion items should be
"Implement all members".

VSCode doesn't show the filterText in this case; it'll only show "Implement all members".
c.f. https://github.com/microsoft/language-server-protocol/issues/898#issuecomment-593968008

In SublimeText, we always show the filterText (a.k.a. trigger).

This is one of the more confusing and contentious completion items.

https://github.com/sublimelsp/LSP/issues/771
"""
yield from self.verify(
completion_items=[{
'insertTextFormat': 2,
'label': 'Implement all members',
'textEdit': {
'newText': 'def foo: Int \u003d ${0:???}\n def boo: Int \u003d ${0:???}',
'range': {
'start': {
'line': 0,
'character': 0
},
'end': {
'line': 0,
'character': 1
}
}
"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"
}
}],
insert_text="I",
expected_text='def foo: Int = ???\n def boo: Int = ???')
insert_text='e',
expected_text='def foo: Int \u003d ???\n def boo: Int \u003d ???')

def test_additional_edits(self) -> 'Generator':
yield from self.verify(
Expand Down