Community
Participate
Working Groups
When setting the elements using WorkingSetModel.setWorkingSets(IWorkingSet[], boolean, IWorkingSet[]) or WorkingSetModel.setActiveWorkingSets(IWorkingSet[]) then the ordering in fActiveWorkingSets and fAllWorkingSets can differ. This is bad because that ordering is used in the dialog and the drop adapter. Test Case: 1. In PackageExplorerPart.internalTestShowWorkingSets replace fWorkingSetModel.setWorkingSets(workingSets, false, workingSets); with: fWorkingSetModel.setActiveWorkingSets(workingSets); 2. Run org.eclipse.jdt.ui.tests.packageview.WorkingSetDropAdapterTest ==> test failure. This happens because previous tests created a different order and when we then set [w1,w2,w3] the order in active and all is different. ==> we need to ensure that the same relative ordering is used.
Please also have a closer look at WorkingSetModel.java 1.40, line 567: allElements.toArray(new IWorkingSet[elements.size()]) The "elements.size()" looks wrong to me.
Created attachment 190139 [details] Patch (In reply to comment #0) > When setting the elements using WorkingSetModel.setWorkingSets(IWorkingSet[], > boolean, IWorkingSet[]) or WorkingSetModel.setActiveWorkingSets(IWorkingSet[]) > then the ordering in fActiveWorkingSets and fAllWorkingSets can differ. This is > bad because that ordering is used in the dialog and the drop adapter. > > Test Case: > 1. In PackageExplorerPart.internalTestShowWorkingSets replace > fWorkingSetModel.setWorkingSets(workingSets, false, workingSets); > with: > fWorkingSetModel.setActiveWorkingSets(workingSets); > 2. Run org.eclipse.jdt.ui.tests.packageview.WorkingSetDropAdapterTest > ==> test failure. > > This happens because previous tests created a different order and when we then > set [w1,w2,w3] the order in active and all is different. > > ==> we need to ensure that the same relative ordering is used. This happens only programmatically when the working sets are created in one order and set as active in another since the working set manager updates the fAllWorkingSets in the created order and fActiveWorkingSets has the newly set order. The second test fails because even though both lists of working sets are restored from the memento in the same order that the previous test left it, the second test creates working sets in a differnt order and only re-sets the fActiveWorkingSet resulting in a mismatch. But this does not happen in DnD operation since org.eclipse.jdt.internal.ui.packageview.WorkingSetDropAdapter.performWorkingSetReordering() takes care of the order of both lists on performDrop(). Also in configuration dialog we have not only active list but all working sets list, so the order there is saved in WorkingSetConfigurationDialog.fAllWorkingSets. We do not see this code path any where else other than the test. So the test must ensure that the order of creation is the same as the order used to set active working sets (since the order is saved by PE in a memento for subsequent tests) or set both the lists with the new order using setWorkingSets(..)(-->as in HEAD) Or to be safe we could also add a check in setActiveWorkingSets(..) for such cases. The only solution in case of a difference in order is to reset fAllWorkingSets to default from the working set managers because the actual order of all working sets is lost. (-->patch 1)
Dani, could you pls review?
(In reply to comment #1) > Please also have a closer look at WorkingSetModel.java 1.40, line 567: > > allElements.toArray(new IWorkingSet[elements.size()]) > > The "elements.size()" looks wrong to me. Also fixed in patch. Thanks for noticing, Markus.
The fix is not good yet: 1. In setActiveWS the order that's already present in all working sets gets destroyed (all active ones are pushed on top). 2. setWorkingSets still has the issue. We should specify in the Javadoc that the relative order must be the same. For setWorkingSets - add to the Javadoc that all active ws must be part of all working sets - rename the parameter 'workingSets' to 'allWorkingSets' > > The "elements.size()" looks wrong to me. > > Also fixed in patch. Thanks for noticing, Markus. Committed that part to HEAD.
Created attachment 190159 [details] Patch_v2 (In reply to comment #5) > The fix is not good yet: > 1. In setActiveWS the order that's already present in all working sets gets > destroyed (all active ones are pushed on top). As discussed, since this is a public method such inconsistencies in ordering could happen accidentally by passing wrong parameters to the setters, we need to ensure the correct order in all working sets by comparing with active working sets list. Fixed this. > 2. setWorkingSets still has the issue. We should specify in the Javadoc that > the relative order must be the same. > > For setWorkingSets > - add to the Javadoc that all active ws must be part of all working sets > - rename the parameter 'workingSets' to 'allWorkingSets' ADded javadoc to both setters and an additional check for order in all working sets in setWorkingSets(..)
Looks good. Some minor details to fix before committing: - move note to Javadoc before @param. - setOrderInAllWorkingSets -> adjustOrderingOfAllWorkingSets - you can reorder without additional list by swapping elements - also improve Javadoc - isOrderDifferentInWorkingSetLists - can be implemented more easily: no need to create another collection and compare it. Also, one can return as soon as one detects the first wrong ordering mismatch. In general avoid creating extra variables and especially collections if not needed.
Created attachment 190435 [details] Patch_v3 Patch with suggested changes.
(In reply to comment #8) > Created attachment 190435 [details] [diff] > Patch_v3 > > Patch with suggested changes. Committed to HEAD.
