Community
Participate
Working Groups
Top-Level Bug for several quick-fix visibility bugs which can be fixed in one go.
Created attachment 223770 [details] Patch for all dependent bugs This patch resolves all of the dependent bugs. Please review and comment. I had to introduce a new method Bindings.findOverriddenMethods and only used it in one place. I think it should be used in favor of Bindings.findOverriddenMethod in several other places too.
Sorry, I ran out of time. I'll review it early M5.
(In reply to comment #2) > Sorry, I ran out of time. I'll review it early M5. M6, I promise.
No problem :) Do you need a reminder? ;)
(In reply to comment #4) > No problem :) Do you need a reminder? ;) No, I feel guilty every day ;-)
For your info: I started with the review. All our tests pass. Is it on purpose that the newly added ModifierCorrectionsQuickFixTest#testInvalidVisabilityOverrideMethod() passes even without your patch to the "real" code?
The patch is really good! I verified that it fixes all dependent bugs (and the duplicates mentioned in there). I only have a few trivial polish items that need to be addressed: - don't add findOverriddenMethods(IMethodBinding, boolean) - just call the method with 3 arguments) - always return a list (i.e. not 'null' from findOverriddenMethods(...), so that we don't need the null-checks in the clients - add Javadoc to findOverriddenMethods(...) - new members with Javadoc should get a @since 3.9 tag - space between "if" and "(" - space between "for" and "(" - space between ")" and "{" - "f.e." ==> "e.g." and with a space in front - copyright date needs to be 2013 in all touched files - improve the entry in the copyright (better summary and smaller URL): [quick fix] Visibility Several - https://bugs.eclipse.org/bugs/show_bug.cgi?id=394692 ==> [quick fix] Fix several visibility issues - https://bugs.eclipse.org/394692
Thx for doing the review. Will fix the stuff you mentioned on the weekend.
Hi Dani, some additional comments: - The test cases don't need a @since tag I guess? - ModifierCorrectionsQuickFixTest#testInvalidVisabilityOverrideMethod() passes without my patches, but if I recall correctly I introduced an error with one of the fixes which broke the tested scenario. That's why I used this test case to verify it will keep working. - Which formatter are you using? I didn't find a formatter on the project level and the eclipse default formatter changed too much. Fixed the formatting problems by hand now.
(In reply to comment #9) > Hi Dani, > > some additional comments: > - The test cases don't need a @since tag I guess? No, except when some Javadoc is added. Then we also add the @since tag. > - ModifierCorrectionsQuickFixTest#testInvalidVisabilityOverrideMethod() > passes without my patches, but if I recall correctly I introduced an error > with one of the fixes which broke the tested scenario. That's why I used > this test case to verify it will keep working. k. > - Which formatter are you using? I didn't find a formatter on the project > level. There is a formatter profile set and when I start a new workspace, clone the jdt.ui repo and import jdt.ui, I do get the formatter on the project (see attached screenshot). Note that you can enable to format just the changed lines on save (see 'Save Actions' preference page; not enabled on the project itself).
Created attachment 227529 [details] Picture of formatter setting
Created attachment 227842 [details] Revision of Patch I've changed all the points mentioned in Daniels review.
Comment on attachment 227842 [details] Revision of Patch UnresolvedVariablesQuickFixTest.testVarParameterAccess() now fails when I apply the patch on 'master'. It looks like you broke something in UnresolvedElementsSubProcessor. Some other trivial issues: - there's a typo: Get's the higher visibility of the given parameters. ==> Gets the higher visibility of the given parameters. - the Javadoc should have an empty line before the tags start In order to catch the M6 bus, I'd need to have a new patch by EOD tomorrow.
Created attachment 228227 [details] 2nd Revision of Patch I'm really sorry, my last patch didn't include any changes in the UnresolvedElementsSubProcessor. EGit just ignored it and didn't make a patch entry and I didn't check if everything is included.... Still learning to use Git, and not feeling comfortable yet. I hope everything is okay now.
(In reply to comment #14) > Created attachment 228227 [details] [diff] > 2nd Revision of Patch ... > I hope everything is okay now. That patch does not cleanly apply on 'master'. I've fixed that, so that we can proceed. > - the Javadoc should have an empty line before the tags start But not between each tag. I've fixed that. Also tweaked some of the Javadoc. I've committed the tweaked patch with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=0e8156a0ee59f04b2e265d8bdbdea1497f47fee3
*** Bug 65876 has been marked as a duplicate of this bug. ***
*** Bug 87239 has been marked as a duplicate of this bug. ***
*** Bug 94152 has been marked as a duplicate of this bug. ***
*** Bug 94755 has been marked as a duplicate of this bug. ***
*** Bug 216898 has been marked as a duplicate of this bug. ***
*** Bug 341209 has been marked as a duplicate of this bug. ***
Thanks for your review and help Dani! Hope to provide a better patch at the first try next time.
(In reply to comment #22) > Thanks for your review and help Dani! Hope to provide a better patch at the > first try next time. Don't worry, the first try was already actually pretty good. Thanks again.
