Community
Participate
Working Groups
To reproduce: Load the /platform/eclipse.platform.ui git repository Import the project org.eclipse.ui.ide Edit the file CopyFilesAndFoldersOperation.java Line 1815 looks like this: copyResources); Change it to this: copyResources.clone()); Save. Observed: The post-save action runs the formatter and the formatter removes all indentation from line 1815, moving the text "copyResources.clone()" to the beginning of the line. Expected: Either no change in indentation or correct indentation.
Note that the formatter formats those same lines correctly when the entire file is formatted -- or if you select a few lines before and after the problematic line. The bug only occurs when the formatter runs as a post-save action.
(In reply to Stefan Xenos from comment #1) > Note that the formatter formats those same lines correctly when the entire > file is formatted -- or if you select a few lines before and after the > problematic line. The bug only occurs when the formatter runs as a post-save > action. The Save Actions setting for that project specifies "Format edited lines" and you edit only the line having "copyResources);". Hence it is same as selecting that line and applying the formatter. In that case, the bug can be seen by using Ctrl+Shift+F also. Simple code snippet to reproduce: package p; public class C { { Math.addExact(1, 2); // select "2);" and press Ctrl+Shift+F } }
(In reply to Noopur Gupta from comment #2) > In that case, the bug can be seen by using Ctrl+Shift+F also. Looks like this behavior was the same in Eclipse Luna, so at least this is not a regression.
This arises as bug 356851 changed the default formatter's line length to 120 characters. That line and its predecessors were likely last edited when the line length was narrower and so was wrapped, but with 120 characters the line no longer needs to be wrapped and so would now be merged with the previous line. But because of the edited-lined-only setting, the formatter cannot change the previous line, and instead reformats it with no spacing.
Some platform.ui committers report that this issue affects them. Would be nice to have this fixed during 4.6
Mateusz, let's consider this for Neon (depending on your other priorities, of course)
New Gerrit change created: https://git.eclipse.org/r/59892
This is the biggest and most risky change since the formatter redesign, so a review may be helpful. One of the tests currently fails because it depends on bug 481143, so please merge that change and rebase this one. This bug required a completely new approach to handling indentation. WrapExecutor's central method is now recursive (applyWraps()) to allow keeping track of indentation in the past and restoring it when current wrap group ends. Recursion makes it more complicated to restart wrapping from the beginning of the line - now it's done by throwing an exception. It's not used for normal wrapping though, restart is only needed when a "Wrap all elements" group is handled or on a rare occasion when additional wrap is necessary. Another change is that wrap policies are now stored for tokens that should not be wrapped, but should still be indented if a line break cannot be removed because of formatting regions or line comments. This includes tokens with "Do not wrap" policy and line breaks on the wrong side of operators, dots and commas. This may be a step towards resolving bug 474362. The last necessary change was finding and applying all the line breaks that lay outside formatting regions. Since this change is so big anyway, I did some refactoring and fixed other problems spotted while working on this: 1. My assumptions about the wrap penalty function turned out to be incorrect in some rare cases so the cached value needs to be recalculated sometimes. This doesn't seem to affect the performance in tests though. 2. The formatter never considered wrapping on field access and similar constructs. I've added this with "Do not wrap" behavior to make sure line breaks that cannot be removed will be wrapped correctly. It'll probably be useful to make it a configurable wrap policy in the future.
*** Bug 480236 has been marked as a duplicate of this bug. ***
*** Bug 477235 has been marked as a duplicate of this bug. ***
If this blocks bug 471780, let's consider this one for 4.5.2 too.
Gerrit change https://git.eclipse.org/r/59892 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=1d27cfb65a8550e3dd44f890da53c4bdcdcccdbf
I've merged to mars already so I can continue working on other things without worrying about merging changes. It can be reviewed later, before backporting to 4.5.2.
*** Bug 480074 has been marked as a duplicate of this bug. ***
(In reply to Mateusz Matela from comment #8) > Recursion makes it more complicated to restart wrapping from the beginning > of the line - now it's done by throwing an exception. It's not used for Mateusz: From the overall code it looks good. Though my expertise is limited in formatting code, I would like you to check whether there is any alternative for the "exception throwing mechanism". If you can find a reasonable alternative, please go ahead and merge with master.
(In reply to comment #12) > Gerrit change https://git.eclipse.org/r/59892 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=1d27cfb65a8550e3dd44f890da53c4bdcdcccdbf That commit causes regression in JDT/UI's CleanUpStressTest.testAllCleanUps, see e.g., http://download.eclipse.org/eclipse/downloads/drops4/N20151227-2000/testresults/html/org.eclipse.jdt.ui.tests_macosx.cocoa.x86_64_8.0.html Is this change in behavior expected / intended?
(In reply to Manoj Palat from comment #15) > I would like you to check whether there is any > alternative for the "exception throwing mechanism". If you can find a > reasonable alternative, please go ahead and merge with master. Yeah, I tried hard to avoid it, but without exceptions it's either a lot of complicated code to achieve the same effect with additional fields and a lot of 'if' statements, or redesigning large part of the wrapping algorithm, which is even more risky. (In reply to Stephan Herrmann from comment #16) > That commit causes regression in JDT/UI's CleanUpStressTest.testAllCleanUps, > see e.g., > http://download.eclipse.org/eclipse/downloads/drops4/N20151227-2000/ > testresults/html/org.eclipse.jdt.ui.tests_macosx.cocoa.x86_64_8.0.html > > Is this change in behavior expected / intended? Hmm, I thought Gerrit would run all the relevant tests and report any problems... Change not intended, good catch. Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=f3886d8ab47e0b5dc32179a8d3e8db45c6fe8498
(In reply to comment #17) > Yeah, I tried hard to avoid it, but without exceptions it's either a lot of > complicated code to achieve the same effect with additional fields and a lot of > 'if' statements, or redesigning large part of the wrapping algorithm, which is > even more risky. I noticed there's one small improvement that could be done to make this approach more palatable: control-flow exceptions such as WrapRestartException don't use or need the stack trace, but the Throwable constructor will fill it anyway by default. A control-flow exception can prevent this justifiably by overriding the fillInStackTrace method with an empty implementation, removing most of the cost involved in throwing the exception: public @Override synchronized Throwable fillInStackTrace() { return this; } With this change it actually becomes embarrassingly hard to demonstrate any real performance difference between using an exception and doing a normal return.
(In reply to Timo Kinnunen from comment #18) > With this change it actually becomes embarrassingly hard to demonstrate any > real performance difference between using an exception and doing a normal > return. Thakns, it's good to know! I see in Java 7 they added a super constructor to set it up, no need for method override. Added this improvement with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=10545bbf89db4e21cbdfaf0260ea872d78aa91e9, also changed the superclass to Throwable to make it harder to associate with exceptional situations.
Verified for Eclipse Neon 4.6 M5 with Build id: I20151229-0800
Reopening for 4.5.2 backport
Backported (all 3 changes) to 4.5.2 with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=R4_5_maintenance&id=fdef0404e52712de8ef2d2d9c1aed91ef3eef4ac
Thanks to the JDT team for helping us (and others) in Platform UI. Stefan, you report this issue for Platform UI via Bug 477478. Can you verify that this is fixed for the issues you had in Platform UI and update this bug with your test results?
Verified for 4.5.2 using M20160113-1000 build
*** Bug 220307 has been marked as a duplicate of this bug. ***
