Community
Participate
Working Groups
I20080516-1333 Fup of bug 231093 Given: package sample; public class C { <-start selection /* * A block comment * on two lines */ end selection-> } 1. Ctrl-Shift-F Is: nothing Should: Format block comment on one line This is a regression compared to 3.3.2 Same problem with single line comment. Also format edited lines does not work in this scenarios, which is a regression compared to 3.4M5 and a must fix, because this is a new feature.
This is not a regression introduced by the new comments formatter as it already did not work using 3.4M6, 3.4M5, 3.4M3, 3.4M2 or 3.4M1! Finally, I redo the test using 3.3.2 and it also didn't work...So, I doubt this should be a must fix for RC2... Here's the test case I used: public class C { /* * A block comment * on two lines */ } I tried both selecting the entire block comment with and without the empty lines before and after. It does not change anything, the comment is never formatted...
Benno please comment.
Created attachment 101017 [details] selection (In reply to comment #1) > This is not a regression introduced by the new comments formatter as it already > did not work using 3.4M6, 3.4M5, 3.4M3, 3.4M2 or 3.4M1! > > Finally, I redo the test using 3.3.2 and it also didn't work...So, I doubt this > should be a must fix for RC2... Sorry, I don't see that, it works in 3.3.2 and it works in 34M6. But it might be required to format the selection twice, which looks like another bug. I'm using the Eclipse built-in profile. The image shows the selection before I do Ctrl-Shift-F Also this is broken: Given: public class C { <-start public void foo() {} <-end /* * A block comment * on two lines */ } Is: Ctrl-Shift-F does also format the block comment A regression compared to M6
Created attachment 101021 [details] save edited lines tests The last two tests are red in head but green in M6.
(In reply to comment #3) > Created an attachment (id=101017) [details] > selection > > Sorry, I don't see that, it works in 3.3.2 and it works in 34M6. But it might > be required to format the selection twice, which looks like another bug. > OK, I got it. The key point is: "format the selection twice"... So, IMO, this does not work well since 3.3.2 => again, I do not see it as a must fix... > I'm using the Eclipse built-in profile. The image shows the selection before I > do Ctrl-Shift-F > > Also this is broken: > Given: > public class C { > <-start > public void > foo() {} > <-end > /* > * A block comment > * on two lines > */ > > } > Is: Ctrl-Shift-F does also format the block comment > > A regression compared to M6 > Sorry, but I disagree again. I have the same behavior in M6, M5 & M4. But not in M3 & 3.3.2... So, again, I do not see it as a must fix for RC2 but more for 3.4.1 and using a new bug as it is another issue
There are two separate issues in this bug: 1) problems with formatting edited lines on save 2) problems when formatting a user selection which also run through different code. 1) is probably caused inside the new JDT Core API to format only a set of regions (see bug 203304) and 2) is caused by JDT Text switching to the new comment formatter support which does not yet support selection based formatting (see bug 232054) and which we decided to workaround in JDT Text land for 3.4. I've filed bug 233011 for 1) and we keep this bug for 2) as we need to improve our workaround should we decide to fix this for 3.4. Benno, can you add more details (test cases) to bug 233011 and take a look at 2) (see JavaFormattingStrategy)?
Simple test showing the problem: public class Foo { /** * a * b * c * d * . */ void m1() { } /* * a * b * c * d * . */ void m2() { } } Format the selected Javadoc + m1 does not run through JDT Text workaround code and formats well. However, selecting block comment + m2, which also doesn't run through the workaround code, doesn't format at all.
And same problem for single line comment: void m3() { // this is a bug } If I select something inside the comment the workaround kicks in and it works. However, If I select from "void" to the end of the line it fails as the workaround isn't triggered.
This is a basic formatter functionality which is broken since 3.3, we definitely need to address this issue before 3.4 shipping.
Created attachment 101863 [details] Proposed patch I finally found a solution to fix this issue. Here are the key points of this solution: 1) do not include comment header/footer in the formatted text. This prevent to concatenate edits inside the comment with those just before and after. This give less chance to reject necessary edit because they do not strictly overlap the provided regions. 2) when regions are partially overlapping comments, adapt them to be sure they will include the entire comment allowing to keep all the edits to format properly the comment. Note that hopefully this patch fixes also bug 233011 as it pass the test case of comment 4. More interestingly, it also fixes bug 232054 and make the JDT/Text done in bug 231093 obsolete.
*** Bug 233011 has been marked as a duplicate of this bug. ***
*** Bug 232054 has been marked as a duplicate of this bug. ***
Philippe, Jerome, Olivier, could you please review? Thanks
I am hearing Dani being very hot about this to be fixed for 3.4.0. Dani, pls explain motivation for text breakage. Comment 7 and comment 8 do not feel compelling to me to put in a non trivial change in at this stage in the game. i.e. why not 3.4.1 ?
>I am hearing Dani being very hot about this to be fixed for 3.4.0. Yes, indeed. Please... >compelling to me to put in a non trivial change in at this stage in the game. Note that the patch has lots of new tests, method renames and new constants for better readability. This makes the patch look a bit bigger than it actually is. Also, we verified that the patch fixes our problem and also ran all our JDT Text and JDT UI tests to verify this. >i.e. why not 3.4.1 ? Because formatting a selection with block and/or single line comment(s) worked perfectly since 3.0. People who want to minimize their outgoing changes select the changed lines and format only those. With 3.4 they will have to manually format code in all situations where the block/single line comment doesn't start and end with some code. If that fails from time to time then they won't trust us anymore. Note that the same selection/scenario works just fine for Javadoc comments. In addition to the 3.0 functionality, it also reduces the usability of the new 'Format edited lines feature on save'. People that look at a new release and write about it in the press or blogs will do this based on 3.4.0 and not on 3.4.1.
+1.
The change is significant, but there is no obvious flaw in it, also interacting with Frederic makes me feel comfortable, and it has been tested strongly by JDT/Text. So +1 for 3.4RC3, as it is an important usecase for Java editor power features. On a related page, the region adapting feels wrong to me. I understand this was already being done for pure code (as opposed to comments, as well now), but essentially it feels that rather providing edits outside the requested regions, the formatter should clip the edits it produces to match expectation (still it can widen region under the cover for implementation purpose, but this should not be reveal to API user). Probably need to be addressed independantly, at a later stage (3.4.1?).
Reviewed patch with Frederic, and even if the fix is not simple, Frederic was able to answer all my questions. This makes me confident that the fix is correct and must be released for RC3. So +1 for this fix.
Released for 3.4RC3 in HEAD stream.
Dani, do not forget that since the fix for this bug has been released, you can now remove the workaround in JDT/Text land added while fixing 231093...
(In reply to comment #20) > Dani, do not forget that since the fix for this bug has been released, you can > now remove the workaround in JDT/Text land added while fixing 231093... > Ooops, I missed that you opened bug 233913 for this...
Thanks for putting this in!
Verified for 3.4RC4 using build I20080530-0100
