Community
Participate
Working Groups
Creating an anonymous inner class results in an system error. Example: Create a SWT Layout and add a button to it, e.g. via the following: Button button = new Button(parent, SWT.PUSH); now type button.addSelectionListener(new SelectionL and use Cntr+Space. JDT suggest to create the anonymous class for you. I you select the creation you end up with: --------- button.addSelectionListener(new SelectionListener() @Override public void widgetSelected(SelectionEvent e) { // TODO Auto-generated method stub } @Override public void widgetDefaultSelected(SelectionEvent e) { // TODO Auto-generated method stub }) ----- This results in a syntax error as the semicolumn is missing at the end of the statement.
That's not what I see using 3.7 M4. There it inserts the whole stub ending with a ';' which of course is also not correct.
I see the same as Lars in M4 and in HEAD with default content assist settings, e.g. with this self-contained snippet: import java.beans.PropertyChangeSupport; public class Snippet { void foo(PropertyChangeSupport pcs) { pcs.addPropertyChangeListener(new PCL) } } The result is technically correct, since you really didn't type the ";", and it's similar to what happens when you type "new Obj" in the arguments list and choose "new Object()" (with the difference that "new Object()" doesn't kill the outer linked mode, so you can just press Tab or Enter and then type the ";"). But I agree that the 99% use case is to have the semicolon at the end, so it would be worth streamlining this work flow.
(In reply to comment #1) The difference is that Lars and I _typed_ the "add*Listener(", so we got the closing ")" for free. When I copy/paste Lars' example without the ")", I also get a ";" for free at the end, but the missing ")" is not inserted before. But that case is IMO less annoying than the original case.
The current version at http://www.eclipse.org/downloads/ is 3.6.2.r362 in which the semicolon at the end of the "anonymous inner class template" is still missing (by using the default configuration): registerScreenView.setCancelButtonListener(new ClickHandle[Ctrl Space] => registerScreenView.setCancelButtonListener(new ClickHandler() { @Override public void onClick(ClickEvent event) { // TODO Auto-generated method stub } }) ^ In any case there BELONGS a semicolon. So a template change would be great =)
@Dani / Markus: Is this just a template change? If yes, could you point me to the template I need to change because I have this syntax error on a regular basis.
(In reply to comment #5) > @Dani / Markus: Is this just a template change? If yes, could you point me to > the template I need to change because I have this syntax error on a regular > basis. No, it's not just a template. We can't just add the ';' always, but need to be more sophisticated, as there might already be a ',' a '.' or a ')'. The ';' was there before but got removed in certain cases (like this one here). See also bug 280801 comment 7.
bug 198508 and bug 198748 which were related to adding semicolon to methods via content assist is fixed now. This feature is available in: Version: 4.3.0 Build id: I20130416-0800 In most of the examples listed in the current bug report the anonymous class is instantiated inside a void method like #addSelectionListener, #addPropertyChangeListener etc. With the above 2 bugs fixed, when the enclosing method is completed via content assist, a semicolon is added to the end of the method. Hence now we do not end up with a syntax error. Let us discuss if this fix solves at least some of the issues and also figure out additional/annoying points we need to handle as part of the current bug.
(In reply to comment #7) > bug 198508 and bug 198748 which were related to adding semicolon to methods > via content assist is fixed now. > > This feature is available in: > Version: 4.3.0 > Build id: I20130416-0800 > > In most of the examples listed in the current bug report the anonymous class > is instantiated inside a void method like #addSelectionListener, > #addPropertyChangeListener etc. With the above 2 bugs fixed, when the > enclosing method is completed via content assist, a semicolon is added to > the end of the method. Hence now we do not end up with a syntax error. > > Let us discuss if this fix solves at least some of the issues and also > figure out additional/annoying points we need to handle as part of the > current bug. I still see both bugs when using N20130422-2130. With both, I mean with or without the automatically added closing parenthesis, the generated code does not (yet) compile.
See also bug 357480.
Created attachment 231241 [details] Patch with testcases. As per the bug report, below are the cases handled with this patch. Button button = new Button(parent, SWT.PUSH); => button.addSelectionListener(new SelectionL| => button.addSelectionListener(new SelectionL|) => button.addSelectionListener(new SelectionL|); Also took care of the scenario mentioned in bug 357480. => class Bug324391 { private Runnable jobs[] = { new Run| }; } I came across 2 scenario for which if a fix is provided for one then it breaks the other, hence no semicolon is added for the below cases during code completion: => public class Bug1 { private Runnable jobs[]{ new Runna| } } => public class Bug2 { { new Runna| } } Some cases still fail if the code is written is different lines with unmatched separators like '(', '{' etc: => button.addSelectionListener( new SelectionL| I have added various testcases in CodeCompletionTest to cover different anonymous type completion cases. Dani, kindly review and let me know if the approach is right.
Would it be possible to create a Gerrit patch set for the suggested change?
(In reply to comment #11) > Would it be possible to create a Gerrit patch set for the suggested change? No. JDT UI is not on Gerrit yet and currently we prefer patches while we focus on Java 8. We might reconsider, once the new CLA is in established (see bug 401236).
Thanks Dani for the clarification. BTW: I see some Gerrit review requests for JDT: https://git.eclipse.org/r/#/q/status:open+JDT,n,z
(In reply to comment #13) > Thanks Dani for the clarification. > > BTW: I see some Gerrit review requests for JDT: > https://git.eclipse.org/r/#/q/status:open+JDT,n,z I see them too ;-). They are for JDT Core.
(In reply to Manju Mathew from comment #10) > Created attachment 231241 [details] [diff] > Patch with testcases. > I came across 2 scenario for which if a fix is provided for one then it > breaks the other, hence no semicolon is added for the below cases during > code completion: > > => public class Bug1 { > private Runnable jobs[]{ > new Runna| > } > } > => public class Bug2 { > { > new Runna| > } > } I see semicolon is added in both the cases even without the patch, which looks correct. Also, the first case is not a valid construct. - Consider the following example: public class Bug { { bar(new Runnable[] { new Runn| }); } void bar(Runnable[] rs) {} } Currently it adds an extra semicolon at the end of the inserted anonymous class, which has to be manually deleted. After the patch, the extra semicolon is added at the end of the existing Runnable array, which looks more confusing to understand what happened with content assist. - The following example gives expected result without the patch but adds an extra semicolon with the patch: class Bug11 { { bar(new Runnable(|)); } void bar(Runnable r) {} } > Some cases still fail if the code is written is different lines with > unmatched separators like '(', '{' etc: > => button.addSelectionListener( > new SelectionL| Dani, can we ignore these cases? > I have added various testcases in CodeCompletionTest to cover different > anonymous type completion cases. > Dani, kindly review and let me know if the approach is right. - The patch has some commented code which can be removed if not required: "doc.replace(posNonWSChar1 + 1, 0, SEMICOLON);" Also, since it is a big patch involving multiple cases, it would be easier to review the code if some more explanation is provided.
(In reply to Noopur Gupta from comment #15) > (In reply to Manju Mathew from comment #10) > > Created attachment 231241 [details] [diff] [diff] > > Patch with testcases. > > > I came across 2 scenario for which if a fix is provided for one then it > > breaks the other, hence no semicolon is added for the below cases during > > code completion: > > > > => public class Bug1 { > > private Runnable jobs[]{ > > new Runna| > > } > > } > > => public class Bug2 { > > { > > new Runna| > > } > > } > I see semicolon is added in both the cases even without the patch, which > looks correct. Also, the first case is not a valid construct. Missed the '=' in the first case, it should be: public class Bug1 { private Runnable jobs2[]= { new Runna| } } The above construct results in compiler error after the code completion(existing behavior). The second case already works(w/o patch), but if a fix is provided for the first case, then the second case was failing. Hence the patch did not provide a fix for the first case. > - The patch has some commented code which can be removed if not required: > "doc.replace(posNonWSChar1 + 1, 0, SEMICOLON);" > Also, since it is a big patch involving multiple cases, it would be easier > to review the code if some more explanation is provided. The commented code is example code snippets of the cases being handled. While working on this bug i did not come across a solution that fits all cases. I will rework on this and see what can be done.
Created attachment 244766 [details] Updated patch + Tests Here is the updated patch. The rule followed while creating this patch is, do not add unwanted/unnecessary semicolon. Still some cases are not fixed: public class NotCorrect{ private Runnable jobs[] = { new Runn } } Noopur, let me know when you are available for a code walk through.
(In reply to Manju Mathew from comment #17) > Here is the updated patch. Would it be possible to use Gerrit reviews? At least for me that is more convenient to read. In case you need help with the setup, let me know, happy to help.
(In reply to Lars Vogel from comment #18) > (In reply to Manju Mathew from comment #17) > > Here is the updated patch. > > Would it be possible to use Gerrit reviews? At least for me that is more > convenient to read. In case you need help with the setup, let me know, happy > to help. Lars, When i tried to push the changes to Gerrit i get an error saying "ssh://mmathew@git.eclipse.org:29418/gitroot/jdt/eclipse.jdt.ui.git: Auth fail". I tried the fix as suggested in http://wiki.typo3.org/Contribution_Walkthrough_with_EGit#.22Auth_fail.22, but didn't help. Any suggestions?
(In reply to Manju Mathew from comment #19) > Just to be on the safe side, you did upload your ssh key to Gerrit, as described in http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#eclipsegerritcontribution ?
(In reply to Manju Mathew from comment #17) > Created attachment 244766 [details] [diff] > Updated patch + Tests The patch fixes various scenarios related to semicolon addition. However, it creates issues in the following scenarios: - Put caret after 'new Run', press Ctrl+Space. Press Enter to choose Runnable(). class Runner1 { public static void main(String[] args) { new RuntimeException(); } } => "timeException" is lost in the result. - Put caret after 'new ', press Ctrl+Space. Press Enter to choose Runnable(). public class Runner { private Object foo(Runnable r) { System.out.println(foo(new ).toString()) return null; } } => A semicolon is added at wrong place. Few comments on code: - Rename ESCAPE_CHAR appropriately. - In AnonymousTypeCompletionProposal#getMatchingClosingSymbols, check if the condition around 'continue' is correct. Also, please verify that the patch does not break any working scenario by checking the cases in all the related bugs from "Show Annotations" action and/or History view.
