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

Provide API to return formatted content from suggest and parameter hints #11877

Closed
joaomoreno opened this issue Sep 12, 2016 · 9 comments
Closed
Assignees
Labels
api feature-request Request for new features or functionality on-testplan release-notes Release notes issues suggest IntelliSense, Auto Complete

Comments

@joaomoreno
Copy link
Member

Take the following snippet:

/**
 * Some documentation is **nice**.
 * 
 * Example usage:
 * 
 * ```
 * function main() {
 *   return hello('world');
 * }
 * ```
 */
function hello(world: string): number {
    return 1337;
}

Here's how it looks in different widgets:

Hover

foo

Suggest

image

Parameter hints

image

Widgets look now somewhat aligned, since #2330. Their contents, on the other hand, could be a bit more.

Hover looks great, since at some point we provided the possibility of returning MarkedString through the API.

Both suggest and parameter hints lack that and look pretty pale in comparison.

We got to find a way to advance the API allowing users to return formatted content from certain API calls without bloating the API too much.

@joaomoreno joaomoreno added feature-request Request for new features or functionality api labels Sep 12, 2016
@joaomoreno
Copy link
Member Author

@jrieken Not sure whether we already have an issue for this.

@aeschli
Copy link
Contributor

aeschli commented Sep 13, 2016

API wise, I'd suggest the following non breaking changes: Whats new are the | MarkedString[] additions.

export class CompletionItem {
   label: string;     // leave as is... Otherwise things get complicated with the completion highlights
   detail: string | MarkedString[];
   documentation: string | MarkedString[];
   /// ...
}
export class ParameterInformation {
   label: string;  // no change ?
   documentation: string | MarkedString[];
}
export class SignatureInformation {
   label: string; // no change ?
   documentation: string | MarkedString[];
}

The following additions are optional, but IMO nice for consistency. For the hover constructor I'd suggest to break the behaviour and release-note that, or and add a new one like shown below.

export class Hover {
   contents: string | MarkedString[];   // adding string for plain text

   /** deprecated: Use constructor(range, string | MarkedString[]) instead.*/
   constructor(contents: MarkedString | MarkedString[], range?: Range);  // no change. 
   constructor(range: Range, contents: string | MarkedString[]);  // order changed, range can be null or undefined
}

For DecorationOptions, we can leave it as is or I suggest to do the following change which fix another API issue with the decoration API (which is not urgent, but just came up recently)

export interface DecorationOptions {
   /** deprecated, use textHoverContents or gutterHoverContents instead */
   hoverMessage?: MarkedString | MarkedString[];

   textHoverContents?: string | MarkedString[];
   gutterHoverContents?: string | MarkedString[];
}

@aeschli aeschli removed their assignment Dec 12, 2016
@mjbvz mjbvz self-assigned this Feb 2, 2017
mjbvz added a commit to mjbvz/vscode that referenced this issue Feb 3, 2017
part of microsoft#11877

This allows marked strings to be used in the `detail` and `documentation` field of completion items. I've tried to preseve the existing behavior as much as possible with this change. The new rendering will only be applied when an array of marked strings is passed in
@mjbvz mjbvz modified the milestone: Backlog Feb 23, 2017
@joaomoreno joaomoreno added the suggest IntelliSense, Auto Complete label Apr 26, 2017
@mjbvz
Copy link
Collaborator

mjbvz commented May 10, 2017

With @ramya-rao-a's work on the completion view, the lack of syntax coloring / markdown in completion item documentation is much more noticeable. Can we revisit this?

Since #19779 wasn't considered the right approach, what are thoughts on alternatives? @joaomoreno mentioned a CompletionItem2 as one possibility. Another thought would be to add optional rich text fields to CompletionItem that take precedence over the current plain text ones:

export class CompletionItem {
   label: string;

   detail?: string;
   richDetail?: MarkedString[]; // If present, would be used in place of `detail`. Would need a better name

   documentation?: string;
   richDocumentation?: MarkedString[]
   
   ...
}

The would keep the API backwards compatible but it's still ugly. Other thoughts?

@jrieken
Copy link
Member

jrieken commented May 10, 2017

Idea is to use a new type that subsumes MarkedString but also allows for other things like HtmlString (think of javadoc). Ideas are early, we don't ugly and something that lasts for a little longer. It should also remove the ambiguity the marked string has today (when to escape and when not)

@aeschli
Copy link
Contributor

aeschli commented May 11, 2017

Note that any change also needs to go into the language server protocol and it would be nice if we keep the APIs as similar as possible (not a must, but it's a better story). FYI @dbaeumer

The feedback we got from the snippet work was that we should not change the shape of property types (e.g. from string to string | MarkedString[]) as this breaks old language client implementations. For the snippetText we went with an additional 'format' field where you specify what the string format is.

In this case it would be new optional fields 'detailFormat' and 'documentationFormat` on the completion item. Values are of a number enum: 'PlainText' or 'Markdown'. The default would be 'PlainText'.

@jrieken
Copy link
Member

jrieken commented Aug 23, 2017

This will be enabled with the work we are doing for #29076

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 1, 2017

@jrieken Are you already looking into updating Suggest and/or Parameter hint to use MarkedString, or should I take a stab at it?

@jrieken
Copy link
Member

jrieken commented Sep 4, 2017

This is the issue to do that and I have scheduled it for September.

@jrieken jrieken self-assigned this Sep 7, 2017
jrieken added a commit that referenced this issue Sep 8, 2017
jrieken added a commit that referenced this issue Sep 8, 2017
Update internal api, update rendering of suggestion details, tweak shared renderer
@jrieken
Copy link
Member

jrieken commented Sep 8, 2017

The API is there, the UI is adopted, it is now up to the languages to return completion items and signature help that uses it. fyi @mjbvz

@jrieken jrieken closed this as completed Sep 8, 2017
@jrieken jrieken mentioned this issue Sep 12, 2017
@jrieken jrieken added the release-notes Release notes issues label Sep 29, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality on-testplan release-notes Release notes issues suggest IntelliSense, Auto Complete
Projects
None yet
Development

No branches or pull requests

4 participants