Skip to content

Commit

Permalink
Weak reference collection view when queueing update
Browse files Browse the repository at this point in the history
Summary:
Found a "bug" when adding some new unit tests. The queue method holds a strong ref to the collection view, and doesn't release it until that block is executed. The adapter may have already been released, holding onto the collection view is wasteful.

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
Closes #930

Differential Revision: D5861975

Pulled By: rnystrom

fbshipit-source-id: 986ead725839d39b30daadf92675b397f8794520
  • Loading branch information
Ryan Nystrom authored and facebook-github-bot committed Sep 19, 2017
1 parent 3f7c7f3 commit d322c2e
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 2 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

The changelog for `IGListKit`. Also see the [releases](https://github.com/instagram/IGListKit/releases) on GitHub.

3.2.0 (upcoming release)
-----

### Fixes

- Weakly reference the `UICollectionView` in coalescence so that it can be released if the rest of system is destroyed. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd)


3.1.1
-----

Expand Down
10 changes: 8 additions & 2 deletions Source/IGListAdapterUpdater.m
Original file line number Diff line number Diff line change
Expand Up @@ -380,16 +380,22 @@ - (void)queueUpdateWithCollectionView:(UICollectionView *)collectionView {
}

__weak __typeof__(self) weakSelf = self;
__weak __typeof__(collectionView) weakCollectionView = collectionView;

// dispatch_async to give the main queue time to collect more batch updates so that a minimum amount of work
// (diffing, etc) is done on main. dispatch_async does not garauntee a full runloop turn will pass though.
// see -performUpdateWithCollectionView:fromObjects:toObjects:animated:]objectTransitionBlock:completion: for more
// details on how coalescence is done.
dispatch_async(dispatch_get_main_queue(), ^{
if (weakSelf.state != IGListBatchUpdateStateIdle
|| ![weakSelf hasChanges]) {
return;
}

if (weakSelf.hasQueuedReloadData) {
[weakSelf performReloadDataWithCollectionView:collectionView];
[weakSelf performReloadDataWithCollectionView:weakCollectionView];
} else {
[weakSelf performBatchUpdatesWithCollectionView:collectionView];
[weakSelf performBatchUpdatesWithCollectionView:weakCollectionView];
}
});
}
Expand Down
10 changes: 10 additions & 0 deletions Tests/IGListAdapterE2ETests.m
Original file line number Diff line number Diff line change
Expand Up @@ -1124,6 +1124,9 @@ - (void)test_whenDataSourceDeallocatedAfterUpdateQueued_thatUpdateSuccesfullyCom

- (void)test_whenQueuingUpdate_withSectionControllerBatchUpdate_thatSectionControllerNotRetained {
__weak id weakSectionController = nil;
__weak id weakAdapter = nil;
__weak id weakCollectionView = nil;

@autoreleasepool {
IGListAdapter *adapter = [[IGListAdapter alloc] initWithUpdater:[IGListAdapterUpdater new] viewController:nil];
IGTestDelegateDataSource *dataSource = [IGTestDelegateDataSource new];
Expand All @@ -1146,9 +1149,16 @@ - (void)test_whenQueuingUpdate_withSectionControllerBatchUpdate_thatSectionContr
dataSource.objects = @[object, genTestObject(@2, @2)];
[adapter performUpdatesAnimated:YES completion:^(BOOL finished) {}];

weakAdapter = adapter;
weakCollectionView = collectionView;
weakSectionController = section;

XCTAssertNotNil(weakAdapter);
XCTAssertNotNil(weakCollectionView);
XCTAssertNotNil(weakSectionController);
}
XCTAssertNil(weakAdapter);
XCTAssertNil(weakCollectionView);
XCTAssertNil(weakSectionController);
}

Expand Down

0 comments on commit d322c2e

Please sign in to comment.