From b0c715d1c27b994ba3d5be056f218e818803a77b Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Tue, 3 Jul 2018 14:04:26 -0700 Subject: [PATCH 01/15] Add move detection and support to ASLayoutTransition ...and NSArray+Diffing. Add some tests. --- AsyncDisplayKit.xcodeproj/project.pbxproj | 8 + Source/ASDisplayNode+Layout.mm | 2 +- Source/AsyncDisplayKit.h | 1 + Source/Details/NSArray+Diffing.h | 8 + Source/Details/NSArray+Diffing.m | 125 +++++++++++--- Source/Layout/ASLayout+IGListKit.h | 15 ++ Source/Layout/ASLayout+IGListKit.mm | 31 ++++ Source/Private/ASLayoutTransition.h | 5 +- Source/Private/ASLayoutTransition.mm | 68 ++++++-- Tests/ASDisplayNodeImplicitHierarchyTests.m | 6 +- Tests/ASDisplayNodeTests.mm | 29 +++- Tests/ArrayDiffingTests.m | 180 +++++++++++++++++++- 12 files changed, 427 insertions(+), 51 deletions(-) create mode 100644 Source/Layout/ASLayout+IGListKit.h create mode 100644 Source/Layout/ASLayout+IGListKit.mm diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index bb8227f27..d01f0a005 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -31,6 +31,8 @@ 058D0A40195D057000B7D73C /* ASTextNodeTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 058D0A36195D057000B7D73C /* ASTextNodeTests.m */; }; 058D0A41195D057000B7D73C /* ASTextNodeWordKernerTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 058D0A37195D057000B7D73C /* ASTextNodeWordKernerTests.mm */; }; 05EA6FE71AC0966E00E35788 /* ASSnapshotTestCase.m in Sources */ = {isa = PBXBuildFile; fileRef = 05EA6FE61AC0966E00E35788 /* ASSnapshotTestCase.m */; }; + 0FAFDF7520EC1C90003A51C0 /* ASLayout+IGListKit.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FAFDF7320EC1C8F003A51C0 /* ASLayout+IGListKit.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 0FAFDF7620EC1C90003A51C0 /* ASLayout+IGListKit.mm in Sources */ = {isa = PBXBuildFile; fileRef = 0FAFDF7420EC1C90003A51C0 /* ASLayout+IGListKit.mm */; }; 18C2ED7F1B9B7DE800F627B3 /* ASCollectionNode.h in Headers */ = {isa = PBXBuildFile; fileRef = 18C2ED7C1B9B7DE800F627B3 /* ASCollectionNode.h */; settings = {ATTRIBUTES = (Public, ); }; }; 18C2ED831B9B7DE800F627B3 /* ASCollectionNode.mm in Sources */ = {isa = PBXBuildFile; fileRef = 18C2ED7D1B9B7DE800F627B3 /* ASCollectionNode.mm */; }; 1A6C000D1FAB4E2100D05926 /* ASCornerLayoutSpec.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A6C000B1FAB4E2000D05926 /* ASCornerLayoutSpec.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -601,6 +603,8 @@ 058D0A44195D058D00B7D73C /* ASBaseDefines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASBaseDefines.h; sourceTree = ""; }; 05EA6FE61AC0966E00E35788 /* ASSnapshotTestCase.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASSnapshotTestCase.m; sourceTree = ""; }; 05F20AA31A15733C00DCA68A /* ASImageProtocols.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASImageProtocols.h; sourceTree = ""; }; + 0FAFDF7320EC1C8F003A51C0 /* ASLayout+IGListKit.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "ASLayout+IGListKit.h"; sourceTree = ""; }; + 0FAFDF7420EC1C90003A51C0 /* ASLayout+IGListKit.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "ASLayout+IGListKit.mm"; sourceTree = ""; }; 18C2ED7C1B9B7DE800F627B3 /* ASCollectionNode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASCollectionNode.h; sourceTree = ""; }; 18C2ED7D1B9B7DE800F627B3 /* ASCollectionNode.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ASCollectionNode.mm; sourceTree = ""; }; 1950C4481A3BB5C1005C8279 /* ASEqualityHelpers.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASEqualityHelpers.h; sourceTree = ""; }; @@ -1635,6 +1639,8 @@ ACF6ED0A1B17843500DA7C62 /* ASInsetLayoutSpec.mm */, ACF6ED0B1B17843500DA7C62 /* ASLayout.h */, ACF6ED0C1B17843500DA7C62 /* ASLayout.mm */, + 0FAFDF7320EC1C8F003A51C0 /* ASLayout+IGListKit.h */, + 0FAFDF7420EC1C90003A51C0 /* ASLayout+IGListKit.mm */, ACF6ED111B17843500DA7C62 /* ASLayoutElement.h */, E55D86311CA8A14000A0C26F /* ASLayoutElement.mm */, 698C8B601CAB49FC0052DC3F /* ASLayoutElementExtensibility.h */, @@ -1850,6 +1856,7 @@ 69E0E8A71D356C9400627613 /* ASEqualityHelpers.h in Headers */, 698C8B621CAB49FC0052DC3F /* ASLayoutElementExtensibility.h in Headers */, 69F10C871C84C35D0026140C /* ASRangeControllerUpdateRangeProtocol+Beta.h in Headers */, + 0FAFDF7520EC1C90003A51C0 /* ASLayout+IGListKit.h in Headers */, B350623C1B010EFD0018CF92 /* _ASAsyncTransaction.h in Headers */, 68355B411CB57A6C001D4E68 /* ASImageContainerProtocolCategories.h in Headers */, 7630FFA81C9E267E007A7C0E /* ASVideoNode.h in Headers */, @@ -2368,6 +2375,7 @@ E5B078001E69F4EB00C24B5B /* ASElementMap.m in Sources */, 9C8898BC1C738BA800D6B02E /* ASTextKitFontSizeAdjuster.mm in Sources */, 690ED59B1E36D118000627C0 /* ASImageNode+tvOS.m in Sources */, + 0FAFDF7620EC1C90003A51C0 /* ASLayout+IGListKit.mm in Sources */, CCDC9B4E200991D10063C1F8 /* ASGraphicsContext.m in Sources */, CCCCCCD81EC3EF060087FE10 /* ASTextInput.m in Sources */, 34EFC7621B701CA400AD841F /* ASBackgroundLayoutSpec.mm in Sources */, diff --git a/Source/ASDisplayNode+Layout.mm b/Source/ASDisplayNode+Layout.mm index 239057e33..8037cbf1b 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -680,7 +680,7 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize } // Apply the subnode insertion immediately to be able to animate the nodes - [pendingLayoutTransition applySubnodeInsertions]; + [pendingLayoutTransition applySubnodeInsertionsAndMoves]; // Kick off animating the layout transition { diff --git a/Source/AsyncDisplayKit.h b/Source/AsyncDisplayKit.h index 227b13f21..d98d00361 100644 --- a/Source/AsyncDisplayKit.h +++ b/Source/AsyncDisplayKit.h @@ -137,3 +137,4 @@ #import #import +#import diff --git a/Source/Details/NSArray+Diffing.h b/Source/Details/NSArray+Diffing.h index 3a1729337..02a40685c 100644 --- a/Source/Details/NSArray+Diffing.h +++ b/Source/Details/NSArray+Diffing.h @@ -35,4 +35,12 @@ */ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions compareBlock:(BOOL (^)(id lhs, id rhs))comparison; +/** + * @abstract Compares two arrays, providing the insertion, deletion, and move indexes needed to transform into the target array. + * @discussion This compares the equality of each object with `isEqual:`. + * This diffing algorithm uses a bottom-up memoized longest common subsequence solution to identify differences. + * It runs in O(mn) complexity. + * The moves are returned in ascending order of their destination index. + */ +- (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions moves:(NSArray **)moves; @end diff --git a/Source/Details/NSArray+Diffing.m b/Source/Details/NSArray+Diffing.m index ee0ba0ff3..d8714f453 100644 --- a/Source/Details/NSArray+Diffing.m +++ b/Source/Details/NSArray+Diffing.m @@ -20,50 +20,106 @@ @implementation NSArray (Diffing) +typedef BOOL (^compareBlock)(id _Nonnull lhs, id _Nonnull rhs); + +- (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions +{ + [self asdk_diffWithArray:array insertions:insertions deletions:deletions moves:nil compareBlock:[NSArray defaultCompareBlock]]; +} + +- (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions + compareBlock:(compareBlock)comparison +{ + [self asdk_diffWithArray:array insertions:insertions deletions:deletions moves:nil compareBlock:comparison]; +} + - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions + moves:(NSArray **)moves { - [self asdk_diffWithArray:array insertions:insertions deletions:deletions compareBlock:^BOOL(id lhs, id rhs) { - return [lhs isEqual:rhs]; - }]; + [self asdk_diffWithArray:array insertions:insertions deletions:deletions moves:moves + compareBlock:[NSArray defaultCompareBlock]]; } -- (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions compareBlock:(BOOL (^)(id lhs, id rhs))comparison +- (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions + moves:(NSArray **)moves compareBlock:(compareBlock)comparison { NSAssert(comparison != nil, @"Comparison block is required"); - NSIndexSet *commonIndexes = [self _asdk_commonIndexesWithArray:array compareBlock:comparison]; - - if (insertions) { - NSArray *commonObjects = [self objectsAtIndexes:commonIndexes]; - NSMutableIndexSet *insertionIndexes = [[NSMutableIndexSet alloc] init]; - for (NSInteger i = 0, j = 0; i < commonObjects.count || j < array.count;) { - if (i < commonObjects.count && j < array.count && comparison(commonObjects[i], array[j])) { - i++; j++; + NSAssert(moves == nil || comparison == [NSArray defaultCompareBlock], @"move detection requires isEqual: and hash (no custom compare)"); + NSMutableDictionary *> *potentialMoves = nil; + NSMutableArray *moveIndexPaths = nil; + NSMutableIndexSet *insertionIndexes = nil, *deletionIndexes = nil; + if (moves) { + potentialMoves = [NSMutableDictionary new]; + moveIndexPaths = [NSMutableArray new]; + } + NSMutableIndexSet *commonIndexes = [[self _asdk_commonIndexesWithArray:array compareBlock:comparison] mutableCopy]; + + if (deletions || moves) { + deletionIndexes = [NSMutableIndexSet indexSet]; + for (NSUInteger i = 0; i < self.count; i++) { + if (![commonIndexes containsIndex:i]) { + [deletionIndexes addIndex:i]; + } + NSNumber *hash = @([self[i] hash]); + if (!potentialMoves[hash]) { + potentialMoves[hash] = [@[@(i)] mutableCopy]; } else { - [insertionIndexes addIndex:j]; - j++; + [potentialMoves[hash] addObject:@(i)]; } } - *insertions = insertionIndexes; } - - if (deletions) { - NSMutableIndexSet *deletionIndexes = [[NSMutableIndexSet alloc] init]; - for (NSInteger i = 0; i < self.count; i++) { - if (![commonIndexes containsIndex:i]) { - [deletionIndexes addIndex:i]; + + if (insertions || moves) { + insertionIndexes = [NSMutableIndexSet indexSet]; + NSArray *commonObjects = [self objectsAtIndexes:commonIndexes]; + BOOL moveFound; + NSUInteger movedFrom = NSNotFound; + for (NSUInteger i = 0, j = 0; j < array.count; j++) { + NSNumber *hash = @([array[j] hash]); + moveFound = (potentialMoves[hash] != nil); + if (moveFound) { + movedFrom = [[potentialMoves[hash] firstObject] unsignedIntegerValue]; + [potentialMoves[hash] removeObjectAtIndex:0]; + if (![potentialMoves[hash] count]) { + potentialMoves[hash] = nil; + } + if (movedFrom != j) { + NSUInteger indexes[] = {movedFrom, j}; + [moveIndexPaths addObject:[NSIndexPath indexPathWithIndexes:indexes length:2]]; + } + } + if (i < commonObjects.count && j < array.count && comparison(commonObjects[i], array[j])) { + i++; + } else { + if (moveFound) { + // moves will coalesce a delete / insert - the insert is just not done, and here we remove the delete: + [deletionIndexes removeIndex:movedFrom]; + // OR a move will have come from the LCS: + if ([commonIndexes containsIndex:movedFrom]) { + [commonIndexes removeIndex:movedFrom]; + commonObjects = [self objectsAtIndexes:commonIndexes]; + } + } else { + [insertionIndexes addIndex:j]; + } } } - *deletions = deletionIndexes; } + + if (moves) {*moves = moveIndexPaths;} + if (deletions) {*deletions = deletionIndexes;} + if (insertions) {*insertions = insertionIndexes;} } +// https://github.com/raywenderlich/swift-algorithm-club/tree/master/Longest%20Common%20Subsequence is not exactly this code (obviously), but +// is a good commentary on the algorithm. - (NSIndexSet *)_asdk_commonIndexesWithArray:(NSArray *)array compareBlock:(BOOL (^)(id lhs, id rhs))comparison { NSAssert(comparison != nil, @"Comparison block is required"); - + NSInteger selfCount = self.count; NSInteger arrayCount = array.count; - + // Allocate the diff map in the heap so we don't blow the stack for large arrays. NSInteger **lengths = NULL; lengths = (NSInteger **)malloc(sizeof(NSInteger*) * (selfCount+1)); @@ -71,7 +127,7 @@ - (NSIndexSet *)_asdk_commonIndexesWithArray:(NSArray *)array compareBlock:(BOOL ASDisplayNodeFailAssert(@"Failed to allocate memory for diffing"); return nil; } - + // Fill in a LCS length matrix: for (NSInteger i = 0; i <= selfCount; i++) { lengths[i] = (NSInteger *)malloc(sizeof(NSInteger) * (arrayCount+1)); if (lengths[i] == NULL) { @@ -89,8 +145,8 @@ - (NSIndexSet *)_asdk_commonIndexesWithArray:(NSArray *)array compareBlock:(BOOL } } } - - NSMutableIndexSet *common = [[NSMutableIndexSet alloc] init]; + // Backtrack to fill in indices based on length matrix: + NSMutableIndexSet *common = [NSMutableIndexSet indexSet]; NSInteger i = selfCount, j = arrayCount; while(i > 0 && j > 0) { if (comparison(self[i-1], array[j-1])) { @@ -110,4 +166,19 @@ - (NSIndexSet *)_asdk_commonIndexesWithArray:(NSArray *)array compareBlock:(BOOL return common; } +static compareBlock defaultCompare = nil; + ++ (compareBlock)defaultCompareBlock +{ + static dispatch_once_t onceToken; + + dispatch_once(&onceToken, ^{ + defaultCompare = [^BOOL(id lhs, id rhs) { + return [lhs isEqual:rhs]; + } copy]; + }); + + return defaultCompare; +} + @end diff --git a/Source/Layout/ASLayout+IGListKit.h b/Source/Layout/ASLayout+IGListKit.h new file mode 100644 index 000000000..178e79ca6 --- /dev/null +++ b/Source/Layout/ASLayout+IGListKit.h @@ -0,0 +1,15 @@ +// +// ASLayout+IGListKit.h +// AsyncDisplayKit +// +// Created by Kevin Smith on 7/1/18. +// Copyright © 2018 Pinterest. All rights reserved. +// + +#if AS_IG_LIST_KIT +#import +#import +@interface ASLayout(IGListKit) +@end + +#endif // AS_IG_LIST_KIT diff --git a/Source/Layout/ASLayout+IGListKit.mm b/Source/Layout/ASLayout+IGListKit.mm new file mode 100644 index 000000000..d69a343b5 --- /dev/null +++ b/Source/Layout/ASLayout+IGListKit.mm @@ -0,0 +1,31 @@ +// +// ASLayout+IGListKit.mm +// AsyncDisplayKit +// +// Created by Kevin Smith on 7/1/18. +// Copyright © 2018 Pinterest. All rights reserved. +// +#import +#if AS_IG_LIST_KIT +#import "ASLayout+IGListKit.h" +#import +#import +#import + +@implementation ASLayout(IGListKit) +- (nonnull id )diffIdentifier +{ + return @([self.layoutElement hash]); +} + +- (BOOL)isEqualToDiffableObject:(nullable id )other +{ + if (other == self) return YES; + + ASLayout *otherLayout = ASDynamicCast(other, ASLayout); + if (!otherLayout) return NO; + + return [otherLayout.layoutElement isEqual:self.layoutElement]; +} +@end +#endif // AS_IG_LIST_KIT diff --git a/Source/Private/ASLayoutTransition.h b/Source/Private/ASLayoutTransition.h index a34dd2e72..1060a57d8 100644 --- a/Source/Private/ASLayoutTransition.h +++ b/Source/Private/ASLayoutTransition.h @@ -85,9 +85,10 @@ AS_SUBCLASSING_RESTRICTED - (void)commitTransition; /** - * Insert all new subnodes that were added between the previous layout and the pending layout + * Insert all new subnodes that were added and move the subnodes that moved between the previous layout and + * the pending layout. */ -- (void)applySubnodeInsertions; +- (void)applySubnodeInsertionsAndMoves; /** * Remove all subnodes that are removed between the previous layout and the pending layout diff --git a/Source/Private/ASLayoutTransition.mm b/Source/Private/ASLayoutTransition.mm index f1b15dd02..da86fc280 100644 --- a/Source/Private/ASLayoutTransition.mm +++ b/Source/Private/ASLayoutTransition.mm @@ -26,10 +26,16 @@ #import #import +#import #import #import +#if AS_IG_LIST_KIT +#import +#import +#endif + /** * Search the whole layout stack if at least one layout has a layoutElement object that can not be layed out asynchronous. * This can be the case for example if a node was already loaded @@ -67,6 +73,7 @@ @implementation ASLayoutTransition { NSArray *_removedSubnodes; std::vector _insertedSubnodePositions; std::vector _removedSubnodePositions; + std::vector> _subnodeMoves; } - (instancetype)initWithNode:(ASDisplayNode *)node @@ -98,27 +105,44 @@ - (BOOL)isSynchronous - (void)commitTransition { - [self applySubnodeInsertions]; [self applySubnodeRemovals]; + [self applySubnodeInsertionsAndMoves]; } -- (void)applySubnodeInsertions +- (void)applySubnodeInsertionsAndMoves { ASDN::MutexSharedLocker l(__instanceLock__); [self calculateSubnodeOperationsIfNeeded]; // Create an activity even if no subnodes affected. - as_activity_create_for_scope("Apply subnode insertions"); - if (_insertedSubnodes.count == 0) { + as_activity_create_for_scope("Apply subnode insertions and moves"); + if (_insertedSubnodePositions.size() == 0 && _subnodeMoves.size() == 0) { return; } ASDisplayNodeLogEvent(_node, @"insertSubnodes: %@", _insertedSubnodes); NSUInteger i = 0; - for (ASDisplayNode *node in _insertedSubnodes) { + NSUInteger j = 0; + for (j = 0; j < _subnodeMoves.size(); ++j) { + [_subnodeMoves[j].first _removeFromSupernodeIfEqualTo:_node]; + } + j = 0; + while (i < _insertedSubnodePositions.size() && j < _subnodeMoves.size()) { NSUInteger p = _insertedSubnodePositions[i]; - [_node _insertSubnode:node atIndex:p]; - i += 1; + NSUInteger q = _subnodeMoves[j].second; + if (p < q) { + [_node _insertSubnode:_insertedSubnodes[i] atIndex:p]; + i++; + } else { + [_node _insertSubnode:_subnodeMoves[j].first atIndex:q]; + j++; + } + } + for (; i < _insertedSubnodePositions.size(); ++i) { + [_node _insertSubnode:_insertedSubnodes[i] atIndex:_insertedSubnodePositions[i]]; + } + for (; j < _subnodeMoves.size(); ++j) { + [_node _insertSubnode:_subnodeMoves[j].first atIndex:_subnodeMoves[j].second]; } } @@ -156,18 +180,40 @@ - (void)calculateSubnodeOperationsIfNeeded ASLayout *pendingLayout = _pendingLayout->layout; if (previousLayout) { +#if AS_IG_LIST_KIT + // IGListDiff completes in linear time O(m+n), so use it if we have it: + IGListIndexSetResult *result = IGListDiff(previousLayout.sublayouts, pendingLayout.sublayouts, IGListDiffEquality); + _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, result.inserts, &_insertedSubnodes); + _removedSubnodePositions = findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes); + for (IGListMoveIndex *move in result.moves) { + id subnode = previousLayout.sublayouts[static_cast(move.from)].layoutElement; + _subnodeMoves.push_back(std::pair(subnode, move.to)); + } + + // Sort by ascending order of move destinations, this will allow easy loop of `insertSubnode:AtIndex` later. + std::sort(_subnodeMoves.begin(), _subnodeMoves.end(), [](std::pair a, + std::pair b) { + return a.second < b.second; + }); +#else NSIndexSet *insertions, *deletions; + NSArray *moves; [previousLayout.sublayouts asdk_diffWithArray:pendingLayout.sublayouts insertions:&insertions deletions:&deletions - compareBlock:^BOOL(ASLayout *lhs, ASLayout *rhs) { - return ASObjectIsEqual(lhs.layoutElement, rhs.layoutElement); - }]; + moves:&moves]; + _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, insertions, &_insertedSubnodes); _removedSubnodePositions = findNodesInLayoutAtIndexesWithFilteredNodes(previousLayout, deletions, _insertedSubnodes, &_removedSubnodes); + // These should arrive sorted in ascending order of move destinations. + for (NSIndexPath *move in moves) { + id subnode = previousLayout.sublayouts[static_cast([move indexAtPosition:0])].layoutElement; + _subnodeMoves.push_back(std::pair(subnode, [move indexAtPosition:1])); + } +#endif } else { NSIndexSet *indexes = [NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [pendingLayout.sublayouts count])]; _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, indexes, &_insertedSubnodes); @@ -254,7 +300,7 @@ - (ASSizeRange)transitionContext:(_ASTransitionContext *)context constrainedSize for (ASLayout *sublayout in layout.sublayouts) { if (idx > lastIndex) { break; } if (idx >= firstIndex && [indexes containsIndex:idx]) { - ASDisplayNode *node = (ASDisplayNode *)sublayout.layoutElement; + ASDisplayNode *node = ASDynamicCast(sublayout.layoutElement, ASDisplayNode); ASDisplayNodeCAssert(node, @"ASDisplayNode was deallocated before it was added to a subnode. It's likely the case that you use automatically manages subnodes and allocate a ASDisplayNode in layoutSpecThatFits: and don't have any strong reference to it."); // Ignore the odd case in which a non-node sublayout is accessed and the type cast fails if (node != nil) { diff --git a/Tests/ASDisplayNodeImplicitHierarchyTests.m b/Tests/ASDisplayNodeImplicitHierarchyTests.m index 383d5cfd9..5d305ceb8 100644 --- a/Tests/ASDisplayNodeImplicitHierarchyTests.m +++ b/Tests/ASDisplayNodeImplicitHierarchyTests.m @@ -150,7 +150,11 @@ - (void)testCalculatedLayoutHierarchyTransitions ASDisplayNode *node1 = [[ASDisplayNode alloc] init]; ASDisplayNode *node2 = [[ASDisplayNode alloc] init]; ASDisplayNode *node3 = [[ASDisplayNode alloc] init]; - + + node1.debugName = @"a"; + node2.debugName = @"b"; + node3.debugName = @"c"; + // As we will involve a stack spec we have to give the nodes an intrinsic content size node1.style.preferredSize = kSize; node2.style.preferredSize = kSize; diff --git a/Tests/ASDisplayNodeTests.mm b/Tests/ASDisplayNodeTests.mm index 989997007..6a3385c94 100644 --- a/Tests/ASDisplayNodeTests.mm +++ b/Tests/ASDisplayNodeTests.mm @@ -2402,14 +2402,26 @@ - (void)testThatStackSpecOrdersSubnodesCorrectly DeclareNodeNamed(b); DeclareNodeNamed(c); DeclareNodeNamed(d); + DeclareNodeNamed(e); NSArray *nodesForwardOrder = @[a, b, c, d]; NSArray *nodesReverseOrder = @[d, c, b, a]; + NSArray *addAndMoveOrder = @[a, b, e, d, c]; __block BOOL flipItemOrder = NO; + __block NSUInteger testCount = 0; node.layoutSpecBlock = ^(ASDisplayNode *node, ASSizeRange size) { ASStackLayoutSpec *stack = [ASStackLayoutSpec verticalStackLayoutSpec]; - stack.children = flipItemOrder ? nodesReverseOrder : nodesForwardOrder; + switch(testCount) { + case 0: + stack.children = nodesForwardOrder; break; + case 1: + stack.children = nodesReverseOrder; break; + case 2: + default: + stack.children = addAndMoveOrder; break; + } + testCount++; return stack; }; @@ -2423,13 +2435,22 @@ - (void)testThatStackSpecOrdersSubnodesCorrectly flipItemOrder = YES; [node invalidateCalculatedLayout]; + [node.view setNeedsLayout]; [node.view layoutIfNeeded]; // In this case, it's critical that the items are in the new order so that event handling and apparent z-position are correct. // FIXME: The reversal case is not currently passing. - // XCTAssert([node.subnodes isEqualToArray:nodesReverseOrder], @"subnodes: %@, array: %@", node.subnodes, nodesReverseOrder); - // XCTAssertNodeSubnodeSubviewSublayerOrder(node, YES /* isLoaded */, NO /* isLayerBacked */, - // @"d,c,b,a", @"Reverse order"); + XCTAssert([node.subnodes isEqualToArray:nodesReverseOrder], @"subnodes: %@, array: %@", node.subnodes, nodesReverseOrder); + XCTAssertNodeSubnodeSubviewSublayerOrder(node, YES /* isLoaded */, NO /* isLayerBacked */, + @"d,c,b,a", @"Reverse order"); + + [node invalidateCalculatedLayout]; + [node.view setNeedsLayout]; + [node.view layoutIfNeeded]; + XCTAssert([node.subnodes isEqualToArray:addAndMoveOrder], @"subnodes: %@, array: %@", node.subnodes, addAndMoveOrder); + XCTAssertNodeSubnodeSubviewSublayerOrder(node, YES /* isLoaded */, NO /* isLayerBacked */, + @"a,b,e,d,c", @"AddAndMove order"); + } - (void)testThatOverlaySpecOrdersSubnodesCorrectly diff --git a/Tests/ArrayDiffingTests.m b/Tests/ArrayDiffingTests.m index 5d25bfde7..f13788c61 100644 --- a/Tests/ArrayDiffingTests.m +++ b/Tests/ArrayDiffingTests.m @@ -127,7 +127,8 @@ - (void)testDiffingInsertionsAndDeletions { @[@0, @1, @2], ], ]; - + + long n = 0; for (NSArray *test in tests) { NSIndexSet *insertions, *deletions; [test[0] asdk_diffWithArray:test[1] insertions:&insertions deletions:&deletions]; @@ -135,17 +136,186 @@ - (void)testDiffingInsertionsAndDeletions { NSMutableIndexSet *mutableDeletions = [deletions mutableCopy]; for (NSNumber *index in (NSArray *)test[2]) { - XCTAssert([mutableInsertions containsIndex:[index integerValue]]); + XCTAssert([mutableInsertions containsIndex:[index integerValue]], @"Test #%ld: insertions %@ does not contain %@", + n, insertions, index); [mutableInsertions removeIndex:[index integerValue]]; } for (NSNumber *index in (NSArray *)test[3]) { - XCTAssert([mutableDeletions containsIndex:[index integerValue]]); + XCTAssert([mutableDeletions containsIndex:[index integerValue]], @"Test #%ld: deletions %@ does not contain %@", + n, deletions, index + ); [mutableDeletions removeIndex:[index integerValue]]; } - XCTAssert([mutableInsertions count] == 0, @"Unaccounted insertions: %@", mutableInsertions); - XCTAssert([mutableDeletions count] == 0, @"Unaccounted deletions: %@", mutableDeletions); + XCTAssert([mutableInsertions count] == 0, @"Test #%ld: Unaccounted insertions: %@", n, mutableInsertions); + XCTAssert([mutableDeletions count] == 0, @"Test #%ld: Unaccounted deletions: %@", n, mutableDeletions); + n++; + } +} + +- (void)testDiffingInsertsDeletesAndMoves +{ + NSArray *tests = @[ + @[ + @[@"a", @"b"], + @[@"b", @"a"], + @[], + @[], + @[[NSIndexPath indexPathWithIndexes:(NSUInteger[]) {1, 0} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]) {0, 1} length:2] + ]], + @[ + @[@"bob", @"alice", @"dave"], + @[@"bob", @"alice", @"dave", @"gary"], + @[@3], + @[], + @[]], + @[ + @[@"a", @"b", @"c", @"d"], + @[@"d", @"c", @"b", @"a"], + @[], + @[], + @[[NSIndexPath indexPathWithIndexes:(NSUInteger[]){3, 0} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){2, 1} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){1, 2} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){0, 3} length:2] + ]], + @[ + @[@"bob", @"alice", @"dave"], + @[@"bob", @"gary", @"dave", @"alice"], + @[@1], + @[], + @[[NSIndexPath indexPathWithIndexes:(NSUInteger[]) {1, 3} length:2] + ]], + @[ + @[@"bob", @"alice", @"dave"], + @[@"bob", @"alice"], + @[], + @[@2], + @[]], + @[ + @[@"bob", @"alice", @"dave"], + @[], + @[], + @[@0, @1, @2], + @[]], + @[ + @[@"bob", @"alice", @"dave"], + @[@"gary", @"alice", @"dave", @"jack"], + @[@0, @3], + @[@0], + @[]], + @[ + @[@"bob", @"alice", @"dave", @"judy", @"lynda", @"tony"], + @[@"gary", @"bob", @"suzy", @"tony"], + @[@0, @2], + @[@1, @2, @3, @4], + @[[NSIndexPath indexPathWithIndexes:(NSUInteger[]){0, 1} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){5, 3} length:2] + ]], + @[ + @[@"bob", @"alice", @"dave", @"judy"], + @[@"judy", @"dave", @"alice", @"bob"], + @[], + @[], + @[[NSIndexPath indexPathWithIndexes:(NSUInteger[]){3, 0} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){2, 1} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){1, 2} length:2], + [NSIndexPath indexPathWithIndexes:(NSUInteger[]){0, 3} length:2] + ]] + + ]; + + long n = 0; + for (NSArray *test in tests) { + NSIndexSet *insertions, *deletions; + NSArray *moves; + [test[0] asdk_diffWithArray:test[1] insertions:&insertions deletions:&deletions moves:&moves]; + NSMutableIndexSet *mutableInsertions = [insertions mutableCopy]; + NSMutableIndexSet *mutableDeletions = [deletions mutableCopy]; + + for (NSNumber *index in (NSArray *) test[2]) { + XCTAssert([mutableInsertions containsIndex:[index integerValue]], @"Test #%ld, insertions does not contain %ld", + n, (long)[index integerValue]); + [mutableInsertions removeIndex:(NSUInteger) [index integerValue]]; + } + for (NSNumber *index in (NSArray *) test[3]) { + XCTAssert([mutableDeletions containsIndex:[index integerValue]], @"Test #%ld, deletions does not contain %ld", + n, (long)[index integerValue]); + [mutableDeletions removeIndex:(NSUInteger) [index integerValue]]; + } + + XCTAssert([mutableInsertions count] == 0, @"Test #%ld, Unaccounted insertions: %@", n, mutableInsertions); + XCTAssert([mutableDeletions count] == 0, @"Test #%ld, Unaccounted deletions: %@", n, mutableDeletions); + + XCTAssert([moves isEqual:test[4]], @"Test #%ld, %@ !isEqual: %@", n, moves, test[4]); + n++; } } +- (void)testArrayDiffingRebuildingWithRandomElements +{ + NSArray *original = @[]; + NSArray *pending = @[]; + + NSIndexSet *insertions = nil; + NSIndexSet *deletions = nil; + NSArray *moves; + + for (int testNumber = 0; testNumber <= 25; testNumber++) { + int len = arc4random_uniform(10); + for (int j = 0; j < len; j++) { + original = [original arrayByAddingObject:@(arc4random_uniform(25))]; + } + len = arc4random_uniform(10); + for (int j = 0; j < len; j++) { + pending = [pending arrayByAddingObject:@(arc4random_uniform(25))]; + } + // Some sequences that presented issues in the past: + if (testNumber == 0) { + original = [@[@20, @11, @14, @2, @14, @5, @4, @18, @0] mutableCopy]; + pending = @[@9, @18, @18, @19, @20, @18, @22, @10, @3]; + } + if (testNumber == 1) { + original = [@[@5, @9, @21, @11, @5, @9, @8] mutableCopy]; + pending = @[@2, @12, @17, @19, @9, @1, @8, @5, @21]; + } + if (testNumber == 2) { + original = [@[@14, @14, @12, @8, @20, @4, @0, @10] mutableCopy]; + pending = @[@14]; + } + + [original asdk_diffWithArray:pending insertions:&insertions deletions:&deletions moves:&moves]; + + NSMutableArray *deletionsList = [NSMutableArray new]; + [deletions enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL *stop) { + [deletionsList addObject:@(idx)]; + }]; + NSMutableArray *insertionsList = [NSMutableArray new]; + [insertions enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL *stop) { + [insertionsList addObject:@(idx)]; + }]; + NSMutableArray *movesList = [moves mutableCopy]; + + NSUInteger i = 0; + NSUInteger j = 0; + NSMutableArray *test = [NSMutableArray new]; + for (NSUInteger count = 0; count < [pending count]; count++) { + if (i < [insertionsList count] && [insertionsList[i] unsignedIntegerValue] == count) { + [test addObject:pending[[insertionsList[i] unsignedIntegerValue]]]; + i++; + } else if (j < [movesList count] && [movesList[j] indexAtPosition:1] == count) { + [test addObject:original[[movesList[j] indexAtPosition:0]]]; + j++; + } else { + [test addObject:original[count]]; + } + } + + XCTAssert([test isEqualToArray:pending], @"Did not mutate to expected new array: %@, insertions: %@, moves: %@, deletions: %@", + [test componentsJoinedByString:@","], insertionsList, movesList, deletionsList); + original = @[]; + pending = @[]; + } +} @end From f189d1bc90f13313f5ca82c62b6d6d2ee6eba572 Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 3 Jul 2018 14:20:59 -0700 Subject: [PATCH 02/15] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e47a52ecc..0a896aaa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## master * Add your own contributions to the next release on the line below this with your name. +- [ASLayoutTransition] Add support for preserving order after node moves during transitions. (This order defines the z-order as well.) [Kevin Smith](https://github.com/wiseoldduck) - [ASDisplayNode] Adds support for multiple interface state delegates. [Garrett Moon](https://github.com/garrettmoon) [#979](https://github.com/TextureGroup/Texture/pull/979) - [ASDataController] Add capability to renew supplementary views (update map) when size change from zero to non-zero.[Max Wang](https://github.com/wsdwsd0829) [#842](https://github.com/TextureGroup/Texture/pull/842) - Make `ASPerformMainThreadDeallocation` visible in C. [Adlai Holler](https://github.com/Adlai-Holler) From 09d43aed4d61eb5d843d7828bde34f8887003894 Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 3 Jul 2018 14:24:37 -0700 Subject: [PATCH 03/15] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a896aaa8..b8b779814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## master * Add your own contributions to the next release on the line below this with your name. -- [ASLayoutTransition] Add support for preserving order after node moves during transitions. (This order defines the z-order as well.) [Kevin Smith](https://github.com/wiseoldduck) +- [ASLayoutTransition] Add support for preserving order after node moves during transitions. (This order defines the z-order as well.) [Kevin Smith](https://github.com/wiseoldduck) [#1006] - [ASDisplayNode] Adds support for multiple interface state delegates. [Garrett Moon](https://github.com/garrettmoon) [#979](https://github.com/TextureGroup/Texture/pull/979) - [ASDataController] Add capability to renew supplementary views (update map) when size change from zero to non-zero.[Max Wang](https://github.com/wsdwsd0829) [#842](https://github.com/TextureGroup/Texture/pull/842) - Make `ASPerformMainThreadDeallocation` visible in C. [Adlai Holler](https://github.com/Adlai-Holler) From baba616ddc4ab418d994230b3d815787815ee8c0 Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 3 Jul 2018 14:27:27 -0700 Subject: [PATCH 04/15] Update ASLayout+IGListKit.h --- Source/Layout/ASLayout+IGListKit.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Source/Layout/ASLayout+IGListKit.h b/Source/Layout/ASLayout+IGListKit.h index 178e79ca6..65c45a029 100644 --- a/Source/Layout/ASLayout+IGListKit.h +++ b/Source/Layout/ASLayout+IGListKit.h @@ -1,9 +1,13 @@ // -// ASLayout+IGListKit.h -// AsyncDisplayKit +// ASLayout+IGListKit.mm +// Texture // -// Created by Kevin Smith on 7/1/18. -// Copyright © 2018 Pinterest. All rights reserved. +// Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 // #if AS_IG_LIST_KIT From beb204eed3ffb5d1f3ad5ee3d7085008ed055508 Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 3 Jul 2018 14:29:02 -0700 Subject: [PATCH 05/15] Update ASLayout+IGListKit.mm --- Source/Layout/ASLayout+IGListKit.mm | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Source/Layout/ASLayout+IGListKit.mm b/Source/Layout/ASLayout+IGListKit.mm index d69a343b5..477d30e73 100644 --- a/Source/Layout/ASLayout+IGListKit.mm +++ b/Source/Layout/ASLayout+IGListKit.mm @@ -1,9 +1,13 @@ // // ASLayout+IGListKit.mm -// AsyncDisplayKit +// Texture // -// Created by Kevin Smith on 7/1/18. -// Copyright © 2018 Pinterest. All rights reserved. +// Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 // #import #if AS_IG_LIST_KIT From d4ce1bcdec0904e0f14d8baeb642f339e6dcadb4 Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Fri, 6 Jul 2018 12:54:03 -0700 Subject: [PATCH 06/15] Use std collections to avoid NSNumber boxing --- AsyncDisplayKit.xcodeproj/project.pbxproj | 8 ++--- .../{NSArray+Diffing.m => NSArray+Diffing.mm} | 30 ++++++++++--------- 2 files changed, 20 insertions(+), 18 deletions(-) rename Source/Details/{NSArray+Diffing.m => NSArray+Diffing.mm} (88%) diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index d01f0a005..4ce8706e6 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -110,7 +110,7 @@ 509E68641B3AEDB7009B9150 /* ASCollectionViewLayoutController.m in Sources */ = {isa = PBXBuildFile; fileRef = 205F0E1C1B373A2C007741D0 /* ASCollectionViewLayoutController.m */; }; 509E68651B3AEDC5009B9150 /* CoreGraphics+ASConvenience.h in Headers */ = {isa = PBXBuildFile; fileRef = 205F0E1F1B376416007741D0 /* CoreGraphics+ASConvenience.h */; settings = {ATTRIBUTES = (Public, ); }; }; 509E68661B3AEDD7009B9150 /* CoreGraphics+ASConvenience.m in Sources */ = {isa = PBXBuildFile; fileRef = 205F0E201B376416007741D0 /* CoreGraphics+ASConvenience.m */; }; - 636EA1A41C7FF4EC00EE152F /* NSArray+Diffing.m in Sources */ = {isa = PBXBuildFile; fileRef = DBC452DA1C5BF64600B16017 /* NSArray+Diffing.m */; }; + 636EA1A41C7FF4EC00EE152F /* NSArray+Diffing.mm in Sources */ = {isa = PBXBuildFile; fileRef = DBC452DA1C5BF64600B16017 /* NSArray+Diffing.mm */; }; 636EA1A51C7FF4EF00EE152F /* ASDefaultPlayButton.m in Sources */ = {isa = PBXBuildFile; fileRef = AEB7B0191C5962EA00662EF4 /* ASDefaultPlayButton.m */; }; 680346941CE4052A0009FEB4 /* ASNavigationController.h in Headers */ = {isa = PBXBuildFile; fileRef = 68FC85DC1CE29AB700EDD713 /* ASNavigationController.h */; settings = {ATTRIBUTES = (Public, ); }; }; 68355B341CB579B9001D4E68 /* ASImageNode+AnimatedImage.mm in Sources */ = {isa = PBXBuildFile; fileRef = 68355B2E1CB5799E001D4E68 /* ASImageNode+AnimatedImage.mm */; }; @@ -968,7 +968,7 @@ DB55C2601C6408D6004EDCF5 /* _ASTransitionContext.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = _ASTransitionContext.m; path = ../_ASTransitionContext.m; sourceTree = ""; }; DB55C2651C641AE4004EDCF5 /* ASContextTransitioning.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASContextTransitioning.h; sourceTree = ""; }; DBC452D91C5BF64600B16017 /* NSArray+Diffing.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "NSArray+Diffing.h"; sourceTree = ""; }; - DBC452DA1C5BF64600B16017 /* NSArray+Diffing.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSArray+Diffing.m"; sourceTree = ""; }; + DBC452DA1C5BF64600B16017 /* NSArray+Diffing.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = "NSArray+Diffing.mm"; sourceTree = ""; }; DBC452DD1C5C6A6A00B16017 /* ArrayDiffingTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; lineEnding = 0; path = ArrayDiffingTests.m; sourceTree = ""; xcLanguageSpecificationIdentifier = xcode.lang.objc; }; DBC453211C5FD97200B16017 /* ASDisplayNodeImplicitHierarchyTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; lineEnding = 0; path = ASDisplayNodeImplicitHierarchyTests.m; sourceTree = ""; xcLanguageSpecificationIdentifier = xcode.lang.objc; }; DBDB83921C6E879900D0098C /* ASPagerFlowLayout.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASPagerFlowLayout.h; sourceTree = ""; }; @@ -1420,7 +1420,7 @@ 205F0E1F1B376416007741D0 /* CoreGraphics+ASConvenience.h */, 205F0E201B376416007741D0 /* CoreGraphics+ASConvenience.m */, DBC452D91C5BF64600B16017 /* NSArray+Diffing.h */, - DBC452DA1C5BF64600B16017 /* NSArray+Diffing.m */, + DBC452DA1C5BF64600B16017 /* NSArray+Diffing.mm */, CC4981BA1D1C7F65004E13CC /* NSIndexSet+ASHelpers.h */, CC4981BB1D1C7F65004E13CC /* NSIndexSet+ASHelpers.m */, 058D09F5195D050800B7D73C /* NSMutableAttributedString+TextKitAdditions.h */, @@ -2410,7 +2410,7 @@ B350624E1B010EFD0018CF92 /* ASDisplayNode+AsyncDisplay.mm in Sources */, E5667E8E1F33872700FA6FC0 /* _ASCollectionGalleryLayoutInfo.m in Sources */, 25E327591C16819500A2170C /* ASPagerNode.m in Sources */, - 636EA1A41C7FF4EC00EE152F /* NSArray+Diffing.m in Sources */, + 636EA1A41C7FF4EC00EE152F /* NSArray+Diffing.mm in Sources */, B35062501B010EFD0018CF92 /* ASDisplayNode+DebugTiming.mm in Sources */, DEC146B91C37A16A004A0EE7 /* ASCollectionInternal.m in Sources */, 254C6B891BF94F8A003EC431 /* ASTextKitRenderer+Positioning.mm in Sources */, diff --git a/Source/Details/NSArray+Diffing.m b/Source/Details/NSArray+Diffing.mm similarity index 88% rename from Source/Details/NSArray+Diffing.m rename to Source/Details/NSArray+Diffing.mm index d8714f453..05767fe8d 100644 --- a/Source/Details/NSArray+Diffing.m +++ b/Source/Details/NSArray+Diffing.mm @@ -16,6 +16,8 @@ // #import +#import +#import #import @implementation NSArray (Diffing) @@ -43,13 +45,14 @@ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions moves:(NSArray **)moves compareBlock:(compareBlock)comparison { + typedef std::unordered_map> move_map; NSAssert(comparison != nil, @"Comparison block is required"); - NSAssert(moves == nil || comparison == [NSArray defaultCompareBlock], @"move detection requires isEqual: and hash (no custom compare)"); - NSMutableDictionary *> *potentialMoves = nil; + NSAssert(moves == nil || comparison == [NSArray defaultCompareBlock], @"move detection requires isEqual: and hash (no custom compare)"); + std::unique_ptr potentialMoves(nullptr); NSMutableArray *moveIndexPaths = nil; NSMutableIndexSet *insertionIndexes = nil, *deletionIndexes = nil; if (moves) { - potentialMoves = [NSMutableDictionary new]; + potentialMoves = std::unique_ptr(new move_map()); moveIndexPaths = [NSMutableArray new]; } NSMutableIndexSet *commonIndexes = [[self _asdk_commonIndexesWithArray:array compareBlock:comparison] mutableCopy]; @@ -60,12 +63,11 @@ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions if (![commonIndexes containsIndex:i]) { [deletionIndexes addIndex:i]; } - NSNumber *hash = @([self[i] hash]); - if (!potentialMoves[hash]) { - potentialMoves[hash] = [@[@(i)] mutableCopy]; - } else { - [potentialMoves[hash] addObject:@(i)]; + NSUInteger hash = [self[i] hash]; + if (potentialMoves) { + (*potentialMoves)[hash].push(i); } + } } @@ -75,13 +77,13 @@ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions BOOL moveFound; NSUInteger movedFrom = NSNotFound; for (NSUInteger i = 0, j = 0; j < array.count; j++) { - NSNumber *hash = @([array[j] hash]); - moveFound = (potentialMoves[hash] != nil); + NSUInteger hash = [array[j] hash]; + moveFound = (potentialMoves && potentialMoves->count(hash)); if (moveFound) { - movedFrom = [[potentialMoves[hash] firstObject] unsignedIntegerValue]; - [potentialMoves[hash] removeObjectAtIndex:0]; - if (![potentialMoves[hash] count]) { - potentialMoves[hash] = nil; + movedFrom = (*potentialMoves)[hash].front(); + (*potentialMoves)[hash].pop(); + if ((*potentialMoves)[hash].empty()) { + potentialMoves->erase(hash); } if (movedFrom != j) { NSUInteger indexes[] = {movedFrom, j}; From 18bf81a43c4db8cacf1bcc9c47749044b68c3b6a Mon Sep 17 00:00:00 2001 From: Kevin Date: Sat, 7 Jul 2018 01:05:31 -0700 Subject: [PATCH 07/15] Update ASLayoutTransition.mm --- Source/Private/ASLayoutTransition.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Private/ASLayoutTransition.mm b/Source/Private/ASLayoutTransition.mm index da86fc280..567a492c8 100644 --- a/Source/Private/ASLayoutTransition.mm +++ b/Source/Private/ASLayoutTransition.mm @@ -33,7 +33,7 @@ #if AS_IG_LIST_KIT #import -#import +#import #endif /** From 22d24b5acd20d3abd3cf8a45ea9d82a782f2306c Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Mon, 9 Jul 2018 18:07:47 -0700 Subject: [PATCH 08/15] Code review updates. * Use `unordered_multimap` on stack instead of unordered_map on heap * Remove notFound BOOL (use NSNotFound sentinel value) and put some vars inside the if (insertions/moves) loop * Don't copy defaultCompare block (redundant under ARC) * Whitespace * Remove unneeded mutableCopy-s in ArrayDiffingTests --- Source/Details/NSArray+Diffing.mm | 61 +++++++++++++++---------------- Tests/ArrayDiffingTests.m | 16 ++++---- 2 files changed, 38 insertions(+), 39 deletions(-) diff --git a/Source/Details/NSArray+Diffing.mm b/Source/Details/NSArray+Diffing.mm index 05767fe8d..295943e0f 100644 --- a/Source/Details/NSArray+Diffing.mm +++ b/Source/Details/NSArray+Diffing.mm @@ -16,9 +16,9 @@ // #import -#import -#import +#import #import +#import @implementation NSArray (Diffing) @@ -45,55 +45,54 @@ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions moves:(NSArray **)moves compareBlock:(compareBlock)comparison { - typedef std::unordered_map> move_map; + struct NSObjectHash + { + std::size_t operator()(id k) const { return (std::size_t) [k hash]; }; + }; + struct NSObjectCompare + { + bool operator()(id lhs, id rhs) const { return (bool) [lhs isEqual:rhs]; }; + }; + std::unordered_multimap potentialMoves; + NSAssert(comparison != nil, @"Comparison block is required"); - NSAssert(moves == nil || comparison == [NSArray defaultCompareBlock], @"move detection requires isEqual: and hash (no custom compare)"); - std::unique_ptr potentialMoves(nullptr); + NSAssert(moves == nil || comparison == [NSArray defaultCompareBlock], @"move detection requires isEqual: and hash (no custom compare)"); NSMutableArray *moveIndexPaths = nil; NSMutableIndexSet *insertionIndexes = nil, *deletionIndexes = nil; if (moves) { - potentialMoves = std::unique_ptr(new move_map()); moveIndexPaths = [NSMutableArray new]; } - NSMutableIndexSet *commonIndexes = [[self _asdk_commonIndexesWithArray:array compareBlock:comparison] mutableCopy]; + NSMutableIndexSet *commonIndexes = [self _asdk_commonIndexesWithArray:array compareBlock:comparison]; if (deletions || moves) { deletionIndexes = [NSMutableIndexSet indexSet]; - for (NSUInteger i = 0; i < self.count; i++) { + NSUInteger i = 0; + for (id element in self) { if (![commonIndexes containsIndex:i]) { [deletionIndexes addIndex:i]; } - NSUInteger hash = [self[i] hash]; - if (potentialMoves) { - (*potentialMoves)[hash].push(i); + if (moves) { + potentialMoves.insert(std::pair(element, i)); } - + ++i; } } if (insertions || moves) { insertionIndexes = [NSMutableIndexSet indexSet]; NSArray *commonObjects = [self objectsAtIndexes:commonIndexes]; - BOOL moveFound; - NSUInteger movedFrom = NSNotFound; for (NSUInteger i = 0, j = 0; j < array.count; j++) { - NSUInteger hash = [array[j] hash]; - moveFound = (potentialMoves && potentialMoves->count(hash)); - if (moveFound) { - movedFrom = (*potentialMoves)[hash].front(); - (*potentialMoves)[hash].pop(); - if ((*potentialMoves)[hash].empty()) { - potentialMoves->erase(hash); - } - if (movedFrom != j) { - NSUInteger indexes[] = {movedFrom, j}; - [moveIndexPaths addObject:[NSIndexPath indexPathWithIndexes:indexes length:2]]; - } + auto moveFound = potentialMoves.find(array[j]); + NSUInteger movedFrom = NSNotFound; + if (moveFound != potentialMoves.end() && moveFound->second != j) { + movedFrom = moveFound->second; + potentialMoves.erase(moveFound); + [moveIndexPaths addObject:[NSIndexPath indexPathForItem:j inSection:movedFrom]]; } - if (i < commonObjects.count && j < array.count && comparison(commonObjects[i], array[j])) { + if (i < commonObjects.count && j < array.count && comparison(commonObjects[i], array[j])) { i++; } else { - if (moveFound) { + if (movedFrom != NSNotFound) { // moves will coalesce a delete / insert - the insert is just not done, and here we remove the delete: [deletionIndexes removeIndex:movedFrom]; // OR a move will have come from the LCS: @@ -115,7 +114,7 @@ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions // https://github.com/raywenderlich/swift-algorithm-club/tree/master/Longest%20Common%20Subsequence is not exactly this code (obviously), but // is a good commentary on the algorithm. -- (NSIndexSet *)_asdk_commonIndexesWithArray:(NSArray *)array compareBlock:(BOOL (^)(id lhs, id rhs))comparison +- (NSMutableIndexSet *)_asdk_commonIndexesWithArray:(NSArray *)array compareBlock:(BOOL (^)(id lhs, id rhs))comparison { NSAssert(comparison != nil, @"Comparison block is required"); @@ -175,9 +174,9 @@ + (compareBlock)defaultCompareBlock static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - defaultCompare = [^BOOL(id lhs, id rhs) { + defaultCompare = ^BOOL(id lhs, id rhs) { return [lhs isEqual:rhs]; - } copy]; + }; }); return defaultCompare; diff --git a/Tests/ArrayDiffingTests.m b/Tests/ArrayDiffingTests.m index f13788c61..b9af5681a 100644 --- a/Tests/ArrayDiffingTests.m +++ b/Tests/ArrayDiffingTests.m @@ -273,15 +273,15 @@ - (void)testArrayDiffingRebuildingWithRandomElements } // Some sequences that presented issues in the past: if (testNumber == 0) { - original = [@[@20, @11, @14, @2, @14, @5, @4, @18, @0] mutableCopy]; + original = @[@20, @11, @14, @2, @14, @5, @4, @18, @0]; pending = @[@9, @18, @18, @19, @20, @18, @22, @10, @3]; } if (testNumber == 1) { - original = [@[@5, @9, @21, @11, @5, @9, @8] mutableCopy]; + original = @[@5, @9, @21, @11, @5, @9, @8]; pending = @[@2, @12, @17, @19, @9, @1, @8, @5, @21]; } if (testNumber == 2) { - original = [@[@14, @14, @12, @8, @20, @4, @0, @10] mutableCopy]; + original = @[@14, @14, @12, @8, @20, @4, @0, @10]; pending = @[@14]; } @@ -295,7 +295,6 @@ - (void)testArrayDiffingRebuildingWithRandomElements [insertions enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL *stop) { [insertionsList addObject:@(idx)]; }]; - NSMutableArray *movesList = [moves mutableCopy]; NSUInteger i = 0; NSUInteger j = 0; @@ -304,16 +303,17 @@ - (void)testArrayDiffingRebuildingWithRandomElements if (i < [insertionsList count] && [insertionsList[i] unsignedIntegerValue] == count) { [test addObject:pending[[insertionsList[i] unsignedIntegerValue]]]; i++; - } else if (j < [movesList count] && [movesList[j] indexAtPosition:1] == count) { - [test addObject:original[[movesList[j] indexAtPosition:0]]]; + } else if (j < [moves count] && [moves[j] indexAtPosition:1] == count) { + [test addObject:original[[moves[j] indexAtPosition:0]]]; j++; } else { [test addObject:original[count]]; } } - XCTAssert([test isEqualToArray:pending], @"Did not mutate to expected new array: %@, insertions: %@, moves: %@, deletions: %@", - [test componentsJoinedByString:@","], insertionsList, movesList, deletionsList); + XCTAssert([test isEqualToArray:pending], @"Did not mutate to expected new array:\n [%@] -> [%@], actual: [%@]\ninsertions: %@\nmoves: %@\ndeletions: %@", + [original componentsJoinedByString:@","], [pending componentsJoinedByString:@","], [test componentsJoinedByString:@","], + insertions, moves, deletions); original = @[]; pending = @[]; } From 9898f0eaa3fca357d03c37e1ad44a8fa1b403eba Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Mon, 9 Jul 2018 18:50:21 -0700 Subject: [PATCH 09/15] Code review updates. * Type _subnodeMoves pair.first to ASDisplayNode * instead of id * C++ enumeration * unowned refs for adding previousLayout nodes to _subnodeMoves * Remove unreleated ASDynamicCast that is probably right though --- Source/Private/ASLayoutTransition.mm | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Source/Private/ASLayoutTransition.mm b/Source/Private/ASLayoutTransition.mm index 567a492c8..ea5e9bce5 100644 --- a/Source/Private/ASLayoutTransition.mm +++ b/Source/Private/ASLayoutTransition.mm @@ -73,7 +73,7 @@ @implementation ASLayoutTransition { NSArray *_removedSubnodes; std::vector _insertedSubnodePositions; std::vector _removedSubnodePositions; - std::vector> _subnodeMoves; + std::vector> _subnodeMoves; } - (instancetype)initWithNode:(ASDisplayNode *)node @@ -123,8 +123,8 @@ - (void)applySubnodeInsertionsAndMoves ASDisplayNodeLogEvent(_node, @"insertSubnodes: %@", _insertedSubnodes); NSUInteger i = 0; NSUInteger j = 0; - for (j = 0; j < _subnodeMoves.size(); ++j) { - [_subnodeMoves[j].first _removeFromSupernodeIfEqualTo:_node]; + for (auto const &move : _subnodeMoves) { + [move.first _removeFromSupernodeIfEqualTo:_node]; } j = 0; while (i < _insertedSubnodePositions.size() && j < _subnodeMoves.size()) { @@ -186,13 +186,13 @@ - (void)calculateSubnodeOperationsIfNeeded _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, result.inserts, &_insertedSubnodes); _removedSubnodePositions = findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes); for (IGListMoveIndex *move in result.moves) { - id subnode = previousLayout.sublayouts[static_cast(move.from)].layoutElement; - _subnodeMoves.push_back(std::pair(subnode, move.to)); + unowned ASDisplayNode *subnode = previousLayout.sublayouts[move.from].layoutElement; + _subnodeMoves.push_back(std::pair(subnode, move.to)); } // Sort by ascending order of move destinations, this will allow easy loop of `insertSubnode:AtIndex` later. - std::sort(_subnodeMoves.begin(), _subnodeMoves.end(), [](std::pair a, - std::pair b) { + std::sort(_subnodeMoves.begin(), _subnodeMoves.end(), [](std::pair, NSUInteger> a, + std::pair b) { return a.second < b.second; }); #else @@ -210,7 +210,7 @@ - (void)calculateSubnodeOperationsIfNeeded &_removedSubnodes); // These should arrive sorted in ascending order of move destinations. for (NSIndexPath *move in moves) { - id subnode = previousLayout.sublayouts[static_cast([move indexAtPosition:0])].layoutElement; + unowned ASDisplayNode *subnode = previousLayout.sublayouts[([move indexAtPosition:0])].layoutElement; _subnodeMoves.push_back(std::pair(subnode, [move indexAtPosition:1])); } #endif @@ -300,7 +300,7 @@ - (ASSizeRange)transitionContext:(_ASTransitionContext *)context constrainedSize for (ASLayout *sublayout in layout.sublayouts) { if (idx > lastIndex) { break; } if (idx >= firstIndex && [indexes containsIndex:idx]) { - ASDisplayNode *node = ASDynamicCast(sublayout.layoutElement, ASDisplayNode); + ASDisplayNode *node = (ASDisplayNode *) sublayout.layoutElement; ASDisplayNodeCAssert(node, @"ASDisplayNode was deallocated before it was added to a subnode. It's likely the case that you use automatically manages subnodes and allocate a ASDisplayNode in layoutSpecThatFits: and don't have any strong reference to it."); // Ignore the odd case in which a non-node sublayout is accessed and the type cast fails if (node != nil) { From a8cf0656d752fa69bcda7d0b17dcb714bf3b8da3 Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Tue, 10 Jul 2018 12:10:46 -0700 Subject: [PATCH 10/15] Add commentary to NSArray+Diffing.h; make multimap elements unowned --- Source/Details/NSArray+Diffing.h | 54 +++++++++++++++++++++++++++++++ Source/Details/NSArray+Diffing.mm | 2 +- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/Source/Details/NSArray+Diffing.h b/Source/Details/NSArray+Diffing.h index 02a40685c..3fd83806e 100644 --- a/Source/Details/NSArray+Diffing.h +++ b/Source/Details/NSArray+Diffing.h @@ -17,6 +17,60 @@ #import +/** + * These changes can be used to transform `self` to `array` by applying them in (any) order, *without shifting* the + * other elements. This can be done (in an NSMutableArray) by calling `setObject:atIndexedSubscript:` (or just use + * [subscripting] directly) for insertions from `array` into `self` (not the seemingly more apt `insertObject:atIndex`!), + * and using the same method for deletions from `self` (*set* a `[NSNull null]` as opposed to `removeObject:atIndex:`). + * After all inserts/deletes have been applied, there will be no nulls left (except possibly at the end of the array if + * `[array count] < [self count]`) + + * Some examples: + * in: ab c + * out: abdc + * diff: ..+. + * + * in: abcd + * out: dcba + * dif: ---.+++ + * + * in: abcd + * out: ab d + * diff: ..-. + * + * in: a bcd + * out: adbc + * diff: .+..- + * + * If `moves` pointer is passed in, instances where one element moves to another location are detected and reported, + * possibly replacing pairs of delete/insert. The process for transforming an array remains the same, however now it is + * important to apply the moves in order and not overwrite an element that needs to be moved somewhere else. + * + * the same examples, with moves: + * in: ab c + * out: abdc + * diff: ..+. + * + * in: abcd + * out: dcba + * diff: 321. + * + * in: abcd + * out: ab d + * diff: ..-. + * + * in: abcd + * out: adbc + * diff: .312 + * + * Other notes: + * + * No index will be both moved from and deleted. + * Each index 0...[self count] will be either moved from or deleted. If it is moved to the same location, we omit it. + * Each index 0...[array count] will be the destination of ONE move or ONE insert. + * Knowing these things means any two of the three (delete, move, insert) implies the third. + */ + @interface NSArray (Diffing) /** diff --git a/Source/Details/NSArray+Diffing.mm b/Source/Details/NSArray+Diffing.mm index 295943e0f..64063ff71 100644 --- a/Source/Details/NSArray+Diffing.mm +++ b/Source/Details/NSArray+Diffing.mm @@ -53,7 +53,7 @@ - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions { bool operator()(id lhs, id rhs) const { return (bool) [lhs isEqual:rhs]; }; }; - std::unordered_multimap potentialMoves; + std::unordered_multimap potentialMoves; NSAssert(comparison != nil, @"Comparison block is required"); NSAssert(moves == nil || comparison == [NSArray defaultCompareBlock], @"move detection requires isEqual: and hash (no custom compare)"); From 14150e16ce273a3a0ab5011f78e4311a633ed3af Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Tue, 10 Jul 2018 15:22:47 -0700 Subject: [PATCH 11/15] Use std::make_pair, optimize ASLayout+IGListKit --- Source/Layout/ASLayout+IGListKit.mm | 18 +++++++++++------- Source/Private/ASLayoutTransition.mm | 14 +++----------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/Source/Layout/ASLayout+IGListKit.mm b/Source/Layout/ASLayout+IGListKit.mm index 477d30e73..9bf944e45 100644 --- a/Source/Layout/ASLayout+IGListKit.mm +++ b/Source/Layout/ASLayout+IGListKit.mm @@ -12,24 +12,28 @@ #import #if AS_IG_LIST_KIT #import "ASLayout+IGListKit.h" -#import -#import -#import + +@interface ASLayout() { +@public + id _layoutElement; +} +@end @implementation ASLayout(IGListKit) -- (nonnull id )diffIdentifier + +- (id )diffIdentifier { - return @([self.layoutElement hash]); + return [NSValue valueWithPointer: (__bridge void*) self->_layoutElement]; } -- (BOOL)isEqualToDiffableObject:(nullable id )other +- (BOOL)isEqualToDiffableObject:(id )other { if (other == self) return YES; ASLayout *otherLayout = ASDynamicCast(other, ASLayout); if (!otherLayout) return NO; - return [otherLayout.layoutElement isEqual:self.layoutElement]; + return (otherLayout->_layoutElement == self->_layoutElement); } @end #endif // AS_IG_LIST_KIT diff --git a/Source/Private/ASLayoutTransition.mm b/Source/Private/ASLayoutTransition.mm index ea5e9bce5..091d50c3e 100644 --- a/Source/Private/ASLayoutTransition.mm +++ b/Source/Private/ASLayoutTransition.mm @@ -20,16 +20,9 @@ #import #import -#import #import // Required for _insertSubnode... / _removeFromSupernode. -#import #import -#import -#import - -#import -#import #if AS_IG_LIST_KIT #import @@ -186,8 +179,7 @@ - (void)calculateSubnodeOperationsIfNeeded _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, result.inserts, &_insertedSubnodes); _removedSubnodePositions = findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes); for (IGListMoveIndex *move in result.moves) { - unowned ASDisplayNode *subnode = previousLayout.sublayouts[move.from].layoutElement; - _subnodeMoves.push_back(std::pair(subnode, move.to)); + _subnodeMoves.push_back(std::make_pair(previousLayout.sublayouts[move.from].layoutElement, move.to)); } // Sort by ascending order of move destinations, this will allow easy loop of `insertSubnode:AtIndex` later. @@ -210,8 +202,8 @@ - (void)calculateSubnodeOperationsIfNeeded &_removedSubnodes); // These should arrive sorted in ascending order of move destinations. for (NSIndexPath *move in moves) { - unowned ASDisplayNode *subnode = previousLayout.sublayouts[([move indexAtPosition:0])].layoutElement; - _subnodeMoves.push_back(std::pair(subnode, [move indexAtPosition:1])); + _subnodeMoves.push_back(std::make_pair(previousLayout.sublayouts[([move indexAtPosition:0])].layoutElement, + [move indexAtPosition:1])); } #endif } else { From b400caca58674c19fbf478e054ec95ac670fb330 Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Wed, 11 Jul 2018 10:47:29 -0700 Subject: [PATCH 12/15] Oops I thought I had added these headers but nope --- Source/Private/ASLayoutTransition.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Source/Private/ASLayoutTransition.mm b/Source/Private/ASLayoutTransition.mm index 091d50c3e..e9a09c1c8 100644 --- a/Source/Private/ASLayoutTransition.mm +++ b/Source/Private/ASLayoutTransition.mm @@ -20,7 +20,9 @@ #import #import +#import #import // Required for _insertSubnode... / _removeFromSupernode. +#import #import From 0c949c4e732c8dffce1f7bc46f0b5fb3450ba62a Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Wed, 11 Jul 2018 12:40:27 -0700 Subject: [PATCH 13/15] Simplify simplify --- Source/Layout/ASLayout+IGListKit.mm | 9 ++------- Source/Layout/ASLayout.mm | 2 ++ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/Source/Layout/ASLayout+IGListKit.mm b/Source/Layout/ASLayout+IGListKit.mm index 9bf944e45..438689153 100644 --- a/Source/Layout/ASLayout+IGListKit.mm +++ b/Source/Layout/ASLayout+IGListKit.mm @@ -23,17 +23,12 @@ @implementation ASLayout(IGListKit) - (id )diffIdentifier { - return [NSValue valueWithPointer: (__bridge void*) self->_layoutElement]; + return self->_layoutElement; } - (BOOL)isEqualToDiffableObject:(id )other { - if (other == self) return YES; - - ASLayout *otherLayout = ASDynamicCast(other, ASLayout); - if (!otherLayout) return NO; - - return (otherLayout->_layoutElement == self->_layoutElement); + return [self isEqual:other]; } @end #endif // AS_IG_LIST_KIT diff --git a/Source/Layout/ASLayout.mm b/Source/Layout/ASLayout.mm index 2007c28da..7e47ea027 100644 --- a/Source/Layout/ASLayout.mm +++ b/Source/Layout/ASLayout.mm @@ -283,6 +283,8 @@ - (ASLayout *)filteredNodeLayoutTree NS_RETURNS_RETAINED - (BOOL)isEqual:(id)object { + if (self == object) return YES; + ASLayout *layout = ASDynamicCast(object, ASLayout); if (layout == nil) { return NO; From 067aa85b1b843abbfb8fa38c9ab33df05a054b8f Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Thu, 12 Jul 2018 15:57:19 -0700 Subject: [PATCH 14/15] Diff subnodes instead of sublayouts --- Source/Private/ASLayoutTransition.mm | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/Source/Private/ASLayoutTransition.mm b/Source/Private/ASLayoutTransition.mm index e9a09c1c8..77a8fdd7c 100644 --- a/Source/Private/ASLayoutTransition.mm +++ b/Source/Private/ASLayoutTransition.mm @@ -67,7 +67,6 @@ @implementation ASLayoutTransition { NSArray *_insertedSubnodes; NSArray *_removedSubnodes; std::vector _insertedSubnodePositions; - std::vector _removedSubnodePositions; std::vector> _subnodeMoves; } @@ -179,7 +178,7 @@ - (void)calculateSubnodeOperationsIfNeeded // IGListDiff completes in linear time O(m+n), so use it if we have it: IGListIndexSetResult *result = IGListDiff(previousLayout.sublayouts, pendingLayout.sublayouts, IGListDiffEquality); _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, result.inserts, &_insertedSubnodes); - _removedSubnodePositions = findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes); + findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes); for (IGListMoveIndex *move in result.moves) { _subnodeMoves.push_back(std::make_pair(previousLayout.sublayouts[move.from].layoutElement, move.to)); } @@ -192,16 +191,15 @@ - (void)calculateSubnodeOperationsIfNeeded #else NSIndexSet *insertions, *deletions; NSArray *moves; - [previousLayout.sublayouts asdk_diffWithArray:pendingLayout.sublayouts + NSArray *previousNodes = [previousLayout.sublayouts valueForKey:@"layoutElement"]; + NSArray *pendingNodes = [pendingLayout.sublayouts valueForKey:@"layoutElement"]; + [previousNodes asdk_diffWithArray:pendingNodes insertions:&insertions deletions:&deletions moves:&moves]; _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, insertions, &_insertedSubnodes); - _removedSubnodePositions = findNodesInLayoutAtIndexesWithFilteredNodes(previousLayout, - deletions, - _insertedSubnodes, - &_removedSubnodes); + _removedSubnodes = [previousNodes objectsAtIndexes:deletions]; // These should arrive sorted in ascending order of move destinations. for (NSIndexPath *move in moves) { _subnodeMoves.push_back(std::make_pair(previousLayout.sublayouts[([move indexAtPosition:0])].layoutElement, @@ -212,7 +210,6 @@ - (void)calculateSubnodeOperationsIfNeeded NSIndexSet *indexes = [NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [pendingLayout.sublayouts count])]; _insertedSubnodePositions = findNodesInLayoutAtIndexes(pendingLayout, indexes, &_insertedSubnodes); _removedSubnodes = nil; - _removedSubnodePositions.clear(); } _calculatedSubnodeOperations = YES; } From d2ca43e7881b2e8f7951b1c3e9c6ae423072f3d0 Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Thu, 12 Jul 2018 15:58:57 -0700 Subject: [PATCH 15/15] Another randomized test with actual ASLayouts --- Tests/ASDisplayNodeTests.mm | 71 +++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/Tests/ASDisplayNodeTests.mm b/Tests/ASDisplayNodeTests.mm index 6a3385c94..3c6d9a71e 100644 --- a/Tests/ASDisplayNodeTests.mm +++ b/Tests/ASDisplayNodeTests.mm @@ -2392,6 +2392,77 @@ - (void)testThatHavingTheSameNodeTwiceInALayoutSpecCausesExceptionOnLayoutCalcul XCTAssertThrowsSpecificNamed([node calculateLayoutThatFits:ASSizeRangeMake(CGSizeMake(100, 100))], NSException, NSInternalInconsistencyException); } +- (void)testThatStackSpecOrdersSubnodesCorrectlyRandomness +{ + // This test ensures that the z-order of nodes matches the stack spec, including after several random relayouts / transitions. + ASDisplayNode *node = [[ASDisplayNode alloc] init]; + node.automaticallyManagesSubnodes = YES; + + DeclareNodeNamed(a); + DeclareNodeNamed(b); + DeclareNodeNamed(c); + DeclareNodeNamed(d); + DeclareNodeNamed(e); + DeclareNodeNamed(f); + DeclareNodeNamed(g); + DeclareNodeNamed(h); + DeclareNodeNamed(i); + DeclareNodeNamed(j); + + NSMutableArray *allNodes = [@[a, b, c, d, e, f, g, h, i, j] mutableCopy]; + NSArray *testPrevious = @[]; + NSArray __block *testPending = @[]; + + int len1 = 1 + arc4random_uniform(9); + for (NSUInteger n = 0; n < len1; n++) { // shuffle and add + [allNodes exchangeObjectAtIndex:n withObjectAtIndex:n + arc4random_uniform(10 - (uint32_t) n)]; + testPrevious = [testPrevious arrayByAddingObject:allNodes[n]]; + } + + __block NSUInteger testCount = 0; + node.layoutSpecBlock = ^(ASDisplayNode *node, ASSizeRange size) { + ASStackLayoutSpec *stack = [ASStackLayoutSpec verticalStackLayoutSpec]; + + if (testCount++ == 0) { + stack.children = testPrevious; + } + else { + testPending = @[]; + int len2 = 1 + arc4random_uniform(9); + for (NSUInteger n = 0; n < len2; n++) { // shuffle and add + [allNodes exchangeObjectAtIndex:n withObjectAtIndex:n + arc4random_uniform(10 - (uint32_t) n)]; + testPending = [testPending arrayByAddingObject:allNodes[n]]; + } + stack.children = testPending; + } + + return stack; + }; + + ASDisplayNodeSizeToFitSize(node, CGSizeMake(100, 100)); + [node.view layoutIfNeeded]; + + // Because automaticallyManagesSubnodes is used, the subnodes array is constructed from the layout spec's children. + NSString *expected = [[testPrevious valueForKey:@"debugName"] componentsJoinedByString:@","]; + XCTAssert([node.subnodes isEqualToArray:testPrevious], @"subnodes: %@, array: %@", node.subnodes, testPrevious); + XCTAssertNodeSubnodeSubviewSublayerOrder(node, YES /* isLoaded */, NO /* isLayerBacked */, + expected, @"Initial order"); + + for (NSUInteger n = 0; n < 25; n++) { + [node invalidateCalculatedLayout]; + [node.view setNeedsLayout]; + [node.view layoutIfNeeded]; + + + XCTAssert([node.subnodes isEqualToArray:testPending], @"subnodes: %@, array: %@", node.subnodes, testPending); + expected = [[testPending valueForKey:@"debugName"] componentsJoinedByString:@","]; + + XCTAssertEqualObjects(orderStringFromSubnodes(node), expected, @"Incorrect node order for Random order #%ld", (unsigned long) n); + XCTAssertEqualObjects(orderStringFromSubviews(node.view), expected, @"Incorrect subviews for Random order #%ld", (unsigned long) n); + XCTAssertEqualObjects(orderStringFromSublayers(node.layer), expected, @"Incorrect sublayers for Random order #%ld", (unsigned long) n); + } +} + - (void)testThatStackSpecOrdersSubnodesCorrectly { // This test ensures that the z-order of nodes matches the stack spec, including after relayout / transition.