Bugzilla – Full Text Bug Listing
| Summary: | [Save actions] Correct indentation conflicts with formatter causing staircase effect | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Steven Post <redalert.commander> | ||||||||||||||
| Component: | Text | Assignee: | Noopur Gupta <noopur_gupta> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | daniel_megert, marcel.bruch, noopur_gupta, redalert.commander | ||||||||||||||
| Version: | 3.7 | Flags: | daniel_megert:
review+
|
||||||||||||||
| Target Milestone: | 4.5 M7 | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | 463596 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
Noopur, can you check whether your fix also covers this issue? (In reply to Dani Megert from comment #1) > Noopur, can you check whether your fix also covers this issue? This is a different case, not covered in the fix for bug 439582. Even this example will result in toggle on save when save actions are applied. Created attachment 251894 [details] Fix + tests Attached patch fixes this issue. Also, fixed the issue where 'correct indentation' was not taking the "Indent on column" policy for enum constants into account. This patch fix bug 458763 also for enum. Added new tests and all existing tests are green. Dani, please review. Created attachment 251896 [details]
Patch - to prevent toggle on save when formatter is applied on all lines
Attached patch adds a check so that 'correct indentation' is applied when either the formatter is not active in save actions or when the formatter is applied only on edited lines. It will prevent the toggle on save issue in cases where the formatter is active on all lines in save actions.
Dani, please have a look.
Created attachment 252004 [details] Fix + tests (In reply to Noopur Gupta from comment #3) > Created attachment 251894 [details] [diff] > Fix + tests > > Attached patch fixes this issue. Also, fixed the issue where 'correct > indentation' was not taking the "Indent on column" policy for enum constants > into account. This patch fix bug 458763 also for enum. > Added new tests and all existing tests are green. Dani, please review. Updated patch to remove newline at end of file from test file of bug 458763. (In reply to Noopur Gupta from comment #5) > Created attachment 252004 [details] [diff] > Fix + tests > Updated patch to remove newline at end of file from test file of bug 458763. This test (IndentActionTest#testBug458763) is green when IndentActionTest is run separately. Also, the test case behaves properly when checked with the example in an editor. However, on running the JdtTextTestSuite, it fails. I am looking into the issue. Created attachment 252035 [details] Fix + tests (In reply to Noopur Gupta from comment #6) > This test (IndentActionTest#testBug458763) is green when IndentActionTest is > run separately. Also, the test case behaves properly when checked with the > example in an editor. However, on running the JdtTextTestSuite, it fails. I > am looking into the issue. It happens as BreakContinueTargetFinderTest in JdtTextTestSuite uses ProjectTestSetup.setUp() that changes the default value of DefaultCodeFormatterConstants.FORMATTER_INDENT_SWITCHSTATEMENTS_COMPARE_TO_SWITCH to 'true'. Modified the test to set the above indent value to 'false' for this test. (In reply to Noopur Gupta from comment #7) > Created attachment 252035 [details] [diff] > Fix + tests + } finally { + project.setOption(DefaultCodeFormatterConstants.FORMATTER_CONTINUATION_INDENTATION_FOR_ARRAY_INITIALIZER, value); + } This is using the wrong constant. The patch goes into the right direction but it indents too much using the example in comment 0. (In reply to Dani Megert from comment #8) > (In reply to Noopur Gupta from comment #7) > > Created attachment 252035 [details] [diff] [diff] > > Fix + tests > > > + } finally { > + > project.setOption(DefaultCodeFormatterConstants.FORMATTER_CONTINUATION_INDENTATION_FOR_ARRAY_INITIALIZER, > value); > + } > > This is using the wrong constant. This should be corrected in the next patch. > The patch goes into the right direction but it indents too much using the > example in comment 0. This patch was implemented based on the current formatter behavior and expected changes in the formatter based on the comments in bug 458208. I have opened bug 463596 to discuss the correct behavior of formatter. This patch will be updated based on the result to follow the formatter. Created attachment 252631 [details] Fix + tests Updated patch based on bug 463596 comment #1. Dani, please have a look. (In reply to Noopur Gupta from comment #10) > Created attachment 252631 [details] [diff] > Fix + tests > > Updated patch based on bug 463596 comment #1. > Dani, please have a look. Looks good. Committed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=40eb8fc501f8fa2424809ab7a59ea58807fc5787 I suggest we keep the bug open since the staircase issue can still occur until the formatter has been adjusted. (In reply to Dani Megert from comment #11) > Looks good. Committed with > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=40eb8fc501f8fa2424809ab7a59ea58807fc5787 It's http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3a443bf6c995853505a6d457387e6d31e34b8346 After the fix, correct indentation works as expected as per the examples in bug 463596 and bug 463596 comment #1. Hence, closing this bug. |
Created attachment 226996 [details] formatter settings to reproduce Hi, Version: Juno Service Release 1 Build id: 20121004-1855 I noticed that the Save Action 'correct indentation' conflicts with the formatter 'line wrapping'. I attached my formatter configuration as reference. The issue can be seen when using arguments with enums. The formatter uses indentation so that all types are indented on the same column, as well as the arguments for the enums and arguments to those arguments (think arrays). Desired result (which the formatter does correctly): <pre> public enum TestEnum { FIRST_ENUM("first type", new SomeClass(), new OtherEnumType[] { OtherEnumType.FOO}), SECOND_ENUM("second type", new SomeClassOtherClass(), new OtherEnumType[] { OtherEnumType.BAR}), THIRD_ENUM("third type", new SomeThirdClass(), new OtherEnumType[] { OtherEnumType.BAZ}), FOURTH_ENUM("fourth type", new YetAnotherClass(), new OtherEnumType[] { OtherEnumType.FOOBAR, OtherEnumType.FOO, OtherEnumType.FOOBARBAZ, OtherEnumType.LONGERFOOBARBAZ, OtherEnumType.REALLYLONGFOOBARBAZ, OtherEnumType.MORELETTERSINHERE}); /* data members and methods go here */ } </pre> However when using the save action 'correct indentation', the 'SECOND_ENUM' start on the column of the arguments for 'FIRST_ENUM', the 'THIRD_ENUM' start on the column for arguments to the second one, and so on. This creates the dreaded staircase effect.