LSP icon indicating copy to clipboard operation
LSP copied to clipboard

detailedLabelSupport not correctly implemented

Open ryuukk opened this issue 3 years ago • 22 comments

Describe the bug

A screenshot worth more than words:

actual:

image

expected: (vscode)

image

To Reproduce

Use D's LSP according to the documentation

https://github.com/Pure-D/serve-d/

https://lsp.sublimetext.io/language_servers/#d

Expected behavior

with detailedLabelSupport, i expect each completion item to properly strip the funciton parameters, so it doesn't show the item label twice..

Screenshots

please refer to the 1st block above

Environment (please complete the following information):

  • OS: arch linux
  • Sublime Text version: 4126
  • LSP version: 1.14
  • Language servers used: D

ryuukk avatar Dec 28 '21 16:12 ryuukk

There is no feasible solution. With ST's API, things you can fill in are trigger, annotation and details.

image

Related discussion

  • https://discord.com/channels/280102180189634562/645268178397560865/893676807616884789

jfcherng avatar Dec 28 '21 17:12 jfcherng

Oh i see, is it being looked at by the sublime text developpers?

ryuukk avatar Dec 28 '21 19:12 ryuukk

I am looking at the image for the expect behavior and IMO displaying that much info in the autocomplete panel is distracting.

For me the LSP signature popup is used to show the parameters for a given function.

Is it helpful to see a pretty wide autocomplete popup with a bunch of information? And if so please emphasize what information is really important.

predragnikolic avatar Dec 28 '21 21:12 predragnikolic

@predragnikolic

showing all the lines isn't the point of the feature, let's ignore it for the moment (note that it is the case already for ST, it shows all the lines already with duplicate info)

it's the fact that no information is duplicated anymore

before:

create_texture --------------------------- Texture2D create_texture(uint....)

why is create_texture displayed twice?

after:

create_texture (uint...) --------------------------- Texture2D

There is less confusing, and the duplication of information is no more, shorter, and less distraction

--

Then for showing all the lines at once, it could be an option "show_details_for_all_items": "true|false"

ryuukk avatar Dec 28 '21 23:12 ryuukk

  1. Are you sure that the D server has the completionProvider.completionItem.labelDetailsSupport capability? I think this capability was renamed from detailedLabelSupport, so if the server still expects the old name, it may not send the CompletionItemLabelDetails at all to this client. An excerpt from the log panel with the payload when triggering autocompletions would reveal if that is the case.

    If you look at the code https://github.com/sublimelsp/LSP/blob/f1e96dcc67b1351c409e145dcb338c458977db02/plugin/core/views.py#L884 you can see that the only thing this client puts into the "annotation" field is CompletionItem.detail, but not CompletionItem.labelDetails.detail. It seems to be the case that the D server just puts everything into CompletionItem.detail here when it communicates with this client. The information from CompletionItemLabelDetails is only used for the "details" area at the bottom of the autocomplete popup: https://github.com/sublimelsp/LSP/blob/f1e96dcc67b1351c409e145dcb338c458977db02/plugin/core/views.py#L889-L905

  2. As it was already pointed out above, your suggestion

    create_texture (uint...) --------------------------- Texture2D

    is currently not possible with the ST API. If the additional details text is put into "trigger", then it would also be used for filtering of the items while typing, which probably would make the autocompletion behavior really bad.

  3. Note that even VSCode does it a bit wrong; according to the LSP specs, CompletionItemLabelDetails.detail should be

    rendered less prominently directly after label, without any spacing.

    So it ideally should be create_texture(uint...), not create_texture (uint...)

jwortmann avatar Dec 29 '21 00:12 jwortmann

  1. i guess so, otherwise it wouldn't work on vscode?
  2. vscode have 2 thing, label, so it show the text, and insertText, that's what get put instead of the label https://github.com/Pure-D/serve-d/blob/84094fade433f3d52e43c5296d20af53b102ffdd/source/served/commands/complete.d#L1197-L1219
  3. i think the spacing was done intentionally by the D server https://github.com/Pure-D/serve-d/blob/84094fade433f3d52e43c5296d20af53b102ffdd/source/served/commands/complete.d#L1073

ryuukk avatar Dec 29 '21 02:12 ryuukk

  1. i guess so, otherwise it wouldn't work on vscode?

Yes, but I meant that maybe the D server expects detailedLabelSupport instead of labelDetailsSupport as a client capability, and perhaps VSCode also uses the old capability name for backwards compatibility or so. Anyway, you can test it if you set "log_server": ["panel"] in the LSP settings, then use "LSP: Toggle Log Panel" from the command palette and look for the server response after textDocument/completion. Then you can see if it contains "labelDetails" or not (I assume it doesn't).

  1. vscode have 2 thing, label, so it show the text, and insertText, that's what get put instead of the label https://github.com/Pure-D/serve-d/blob/84094fade433f3d52e43c5296d20af53b102ffdd/source/served/commands/complete.d#L1197-L1219

Sublime LSP also supports and uses insertText. The point is that the "trigger" field is used for filtering. For example if you type foo, then do you want to see a completion item for the following? foo

  1. i think the spacing was done intentionally by the D server https://github.com/Pure-D/serve-d/blob/84094fade433f3d52e43c5296d20af53b102ffdd/source/served/commands/complete.d#L1073

OK, then the screenshot from VSCode makes sense.

jwortmann avatar Dec 29 '21 10:12 jwortmann

@jwortmann

{
	'kind': 3,
	'data': None,
	'label': 'create_texture',
	'labelDetails': {
		'description': 'Texture2D',
		'detail': ' (uint width, uint height, ubyte* ptr, PixelFormat format = PixelFormat.Rgba)'
	},
	'sortText': '2_4_Texture2D create_texture(uint width, uint height, ubyte* ptr, PixelFormat format = PixelFormat.Rgba)',
	'detail': 'Texture2D create_texture(uint width, uint height, ubyte* ptr, PixelFormat format = PixelFormat.Rgba)'
}

ryuukk avatar Dec 29 '21 18:12 ryuukk

Maybe LSP should favor labelDetails.description, if it exists, over detail for the "annotation" field, and don't show it in the "details" area at the bottom then? I think the descriptions of the fields in the LSP specs are written in a way that doesn't consider having an additional panel area at the bottom.

     if isinstance(lsp_label_description, str): 
         # `description` should be rendered less prominent than `detail`. 
         # Additional separation is added. 
-        st_details += " - {}".format(_wrap_in_tags("i", lsp_label_description))
+        st_annotation = lsp_label_description.replace('\n', ' ')
     st_details += "</p>" 

This would be sililar to what VSCode uses on the right side for the items.

But it would be useful to know if other language servers use the fields in the same way.

jwortmann avatar Dec 29 '21 22:12 jwortmann

Any news on this?

This is annoying because the current popup is unreadable, most of the info is hidden because too mutch duplicate informations

image

it is lot of noise

image

ryuukk avatar Mar 04 '22 14:03 ryuukk

From the servers I'm using regularly (LSP-typescript, LSP-pyright, LSP-vue, LSP-dockerfile, LSP-json, LSP-terraform, LSP-yaml), none of them supports labelDetails it seems (or at least not for the completions I've tried) so the impact of such change would probably be pretty low but ideally someone who uses server that supports it should make such change and run with it for a while to see the upside and downside clearly.

rchl avatar Mar 10 '22 20:03 rchl

The more we have to wait, the more i'm suggesting people to use vscode instead

https://github.com/zigtools/zls/pull/507

I just sent this PR over there, and it works out of the box with vscode

It is a shame that it takes that long for sublime text to eventually think about an eventuality that it could eventually be added in the future, eventually

ryuukk avatar Jul 02 '22 14:07 ryuukk

What do you mean by “sublime text”? We’re all unpaid volunteers here. Please also research clangd on how it renders the labelDetails.

rwols avatar Jul 02 '22 17:07 rwols

What clangd does is incomplete

You need to understand why the feature was created and proposed for the LSP 3.17 spec

https://github.com/microsoft/vscode/issues/39441

It is to show information on all the completion items, not just the selected one, otherwise it makes no sense

I understand it is volunteers effort, i volunteer my time to improve many people's experiences too, i am just sad nobody follows, sublime is not open source, so there is nothing much i can do other than bump issues i opened

ryuukk avatar Jul 03 '22 01:07 ryuukk

LSP is open source and it's possible to experiment with it per @jwortmann's comment. But if you'd expect a more complete solution from Sublime Text itself then it would need to be requested in its bug tracker (if it isn't already).

rchl avatar Jul 17 '22 11:07 rchl

The typescript server now implements support for labelDetails (behind a flag) and it looks far from ideal in ST:

Screenshot 2022-07-24 at 00 28 43

It provides two completions, one bar and one bar(x) {} snippet but label is the same for both.

rchl avatar Jul 23 '22 22:07 rchl

I guess best ST can do is something like below which looks rather weird.:

Screenshot 2022-07-24 at 00 47 41

...or...

Screenshot 2022-07-24 at 01 22 45

rchl avatar Jul 23 '22 22:07 rchl

Or we could in fact try to put this into the trigger. The claim that it would make filtering worse would have to be tested.

Screenshot 2022-07-24 at 16 36 36

rchl avatar Jul 24 '22 14:07 rchl

Did a quick change for testing: https://github.com/sublimelsp/LSP/pull/2002

rchl avatar Jul 24 '22 15:07 rchl

Did a quick change for testing: #2002

Thanks!

As expected, the result looks a little bit hard to differentiate, wich ends up being the hack that was used prior to that LSP suggestion

image

Something like that would be better

image

Can theming be applied to that string so a contrast/brightness filter could be applied?

ryuukk avatar Jul 24 '22 15:07 ryuukk

No, the left-aligned part of the completion is a plain text and can't have styling applied.

rchl avatar Jul 24 '22 18:07 rchl

As expected, the result looks a little bit hard to differentiate, wich ends up being the hack that was used prior to that LSP suggestion

image

This influences the matching algorithm, which is not acceptable for me.

image

Something like that would be better

image

Can theming be applied to that string so a contrast/brightness filter could be applied?

The effect you want to achieve cannot be done as there are only three slots in the completion widget which can be filled in; one is left-aligned and determines the matching, one is right-aligned and dimmed, and the last is a "detail" field (which can have styling).

If you want to have this "trigger detail" in ST, you need to open a feature request here: https://github.com/sublimehq/sublime_text/issues

rwols avatar Jul 25 '22 18:07 rwols

This was addressed with https://github.com/sublimelsp/LSP/pull/2010

NOTE: there is still something that needs to be addressed on the server side

predragnikolic avatar Aug 20 '22 19:08 predragnikolic

Summarizing, detailed label support is something that must also be supported from ST core. We can only do so much with the slots currently given. I hope the changes suite your workflow better. Overall I hope you get work done with your 3D engine regardless of the particular completion widget details.

rwols avatar Aug 20 '22 20:08 rwols