Community
Participate
Working Groups
It would be handy to have some method to get the IWorkingSets that are part of an aggregating IWorkingSet. Actually there is an method getComponents() in AggregateWorkingSet, but this class is internal and there is no API garantee that if an IWorkingSet.isAggregateWorkingSet() can be cast to an AggregateWorkingSet. There is an particular use case, when you need to work with IWorkingSetSelectionDialog and need to iterate the IWorkingSet[] it returns, because you want to do some action only if there is an particular working set ID inside the array. Because the user can select the window working set, it always returns the current IWorkbenchPage.getAggregateWorkingSet(). The workaround for this case is to examine if the result is the window working set, and get its components via IWorkbenchPage.getWorkingSets(). However, if we had this method, I could just work always using IWorkingSetSelectionDialog with multi == false, examine the result of isAggregateWorkingSet(), and then get the component IWorkingSets (== true) or work with the result itself (== false). I'd like to suggest to include an method IWorkingSet.getComponents() which would return the working set itself (if isAggregateWorkingSet() == false), or the component working sets (if isAggregateWorkingSet() == true).
Kim, I'm interested in this as well, as I'm trying to clean up the compiler warnings in the CNF. I would propose a new interface, IAggregateWorkingSet which would have the getComponents() method (only). If this is OK with you, I can do the work and put this in.
Created attachment 122860 [details] Proposed patch
Boris, let me know if this is OK and I will put it in.
Nits: - Can we promise that if IWorkingSet.isAggregateWorkingSet() returns true that you will be able to cast to IAggregateWorkingSet? If yes, the JavaDoc for that method should say so and link to IAggregateWorkingSet. - We should document how getComponents() is to be used by clients. I assume that clients are not allowed to hold on to the result of the method as it can change over time? - Could returning our internal array cause harm? - If the answer to my first question is yes, we should also mention in IAggregateWorkingSet that "Instances of IWorkingSet can be cast to IAggregateWorkingSet if IWorkingSet.isAggregateWorkingSet() returns true." Btw, you don't have to follow our example of just listing your company name in the Contributor section, we are just lazy. Only sometimes I notice this and then I write "Boris Bokowski, IBM"...
Created attachment 123325 [details] Revised for Boris' comments
Boris, thanks for your comments, I think I addressed them all in this revision.
Released to HEAD (3.5M5)
Created attachment 123496 [details] test > - We should document how getComponents() is to be used by clients. I assume > that clients are not allowed to hold on to the result of the method as it can > change over time? The returned array does not reflect changes made internally. > - Could returning our internal array cause harm? I feel uncomfortable with idea of not safeguarding the components array. Trouble occurs when the array is passed around or clients fail to pay attention to the spec. IMHO a copy of the array should be returned. The attached test cases are rather silly but demonstrate a potential problem. For example the 'testSaveWSet()' shows that it makes it look like the workingset API is at fault, without any hints to the actual culprit. Is this concern unwarranted? Please share your thoughts on this.
Instead of copying the array, we could change the API to return a List and use Collections.unmodifiableList(). I don't care either way, but agree with Hitesh that we'll be better off with the additional safety.
Created attachment 123560 [details] Uggg, ignore this it's the wrong patch
Comment on attachment 123560 [details] Uggg, ignore this it's the wrong patch ### Eclipse Workspace Patch 1.0 #P org.eclipse.ui.workbench Index: Eclipse UI/org/eclipse/ui/internal/AggregateWorkingSet.java =================================================================== RCS file: /cvsroot/eclipse/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/AggregateWorkingSet.java,v retrieving revision 1.9 diff -u -r1.9 AggregateWorkingSet.java --- Eclipse UI/org/eclipse/ui/internal/AggregateWorkingSet.java 22 Jan 2009 11:14:34 -0000 1.9 +++ Eclipse UI/org/eclipse/ui/internal/AggregateWorkingSet.java 23 Jan 2009 17:31:58 -0000 @@ -163,26 +163,29 @@ restoreWorkingSet(); workingSetMemento = null; } - return components; + + IWorkingSet[] copiedArray = new IWorkingSet[components.length]; + System.arraycopy(components, 0, copiedArray, 0, components.length); + return copiedArray; } public void propertyChange(PropertyChangeEvent event) { String property = event.getProperty(); if (property.equals(IWorkingSetManager.CHANGE_WORKING_SET_REMOVE)) { - for (int i = 0; i < getComponents().length; i++) { - IWorkingSet set = getComponents()[i]; + for (int i = 0; i < components.length; i++) { + IWorkingSet set = components[i]; if (set.equals(event.getOldValue())) { IWorkingSet[] newComponents = new IWorkingSet[components.length - 1]; Util - .arrayCopyWithRemoval(getComponents(), + .arrayCopyWithRemoval(components, newComponents, i); setComponents(newComponents); } } } else if (property .equals(IWorkingSetManager.CHANGE_WORKING_SET_CONTENT_CHANGE)) { - for (int i = 0; i < getComponents().length; i++) { - IWorkingSet set = getComponents()[i]; + for (int i = 0; i < components.length; i++) { + IWorkingSet set = components[i]; if (set.equals(event.getNewValue())) { constructElements(true); break; @@ -221,13 +224,13 @@ AggregateWorkingSet workingSet = (AggregateWorkingSet) object; return Util.equals(workingSet.getName(), getName()) - && Util.equals(workingSet.getComponents(), getComponents()); + && Util.equals(workingSet.components, components); } return false; } public int hashCode() { - int hashCode = getName().hashCode() & getComponents().hashCode(); + int hashCode = getName().hashCode() & components.hashCode(); return hashCode; } Index: Eclipse UI/org/eclipse/ui/IAggregateWorkingSet.java =================================================================== RCS file: /cvsroot/eclipse/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/IAggregateWorkingSet.java,v retrieving revision 1.1 diff -u -r1.1 IAggregateWorkingSet.java --- Eclipse UI/org/eclipse/ui/IAggregateWorkingSet.java 22 Jan 2009 11:14:34 -0000 1.1 +++ Eclipse UI/org/eclipse/ui/IAggregateWorkingSet.java 23 Jan 2009 17:31:58 -0000 @@ -30,9 +30,8 @@ /** * Returns the working sets contained in this aggregate working set. * - * <p> - * The returned array is subject to change if the aggregate working set - * changes. Clients should not modify the contents of the array. + * Clients can do what they wish with the returned array, as it + * will have no effect on the state of the this object. * * @return the working sets contained in this aggregate working set. */
Created attachment 123562 [details] Correct patch for copying array
Released revisions and test case (thanks!) to HEAD for 3.5M5
*** Bug 153961 has been marked as a duplicate of this bug. ***
