Community
Participate
Working Groups
HEAD IJavaProject#getPackageFragmentRoots() and #getChildren() should return the roots in the order they are defined by the classpath. IJavaProject#getAllPackageFragmentRoots() already returns them in the expected order, and it's confusing that #getPackageFragmentRoots() and #getChildren() are not sorted even though a natural order exists. In practice, the elements are often sorted as expected. That's probably why we never got bugs for this. In the Package Explorer, the wrong ordering shows up when you check out org.eclipse.jdt.ui from CVS and then quickly expand the Java project node and watch source folders trickle in. We use JavaElementComparator to sort the elements, and if you set a breakpoint in #getClassPathIndex(..), you can see the unexpected ordering. I could probably fix the UI problem by using getAllPackageFragmentRoots(), but I think that would have performance impacts (#getChildren() seems to cached), and I guess we have other places where we expect the right order.
Even if you expect them to be in order, there is nothing in the documentation that says the order has to be the same as the .classpath file. The documentation says this is equivalent to getChildren() which explicitly says there is no specified order. org.eclipse.jdt.core.IJavaProject.getAllPackageFragmentRoots() is specified to return the values in the order of the .classpath file. Since changing the order should not break existing clients as it is technically speaking unspecified, if we change this, this should be reflected in the documentation of this method. To me, it looks like the UI made wrong assumptions on the returned value. Jay, please take a look and see what can be done. Thanks.
> To me, it looks like the UI made wrong assumptions on the returned value. Correct. But rather than working around the unspecified order on the client side, the more sustainable solution is to adjust the implementation and specify the order.
The IJavaProject#getPackageFragmentRoots() directly uses #getChildren. And the children are added at various points such as resource change event etc., which means, the order can't be guaranteed with the existing approach. And this is supported by the API documentation. In order to meet the requirement stated in the bug, we either have to compute the package fragments every time from the classpath (just like the getAllPackageFragmentRoots) or pick up the existing roots and sort them by comparing with the classpath order. Now I see there are are quite a few eclipse components already using this API and if they want the roots in any particular order, they already have some code in place. My point is, this is not going to be of any use for most existing clients but if there is any performance impact, even if small, they will have to bear it too.
> Now I see there are are quite a few eclipse > components already using this API and if they want the roots in any particular > order, they already have some code in place. My point is, this is not going to > be of any use for most existing clients but if there is any performance impact, > even if small, they will have to bear it too. My point is that most clients do not explicitly deal with the order because they actually already now expect the package fragment roots to be ordered. That's what they currently see, as long as the project is not modified. Even the Package Explorer (a very visible client) has lived under this assumption forever. We even have JavaElementComparator#getClassPathIndex(IPackageFragmentRoot) that explicitly uses getPackageFragmentRoots() to get the order. I think it makes more sense to do the ordering once at the point where the roots are modified and cached. If clients have to use getAllPackageFragmentRoots(), then that's going to be a lot more expensive. And we definitely don't want to force all clients to add another level of caching just to get acceptable performance.
Created attachment 189914 [details] Proposed patch Patch contains the API change + fix required to support the API change. I am still trying to come up with a scenario where the order of the package fragment roots is not same as classpath order. Markus, any hints for this?
(In reply to comment #5) > I am still trying to come up with a scenario where the order of the package > fragment roots is not same as classpath order. > > Markus, any hints for this? Yes, see comment 0: > In the Package Explorer, the wrong ordering shows up when you check out > org.eclipse.jdt.ui from CVS and then quickly expand the Java project node and > watch source folders trickle in. We use JavaElementComparator to sort the > elements, and if you set a breakpoint in #getClassPathIndex(..), you can see > the unexpected ordering.
(In reply to comment #5) > Created attachment 189914 [details] [diff] > Proposed patch Sorry, I don't think that helps much. With this, the order of IJavaProject#getChildren() is still not as expected. In all other IJavaElement#getChildren APIs where the order of elements depends on the order in a source or class file (ITypeRoot, IMember), we get the elements ordered and properly cached. The package fragment roots depend on the source order in .classpath, and it would be consistent to sort them just as well. When I debug this by setting a breakpoint in OpenableElementInfo#addChild(IJavaElement) with condition "child instanceof org.eclipse.jdt.core.IPackageFragmentRoot", then I see how roots can be added at the end of the list of children here: Thread [Worker-10] (Suspended (breakpoint at line 32 in OpenableElementInfo)) JavaProjectElementInfo(OpenableElementInfo).addChild(IJavaElement) line: 32 DeltaProcessor.addToParentInfo(Openable) line: 285 DeltaProcessor.elementAdded(Openable, IResourceDelta, DeltaProcessor$RootInfo) line: 1088 DeltaProcessor.updateCurrentDeltaAndIndex(IResourceDelta, int, DeltaProcessor$RootInfo) line: 2415 DeltaProcessor.traverseDelta(IResourceDelta, int, DeltaProcessor$RootInfo, DeltaProcessor$OutputsInfo) line: 2145 DeltaProcessor.traverseDelta(IResourceDelta, int, DeltaProcessor$RootInfo, DeltaProcessor$OutputsInfo) line: 2195 DeltaProcessor.processResourceDelta(IResourceDelta) line: 1826 DeltaProcessor.resourceChanged(IResourceChangeEvent) line: 1999 DeltaProcessingState.resourceChanged(IResourceChangeEvent) line: 470 NotificationManager$1.run() line: 291 SafeRunner.run(ISafeRunnable) line: 42 NotificationManager.notify(ResourceChangeListenerList$ListenerEntry[], IResourceChangeEvent, boolean) line: 285 NotificationManager.broadcastChanges(ElementTree, ResourceChangeEvent, boolean) line: 149 Workspace.broadcastPostChange() line: 395 Workspace.endOperation(ISchedulingRule, boolean, IProgressMonitor) line: 1496 File.create(InputStream, int, IProgressMonitor) line: 183 File.create(InputStream, boolean, IProgressMonitor) line: 196 EclipseFile.setContents(InputStream, int, boolean, IProgressMonitor) line: 184 Session.receiveFile(ICVSStorage, boolean, int, IProgressMonitor) line: 801 CheckoutWithOverwrite$CreatedResponseHandler(UpdatedHandler).receiveTargetFile(Session, ICVSFile, String, Date, boolean, boolean, boolean, IProgressMonitor) line: 119 ... When you step over DeltaProcessor#addToParentInfo(..) and then go to the runtime workbench and refresh the Java project, you see that the source folder was added after the JRE System Library node. I think the fix should go somewhere near JavaProjectElementInfo#addChild(..) or its caller DeltaProcessor#addToParentInfo(Openable).
(In reply to comment #7) > (In reply to comment #5) > > Created attachment 189914 [details] [details] [diff] > > Proposed patch > > Sorry, I don't think that helps much. With this, the order of > IJavaProject#getChildren() is still not as expected. From the problem you described, I assumed it should be enough to fix the IJavaProject#getPackageFragmentRoots() and leave the #getChildren() untouched. Anyway, I will post an updated patch soon.
Created attachment 190241 [details] Updated ptch This patch works for the scenario mentioned in comment #0 and includes a regression test in ClasspathTests Markus, I have updated the doc only for the IJavaProject#getPackageFragmentRoots() and not for #getChildren as the fix covers only the IPackageFragmentRoot and not other IJavaElement types. Do you think a overridden IJavaProject#getChildren() with doc mentioning about the order is acceptable?
> Markus, I have updated the doc only for the > IJavaProject#getPackageFragmentRoots() and not for #getChildren as the fix > covers only the IPackageFragmentRoot and not other IJavaElement types. Do you > think a overridden IJavaProject#getChildren() with doc mentioning about the > order is acceptable? No, you should follow the pattern that is used everywhere else. IParent#getChildren() already tells that subtypes can specify an order, and that's what some subtypes already do, see e.g. Javadoc of IMember or ICompilationUnit. - IJavaProject should specify the order of children - IJavaProject#getPackageFragmentRoots() should keep reference to getChildren() - The comment you added to IJavaProject#getPackageFragmentRoots() is good. I would keep this, even though it's redundant when the getChildren() reference comes back.
Would be nice to get this fixed for M6 so that it can be extensively tested during M7 cycle.
Created attachment 190348 [details] Patch with updated doc This patch contains doc changes as suggested by Markus. Satyam, would you be able to review this today for M6? TIA!
Created attachment 190364 [details] Updated patch Patch updated as per Satyam's suggestions. Satyam, please take a look at this.
Jay, just two more comments 1. --->if (roots[i].equals(child.getPath())) { I think you would have meant --->if (roots[i].equals(child)) 2. currentEntry.getPath() could be computed and kept into a variable. Otherwise, I think the changes are good.
Created attachment 190377 [details] Updated patch Patch updated as per suggestions in comment #14. All tests pass.
Released in HEAD for 3.7M6.
Verified for 3.7M6 using build I20110307-0800
