Bug 67913

Comment 1 Michael Fraenkel 2004-06-18 21:53:20 EDT

Created attachment 12546 [details]
Remove invisibleBars

Comment 2 Nick Edgar 2004-06-21 12:00:57 EDT

Tagging for 3.0.1.

Glancing at the patch, it doesn't look like removeActionSet will remove the
entry from invisibleBars if it was found there.

Comment 3 Michael Fraenkel 2004-06-21 12:51:30 EDT

The removal was already being done in removeActionSet (line 83).  But this is 
partly why I made the comment about cleaning up some of the code.  I think 
some of the copying and management could be simplified.  But that is separate 
from the current problem.

Comment 4 Douglas Pollock 2004-07-06 15:12:27 EDT

Moving to the 3.0.1 milestone for consideration.... 

Comment 5 Douglas Pollock 2004-08-13 15:13:06 EDT

I have modified the patch slightly (rather than iterator two lists, it 
iterators over one larger list containing all items).   
 
I'm not entirely sure how to recreate the test case you give (i.e., it doesn't 
seem to work for me, though I'm not entirely convinced that I know what you 
mean by 'org.eclipse.debug.internal.ui.actions.DebugDropDownAction').  I would 
really appreciate it if you could test this fix when it becomes available. 
 
This patch has been reviewed by Kim, and is committed to the 3.1 and 3.0.1 
branches.  It should appear in I200408170800 and M200408180800. 
 
I have one minor concern that the clearActionSets method could be called in a 
situation where the invisible bars aren't meant to be disposed.  
WorkbenchWindow.updateActionSets is the possible culprit, but it only does if 
the active page is null.  But, I don't believe this can ever happen. 
 

Comment 6 Michael Fraenkel 2004-08-13 17:53:23 EDT

It works.  However, I would have done some slight modifications like those 
seen below.  Nothing earth shattering or required.

Index: ActionPresentation.java
===================================================================
RCS file: /home/eclipse/org.eclipse.ui.workbench/Eclipse 
UI/org/eclipse/ui/internal/ActionPresentation.java,v
retrieving revision 1.14
diff -u -r1.14 ActionPresentation.java
--- ActionPresentation.java	13 Aug 2004 19:11:26 -0000	1.14
+++ ActionPresentation.java	13 Aug 2004 21:51:20 -0000
@@ -63,7 +63,7 @@
     public void clearActionSets() {
         // Get all of the action sets -- both visible and invisible.
         List oldList = copyActionSets(mapDescToRec);
-        oldList.addAll(copyActionSets(invisibleBars));
+        oldList.addAll(invisibleBars.keySet());
 
         Iterator iter = oldList.iterator();
         while (iter.hasNext()) {
@@ -78,10 +78,7 @@
     private List copyActionSets(Map map) {
         Set keys = map.keySet();
         ArrayList list = new ArrayList(keys.size());
-        Iterator iter = keys.iterator();
-        while (iter.hasNext()) {
-            list.add(iter.next());
-        }
+        list.addAll(keys);
         return list;
     }
 
@@ -89,14 +86,11 @@
      * Destroy an action set.
      */
     public void removeActionSet(IActionSetDescriptor desc) {
-        SetRec rec = (SetRec) mapDescToRec.get(desc);
+        SetRec rec = (SetRec) mapDescToRec.remove(desc);
         if (rec == null) {
-            rec = (SetRec) invisibleBars.get(desc);
+            rec = (SetRec) invisibleBars.remove(desc);
         }
         if (rec != null) {
-            mapDescToRec.remove(desc);
-            // Remove from the map that stores invisible bars
-            invisibleBars.remove(desc);
             IActionSet set = rec.set;
             SubActionBars bars = rec.bars;
             if (bars != null) {

Comment 7 Douglas Pollock 2004-08-16 10:24:29 EDT

For 3.0.1, I'm not going to make any further changes.  For 3.1, I've taken your 
suggestions and gone one step further.  I've removed "copyActionSets" entirely. 

Comment 8 Nick Edgar 2004-08-17 13:52:53 EDT

Re comment #5, the active page is null if all perspectives are closed (e.g.
Window > Close All Perspectives).  Not sure if this case matters though.

Comment 9 Douglas Pollock 2004-08-24 13:50:52 EDT

Michael: can you verify that the fix exists in M200408180800, and works as 
you'd like?  Thanks. 

Comment 10 Michael Fraenkel 2004-08-24 23:00:17 EDT

Its fixed in M200408180800.

Just to clarify my test:

1. Create a new Workbench Window via Window -> New Window
2. Expose the Java Browsing Member View, Window -> Show View -> Other... -> 
Java Browsing -> Members
3. Switch to the Task View which has the Member View currently selected
4. Close the Workbench Window

Comment 11 Douglas Pollock 2004-08-25 12:02:09 EDT

Verified by reporter.