Community
Participate
Working Groups
N20101114-2000. 1. enable 'Never join already wrapped lines' in the formatter profile 2. paste this (all indents are tabs): public class Test { private void helper2( boolean[] booleans) { if (booleans[0]) { } } } 3. format (Ctrl+Shift+F) ==> correctly indents the third line with 3 tabs 4. now add some more indentation to line 3 (either tabs and/or spaces) 5. format (Ctrl+Shift+F) ==> BUG: the indentation of line 3 is not correct
Dani, I guess that if I said that's the expected behavior, you'll be a little bit surprised... but it's the case :-S Now, when not joining already wrapped lines, the formatter keeps the indentation of the wrapped line as soon as it is over the expected indentation (i.e. 3 tabs in your test case). This behavior, initially implemented to fix bug 286912 and avoid some observed instabilities, was also introduced to ensure that if user had manually increased the indentation of a wrapped line, he won't see the formatter put it down to a lower level. Typically, when users wrap lines they do not really expect the formatter to touch them after... Illustrating this with your test case: 1) Initial code: --------------- public class Test { private void helper2( boolean[] booleans) { if (booleans[0]) { } } } 2) Code after first formatting: ------------------------------ public class Test { private void helper2( boolean[] booleans) { if (booleans[0]) { } } } 3) Code after user increase the indentation: ------------------------------------------- public class Test { private void helper2( boolean[] booleans) { if (booleans[0]) { } } } Then, if user formats this last code again, I'm quite sure that he would be hurt if the formatter reset the indentation to 3 tabs! However, I definitely agree that the current behavior is not perfect as it does not really keep the user indentation but rounds it to the next tab (note that it only happens when 'Tabs only' is used for tab policy...). So, I'll fix this issue but I'll keep the current described behavior. Would you agree with this proposal?
I see the intention behind this but we should not add hidden smartness. The preference should only affect the line wrapping. It makes sense that users want to wrap a bit more than the formatter suggests but changing the default indentation seems just wrong and is probably something a user would see fixed when formatting the file. If we get users that complain about that and they can give good reasons for adding such a behavior/preference then we can look into this at that time.
(In reply to comment #2) > I see the intention behind this but we should not add hidden smartness. The > preference should only affect the line wrapping. It makes sense that users want > to wrap a bit more than the formatter suggests but changing the default > indentation seems just wrong and is probably something a user would see fixed > when formatting the file. > > If we get users that complain about that and they can give good reasons for > adding such a behavior/preference then we can look into this at that time. I think that good reasons for this behavior were: - bug 286601 - bug 286668 - bug 286912 Without it, we will face those issues again, hence this hidden smartness. Maybe that if the formatter would have untouched your extra indentation, you even didn't have thought to open a bug...
I don't see why you need the hidden behavior to fix the mentioned bugs. Look for example at bug 286601: this example is weird too now: if I further indent the opening brace then the closing one is touched (moved to the right to be aligned to the opening brace).
(In reply to comment #4) > I don't see why you need the hidden behavior to fix the mentioned bugs. Look > for example at bug 286601: this example is weird too now: if I further indent > the opening brace then the closing one is touched (moved to the right to be > aligned to the opening brace). I agree that current implementation is still obviously not perfect :-S However, we definitely need something fixing the formatter instabilities, as shown with the following example: public class Bug { public Bug(String string, Object object) {} public Bug foo() { Bug test = new Bug("makes formatter unstable", new Object() { /* (non-Javadoc) * @see java.lang.Object.toString() */ public String toString() { return "The formatter does not work!"; } }); return test; } } Format it with Eclipse build-in profile + 1) Tab policy = 'Spaces only' 2) 'Never join already wrapped lines' checked 3) Maximum line width = 60 4 All Brace positions set to 'Next Line' Then, you get: public class Bug { public Bug(String string, Object object) { } public Bug foo() { Bug test = new Bug("makes formatter unstable", new Object() { /* * (non-Javadoc) * * @see java.lang.Object.toString() */ public String toString() { return "The formatter does not work!"; } }); return test; } } Which is quite correct :-) However, format it again... and you get: public class Bug { public Bug(String string, Object object) { } public Bug foo() { Bug test = new Bug("makes formatter unstable", new Object() { /* * (non-Javadoc) * * @see java.lang.Object.toString() */ public String toString() { return "The formatter does not work!"; } }); return test; } } Observe some lines in the anonymous declaration which have moved 2 indentations to the left :-(( Note that this test was done after having removed all fixes in this area since bug 286601... That means current fixes are necessary but not good enough, hence I have to think about another more complete solution. I've well noted that this solution should not keep the current indentation but set it at the level designed by the current formatter profile...
>I've well noted that this solution should not keep the current indentation but >set it at the level designed by the current formatter profile... Yep.
(In reply to comment #5) To see the formatter instability using an "official" build, one need to use 3.5.1 as bug 286601 and others in the same area (see bug 291775) were fixed both in 3.6.0 and 3.5.2...
Created attachment 185836 [details] Proposed patch A definitely tricky patch to do... The main problem is that the current formatter design does not allow to entirely fix issues with 'Never Join Lines' preferences as the scribe needs complete ASTNodes tree information to know exactly what would be the correct indentation (got from the preferences) to apply on the "unjoined" line... Hence, I did my best to fix the original issue (of course) but also to minimize the impact on the existing behavior... Unfortunately, it was not always possible :-( The good news is that in 99% of changes, it was an improvement :-) In rare cases, it was a unexpected change but they look really not too bad (i.e. only an indentation increased/decreased by one or two). So, I finally decided to release the patch because it makes the 'Never Join Lines' preferences greatly more stable (around 210 unstabilities fixed vs HEAD while running all formatter massive regression tests). It also fixes many other problems I discovered while tuning the patch (the released one is the 40th version!): 1) comments in switch cases which were sometimes wrongly indented even when the 'Never Join Lines' preferences was not set, 2) some lines were still joined even with the 'Never Join Lines' preference activated, e.g.: class X { void foo(boolean condition) { if (condition) { // true } else { // false } } } Using the Eclipse [buil-in] + Never Join Lines preference, this sample was formatted as follows with 3.7M4: class X { void foo(boolean condition) { if (condition) { // true } else { // false } } } With this patch it's now formatted as: class X { void foo(boolean condition) { if (condition) { // true } else { // false } } } 3) It also fixes bug 317039
Released for 3.7M5 in HEAD stream.
Verified in I20110124-1800 that test case from comment 0 now works as expected.
Dani, Frederic, the code committed in scope of this bug has the effect that method calls inside binary expressions are indented in relation to the rest of the expression. before: if ((object.method(arguments) && object.method(arguments)) || (object.method(arguments) != 0 && object.method(arguments) && object .equals(something))) after: if ((object.method(arguments) && object.method(arguments)) || (object.method(arguments) != 0 && object.method(arguments) && object .equals(something))) please note the extra tab before equals. This has apparently been done deliberately, with the following code in Scribe.java, method createAlignment: + // specific break indentation for message arguments inside binary expressions + if ((this.currentAlignment == null && this.formatter.expressionsDepth >= 0) || + (this.currentAlignment != null && this.currentAlignment.kind == Alignment.BINARY_EXPRESSION && + (this.formatter.expressionsPos & CodeFormatterVisitor.EXPRESSIONS_POS_MASK) == CodeFormatterVisitor.EXPRESSIONS_POS_BETWEEN_TWO)) { + switch (kind) { + case Alignment.CONDITIONAL_EXPRESSION: + case Alignment.MESSAGE_ARGUMENTS: + case Alignment.MESSAGE_SEND: + if (this.formatter.lastBinaryExpressionAlignmentBreakIndentation == alignment.breakIndentationLevel) { + alignment.breakIndentationLevel += this.indentationSize; + alignment.shiftBreakIndentationLevel += this.indentationSize; + this.formatter.lastBinaryExpressionAlignmentBreakIndentation = 0; + } + break; + } + } I have people complaining about this change. Can you please give me a rationale on why was it done?
Piotr, if you think there is a bug, please open a new bug and explain the incorrect behavior. We can then take a closer look. Thanks.
