Community
Participate
Working Groups
Created attachment 271979 [details] Screenshot Eclipse 4.8 M4 introduced inlined annotation support. See https://www.eclipse.org/eclipse/news/4.8/M4/#inlined-annotations-support It would be great if JDT would allow to show the parameter names as additional information via this support. This is a feature available in IntelliJ and very useful. See screenshot for its usage in IntelliJ. Angelo Zerr suggested to open a enhancement request for this support.
Thanks @Lars to have created this bug. Just one question, the parameter names annotations are displayed only for the selected line or for all lines of the editor which have methods?
(In reply to Angelo ZERR from comment #1) > Thanks @Lars to have created this bug. Just one question, the parameter > names annotations are displayed only for the selected line or for all lines > of the editor which have methods? There are displayed for all method invocations in the current editor.
(In reply to Lars Vogel from comment #2) > There are displayed for all method invocations in the current editor. Last time I saw IJ in action, it was displayed only for some methods (not all). I tried to understand when it was written and when it wasn't and have the impression they were only shown when various arguments have the same type which is a more error prone situation.
(In reply to Mickael Istria from comment #3) > (In reply to Lars Vogel from comment #2) > > There are displayed for all method invocations in the current editor. > > Last time I saw IJ in action, it was displayed only for some methods (not > all). I tried to understand when it was written and when it wasn't and have > the impression they were only shown when various arguments have the same > type which is a more error prone situation. I just checked and indeed IJ does not always show them. I think if we add the option to show them, we should show them always. Conditional behavior is not good design IMHO.
Found this feature at https://www.jetbrains.com/idea/whatsnew/ --------------------------------------------------------------------------- Parameter names To make the code even more readable, the editor now shows hints for the parameter names in method calls where the meaning of the arguments may not be clear from the context. --------------------------------------------------------------------------- It's a very cool feature:) @Mickael I think CodeMining should support LineHeaderAnnoation (like today) & LineContentAnnoation both. We have a usecase now.
(In reply to Angelo ZERR from comment #5) > It's a very cool feature:) @Mickael I think CodeMining should support > LineHeaderAnnoation (like today) & LineContentAnnoation both. We have a > usecase now. Yes, it would be nice to have it. And I believe the current APIs are not that far from being able to support both.
Perhaps LineHeaderAnnotation & LineContentAnnation should be merged in one AbstractInlinedAnnotation which should provide the capability to switch to inlined type (header, content). What do you think @Mickael?
(In reply to Angelo ZERR from comment #7) > Perhaps LineHeaderAnnotation & LineContentAnnation should be merged in one > AbstractInlinedAnnotation which should provide the capability to switch to > inlined type (header, content). What do you think @Mickael? While they could have some code in a common parent class, I don't think it's worth merging them as those are actually different "types" of annotation, with different constraints, constructors, behavior... I also don't see any use-case for dynamically changing the rendering from "header" to "content". To me, merging them wouldn't help usage nor maintenance.
(In reply to Lars Vogel from comment #4) > I just checked and indeed IJ does not always show them. > I think if we add the option to show them, we should show them always. > Conditional behavior is not good design IMHO. In the vast majority of cases, one really doesn't need parameter names hint because the typing clarifies all possible ambiguity. Think about `list.set(2, item)`. This code is pretty easy to understand without additional info. So it's noisy and distracting for no value. Now imagine some utility method such as `SearchEngine.search(text, content)`: it's really not clear which is what, hence showing `SearchEngine.search(sourceText: text, patternToSearch: content)`, then it's really useful. I believe IJ's approach is actually the best of both. That said, that's an implementation detail and a preference can simply allow to enable nowhere/everywhere/when typing doesn't help only.
(In reply to Mickael Istria from comment #9) > I believe IJ's approach is actually the best of both. +1 > That said, that's an implementation detail and a preference can simply allow > to enable nowhere/everywhere/when typing doesn't help only. +1
what are "type parameter types"?
(In reply to Stephan Herrmann from comment #11) > what are "type parameter types"? type parameter names
@Angelo: Do you have a patch set ready for this one?
This is very useful feature for Eclipse, It might be more useful if it can be enabled for other plugins on SDK level so developers would have this support out of the box
(In reply to Roman Parashchyn from comment #14) > This is very useful feature for Eclipse, It might be more useful if it can > be enabled for other plugins on SDK level so developers would have this > support out of the box It can be enabled on any TextEditor. See https://www.eclipse.org/eclipse/news/4.8/M5/#codemining-extension-point for more details about this addition and examples of how to enable it in your own editor, or in the Generic Editor.
(In reply to Mickael Istria from comment #15) > (In reply to Roman Parashchyn from comment #14) > > This is very useful feature for Eclipse, It might be more useful if it can > > be enabled for other plugins on SDK level so developers would have this > > support out of the box > > It can be enabled on any TextEditor. See > https://www.eclipse.org/eclipse/news/4.8/M5/#codemining-extension-point for > more details about this addition and examples of how to enable it in your > own editor, or in the Generic Editor. Thanks for pointing to documentation. Are there any plans to add this marker like? so you can just create a marker and set additional attributes that will make it either header or inline and provide text to be shown.
(In reply to Roman Parashchyn from comment #17) > Are there any plans to add this marker like? so you can just create a marker > and set additional attributes that will make it either header or inline and > provide text to be shown. Not until you mentioned it ;) Please open a dedicated enhancement request to track this idea.
Micael created according ticket https://bugs.eclipse.org/bugs/show_bug.cgi?id=531296
> @Angelo: Do you have a patch set ready for this one? Let's me clean my code and I will do a gerrit patch which will include too this https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825 @Mickeal please help me https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825#c10
New Gerrit change created: https://git.eclipse.org/r/117592
@Mickael, I have created a gerrit patch for this issue at https://git.eclipse.org/r/#/c/117592/ Please note this gerrit patch includes too this https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825 and it misses comments (code is not cleaned), but I think I hope it will help you to understand how I manage this issue. If you have better idea, don't hesitate to tell me. According your review, I will comment my code (sorry my code is not cleaned).
Even if parameter name can be supported by current PLatform CodeMining (in jdt-codeminig it is supported), this issue depends on https://bugs.eclipse.org/bugs/show_bug.cgi?id=531769 to have better renderer of inlined content annotation
(In reply to Lars Vogel from comment #12) > (In reply to Stephan Herrmann from comment #11) > > what are "type parameter types"? > > type parameter names In a method like <T> Set<T> Collections.emptySet() <T> is a type parameter, "T" would be its name. In this bug I have found no indication that this is the subject of the bug, hence correcting the summary. Perhaps, the intention is to show parameter types and parameter names?
New Gerrit change created: https://git.eclipse.org/r/138941
New Gerrit change created: https://git.eclipse.org/r/139892
New Gerrit change created: https://git.eclipse.org/r/140190
Gerrit change https://git.eclipse.org/r/140190 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=7a31dce408b9acbeb6c0785f182e885ed4d9d916
@Mickael, is the println necessary at https://git.eclipse.org/c/platform/eclipse.platform.text.git/tree/org.eclipse.jface.text/src/org/eclipse/jface/text/source/inlined/InlinedAnnotationDrawingStrategy.java?id=7a31dce408b9acbeb6c0785f182e885ed4d9d916#n267 or is it left over from debugging. When I try the change on the JDT side I see a significant amount of : StyleRange {2952, 1, fontStyle=normal, foreground=Color {0, 0, 0, 255}, metrics=GlyphMetrics {0, 0, 72}} ... being printed to STDERR.
New Gerrit change created: https://git.eclipse.org/r/140244
New Gerrit change created: https://git.eclipse.org/r/140245
(In reply to Roland Grunberg from comment #29) > @Mickael, is the println necessary at > https://git.eclipse.org/c/platform/eclipse.platform.text.git/tree/org. > eclipse.jface.text/src/org/eclipse/jface/text/source/inlined/ > InlinedAnnotationDrawingStrategy. > java?id=7a31dce408b9acbeb6c0785f182e885ed4d9d916#n267 or is it left over > from debugging. Sorry! It's obviously the later one (garbage from debugging), I've just pushed https://git.eclipse.org/r/140245 to remove it before M1 deadline!
Gerrit change https://git.eclipse.org/r/140245 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=e23a034baf163e8269235c90ffc766551598cd42
Gerrit change https://git.eclipse.org/r/138941 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=71e7d3ed753cbba81df7204c8848ca6d035c7e2b
Gerrit change https://git.eclipse.org/r/139892 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=4f5305367fee6f20d054d6f4e0d0b49c5e16da97
Thank you very much Roland for your careful and patient and super useful reviews! I'm working on the documentation right now, to have it in M1 before deadline.
New Gerrit change created: https://git.eclipse.org/r/140249
Gerrit change https://git.eclipse.org/r/140249 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=ddb36ac76b4c6f640c6e03f46b7f8fa1875d9ebc
Ok, we're done here. Thanks to everyone involved, especially Angelo for the inception of this feature and Roland for the reviews!
It's a very great news, thanks @Mickael and @Roland!
Thanks, Master Mickael and Roland for this. One feature less that does drive people to IntelliJ.
New Gerrit change created: https://git.eclipse.org/r/140268
Gerrit change https://git.eclipse.org/r/140268 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=304a6037c93be0994ebb87edb5ded102ba26b03e
The new test fails on all platforms: https://download.eclipse.org/eclipse/downloads/drops4/I20190408-1800/testresults/html/org.eclipse.jdt.text.tests_ep412I-unit-cen64-gtk3-java8_linux.gtk.x86_64_8.0.html Code mining not available on expected chars junit.framework.AssertionFailedError: Code mining not available on expected chars at junit.framework.Assert.fail(Assert.java:57) at junit.framework.Assert.assertTrue(Assert.java:22) at junit.framework.TestCase.assertTrue(TestCase.java:192) at org.eclipse.jdt.text.tests.codemining.ParameterNamesCodeMiningTest.testCollapsedFoldingAndToggleHighlight(ParameterNamesCodeMiningTest.java:182)
> Thanks, Master Mickael and Roland for this. One feature less that does drive people to IntelliJ. We have discussed with Mickael about the filter topic in private, it's not the priority, but IMHO I think filter parameter name should be available to avoid having feedback "Eclipse is less intelligent than IJ" because it shows too many parameter name/type. I hav estarted to implement that, see https://github.com/angelozerr/jdt-codemining/issues/61
*** Bug 546222 has been marked as a duplicate of this bug. ***
About the tests, it currently assumes that test is running normally, in an actual Windowing System, receives event from the WS for redraw... It works on Gerrit with Xvnc. How are those happening in the "official" build? I see a lot of occurence where the behavior seems pretty different and make things fail in the main build when they succeed everywhere else.
(In reply to Mickael Istria from comment #47) > About the tests, it currently assumes that test is running normally, in an > actual Windowing System, receives event from the WS for redraw... It works > on Gerrit with Xvnc. It fails for me if just running the test form the IDE. I'm on RHEL 7.4 / X11, direct access, no vnc.
(In reply to Andrey Loskutov from comment #48) > It fails for me if just running the test form the IDE. I'm on RHEL 7.4 / > X11, direct access, no vnc. Lucky you ;) Could you please try just growing the timeouts for DisplayHelper to some higher value and see how it impacts stuff?
(In reply to Mickael Istria from comment #49) > (In reply to Andrey Loskutov from comment #48) > > It fails for me if just running the test form the IDE. I'm on RHEL 7.4 / > > X11, direct access, no vnc. > > Lucky you ;) > Could you please try just growing the timeouts for DisplayHelper to some > higher value and see how it impacts stuff? Time is not the problem. I see that StyleRange has null metrics, so condition is never met.
(In reply to Andrey Loskutov from comment #50) > Time is not the problem. I see that StyleRange has null metrics, so > condition is never met. The metrics are supposed to be added after code-mining is resolved (running in a different thread) and then a paintEvent is honored and invokes the annotition's IDrawingStrategy. If you have the opportunity to place a breakpoint on JavaMethodParameterCodeMiningProvider#L61, it could help in reducing the scope of potential culprit. Or may, on ParameterNamesCodeMiningTest#L181, place a breakpoint and then invoke DisplayHelper.sleep(widget.getDisplay(), 30000) from Debug Shell to see whether the project has some classpath or compilation issue that I don't have here.
New Gerrit change created: https://git.eclipse.org/r/140297
(In reply to Eclipse Genie from comment #52) > New Gerrit change created: https://git.eclipse.org/r/140297 That is the fix.
Gerrit change https://git.eclipse.org/r/140297 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5573bbd6933649a2f28dc52a2b2a15c181eb851f
Test is OK now.
Created attachment 278206 [details] Screenshot - random I20190409-1800 The parameter names are missing randomly in many places. Steps: - Check all the checkboxes in code minings preferences. - In JDT UI code, open UnresolvedElementsSubProcessor.addRequiresModuleProposals(...). - Click on '5 references' mining annotation. Check the parameter names in all the references. Only some of the names are available at random locations. See attached screenshot.
(In reply to Noopur Gupta from comment #56) > Created attachment 278206 [details] > Screenshot - random It's actually not random, it follows some rule that are hardcoded in the code. But I understand it can be confusing. Can you please open a separate bug on this topic so we can discuss some better rules (or maybe better label for the preference)?
(In reply to Mickael Istria from comment #57) > (In reply to Noopur Gupta from comment #56) > > Created attachment 278206 [details] > > Screenshot - random > > It's actually not random, it follows some rule that are hardcoded in the > code. > But I understand it can be confusing. Can you please open a separate bug on > this topic so we can discuss some better rules (or maybe better label for > the preference)? If you look at the screenshot: - The parameter name "cu" is shown in one reference call (left side) and not in the other one (right side). I was expecting it to be consistent at least for a particular parameter name. - The left side call shows only first parameter name and the right side call shows only the last parameter name. Is this expected based on the hardcoded rules?
Seems like if the parameter value is some kind of SimpleName/Literal that contains the parameter name, then the annotation is turned off. Intuitively it makes sense because there's no point hinting at some parameter being "start" if I'm giving it nodeStartOffset. On the other hand it does create some inconsistency.
(In reply to Noopur Gupta from comment #58) > Is this expected based on the hardcoded rules? Yes. The rule, at the moment, is https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/tree/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/CalleeJavaMethodParameterVisitor.java#n98 . It skips the codemining if the value contains the parameter name as substring. So 'cu' parameter is not shown for 'cu' parameter value, same for 'node' and 'relevance' is not shown for 'IPoposalRelevance...'
It definitely creates inconsistency and confusion for the users. Please check if this can be improved in some way for M1. Otherwise, create a separate bug for M3 to discuss.
(In reply to Noopur Gupta from comment #61) > It definitely creates inconsistency and confusion for the users. Please > check if this can be improved in some way for M1. Otherwise, create a > separate bug for M3 to discuss. I'm not available to work on it for M1. I suggest that people who find it confusing open the bug and explain their confusion and expectation as a starter for the discussion ;)
>>But I understand it can be confusing. >>It skips the codemining if the value contains the parameter name as substring. The exact matching scenarios are OK but the substring match case is not very obvious and is confusing. I will update the N&N entry to reflect the same. >>we can discuss some better rules (or maybe better label for the preference)? Or may be even skip all rules altogether ( my personal preference). I will open a bug to discuss that.
Let's close the ticket and discuss the rules of when showing/not showing and better explain it to user for M3, in 546311
I will update the N&N entry so that the new users are well informed of what they would see. ( or rather what they won't see) while using this new feature.
New Gerrit change created: https://git.eclipse.org/r/140412
Gerrit change https://git.eclipse.org/r/140412 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=3a0162ad602721a155ef347422dff59b40948892
*** Bug 485542 has been marked as a duplicate of this bug. ***
