Inline Hints controller and API proposal #113285

Kingwl · 2020-12-22T10:38:10Z

This PR will
fixes #113276
enables #16221

This reverts commit 741a568.

Kingwl · 2020-12-23T07:09:46Z

It's basically work on editor with new provider. But there are many things todo.
Signature arguments label is a temporary name. And I think we could confirm the name of the api.

Kingwl · 2020-12-24T04:35:16Z

I think it's basically work on code side. We may going to improve

protocol
- ~~naming (feature name and protocol changes)~~
user experience
- ~~default styles~~
- customized style
- ~~refresh schedule~~
- may hide labels when user is editing some line.
performance
- ~~Should we add all decorations in a file? or only in the viewport~~
- Do we need some cache?
code styles
- ~~Add missing comments~~
- ~~Lint~~
docs
- I'm not sure what should i do
tests
- I'm not sure too

jrieken · 2020-12-24T09:16:32Z

/cc @CyrusNajmabadi

Thanks for starting this! This is quite an effort and you have come very far already 👏. I didn't do a full review yet (and likely won't before early January) but a few quick notes:

There exists a matching Roslyn API, they call this InlineHints which I think is a good name
Inline hints shouldn't be computed for the whole document but for ranges (the currently visible range). So the provide API must be like this provideInlineHint(document: TextDocument, range: Range, token: CancellationToken). The range should be the current view port. This will allow for better performance when dealing with large/huge files. However, this will make the editor contribution slightly more complex because you need to listen to view port changes, e.g ICodeEditor#onDidScrollChange
(optional) Inline hints can target a position or a range. A "position-hint" shows in-between the text (as you have implemented it), a range-hint overlays the text when a special mode is entered. Note that we can treat this as a separate work package and leave it out of this PR
I understand how the TypeScript implementation helps you drive this but at the end this should be spilt into two separate PRs - one for the API/feature and one for the TypeScript adoption
When adding new API don't put it in vscode.d.ts but in vscode.proposed.d.ts. Also use the checkProposedApiEnabled-function to guard proposed API from un-allowed usage (sample)
A few code nits. In addition to our coding guidelines each feature area owner has their own style preferences. For me that's: (1) prefix all private methods and fields with an underscore for better readability, (2) don't use the public-keyword because things are public by default, (3) don't extend from Disposable, use a DisposableStore-property instead. It's not urgent to make these changes but I would make them otherwise myself

jrieken · 2020-12-24T09:17:16Z

more popular related issue: #16221

jrieken · 2020-12-24T09:18:06Z

for reference the Roslyn/C# IInlineHintsService and friends: http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/InlineHints/IInlineHintsService.cs,79bf00b7826d13e4

Kingwl · 2020-12-24T09:20:34Z

Thanks for the review.

I'll do them soon.
Enjoy your vacation. Merry Christmas to you in advance.

Kingwl · 2020-12-24T09:53:39Z

Currently it's show the before decorations only.
Sould we support after decorations too?
If so, It will be more common.

Eg:

jrieken · 2020-12-24T11:20:34Z

Currently it's show the before decorations only.

I think that's OK for now.

Enjoy your vacation. Merry Christmas to you in advance.

Thank you.

CyrusNajmabadi · 2020-12-24T12:04:09Z

LMK how i can be of assistance? I implemented this for VB and C# in Roslyn, and we'd obviously want the same computation to be portable over to whatever is created for VSCode/LSP.

Kingwl · 2020-12-24T12:04:42Z

@CyrusNajmabadi To take some review I guess.

CyrusNajmabadi · 2020-12-24T12:05:03Z

src/vs/vscode.d.ts

+		/**
+		 * The name of the parameter.
+		 */
+		name: string;


note: in roslyn we support classified text in our 'hints', not just plain text.

We also support the user hovering over these hints to get additional information (for example, what quick-info would show for teh entity in the hint).

for reference: https://github.com/dotnet/roslyn/blob/18d8e7c9a135e0b042a079823d9ef95c63b84b33/src/Features/Core/Portable/InlineHints/InlineHint.cs#L16-L18

CyrusNajmabadi · 2020-12-24T12:08:11Z

Note: roslyn supports two modes for inline-hints (and allows hte languages to specify different behavior depending on the mode). The first mode is that the user can request the hints always be shown. THe second is that the hints are shown only when a certain keybinding is held down (in our case alt-f1).

Depending on which mode the user activates, we show different hints. For example, say the user has the code:

var v = Frob();

With the mode on to just always show hte hints, we'll display that as:

var v |string| = Frob();

In this manner, the inline-hint only augments the text, but never affects your ability to interact with anything. However, if the user holds down alt-f1 we will display this as:

|string| v = Frob();

Here, we actually overwrite a portion of teh user's text (the var) with the hint. This is not a problem and does not impact typing as it only is shown while the user is holding alt-f1.

src/vs/editor/common/standalone/standaloneEnums.ts

src/vs/editor/contrib/signatureArgumentsLabel/signatureArgumentsLabel.ts

CyrusNajmabadi · 2020-12-24T12:14:49Z

In terms of presentation, it looks like this in VS:

Kingwl · 2020-12-24T12:15:28Z

Note: roslyn supports two modes for inline-hints (and allows hte languages to specify different behavior depending on the mode).

I don't think that's a good idea to implement the overwrite mode now....
It's could be another iteration.

…self contained if at all)

…ecorations, and much 💄

jrieken · 2021-01-18T15:58:45Z

I have started in depth reviewing and also pushed a bunch of commits. Sorry, but I also removed some thing that got added in last days, namely:

Removal of the context menu for inline hints. That's not saying I don't like the idea of commands but this shouldn't be done via context keys and menu
Removal of the special hover tricks. Decorations already show hovers
Removal of some double decorations

I have also pushed a few cosmetic changes so that it is easier for me to work with these changes in the future. I will spend some more time on this branch and then I'll be ready to merge the current state.

Kingwl · 2021-01-18T16:07:30Z

Thank you very much. I'll read the changes too.

Kingwl · 2021-01-18T16:15:02Z

Cool. It's much simple and clear. Are there anything else can/need I do?

Kingwl · 2021-01-18T16:19:32Z

Removal of the special hover tricks. Decorations already show hovers.

I have tried to assign hover message into decoaration options. And there's no tooltip show.
And after debug. I think It's caused by zero length range - same as the colorPicker.
Another problem is before options does not have hoverMessage( Ok we could add one ).

PS. It's based my debug experience. So please let me know if there is any incorrect.

jrieken · 2021-01-18T16:53:58Z

I have tried to assign hover message into decoaration options. And there's no tooltip show.

Yeah, that is correct. It works if you give a non empty range but my point is that it's an editor limitation and should be treated as such. E.g the editor should simply support such decorations with hover. I know it is tricky because ::before/after nodes don't really show up as event targets. So, some tricks are needed. I'll make sure to talk to the right folks here

jrieken

There is still some follow-up work:

overlay mode
testing

I am planning on tackling that after merging

…d tests

jrieken · 2021-01-19T07:43:22Z

Merged. This means we have proposed API for this and a first variant of the UI (inbetween rendering).

Thanks @Kingwl for this great contribution

Kingwl · 2021-01-19T08:33:45Z

I'm also happy to add more test cases or doc too!

Kingwl added 2 commits December 22, 2020 18:34

WIP

8476ff1

Avoid generated files

741a568

vscode-triage-bot assigned joaomoreno Dec 22, 2020

jrieken assigned jrieken and unassigned joaomoreno Dec 22, 2020

Kingwl added 3 commits December 23, 2020 01:00

Add controller

52c1cce

Revert "Avoid generated files"

434f631

This reverts commit 741a568.

Add dummy support

9f5cfc2

Kingwl added 2 commits December 23, 2020 18:51

Simplify code

3de96fd

Use utils type converter

a6fd2cc

Kingwl added 3 commits December 24, 2020 15:19

Avoid generated file

bf4e237

Avoid changes

ff8ae0b

Improve comments and code style

d24ab0a

jrieken changed the title ~~[WIP]: Support signature arguments label~~ [WIP]: Inline Hints controller and API proposal Dec 24, 2020

CyrusNajmabadi reviewed Dec 24, 2020

View reviewed changes

src/vs/editor/common/standalone/standaloneEnums.ts Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 24, 2020

View reviewed changes

src/vs/editor/contrib/signatureArgumentsLabel/signatureArgumentsLabel.ts Outdated Show resolved Hide resolved

Kingwl and others added 5 commits January 18, 2021 17:48

Avoid extra fields

1c131cf

Merge branch 'master' into signaure_arguments_label

756337d

don't propose new API on ThemableDecorationAttachmentRenderOptions

ae67879

remove hover (should come via decoration) and action/menu (should be …

0d4bf78

…self contained if at all)

less state inside InlineHintsController-type, only have one type of d…

1981776

…ecorations, and much 💄

💄

5087b08

jrieken added 4 commits January 18, 2021 17:28

simpler decoration type management (rely on internal ref counting)

4651f66

padding should depend on font size too

1d3b03b

use all of context decoration as decoration type key

d65ab8d

clamp font size at editor font size, don't go bigger

7a93867

jrieken self-requested a review January 18, 2021 17:09

jrieken approved these changes Jan 18, 2021

View reviewed changes

jrieken added this to the January 2021 milestone Jan 18, 2021

jrieken added the editor-contrib Editor collection of extras label Jan 18, 2021

jrieken and others added 2 commits January 18, 2021 18:41

add API command vscode.executeInlineHintProvider and some end-to-en…

a40b4e7

…d tests

Merge branch 'master' into signaure_arguments_label

30f17c9

Kingwl mentioned this pull request Jan 19, 2021

Add inlay hints support microsoft/TypeScript#42089

Merged

10 tasks

jrieken merged commit d29bb62 into microsoft:master Jan 19, 2021

Kingwl deleted the signaure_arguments_label branch January 19, 2021 08:24

jrieken added the feature-request Request for new features or functionality label Jan 19, 2021

jrieken mentioned this pull request Jan 20, 2021

Upstream inlay hints rust-lang/rust-analyzer#2797

Closed

Kingwl mentioned this pull request Feb 8, 2021

Add support for Inline Parameter Name Hints OmniSharp/omnisharp-roslyn#1932

Closed

github-actions bot locked and limited conversation to collaborators Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline Hints controller and API proposal #113285

Inline Hints controller and API proposal #113285

Kingwl commented Dec 22, 2020 •

edited by jrieken

Loading

Kingwl commented Dec 23, 2020 •

edited

Loading

Kingwl commented Dec 24, 2020 •

edited

Loading

jrieken commented Dec 24, 2020

jrieken commented Dec 24, 2020

jrieken commented Dec 24, 2020

Kingwl commented Dec 24, 2020 •

edited

Loading

Kingwl commented Dec 24, 2020 •

edited

Loading

jrieken commented Dec 24, 2020

CyrusNajmabadi commented Dec 24, 2020

Kingwl commented Dec 24, 2020

CyrusNajmabadi Dec 24, 2020

CyrusNajmabadi Dec 24, 2020

CyrusNajmabadi Dec 24, 2020

CyrusNajmabadi commented Dec 24, 2020 •

edited

Loading

CyrusNajmabadi commented Dec 24, 2020

Kingwl commented Dec 24, 2020

jrieken commented Jan 18, 2021

Kingwl commented Jan 18, 2021

Kingwl commented Jan 18, 2021

Kingwl commented Jan 18, 2021

jrieken commented Jan 18, 2021

jrieken left a comment

jrieken commented Jan 19, 2021

Kingwl commented Jan 19, 2021 •

edited

Loading

Inline Hints controller and API proposal #113285

Inline Hints controller and API proposal #113285

Conversation

Kingwl commented Dec 22, 2020 • edited by jrieken Loading

Kingwl commented Dec 23, 2020 • edited Loading

Kingwl commented Dec 24, 2020 • edited Loading

jrieken commented Dec 24, 2020

jrieken commented Dec 24, 2020

jrieken commented Dec 24, 2020

Kingwl commented Dec 24, 2020 • edited Loading

Kingwl commented Dec 24, 2020 • edited Loading

jrieken commented Dec 24, 2020

CyrusNajmabadi commented Dec 24, 2020

Kingwl commented Dec 24, 2020

CyrusNajmabadi Dec 24, 2020

Choose a reason for hiding this comment

CyrusNajmabadi Dec 24, 2020

Choose a reason for hiding this comment

CyrusNajmabadi Dec 24, 2020

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 24, 2020 • edited Loading

CyrusNajmabadi commented Dec 24, 2020

Kingwl commented Dec 24, 2020

jrieken commented Jan 18, 2021

Kingwl commented Jan 18, 2021

Kingwl commented Jan 18, 2021

Kingwl commented Jan 18, 2021

jrieken commented Jan 18, 2021

jrieken left a comment

Choose a reason for hiding this comment

jrieken commented Jan 19, 2021

Kingwl commented Jan 19, 2021 • edited Loading

Kingwl commented Dec 22, 2020 •

edited by jrieken

Loading

Kingwl commented Dec 23, 2020 •

edited

Loading

Kingwl commented Dec 24, 2020 •

edited

Loading

Kingwl commented Dec 24, 2020 •

edited

Loading

Kingwl commented Dec 24, 2020 •

edited

Loading

CyrusNajmabadi commented Dec 24, 2020 •

edited

Loading

Kingwl commented Jan 19, 2021 •

edited

Loading