From fcd452e3661ac2b0737679b6747b83ec8f22a21a Mon Sep 17 00:00:00 2001 From: Zhisheng Huang Date: Thu, 9 Jan 2020 14:51:24 -0800 Subject: [PATCH] Fix a bug in IGListBindingSingleSectionController Summary: Originally we want to update the cell when the SectionController is asked to update to a new value, that's pretty much what the experiment `singleItemSectionUpdates` is for inside `IGListAdapterUpdater`. However, we should not request any cell during the initial setup, otherwise it would trigger cell to render in a wrong time, in particular we shouldn't call ``` UICollectionViewCell *cell = [self.collectionView cellForItemAtIndexPath:indexPath]; ``` as the data source set up is not ready, and it can get crashy to compare the initial dataSource(nil) to the valid data source(IGListAdapter), which doesn't make sense. So tl;dr is do not access cell during the initial data source setup. We might need to audit other callsite that might have violate this invariant. Solution here is to rely on the willDisplay/endDisplay API from `IGListDisplayDelegate` so that we can set/unset the displaying cell here before we configure the cell with the updated data. This change is also safe as it's protected by `_enabledCellConfigurationDuringUpdate` which defaults to be NO. = Facebook = Also currently the `singleItemSectionUpdates` is only setup for Inbox thread-bump useage for now. So I added a `enabledCellConfigurationDuringUpdate` to properly gate it behind QE so that we have a safer exit here. Reviewed By: maxolls Differential Revision: D19332040 fbshipit-source-id: cbca7a63b0486c053ceabe759ce316f5d841fef5 --- .../IGListBindingSingleSectionController.h | 3 ++ .../IGListBindingSingleSectionController.m | 38 +++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/Source/IGListKit/IGListBindingSingleSectionController.h b/Source/IGListKit/IGListBindingSingleSectionController.h index aa619c15..dac74192 100644 --- a/Source/IGListKit/IGListBindingSingleSectionController.h +++ b/Source/IGListKit/IGListBindingSingleSectionController.h @@ -20,6 +20,9 @@ NS_ASSUME_NONNULL_BEGIN NS_SWIFT_NAME(ListBindingSingleSectionController) @interface IGListBindingSingleSectionController<__covariant ViewModel : id, Cell : UICollectionViewCell *> : IGListSectionController +// Testing the stability of this infra. Defaults to NO. +@property (nonatomic, readwrite, assign) BOOL enabledCellConfigurationDuringUpdate; + #pragma mark - Subclass // Required to be implemented by subclass. diff --git a/Source/IGListKit/IGListBindingSingleSectionController.m b/Source/IGListKit/IGListBindingSingleSectionController.m index e00c28e8..768f2235 100644 --- a/Source/IGListKit/IGListBindingSingleSectionController.m +++ b/Source/IGListKit/IGListBindingSingleSectionController.m @@ -9,9 +9,20 @@ #import +@interface IGListBindingSingleSectionController () + +@end @implementation IGListBindingSingleSectionController { id _item; + __weak UICollectionViewCell *_displayingCell; +} + +- (instancetype)init { + if (self = [super init]) { + self.displayDelegate = self; + } + return self; } - (void)didSelectItemWithCell:(UICollectionViewCell *)cell { @@ -44,6 +55,26 @@ return CGSizeZero; } +#pragma mark - IGListDisplayDelegate + +- (void)listAdapter:(nonnull IGListAdapter *)listAdapter willDisplaySectionController:(nonnull IGListSectionController *)sectionController { + // no-op +} + +- (void)listAdapter:(nonnull IGListAdapter *)listAdapter didEndDisplayingSectionController:(nonnull IGListSectionController *)sectionController { + // no-op +} + +- (void)listAdapter:(nonnull IGListAdapter *)listAdapter willDisplaySectionController:(nonnull IGListSectionController *)sectionController cell:(nonnull UICollectionViewCell *)cell atIndex:(NSInteger)index { + IGParameterAssert(index == 0); + _displayingCell = cell; +} + +- (void)listAdapter:(nonnull IGListAdapter *)listAdapter didEndDisplayingSectionController:(nonnull IGListSectionController *)sectionController cell:(nonnull UICollectionViewCell *)cell atIndex:(NSInteger)index { + IGParameterAssert(index == 0); + _displayingCell = nil; +} + #pragma mark - IGListSectionController Overrides - (NSInteger)numberOfItems { @@ -66,9 +97,10 @@ - (void)didUpdateToObject:(id)object { _item = object; - UICollectionViewCell *cell = [self.collectionContext cellForItemAtIndex:0 sectionController:self]; - if (cell) { - [self configureCell:cell withViewModel:_item]; + if (_enabledCellConfigurationDuringUpdate) { + if (_displayingCell) { + [self configureCell:_displayingCell withViewModel:_item]; + } } }