-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Add ts inline hints #113412
Add ts inline hints #113412
Conversation
69fc54f
to
25c5eac
Compare
25c5eac
to
3944756
Compare
Friendly ping @mjbvz . This PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all of your work on this feature!
I left a few comments for this PR. I'd also like the upstream TS pr to be merged in before we merge this
extensions/typescript-language-features/src/languageFeatures/inlineHints.ts
Outdated
Show resolved
Hide resolved
extensions/typescript-language-features/src/utils/configuration.ts
Outdated
Show resolved
Hide resolved
extensions/typescript-language-features/src/languageFeatures/inlineHints.ts
Outdated
Show resolved
Hide resolved
extensions/typescript-language-features/src/languageFeatures/inlineHints.ts
Outdated
Show resolved
Hide resolved
OKay. Thanks for the review. I'd like to update them after my work. |
Sounds a good idea. We might need something to maintain the range. Are there any existed things like that? |
This is some generic feedback on the editor inline hints implementation, not this TS-specific support, but I wasn’t sure where else to share it. I found the experience of splitting up a long function call with parameter name hints extremely disorienting, because I wanted to put my cursor before the hint to move it down a line with the argument. But the cursor position only exists after the hint, so the impression you get as you make a refactor like this is that you’re actually inserting a newline between the label and the argument it’s labeling. And indeed, there is a moment of delay before the label disappears and reappears on the new line: Kapture.2021-06-17.at.09.59.18.mp4I obviously understand what’s happening and how I need to adjust for it—I’m exaggerating my struggle in this screen cap to demonstrate the nature of why it feels unintuitive—but it really is kind of jarring. It seems to me that you’d either want to duplicate the cursor position on either side of these labels, so it appears as if there are two cursor positions, even though they’re actually the same position in the source text, or you’d want an API that allows these labels to be “sticky” on either the start or end, which would control which side of the label you can place the cursor. Prefix labels like these would be sticky on their end side, such that you can’t separate the label from the argument that follows it. This would result not only in a more intuitive editing experience, but also fewer reflows of label position mid-edit like you see in every newline insertion here. |
TypeScript side is ready for the hints service API and it will be a part of v4.4 (microsoft/TypeScript#42089). I have merged master and aligned with the latest changes. |
Co-authored-by: Daniel Rosenwasser <[email protected]>
@Kingwl very nice! |
Yep. You could wrote a typescript plugin to provide your own hints! (after this be done) |
Awesome, thanks for this @Kingwl! Suggestion: could the hints be optionally displayed in separate lines, above the actual code, so that they don't take up inline space? So that instead of taking up horizontal space, they'd take up vertical space. I currently use this feature in WebStorm, but sometimes it can be distracting when reading the code and doing selections. I think added lines would be less distracting. This also solves the mentioned above cursor position problem, because we're not actually placing anything inline with the code. Of course this could be an option, but I think if we're doing implementation from scratch, it might be good to challenge the status quo assumptions. I understand that then there would be the problem of the variable used as argument being shorter than the argument name - my solution to this would be to do an ellipsis abbreviation on that name and fit as much as possible, and have the full argument name show on hover. |
I assume your means something like codelens? Well, I guess it could. But I have some concerns about that: It may take up too much vertical space. I'm not pretty sure that would be a good experience. @jrieken might have some suggestions. |
"enum": [ | ||
"none", | ||
"literals", | ||
"all" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add descriptions for these too.
- None: Disable parameter name hints.
- Literals: Enable parameter name hints only for literal arguments.
- All: Enable parameter name hints for literal and non-literal arguments.
@DanielRosenwasser, can you think of a more approachable way to describe this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we can’t find a concise way to explain what literal expressions are or are not, these descriptions don’t add much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor feedback but looks good overall
extensions/typescript-language-features/src/languageFeatures/inlayHints.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
private someInlayHintsEnabled(model: vscode.TextDocument) { | ||
const config = vscode.workspace.getConfiguration(isTypeScriptDocument(model) ? 'typescript' : 'javascript', model.uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we typically register two providers: one for TS files and one for JS files. This can make registering and unregistering them a bit simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not pretty sure but I have some changes.
extensions/typescript-language-features/src/languageFeatures/inlayHints.ts
Outdated
Show resolved
Hide resolved
Thanks for all your amazing work here @Kingwl! This change will be in the upcoming VS Code 1.59 insiders and is scheduled to be release in VS Code 1.59. It will still require upgrading to TS 4.4. We should bundle TS 4.4 with VS Code 1.60 |
Hello everyone, this is a great addition. I would like to share my opinion on this :)
I could see how many people will have a different point of view on this feature, but to me and potentially others from disabling it, I think like the ability to control when and where the hint gets paced... Ability to enable/disable hints for:
I marked hints in PS: I did not test it. I judge from the view @andrewbranch posted. |
This is amazing and a huge wish coming true. Thanks a lot for doing this |
This PR fixes #113276
This PR should be merge after #113285