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

Inline parameter name hints for Java #1247

Merged
merged 15 commits into from
Jun 14, 2019
Merged

Conversation

jlahoda
Copy link
Contributor

@jlahoda jlahoda commented May 12, 2019

For method calls, parameter names can be injected (in a virtual text) directly into the editor. Disabled by default, as this probably needs more experience tweaking. Enable using View/Inline Hints.

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I'd mention the new preferenes key name in arch.xml as <api group="property"/>.

The feature has to be on by default, in my opinion.

@jlahoda
Copy link
Contributor Author

jlahoda commented May 12, 2019

The feature has to be on by default, in my opinion.

Even though I agree in principle, I don't think the feature is in a state good enough to be enabled by default now. So either we wait with merging this until it is good enough, or we merge it as disabled by default, and flip the enabled/disabled by default switch when the time comes for that.

@AlexFalappa
Copy link
Contributor

Is it possible to have a small screenshot of this new editor feature?

@wumpz
Copy link

wumpz commented May 23, 2019

Is this features included within the next release of netbeans?

@jlahoda
Copy link
Contributor Author

jlahoda commented May 24, 2019

Screenshot:
inline-names

@junichi11 junichi11 added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label May 29, 2019
@neilcsmith-net
Copy link
Member

@jlahoda fantastic! 😄 I started looking at what was required to implement this myself a couple of months ago but got waylaid. So, what are your thoughts on merging as off-by-default for NB11.1 to get some testing in the wild, with a view to on-by-default in NB12.x if not before?

@svenreimers
Copy link
Member

Agreed, off by default for 11.1 if possible and on by default for 12?

@AlexFalappa
Copy link
Contributor

I very much agree on having this in NB11.1, even off-by-default.

Parameter names and in general in-line dynamic editor annotations/helpers are hype in competitor IDEs. This would allow to show that NB is not lagging behind.

Copy link
Member

@geertjanw geertjanw left a comment

Choose a reason for hiding this comment

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

Very cool. Off by default in 11.1, can this be done?

@geertjanw
Copy link
Member

Or, it seems to be off by default already? If so, let's merge this.

@geertjanw
Copy link
Member

OK, then. Merging.

@geertjanw geertjanw merged commit 8971234 into apache:master Jun 14, 2019
@Chris2011
Copy link
Contributor

Wow, this is exactly what we need for each language. I wanted to try to achieve that, but I didn't know how to create virtual text or readonly text inside the editor. Is there an public API/documentation for creating such virtual text to makes it easier @jlahoda?

@jlahoda
Copy link
Contributor Author

jlahoda commented Jun 23, 2019

@Chris2011, yes, there is an API. A short description is in the apichanges:
https://bits.netbeans.org/dev/javadoc/org-netbeans-modules-editor-lib2/apichanges.html#PrependedTextOpt

Basically, create a highlight that will have "virtual-text-prepend" as the highlight key, value is the virtual text. This is the place where it is used in Java, which may be useful as well:

@Chris2011
Copy link
Contributor

Great. Thx :)

@graben
Copy link
Contributor

graben commented Jul 10, 2019

It seems this patch only affect literals? Shouldn't it fire for every kind of parameter? Maybe configurable for performance reasons? :)

@sharpedavid
Copy link

sharpedavid commented Oct 9, 2019

It seems this patch only affect literals? Shouldn't it fire for every kind of parameter? Maybe configurable for performance reasons? :)

I bet the thought is that if you're passing in an actual variable, you should just a good variable name. Otherwise you have lots of instances of:

firstName.substring(beginIndex: beginIndex, endIndex: endIndex);

Edit: Just to reword, I'm saying that if your variable names are good, these hints would add a lot of screen clutter. I am still interested in this feature, but I am thinking it has some interesting unintended consequences.

// Example 1: Clutter
Car car = factory.buildCar(make: carMake, model: carModel);
car.drive(speed: fastSpeed, destination: nearestArbys)

// Example 2: Cleaner
Car car = factory.buildCar(carMake, carModel);
car.drive(fastSpeed, nearestArbys);

Please don't get too caught up deciding if you prefer example 1 or 2, my point is that it's probably a preference.

@neilcsmith-net
Copy link
Member

@graben was trying to find time to look at enhancing similarly - been mentioned various times by users. While I somewhat understand @sharpedavid comment, maybe there's a way to make it configurable? Also would be great to cover parameters that are method return values.

One area that would be good to drop duplication though is on varargs. Wonder how easy to just label the first?

@graben
Copy link
Contributor

graben commented Oct 10, 2019

Just copy the "specific" code (DRAFT :)) from visitliteral to visitidentifier and visitmethodinvokation does make the feature work for variables and methods return, too. I do not see any performance issues at the moment.

@graben
Copy link
Contributor

graben commented Oct 10, 2019

It seems this patch only affect literals? Shouldn't it fire for every kind of parameter? Maybe configurable for performance reasons? :)

I bet the thought is that if you're passing in an actual variable, you should just a good variable name. Otherwise you have lots of instances of:

firstName.substring(beginIndex: beginIndex, endIndex: endIndex);

It's just a simple if around the put of the hint :)

image

@Chris2011
Copy link
Contributor

I also had some troubles, that not always shows the inline hint. Will show an example later by reprodcution.

@robinsonrk
Copy link

Edit: Just to reword, I'm saying that if your variable names are good, these hints would add a lot of screen clutter. I am still interested in this feature, but I am thinking it has some interesting unintended consequences.

// Example 1: Clutter
Car car = factory.buildCar(make: carMake, model: carModel);
car.drive(speed: fastSpeed, destination: nearestArbys)

// Example 2: Cleaner
Car car = factory.buildCar(carMake, carModel);
car.drive(fastSpeed, nearestArbys);

Please don't get too caught up deciding if you prefer example 1 or 2, my point is that it's probably a preference.

Is there an option to configure the editor behavior in this case?

@Chris2011
Copy link
Contributor

There is just activating/deactivating afaik.

@eirikbakke
Copy link
Contributor

Just discovered this feature... very cool! It will be very useful during code review workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet