From 0d8518912e1fdba25ac381ee9355893edd666aab Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Thu, 8 Sep 2016 20:59:57 +0200 Subject: [PATCH] Initial commit to throw an exception if manually adding / removing subnodes if node has automaticallyManagesSubnodes enabled (#2214) --- AsyncDisplayKit/ASDisplayNode.mm | 46 +++++++++++++++++-- .../Private/ASDisplayNodeInternal.h | 11 +++++ AsyncDisplayKit/Private/ASLayoutTransition.mm | 4 +- .../ASDisplayNodeImplicitHierarchyTests.m | 18 +++++++- 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 3713f6e930..947c63dce1 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1553,6 +1553,12 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD } - (void)addSubnode:(ASDisplayNode *)subnode +{ + ASDisplayNodeAssert(self.automaticallyManagesSubnodes == NO, @"Attempt to manually add subnode to node with automaticallyManagesSubnodes=YES. Node: %@", subnode); + [self _addSubnode:subnode]; +} + +- (void)_addSubnode:(ASDisplayNode *)subnode { ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(__instanceLock__); @@ -1568,7 +1574,7 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD if (isMovingEquivalentParents) { [subnode __incrementVisibilityNotificationsDisabled]; } - [subnode removeFromSupernode]; + [subnode _removeFromSupernode]; if (!_subnodes) { _subnodes = [[NSMutableArray alloc] init]; @@ -1624,8 +1630,8 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD [subnode __incrementVisibilityNotificationsDisabled]; } - [subnode removeFromSupernode]; - [oldSubnode removeFromSupernode]; + [subnode _removeFromSupernode]; + [oldSubnode _removeFromSupernode]; if (!_subnodes) _subnodes = [[NSMutableArray alloc] init]; @@ -1661,6 +1667,12 @@ static bool disableNotificationsForMovingBetweenParents(ASDisplayNode *from, ASD } - (void)replaceSubnode:(ASDisplayNode *)oldSubnode withSubnode:(ASDisplayNode *)replacementSubnode +{ + ASDisplayNodeAssert(self.automaticallyManagesSubnodes == NO, @"Attempt to manually replace old node with replacement node to node with automaticallyManagesSubnodes=YES. Old Node: %@, replacement node: %@", oldSubnode, replacementSubnode); + [self _replaceSubnode:oldSubnode withSubnode:replacementSubnode]; +} + +- (void)_replaceSubnode:(ASDisplayNode *)oldSubnode withSubnode:(ASDisplayNode *)replacementSubnode { ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(__instanceLock__); @@ -1691,6 +1703,12 @@ static NSInteger incrementIfFound(NSInteger i) { } - (void)insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below +{ + ASDisplayNodeAssert(self.automaticallyManagesSubnodes == NO, @"Attempt to manually insert subnode to node with automaticallyManagesSubnodes=YES. Node: %@", subnode); + [self _insertSubnode:subnode belowSubnode:below]; +} + +- (void)_insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below { ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(__instanceLock__); @@ -1736,6 +1754,12 @@ static NSInteger incrementIfFound(NSInteger i) { } - (void)insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above +{ + ASDisplayNodeAssert(self.automaticallyManagesSubnodes == NO, @"Attempt to manually insert subnode to node with automaticallyManagesSubnodes=YES. Node: %@", subnode); + [self _insertSubnode:subnode aboveSubnode:above]; +} + +- (void)_insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above { ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(__instanceLock__); @@ -1784,6 +1808,12 @@ static NSInteger incrementIfFound(NSInteger i) { } - (void)insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx +{ + ASDisplayNodeAssert(self.automaticallyManagesSubnodes == NO, @"Attempt to manually insert subnode to node with automaticallyManagesSubnodes=YES. Node: %@", subnode); + [self _insertSubnode:subnode atIndex:idx]; +} + +- (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx { ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(__instanceLock__); @@ -1854,10 +1884,18 @@ static NSInteger incrementIfFound(NSInteger i) { [subnode __setSupernode:nil]; } -// NOTE: You must not called this method while holding the receiver's property lock. This may cause deadlocks. - (void)removeFromSupernode +{ + ASDisplayNodeAssert(self.supernode.automaticallyManagesSubnodes == NO, @"Attempt to manually remove subnode from node with automaticallyManagesSubnodes=YES. Node: %@", self); + + [self _removeFromSupernode]; +} + +// NOTE: You must not called this method while holding the receiver's property lock. This may cause deadlocks. +- (void)_removeFromSupernode { ASDisplayNodeAssertThreadAffinity(self); + __instanceLock__.lock(); __weak ASDisplayNode *supernode = _supernode; __weak UIView *view = _view; diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index 05df7b12e1..715b39d61b 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -199,6 +199,17 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo - (void)__layout; - (void)__setSupernode:(ASDisplayNode *)supernode; +/** + Internal method to add / replace / insert subnode and remove from supernode without checking if + node has automaticallyManagesSubnodes set to YES. + */ +- (void)_addSubnode:(ASDisplayNode *)subnode; +- (void)_replaceSubnode:(ASDisplayNode *)oldSubnode withSubnode:(ASDisplayNode *)replacementSubnode; +- (void)_insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below; +- (void)_insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above; +- (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx; +- (void)_removeFromSupernode; + // Private API for helper functions / unit tests. Use ASDisplayNodeDisableHierarchyNotifications() to control this. - (BOOL)__visibilityNotificationsDisabled; - (BOOL)__selfOrParentHasVisibilityNotificationsDisabled; diff --git a/AsyncDisplayKit/Private/ASLayoutTransition.mm b/AsyncDisplayKit/Private/ASLayoutTransition.mm index 90357a266e..1c3bf9b818 100644 --- a/AsyncDisplayKit/Private/ASLayoutTransition.mm +++ b/AsyncDisplayKit/Private/ASLayoutTransition.mm @@ -98,7 +98,7 @@ static inline BOOL ASLayoutCanTransitionAsynchronous(ASLayout *layout) { NSUInteger i = 0; for (ASDisplayNode *node in _insertedSubnodes) { NSUInteger p = _insertedSubnodePositions[i]; - [_node insertSubnode:node atIndex:p]; + [_node _insertSubnode:node atIndex:p]; i += 1; } } @@ -108,7 +108,7 @@ static inline BOOL ASLayoutCanTransitionAsynchronous(ASLayout *layout) { ASDN::MutexSharedLocker l(__instanceLock__); [self calculateSubnodeOperationsIfNeeded]; for (ASDisplayNode *subnode in _removedSubnodes) { - [subnode removeFromSupernode]; + [subnode _removeFromSupernode]; } } diff --git a/AsyncDisplayKitTests/ASDisplayNodeImplicitHierarchyTests.m b/AsyncDisplayKitTests/ASDisplayNodeImplicitHierarchyTests.m index 95ba170b72..c1877b26da 100644 --- a/AsyncDisplayKitTests/ASDisplayNodeImplicitHierarchyTests.m +++ b/AsyncDisplayKitTests/ASDisplayNodeImplicitHierarchyTests.m @@ -18,6 +18,7 @@ #import "ASStaticLayoutSpec.h" #import "ASStackLayoutSpec.h" +#import "ASInsetLayoutSpec.h" @interface ASSpecTestDisplayNode : ASDisplayNode @@ -117,9 +118,24 @@ XCTAssertEqual(node.subnodes[2], node2); } +- (void)testLayoutTransitionWillThrowForManualSubnodeManagement +{ + ASDisplayNode *node1 = [[ASDisplayNode alloc] init]; + node1.name = @"node1"; + + ASSpecTestDisplayNode *node = [[ASSpecTestDisplayNode alloc] init]; + node.automaticallyManagesSubnodes = YES; + node.layoutSpecBlock = ^ASLayoutSpec *(ASDisplayNode *weakNode, ASSizeRange constrainedSize){ + return [ASStaticLayoutSpec staticLayoutSpecWithChildren:@[node1]]; + }; + + XCTAssertNoThrow([node layoutThatFits:ASSizeRangeMake(CGSizeZero)]); + XCTAssertThrows([node1 removeFromSupernode]); +} + - (void)testLayoutTransitionMeasurementCompletionBlockIsCalledOnMainThread { - ASDisplayNode *displayNode = [ASDisplayNode new]; + ASDisplayNode *displayNode = [[ASDisplayNode alloc] init]; // Trigger explicit view creation to be able to use the Transition API [displayNode view];