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

Support for textDocument/InlayHint #593

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Mar 11, 2022

Support for textDocument/InlayHint

Fixes #595

Signed-off-by: azerr azerr@redhat.com

@angelozerr
Copy link
Contributor Author

This PR requires redhat-developer/vscode-quarkus#474

@angelozerr
Copy link
Contributor Author

Here the result:

image

It display Java type before the alias.

It's just a POC and it doesn't use the real InlayHint of LSP4J but uses the structure like described in the spec.

The inlay hint doesn't appear when the Java type is not resolved. So you must type something in the text editor to refresh the inlay hint.

@rgrunber
Copy link
Member

Nice :)

Only 2 things immediately stand out.

  1. We should think about the best place to put the hint. Todo todo or todo Todo, or maybe todo : Todo, or todo (Todo). Basically it might cause some confusion if it just appears there.
  2. I would play around with typing out the full line and confirm it isn't annoying if the hint appears as someone is typing. Somtimes the logic can resolve a hint at various points that causes the cursor to jump.

@angelozerr angelozerr force-pushed the inlayhint branch 2 times, most recently from d6f2dae to 9c32c1d Compare March 13, 2022 06:07
@angelozerr
Copy link
Contributor Author

@rgrunber here the new renderer:

image

@angelozerr angelozerr changed the title Support for InlayHint Support for textDocument/InlayHint Mar 13, 2022
@angelozerr angelozerr force-pushed the inlayhint branch 6 times, most recently from 2ec4cbc to 4709ec4 Compare March 14, 2022 09:30
@angelozerr angelozerr marked this pull request as ready for review March 14, 2022 09:31
@fbricon
Copy link
Collaborator

fbricon commented Mar 14, 2022

please don't use package org.eclipse.lsp4j.proposed; this is not an eclipse project nor is it code copied from an eclipse project.

@angelozerr
Copy link
Contributor Author

please don't use package org.eclipse.lsp4j.proposed; this is not an eclipse project nor is it code copied from an eclipse project.

rename with com.redhat.lsp4j.proposed. Once LSP4J will provide Inlay Hint, we will able to remove those classes.

@angelozerr angelozerr force-pushed the inlayhint branch 5 times, most recently from 546c257 to 31c57b5 Compare March 15, 2022 10:46
@angelozerr angelozerr mentioned this pull request Mar 16, 2022
@angelozerr angelozerr marked this pull request as draft March 17, 2022 21:22
@angelozerr angelozerr force-pushed the inlayhint branch 4 times, most recently from 7711fb0 to 0556249 Compare April 1, 2022 09:19
@angelozerr angelozerr marked this pull request as ready for review April 1, 2022 09:21
@angelozerr angelozerr requested a review from AlexXuChen April 1, 2022 09:31
@angelozerr
Copy link
Contributor Author

Here a new demo with inlay hint:

InlayHintDemo2

Now I support only section parameter (and method parameter type has been removed, we will see in an another PR the method parameter name support).

This PR requires redhat-developer/vscode-quarkus#474 which provide settings to enable/disable inlay hints + provide inlay hint TypeScript by waiting for the release of vscode-languageclient which will provide this feature.

Fixes redhat-developer#595

Signed-off-by: azerr <azerr@redhat.com>
Copy link
Contributor

@AlexXuChen AlexXuChen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, working as intended

@angelozerr angelozerr merged commit cec54cf into redhat-developer:master Apr 1, 2022
@angelozerr
Copy link
Contributor Author

Thanks @AlexXuChen for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for textDocument/InlayHint
4 participants