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/FLEX-34119 Commit: 04bb0b27de8885b7378bcdc2b0ed11a7a1ccd984 Parents: 65d90e3 Author: Mihai Chira <mih...@apache.org> Authored: Fri Aug 8 12:39:57 2014 +0100 Committer: Mihai Chira <mih...@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();
