Community
Participate
Working Groups
I20121120-0800 public class Try { public static void main(String[] args) { main(new String[] { "abc", "def",| ); } // end of main } When I replace | with the caret and then press the return key, the result is: public class Try { public static void main(String[] args) { main(new String[] { "abc", "def", ); } } // end of main } Expected: public class Try { public static void main(String[] args) { main(new String[] { "abc", "def", | }); } // end of main } => "Automatically close {Braces}" should not put the } after the rest of the current line. It works fine as long as the array initializer only contains at most one argument. Maybe related to bug 254704. In some situations, the behavior is not completely predictable. When I have main(new String[] { "abc",| , then the } is sometimes correct and sometimes at the bad position after the );.
*** Bug 200015 has been marked as a duplicate of this bug. ***
(In reply to comment #0) Bug is in JavaAutoIndentStrategy.computeAnonymousPosition(IDocument, int, String, int). > It works fine as long as the array initializer only contains at most one > argument. Maybe related to bug 254704. > > In some situations, the behavior is not completely predictable. When I have > > main(new String[] { "abc",| > > , then the } is sometimes correct and sometimes at the bad position after > the );. The above method calls JavaAutoIndentStrategy.looksLikeAnonymousClassDef(IDocument, String, JavaHeuristicScanner, int) to check if the content at the position looks like an anonymous class definition. In the case when the caret is immediately next to ',' without any space: main(new String[] { "abc",| the method looksLikeAnonymousClassDef(...) looks for a ',', '(' or '=' backwards at a position starting from closing " and then looks for keyword 'new'. Hence, it finds that and considers it as an anonymous class definition, so the } is placed correctly. In the case when the caret is placed after a space from ',': main(new String[] { "abc",<space>| the method looksLikeAnonymousClassDef(...) looks backward starting at ',' itself and hence doesn't find 'new' after that. So the } is placed incorrectly. The same is true for cases where more than one argument is present. ( ',' is found and no 'new' found after that). However, in my understanding, the method looksLikeAnonymousClassDef(...) is to be used for only an anonymous class definition. For the current bug, would it be good to write a similar method: looksLikeAnonymousArrayDecl(...)? It would check if the content is of the form: [ ',', '(', '='] new <Type>[]{... Also, the bug 254704 would need special handling for annotations as the current method computeAnonymousPosition(...) returns a value other than -1 only if the content is an anonymous class definition or has 'new' token as in the first case. Dani/Markus, please suggest a suitable way.
(In reply to comment #2) Good findings! Since #looksLikeAnonymousClassDef is only used inside the auto-indent strategy, we can simply fix the implementation to also handle the case from this bug. Afterwards, we can fix bug 254704.
I didn't look at the code, but "looksLikeAnonymousArrayDecl" would definitely be wrong. Java doesn't have "anonymous arrays". The proper term for that would be "array initializer", see the JLS. This is an area where we have to apply guesses, since the code can have arbitrary syntax errors. To reduce surprises, make sure the strategy for adding smartness is a positive one. I.e. only do the special-case handling if you have a positive pattern hit as close as possible to the caret position. If in doubt, don't add smartness. For anonymous type completions, the grammar in ClassInstanceCreation and AnonymousClassDeclaration tells that you need at least this pattern to create an anonymous class: new <Type> ( <Args> ) { ..., where <Type> and <Args> can vary. The placeholders can contain "<>()", but these always have to nest properly. => The strategy should be to include the top-level "()" in the pattern matching; not to exclude cases that match "new <Type>[]{".
Created attachment 226769 [details] Patch Thanks Dani and Markus for the comments. Fixed JavaAutoIndentStrategy.computeAnonymousPosition(...) and the offset passed to it. The patch handles the case from this bug 395071 and also fixes the bug 254704. Also, verified that no regression is caused by checking the cases in related bugs from the past: bug 29379, bug 237466 and bug 256087. Dani, please check the behavior and the patch.
Comment on attachment 226769 [details] Patch (In reply to comment #5) > Created attachment 226769 [details] [diff] > Patch > > Thanks Dani and Markus for the comments. > Fixed JavaAutoIndentStrategy.computeAnonymousPosition(...) and the offset > passed to it. > > The patch handles the case from this bug 395071 and also fixes the bug > 254704. > Also, verified that no regression is caused by checking the cases in related > bugs from the past: bug 29379, bug 237466 and bug 256087. > > Dani, please check the behavior and the patch. It does not fix the duplicate bug 200015. Test Case: 1. paste this into the 'Package Explorer': enum ReviewResult { Good , Bad } 2. start to add a body to 'Good': Good {<caret>, Bad 3. press 'Enter' ==> enum ReviewResult { Good { , Bad } } instead of: enum ReviewResult { Good { }, Bad }
Noopur, please also add additional test cases to 'org.eclipse.jdt.text.tests.JavaAutoIndentStrategyTest'.
Created attachment 227117 [details] Patch (In reply to comment #6) The updated patch fixes bug 200015 and other such cases where no '(' or ')' is involved in the expression. Example: 1. enum ReviewResult1 { Good {|, Bad } 2. int[] a= new int[] {|; 3. {| int a; Currently in such cases, the code in the rest of the line is copied to the next line before adding '}'. Hence, only example 3 gives the expected result and examples 1, 2 give the wrong result. The patch checks the next non-whitespace character on the line after the caret position where 'Enter' is pressed. If the char is a ',' or ';', we first insert '}' and then move the rest of the code after it. Also, renamed the method computeAnonymousPosition(...) to copyContent(...) and changed the return type to boolean since the returned value is only used to check if the old content has to be copied or not. (In reply to comment #7) Added test cases to 'org.eclipse.jdt.text.tests.JavaAutoIndentStrategyTest'.
*** Bug 254704 has been marked as a duplicate of this bug. ***
Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=cf0c4beeb2326627bec6196f67c096b928022cf8
