flex-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mih...@apache.org
Subject [17/19] git commit: [flex-sdk] [refs/heads/develop] - FLEX-34456 CAUSE: When a HierarchicalCollectionViewCursor reacted to a deletion before another cursor, it deleted the parent mappings of the deleted nodes (even if it didn't need to make any changes t
Date Mon, 11 Aug 2014 11:41:59 GMT
FLEX-34456
CAUSE:
When a HierarchicalCollectionViewCursor reacted to a deletion before another cursor, it deleted
the parent mappings of the deleted nodes (even if it didn't need to make any changes to its
internal state). Since these mappings are on the HierarchicalCollectionView, the next cursor
to react to the same deletion would compute that the parent of the deleted node is null, and
everything went bad from there.
In other words, if two or more cursors are created on the same HierarchicalCollectionView,
they will work against each other when deletions occur (or replacements, which HierarchicalCollectionView
transforms into deletions).
I think that the mapping deletions happened because HierarchicalCollectionViewCursor was written
with the assumption that there would only be one cursor for each collection.

SOLUTION:
Now we're making it HierarchicalCollectionView's job to remove the parent mappings of all
the affected nodes after it gives all the cursors the chance to react to the deletion.

NOTES:
-As part of this change I realised that the conditions for an if clause introduced for FLEX-34424
were not enough: in order to choose strategy 2) (see revision 82d6b51 for details) for adjusting
the cursor position, not only does current need to be null, but afterLast and beforeFirst
need to be false (because if they are true, it's expected and correct that current == null).
-This led me to notice that HierarchicalCollectionViewCursor.beforeFirst was true if current
== null && currentIndex <= collection.length, which doesn't make sense. In fact,
these conditions correctly determine the FLEX-34424 situation, in which the item at the current
index doesn't exist anymore, which makes current null, although currentIndex is within bounds.
(Another clue that the condition for beforeFirst is wrong is ListCollectionView.beforeFirst,
which checks the index against -1, rather than allowing any index up to the collection length.)
So I changed this to check for any value < 0.
-I also made HierarchicalCollectionView.parentMap private instead of accessible from other
classes (like HierarchicalCollectionViewCursor), and made the operations on it (deletions
and additions) accessible only through functions, rather than using the map directly, as before.
-Both the above changes also affected LeafNodeCursor, which also had a defective beforeFirst
getter, and also directly manipulated the collection's parentMap.
-Also removed unused variables from HierarchicalCollectionView.removeChild() and .collectionChangeHandler().


Project: http://git-wip-us.apache.org/repos/asf/flex-sdk/repo
Commit: http://git-wip-us.apache.org/repos/asf/flex-sdk/commit/04bb0b27
Tree: http://git-wip-us.apache.org/repos/asf/flex-sdk/tree/04bb0b27
Diff: http://git-wip-us.apache.org/repos/asf/flex-sdk/diff/04bb0b27

Branch: refs/heads/develop
Commit: 04bb0b27de8885b7378bcdc2b0ed11a7a1ccd984
Parents: 65d90e3
Author: Mihai Chira <mihaic@apache.org>
Authored: Fri Aug 8 12:39:57 2014 +0100
Committer: Mihai Chira <mihaic@apache.org>
Committed: Fri Aug 8 12:39:57 2014 +0100

----------------------------------------------------------------------
 .../collections/HierarchicalCollectionView.as   | 39 +++++++++++++-------
 .../HierarchicalCollectionViewCursor.as         | 21 +++--------
 .../src/mx/collections/LeafNodeCursor.as        |  7 ++--
 ...hicalCollectionViewCursor_FLEX_34456_Test.as |  3 --
 4 files changed, 35 insertions(+), 35 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/flex-sdk/blob/04bb0b27/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionView.as
----------------------------------------------------------------------
diff --git a/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionView.as
b/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionView.as
index 40ba8c3..6d4901c 100644
--- a/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionView.as
+++ b/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionView.as
@@ -139,7 +139,7 @@ public class HierarchicalCollectionView extends EventDispatcher
      *  Mapping of UID to parents.  Must be maintained as things get removed/added
      *  This map is created as objects are visited
      */
-    mx_internal var parentMap:Object;
+    private var parentMap:Object;
 
     //--------------------------------------------------------------------------
     //
@@ -536,6 +536,17 @@ public class HierarchicalCollectionView extends EventDispatcher
 
         return undefined;
     }
+
+    public function deleteParentMapping(uid:String):void
+    {
+        delete parentMap[uid];
+    }
+
+    public function addParentMapping(uid:String, parent:Object, replaceExisting:Boolean =
true):void
+    {
+        if(replaceExisting || !parentMap.hasOwnProperty(uid))
+            parentMap[uid] = parent;
+    }
     
     /**
      *  @inheritDoc 
@@ -677,7 +688,7 @@ public class HierarchicalCollectionView extends EventDispatcher
             while (!cursor.afterLast)
             {
                 var uid:String = UIDUtil.getUID(cursor.current);
-                delete parentMap[uid];
+                deleteParentMapping(uid);
 
                 try
                 {
@@ -722,7 +733,6 @@ public class HierarchicalCollectionView extends EventDispatcher
     public function removeChild(parent:Object, child:Object):Boolean
     {
         var cursor:IViewCursor;
-        var index:int = 0;
         if (parent == null)
         {
             cursor = treeData.createCursor();
@@ -1007,7 +1017,7 @@ public class HierarchicalCollectionView extends EventDispatcher
         {
             var item:Object = cursor.current;
             var uid:String = UIDUtil.getUID(item);
-            parentMap[uid] = node;
+            addParentMapping(uid, node);
             
             // check that the node is opened or not.
             // If it is open, then update the length with the node's children.
@@ -1091,7 +1101,7 @@ public class HierarchicalCollectionView extends EventDispatcher
         else
         {
             var uid:String = UIDUtil.getUID(node);
-            parentMap[uid] = parent;
+            addParentMapping(uid, parent);
             if (node != null &&
                 openNodes[uid] &&
                 hierarchicalData.canHaveChildren(node) &&
@@ -1303,7 +1313,7 @@ public class HierarchicalCollectionView extends EventDispatcher
         nodeArray.push(node);
 
         var uid:String = UIDUtil.getUID(node);
-        parentMap[uid] = parent;
+        addParentMapping(uid, parent);
         if (openNodes[uid] != null &&
             hierarchicalData.canHaveChildren(node) &&
             hierarchicalData.hasChildren(node))
@@ -1422,11 +1432,7 @@ public class HierarchicalCollectionView extends EventDispatcher
     {
         var i:int;
         var n:int;
-        var location:int;
-        var uid:String;
-        var parent:Object;
         var node:Object;
-        var items:Array;
 
         var convertedEvent:CollectionEvent;
         
@@ -1482,9 +1488,16 @@ public class HierarchicalCollectionView extends EventDispatcher
                         stopTrackUpdates(node);
                     getVisibleNodes(node, null, convertedEvent.items);
                 }
+
                 currentLength -= convertedEvent.items.length;
+
                 dispatchEvent(convertedEvent);
-                
+
+                n = convertedEvent.items.length;
+                for (i = 0; i < n; i++)
+                {
+                    deleteParentMapping(UIDUtil.getUID(convertedEvent.items[i]));
+                }
             }
             else if (ce.kind == CollectionEventKind.UPDATE)
             {
@@ -1629,7 +1642,7 @@ public class HierarchicalCollectionView extends EventDispatcher
                         // if the parent node is opened
                         if (_openNodes[UIDUtil.getUID(parentOfChangingNode)] != null)
                         {
-                            parentMap[UIDUtil.getUID(ce.items[i].newValue)] = parentOfChangingNode;
+                            addParentMapping(UIDUtil.getUID(ce.items[i].newValue), parentOfChangingNode);
                         }
                     }
                 }
@@ -1659,7 +1672,7 @@ public class HierarchicalCollectionView extends EventDispatcher
                     parentOfChangingNode = getParentItem(changingNode);
 
                     if (parentOfChangingNode != null && _openNodes[UIDUtil.getUID(parentOfChangingNode)]
!= null)
-                        delete parentMap[UIDUtil.getUID(changingNode)];
+                        deleteParentMapping(UIDUtil.getUID(changingNode));
                 }
             }
             else if (ce.kind == CollectionEventKind.RESET)

http://git-wip-us.apache.org/repos/asf/flex-sdk/blob/04bb0b27/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionViewCursor.as
----------------------------------------------------------------------
diff --git a/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionViewCursor.as
b/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionViewCursor.as
index 954d948..ef89479 100644
--- a/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionViewCursor.as
+++ b/frameworks/projects/advancedgrids/src/mx/collections/HierarchicalCollectionViewCursor.as
@@ -265,7 +265,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
      */
     public function get beforeFirst():Boolean
     {
-        return (currentIndex <= collection.length && current == null);
+        return currentIndex < 0 && current == null;
     }
     
     //----------------------------------
@@ -281,7 +281,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
      */
     public function get afterLast():Boolean
     {
-        return (currentIndex >= collection.length && current == null); 
+        return currentIndex >= collection.length && current == null;
     } 
     
     //----------------------------------
@@ -755,7 +755,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
 
         updateParentMap(currentNode);
 
-        currentIndex--; 
+        currentIndex--;
         return true;
     }
 
@@ -878,7 +878,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
     public function insert(item:Object):void
     {
         var parent:Object = collection.getParentItem(current);
-        collection.addChildAt(parent, item, currentIndex); 
+        collection.addChildAt(parent, item, currentIndex);
     }
     
     /**
@@ -923,8 +923,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
         if (currentNode != null)
         {
             var uid:String = UIDUtil.getUID(currentNode);
-            if (!collection.parentMap.hasOwnProperty(uid))
-                collection.parentMap[uid] = parentNodes[parentNodes.length - 1];
+            collection.addParentMapping(uid, parentNodes[parentNodes.length - 1], false);
         }
     }
 
@@ -1227,7 +1226,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
             {
                 var lastIndexAffectedByDeletion:int = event.location + n;
                 var isCurrentIndexAmongRemovedNodes:Boolean = lastIndexAffectedByDeletion
>= currentIndex;
-                var currentItemNotFoundAmongItsSiblings:Boolean = isCurrentIndexAmongRemovedNodes
? false : current == null;
+                var currentItemNotFoundAmongItsSiblings:Boolean = isCurrentIndexAmongRemovedNodes
? false : (!afterLast && !beforeFirst && current == null);
 
                 if (isCurrentIndexAmongRemovedNodes || currentItemNotFoundAmongItsSiblings)
                 {
@@ -1237,10 +1236,6 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
                     var indexToReturnTo:int = isCurrentIndexAmongRemovedNodes ? event.location
: currentIndex - n;
                     moveToFirst();
                     seek(CursorBookmark.FIRST, indexToReturnTo);
-                    for (i = 0; i < n; i++)
-                    {
-                        delete collection.parentMap[UIDUtil.getUID(event.items[i])];
-                    }
 
                     return;
                 }
@@ -1303,10 +1298,7 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
                         }
                     }
                 }
-
-                delete collection.parentMap[UIDUtil.getUID(changingNode)];
             }
-            
         }
         else if (event.kind == CollectionEventKind.RESET)
         {
@@ -1326,5 +1318,4 @@ public class HierarchicalCollectionViewCursor extends EventDispatcher
         }
     }
 }
-
 }

http://git-wip-us.apache.org/repos/asf/flex-sdk/blob/04bb0b27/frameworks/projects/advancedgrids/src/mx/collections/LeafNodeCursor.as
----------------------------------------------------------------------
diff --git a/frameworks/projects/advancedgrids/src/mx/collections/LeafNodeCursor.as b/frameworks/projects/advancedgrids/src/mx/collections/LeafNodeCursor.as
index 143e44f..0703b78 100644
--- a/frameworks/projects/advancedgrids/src/mx/collections/LeafNodeCursor.as
+++ b/frameworks/projects/advancedgrids/src/mx/collections/LeafNodeCursor.as
@@ -236,7 +236,7 @@ public class LeafNodeCursor extends EventDispatcher
 	 */
     public function get beforeFirst():Boolean
     {
-    	return (currentIndex <= collection.length && current == null);
+    	return currentIndex < 0 && current == null;
     }
     
 	//----------------------------------
@@ -247,7 +247,7 @@ public class LeafNodeCursor extends EventDispatcher
 	 */
     public function get afterLast():Boolean
     {
-        return (currentIndex >= collection.length && current == null); 
+        return currentIndex >= collection.length && current == null;
     } 
     
 	//----------------------------------
@@ -304,8 +304,7 @@ public class LeafNodeCursor extends EventDispatcher
     		return false; 
     	
 		var uid:String = UIDUtil.getUID(currentNode);
-		if (!collection.parentMap.hasOwnProperty(uid))
-			collection.parentMap[uid] = parentNodes[parentNodes.length - 1];
+        collection.addParentMapping(uid, parentNodes[parentNodes.length - 1], false);
 
 		var flag:Boolean = true;		
 		// If current node is a branch and is open, the first child is our next item so return
it

http://git-wip-us.apache.org/repos/asf/flex-sdk/blob/04bb0b27/frameworks/tests/unitTests/mx/collections/HierarchicalCollectionViewCursor_FLEX_34456_Test.as
----------------------------------------------------------------------
diff --git a/frameworks/tests/unitTests/mx/collections/HierarchicalCollectionViewCursor_FLEX_34456_Test.as
b/frameworks/tests/unitTests/mx/collections/HierarchicalCollectionViewCursor_FLEX_34456_Test.as
index 5682c7f..2127597 100644
--- a/frameworks/tests/unitTests/mx/collections/HierarchicalCollectionViewCursor_FLEX_34456_Test.as
+++ b/frameworks/tests/unitTests/mx/collections/HierarchicalCollectionViewCursor_FLEX_34456_Test.as
@@ -49,7 +49,6 @@ package mx.collections
 			_currentHierarchy = _utils.clone(_generatedHierarchy);
 			_utils.openAllNodes(_currentHierarchy);
 			_sut = _currentHierarchy.createCursor() as HierarchicalCollectionViewCursor;
-			_sut.name = "_sut";
 		}
 		
 		[After]
@@ -76,7 +75,6 @@ package mx.collections
 		   
             //2. Perform operation
 		   _operationCursor = _currentHierarchy.createCursor() as HierarchicalCollectionViewCursor;
-		   _operationCursor.name = "_operationCursor";
 		   _operationCursor.seek(new CursorBookmark(operationIndex));
 		   
             if (operation == OP_ADD)
@@ -89,7 +87,6 @@ package mx.collections
 
             //3. Create mirror HierarchicalCollectionView from the changed root, as the source
of truth
             _mirrorCursor = _utils.navigateToItem(_currentHierarchy.createCursor() as HierarchicalCollectionViewCursor,
selectedNode) as HierarchicalCollectionViewCursor;
-			_mirrorCursor.name = "_mirrorCursor";
 
             //4. Navigate somewhere in both HierarchicalCollectionViews and make sure they
do the same thing
             _sut.moveNext();


Mime
View raw message