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.

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

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

Closed
bmewburn opened this issue Dec 1, 2017 · 50 comments
Closed
Assignees
Labels
api-finalization feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan suggest IntelliSense, Auto Complete ux User experience issues
Milestone

Comments

@bmewburn
Copy link

bmewburn commented Dec 1, 2017

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?

@cleidigh cleidigh added feature-request Request for new features or functionality suggest IntelliSense, Auto Complete labels Dec 2, 2017
@bmewburn
Copy link
Author

bmewburn commented Feb 7, 2018

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
Copy link

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
Copy link

gabro commented Feb 13, 2019

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

image

@bmewburn
Copy link
Author

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

@olafurpg
Copy link

olafurpg commented Apr 30, 2019

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

@jrieken jrieken self-assigned this Oct 22, 2019
@jrieken jrieken added the ux User experience issues label Dec 11, 2019
@jrieken jrieken added this to the December/January 2020 milestone Dec 11, 2019
@jrieken
Copy link
Member

jrieken commented Dec 11, 2019

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
Copy link
Member

jrieken commented Jan 3, 2020

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
Copy link
Contributor

octref commented Jan 10, 2020

Current state:

  • Added CompletionList#isDetailsResolved. When this is set to true, or if completionItemProvider has undefined resolveCompletionItem, we mark an item's detail as resolved.
  • Plan to add a setting, editor.suggest.alwaysRevealInlineDetails to toggle between current behavior and new behavior, which shows all resolved details.

details

Current issues:

  • When a completion item provider can async resolve items but doesn't have isDetailsResolved (as shown above for completions from TS Server), going up/down to reveal inline detail doesn't feel nice.

  • Multiple circle-i icons look weird.

  • We need clearer guidance for details. Currently it says:

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

    It's unclear whether I should provide a valid code snippet, or some random string.

Questions:

  • Default editor.suggest.alwaysRevealInlineDetails to true or false?
  • Drop duplicated details in the details UI or no?

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

octref added a commit that referenced this issue Jan 10, 2020
@jrieken

This comment has been minimized.

octref added a commit that referenced this issue Jan 10, 2020
@octref

This comment has been minimized.

@jrieken
Copy link
Member

jrieken commented Jan 10, 2020

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
Copy link
Contributor

octref commented Jan 15, 2020

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:

  • CompletionList#isDetailsResolved: One more field on CompletionList. Also feels weird marking property in another interface as "having to be provided upfront".
  • CompletionItem#labelDetails: Should be an optional property, but needs to be provided upfront and not resolved later. Still look strange. Also later if we decide to add more fields such as labelDescription etc, this would make CompletionItem very messy.

label: string | CompletionItemLabel is better because:

  • label has to be provided upfront anyway

  • As we enrich the UI, we can easily add more fields to CompletionItemLabel

     # Now
    
     <label>                      <details>
     <label>                      <details>
     <label>                      <details>
    
     # Future, Maybe
    
     <label><description>         <details>
     <label><description>         <details>
     <label><description>         <details>
    
  • This "string | ComplexObject for showing simple / complex UI" concept already exists. For example, showQuickPick:

     export function showQuickPick(items: string[] | Thenable<string[]>, ...) {}
     export function showQuickPick<T extends QuickPickItem>(items: T[] | Thenable<T[]>, ...) {}

    image

@bmewburn
Copy link
Author

bmewburn commented Jan 15, 2020

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
Copy link
Member

akaroml commented Jan 16, 2020

+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 added a commit that referenced this issue Jun 15, 2021
jrieken added a commit that referenced this issue Jun 15, 2021
jrieken added a commit that referenced this issue Jun 15, 2021
jrieken added a commit that referenced this issue Jun 15, 2021
@jrieken
Copy link
Member

jrieken commented Jun 21, 2021

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

  • You need to complete all label properties during provideCompletionItem and labels cannot be refined during resolve (we want a stable UI)
  • Default filtering and highlighting only operates on CompletionItemLabel#name and not on the other properties

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
Copy link
Contributor

Gama11 commented Jun 21, 2021

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
Copy link
Contributor

DanTup commented Jun 21, 2021

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
Copy link
Contributor

DanTup commented Jun 21, 2021

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
Copy link
Member

jrieken commented Jun 21, 2021

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
Copy link
Contributor

DanTup commented Jun 21, 2021

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
Copy link
Member

andrewbranch commented Jun 21, 2021

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
Copy link
Member

jrieken commented Jun 21, 2021

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
Copy link
Member

jrieken commented Jun 21, 2021

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 #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
Copy link
Member

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
Copy link
Member

jrieken commented Jun 22, 2021

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
Copy link
Member

jrieken commented Jun 22, 2021

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
Copy link
Contributor

DanTup commented Jun 22, 2021

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
Copy link
Member

jrieken commented Jun 22, 2021

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
Copy link
Member

jrieken commented Jun 28, 2021

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

vscode/src/vs/vscode.d.ts

Lines 3978 to 4001 in 24f9000

/**
* Represents a structured label for completion items.
*/
export interface CompletionItemLabel {
/**
* The label of this completion item. By default
* this is also the text that is inserted when selecting
* this completion.
*/
label: string;
/**
* An optional string which is rendered less prominent and directly after {@link CompletionItemLabel.label name},
* without any spacing. Should be used for function signatures or type annotations.
*/
detail?: string;
/**
* An optional string which is rendered less prominent and after {@link CompletionItemLabel.detail}. Should be used
* for fully qualified names or file path.
*/
description?: string;
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-finalization feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes on-testplan suggest IntelliSense, Auto Complete ux User experience issues
Projects
None yet
Development

No branches or pull requests

13 participants