Bug 230364

Summary: [formatter] Test failure in CleanUpStressTest after changes to comment formatter
Product: [Eclipse Project] JDT Reporter: Benno Baumgartner <benno.baumgartner>
Component: CoreAssignee: Frederic Fusier <frederic_fusier>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: eric_jodet, Olivier_Thomann
Version: 3.4Flags: Olivier_Thomann: review+
Target Milestone: 3.4 RC1   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
required changes to make the test green again
none
Proposed patch none

Description Benno Baumgartner CLA 2008-05-06 06:50:41 EDT
Created attachment 98789 [details]
required changes to make the test green again

I20080502-0100

Recent changes in the comment formatter (bug 230184) lead to a failing CleanUpStressTest. We might need to adjust the test.
Comment 1 Benno Baumgartner CLA 2008-05-06 06:51:28 EDT
Needs to be fixed for RC1
Comment 2 Benno Baumgartner CLA 2008-05-06 06:53:51 EDT
Frederic? Can you review? IMHO this looks wrong. The comment alignment was better before.
Comment 3 Frederic Fusier CLA 2008-05-06 11:59:14 EDT
The problem was when splitting line comments, the new comment on the line after started after the first one. E.g:
     // comment needs to be split
was formatted as:
     // comment needs
        // to be split

In this small example, you can see that the second line is longer than the first one, but it would have been really hard for the formatter to figure out that in case of the line max length was just after the word 'needs', the comments would need to be split in three lines:
     // comment needs
        // to be
        // split

So, I changed the rule and now the comment formatter adds one indentation less on the second line while splitting and so format as follows:
     // comment needs
    // to be split

If you definitely think this is worst, then please reassign the bug to me instead of applying the changes on CleanUpStressTests and I'll try to see I could see for another fix for this issue...
Comment 4 Frederic Fusier CLA 2008-05-06 13:01:40 EDT
This was due to a side effect of fix for bug 230184.
Comment 5 Frederic Fusier CLA 2008-05-06 13:18:15 EDT
It seems that my explanation in comment 1 was not really clear... I'll try to make it better!

Set the formatter comment line max length to 40. Set the Java editor to use a non-proportional font (e.g. Courier New), text margin to 40 and display the margin in the editor.

Now, open a java editor on the following simple test case:

public class X { // This comment will need to over the max line length
}

If the formatter preferences was changed from Eclipse built-in, then the test case will be formatted as follow:

public class X { // This comment will
					// need to over the max
					// line length
}

Note that, in bugzilla, tabs are 8 characters (and I cannot see any way to change it) so the pasted formatted output looks strange but with 4 chars per tab it would have looked like:

public class X { // This comment will
                    // need to over the max
                    // line length

}

The problem with this output is that the second line is still over 40 chars. I tried to fix this issue but it seems that my fix was not good enough.

In fact the expected formatted output should be:

public class X { // This comment will
                    // need to over the
                    // max line length
}
Comment 6 Frederic Fusier CLA 2008-05-06 13:32:39 EDT
Created attachment 98890 [details]
Proposed patch

While fixing bug 230184, the initial indentation level was wrongly modified while printing line comments.
Comment 7 Frederic Fusier CLA 2008-05-06 13:35:28 EDT
Olivier, could you please review?

We need to release the patch before today's integration build otherwise the JDT/UI CleanUpStressTests will fail.

Note that this change has no impact on Eclipse 3.0 full-src and Ganymede workspaces tests, I still get respectively 5 and 21 failures (same numbers than last released fix - see bug 230230)
Comment 8 Olivier Thomann CLA 2008-05-06 13:39:17 EDT
+1
Comment 9 Frederic Fusier CLA 2008-05-06 13:44:10 EDT
Released for 3.4RC1 in HEAD stream.
Comment 10 Eric Jodet CLA 2008-05-13 08:59:55 EDT
Verified for 3.4RC1 using build I20080510-2000.