Community
Participate
Working Groups
I20070109-1805 Bug 168954 '[clean up] Provide Sort Members' requires that SortElementsOperation does return a TextEdit. The text edit is already generated by this operation but not exposed to clients. Instead, the edit is applied within the operation. This makes the operation unusable for clean up. Please add API which allows to retrieve the text edit describing the sort. I'll attach a patch...
Created attachment 57147 [details] proposed fix This fix was originally contributed by Alex Blewitt. The patch has been reviewed and slightly changed by me.
Olivier - can you look at this pls ?
Why the source of the given CompilatioUnit is not passed in as a parameter?
Because it's not needed? At the time the ICleanUp is called, the AST has already been provided in the form of the CompilationUnit itself. In any case, for performance reasons, it's not desirable to parse the source again (actually, that's how I first implemented it for a quick'n'dirty solution, but Benno helped there). The source is only needed for the pre-existing sort method in the SortOperation, since it actually needs to apply the edits to an existing source page. In the CleanUp, just the TextEdit is returned (so that the user can preview the change or cancel). Does that answer the question?
You actually need it at the end of the calculateEdit method in order to retrieve the edit. What I am afraid of is that the source has changed when you call: ICompilationUnit cu= (ICompilationUnit)this.elementsToProcess[0]; String content= cu.getBuffer().getContents(); Document document= new Document(content); ending up with problems when applying the astrewrite. Is it sure that the source won't be different? I would also call the verify method at the beginning of the calculateEdit method to ensure that the elementToProcess is right. So I would insert this call: IJavaModelStatus status= verify(); if (!status.isOK()) { throw new JavaModelException(status); } This leads to add this exception inside the exceptions list of the API: * <li> The given compilation unit does not provide from a ICompilationUnit * (NO_ELEMENTS_TO_PROCESS)</li> Let me know what you think and I can release the code.
You don't need the source at the end of the calculate edit method. The calculate edit method just returns a TextEdit. The SortMembersOperation, because it has an ICompilationUnit, must obtain the source and apply the edit, but the calculateEdit doesn't need to. (The older implementation mixed the two up in one method; that's what was refactored out in the applied patch). It is expressly undesirable to pass in the source for the calculateEdit check, because the AST gets manipulated by the changes, and not the source itself. There are a number of tests which exhaustively test the application of this test in the referenced bug, so the operation is clearly correct from that standpoint. There's two things here; the method to apply a sort (called from the SortMembersOperation, through the CompilationUnitSorter method that takes an ICompilationUnit) and the method for calculating a TextEdit, used by the ICleanUp/Fix stuff (called from SortMembersFix on the CompilationUnitSorter that takes an AST). The first one already existed; that's the one that needs the source etc. because that's what it did before. (The API also requires that the ICU is a working copy before this is called, too). In the case of the IFix, we explicitly do *not* want to pass in the source, because we do *not* want to parse it again. Instead, we're passing in an AST. The ICompilationUnit at this point is merely fluff in the construction of the class; it's only really passed in when calculating the TextEdit to keep the constructor happy. I don't know if there's any benefit/point in doing the status.isOK() call -- after all, it never did that before. Bear in mind that the sort method doesn't get called if there are problems with the file as part of the cleanup operation. So I don't see how we could get into a state where the model isn't valid. Perhaps (a) Benno could comment further, and (b) you could attach a diff to show what exactly you're proposing adding where? I'm not sure that I understand the code paths that you've currently described. There are also tests that utilise the SortMembersFix in the associated one; I've tested the original code against those tests (presumably, so has Benno) but if you are going to make changes to the code to do other things (like verification) then it would make sense to run them through the tests again.
(In reply to comment #6) > You don't need the source at the end of the calculate edit method. The > calculate edit method just returns a TextEdit. The end of the calculate edit method is: ICompilationUnit cu= (ICompilationUnit)this.elementsToProcess[0]; String content= cu.getBuffer().getContents(); Document document= new Document(content); return rewrite.rewriteAST(document, null); So you do need the source, don't you? If you don't create the AST from an ICompilationUnit, the call unit.getJavaElement() returns null. The verify call checks this. We don't need to call verify() as long as we handle this case. So this should be specified in the specs. Once these points are clarified I can release the code.
Alex, the problem is that Olivier adds API to core, this API works well for CleanUp, but after all, we are just one client, and Olivier has to make sure, that it works for all clients. CompilationUnit#getJavaElement can return null (not if clean up calls it) resulting in an NPE (cu.getBuffer()), our oversight... I would say: Do not pass in the source, call verify in calculateText edit and spec it because the existing API can also only handle ICompilationUnit. But I don't care either if one can pass in the source, this would make the API more flexible. Whatever you prefer...
I should say: I am a novice in the internals of the JDT, and I was attempting to explain my understanding of what it was doing :-) It didn't also help that I was thinking of the wrong method :-/ that's what happens when you have too many late nights. So, I wasn't objecting to changes, just trying to understand what it was that I was doing :-) I'm happy with you to make whatever changes are necessary to add this functionality in. If the AST can be created from other non-source packages (e.g. presumably .class files) then does the ability to sort the source even make sense? I would suggest that checking the Java element is non-null (and aborting if it is) would probably make sense. But like Benno, I'm happy with whatever changes are necessary to make it work :-)
OK, I'll add a check for the javaElement and I'll document it in the spec. I'll post the new patch here. Please review it.
Created attachment 57341 [details] new patch Let me know what you think. If ok, I'll commit it.
Released for 3.3M5. If we need to make further adjustments, we'll make them against code checked in HEAD.
(In reply to comment #11) > Created an attachment (id=57341) [details] > new patch > > Let me know what you think. > If ok, I'll commit it. > This should do it, thanks a lot!
Cool :-) Thanks everyone!
Verified for 3.3 M5 using build I20070205-0009
