From b3a22ad554995b3a869bb1b75369108e65bd867e Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Wed, 17 Jan 2024 14:06:13 -0800 Subject: [PATCH] clean up IGListExperimentSkipPerformUpdateIfPossible Summary: This is probably not super safe, so lets kill it. If we want to know all the section and cell updates outside the update block, we need to make cells diffable, which is something we're considering. Reviewed By: candance Differential Revision: D52743451 fbshipit-source-id: fd8004876c452552931bf84ee73cd3ac15f3ba40 --- Source/IGListDiffKit/IGListExperiments.h | 4 +- .../Internal/IGListBatchUpdateTransaction.m | 31 +------------- Tests/IGListAdapterUpdaterTests.m | 42 ------------------- 3 files changed, 2 insertions(+), 75 deletions(-) diff --git a/Source/IGListDiffKit/IGListExperiments.h b/Source/IGListDiffKit/IGListExperiments.h index 3f7ded419..826c29419 100644 --- a/Source/IGListDiffKit/IGListExperiments.h +++ b/Source/IGListDiffKit/IGListExperiments.h @@ -18,10 +18,8 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) { IGListExperimentNone = 1 << 1, /// Test invalidating layout when cell reloads/updates in IGListBindingSectionController. IGListExperimentInvalidateLayoutForUpdates = 1 << 2, - /// Test skipping performBatchUpdate if we don't have any updates. - IGListExperimentSkipPerformUpdateIfPossible = 1 << 3, /// Throw NSInternalInconsistencyException during an update - IGListExperimentThrowOnInconsistencyException = 1 << 4 + IGListExperimentThrowOnInconsistencyException = 1 << 3 }; /** diff --git a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m index 1516a7442..d497998d6 100644 --- a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m +++ b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m @@ -169,37 +169,8 @@ - (void)_applyDiff:(IGListIndexSetResult *)diffResult { toObjects:self.sectionData.toObjects listIndexSetResult:diffResult animated:self.animated]; - - // Experiment to skip calling `[UICollectionView performBatchUpdates ...]` if we don't have changes. It does - // require us to call `_applyDataUpdates` outside the update block, but that should be ok as long as we call - // `performBatchUpdates` right after. - const BOOL skipPerformUpdateIfPossible = IGListExperimentEnabled(self.config.experiments, IGListExperimentSkipPerformUpdateIfPossible); - if (skipPerformUpdateIfPossible) { - // From Apple docs: If the collection view's layout is not up to date before you call performBatchUpdates, a reload may - // occur. To avoid problems, you should update your data model inside the updates block or ensure the layout is - // updated before you call performBatchUpdates(_:completion:). - [self.collectionView layoutIfNeeded]; - - [self _applyDataUpdates]; - - if (!diffResult.hasChanges && !self.inUpdateItemCollector.hasChanges) { - // If we don't have any section or item changes, take a shortcut. - [self _finishWithoutUpdate]; - return; - } - } - - // ************************** - // ************************** - // IMPORTANT: The very next thing we call must be `[UICollectionView performBatchUpdates ...]`, because - // we're in a state where the adapter is synced, but not the `UICollectionView`. - // ************************** - // ************************** - void (^updates)(void) = ^ { - if (!skipPerformUpdateIfPossible) { - [self _applyDataUpdates]; - } + [self _applyDataUpdates]; [self _applyCollectioViewUpdates:diffResult]; }; diff --git a/Tests/IGListAdapterUpdaterTests.m b/Tests/IGListAdapterUpdaterTests.m index 6a2f1f734..c15b3c09a 100644 --- a/Tests/IGListAdapterUpdaterTests.m +++ b/Tests/IGListAdapterUpdaterTests.m @@ -985,48 +985,6 @@ - (void)_test_whenCollectionViewSectionCountIsIncorrect_thatDoesNotCrash { [self waitForExpectationsWithTimeout:30 handler:nil]; } -- (void)test_whenNoChanges_thatPerformUpdateExitsEarly { - self.updater.experiments |= IGListExperimentSkipPerformUpdateIfPossible; - - NSArray *from = @[ - [IGSectionObject sectionWithObjects:@[] identifier:@"Foo"] - ]; - NSArray *to = @[ - [IGSectionObject sectionWithObjects:@[] identifier:@"Foo"] - ]; - - self.dataSource.sections = from; - [self.updater reloadDataWithCollectionViewBlock:[self collectionViewBlock] reloadUpdateBlock:^{} completion:nil]; - [self.updater update]; - - id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; - self.updater.delegate = mockDelegate; - [mockDelegate setExpectationOrderMatters:YES]; - - [[mockDelegate expect] listAdapterUpdater:self.updater - willPerformBatchUpdatesWithCollectionView:self.collectionView - fromObjects:from - toObjects:to - listIndexSetResult:OCMOCK_ANY - animated:NO]; - - [[mockDelegate expect] listAdapterUpdater:self.updater didFinishWithoutUpdatesWithCollectionView:self.collectionView]; - - XCTestExpectation *expectation = genExpectation; - [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] - animated:NO - sectionDataBlock:[self dataBlockFromObjects:from toObjects:to] - applySectionDataBlock:self.applySectionDataBlock - completion:^(BOOL finished) { - XCTAssertTrue(finished); - XCTAssertEqual([self.collectionView numberOfSections], 1); - [expectation fulfill]; - }]; - - [self waitForExpectationsWithTimeout:30 handler:nil]; - [mockDelegate verify]; -} - # pragma mark - preferItemReloadsFroSectionReloads - (void)test_whenReloadIsCalledWithSameItemCount_andPreferItemReload_updateIndexPathsHappen {