microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.09k stars 29.26k forks source link

Consider showing completion item detail if available for all list items #39441

Closed bmewburn closed 3 years ago

bmewburn commented 6 years ago

Take the following completion list.

screenshot from 2017-12-02 09-22-52

Each Client item belongs to a different namespace. If the completion item detail was shown immediately for each item then users could, at a glance, easily identify the relevant item out of a list of similarly labeled items. I suppose the argument against is that this can be done by modifying the label to include this, but then what is the role of the detail property?

bmewburn commented 6 years ago

Furthermore, if an extension author does try and add detail to the label to work around this limitation, then it can end up with incorrect highlighting as shown below.

screenshot from 2018-02-07 20-07-35

olafurpg commented 5 years ago

Any pointers on how to contribute a fix displaying the "detail" field if it's available? This seems like a low-hanging fruit that would greatly improve the user experience in cases like below

2019-02-13 11 37 35

gabro commented 5 years ago

As an extra data point, the Java LS works around this by including the package in the "label":

image

bmewburn commented 5 years ago

@gabro , when using the label do you find it can cause incorrect highlighting as in the example above, though?

olafurpg commented 5 years ago

@bmewburn the highlighting works out ok in my experience if you update the filter text to not include the appended namespace.

jrieken commented 4 years ago

fyi - I plan on implementing this for the next (Jan 2020) release and that "label + detail" workarounds will then look bad. Because of that we might start with having this behind a setting

jrieken commented 4 years ago

This should also help with Java which often presents a very busy IntelliSense box, where too much is rendered as label. cc @akaroml

Screenshot 2020-01-03 at 12 40 08
octref commented 4 years ago

Current state:

details

Current issues:

Questions:

Screen Shot 2020-01-10 at 11 26 11 AM

jrieken commented 4 years ago

I'd say show the ℹ️ icon only for the focused element because that's a click region for the doc-window.

octref commented 4 years ago

@jrieken The issue is, when item has details that's equal to labels (such as break), we don't render the readMore icon at all, so it looks ugly...

a

jrieken commented 4 years ago

Yeah, this is really ugly... I start to like what @alexdima suggested. Instead of changing the rendering of detail we could leave it as is and add a new properly labelDetail which we would render right after the label. Needs some more design work...

octref commented 4 years ago

Current proposal:

export interface CompletionItemLabel {

    /**
     * The name of this completion item's label.
     */
    name: string;

    /**
     * A description of the completion item which is rendered
     * less prominent.
     */
    // description?: string;

    /**
     * Details of the completion item that is rendered less
     * prominent to the right.
     */
    details?: string;
}

export class CompletionItem {

    /**
     * The label of this completion item. By default
     * this is also the text that is inserted when selecting
     * this completion.
     */
    label: string | CompletionItemLabel;

    /**
     * A human-readable string with additional information
     * about this item, like type or symbol information.
     */
    detail?: string;

    /**
     * Other fields...
     */
}

Other alternatives we didn't like:

label: string | CompletionItemLabel is better because:

bmewburn commented 4 years ago

I like it. It fits my use case in that I would have the symbol short name in CompletionItemLabel.name, the namespace in CompletionItemLabel.details and still able to have extra info like additional text edit imports in CompletionItem.detail.

akaroml commented 4 years ago

+1 on the idea of having separate detail fields for both the label and the completion item. With the label detail always showing, we can definitely move some of the information from the label name to the label detail, like this mock:

image

What do you think? @fbricon

jrieken commented 4 years ago
export interface CompletionItemLabel {
    // the name of the function or variable
    name: string;
    // the signature, without the return type. is render directly after `name`
    signature?: string; // parameters
    // the fully qualified name, like package name, file path etc
    qualifier?: string;
    // the return-type of a function or type of a property, variable etc
    type?: string;
}

ASCII-art "rendering"

<name><signature>        <qualifier>        <type>
mjbvz commented 4 years ago

@jrieken Do you want early feedback from TS on the idea and the API? Or should we wait until we have a solid proposal for what they should implement?

jrieken commented 4 years ago

Would be great to have TypeScript comment on this. I have seen that they have much of this information after the resolve call but this is information that must be returned from the provide-call. fyi - there are came in https://github.com/microsoft/vscode/issues/88757 which I have re-purposed for that 🤓

octref commented 4 years ago

We are merging below into proposed API:

export interface CompletionItem {

    label: string;

    /**
     * Will be merged into CompletionItem#label
     */
    label2?: CompletionItemLabel;
}

export interface CompletionItemLabel {

    /**
     * The function or variable
     */
    name: string;

    // The signature, without the return type. is render directly after `name`
    // signature?: string; // parameters
    // The fully qualified name, like package name, file path etc
    // qualifier?: string;

    // The return-type of a function or type of a property, variable etc
    type?: string;
}

We use label2 so we can isolate changes to proposed API. In the end label will be string | CompletionItemLabel.

When you provide label2 with type, it'll look like this:

suggest

jrieken commented 4 years ago

@octref Keeping this item alive for the API discussion/process (since it has all the history)

octref commented 4 years ago

Current state of UI, with Status Bar turned on:

image

Implementation:

vscode.languages.registerCompletionItemProvider('plaintext', {
  provideCompletionItems() {
    return {
      isIncomplete: false,
      items: [
        {
          label: 'foo',
          label2: {
            name: 'foo',
            signature: '(bar)',
            qualifier: 'baz.foo',
            type: 'MyType'
          },
          documentation: 'my documentation',
          detail: 'my detail'
        },
        {
          label: 'foo2',
          label2: {
            name: 'foo2',
            signature: '(bar)',
            qualifier: 'baz.foo',
            type: 'MyType'
          },
          documentation: 'my documentation',
          detail: 'my detail'
        }
      ]
    }
  }
})
mjbvz commented 4 years ago

Summary of discussion with TS Team:

Thoughts about current proposal

Ideas

octref commented 4 years ago

The current API doesn't exclude resolving the label later. I think it's reasonable to only return a string label and later return a CompletionItemLabel with type-related information.

We could say, yeah, if your provider is fast and can provide everything upfront, do it and we show info. Otherwise, as user goes up/down, we resolve each item, show info and do not hide the info on changing selection. But I guess that wouldn't be what users want exactly.

So we are in a dilemma:

jrieken commented 4 years ago

I know you are not going to like to hear that, but for JS/TS we cannot eagerly compute all the label information up front.

This isn't about likes or not but about how well does a feature work or not. Already today with our code-base we suffer from this, just trigger completions for Range and 💩. I do believe that TypeScript needs to come up with an answer for this.

Screenshot 2020-02-06 at 10 54 39

I understand the challenge that TS is facing but I think it needs a real, fresh look and things need to be measured (before judgement) and some things might need refactoring:

We're concerned about the size of the completions object. A good number of TS completions responses return more than 1000 items. Adding even a single string field to these means a lot more data is going back and force between VS Code and TS Server

This needs measurements! When I run "F1 > Measure Extension Host Latency" then I see 500-1000Mb/s which hints to me that this might not be the biggest problem here. Then, what are those 1000+ items, surely not member-completion items or locals but likely mostly globals that aren't changing for the whole project. TSServer could compute such static results once and only tell the extension when to suggest them or not. I have a feeling that would help a lot. Last, TS can always experiment with returning incomplete result sets.

Anyways, let's not derail this issue further and lets discuss TS specific issues in https://github.com/microsoft/vscode/issues/88757 and the TS mirror issue, this issue is about the API and UX and for extensions that already today have this information (like Java)

mjbvz commented 4 years ago

Sorry but this is not derailing: I shared real feedback on this proposal from our number one language (by user number). And while there are potential workarounds, we need to take this feedback into account.

I can setup a meeting with TS if you wish to discuss challenges of the proposal and what TS should ideally provide to VS Code for completions.

jrieken commented 4 years ago

I say derailing because there is #88757 to discuss the TypeScript aspect of this. I believe that we already have languages that can implement this, like Java, php, and python (?), and that we should get feedback from them. And yeah, saying let's implement "always showing details" but not doing that isn't constructive and yeah, I'd love to discuss this with TypeScript folks directly.

Gama11 commented 4 years ago

In the Haxe extension, we currently include the package path of types in the completion item label similar to Java. The main reason we do this is so that you can filter by the fully qualified path (related issue: #67887).

Will CompletionItemLabel.qualifier be considered in filtering, or is it purely cosmetic?

Note: yes, there already is a separate filterText that could be used, but I think the qualifier would have to support the same "filter highlight UI" (matched characters being highlighted in blue) that the label currently has to make for a good user experience.

gabro commented 4 years ago

@jrieken you can add Metals (the Scala language server) to the list. We would support this feature right away if it became available via LSP

jrieken commented 4 years ago

Will CompletionItemLabel.qualifier be considered in filtering, or is it purely cosmetic?

Yes - the current design is that only label is being used for filter/highlighting and that the other properties are used for decoration and structure. Adding the qualifier is an interesting idea that's worth being explored but that also adds semantics, e.g. the qualifier must be the long-form of the label (which is our design but we have no plans to enforce that)

jrieken commented 4 years ago

For the current proposal there are a few things I am unsure about and would like feedback

export interface CompletionItemLabel {
    // the name of the function or variable
    name: string;
    // the signature, without the return type. is render directly after `name`
    signature?: string; // parameters
    // the fully qualified name, like package name, file path etc
    qualifier?: string;
    // the return-type of a function or type of a property, variable etc
    type?: string;
}
Gama11 commented 4 years ago

To me, qualifiedName implies that it's the full name, so it would usually include the label. That seems redundant / like it would waste horizontal space in the UI.

Whereas the current qualifier would only be the prefix you have to add to qualify the name / label.

andrewbranch commented 4 years ago

@mjbvz @jrieken—from the TypeScript side, would the ideal response include a separate completion entry for every overload of the same method/function? Currently, we only send one entry per identifier, and then when we send the details, we show the first overload and a string indicating how many additional overloads (if any) are available:

image

Edit: feel free to move to the TS-specific issue I had lost track of; https://github.com/microsoft/TypeScript/issues/36265 —cross posting there.

octref commented 4 years ago

Made the change from CompletionItemlabel#signature to CompletionItemlabel#parameters, so the name can indicate lack of return type.

svipas commented 4 years ago

I'm often checking if this is still in progress. Maybe at least we can have an API from VS Code side so we can implement it in our completion providers (other extensions like Dart, Java, etc.) and then TypeScript will catch up?

@jrieken As far as I understand API is not stable yet and it's in proposal? If it's in proposal and not yet stable, how we can help to move things forward? I know there are other work to do, but this change would improve DX very very much!

jrieken commented 4 years ago

Yes this is a little unfortunate because in contrast to others TypeScript seems unable to implement this (see https://github.com/microsoft/TypeScript/issues/36265#issuecomment-673126687). We could ignore them or revisit - tho updating items incrementally while showing them (understand flicker) isn't something we aim for. The other unfortunate part is that the dev that worked on this left the team which means this is currently in the air.

svipas commented 4 years ago

@jrieken Thanks a lot for info. I'm happy you're aware of this issue and it may be implemented in the future and that's enough for me - the hope. 🙏

jrieken commented 3 years ago

Ping @DanTup, @Gama11, @akaroml, and others: We are planning to finalize this proposal and this is the last call for tweaks. The proposal is an overload to the CompletionItem#label-property, in addition to a plain string we will support a structured object.

Non-obvious implications of this proposal

The CompletionItemLabel-type and how it will be used. We are also still unsure about some naming tweaks... We have gone in circles on signature vs parameters and are open for suggestions


export class CompletionItem {
  label: string | CompletionItemLabel;
  // ... 
}

export interface CompletionItemLabel {
    /**
     * The name of this completion item. By default
     * this is also the text that is inserted when selecting
     * this completion.
     */
    name: string;

    /**
     * The signature of this completion item. Will be rendered right after the
     * {@link CompletionItemLabel.name name}.
     */
    signature?: string;

    /**
     * The fully qualified name, like package name or file path. Rendered after `signature`.
     */
    //todo@API find better name
    qualifier?: string;

    /**
     * The return-type of a function or type of a property/variable. Rendered rightmost.
     */
    type?: string;
}
Gama11 commented 3 years ago

I like signature, that seems like a pretty well-established name with signature help providers and whatnot.

Default filtering and highlighting only operates on CompletionItemLabel#name and not on the other properties

I still think that including the qualifier for filtering is valuable. If it's not the default behavior, maybe this could at least be enabled with a setting in the future? As mentioned previously, a custom filterText alone doesn't seem good enough on its own to enable a good UX because of highlighting.

DanTup commented 3 years ago

Does this replace the detail field? Do you have a screenshot of how things will be rendered? I'm currently putting "auto-import" tags in that render on the right that are fairly useful - these might work well inqualifier but it's not clear if they're going to make it look really busy (although I think we currently add them in resolve, so might need to change this a little anyway):

Screenshot 2021-06-21 at 15 42 25

I prefer the name signature to parameters as it seems a little less specific, but given the presence of type I'm wondering if it's not intended to actually be the full signature (void foo(String a)) and is intended only to be the parameters ((String a)), in which case parameters may be clearer?

DanTup commented 3 years ago

I still think that including the qualifier for filtering is valuable. If it's not the default behavior, maybe this could at least be enabled with a setting in the future? As mentioned previously, a custom filterText alone doesn't seem good enough on its own to enable a good UX because of highlighting.

Slightly related (although probably a little off-topic here), I'd really like to see the ability to provide multiple values for filtering/ranking. A constant complaint I get is about how VS Code ranks completion items once the user starts typing, and it's often because of qualified names like MyEnum.Foo where the user expects those enum values to be top for both MyEnum and Foo but there's no way for us to do this because we can't tell VS Code that it's 99% likely they want these because we know the argument type is this enum). Being able to supply ["MyEnum.Foo", "Foo"] in some way to VS code for ranking (or otherwise have some way of boosting some set of completion items beyond their name) would go a long way to addressing many of these complaints.

(I'm happy to file an issue about this if reasonable, but previous discussions in this area have been closed wontfix)

jrieken commented 3 years ago

Does this replace the detail field? Do you have a screenshot of how things will be rendered?

Yes - the complex label will replace the inline rending of the detail field - only the details/side window will show details. We use qualifier for TS suggestions in VS Code (but not signature nor type). This is how it looks - qualifier shows inline, details atop the doc-string.

Screen Shot 2021-06-21 at 16 58 58

I prefer the name signature to parameters as it seems a little less specific, but given the presence of type I'm wondering if it's not intended to actually be the full signature

Exactly that is the question. Signature is usually arguments and return type and puts a question mark on the dedicated type property... Remove it or split it into parameter and type? The latter would be more strict but it would also mean that the "reading order" is always parameters, qualifier, and type. Having signature and qualifier (no type) might give more freedom to extensions.

DanTup commented 3 years ago

I was going to say I'd prefer a single string for the flexibility and being able to align the whole thing to the right - although looking again, I've been putting (...) on the end of the method which was perhaps because putting the whole sig there was really noisy when it wasn't faded. So perhaps splitting them would be a reasonable update here.

Screenshot 2021-06-21 at 16 11 53

I'm curious how busy it looks with both signature and qualifier though. I might be tempted to only show them for those that will add new imports (and the text might be something like "import from foo" rather than just the filename/package). It's a little harder for me to try these out locally since I'm almost entirely migrated to LSP now.

andrewbranch commented 3 years ago

Since LSP 3.17 has support for this, have you gotten any implementation feedback from the Python folks, e.g. @jakebailey and @heejaechang? I know they face a lot of performance challenges with auto-imports in completions similar to TypeScript, so I would be interested to see if they’re any more optimistic about being able to support this than I am. To repeat my earlier feedback, computing signatures and qualifiers is a lot of work for a potentially unbounded number of global completions, and we don’t feel like we have any room to slow down our response time in most scenarios.

I’m currently working on resolving some of these details in chunks with incomplete responses, and that looks like a fair compromise over the poor display of auto-imports that we have right now. However, the experience still leaves a lot to be desired¹, and it comes at the cost of a lot of additional language server complexity and cumulative traffic/load as the incomplete responses come in. I fully share the desire to avoid label changes during keyboard navigation through the list, so would not suggest simply updating labels as details are resolved one item at a time. What we’re hoping for is an API that would let us set label details for every item visible in the UI, plus a (potentially quite large) overscroll amount. I can resolve module specifiers for auto imports probably a few hundred at a time with acceptable performance. But it’s quite easy for us to get into situations where we have tens of thousands of items, and no amount of ingenious refactoring will let us provide details for all those simultaneously with a cold cache.²

With that concern stated, I don’t think it actually has an immediate impact on the viability of this API—the structure looks good; I just hope we can continue to iterate on how and when it can get resolved in a way that creates a stable UI for consumers but is always possible to provide in finite time by providers.

As for the parameters vs. signature question, I’m in favor of matching what LSP does, which is currently proposed to be parameters. I’m a bit surprised that LSP is choosing to encode so much semantic meaning into what seems to me like general purpose display properties, but nonetheless, I see no reason to diverge. I’ve been taking every chance I can get to align new and updated features of the TypeScript language support with LSP.


¹ Suppose I can resolve details for 100 completion items in acceptable time, but the full list when the user triggers completions by beginning to type an identifier a should contain 200 items. The two strategies I could employ are (1) to limit my response to the first 100 items, including details for all, or (2) to return all 200 items, but leaving 100 of them without details. When the next character of the identifier is typed and the next request is triggered, I would then either (1) mix in the remaining 100 items (or whichever of them still match the typed identifier with its second character), or (2) simply fill in the details of 100 items (or the matching subset of them) that previously lacked details. Both of these strategies are noticeably suboptimal if the user scrolls through the entire completion list after typing the first character—there will either be missing items, or items lacking good labels.

² Another possible API solution could be to trigger additional completions requests on incomplete responses while scrolling. However, any solution where incomplete responses grow, instead of filling in with additional details, has potentially weird implications for sorting.

jrieken commented 3 years ago

I've been putting (...) on the end of the method which was perhaps because putting the whole sig there was really noisy when it wasn't faded

This is how a fake sample would look today. The rendering and fading level can certainly be tweaked. If we go with signature and drop type we could also make qualifier rendering different.

Screen Shot 2021-06-21 at 17 44 38

It doesn't look good when combined with qualifier. There is some alignment and spacing issues...

Screen Shot 2021-06-21 at 17 55 52
jrieken commented 3 years ago

As for the parameters vs. signature question, I’m in favor of matching what LSP does, which is currently proposed to be parameters. I’m a bit surprised that LSP is choosing to encode so much semantic meaning into what seems to me like

LSP proposals are usually a clone of VS Code API proposals. Until a week ago it was still parameters in VS Code but we decided to change that and LSP apparently didn't catch-up. The question about encoding semantics isn't answered tbh - completion does use program'ish lingo in some places, like its kinds . The use of name, signature, qualifier is more for a lack of good alternatives and to provide minimal guidance. I am open for suggestions, but I believe the usual triple of label, description, and detail won't work well...

What we’re hoping for is an API that would let us set label details for every item visible in the UI, plus a (potentially quite large) overscroll amount.

The problem is that scrolling doesn't happen in the usual linear fashion. Users "type to select" which means they are very likely to see a mix of resolved and unresolved items until they have typed "enough" to only see few, resolved items. Like, ts returns 10000 items with top100 resolved. Now, the user types and with that removes/filters say 3000 items which means from the top100 only every third item is resolved, or none, or only one... Now you have a happy mix of resolved and unresolved items and ts would be busy catching up with the typing-speed of its user.

I do agree that the ts challenge is separate from the shape of this API (cough, cough https://github.com/microsoft/vscode/issues/88757) and I don't want to block other extensions any longer because of that. I am not yet convinced that lazy resolving is the answer because I don't believe the UI will feel good but I am happy to try. Indeed, I did start a prototype which operates with incomplete completion lists and resolves in batches. I just couldn't parse the detail-information into a usable format and I do acknowledge that the incomplete result approach is very complex. Maybe ts can divide this into smaller problems, like have complex labels for member completion or something else low hanging. I also don't quite understand why this is so expensive for auto-imports. The full path is already there, we make it relative to the workspace and wouldn't making it relative to the project instead be all?

computing signatures and qualifiers is a lot of work for a potentially unbounded number of global completions, and we don’t feel like we have any room to slow down

On a related note: we do have requests to allow extensions to differentiate between automatic and explicit requests for completions. Users that have pressed ctrl+space are likely more willing to wait than those that use completions as you type. This has been raised explicitly for global completions (by Java) and I do believe it's an issue independent of complex labels or not. Yes, complex labels have the potential to slow down this unpredictably complex operation but they are not the root cause.

andrewbranch commented 3 years ago

Users "type to select" which means they are very likely to see a mix of resolved and unresolved items until they have typed "enough" to only see few, resolved items. Like, ts returns 10000 items with top100 resolved. Now, the user types and with that removes/filters say 3000 items which means from the top100 only every third item is resolved, or none, or only one... Now you have a happy mix of resolved and unresolved items and ts would be busy catching up with the typing-speed of its user.

The thing I’m confused about is, this is already essentially what’s happening with isIncomplete. I’ve added a bunch of optimizations that make our incomplete-triggered requests really fast (15–30ms), but if I add some delay in there, I can see that the completion list UI is totally static while waiting on the response. I don’t see how that’s different from what I’m suggesting, except that my suggestion can also apply to a linear scroll while not typing.

jrieken commented 3 years ago

Todays thinking behind isIncomplete is incomplete wrt the number of items returned, not wrt to the depth of items. However, it does allow for such an experiement. I also do agree that the UI lacks a "loading more" indication. This is a little tricky because often we are dealing with complete and incomplete results at the same time and we don't want to block the whole UI. By now we have an optional suggest status bar and that could show some kind of loading indication.

jrieken commented 3 years ago

On the UI and API - the current thinking is to reduce this to: label, signature, and qualifier. There needs to be a better name for "signature" because it is meant to be arguments and return type, or type annotation etc... So, maybe avoid programming lingo and use detail. Also two rendering styles: (1) all in one row (with spacing and opacity tweaks) or (2) right align the qualifier portion

Screen Shot 2021-06-22 at 11 48 29 Screen Shot 2021-06-22 at 11 45 47
DanTup commented 3 years ago

There needs to be a better name for "signature" because it is meant to be arguments and return type, or type annotation etc... So, maybe avoid programming lingo and use detail

I was just coming to suggest detail or description (both used elsewhere). Although it may be a little confusing there there's detail directly on the CompletionItem and now also on the name.

Of the screenshots I prefer the second one - the first one makes it difficult to scan the filenames (which might be important if you're trying to pick something based on that because there are duplicate identifiers).

What will be shown in the case where the extra information popup is visible? Will any of this information disappear and/or be shown there? (given that it may be truncated here, it seems like it'd be useful to show in the other popup too, but it's not clear if VS Code would do that or we should make it part of the documentation).

jrieken commented 3 years ago

I was just coming to suggest detail or description (both used elsewhere). Although it may be a little confusing there there's detail directly on the CompletionItem and now also on the name.

Some thoughts over here. Tendency to go with the triple of label, detail and description

What will be shown in the case where the extra information popup is visible? Will any of this information disappear and/or be shown there?

No changes for the details widgets planned. It should show items details and doc as it always does.

jrieken commented 3 years ago

This is what has been finalized. Those that can, please give it a try and report back with concerns and issues.

https://github.com/microsoft/vscode/blob/24f9000e97e6a798cf565bb7fbde810ae21ae667/src/vs/vscode.d.ts#L3978-L4001