Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 338229 - [typing] Incorrect indentation in string continuation (press Enter in front of +)
Summary: [typing] Incorrect indentation in string continuation (press Enter in front o...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 3.7   Edit
Hardware: All All
: P1 normal (vote)
Target Milestone: 3.7 M7   Edit
Assignee: Rajesh CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-25 10:34 EST by Markus Keller CLA
Modified: 2011-04-26 10:45 EDT (History)
2 users (show)

See Also:
deepakazad: review-
daniel_megert: review+


Attachments
Patch with tests. (6.37 KB, patch)
2011-02-28 14:02 EST, Rajesh CLA
no flags Details | Diff
more readable tests (3.01 KB, patch)
2011-03-01 22:44 EST, Deepak Azad CLA
no flags Details | Diff
Patch with tests. (10.52 KB, patch)
2011-03-14 02:33 EDT, Rajesh CLA
daniel_megert: iplog+
daniel_megert: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2011-02-25 10:34:27 EST
HEAD (same problem with and without fix for bug 337150), was OK in 3.6.1

- paste this snippet into a project with default formatter settings:

package xy;
public class Try {
	public static void main(String[] args) {
		System.out.println("Some"
				+ new Object()
				+ "string:\n" + definedType.toString());
	}
}

- Select All
- Correct Indentation
=> indentation looks good

- put caret before last +
- press Enter
=> last code line is indented too much (2 tabs):

				+ "string:\n" 
						+ definedType.toString());
Comment 1 Markus Keller CLA 2011-02-25 10:38:48 EST
The wrong indent stays when I use Correct Indentation again.

The code formatter indents correctly (can be seen when you edit the default profile and check Line Wrapping > Never join already wrapped lines).
Comment 2 Deepak Azad CLA 2011-02-25 11:34:28 EST
(In reply to comment #0)
> HEAD (same problem with and without fix for bug 337150), was OK in 3.6.1
Bug 337150 was about 'pasting' a string, which is a slightly different problem and involves different code than this one.
Comment 3 Dani Megert CLA 2011-02-26 01:48:29 EST
(In reply to comment #2)
> (In reply to comment #0)
> > HEAD (same problem with and without fix for bug 337150), was OK in 3.6.1
> Bug 337150 was about 'pasting' a string, which is a slightly different problem
> and involves different code than this one.

It's probably the indent fix from bug bug 65317 that broke both scenarios.
Comment 4 Rajesh CLA 2011-02-28 14:02:39 EST
Created attachment 189981 [details]
Patch with tests.

Have rewritten and made more generic the method that was handling extra indentation for second line of string continuation, for e.g - 

String test =
	"this is the 1st string"
			+ "this is the 1st string"
			+ "this is the 1st string";

This is the one that was causing the issue. All the existing smart paste and other indentation tests pass. Also added more tests and modified one.
Comment 5 Deepak Azad CLA 2011-03-01 22:44:20 EST
Created attachment 190105 [details]
more readable tests

Firstly it was impossible to understand what the tests were testing without some comments. I have added some comments and made them more readable, please use the attached patch. This is also true for a number of older tests in JavaHeuristicScannerTest but I have not touched those.

testContinuationIndentationOfStrings1 – Why is indentation different at pos 73 and 84 ?  Shouldn’t both be “\t\t\t” ?

testContinuationIndentationOfStrings2 – third line – Are you sure it is “string:\n” but not “string:\\n” ?

testContinuationIndentationOfStrings2 - Shouldn't "definedType.toString()" on the new line before you check for indentation ?

- Use this snippet (first snippet in test testContinuationIndentationOfStrings1)
-----------------------------------------------------------------------
	String[] i = new String[] {
			"X.java",
			"public class X extends B{" 
					+ "test"
					+ "    public "};
----------------------------------------------------------------------

- Place caret after 'e' in 'test'
- Press enter
=> This is the result, the new line is indented too much.
---------------------------------------------------------------------
	String[] i = new String[] {
			"X.java",
			"public class X extends B{" 
					+ "te" +
							"st"
					+ "    public "};
---------------------------------------------------------------------

- Use this snippet (second snippet in test testContinuationIndentationOfStrings1)
---------------------------------------------------------------------
	String[] i = new String[] {
			"X.java",
			"public class X extends B{" + 
					"test" + 
					"    public " };
---------------------------------------------------------------------
- Place caret after "class X" 
- Press Enter
- Press Crtl+I => Indentation changes!
Comment 6 Dani Megert CLA 2011-03-04 03:40:49 EST
Ping!
Comment 7 Rajesh CLA 2011-03-08 01:46:16 EST
(In reply to comment #6)
> Ping!

The issues described by snippets in comment 5 are separate pre-existing issues. This is because Newline in a String is handled by completely different code (JavaStringAutoIndentStrategy.java). This class computes the indent for newline cases where the caret was in a string and is completely independent of the JavaIndenter which computes the indent for Ctrl+I, etc. I spent time looking at that code and found various issues there. We should handle these cases separately as it might make sense to modify that code to call the common JavaIndenter instead of separately computing the indent there.
Comment 8 Rajesh CLA 2011-03-14 02:33:39 EDT
Created attachment 191082 [details]
Patch with tests.

(In reply to comment #5)

> testContinuationIndentationOfStrings1 – Why is indentation different at pos 73
> and 84 ?  Shouldn’t both be “\t\t\t” ?

because pos 73 is second line of string continuation it gets extra indent, whereas pos 84 being third line of string continuation only aligns with the previous line.

> 
> testContinuationIndentationOfStrings2 – third line – Are you sure it is
> “string:\n” but not “string:\\n” ?

Actually neither - it should be "string:\\n\"

> 
> testContinuationIndentationOfStrings2 - Shouldn't "definedType.toString()" on
> the new line before you check for indentation ?
nope.

As mentioned in my previous comment, the issue described in the snippets is separate from this bug and requires a re-look at the way newline in a string is handled. I have still made a very small change to JavaStringAutoIndentStrategy.java to address most of the snippet scenarios.
Accepted the additions of comments to the test.
Comment 9 Dani Megert CLA 2011-04-13 08:45:00 EDT
Rajesh, the patch looks good.

Committed to HEAD.
Available in builds >= N20110413-2000.
Comment 10 Dani Megert CLA 2011-04-26 10:45:39 EDT
Verified in I20110425-1800.