Community
Participate
Working Groups
After talking to Martin A at EclipseCon, I suggested that it would be good if Sort Members could be made into an action that could be applied to projects as well as packages, like format and organise imports do. This would then enable such an action to be hooked into the Code Cleanup wizard. I'd love to help out fixing this bug. I've dug around into the code before since I had to apply my own operation to recursively process Java files, and I think that this functionality could be extracted into a superclass that could be made publically available for any others wanting to apply a recursive behaviour to Java projects.
Moving to JDT UI
Alex, of course help would be fantastic. For 3.2 we can't add new API, so a common superclass can probably only be added for 3.3. But maybe you can extract the duplicated code to some utility (JavaModelUtil?) Benno, can you help/review Alex's code?
Doesn't the API freeze only apply to public/exported packages? Couldn't we still have an internal superclass that performed the action, with a view to promoting to public in 3.3 if it were useful? That way, people who are content to use internal packages could still use something in the 3.2 timeframe.
Sure, I will help. I'm not sure yet what you want to achieve. Do you want to have an Action which takes a selection and then applies an operation on each compilation unit in the selection (the operation then would be implemented by subclasses)? I think that would be a good thing to do, but it would also be possible to just copy paste the code from, say, CleanUpAction. Or do you want to add sort members to the clean up wizard? To do that you need to implement ICleanUp and add an instance of your implementation to the CleanUpWizard.
So, ideally a bit of both. Yes, copy-and-paste would work for a multiple selection; but that's been done before (and will probably be done again). It would be good to have an abstract action that can do this in one place, and refactor it from existing places (e.g. CleanUpAction). Hopefully it could be promoted to a public API in the future (but obviously not in time for 3.2) for those that want to do the same. Even if it's an internal API, people could still take advantage of it in 3.2 by importing an internal package... but it would also allow it to be selected from the menu, which can't be done at the moment. But the second goal would be to support the ICleanUp to allow that to fit in with the cleanup wizard. I'm not sure if the cleanup wizard does the recursive descent itself, so it might be independent of the first. Let me get an up-to-date copy of the source to play around with (actually, I'll probably wait for M6 to come out since I'm not going to be doing anything until the weekend anyway) and I'll start digging in and making changes.
(In reply to comment #5) > So, ideally a bit of both. Yes, copy-and-paste would work for a multiple > selection; but that's been done before (and will probably be done again). It > would be good to have an abstract action that can do this in one place, and > refactor it from existing places (e.g. CleanUpAction). Hopefully it could be > promoted to a public API in the future (but obviously not in time for 3.2) for > those that want to do the same. Even if it's an internal API, people could > still take advantage of it in 3.2 by importing an internal package... but it > would also allow it to be selected from the menu, which can't be done at the > moment. > Sounds great, but I can't promisse, that I will be allowed to add this to 3.2 (even as non API) > But the second goal would be to support the ICleanUp to allow that to fit in > with the cleanup wizard. I'm not sure if the cleanup wizard does the recursive > descent itself, so it might be independent of the first. Let me get an > up-to-date copy of the source to play around with (actually, I'll probably wait > for M6 to come out since I'm not going to be doing anything until the weekend > anyway) and I'll start digging in and making changes. > As a starting point: 1. The CleanUpAction collects all the ICompilationUnits in a StructuredSelection 2. The ICompilationUnits are passed to the CleanUpRefactoringWizard 3. The user can select the ICleanUps 4. The ICompilationUnits and the ICleanUps are passed to the CleanUpRefactoring 5. For each ICompilationUnit the CleanUpRefactoring creates an AST (CompilationUnit) and passes the AST to each ICleanUp 6. The ICleanUp does rewrite the AST and returns a TextEdit 7. If the TextEdit does conflict with a previous change then the TextEdit is thrown away and the ICompilationUnit is added at the end of an working queue Mmm, looks like the code we need is in SortElementsOperation#processElement, and this is jdt/core code...
*** Bug 171074 has been marked as a duplicate of this bug. ***
Would it be good to reassign this to JDT/Core instead of JDT/UI? I can then take a stab at fixing this. Of course, there would also need to be some JDT/UI changes (mostly in terms of enabling it in the plugin.xml) but having been in this area I think I now have a better clue of what needs to be done for the Sort Members Operation.
With 168954 fixed this should be easy to fix. No need to move to core, this is all UI... Provide a patch if you want to, see CleanUpAction for a start. There is already the possibility to run clean up without the wizard. Try to generalize CleanUpAction such that it can run any clean up without the wizard. We could use this also for format, which can be executed on multiple sources but is not undoable. Lets try to fix that too...
At the moment, the SortMembersAction calls the older sort() in CompilationUnitSorter that takes an ICompilationUnit. However, the CompilationUnitSorter only ever does thing with the first argument. We could instead do the calculation by using the newer CompilationUnitSorter that takes the AST; but it means that (for example) it wouldn't be useful for non-UI projects (like the FormatSource can be run headless, for example). I was proposing to modify the implementation in the SortMembersOperation's executeOperation() so that it iterated over more than one without reverting to calling the ICleanUp, which is (currently) UI. Of course, if the future is for migrating everything towards ICleanUp, I can do it in UI code. Let me take more of a look now that 168954 is in. Alex.
Created attachment 57663 [details] Patch against JDT Core
Created attachment 57664 [details] Patch against JDT UI
So, these two patches provide different bits of functionality: 1) Sort Members can now be done on the project, package or class level. It uses the same mechanism as before (as opposed to the new CleanUp) which opens files and performs the sort on them. 2) The dialogs are a little bit cleaner. Instead of double negatives in the UI, there's now a selection to 'sort fields' or 'sort enumeration constants'. I've also changed the CleanUp to use the same terms; hopefully, that is better than 'everything' and 'everything except fields, initializers and enumeration constants'. 3) The warning (don't reorder stuff) now correctly appears/disappears when the correct combinations of checkboxes are selected. Before, it was being enabled/disabled, when it should have been visible/invisible. 4) I've modified the sort API to take an array of an array of positions instead of just an array of positions. The API previously would have only worked with 1 element. It's not quite finished (I've left some debugging-type code and TODO-style notes in) but I've tested it out on some simple projects and verified that it works. I've also amended some of the messages and put #deprecated above a couple that are no longer used -- I'm not sure what is the right thing to do about deprecated messages, or whether they can just be removed. As always, comments welcome ...
See also: Bug 133277 - Allow Sort Members to be performed on package and project levels Bug 37496 - [clean up] Allow combining Format with organize import and sort members [code manipulation]
(In reply to comment #14) > See also: > Bug 133277 - Allow Sort Members to be performed on package and project levels > Bug 37496 - [clean up] Allow combining Format with organize import and sort > members [code manipulation] > Sorry, Bug 154109 - [clean up] [plan] Extend Clean Up instead of 133277
Re comment #14, this isn't a clean-up thing. In any case, the sort-members-on-save is implemented in 3.3M5. These patches add functionality for being able to perform sort on a package- or project-level, however, which is what the bug is about :-)
(In reply to comment #13) > So, these two patches provide different bits of functionality: > > 1) Sort Members can now be done on the project, package or class level. It uses > the same mechanism as before (as opposed to the new CleanUp) which opens files > and performs the sort on them. Sorry Alex, this wont work, please use CleanUp. If you look at CleanUpRefactoring you'll understand that clean up is a bit more then just a simple loop, i.e. it creates the required asts in batch which does dramatically increase the performance. You'll also get Undo and (if desired) preview for free. Just iterating over the ICUs and create an ASTs for each won't scale. You'll also get other things for free if using clean up, like validating the changes and checking out required files from source control systems. Also you're opening an Editor for each cu to sort?! If you sort like a million cus (which must be possible) then you open a million editors? Even if you just sort like 100 cus it will kill your memory. And what happens if the user has 'Close editors automatically' enabled? I'm surprised that sort members does open an editor at all, this is not good. Indeed the annotations are removed, we must solve the problem by moving the annotations as well, IMHO the ASTRewrite should do that or the refactoring framework, at least clean up should do it. At the moment sort members clean up does remove the annotations as well, not good. We can certainly not warn the user if he has an annotation somewhere in his project, even if we do not move the annotation, because there will almost always be an annotation. > 2) The dialogs are a little bit cleaner. Instead of double negatives in the UI, > there's now a selection to 'sort fields' or 'sort enumeration constants'. I've > also changed the CleanUp to use the same terms; hopefully, that is better than > 'everything' and 'everything except fields, initializers and enumeration > constants'. I'll need a separate bug (agains jdt/ui) for that, with a separate patch. > 3) The warning (don't reorder stuff) now correctly appears/disappears when the > correct combinations of checkboxes are selected. Before, it was being > enabled/disabled, when it should have been visible/invisible. Why is this better? Don't you have to relayout the dialog then? > 4) I've modified the sort API to take an array of an array of positions instead > of just an array of positions. The API previously would have only worked with 1 > element. Do we need that? > > It's not quite finished (I've left some debugging-type code and TODO-style > notes in) but I've tested it out on some simple projects and verified that it > works. I've also amended some of the messages and put #deprecated above a > couple that are no longer used -- I'm not sure what is the right thing to do > about deprecated messages, or whether they can just be removed. > Unused messages must (should) be removed. I'll do that before we ship 3.3. > As always, comments welcome ... >
I wanted to provide something that was as close to the existing behaviour as possible. Despite the API in CompilationUnitSorter.sort() having an array of IJavaElement, it only ever processed one. So it used the same behaviour as then. As for the position sorting; I have no idea what the current use case for that is, but again, it only worked on a single element so I generalised it to have the effect of supporting multiple. FYI the majority of the JDT UI patch is providing updates to the UI, though there are some things e.g. the changes of the 'run' method and the invocation to the CompilationUnitSorter.sort() that would need to be undone. As for the setvisible/invisible on the warning; it should only be shown if one or more of the fields is selected for the clean up. Before, it was being shown all the time; the code said 'setEnabled' when it probably meant 'setVisible'. I agree in principle that the opening of lots of editors is tedious, but that's the way that it works at the moment. I agree that a CleanUp-based approach would be better in the long run, but even though it's not optimal at present doesn't mean it's not useful. Currently, if there were a hundered files, you'd have to go through each one individually and select the menu option for each one; it would then open up an editor individually. So you're not changing the amount of work that gets done, but you are causing the user more distress. At least what this does is to allow a number of files to be processed with a single menu item, even if it still does the same amount of work under the hood. I'd like the JDT Core patch reviewed for comments by the JDT Core team too for their input, to see if there's a desire to improve the current sort API (and to get comments about whether the positions is actually required). Can you add Oliver as a CC to this bug for his views? Lastly, note that OrganiseImportsAction/Operation does pretty much what I'm doing here; that doesn't use the AST either but passes in a bunch of ICUs. So whilst moving all of these forward to having a single clean-up based solution is likely the right thing to do eventually, for now there is a somewhat kludgy solution that at least does something similar to the way that other operations work on multiple classes prior to the cleanup code.
(In reply to comment #18) > As for the setvisible/invisible on the warning; it should only be shown if one > or more of the fields is selected for the clean up. Before, it was being shown > all the time; the code said 'setEnabled' when it probably meant 'setVisible'. I'm not changing UI unless there is a good reason to do so: This behavior is tested and our users are used to it and I have never heard any complains about it. But you can always open a separate bug report and tell us why you want to change that. > I agree that a CleanUp-based approach > would be better in the long run, but even though it's not optimal at present > doesn't mean it's not useful. This is just not our approach to writing software. If we add something to jdt/ui it is kind of a promise that we (meaning I in this case) will keep it and maintain it, basically forever. So it better should be optimal.
See also Bug 31228 . We seam to have a regression here for Task and Bookmark annotations. This annotations are removed without a warning. It still works for breakpoints. Have to discuss with martin what we should/can do here.
I believe that the breakpoint/task annotations are tracked by the 'positions' array, or at least, that's my understanding of what the purpose was in the original sort() method on CompilationUnitSorter. But I don't know whether it was actually used in that way. As for the UI -- the warning message is shown regardless of what options are shown. In the currently existing code, there is a call to 'label.setEnabled(isTheBoxSelected)' or similar. So, if the box is chosen, the message is 'enabled' and if the box is not chosen, the message is 'disabled'. However, enabling/disabling a label doesn't make sense; there's no visible difference, and there's no practical difference either. All I did was change 'setEnabled' to 'setVisible' so that when it's not selected, the message isn't visible and when it is selected, the message is visible. I think that was just an original UI mistake, and that it should be setVisible all along, but of course you have the CVS commits and can check up on that :-) To be honest, I often wondered what the point of a warning shown all the time is when it doesn't apply; after all, most of the other Eclipse UI elements show warnings as they become applicable and then hide them when they're not, so it's just following the other UIs. I've been working on rolling back the changes requested to only allow a single sort (so it's not relevant to this bug any more, unfortunately) and will submit the changes for the sort enum/fields as a separate bug, as requested. NB I believe that the Sort Members always loses task/bookmark annotations. You can verify this without a cleanup; and in fact, that's one of the messages in the warning AFAIK
I've split up the patch to provide sort fields/enums separately, and filed it as Bug 172801. Re #20; can we verify this behaviour has changed since 3.2? I was under the impression that tasks/bookmarks were always lost with the sort members. I'm not sure that it's regression.
I'd like to move my attention back to this one. I've dug around a bit into how it works. The plan would be to move the SortMembersAction to call CleanUpAction (presumably, with a showWizard of false) and then pass that off to the startCleanupRefactoring() method. That uses createCleanUps() in CleanUpRefactoring, which instantiates all clean ups (as opposed to just running one of them). Obviously, I can do this with the wizard but the trick will be to make sure that all cleanups are disabled. The 'Default profile' or 'Custom Profile' could be used in the code; but how do I know that the cleanups are disabled by default? You wouldn't want to run (say) SortMembersOperation if it would then inadvertently introduce generics. The other approach would be to modfy CleanUpAction to take an array of cleanups to run (defaulting to the full set?) Also, how do you specifically code/set the properties e.g. like CleanUpConstants.getDefaultSettings() but on a per-run basis (as opposed to a per-project or workspace level setting). So, it might look like: CleanUpAction(ICleanup[] cleanups, Map properties) and then push those down into RefactoringExecutionStarter#startCleanupRefactoring. I'm not sure where to put the properties (CleanUpPreferenceUtil seems to have a loadOptions that merges preference/default settings) How does the custom profile work in cleanup at the moment? Ideally, I'd want to try and replicate that. Any help you could offer would be appreciated...
(In reply to comment #23) > I'd like to move my attention back to this one. I've dug around a bit into how > it works. The plan would be to move the SortMembersAction to call CleanUpAction > (presumably, with a showWizard of false) and then pass that off to the > startCleanupRefactoring() method. > > That uses createCleanUps() in CleanUpRefactoring, which instantiates all clean > ups (as opposed to just running one of them). Obviously, I can do this with the > wizard but the trick will be to make sure that all cleanups are disabled. > > The 'Default profile' or 'Custom Profile' could be used in the code; but how do > I know that the cleanups are disabled by default? You wouldn't want to run > (say) SortMembersOperation if it would then inadvertently introduce generics. > > The other approach would be to modfy CleanUpAction to take an array of cleanups > to run (defaulting to the full set?) Also, how do you specifically code/set the > properties e.g. like CleanUpConstants.getDefaultSettings() but on a per-run > basis (as opposed to a per-project or workspace level setting). > > So, it might look like: > > CleanUpAction(ICleanup[] cleanups, Map properties) > > and then push those down into > RefactoringExecutionStarter#startCleanupRefactoring. I'm not sure where to put > the properties (CleanUpPreferenceUtil seems to have a loadOptions that merges > preference/default settings) > > How does the custom profile work in cleanup at the moment? Ideally, I'd want to > try and replicate that. > > Any help you could offer would be appreciated... > You don't need to worry about profiles. Instead you can write something like: Hashtable settings= new Hashtable(); settings.put(CleanUpConstants.SORT_MEMBERS, CleanUpConstants.TRUE); SortMembersCleanUp cleanUp= new SortMembersCleanUp(settings); CleanUpRefactoring refactoring= new CleanUpRefactoring(); refactoring.addCleanUp(cleanUp); //a loop here for all cus refactoring.addCompilationUnit(unit); RefactoringExecutionHelper helper= new RefactoringExecutionHelper( refactoring, IStatus.ERROR, RefactoringSaveHelper.SAVE_JAVA_ONLY_UPDATES, shell, PlatformUI.getWorkbench().getActiveWorkbenchWindow()); try { helper.perform(true, true); } catch (InterruptedException e) { } I have not tested that but it should/must work.
Good man; thanks for the pointers. I'll probably refactor the existing CleanUpAction to GenericCleanUpAction that will handle the resolution of the packages/projects, but otherwise follow the suggestions here. Will get back to you when I've done it ...
Created attachment 58624 [details] Provides SortMembersCleanUpAction which uses SortMembersCleanUp Man, that turned out to be easy. You guys make changing things a doddle :-) I used the idea of your code, but refactored the existing CleanUpAction to do it, since that already had the recursive-descent-into-packages and selection criteria sorted. What I did was to make CleanUpAction abstract, with an abstract getCleanUps() method; I then moved that into RefactoringExecutionStarter so that instead of RefactoringExecutionStarter running on all cleanups all the time, you can now pass in a subset of them. The existing CleanUpAction essentially then became AllCleanUpsAction, which returned all cleanups as returned by CleanUpRefactoring,getCleanUps() (in other words, what it did before). Personally, I think that it should be something like AbstractCleanUpAction and AllCleanUpsAction -- but the diff this way is a lot smaller, and (if it's accepted) it then becomes just a name change operation. The really nice thing is that if desired, you could replace the existing OrganiseImportsAction too as follows: public class OrganizeImportsCleanUpAction extends CleanUpAction { public OrganizeImportsCleanUpAction(JavaEditor editor) { super(editor); } public OrganizeImportsCleanUpAction(IWorkbenchSite site) { super(site); } protected ICleanUp[] getCleanUps() { return new ICleanUp[] { new ImportsCleanUp() }; } } which is less duplication of code :-) PS a thought; the messages in the abstract CleanUpAction may need to be changed by subclasses. Having said that, some of the messages are reasonable sensible anyway e.g. the refactoring makes no changes.
This refactoring of cleanup action means that it's possible to condense other refactoring actions, such as the OrganiseImports etc. (see comment #26). I'd be happy to supply separate bugs/patches for those if you think the approach in the above attachment is the right way around. This also makes it much faster to sort members than before, since it's using the cleanup as opposed to the existing CompilationUnitSorter.sort method.
Here is what we can do on the Annotation front: In the check postcondition method of the sort members clean up we can test for each file which does get sorted if it has a task/bookmark/breakpoint marker on it. If so we can issue a refactoring status warning and warn the user that this markers may be removed. See IResource#findMarkers(...). I've opened bug 174609 to track that. Alex? Do you want to try to fix bug 174609?
I'm willing to try and help out, but I'd like to get this fixed first. As I've noted in 174609, this does not block this at all; it's the same behaviour with or without 133277. Bug 174609 is a valid bug regardless of whether this is applied or not; however, the fix (for SortMembersAction or SortMembersCleanUp) will be different depending on whether the patch here has been applied already. In any case, even if I were to fix 174609 I would have nothing to generate a patch against for the newly patched code above since it's not been applied yet. Therefore, I can't start work on 174609 until the link between the two bugs is broken and this patch is applied. Alex.
For M7
Requires PMC approval
+1 for 3.3M7
Created attachment 62936 [details] fix
Created attachment 62938 [details] fix Without renaming CleanUpAction
fixed > I20070327-0800
*** Bug 37466 has been marked as a duplicate of this bug. ***
