Make more hints for java available

[matthiasblaesing]

There are two PRs currently bitrotting: #2785 and #4522. Both add type information to the java editor view. The type information is shown like the parameter name hint, that is already part of NetBeans. For a visual example please have a look at the linked PRs.

This PR bundles the two, fixes a compilation problem and adds an option to enable and disable the hints individually:

[matthiasblaesing]

@neilcsmith-net @mbien @jtulach: What do you think? I agree, that var and chaining should not be abused and that code should not rely on IDE to help understanding. However there are times where you don't have influence on decisions or you want to understand better what the javac thinks about your code and then it can help to show the hints.

[neilcsmith-net]

Good to see enablement options. My opinion on #4522 is the same as stated there - could we bring it more in line with #2785 ? Hinting method return types, like chaining, at the end of lines would help with most cases of var while not screwing around with formatting within the line.

[mbien]

@matthiasblaesing I had no time to look at the code yet but having more granularity for this setting is great, +1 from me.

The short version about my opinion on inline hints taken from past discussions: I am not a fan of inline-text since the demand for them indicates problems with the code ("code smell"). Code has to be readable without them. Wanting to see the type of a var indicates that this probably should not be a var - swapping var with the type can sometimes even be more compact than letting the IDE draw the type next to var. Enabling those kind of hints by default is risky since it could influence how new devs write new code and lead to more var-overuse - if you see the type anyway, why not use var literally everywhere. The parameter hints have a similar issue (but this has smaller impact IMO) since they could influence formatting.

So how should an IDE make (somewhat) badly written code more readable without influencing how new code is written? I am not sure.

(one thought: if you really want to see the type of a var to understand the code better, why not draw the type instead of var with some kind of toggle button. This would be a pure readability aid and would not make sense to have always on while you write code. There is also the ctrl+p option to render param types on methods - what if this would appear by default if the cursor is over the method name (same could be done for var)? Maybe the answer to all this is to have some buttons in the editor toolbar to quickly toggle the setting on or off on demand instead of global options you set once?)

But more options is a good start +1 from me - good job Matthias. I wouldn't say no to having them on by default - I just would prefer not to due to the reasons given.

[mbien]

tested it and it works great. Please take a look at the lazyInit comment before merge.

[matthiasblaesing]

I pushed an updated version the changes:

• Rebases onto current master
• Adjust formatting
• Move lazy initialization in Utilities and SemanticHighlighterBase to double checked locking
• Move types for var statements to the end of line
• Fix rendering of "prepend-text" at EOL (cursor was display in prepend text and EOL special char was overprinted)
• Make typeinformation for var available as tooltip (independed of inline hints). The tooltip can be called when the cursor is one the var token and CTRL-P is pressed (it is the keystroke, that shows method parameters).

Result:

This has all inline hints enabled and shows the new tooltip

[neilcsmith-net]

Won't get a chance to test it properly until next week, but from the image and description of other changes, it looks great to me! Thanks for persevering.

[matthiasblaesing]

I'll merge this next weekend, if none objects before then.

[mbien]

took another look and it still looks good to me

[jlahoda]

Sorry, I missed this one. I unfortunately didn't have time to try yet. I think it is great to have configuration for the inline hints, seems fine with a few comments.

Besides the inline comment:
-I am not sure about the Ctrl-P - feels a bit unconceptial, it will be difficult to enhance this to other possible cases (like for implicitly typed lambda parameters, which, frankly, and not really different to vars). The tooltip for Ctrl-mouse is showing types of variables (and could be fixed to also work over var), and if a shortcut is needed to allow pure keyboard access, chossing a different action might be more viable for the long term.
-showing the type for var at the end of the line is a nice trick, but I am not sure if it will scale. Specifically, if we try to add the same feature for implicitly type lambda parameters, it will be hard to put the types at the end of the line. Ultimately, var will be the an outlier in how other similar things will be handled. (And, overall, we may need to add more stuff at the end of lines - debugging info comes to mind.)

[jlahoda]

Not sure if this listening and updating settings is really needed - it would probably be enough to read the settings at the start of the task. Listening on the change is often only needed to trigger re-run of the tasks when the settings change.

[matthiasblaesing]

Besides the inline comment: -I am not sure about the Ctrl-P - feels a bit unconceptial, it will be difficult to enhance this to other possible cases (like for implicitly typed lambda parameters, which, frankly, and not really different to vars).

Well - I used the existing code path, that runs through the completion (type tooltip) code path. From a code perspective it seemed logical. Though...

The tooltip for Ctrl-mouse is showing types of variables (and could be fixed to also work over var), and if a shortcut is needed to allow pure keyboard access, chossing a different action might be more viable for the long term.

I did not know of the CTRL-mouse way yet. It indeed looks like the right appoach. Could you give me a hint where this is handled (@jlahoda)?

For the placement of the var type - indeed from a readability perspective I think it would be better placed behind the var, though the same is true for the method invocations.

[neilcsmith-net]

showing the type for var at the end of the line is a nice trick, but I am not sure if it will scale. Specifically, if we try to add the same feature for implicitly type lambda parameters, it will be hard to put the types at the end of the line. Ultimately, var will be the an outlier in how other similar things will be handled. (And, overall, we may need to add more stuff at the end of lines - debugging info comes to mind.)

@jlahoda that depends a little whether you consider if it's conceptually the var type that should be hinted, or the return type of methods where it's chained or not assigned to the same explicit type. eg. could consider hinting where assignment is to a super type as well as var?

Personally I'm not sure completely inline hints scale anyway. Some users get confused by non-editable text appearing in amongst their text, and they affect formatting of code (eg. by default we show a right margin guide, which becomes less useful the more hints there are). Whereas there's at least more space at line ends to show information in a potentially less confusing way? Or we could consider vertical space instead?

I have less concerns where hints are not enabled by default though.

[jlahoda]

The Ctrl-mouse tooltips are computed here:

netbeans/java/java.editor/src/org/netbeans/modules/editor/java/GoToSupport.java

Line 137 in 9ae371e

public static String getGoToElementTooltip(final Document doc, final int offset, final boolean goToSource, final HyperlinkType type) {

It may be needed to add the var "keyword" into the list here:

netbeans/java/java.editor/src/org/netbeans/modules/editor/java/GoToSupport.java

Line 631 in 9ae371e

private static final Set<JavaTokenId> USABLE_TOKEN_IDS = EnumSet.of(JavaTokenId.IDENTIFIER, JavaTokenId.THIS, JavaTokenId.SUPER, JavaTokenId.ARROW);

It is all used from here (among other places): https://github.com/apache/netbeans/blob/master/java/java.editor/src/org/netbeans/modules/java/editor/hyperlink/JavaHyperlinkProvider.java

Thanks.

[matthiasblaesing]

@jlahoda thank you. Indeed the GoToSupport already covers my use case. In my tests it felt a bit flaky, but mostly works:

So I dropped that part from this PR.

The reading of the preferences was simplified and happens now on invocation.

@neilcsmith-net @mbien I returned to the original implementation for the var types. The moment you enable inline hints, the layout is already screwed. Maybe my taste is damaged from working with typescript, but having seen that syntax it looks more natural this way.

I still agree, that inline hints should not be needed, but I can imagine situations were I might be happy to have it.

The screenshot shows, that the rendering of the prepend text still works after simplification and now the "prepend text", the cursor and the end-of-line character are all fully rendered (fourth line).

[mbien]

i have been testing this PR with the following setup and it worked quite well:

• checked all boxes
• turned it off by default
• temp enabled it via hotkey while reading foreign code and I felt it would help

this avoids basically all issues I would encounter while writing code (since its off when i write code). like a cursor which jumps around the inline text which is something I would have to get used to first. It also doesn't interfere in how I write or format code - while aiding with code readability when needed. I really like that.

wondering if it would make sense to add a toolbar toggle icon to the editor toolbar so that this feature and particular usecase could be easier discovered. (doesn't have to be with this PR, just something to think about maybe?)

[neilcsmith-net]

+1 to merging, given we keep extra hints disabled. (I do understand that enabling hints screws the layout anyway, but there are degrees of that!)

I'd be inclined to -1 any further hints being displayed by default, at least as and until we've had a good conversation on usability and discoverability.

I do wonder, looking at some other IDEs, whether the push-to-hint approach is a better default one.

[jlahoda]

Looks good to me, thanks!

[matthiasblaesing]

Lets get this in. Thank you all for taking part in the discussion and review!