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

Need @Value annotation live hover info in vscode #177

Closed
Eskibear opened this issue Jan 9, 2019 · 11 comments
Closed

Need @Value annotation live hover info in vscode #177

Eskibear opened this issue Jan 9, 2019 · 11 comments
Milestone

Comments

@Eskibear
Copy link
Contributor

Eskibear commented Jan 9, 2019

The value of the property may come from multiple sources, e.g. a local config file / a cloud config server. It's useful if I can know the value when the application is running.

Update:
previous screenshot is kind of misleading, removed. Now I find the Value hover info works well for Spring Boot 1.x but NOT for 2.x

Expected behavior should be:
image

@kdvolder
Copy link
Member

kdvolder commented Jan 9, 2019

Good idea. Thanks for the suggestion.

@Eskibear
Copy link
Contributor Author

Eskibear commented Feb 11, 2019

I think I've found the root cause. Let me share the findings.

1. The Value hover provider fails in SpringBoot 2.x projects. (But works well with Spring Boot 1.x)

It's due to the different data structures of the env returned by Acuator.
In 1.x the property sources are flatten, while in 2.x they are grouped into an array. E.g.
screen shot 2019-02-11 at 3 18 30 pm

But in ValueHoverProvider the logic is dedicated for 1.x projects.

JSONObject properties = allProperties.get(app);
Iterator<?> keys = properties.keys();
while (keys.hasNext()) {
String key = (String) keys.next();
if (properties.get(key) instanceof JSONObject) {

2. When hover on the @Value annotation itself instead of the property key, the hover provider doesn't work either.

I find the provider has been designed not to handle such cases for the moment. (In such case, exactNode is a SimpleName instead of a StringLiteral)

// case: @Value("prefix<*>")
if (exactNode != null && exactNode instanceof StringLiteral && exactNode.getParent() instanceof Annotation) {
if (exactNode.toString().startsWith("\"") && exactNode.toString().endsWith("\"")) {
return provideHover(exactNode.toString(), offset - exactNode.getStartPosition(), exactNode.getStartPosition(), doc, runningApps);
}
}
// case: @Value(value="prefix<*>")

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Nieraj Singh:)

The findings appear to be correct. We were not parsing live properties from env for Boot 2.x.

I have fixed one of the two issues listed above. We should now be able to get live properties when hovering over @value for Boot 2.x, IF a user hovers over the property itself. The live hovers should also show the source of the property, and will show multiple sources if the property comes from various sources.

However, the second issue, as to when to trigger live hovers still remains to be fixed. At the moment live hovers will only appear if you hover over the actual property itself, so for example in:

    @Value(value="${a.prop}")

if you hover over a.prop, then you will get live hover information, but not if you hover over value or the annotation itself.

@Eskibear
Copy link
Contributor Author

However, the second issue, as to when to trigger live hovers still remains to be fixed.

Actually I was trying to fix the second issue by adding logic for // case: @Value(value="${a.prop}") hover on value , and then I find the reason why the original author didn't do that.
E.g. @Value(value="${a.prop} concat ${b.prop}"), when you hover over value, what's live hover information supposed to be like? What's the expected behavior? I've no idea for the moment.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Martin Lippert:)

Nice. What about the hover hints? Do they show up for the range of the hover information, too? Maybe the prop name only? That would be quite awesome.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Nieraj Singh:)

@mlippert I'm working on trying to calculate hover hints for value properties when the watchdog requests highlight hints, and see if we can highlight the property name. I think it may require additional traversal of the AST during the live hover hints calculation, as to get to the StringLiteral node for the Annotation that is of interest to us to highlight. I'll update if I get it working.

@nierajsingh
Copy link
Collaborator

However, the second issue, as to when to trigger live hovers still remains to be fixed.

Actually I was trying to fix the second issue by adding logic for // case: @Value(value="${a.prop}") hover on value , and then I find the reason why the original author didn't do that.
E.g. @Value(value="${a.prop} concat ${b.prop}"), when you hover over value, what's live hover information supposed to be like? What's the expected behavior? I've no idea for the moment.

Although my following comment won't address what to do if a user hovers over the Annotation or value, as we haven't considered this yet especially with your example above, I think for the time being, we will try to at least highlight the property itself with a hint as to make it clear to the user that live information is available there.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Nieraj Singh:)

I've been able to add hint highlights for properties as seen in the attached screen shot in Pivotal Tracker (note: screenshot may not be visible in GitHub), so users will now be able to see when live hover information is available for @Value properties. Only the property names will be highlighted as that is the range where we provide live hover information. I think that is what we wanted for now.

I have pushed this to master so it should be available in the STS4 nightly snapshot to test.

Note that this only supports simple cases for properties in @Value expressions.

For more complex expressions that include multiple properties, this is not yet supported as the whole expression is still just one StringLiteral node in the JDT AST, so we would have to parse this expression String separately to obtain individual properties before we can provide both live hovers and hint highlights.

I recommend that we deliver this ticket for simple, single property cases in @value, and handle more complex SpEL expressions as an enhancement for later. I can raise a separate ticket to track this.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Martin Lippert:)

I tested this and it seems to work nicely. But I would like to see it handle multiple occurrences of properties in the same expression while proving the live hover hint.

So if I have something like:

	@Value("${server.port} ${something.else}")
	private String foo;

it should show the live hover hint on both referenced properties.

Interestingly, the hover itself works for the two cases, showing the right value for each property, but the hover hints disappear as soon as there is more than one occurrence.

This doesn't need a real SpEL parser or anything like that, it should just look for ${..} substrings, much in the same way than the hover itself is doing.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Nieraj Singh:)

I pushed some support for live hints for multiple properties. See attached screenshot.

Please check the STS4 nightly snapshot and let me know if it covers your cases.

@spring-projects-issues
Copy link

(comment in Pivotal Tracker added by Martin Lippert:)

Looks great !!!

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

No branches or pull requests

5 participants