From a4c6d8912e1a6a1fbe125057f84ee2f8afca811a Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 14 Feb 2017 16:28:26 -0800 Subject: [PATCH] Don't Skip Remeasurement On First Layout Pass (#3031) * Don't Skip Remeasurement if Initial Bounds are Zero * Remove misleading unit test --- AsyncDisplayKit/ASCollectionView.mm | 41 ++++++++---------------- AsyncDisplayKit/ASTableView.mm | 22 ++++--------- AsyncDisplayKitTests/ASTableViewTests.mm | 23 ------------- 3 files changed, 20 insertions(+), 66 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index 56e7103300..961b77662e 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -89,7 +89,6 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; ASBatchContext *_batchContext; CGSize _lastBoundsSizeUsedForMeasuringNodes; - BOOL _ignoreNextBoundsSizeChangeForMeasuringNodes; NSMutableSet *_registeredSupplementaryKinds; @@ -272,9 +271,6 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; _superIsPendingDataLoad = YES; _lastBoundsSizeUsedForMeasuringNodes = self.bounds.size; - // If the initial size is 0, expect a size change very soon which is part of the initial configuration - // and should not trigger a relayout. - _ignoreNextBoundsSizeChangeForMeasuringNodes = CGSizeEqualToSize(_lastBoundsSizeUsedForMeasuringNodes, CGSizeZero); _layoutFacilitator = layoutFacilitator; @@ -1904,31 +1900,22 @@ static NSString * const kReuseIdentifier = @"_ASCollectionReuseIdentifier"; } _lastBoundsSizeUsedForMeasuringNodes = newBounds.size; - // First size change occurs during initial configuration. An expensive relayout pass is unnecessary at that time - // and should be avoided, assuming that the initial data loading automatically runs shortly afterward. - if (_ignoreNextBoundsSizeChangeForMeasuringNodes) { - _ignoreNextBoundsSizeChangeForMeasuringNodes = NO; - } else { - // Laying out all nodes is expensive, and performing an empty update may be unsafe - // if the data source has pending changes that it hasn't reported yet – the collection - // view will requery the new counts and expect them to match the previous counts. - // - // We only need to do this if the bounds changed in the non-scrollable direction. - // If, for example, a vertical flow layout has its height changed due to a status bar - // appearance update, we do not need to relayout all nodes. - // For a more permanent fix to the unsafety mentioned above, see https://github.com/facebook/AsyncDisplayKit/pull/2182 - ASScrollDirection scrollDirection = self.scrollableDirections; - BOOL fixedVertically = (ASScrollDirectionContainsVerticalDirection(scrollDirection) == NO); - BOOL fixedHorizontally = (ASScrollDirectionContainsHorizontalDirection(scrollDirection) == NO); + // Laying out all nodes is expensive. + // We only need to do this if the bounds changed in the non-scrollable direction. + // If, for example, a vertical flow layout has its height changed due to a status bar + // appearance update, we do not need to relayout all nodes. + // For a more permanent fix to the unsafety mentioned above, see https://github.com/facebook/AsyncDisplayKit/pull/2182 + ASScrollDirection scrollDirection = self.scrollableDirections; + BOOL fixedVertically = (ASScrollDirectionContainsVerticalDirection(scrollDirection) == NO); + BOOL fixedHorizontally = (ASScrollDirectionContainsHorizontalDirection(scrollDirection) == NO); - BOOL changedInNonScrollingDirection = (fixedHorizontally && newBounds.size.width != lastUsedSize.width) || (fixedVertically && newBounds.size.height != lastUsedSize.height); + BOOL changedInNonScrollingDirection = (fixedHorizontally && newBounds.size.width != lastUsedSize.width) || (fixedVertically && newBounds.size.height != lastUsedSize.height); - if (changedInNonScrollingDirection) { - [_dataController relayoutAllNodes]; - [_dataController waitUntilAllUpdatesAreCommitted]; - // We need to ensure the size requery is done before we update our layout. - [self.collectionViewLayout invalidateLayout]; - } + if (changedInNonScrollingDirection) { + [_dataController relayoutAllNodes]; + [_dataController waitUntilAllUpdatesAreCommitted]; + // We need to ensure the size requery is done before we update our layout. + [self.collectionViewLayout invalidateLayout]; } } diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index 38ff2e91ed..c7f3430008 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -29,7 +29,6 @@ #import #import -static const ASSizeRange kInvalidSizeRange = {CGSizeZero, CGSizeZero}; static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; //#define LOG(...) NSLog(__VA_ARGS__) @@ -144,7 +143,6 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; CALayer *_retainedLayer; CGFloat _nodesConstrainedWidth; - BOOL _ignoreNodesConstrainedWidthChange; BOOL _queuedNodeHeightUpdate; BOOL _isDeallocating; BOOL _performingBatchUpdates; @@ -268,9 +266,6 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; _automaticallyAdjustsContentOffset = NO; _nodesConstrainedWidth = self.bounds.size.width; - // If the initial size is 0, expect a size change very soon which is part of the initial configuration - // and should not trigger a relayout. - _ignoreNodesConstrainedWidthChange = (_nodesConstrainedWidth == 0); _proxyDelegate = [[ASTableViewProxy alloc] initWithTarget:nil interceptor:self]; super.delegate = (id)_proxyDelegate; @@ -690,19 +685,14 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; - (void)layoutSubviews { + // Remeasure all rows if our row width has changed. CGFloat constrainedWidth = self.bounds.size.width - [self sectionIndexWidth]; - if (_nodesConstrainedWidth != constrainedWidth) { + if (constrainedWidth > 0 && _nodesConstrainedWidth != constrainedWidth) { _nodesConstrainedWidth = constrainedWidth; - // First width change occurs during initial configuration. An expensive relayout pass is unnecessary at that time - // and should be avoided, assuming that the initial data loading automatically runs shortly afterward. - if (_ignoreNodesConstrainedWidthChange) { - _ignoreNodesConstrainedWidthChange = NO; - } else { - [self beginUpdates]; - [_dataController relayoutAllNodes]; - [self endUpdatesAnimated:(ASDisplayNodeLayerHasAnimations(self.layer) == NO) completion:nil]; - } + [self beginUpdates]; + [_dataController relayoutAllNodes]; + [self endUpdatesAnimated:(ASDisplayNodeLayerHasAnimations(self.layer) == NO) completion:nil]; } // To ensure _nodesConstrainedWidth is up-to-date for every usage, this call to super must be done last @@ -1613,7 +1603,7 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; - (ASSizeRange)dataController:(ASDataController *)dataController constrainedSizeForNodeAtIndexPath:(NSIndexPath *)indexPath { - ASSizeRange constrainedSize = kInvalidSizeRange; + ASSizeRange constrainedSize = ASSizeRangeZero; if (_asyncDelegateFlags.tableNodeConstrainedSizeForRow) { GET_TABLENODE_OR_RETURN(tableNode, constrainedSize); ASSizeRange delegateConstrainedSize = [_asyncDelegate tableNode:tableNode constrainedSizeForRowAtIndexPath:indexPath]; diff --git a/AsyncDisplayKitTests/ASTableViewTests.mm b/AsyncDisplayKitTests/ASTableViewTests.mm index 8717211a33..504835bd58 100644 --- a/AsyncDisplayKitTests/ASTableViewTests.mm +++ b/AsyncDisplayKitTests/ASTableViewTests.mm @@ -395,29 +395,6 @@ [self triggerSizeChangeAndAssertRelayoutAllNodesForTableView:tableView newSize:tableViewFinalSize]; } -- (void)testRelayoutAllNodesWithZeroSizeInitially -{ - // Initial width of the table view is 0. The first size change is part of the initial config. - // Any subsequence size change after that must trigger a relayout. - CGSize tableViewFinalSize = CGSizeMake(100, 500); - ASTestTableView *tableView = [[ASTestTableView alloc] __initWithFrame:CGRectZero - style:UITableViewStylePlain]; - ASTableViewFilledDataSource *dataSource = [ASTableViewFilledDataSource new]; - - tableView.asyncDelegate = dataSource; - tableView.asyncDataSource = dataSource; - - // Initial configuration - UIView *superview = [[UIView alloc] initWithFrame:CGRectMake(0, 0, 500, 500)]; - [superview addSubview:tableView]; - // Width and height are swapped so that a later size change will simulate a rotation - tableView.frame = CGRectMake(0, 0, tableViewFinalSize.height, tableViewFinalSize.width); - [tableView layoutIfNeeded]; - - XCTAssertEqual(tableView.testDataController.numberOfAllNodesRelayouts, 0); - [self triggerSizeChangeAndAssertRelayoutAllNodesForTableView:tableView newSize:tableViewFinalSize]; -} - - (void)testRelayoutVisibleRowsWhenEditingModeIsChanged { CGSize tableViewSize = CGSizeMake(100, 500);