From 10f85345702659c4eaf9f7c97f2daa77497210ab Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 5 Jul 2017 18:12:13 +0100 Subject: [PATCH 1/3] ASDataController to apply new visible map inside batch updates block --- Source/ASCollectionView.mm | 6 +++++- Source/ASTableView.mm | 6 +++++- Source/Details/ASDataController.h | 8 +++++++- Source/Details/ASDataController.mm | 16 ++++++++++++---- Source/Details/ASRangeController.h | 8 +++++++- Source/Details/ASRangeController.mm | 4 ++-- 6 files changed, 38 insertions(+), 10 deletions(-) diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm index 8777145aa..58cb85ee0 100644 --- a/Source/ASCollectionView.mm +++ b/Source/ASCollectionView.mm @@ -1895,10 +1895,11 @@ - (void)rangeController:(ASRangeController *)rangeController willUpdateWithChang } } -- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet +- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet updates:(dispatch_block_t)updates { ASDisplayNodeAssertMainThread(); if (!self.asyncDataSource || _superIsPendingDataLoad) { + updates(); [changeSet executeCompletionHandlerWithFinished:NO]; return; // if the asyncDataSource has become invalid while we are processing, ignore this request to avoid crashes } @@ -1907,6 +1908,7 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange as_activity_scope(as_activity_create("Commit collection update", changeSet.rootActivity, OS_ACTIVITY_FLAG_DEFAULT)); if (changeSet.includesReloadData) { _superIsPendingDataLoad = YES; + updates(); [super reloadData]; as_log_debug(ASCollectionLog(), "Did reloadData %@", self.collectionNode); [changeSet executeCompletionHandlerWithFinished:YES]; @@ -1915,6 +1917,8 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange __block NSUInteger numberOfUpdates = 0; [self _superPerformBatchUpdates:^{ + updates(); + for (_ASHierarchyItemChange *change in [changeSet itemChangesOfType:_ASHierarchyChangeTypeReload]) { [super reloadItemsAtIndexPaths:change.indexPaths]; numberOfUpdates++; diff --git a/Source/ASTableView.mm b/Source/ASTableView.mm index 3b9215400..c0dfd1428 100644 --- a/Source/ASTableView.mm +++ b/Source/ASTableView.mm @@ -1486,10 +1486,11 @@ - (void)rangeController:(ASRangeController *)rangeController willUpdateWithChang } } -- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet +- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet updates:(dispatch_block_t)updates { ASDisplayNodeAssertMainThread(); if (!self.asyncDataSource || _updatingInResponseToInteractiveMove) { + updates(); [changeSet executeCompletionHandlerWithFinished:NO]; return; // if the asyncDataSource has become invalid while we are processing, ignore this request to avoid crashes } @@ -1500,6 +1501,7 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange if (self.test_enableSuperUpdateCallLogging) { NSLog(@"-[super reloadData]"); } + updates(); [super reloadData]; // Flush any range changes that happened as part of submitting the reload. [_rangeController updateIfNeeded]; @@ -1513,6 +1515,8 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange LOG(@"--- UITableView beginUpdates"); [super beginUpdates]; + + updates(); for (_ASHierarchyItemChange *change in [changeSet itemChangesOfType:_ASHierarchyChangeTypeReload]) { NSArray *indexPaths = change.indexPaths; diff --git a/Source/Details/ASDataController.h b/Source/Details/ASDataController.h index 474f3fe49..2bd56fe5e 100644 --- a/Source/Details/ASDataController.h +++ b/Source/Details/ASDataController.h @@ -117,8 +117,14 @@ extern NSString * const ASCollectionInvalidUpdateException; * Called for change set updates. * * @param changeSet The change set that includes all updates + * + * @param updates The block that performs relevant data updates. + * + * @discussion The updates block must always be executed or the data controller will get into a bad state. + * It should be called at the time the backing view is ready to process the updates, + * i.e inside the updates block of `-[UICollectionView performBatchUpdates:completion:] or after calling `-[UITableView beginUpdates]`. */ -- (void)dataController:(ASDataController *)dataController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet; +- (void)dataController:(ASDataController *)dataController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet updates:(dispatch_block_t)updates; @end diff --git a/Source/Details/ASDataController.mm b/Source/Details/ASDataController.mm index 9265d8d0a..88231ab26 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -610,12 +610,20 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet [_mainSerialQueue performBlockOnMainThread:^{ as_activity_scope_leave(&preparationScope); + // TODO Merge the below two delegate methods [_delegate dataController:self willUpdateWithChangeSet:changeSet]; - // Step 4: Deploy the new data as "completed" and inform delegate - self.visibleMap = newMap; - - [_delegate dataController:self didUpdateWithChangeSet:changeSet]; + // Step 4: Inform the delegate + [_delegate dataController:self didUpdateWithChangeSet:changeSet updates:^(){ + // Step 5: Deploy the new data as "completed" + // + // Note that since the backing collection view might be busy responding to user events (e.g scrolling), + // it will not consume the batch update blocks immediately. + // As a result, in a short intermidate time, the view will still be relying on the old data source state. + // Thus, we can't just swap the new map immediately before step 4, but until this update block is executed. + // (https://github.com/TextureGroup/Texture/issues/378) + self.visibleMap = newMap; + }]; }]; }]; }); diff --git a/Source/Details/ASRangeController.h b/Source/Details/ASRangeController.h index e7ff6215f..7d707c538 100644 --- a/Source/Details/ASRangeController.h +++ b/Source/Details/ASRangeController.h @@ -156,8 +156,14 @@ AS_SUBCLASSING_RESTRICTED * Called after updating with given change set. * * @param changeSet The change set that includes all updates + * + * @param updates The block that performs relevant data updates. + * + * @discussion The updates block must always be executed or the data controller will get into a bad state. + * It should be called at the time the backing view is ready to process the updates, + * i.e inside the updates block of `-[UICollectionView performBatchUpdates:completion:] or after calling `-[UITableView beginUpdates]`. */ -- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet; +- (void)rangeController:(ASRangeController *)rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet updates:(dispatch_block_t)updates; @end diff --git a/Source/Details/ASRangeController.mm b/Source/Details/ASRangeController.mm index 4384b1b15..0be9bf5b6 100644 --- a/Source/Details/ASRangeController.mm +++ b/Source/Details/ASRangeController.mm @@ -502,11 +502,11 @@ - (void)dataController:(ASDataController *)dataController willUpdateWithChangeSe [_delegate rangeController:self willUpdateWithChangeSet:changeSet]; } -- (void)dataController:(ASDataController *)dataController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet +- (void)dataController:(ASDataController *)dataController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet updates:(dispatch_block_t)updates { ASDisplayNodeAssertMainThread(); _rangeIsValid = NO; - [_delegate rangeController:self didUpdateWithChangeSet:changeSet]; + [_delegate rangeController:self didUpdateWithChangeSet:changeSet updates:updates]; } #pragma mark - Memory Management From 986f98b6b485b2bace905c3d0f3d9940715bae95 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 5 Jul 2017 18:43:16 +0100 Subject: [PATCH 2/3] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b85ccc6f3..09acf56b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Fix an issue where inserting/deleting sections could lead to inconsistent supplementary element behavior. [Adlai Holler](https://github.com/Adlai-Holler) - Overhaul logging and add activity tracing support. [Adlai Holler](https://github.com/Adlai-Holler) - Fix a crash where scrolling a table view after entering editing mode could lead to bad internal states in the table. [Huy Nguyen](https://github.com/nguyenhuy) [#416](https://github.com/TextureGroup/Texture/pull/416/) +- Fix a crash in collection view that occurs if batch updates are performed while scrolling [Huy Nguyen](https://github.com/nguyenhuy) [#378](https://github.com/TextureGroup/Texture/issues/378) ##2.3.4 - [Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration [Scott Goodson](https://github.com/appleguy) From 735946ffbae73605c970a76f47ab35237d2701fc Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 5 Jul 2017 18:47:38 +0100 Subject: [PATCH 3/3] Sorry, put up a PR that doesn't even build LOL --- Source/Details/ASDataController.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/Details/ASDataController.mm b/Source/Details/ASDataController.mm index 88231ab26..9a92a4404 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -610,11 +610,11 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet [_mainSerialQueue performBlockOnMainThread:^{ as_activity_scope_leave(&preparationScope); - // TODO Merge the below two delegate methods + // TODO Merge the two delegate methods below [_delegate dataController:self willUpdateWithChangeSet:changeSet]; // Step 4: Inform the delegate - [_delegate dataController:self didUpdateWithChangeSet:changeSet updates:^(){ + [_delegate dataController:self didUpdateWithChangeSet:changeSet updates:^{ // Step 5: Deploy the new data as "completed" // // Note that since the backing collection view might be busy responding to user events (e.g scrolling),