Community
Participate
Working Groups
There's a lot of functionality in the clean up code, except for Sort Members. There was a few mentions of it in other (closed) bugs, so I decided to write an implementation that would do the work. There's some changes to UI and to core; the SortMembers operation actually made the changes to the compilation unit, so I had to refactor in a bit of code which returned a TextEdit instead. That then allowed the clean up/fix to work. The changes were made against 3.3M4, but they are just the files rather than patches since I can't get a checkout. If it's not possible to apply these files I can try to come up with something ... Here are the changed files: ./org.eclipse.jdt.core/src/org/eclipse/jdt/core/util/CompilationUnitSorter.java ./org.eclipse.jdt.core/src/org/eclipse/jdt/internal/core/SortElementsOperation.java ./org.eclipse.jdt.ui/src/org/eclipse/jdt/internal/corext/codemanipulation/SortMembersOperation.java ./org.eclipse.jdt.ui/src/org/eclipse/jdt/internal/ui/fix/MultiFixMessages.java ./org.eclipse.jdt.ui/src/org/eclipse/jdt/internal/ui/fix/SortMembersCleanUp.java ./org.eclipse.jdt.ui/src/org/eclipse/jdt/internal/ui/fix/SortMembersFix.java ./org.eclipse.jdt.ui/src/org/eclipse/jdt/internal/ui/fix/MultiFixMessages.properties
Created attachment 56111 [details] Tar containing Java source files These files were created from 3.3M4
Created attachment 56112 [details] Properties file with messages in Oops ... forgot to include the new Messages properties file
Created attachment 56314 [details] Patch based on 3.3M4 code Provides a patch based on code dated 20061214
Created attachment 56315 [details] Patch created based on HEAD (20070103) This has been weaved in based on the (current) HEAD as of 20060103. This should be relatively trivial to apply to the existing code.
Thanks a lot Alex. Here my 2 cents: 1. SortElementsOperation is internal in core, I can't use it directly in SortMembersFix 2. SortElementsOperation does create an AST, this is not required when running clean up. Because either this AST has been created in batch (if running the action) or the AST is taken from the editor (on save, Bug 162540). Creating the AST again is too expensive and a no go. What we would need is something like: public static TextEdit sort(CompilationUnit ast, int options, Comparator comparator, IProgressMonitor monitor); on org.eclipse.jdt.core.util.CompilationUnitSorter Do you have time and are you willing to look at this?
Re comment #5 point 1 -- yes, I saw it was internal. I thought it might be best to get this out first, and then (if necessary) provide some kind of static factory method in the non-UI code to forward it on. And I'd seen the CompilationUnitSorter but forgotten where it was until you mentioned it :-) Is the difference between an ICompiationUnit and a CompilationUnit relevant? I'm not sure I understand your point about the AST being created twice; but I don't really understand how it works anyway ;-) This code is in SortElementsOperation: ASTParser parser = ASTParser.newParser(this.apiLevel); parser.setCompilerOptions(options.getMap()); parser.setSource(source); parser.setKind(ASTParser.K_COMPILATION_UNIT); parser.setResolveBindings(false); org.eclipse.jdt.core.dom.CompilationUnit domUnit = (org.eclipse.jdt.core.dom.CompilationUnit) parser.createAST(null); Is that what you are referring to? I can put in a delegate method via the CompilationUnitSorter (addressing point 1) but I don't know how to tackle point 2. If you can help me through it, I can try to move further. Alex.
Created attachment 56440 [details] Patch (against HEAD) providing static member on CompilationSortThingy This is ugly (in terms of not commenting the code, API could use a bit of tweaking etc.) but is this what you had in mind for the comment #5 using an AST instead of an ICompilationUnit?
Created attachment 56470 [details] core fix SortMembersOperation is a pretty ugly thing... Why do they use a elements operation which can have 0 to n elements but only process one element? Anyway. I want to get rid of the applyEdit flag. What to you think about my patch? > Is the difference between an ICompiationUnit and a CompilationUnit relevant? ICU is just a handle, very light weight. CompilationUnit is the fully parsed version of a ICU (with or without all bindings resolved). Parsing and binding resolving is expensive, time and memory wise. There is one big issue left (which I can see at the moment): In SortMembersFix the call to getPreviewDocument and the comparison of the contents, this is too expensive. Cleanest solution here would be that SortMembersOperation returns null if there is nothing to sort. Otherwise we have to find another solution, in the worst case we have to create a fix all the time. Last but not least we must add tests to CleanUpTest and CleanUpStressTest
Yeah, SortMembersOp isn't exactly the cleanest implementation, is it? I agree that the apply edits aren't necessary for the clean up, but I wanted to try and reuse the implementation. Note that the sort members operation seems to want to track positions when called by the Sort itself, so I'm not sure how you've maintained the previous behaviour without this? You only know about the positions being reordered after the sort occurs, and I couldn't find a clean way of tracking/maintaining that. Well, not a quick'n'dirty, anyway. As for the ICompilationUnit/CompilationUnit, it turns out there's many CompilationUnits in different packages :-) I was thinking of the one that implemented the ICompilationUnit interface, whereas you meant a completely different ast.CompilationUnit ... (Incidentally, if the IFix/ICleanup gets promoted, it might not be a bad idea to start calling the JDT ones something specific e.g. IJDTFix and IJDTCleanup, so that a generic name of IFix and ICleanUp can be used elsewhere. Of course, they'd be in the JDT package, but when opening types with e.g. Ctrl+Shift+O, if you have too many IFixes, then you might not know which IFix you're referring to ...) Agreed that it would be good if SortMembersOp returned a null change when there's nothing to do, but I wanted to (a) get something that worked, and (b) not damage the SortMembersOp too much in the processes for the existing use. I'm a little bit concerned that they're moving further apart and I wanted to ensure that we didn't have too much duplication between the two, even if it was ugly. As for the tests ... I'm getting a little out of my depth here. Are they in org.eclipse.jdt.ui.tests? I'll see if I can do something. What's the plan here, by the way? Continue working with patches and then apply everything in one go when it's finished, or add the patches so far and then work on improving it?
OK, I think I've found something. org.eclipse.jdt.ui.tests.quickfix.CleanUpTest? It looks like I need to add a method like testSortMembers or something, with: IPackageFragment sortmem = fSourceFolder.createPackageFragment("sortmem", false, null); StringBuffer buf= new StringBuffer(); buf.append("package sortmem;\n"); buf.append("public class A {\n"); buf.append(" int b;\n"); buf.append(" int a;\n"); buf.append(" void d() {}\n"); buf.append(" void c() {}\n"); buf.append(" }\n"); buf.append("}\n"); ICompilationUnit cu= sortmem.createCompilationUnit("A.java", buf.toString(), false, null); enable(CleanUpConstants.SORT_MEMBERS); assertRefactoringResultAsExpected(new ICompilationUnit[] {cu}, new String[] { "expected"}); Something like that? What about the stress tests? node.put(CleanUpConstants.SORT_MEMBERS, CleanUpConstants.TRUE); node.put(CleanUpConstants.SORT_MEMBERS_FIELDS, CleanUpConstants.TRUE); And, presumably I'll need to muck around a bit with the expected results. Let me know before I spend a bit too much time on that ...
> Note that the sort members operation seems to > want to track positions when called by the Sort itself, so I'm not sure how > you've maintained the previous behaviour without this? I have to check that again, but after a quick look it should work: Calculate the rewrite, calculat the edits, add the markers, apply the edits (this will move the range markers), then update the positions with the markers. Since we give the edits back to the caller we don't need to track this positions, the caller can do that if he needs to. > As for the ICompilationUnit/CompilationUnit, it turns out there's many > CompilationUnits in different packages :-) I was thinking of the one that > implemented the ICompilationUnit interface, whereas you meant a completely > different ast.CompilationUnit ... Yes, sorry, the internal CU is just the implementation of ICU, nothing to worry about for us. > Agreed that it would be good if SortMembersOp returned a null change when > there's nothing to do, but I wanted to (a) get something that worked, and (b) > not damage the SortMembersOp too much in the processes for the existing use. > I'm a little bit concerned that they're moving further apart and I wanted to > ensure that we didn't have too much duplication between the two, even if it was > ugly. We want the highest quality code. It not only has to work, it must be mantainable and performant. Besides that, SMO is not the nicest code I've seen in my life, I wouldn't be to afraid of changing it. > Something like that? What about the stress tests? Exactly, IMHO two testcases should be enough. For stress test: 1. Enable sort members clean up 2. Uncomment all the code at the bottom of the class and the caller to this code 3. Run it 4. If the run fails with a compile error -> Fix sort members clean up else -> A new table has been generated and is in the clipboard 5. Replace the existing table with the generated one > What's the plan here, by the way? Continue working with patches and then apply > everything in one go when it's finished, or add the patches so far and then > work on improving it? First we have to solve the problem with the preview comparison. Then we need tests. I can't commit the core parts, because I'm not a core commiter. We have to open a bug against core when we have a patch and hope that they will accept and commit our required changes in core (since this is a plan item and just a small change I'm optimistic that they will accept the change).
In the words of Professor Farnsworth, "Good news, everybody". I've spent a bit more time on this, and I've addressed the following issues: * JavaDoc'd the public API for sort * Applied Benno's patch for refactoring out the apply range edits * Written tests in the performance test and stress test for quick fixes * Updated the internal test table so that it passes * Modified the sort so that if no changes occur, it returns a MultiTextEdit with no length (not sure if this should be null instead) * Changed the preview so that if the sort API returns null or zero length, then it doesn't get used in the preview (i.e. addresses point 5 in comment 11) In fact, this also improves the SortMembersOperation -- this would previously always change the code, even if nothing needed changing. Now, changes are only written for things that need changing; in other words, there's no body to the edit if the code is already sorted. This should speed up the SortMembersAction too. (Plus, a bit of refactoring to get rid of copy'n'pasted code.) The bad news: I'm only able to access web pages on a public computer ATM, so I can't transfer my code from my laptop via a USB key to attach here. But I do have working code/tests, and it should be possible to apply the patches when I can file them; that may not be for a couple of weeks. (But the API freeze is mid-Feb, right? I should be able to upload before end of Jan.) Anyway, code is ready to roll :-)
(In reply to comment #12) > that may not be for a couple of weeks. (But the API freeze is > mid-Feb, right? I should be able to upload before end of Jan.) > > Anyway, code is ready to roll :-) Sounds great alex! Please don't forget, that core also needs time to look at the core part of the patch, M5 is February 9. Also try to keep the patch minimal, meaning don't reformat the code or change its indentation. Thanks.
I should be able to upload on/around 28th Jan. Might be able to get it before then. I'll do my best on the not reformatting, but if there's a standard .settings for formatting options that is available (or .epf) I could try formatting against that. I could even run Sort Members on the clean-up ;-) (No, I'm not going to ...)
(In reply to comment #14) > I should be able to upload on/around 28th Jan. Might be able to get it before > then. I'll do my best on the not reformatting, but if there's a standard > .settings for formatting options that is available (or .epf) I could try > formatting against that. Unfortunately JDT/UI was not yet able to agree on a common formating profile... > > I could even run Sort Members on the clean-up ;-) > > (No, I'm not going to ...) >
Created attachment 57040 [details] Patch against org.eclipse.jdt.core providing CompilationUnitSorter.sort This is the patch solely for the JDT Core, if you want to open up a separate bug with this
Created attachment 57041 [details] org.eclipse.jdt.ui (and .ui.test) patch This provides a patch against the jdt.ui and jdt.ui.tests packages. Requires the previous patch to be installed. There's a few formatting-related diff noise in there, but only a couple of lines -- there really ought to be some kind of standardised formatting for putting code in so that you can commit without this kind of noise. I spent ages tracking down trailing spaces and tabs in the making sure everything matched ...
OK, so (after a brief update and merging some changes) I've got it working again. I've done the stuff described earlier in #12, and tested it with the new things. (PS there was a new method added in ICleanUp -- something like requireAST -- to which I implemented 'return true'. I don't know what that method is for, but the tests seem to pass anyway. There's also an 'if' block in the CleanUp on the save participants which I'm not sure might be relevant to this cleanup; it's not inside the block at the moment. (CodeFormattingTabPage, line 116 ish if (!fIsSaveParticipantConfiguration) { ... Don't know if that's relevant at all. Anyway, the 'core fix' (https://bugs.eclipse.org/bugs/attachment.cgi?id=56470) is also obsoleted by the two patches I've just supplied, but for some reason I couldn't obsolete that patch when I added mine.
Opened Bug 171066 against core. I've slightly changed your patch: - Don't compare lists - Added TextEditGroup as parameter - Return null if no sorting required Last but not least - Added Alexes name to the Header of the changed files
:-)
*** Bug 171076 has been marked as a duplicate of this bug. ***
Created attachment 57408 [details] reviewed ui patch
fixed > I20070116-1510
Thanks Alex, have fun with your feature.
Thanks :-)
The patch as released to HEAD has caused bug 172028.
This one made it to New & Noteworthy for 3.3M5 :-) Thanks everyone!
Verified this in 3.3M5
Closing, since 3.3 has been released.
