Bugzilla – Full Text Bug Listing
| Summary: | [typing] Incorrect indentation in string continuation | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||||
| Component: | Text | Assignee: | Rajesh <rthakkar> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P1 | CC: | daniel_megert, Olivier_Thomann | ||||||
| Version: | 3.7 | Flags: | deepakazad:
review-
deepakazad: review+ |
||||||
| Target Milestone: | 3.7 M6 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
This works correctly in 3.6. I guess, if the first line does not contain anything else other than the string there is no need to indent the second line. Dani - based on our discussion, I looked at this further and it still remains tricky. Here is why - 1. As per formatter, second line of string continuation should be indented. 2. The Indenter does that in some cases but not all (which is a known issue that needs to be fixed) The 2 options we have are either to not indent the second line(just need to make a small change) or handle SmartPaste as a special condition (as we had discussed). Looked at it in more detailed and discussed with Markus. We should update the logic when pasting as follows:
1. compare the indent of first and second line of pasted stuff
2a. first line indent >= second line ==> only use first line to compute indent
2b. first line indent < second line use second line to compute indent and
also adjust/indent first line according to formatter rules
For the example in comment 0 it means that depending on the string that's pasted, the result from comment 0 might or might not be expected:
If the selection contains:
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"
(note the leading indent) then the result should be as Deepak expects. Same with this selection:
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"
However, if the selection contains:
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"+
"this is the new 2nd string"
then the (expected) result is as outlined in comment 0.
Note that we can also see/test this with a simpler example:
class A {
void foo() {
String array = "this is the 1st string"
+ "this is the 1st string"
+ "this is the 1st string";
}
}
1. fully select all three lines of foo's body
2. copy
3. paste
==> the result in this case should be:
class A {
void foo() {
String array = "this is the 1st string"
+ "this is the 1st string"
+ "this is the 1st string";
}
}
In case the user selects from the first non-whitespace character to the last non-whitespace character in the body, the result should be:
class A {
void foo() {
String array = "this is the 1st string"
+ "this is the 1st string"
+ "this is the 1st string";
}
}
Created attachment 189577 [details]
Patch with tests.
The patch works as outlined in comment 4. Code review - rename firstLineCurrentIndent to firstLineIndent - In JavaAutoIndentStrategyTest the already existing tests- testPasteDefaultAtEnd(), testPasteFooAtEnd(), testPasteLongStringWithContinuations() - need an assert statement. - testPasteLongStringWithContinuations and the new tests should have similar names. - javaAutoIndentStrategy, documentCommand, accessor can be converted to fields and initialized in the constructor - reduces lines of code - accessor.invoke(...) calls can be extracted to a method e.g. performPaste() - reduces code duplication and makes it easy to read the tests. Created attachment 189742 [details]
Patch with tests.
Thanks for the patch! Few minor things - Fields should be prefixed with ‘f’ - In testPasteAndIndentOfLongStringWithContinuations1() “\n” inside the string should be escaped as per the test case in bug 330556 - It is also a good practice to mention bug numbers in the tests. - Added one more test for the case when first line indent < second line. Committed the patch to HEAD with these changes. Verified that pasting works as outlined in this bug, but there's now bug 338229 regarding the auto-indent on 'Enter'. Verified in I20110310-1119 (there's still bug 338229). I still have issues regarding indentation. Go to: org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest.test293() Copy the expected output string and replace it with "". Now select "" and paste the string you copied before. I still get an identation that I don't expect. (In reply to comment #12) > I still have issues regarding indentation. > Go to: org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest.test293() > Copy the expected output string and replace it with "". > Now select "" and paste the string you copied before. > I still get an identation that I don't expect. Olivier, can you please file a new bug. Best with a little snippet, so that one doesn't have to load the whole JDT Core test project. Thanks. See bug 347840 for a remaining issue. |
HEAD Started with -------------------------------------------------------------------- package p; class A { void foo() { String[] array ={ "this is the 1st string"+ "this is the 1st string"+ "this is the 1st string"+ "this is the 1st string"+ "this is the 1st string", "this is the 2nd string"+ "this is the 2nd string"+ "this is the 2nd string"+ "this is the 2nd string"+ "this is the 2nd string" }; } } --------------------------------------------------------------------- Replace "this is the 2nd string"+ "this is the 2nd string"+ "this is the 2nd string"+ "this is the 2nd string"+ "this is the 2nd string" with "this is the new 2nd string"+ "this is the new 2nd string"+ "this is the new 2nd string"+ "this is the new 2nd string"+ "this is the new 2nd string" => Result is ----------------------------------------------------------------------------- package p; class A { void foo() { String[] array ={ "this is the 1st string"+ "this is the 1st string"+ "this is the 1st string"+ "this is the 1st string"+ "this is the 1st string", "this is the new 2nd string"+ "this is the new 2nd string"+ "this is the new 2nd string"+ "this is the new 2nd string"+ "this is the new 2nd string" }; } } ----------------------------------------------------------------------------- All the lines after the second one are indented by 2. (This works fine if the 1st string is replaced) The concrete use case can be seen in org.eclipse.jdt.core.tests.compiler.regression.JavadocBugsTest.testBug128954(). Thanks Olivier for pointing this out.