Bug 335734 - [FlexHierarchy] InternalTreeModelViewer saveElementState and doSaveElementState hurts performance for virtual tree

4 files changed, 112 insertions, 18 deletions

diff --git a/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/LazyTests.java b/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/LazyTests.java
index 7c0ac8e2b..4f464a9a9 100644
--- a/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/LazyTests.java
+++ b/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/LazyTests.java
@@ -16,7 +16,6 @@ import junit.framework.TestCase;
 import org.eclipe.debug.tests.viewer.model.TestModel.TestElement;
 import org.eclipse.core.commands.ExecutionException;
 import org.eclipse.debug.internal.ui.viewers.model.ITreeModelContentProviderTarget;
-import org.eclipse.debug.internal.ui.viewers.model.ITreeModelViewer;
 import org.eclipse.debug.internal.ui.viewers.model.provisional.IModelDelta;
 import org.eclipse.debug.internal.ui.viewers.model.provisional.ModelDelta;
 import org.eclipse.jface.viewers.IStructuredSelection;
@@ -37,7 +36,7 @@ abstract public class LazyTests extends TestCase implements ITestModelUpdatesLis
     
     Display fDisplay;
     Shell fShell;
-    ITreeModelViewer fViewer;
+    ITreeModelContentProviderTarget fViewer;
     TestModelUpdatesListener fListener;
     
     public LazyTests(String name) {
@@ -61,8 +60,7 @@ abstract public class LazyTests extends TestCase implements ITestModelUpdatesLis
     }
 
     abstract protected ITreeModelContentProviderTarget createViewer(Display display, Shell shell);
-    
-    /**
+        /**
      * @throws java.lang.Exception
      */
     protected void tearDown() throws Exception {
@@ -196,5 +194,60 @@ abstract public class LazyTests extends TestCase implements ITestModelUpdatesLis
         Assert.assertEquals(((IStructuredSelection)fViewer.getSelection()).getFirstElement(), _1_0_newElement);
     }
 
+    /**
+     */
+    public void testContentRefresh() {
+        // Create test model with lots of children.
+        TestModel model = largeSubtreeModel(1000); 
+        
+        // Expand children all
+        fViewer.setAutoExpandLevel(-1);
+
+        // Populate initial view content
+        fListener.reset(TreePath.EMPTY, model.getRootElement(), -1, true, true); 
+        fViewer.setInput(model.getRootElement());
+        while (!fListener.isFinished(CONTENT_UPDATES_COMPLETE | LABEL_UPDATES_COMPLETE)) if (!fDisplay.readAndDispatch ()) fDisplay.sleep ();
+
+        // Turn off autoexpand
+        fViewer.setAutoExpandLevel(0);
+
+        // Reposition the viewer to middle of list
+        fListener.reset();
+        fListener.setFailOnRedundantUpdates(false);
+        fViewer.reveal(model.findElement("1"), 500);
+        while (!fListener.isFinished(CONTENT_UPDATES_COMPLETE)) if (!fDisplay.readAndDispatch ()) fDisplay.sleep ();
+        
+        // Create delta to refresh the "1" element.
+        TestElement rootElement = model.getRootElement();
+        ModelDelta rootDelta = new ModelDelta(rootElement, IModelDelta.NO_CHANGE);
+        ModelDelta expandDelta = model.getBaseDelta(rootDelta);
+        TestElement expandElement = rootElement.getChildren()[0];
+        expandDelta.addNode(expandElement, 0, IModelDelta.CONTENT, expandElement.getChildren().length);
+
+        // Rinse and repeast.  The refresh in bug 335734 is only triggered 
+        // only on the second time.
+        for (int repeatCount = 0; repeatCount < 3; repeatCount++) {
+            // Add first 250 elements (after element 500) as acceptable to materialize
+            fListener.reset(); 
+            fListener.setFailOnRedundantUpdates(true);
+            TreePath refreshElementPath = model.findElement("1");
+            fListener.addRedundantExceptionChildCount(refreshElementPath);
+            fListener.addRedundantExceptionLabel(refreshElementPath);
+            fListener.addChildreUpdate(TreePath.EMPTY, 0);
+            fListener.addHasChildrenUpdate(refreshElementPath);
+            fListener.addChildreCountUpdate(refreshElementPath);
+            fListener.addLabelUpdate(refreshElementPath); // TODO: not sure why label is updated upon expand?
+            for (int i = 499; i < 750; i++) {
+                fListener.addChildreUpdate(refreshElementPath, i);
+                TreePath childPath = refreshElementPath.createChildPath(expandElement.getChildren()[i]);
+                fListener.addLabelUpdate(childPath);
+                fListener.addHasChildrenUpdate(childPath);
+            }
+            model.postDelta(rootDelta);
+    
+            while (!fListener.isFinished(CONTENT_UPDATES_COMPLETE | MODEL_CHANGED_COMPLETE | LABEL_UPDATES_COMPLETE)) 
+                if (!fDisplay.readAndDispatch ()) fDisplay.sleep ();
+        }
+    }
 
 }
diff --git a/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/TestModelUpdatesListener.java b/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/TestModelUpdatesListener.java
index 7cbe56003..500d1038c 100644
--- a/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/TestModelUpdatesListener.java
+++ b/org.eclipse.debug.tests/src/org/eclipe/debug/tests/viewer/model/TestModelUpdatesListener.java
@@ -52,6 +52,10 @@ public class TestModelUpdatesListener
     
     private boolean fFailOnRedundantUpdates;
     private Set fRedundantUpdates = new HashSet();
+    private Set fRedundantHasChildrenUpdateExceptions = new HashSet();
+    private Set fRedundantChildCountUpdateExceptions = new HashSet();
+    private Set fRedundantChildrenUpdateExceptions = new HashSet();
+    private Set fRedundantLabelUpdateExceptions = new HashSet();
     
     private boolean fFailOnMultipleModelUpdateSequences;
     private boolean fMultipleModelUpdateSequencesObserved;
@@ -161,6 +165,10 @@ public class TestModelUpdatesListener
     public void reset() {
         fJobError = null;
         fRedundantUpdates.clear();
+        fRedundantHasChildrenUpdateExceptions.clear();
+        fRedundantChildCountUpdateExceptions.clear();
+        fRedundantChildrenUpdateExceptions.clear();
+        fRedundantLabelUpdateExceptions.clear();
         fMultipleLabelUpdateSequencesObserved = false;
         fMultipleModelUpdateSequencesObserved = false;
         fHasChildrenUpdatesScheduled.clear();
@@ -255,6 +263,22 @@ public class TestModelUpdatesListener
         }  
     }
     
+    public void addRedundantExceptionHasChildren(TreePath path) {
+        fRedundantHasChildrenUpdateExceptions.add(path);
+    }
+
+    public void addRedundantExceptionChildCount(TreePath path) {
+        fRedundantChildCountUpdateExceptions.add(path);
+    }
+
+    public void addRedundantExceptionChildren(TreePath path) {
+        fRedundantChildrenUpdateExceptions.add(path);
+    }
+
+    public void addRedundantExceptionLabel(TreePath path) {
+        fRedundantLabelUpdateExceptions.add(path);
+    }
+    
     /**
      * Returns a tree path for the node, *not* including the root element.
      * 
@@ -441,16 +465,23 @@ public class TestModelUpdatesListener
         }
 
         if (!update.isCanceled()) {
+            TreePath updatePath = update.getElementPath();
             if (update instanceof IHasChildrenUpdate) {
                 fHasChildrenUpdatesRunning.remove(update);
                 fHasChildrenUpdatesCompleted.add(update);                
-                if (!fHasChildrenUpdatesScheduled.remove(update.getElementPath()) && fFailOnRedundantUpdates) {
+                if (!fHasChildrenUpdatesScheduled.remove(updatePath) &&
+                    fFailOnRedundantUpdates && 
+                    fRedundantHasChildrenUpdateExceptions.contains(updatePath)) 
+                {
                     fRedundantUpdates.add(update);
                 }
             } if (update instanceof IChildrenCountUpdate) {
                 fChildCountUpdatesRunning.remove(update);
                 fChildCountUpdatesCompleted.add(update);                
-                if (!fChildCountUpdatesScheduled.remove(update.getElementPath()) && fFailOnRedundantUpdates) {
+                if (!fChildCountUpdatesScheduled.remove(updatePath) &&
+                    fFailOnRedundantUpdates &&
+                    !fRedundantChildCountUpdateExceptions.contains(updatePath)) 
+                {
                     fRedundantUpdates.add(update);
                 }
             } else if (update instanceof IChildrenUpdate) {
@@ -460,15 +491,15 @@ public class TestModelUpdatesListener
                 int start = ((IChildrenUpdate)update).getOffset();
                 int end = start + ((IChildrenUpdate)update).getLength();
                 
-                Set childrenIndexes = (Set)fChildrenUpdatesScheduled.get(update.getElementPath());
+                Set childrenIndexes = (Set)fChildrenUpdatesScheduled.get(updatePath);
                 if (childrenIndexes != null) {
                     for (int i = start; i < end; i++) {
                         childrenIndexes.remove(new Integer(i));
                     }
                     if (childrenIndexes.isEmpty()) {
-                        fChildrenUpdatesScheduled.remove(update.getElementPath());
+                        fChildrenUpdatesScheduled.remove(updatePath);
                     }
-                } else if (fFailOnRedundantUpdates) {
+                } else if (fFailOnRedundantUpdates && fRedundantChildrenUpdateExceptions.contains(updatePath)) {
                     fRedundantUpdates.add(update);
                 }
             } 
@@ -492,7 +523,10 @@ public class TestModelUpdatesListener
             fLabelUpdatesCompleted.add(update);
         	fLabelUpdatesCounter--;
         }
-        if (!fLabelUpdates.remove(update.getElementPath()) && fFailOnRedundantUpdates) {
+        if (!fLabelUpdates.remove(update.getElementPath()) && 
+            fFailOnRedundantUpdates && 
+            !fRedundantLabelUpdateExceptions.contains(update.getElementPath())) 
+        {
             Assert.fail("Redundant update: " + update);
         }
     }
diff --git a/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/InternalTreeModelViewer.java b/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/InternalTreeModelViewer.java
index c44e04c9e..9b41cbb5a 100644
--- a/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/InternalTreeModelViewer.java
+++ b/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/InternalTreeModelViewer.java
@@ -2301,13 +2301,13 @@ public class InternalTreeModelViewer extends TreeViewer
             items = tree.getItems(); 
         } else if (w instanceof TreeItem) {
             TreeItem item = (TreeItem)w;
-            int itemCount = item.getItemCount();
-            delta.setChildCount(((ITreeModelContentProvider)getContentProvider()).viewToModelCount(path, itemCount));
             if (item.getExpanded()) {
+                int itemCount = item.getData() != null ? item.getItemCount() : -1;
+                delta.setChildCount(((ITreeModelContentProvider)getContentProvider()).viewToModelCount(path, itemCount));
                 if ((flagsToSave & IModelDelta.EXPAND) != 0) {
                     delta.setFlags(delta.getFlags() | IModelDelta.EXPAND);
                 }
-            } else if ((flagsToSave & IModelDelta.COLLAPSE) != 0 && itemCount > 0){
+            } else if ((flagsToSave & IModelDelta.COLLAPSE) != 0){
                 delta.setFlags(delta.getFlags() | IModelDelta.COLLAPSE);
             }
             
@@ -2331,12 +2331,11 @@ public class InternalTreeModelViewer extends TreeViewer
         if (element != null) {
             boolean expanded = item.getExpanded();
             boolean selected = set.contains(item);
-            int itemCount = item.getItemCount();
             int flags = IModelDelta.NO_CHANGE;
             if (expanded && (flagsToSave & IModelDelta.EXPAND) != 0) {
                 flags = flags | IModelDelta.EXPAND;
             } 
-            if (!expanded && (flagsToSave & IModelDelta.COLLAPSE) != 0 && itemCount > 0) {
+            if (!expanded && (flagsToSave & IModelDelta.COLLAPSE) != 0) {
                 flags = flags | IModelDelta.COLLAPSE;
             }
             if (selected && (flagsToSave & IModelDelta.SELECT) != 0) {
@@ -2345,9 +2344,13 @@ public class InternalTreeModelViewer extends TreeViewer
             if (expanded || flags != IModelDelta.NO_CHANGE) {
                 int modelIndex = ((ITreeModelContentProvider)getContentProvider()).viewToModelIndex(parentPath, index);
                 TreePath elementPath = parentPath.createChildPath(element);
-                int numChildren = ((ITreeModelContentProvider)getContentProvider()).viewToModelCount(elementPath, itemCount);
-                ModelDelta childDelta = delta.addNode(element, modelIndex, flags, numChildren);
+                ModelDelta childDelta = delta.addNode(element, modelIndex, flags, -1);
                 if (expanded) {
+                    // Only get the item count if the item is expanded.  Getting
+                    // item count triggers an update of the element (bug 335734).
+                    int itemCount = item.getItemCount();                
+                    int numChildren = ((ITreeModelContentProvider)getContentProvider()).viewToModelCount(elementPath, itemCount);
+                    childDelta.setChildCount(numChildren);
                     TreeItem[] items = item.getItems();
                     for (int i = 0; i < items.length; i++) {
                         doSaveElementState(elementPath, childDelta, items[i], set, i, flagsToSave);
diff --git a/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/TreeModelContentProvider.java b/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/TreeModelContentProvider.java
index abdf54b8c..ab1136b1e 100644
--- a/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/TreeModelContentProvider.java
+++ b/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/model/TreeModelContentProvider.java
@@ -579,7 +579,11 @@ public class TreeModelContentProvider extends ModelContentProvider implements IT
                 if (DEBUG_STATE_SAVE_RESTORE && DEBUG_TEST_PRESENTATION_ID(getPresentationContext())) {
                     System.out.println("\tRESTORE COLLAPSE: " + treePath.getLastSegment()); //$NON-NLS-1$
                 }
-                getViewer().setExpandedState(treePath, false);
+                // Check auto-expand before collapsing an element (bug 335734)
+                int autoexpand = getViewer().getAutoExpandLevel();
+                if (autoexpand != ITreeModelViewer.ALL_LEVELS && autoexpand < (treePath.getSegmentCount() + 1)) {
+                    getViewer().setExpandedState(treePath, false);
+                }
                 delta.setFlags(delta.getFlags() & ~IModelDelta.COLLAPSE);
             }
 		}