feat: Add ClientFeature support for secondary text in completion items

[xuming]

SetsecondaryText overrides the label, causing it to display incorrectly in VS Code. I’ve submitted this patch to try to fix the issue.

However, I wonder whether this functionality is actually needed at all?

before

after

useSecondaryText is not seted

[genericptr]

Yeah that doesn't look right at all. In sublime text I use the $ suffix for the list (there's an overload policy setting) but for VSCode I'm not sure the right way to handle overloads. @zen010101 Did you add that secondary text for overloads, and if so can you explain the reasoning.

[mvancanneyt]

Forgive me my ignorance, but I don't see what the screenshots are meant to convey and what the relation is with the secondary text setting ?

[genericptr]

Forgive me my ignorance, but I don't see what the screenshots are meant to convey and what the relation is with the secondary text setting ?

The question appears to be about how overloads should be handled. You can see my solution for SublimeText and flat lists ($n suffix where n is the number of parameters) but I think one of the contributors added a pure text item to the list (+1 overload) and xuming wants to hide those.

I added an overloadPolicy setting to give some options to users but at the time I wasn't using VSCode.

[mvancanneyt]

In case of overloads, it seems better to list the parameters, that's what lazarus itself does ?

[genericptr]

In case of overloads, it seems better to list the parameters, that's what lazarus itself does ?

With Sublime Text the problem with that is then the text gets filtered in the list so you're searching through parameter names also and I wasn't using the container name.

I see DocumentSymbol has a detail field which we could use? Maybe it's worth trying but it could really messy unless we keep a simple type list like DoThis(Integer,String). Not sure how it looks though but it's worth making this an option.

Lets see who added that +1 overload text and maybe we can remove that and use detail instead.

[zen010101]

Yeah that doesn't look right at all. In sublime text I use the $ suffix for the list (there's an overload policy setting) but for VSCode I'm not sure the right way to handle overloads. @zen010101 Did you add that secondary text for overloads, and if so can you explain the reasoning.Yeah that doesn't look right at all. In sublime text I use the $ suffix for the list (there's an overload policy setting) but for VSCode I'm not sure the right way to handle overloads. @zen010101 Did you add that secondary text for overloads, and if so can you explain the reasoning.

to @genericptr: That piece of my code was copied from the original code, and I haven't studied its specific function.

to @xuming: The ClientProfile mechanism will be refactored and merged into serverSettings. At that time, you can submit a PR on the refactored version.

[xuming]

Sorry for not explaining it clearly. There are actually two issues here:
1. The label is being overridden by SetSecondaryText, which causes the function that should originally be displayed, As in the picture, the function 'dotest', not been displayed.
2. I think the overload is not necessary here. Displaying multiple overloaded functions in this context is meaningless unless detailed parameters are shown, but that would make the UI look too verbose.

In VS Code, after making a selection, if there are overloads, we can switch between functions in the way shown in the image below.

[genericptr]

In VS Code, after making a selection, if there are overloads, we can switch between functions in the way shown in the image below.

Oh that's "SignatureHelp". Sorry I was confused before. I'm not seeing what you are at least in Sublime Text. Not sure where the +1 text is even coming from.

[genericptr]

I had some time to investigate this and I think it's combination of a 1) a bad idea that I tried years ago and 2) a regression when cfFilterTextOnly was added.

Since I've been using this server for years now I know I never want to see "SometFunc +2 overloads" and simply choosing the name then scrolling through the list on parameters is correct usage. Maybe if we found a way to show the actual arguments in the list it would be useful but not like this.

I'll keep this open and make another PR later which removes the thing entirely along with cfFilterTextOnly and close this PR as unnecessary.

[xuming]

That's right. cfFilterTextOnly is not necessary.

[genericptr]

It was supposed to say like "DoThis +1 overloads" but there was a regression and it got lost then I didn't notice in PR review.

I see there is a new CompletionItem.labelDetails in LSP 3.17 which we can put parameters so at some point we can enable multiple overloads in the list as an option and show the parameters there. I think I tried before and couldn't get a clean parameter list from CodeTools or something. The snippet text in the completion is lame also and just does DoThis($0) instead of parsing the parameters and inserting them.

I'm happy with just one overload my self then using signature help to tab through the list.

[xuming]

I also like just one overload then using signature help to tab through the list.