Skip to content

Commit

Permalink
clean up IGListExperimentSkipPerformUpdateIfPossible
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Maxime Ollivier authored and facebook-github-bot committed Jan 17, 2024
1 parent 158beab commit b3a22ad
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 75 deletions.
4 changes: 1 addition & 3 deletions Source/IGListDiffKit/IGListExperiments.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

/**
Expand Down
31 changes: 1 addition & 30 deletions Source/IGListKit/Internal/IGListBatchUpdateTransaction.m
Original file line number Diff line number Diff line change
Expand Up @@ -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];
};

Expand Down
42 changes: 0 additions & 42 deletions Tests/IGListAdapterUpdaterTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit b3a22ad

Please sign in to comment.