Skip to content

Commit be0b74b

Browse files
authored
Check for batch fetching on scroll (#2124)
## Summary For collection views, we do not currently check for batch fetching on scroll. This appears to be a bug, as `scrollViewDidScroll:` does call `_checkForBatchFetching`, and [the commit that added it states it's trying to check on each scroll](df497b8). Unfortunately, `_checkForBatchFetching` checks for `isTracking` and `isDragging` first and returns if either are `YES`. Since we're in a scroll, they are `YES`. So the call is effectively a no-op. This bug has been around for 9 years and I'm unsure of the performance implications of turning the batch fetching check on for each scroll tick. Therefore, put the fix behind an experiment feature flag _and_ only call it on the first scroll tick for the scroll session, instead of every scroll tick. Finally, I moved the check after the delegate call, in case the delegate has logic in it to turn on or off the batch fetching. ## Test plan Ran the app and manually tested batching getting called or not. Also ran `build.sh tests`.
1 parent e894389 commit be0b74b

File tree

3 files changed

+19
-2
lines changed

3 files changed

+19
-2
lines changed

Source/ASCollectionView.mm

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,12 @@ @interface ASCollectionView () <ASRangeControllerDataSource, ASRangeControllerDe
142142
*/
143143
BOOL _hasEverCheckedForBatchFetchingDueToUpdate;
144144

145+
/**
146+
* We want to check for batch fetching on scroll, but every tick would be too much. So check once at the
147+
* beginning
148+
*/
149+
BOOL _hasCheckedForBatchFetchingOnScroll;
150+
145151
/**
146152
* Set during beginInteractiveMovementForItemAtIndexPath and UIGestureRecognizerStateEnded
147153
* (or UIGestureRecognizerStateFailed, UIGestureRecognizerStateCancelled.
@@ -1620,7 +1626,9 @@ - (void)collectionView:(UICollectionView *)collectionView moveItemAtIndexPath:(N
16201626
- (void)scrollViewDidScroll:(UIScrollView *)scrollView
16211627
{
16221628
ASInterfaceState interfaceState = [self interfaceStateForRangeController:_rangeController];
1623-
if (ASInterfaceStateIncludesVisible(interfaceState)) {
1629+
if (ASInterfaceStateIncludesVisible(interfaceState) && !ASActivateExperimentalFeature(ASExperimentalCheckBatchFetchingOnScroll)) {
1630+
// The following call to _checkForBatchFetching is effectively a no-op, because during scrolling
1631+
// isDragging and isTracking are YES.
16241632
[self _checkForBatchFetching];
16251633
}
16261634
for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) {
@@ -1630,6 +1638,11 @@ - (void)scrollViewDidScroll:(UIScrollView *)scrollView
16301638
if (_asyncDelegateFlags.scrollViewDidScroll) {
16311639
[_asyncDelegate scrollViewDidScroll:scrollView];
16321640
}
1641+
if (ASInterfaceStateIncludesVisible(interfaceState) && !_hasCheckedForBatchFetchingOnScroll && ASActivateExperimentalFeature(ASExperimentalCheckBatchFetchingOnScroll)) {
1642+
// Check after the delegate it give it a chance to turn on/off batch fetching
1643+
[self _beginBatchFetchingIfNeededWithContentOffset:self.contentOffset velocity:CGPointZero];
1644+
_hasCheckedForBatchFetchingOnScroll = YES;
1645+
}
16331646
}
16341647

16351648
- (void)scrollViewWillEndDragging:(UIScrollView *)scrollView withVelocity:(CGPoint)velocity targetContentOffset:(inout CGPoint *)targetContentOffset
@@ -1645,6 +1658,8 @@ - (void)scrollViewWillEndDragging:(UIScrollView *)scrollView withVelocity:(CGPoi
16451658
[self _beginBatchFetchingIfNeededWithContentOffset:*targetContentOffset velocity:velocity];
16461659
}
16471660

1661+
_hasCheckedForBatchFetchingOnScroll = NO; // reset once the scroll is done
1662+
16481663
if (_asyncDelegateFlags.scrollViewWillEndDragging) {
16491664
[_asyncDelegate scrollViewWillEndDragging:scrollView withVelocity:velocity targetContentOffset:(targetContentOffset ? : &contentOffset)];
16501665
}

Source/ASExperimentalFeatures.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
3434
ASExperimentalNoTextRendererCache = 1 << 13, // exp_no_text_renderer_cache
3535
ASExperimentalLockTextRendererCache = 1 << 14, // exp_lock_text_renderer_cache
3636
ASExperimentalHierarchyDisplayDidFinishIsRecursive = 1 << 15, // exp_hierarchy_display_did_finish_is_recursive
37+
ASExperimentalCheckBatchFetchingOnScroll = 1 << 16, // exp_check_batch_fetching_on_scroll
3738
ASExperimentalFeatureAll = 0xFFFFFFFF
3839
};
3940

Source/ASExperimentalFeatures.mm

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@
2727
@"exp_range_update_on_changeset_update",
2828
@"exp_no_text_renderer_cache",
2929
@"exp_lock_text_renderer_cache",
30-
@"exp_hierarchy_display_did_finish_is_recursive"]));
30+
@"exp_hierarchy_display_did_finish_is_recursive",
31+
@"exp_check_batch_fetching_on_scroll"]));
3132

3233
if (flags == ASExperimentalFeatureAll) {
3334
return allNames;

0 commit comments

Comments
 (0)