Community
Participate
Working Groups
Build Identifier: 20110615-0604 Bug 334652 missed fixing one spot -- the content-assist informational window does not display the javadoc, although the editor will be capable of doing so. Hopefully this can find its way into 3.7.1 or 3.7.2 as it affects the usability of our commercial product; it makes it tricky for users to understand one of our API points, which method to call. Reproducible: Always Steps to Reproduce: To reproduce, take the attachments from bug 334652 and simply add a new constructor, say: /** * javadoc for InnerFinalException(String property, Throwable cause) * @param property the property argument * @param cause the chained exception */ public InnerFinalException(String property, Throwable cause) { } then, you will notice that any call like InnerFinalException e = new InnerFinalException([invoke-content-assist] will display both constructors, but fail to display javadoc during the content assist chooser.
This is not limited to constructors. No methods inside a non-static member class gets any javadoc. Will investigate. Depending on the fix it will target 3.7.1 (getting late for this one) or 3.7.2.
Markus, a fix is required from the UI side in order to retrieve the right constructor.
Created attachment 201749 [details] Proposed fix The change in JDT/Core is required to get the javadoc for methods that are not constructors. The change in JDT/UI is required to select the right binary methods. Markus, There might be another way to fix the UI code. Let me know what you think.
My mistake. This is not sufficient for source methods.
Created attachment 201750 [details] Proposed fix + regression test This works better. The regression test won't run without some binary files that cannot be part of the patch.
Created attachment 201751 [details] bug354766.jar Should be added to: /org.eclipse.jdt.core.tests.model/workspace/AttachedJavadocProject
Created attachment 201752 [details] bug354766.jar javadoc Corresponding javadoc zip to put into the same location.
Created attachment 201753 [details] Proposed fix + regression test Same as previous patch but with all regression tests in org.eclipse.jdt.core.tests.model.AttachedJavadocTests enabled. The previous patch was enabling only the new test.
Released the part of the fix related to JDT/Core. Will release into 3.7.1 or 3.7.2 after review. Ayushman, please review.
I think we need an API method in the java model to ease the retrieval of IMethod inside an IType. The users should not have to go through the pain of checking the enclosing type (static or not, member type or not), the type of the method (constructor vs non-constructor), where it comes from (binary or source case). I'll try to see where this method would fit the best.
(In reply to comment #9) > Released the part of the fix related to JDT/Core. > Will release into 3.7.1 or 3.7.2 after review. > > Ayushman, please review. I have two comments: 1) I think the line anchor = anchor.substring(0, indexOfOpeningParen) + anchor.substring(index); should be left as it is. This is because for constructors with no arguments, we still need to remove the synthetic argument. So, if I add this constructor to the attachments from bug 334652 /** * javadoc for default const. InnerFinalException() */ public InnerFinalException() { } I still don't see any javadoc for this at the call site. 2) I don't think the variable 'depth' is needed anymore.
Created attachment 201974 [details] screenshot On a related note, I observed that if I see the outline of a non-static inner class inside a JAR in the package explorer view, I see the constructors with the synthetic argument. While if I just go to the source of that class (assuming source is attached), and check the "outline" view, the constructors do not have the synthetic argument. Markus, is this intended, or a bug?
(In reply to comment #11) > I have two comments: > 1) I think the line > anchor = anchor.substring(0, indexOfOpeningParen) + anchor.substring(index); > should be left as it is. This is because for constructors with no arguments, we > still need to remove the synthetic argument. So, if I add this constructor to > the attachments from bug 334652 Thanks. I'll fix this. And add regression test for it. > 2) I don't think the variable 'depth' is needed anymore. ok. A new patch will follow soon.
Created attachment 202006 [details] patch for MethodProposalInfo (In reply to comment #12) > On a related note, I observed that if I see the outline of a non-static inner > class inside a JAR in the package explorer view, I see the constructors with > the synthetic argument. That's a long standing issue, see bug 47354, bug 257508, bug 99137. In general, I think synthetic parameters should not show up in the UI and in the IMethods. Note that IMethodBinding is already correct. However, we already have workarounds for this in place e.g. in the JavaElementLabelComposer, so I guess we shouldn't stir up too much here. The changes in MethodProposalInfo look reasonable, although we should avoid allocating another array. See my patch for a simpler solution.
(In reply to comment #14) > The changes in MethodProposalInfo look reasonable, although we should avoid > allocating another array. See my patch for a simpler solution. Agreed. My patch was not meant to be released as is. It was a proof of concept that something was wrong in the way the argument were checked.
Created attachment 202008 [details] Complement + regression test. This fixes the issue reported by Ayushman. Ayushman, please review again. Thanks.
I released in HEAD the new binaries.
(In reply to comment #16) > Created attachment 202008 [details] [diff] > Complement + regression test. > > This fixes the issue reported by Ayushman. > Ayushman, please review again. > Thanks. Looks good. +1.
Daniel, let me know if you want to get this one for 3.7.2. I don't think this ever worked but I can understand that this is annoying for users with no workaround.
FWIW, as I(In reply to comment #19) > Daniel, let me know if you want to get this one for 3.7.2. I don't think this > ever worked but I can understand that this is annoying for users with no > workaround. As the original reporter, please get this into as soon a maintenance release as possible. What you need to remember is the use case of a no-source-provided library, like our product's API library, where w/out the javadoc displaying when they are selecting a method/constructor from auto completion, they end up having a bad experience not knowing which to choose (and sadly, yes, one of the methods that made me discover this bug is one that has two strings, making it really tough for them to know which string takes which argument!)
Created attachment 202020 [details] patch2 for MethodProposalInfo Here's a better patch for MethodProposalInfo that also works if the synthetic parameter disappears from IMethod APIs. Released to HEAD.
> Daniel, let me know if you want to get this one for 3.7.2. I don't think this > ever worked but I can understand that this is annoying for users with no > workaround. I don't understand the need for the new local variable 'depth'. Other than that I'm fine to fix it in 3.7.x. > Here's a better patch for MethodProposalInfo that also works if the synthetic > parameter disappears from IMethod APIs. Released to HEAD. Is this needed in case we decide to backport this?(In reply to comment #19)
(In reply to comment #22) > I'm fine to fix it in 3.7.x. Ok, targetting 3.7.2 as we are already in 3.7.1RC2. > Is this needed in case we decide to backport this?(In reply to comment #19) Yes, we need fixes from both JDT/UI and JDT/Core.
Srikanth, please review for 3.7.2 inclusion. Thanks.
Ayushman, please follow up for 3.7.2.
(In reply to comment #24) > Srikanth, please review for 3.7.2 inclusion. Thanks. +1 for 3.7.2 inclusion, Ayush, please arrange to release in maintenance as well as master.
Released in HEAD for 3.8M4 via commit e8dcde7054627e60ae6e2210fd96dea3c0d1010e
Created attachment 206108 [details] test project for verification Call InnerFinalException constructor from another project after adding the .jar file contained in above project and providing the javadoc location as the doc zip contained in this project.
Created attachment 206119 [details] patch for R_3_7_maintenance
Released in R3_7_maintenance for 3.7.2 via commit d0c82e031dc6efcbc5ff95f96250ec28c20f0922
Markus, please backport the jdt/ui part of the patch and then mark this as RESOLVED. Thanks!
Released comment 21 to R3_7_maintenance. commit b9f7b7c3be13a73658d77367f2a5dc0c3782788e
Verified for 3.8M4 with build I20111202-0800.
Verified for 3.7.2RC2 using build 20120118-0800.
