Bug 458208

Comment 1 Mateusz Matela 2015-01-23 05:48:48 EST

When I said "the new formatter doesn't align asterisks at comment line beginnings when comment formatting is disabled", I meant it as a feature - don't format comments should mean leave them alone, so aligning asterisks is not desired. A lot of modifications for tests in bug 303519 was related to this. It's a non-breaking change (it will not cause sources formatted with the old formatter to be changed by the new formatter) so I didn't mention it.

So I think we should change the scenarios you used Manoj, if they are somewhere in the repos, otherwise there's nothing to do :)

Comment 2 Markus Keller 2015-02-11 13:43:41 EST

In the org.eclipse.jdt.ui.tests.refactoring.all.AllAllRefactoringTests, there's one failure in InlineTempTests18#test1().

Reduced test case (inline variable "fi"):
-------
package p;
import java.util.function.IntUnaryOperator;
class TestInlineLambda1 {
	{
		IntUnaryOperator fi = x -> x;
		int hi = fi.hashCode();
	}
}
-------
Result with a space too much after "x -> x":

		int hi = ((IntUnaryOperator) x -> x ).hashCode();

The underlying reason is this behavior in the new formatter, where the space is inserted after the "x -> {}":
-------
package p;
import java.util.function.IntConsumer;
class TestInlineLambda1 {
	{
		IntConsumer op = (x -> {
		});
	}
}
-------

The space is inserted in SpacePreparator#visit(Block). A fix is to make that method end like this, i.e. extends the special case to TokenNameRPAREN as well:

if (this.options.insert_space_after_closing_brace_in_block) {
	int closeBraceIndex = this.tm.lastIndexIn(node, TokenNameRBRACE);
	if (closeBraceIndex + 1 < this.tm.size()) {
		int nextToken = this.tm.get(closeBraceIndex + 1).tokenType;
		if (nextToken != TokenNameSEMICOLON && nextToken != TokenNameRPAREN) {
			this.tm.get(closeBraceIndex).spaceAfter();
		}
	}
}
return true;

Comment 3 Markus Keller 2015-02-11 15:52:20 EST

For this source, the line break after the last ',' is removed, even if the option "Never join already wrapped lines" is checked:

	String[] strings = {
			"Hello World",
			"How are you",
			"anything and yet nothing in particular",
	};

The line break is also removed if there's no trailing comma.

For this source, all line breaks are removed (despite the never-join setting):

	private static final String CU_POSTFIX= " {\n" +
			"	\n" +
			"}\n" +
			"}\n";

Comment 4 Mateusz Matela 2015-02-14 18:10:19 EST

(In reply to Markus Keller from comment #3)
> For this source, the line break after the last ',' is removed, even if the
> option "Never join already wrapped lines" is checked:
> 
> 	String[] strings = {
> 			"Hello World",
> 			"How are you",
> 			"anything and yet nothing in particular",
> 	};
The thing is the old formatter never wrapped on the closing brace in an array initializer - it either wrapped on the last element or not at all. In the new formatter the "Never join already wrapped lines" option only works in places where formatter itself is allowed to wrap (more about this in bug 372801), so a line break before a closing brace is removed.
I think it's a good idea to allow wrapping on the closing brace in this case, but with lower priority to avoid effects like this:
 	String[] strings = { "Hello World", "How are you",
 	};
I'll implement it this way.

> The line break is also removed if there's no trailing comma.
> 
> For this source, all line breaks are removed (despite the never-join
> setting):
> 
> 	private static final String CU_POSTFIX= " {\n" +
> 			"	\n" +
> 			"}\n" +
> 			"}\n";
The formatter settings probably say wrapping should take place before a binary operator, so line breaks after an operator are not considered a valid wrap to preserve (as mentioned above). I this behavior acceptable?

Comment 5 Jay Arthanareeswaran 2015-02-15 23:29:04 EST

See bug 303519 comment #145 as well.

Comment 6 Markus Keller 2015-02-16 06:45:37 EST

The "Never join already wrapped lines" should continue to do exactly what it says. When enabled, the formatter should just never remove a line delimiter, independent of any other settings.

Comment 7 Mateusz Matela 2015-02-16 08:42:36 EST

(In reply to Markus Keller from comment #6)
> The "Never join already wrapped lines" should continue to do exactly what it
> says. When enabled, the formatter should just never remove a line delimiter,
> independent of any other settings.
Well, it still does what it says, it's just that the definition of "already wrapped lines" has changed a bit. Delimiters that break formatting are not line wrapping. Even the old formatter did remove some line delimiters, though inconsistently.
We can add a few exceptions and leave some additional line breaks if really necessary, but IMO leaving all the delimiters in would not make much sense. I present my arguments in bug 372801, we can continue the discussion there.

Comment 8 Markus Keller 2015-02-16 11:17:22 EST

(In reply to Mateusz Matela from comment #4)
> > For this source, all line breaks are removed (despite the never-join
> > setting):
> > 
> > 	private static final String CU_POSTFIX= " {\n" +
> > 			"	\n" +
> > 			"}\n" +
> > 			"}\n";
> The formatter settings probably say wrapping should take place before a
> binary operator, so line breaks after an operator are not considered a valid
> wrap to preserve (as mentioned above). I this behavior acceptable?

When I uncheck "Wrap before operator" (and have "Never join..." checked) then the line breaks indeed stay as expected.

Nevertheless, the "Never join..." option used to keep such line breaks regardless of the "Wrap before operator" setting and the position of the operator in pre-wrapped code. This behavior needs to be preserved in the new formatter. String concatenation is the prime example where the "Never join..." option is used all the time, and changing this now would cause too many surprises.

Comment 9 Noopur Gupta 2015-02-18 07:12:48 EST

Created attachment 250909 [details]
Example project

Found 2 differences in the formatter wrt line wrapping, which may be the same as discussed in above comments.

In the attached example, press Ctrl+Shift+F on the file.
The two formatting changes were not happening with the earlier formatter.

Comment 10 Mateusz Matela 2015-02-19 08:25:11 EST

(In reply to Noopur Gupta from comment #9)
> Found 2 differences in the formatter wrt line wrapping, which may be the
> same as discussed in above comments.
The first change (array initializer closing brace) was duscussed earlier and I had an idea in comment 4 but I still think about a better solution. The method invocation chain is joined because wrapping policy for "Function Calls -> Qualified invocations" is "Do not wrap". If you change it to "Wrap where necessary", formatting should work as expected. Is this OK? Correct me if I'm wrong, but I can't imagine it would be very important to someone to always preserve existing wrapping, but othwersie allow exceeding the line limit. Even then, maybe a good solution would be to increase maximum line width (unless this strange requirement is only for qualified invocations, which makes this scenario even stranger).

Comment 11 Mateusz Matela 2015-02-19 09:42:30 EST

(In reply to Markus Keller from comment #8)
> Nevertheless, the "Never join..." option used to keep such line breaks
> regardless of the "Wrap before operator" setting and the position of the
> operator in pre-wrapped code. This behavior needs to be preserved in the new
> formatter. String concatenation is the prime example where the "Never
> join..." option is used all the time, and changing this now would cause too
> many surprises.
What do you think about adding a setting for this? Next to the "Wrap before operator" checkbox, there would be something like "Never join if opposite" checkbox, checked by default, enabled only when "Never join already wrapped lines" is true.
I think it's important, because if we revert to the old behavior without an option, it would render the "Wrap before operator" setting pretty much meaningless when the "Never join..." setting is on. One could accidentally type some code that breaks the rule agreed upon by their team and the formatter would just ignore it.

Comment 12 Mateusz Matela 2015-02-19 15:56:11 EST

Created attachment 250953 [details]
patch - version 1

- fix for extra space after a lambda proposed in comment 2 + a test
 -> InlineTempTests18#test1() can be enabled again
- "Never join lines" preserves a line break before array initializer closing brace as per comment 4 + changed affected tests
- removed unnecessary file as per comment 5
- added FormatterOldBugsGistTests to RunFormatterTests (I missed that in the original patch)

Comment 13 Mateusz Matela 2015-02-28 11:47:42 EST

Mrkus, Manoj,
In addition to comment 11, I have a few other ideas for changes in formatter settings. I'd like to know if you'll have the time to discuss, review and commit these in the 3 weeks before API freeze.

0. see comment 11

1. New options to allow wrapping before/after comparison operators. Current formatter will do something like this:
if (aaaaaaaaa
	.bbbbbb() == ccccc
		.ddddddd() {
And I think this would be nicer:
if (aaaaaaaaa.bbbbbb()
	== ccccc.ddddddd() {

2. New option to prefer lines of similar length, to avoid code like this:
int a = some.example(1, 2, 3, 4, 5, 6,
	7);
in favor of this:
int a = some.example(1, 2,
	3, 4, 5, 6, 7);
This rule would have the lowest priority and it would try to minimize the variation for number of tokens in each line.

3. In the "Braces" settings card, add a new option for each combo box: "Keep existing position". This would be a partial remedy for people who won't like the new interpretation of the "Never join lines" rule.

4. In the "Braces" settings card, add the position configuration for array initializer closing brace. This would mostly solve bug 176820. It would probably make the "Keep empty array..." option unnecessary. It may be a good idea to add this for lambda closing brace too.

Comment 14 Noopur Gupta 2015-03-05 06:56:46 EST

(In reply to Mateusz Matela from comment #10)
> (In reply to Noopur Gupta from comment #9)
> > Found 2 differences in the formatter wrt line wrapping, which may be the
> > same as discussed in above comments.
> The first change (array initializer closing brace) was duscussed earlier and
> I had an idea in comment 4 but I still think about a better solution. The
> method invocation chain is joined because wrapping policy for "Function
> Calls -> Qualified invocations" is "Do not wrap". If you change it to "Wrap
> where necessary", formatting should work as expected. Is this OK? Correct me
> if I'm wrong, but I can't imagine it would be very important to someone to
> always preserve existing wrapping, but othwersie allow exceeding the line
> limit. Even then, maybe a good solution would be to increase maximum line
> width (unless this strange requirement is only for qualified invocations,
> which makes this scenario even stranger).

I received these examples through bug 459341. Can't say how important it is to preserve the existing wrapping here.

Comment 15 Noopur Gupta 2015-03-05 07:07:50 EST

Created attachment 251319 [details]
Example Project 2

Attached another example that shows change in formatter behavior. Formatting the code in example adds additional indentation to enum constants now. This results in toggle on save (similar to other cases mentioned in bug 439582). Let me know if the new formatter behavior is as expected. In that case, 'correct indentation' will have to be adapted accordingly.

Comment 16 Mateusz Matela 2015-03-10 09:45:38 EDT

(In reply to Noopur Gupta from comment #15)
> Attached another example that shows change in formatter behavior. Formatting
> the code in example adds additional indentation to enum constants now. This
> results in toggle on save (similar to other cases mentioned in bug 439582).
> Let me know if the new formatter behavior is as expected. In that case,
> 'correct indentation' will have to be adapted accordingly.
I confirm this change is intentional. The old formatter treated the "Indent on column" policy for enum constants the same as "Default indentation", which didn't make sense. The new formatter actually indents on column, the same way as other elements.

Comment 17 Manoj N Palat 2015-03-18 05:40:56 EDT

Moving to M7

Comment 18 Noopur Gupta 2015-03-19 10:04:23 EDT

Created attachment 251749 [details]
File - for Example Project 2

(In reply to Mateusz Matela from comment #16)
> (In reply to Noopur Gupta from comment #15)
> > Attached another example that shows change in formatter behavior. Formatting
> > the code in example adds additional indentation to enum constants now. This
> > results in toggle on save (similar to other cases mentioned in bug 439582).
> > Let me know if the new formatter behavior is as expected. In that case,
> > 'correct indentation' will have to be adapted accordingly.
> I confirm this change is intentional. The old formatter treated the "Indent
> on column" policy for enum constants the same as "Default indentation",
> which didn't make sense. The new formatter actually indents on column, the
> same way as other elements.

Add the attached file to example project given in comment #15 and press Ctrl+Shift+F on the file to format. In "TestEnum", only the first enum constant is indented on column and other enum constants are not.

Comment 19 Mateusz Matela 2015-03-19 16:31:25 EDT

(In reply to Noopur Gupta from comment #18)
> Add the attached file to example project given in comment #15 and press
> Ctrl+Shift+F on the file to format. In "TestEnum", only the first enum
> constant is indented on column and other enum constants are not.

This is the result of copying the old formatter's behavior for wrapped elements separated by empty lines. Usually, when wrap policy is 'wrap where necessary', having an empty line before a wrappable element results in this element not being indented, for example:
	public void test(int a,

	int b) {
	}
I just didn't notice that this behavior was different if the wrap policy is 'wrap all elements + force split'.

This is easy to change in WrapExecutor#getWrapIndent(). I'm only wondering if we should keep the old behavior for 'wrap where necessary' policy or just add the indentation in every case. Manoj, Markus, do you have an opinion on this?
There are currently 3 tests that relay on this behavior, all use the same sample code - see .howMany() method call at the end of
/org.eclipse.jdt.core.tests.model/workspace/FormatterJSR335/testLambda/A_out.java

Comment 20 Noopur Gupta 2015-03-24 07:17:35 EDT

(In reply to Mateusz Matela from comment #16)
> (In reply to Noopur Gupta from comment #15)
> > Attached another example that shows change in formatter behavior. Formatting
> > the code in example adds additional indentation to enum constants now. This
> > results in toggle on save (similar to other cases mentioned in bug 439582).
> > Let me know if the new formatter behavior is as expected. In that case,
> > 'correct indentation' will have to be adapted accordingly.
> I confirm this change is intentional. The old formatter treated the "Indent
> on column" policy for enum constants the same as "Default indentation",
> which didn't make sense. The new formatter actually indents on column, the
> same way as other elements.

Looks good. However, when I change the Indentation policy for enum constants to "Indent by one", I see 8 spaces before the constants and changing it to "Default indentation" adds only 4 spaces. In my view, this should be reversed. Please correct me if I am wrong.

Comment 21 Noopur Gupta 2015-03-24 07:27:05 EDT

Created attachment 251859 [details]
File - for built-in formatter profile - indents comments in switch-case

In the attached file, apply formatter with built-in formatter profile. The comments in switch-case are unnecessarily indented.

Comment 22 Mateusz Matela 2015-03-24 13:04:40 EDT

(In reply to Noopur Gupta from comment #20)
> 
> Looks good. However, when I change the Indentation policy for enum constants
> to "Indent by one", I see 8 spaces before the constants and changing it to
> "Default indentation" adds only 4 spaces. In my view, this should be
> reversed. Please correct me if I am wrong.
Yes, this is counterintuitive, but the old formater behaved this way. I've decided that this issue is not important enough to change it now and risk potential problems related to the need to change the workspace/project settings on switching to new Eclipse.

Comment 23 Noopur Gupta 2015-03-31 09:54:06 EDT

(In reply to Noopur Gupta from comment #21)
> Created attachment 251859 [details]
> File - for built-in formatter profile - indents comments in switch-case
> 
> In the attached file, apply formatter with built-in formatter profile. The
> comments in switch-case are unnecessarily indented.

Reported bug 463590 for this.

Comment 24 Noopur Gupta 2015-03-31 10:49:34 EDT

Based on discussion with Dani, reported bug 463596 to discuss the enum related issues reported in this bug.

Comment 25 Manoj N Palat 2015-04-21 00:18:44 EDT

Mateusz: Any further action expected in this for M7?

Comment 26 Mateusz Matela 2015-04-21 07:29:24 EDT

As I mentioned in bug 463590 comment 1 and bug 463372 comment 1, I thought you would quickly push the fix for bug 462945 and then I'd upload the second version of the patch here with additional fixes for issues mentioned by Noopur.
In the meantime a few separate bugs were added with problems addressed here so I'm not sure if you prefer one patch in this bug or separate patches in the new bugs (I prefer the former).

Comment 27 Mateusz Matela 2015-04-28 10:51:00 EDT

Created attachment 252854 [details]
patch - version2

Rebased everything on current trunk, and in addition to things listed in comment 12:
- Better indentation of comments in switch statements as per comment 21 and bug 463590
- "Never join lines" preserves line breaks on the wrong side of operators + changed affected tests, as per comment 8 and bug 463372
- Elements wrapped with a "Wrap all" policy are properly indented even if separated by empty lines, as per comment 19
- "Use spaces to indent wrapped lines" does not affect enum constants, as per comment 24 and bug 463596

Comment 28 Jay Arthanareeswaran 2015-04-29 01:29:42 EDT

(In reply to Mateusz Matela from comment #27)
> Created attachment 252854 [details]
> patch - version2

I find a .txt file part of the patch, which probably shouldn't be there?

a/org.eclipse.jdt.core/eclipse.jdt.ui.patch.txt

Comment 29 Mateusz Matela 2015-04-29 03:09:48 EDT

(In reply to Jay Arthanareeswaran from comment #28)
> 
> I find a .txt file part of the patch, which probably shouldn't be there?
> 
> a/org.eclipse.jdt.core/eclipse.jdt.ui.patch.txt
The .txt shouldn't be there, so this patch removes it. As per your comment 5 :)

Comment 30 Jay Arthanareeswaran 2015-04-29 03:14:21 EDT

(In reply to Mateusz Matela from comment #29)
> The .txt shouldn't be there, so this patch removes it. As per your comment 5
> :)

I see, thanks!

Comment 31 Manoj N Palat 2015-04-29 03:31:43 EDT

(In reply to Mateusz Matela from comment #27)
> Created attachment 252854 [details]
> patch - version2
> 

> - "Use spaces to indent wrapped lines" does not affect enum constants, as
> per comment 24 and bug 463596

In this bug 463596, in the "E2.java" of project "Modified Profile 1" in the attachment, after the application of this patch the 2nd and 3rd enums are formatted differently. This can be taken up when bug 463596 is addressed and this patch can be committed. +1.

Comment 32 Eclipse Genie 2015-04-29 04:12:56 EDT

New Gerrit change created: https://git.eclipse.org/r/46737

Comment 33 Eclipse Genie 2015-04-29 10:08:21 EDT

Gerrit change https://git.eclipse.org/r/46737 was merged to [master].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=871cac4ba4cecfc6322ce0af5777177652a514a9

Comment 34 Mateusz Matela 2015-04-29 18:06:32 EDT

(In reply to Manoj Palat Away Until May 11 2015 from comment #31)
> In this bug 463596, in the "E2.java" of project "Modified Profile 1" in the
> attachment, after the application of this patch the 2nd and 3rd enums are
> formatted differently. This can be taken up when bug 463596 is addressed and
> this patch can be committed. +1.

It turns out this problem is not speciffic to enum constants, so I'll keep the discussion in this bug.

This is actually a follow-up to comment 19:
(In reply to Mateusz Matela from comment #19)
> (In reply to Noopur Gupta from comment #18)
> > Add the attached file to example project given in comment #15 and press
> > Ctrl+Shift+F on the file to format. In "TestEnum", only the first enum
> > constant is indented on column and other enum constants are not.
> 
> This is the result of copying the old formatter's behavior for wrapped
> elements separated by empty lines. Usually, when wrap policy is 'wrap where
> necessary', having an empty line before a wrappable element results in this
> element not being indented, for example:
> 	public void test(int a,
> 
> 	int b) {
> 	}
> I just didn't notice that this behavior was different if the wrap policy is
> 'wrap all elements + force split'.
> 
> This is easy to change in WrapExecutor#getWrapIndent(). I'm only wondering
> if we should keep the old behavior for 'wrap where necessary' policy or just
> add the indentation in every case. Manoj, Markus, do you have an opinion on
> this?
> There are currently 3 tests that relay on this behavior, all use the same
> sample code - see .howMany() method call at the end of
> /org.eclipse.jdt.core.tests.model/workspace/FormatterJSR335/testLambda/A_out.
> java
There was a decision to be made and I went with an option that seemed to be safer: keep the old formatter's behavior for elements wrapped with the "wrap where necessary" policy and fix indentation only for "wrap all elements.." policies. But this doesn't work for the first element with the "wrap all elements except first..." policy. I don't see a simple way to change the behavior for this particular case, so now I'm leaning towards indenting all elements regardless of wrapping policy. This "feature" of skipping indentation after an empty line didn't seem to make much sense anyway...

If you don't object, I'll upload a second patch here with this change.

Comment 35 Jay Arthanareeswaran 2015-04-29 23:39:36 EDT

The patch has already been released. We have two options: Either to keep this open and address during RC1 or close this and take it up on another bug for RC1.

Comment 36 Jay Arthanareeswaran 2015-04-30 04:48:36 EDT

.

Comment 37 Manoj N Palat 2015-05-26 01:59:41 EDT

*** Bug 463590 has been marked as a duplicate of this bug. ***

Comment 38 Jay Arthanareeswaran 2015-05-26 02:16:29 EDT

Verified for 4.5 RC3 with build I20150524-2000.