Skip to content
Permalink
Browse files

Fix #1275 layouts inconsistency in `updateAnimated:completion` of IGL…

…istBindingSectionController (#1285)

Summary:
## Changes in this pull request

Issue fixed: #1275

This PR is straintforward solution as mentioned in #1275. When I finished it, I realized that it is not a problem in `IGListCollectionViewLayout Partial Optimization` and `IGListBindingSectionController` deserved it. Maybe `IGListCollectionViewLayout` is rarely used because of great builtin UICollectionViewFlowLayout or cells in section rarely need changing their sizes in practices.

Despite it is not `IGListCollectionViewLayout`'s fault, I think it can be more optimized against `invalidateLayoutWithContext`. I would like to make an another PR to optimize it.

Due to this PR just giving a proposed solution, it lacks of adding tests and changing log. When this solution is accepted, I would like to complete these todos.

### Checklist

- [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.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Pull Request resolved: #1285

Reviewed By: candance

Differential Revision: D15592807

Pulled By: lorixx

fbshipit-source-id: ae06abce896341509de4f3dfb73b3a7bc0a68c51
  • Loading branch information...
qhhonx authored and facebook-github-bot committed Jun 12, 2019
1 parent de08713 commit 08bf69cc2f22dc1ed184af94c18947af9e5d7184
@@ -26,6 +26,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag

### Fixes

- Fixed bug with layouts inconsistency in `updateAnimated:completion` of IGListBindingSectionController. [Qinghua Hong](https://github.com/xohozu) [(#1285)](https://github.com/Instagram/IGListKit/pull/1285)

- Fixed bug with `-[IGListAdapter scrollToObject:supplementaryKinds:scrollDirection:scrollPosition:animated:]` where the content inset(bottom/right) of the collection view was incorrectly being applied to the final offset and was inconsistent with the content inset(top/left) of the collection view being applied. [Qinghua Hong](https://github.com/xohozu) [(#1284)](https://github.com/Instagram/IGListKit/pull/1284)

- Fixed crash when the data source is nil before calling `-[IGListAdapterUpdater performUpdateWithCollectionViewBlock:fromObjects:toObjectsBlock:animated:objectTransitionBlock:completion:]`. [Zhisheng Huang](https://github.com/lorixx) (tbd)
@@ -92,6 +92,12 @@
13E1028C1FA4019000123403 /* IGListTestAdapterStackedReorderingDataSource.m in Sources */ = {isa = PBXBuildFile; fileRef = 13E1028B1FA4019000123403 /* IGListTestAdapterStackedReorderingDataSource.m */; };
13E1028D1FA4019000123403 /* IGListTestAdapterStackedReorderingDataSource.m in Sources */ = {isa = PBXBuildFile; fileRef = 13E1028B1FA4019000123403 /* IGListTestAdapterStackedReorderingDataSource.m */; };
13E102941FA414C400123403 /* IGListReorderableStackSectionControllerTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 13E102921FA414C400123403 /* IGListReorderableStackSectionControllerTests.m */; };
16B71CEB22B0A08400FE96ED /* IGTestInvalidateLayoutSectionController.m in Sources */ = {isa = PBXBuildFile; fileRef = 16B71CE722B0A08300FE96ED /* IGTestInvalidateLayoutSectionController.m */; };
16B71CEC22B0A08400FE96ED /* IGTestInvalidateLayoutSectionController.m in Sources */ = {isa = PBXBuildFile; fileRef = 16B71CE722B0A08300FE96ED /* IGTestInvalidateLayoutSectionController.m */; };
16B71CED22B0A08400FE96ED /* IGTestInvalidateLayoutDataSource.m in Sources */ = {isa = PBXBuildFile; fileRef = 16B71CE822B0A08300FE96ED /* IGTestInvalidateLayoutDataSource.m */; };
16B71CEE22B0A08400FE96ED /* IGTestInvalidateLayoutDataSource.m in Sources */ = {isa = PBXBuildFile; fileRef = 16B71CE822B0A08300FE96ED /* IGTestInvalidateLayoutDataSource.m */; };
16B71CEF22B0A08400FE96ED /* IGTestInvalidateLayoutObject.m in Sources */ = {isa = PBXBuildFile; fileRef = 16B71CEA22B0A08300FE96ED /* IGTestInvalidateLayoutObject.m */; };
16B71CF022B0A08400FE96ED /* IGTestInvalidateLayoutObject.m in Sources */ = {isa = PBXBuildFile; fileRef = 16B71CEA22B0A08300FE96ED /* IGTestInvalidateLayoutObject.m */; };
1F2984CA1E8039EC005FA211 /* IGListCollectionViewLayoutInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = 917E89871E800EE70015F934 /* IGListCollectionViewLayoutInternal.h */; settings = {ATTRIBUTES = (Private, ); }; };
262373C2015556E71A70FA30 /* Pods_IGListKit_tvOSTests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = E980179F5E885E417EB20D55 /* Pods_IGListKit_tvOSTests.framework */; };
26271C8A1DAE94E40073E116 /* IGTestSingleNibItemDataSource.m in Sources */ = {isa = PBXBuildFile; fileRef = 26271C891DAE94E40073E116 /* IGTestSingleNibItemDataSource.m */; };
@@ -475,6 +481,12 @@
13E1028A1FA4019000123403 /* IGListTestAdapterStackedReorderingDataSource.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = IGListTestAdapterStackedReorderingDataSource.h; sourceTree = "<group>"; };
13E1028B1FA4019000123403 /* IGListTestAdapterStackedReorderingDataSource.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = IGListTestAdapterStackedReorderingDataSource.m; sourceTree = "<group>"; };
13E102921FA414C400123403 /* IGListReorderableStackSectionControllerTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = IGListReorderableStackSectionControllerTests.m; sourceTree = "<group>"; };
16B71CE522B0A08300FE96ED /* IGTestInvalidateLayoutDataSource.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = IGTestInvalidateLayoutDataSource.h; sourceTree = "<group>"; };
16B71CE622B0A08300FE96ED /* IGTestInvalidateLayoutSectionController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = IGTestInvalidateLayoutSectionController.h; sourceTree = "<group>"; };
16B71CE722B0A08300FE96ED /* IGTestInvalidateLayoutSectionController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGTestInvalidateLayoutSectionController.m; sourceTree = "<group>"; };
16B71CE822B0A08300FE96ED /* IGTestInvalidateLayoutDataSource.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGTestInvalidateLayoutDataSource.m; sourceTree = "<group>"; };
16B71CE922B0A08300FE96ED /* IGTestInvalidateLayoutObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = IGTestInvalidateLayoutObject.h; sourceTree = "<group>"; };
16B71CEA22B0A08300FE96ED /* IGTestInvalidateLayoutObject.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGTestInvalidateLayoutObject.m; sourceTree = "<group>"; };
1AB7195278D0BBB5DA88D36F /* Pods_IGListKitTests.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_IGListKitTests.framework; sourceTree = BUILT_PRODUCTS_DIR; };
1D6BFC11D5380CB8311E1029 /* Pods-IGListKit-tvOSTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-IGListKit-tvOSTests.debug.xcconfig"; path = "Pods/Target Support Files/Pods-IGListKit-tvOSTests/Pods-IGListKit-tvOSTests.debug.xcconfig"; sourceTree = "<group>"; };
26271C881DAE94E40073E116 /* IGTestSingleNibItemDataSource.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = IGTestSingleNibItemDataSource.h; sourceTree = "<group>"; };
@@ -929,6 +941,12 @@
298DD9F91E3AE1AA00F76F50 /* IGTestDiffingObject.m */,
298DD9D01E3ADDB400F76F50 /* IGTestDiffingSectionController.h */,
298DD9D11E3ADDB400F76F50 /* IGTestDiffingSectionController.m */,
16B71CE522B0A08300FE96ED /* IGTestInvalidateLayoutDataSource.h */,
16B71CE822B0A08300FE96ED /* IGTestInvalidateLayoutDataSource.m */,
16B71CE922B0A08300FE96ED /* IGTestInvalidateLayoutObject.h */,
16B71CEA22B0A08300FE96ED /* IGTestInvalidateLayoutObject.m */,
16B71CE622B0A08300FE96ED /* IGTestInvalidateLayoutSectionController.h */,
16B71CE722B0A08300FE96ED /* IGTestInvalidateLayoutSectionController.m */,
2904861E1DCD02750007F41D /* IGTestNibSupplementaryView.h */,
2904861F1DCD02750007F41D /* IGTestNibSupplementaryView.m */,
298DD9E01E3ADE4300F76F50 /* IGTestNumberBindableCell.h */,
@@ -1590,9 +1608,12 @@
885FE23C1DC51B86009CE2B4 /* IGTestCell.m in Sources */,
298DDA001E3AE28000F76F50 /* IGTestDiffingObject.m in Sources */,
29C579331DE0DA8A003A149B /* IGTestStoryboardSupplementaryView.m in Sources */,
16B71CF022B0A08400FE96ED /* IGTestInvalidateLayoutObject.m in Sources */,
16B71CEC22B0A08400FE96ED /* IGTestInvalidateLayoutSectionController.m in Sources */,
2995409F1F588C9500F647CF /* IGTestBindingWithoutDeselectionDelegate.m in Sources */,
885FE2401DC51B86009CE2B4 /* IGTestSingleItemDataSource.m in Sources */,
885FE2451DC51B86009CE2B4 /* IGTestStoryboardCell.m in Sources */,
16B71CEE22B0A08400FE96ED /* IGTestInvalidateLayoutDataSource.m in Sources */,
298DD9CF1E3ADD1400F76F50 /* IGListBindingSectionControllerTests.m in Sources */,
885FE22F1DC51B76009CE2B4 /* IGListDiffSwiftTests.swift in Sources */,
885FE23F1DC51B86009CE2B4 /* IGTestObject.m in Sources */,
@@ -1718,6 +1739,7 @@
13DF01771FA1000E0092A320 /* IGTestReorderableSection.m in Sources */,
829D7BAA1DD1819000549816 /* IGListSectionMapTests.m in Sources */,
E56B7B3420A9D7100071010C /* IGListCollectionScrollingTraitsTests.m in Sources */,
16B71CEB22B0A08400FE96ED /* IGTestInvalidateLayoutSectionController.m in Sources */,
29C5792E1DE0DA89003A149B /* IGTestNibSupplementaryView.m in Sources */,
88144F101D870EDC007C7F66 /* IGListSingleSectionControllerTests.m in Sources */,
88144F121D870EDC007C7F66 /* IGListWorkingRangeHandlerTests.m in Sources */,
@@ -1730,6 +1752,8 @@
88144F1D1D870EDC007C7F66 /* IGTestSupplementarySource.m in Sources */,
E8D312E01FC472A60009FA2F /* IGListContentInsetTests.m in Sources */,
298DDA071E3AE2B100F76F50 /* IGTestStringBindableCell.m in Sources */,
16B71CED22B0A08400FE96ED /* IGTestInvalidateLayoutDataSource.m in Sources */,
16B71CEF22B0A08400FE96ED /* IGTestInvalidateLayoutObject.m in Sources */,
88144F081D870EDC007C7F66 /* IGListAdapterTests.m in Sources */,
8240C7F21DC284C300B3AAE7 /* IGListAdapterStoryboardTests.m in Sources */,
8240C7F01DC272CA00B3AAE7 /* IGTestStoryboardSupplementaryView.m in Sources */,
@@ -26,6 +26,8 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
IGListExperimentGetCollectionViewAtUpdate = 1 << 7,
/// Test skipping layout when UICollectionView isn't visible
IGListExperimentSkipLayout = 1 << 8,
/// Test invalidating layout when cell reloads/updates in IGListBindingSectionController.
IGListExperimentInvalidateLayoutForUpdates = 1 << 9,
};

/**
@@ -1196,6 +1196,24 @@ - (void)deleteInSectionController:(IGListSectionController *)sectionController a
[self _updateBackgroundViewShouldHide:![self _itemCountIsZero]];
}

- (void)invalidateLayoutInSectionController:(IGListSectionController *)sectionController atIndexes:(NSIndexSet *)indexes {
IGAssertMainThread();
IGParameterAssert(indexes != nil);
IGParameterAssert(sectionController != nil);
UICollectionView *collectionView = self.collectionView;
IGAssert(collectionView != nil, @"Invalidating items from %@ without a collection view at indexes %@.", sectionController, indexes);

if (indexes.count == 0) {
return;
}

NSArray *indexPaths = [self indexPathsFromSectionController:sectionController indexes:indexes usePreviousIfInUpdateBlock:NO];
UICollectionViewLayout *layout = collectionView.collectionViewLayout;
UICollectionViewLayoutInvalidationContext *context = [[[layout.class invalidationContextClass] alloc] init];
[context invalidateItemsAtIndexPaths:indexPaths];
[layout invalidateLayoutWithContext:context];
}

- (void)moveInSectionController:(IGListSectionController *)sectionController fromIndex:(NSInteger)fromIndex toIndex:(NSInteger)toIndex {
IGAssertMainThread();
IGParameterAssert(sectionController != nil);
@@ -47,6 +47,15 @@ NS_SWIFT_NAME(ListBatchContext)
- (void)deleteInSectionController:(IGListSectionController *)sectionController
atIndexes:(NSIndexSet *)indexes;

/**
Invalidates layouts of cells at specific in the section controller.
@param sectionController The section controller who's cells need invalidating.
@param indexes The indexes of items that need invalidating.
*/
- (void)invalidateLayoutInSectionController:(IGListSectionController *)sectionController
atIndexes:(NSIndexSet *)indexes;

/**
Moves a cell from one index to another within the section controller.
@@ -73,7 +73,9 @@ - (void)updateAnimated:(BOOL)animated completion:(void (^)(BOOL))completion {
}
}];


if (IGListExperimentEnabled(self.collectionContext.experiments, IGListExperimentInvalidateLayoutForUpdates)) {
[batchContext invalidateLayoutInSectionController:self atIndexes:result.updates];
}
[batchContext deleteInSectionController:self atIndexes:result.deletes];
[batchContext insertInSectionController:self atIndexes:result.inserts];

@@ -9,6 +9,7 @@

#import <IGListKit/IGListBatchContext.h>
#import <IGListKit/IGListCollectionScrollingTraits.h>
#import <IGListKit/IGListExperiments.h>

NS_ASSUME_NONNULL_BEGIN

@@ -46,6 +47,11 @@ NS_SWIFT_NAME(ListCollectionContext)
*/
@property (nonatomic, readonly) IGListCollectionScrollingTraits scrollingTraits;

/**
A bitmask of experiments to conduct on the section controller.
*/
@property (nonatomic, assign) IGListExperiment experiments;

/**
Returns size of the collection view relative to the section controller.
@@ -7,6 +7,7 @@

#import <IGListKit/IGListSectionController.h>

#import <IGListKit/IGListExperiments.h>
#import <IGListKit/IGListMacros.h>

NS_ASSUME_NONNULL_BEGIN
@@ -25,6 +26,11 @@ IGLK_SUBCLASSING_RESTRICTED
NS_SWIFT_NAME(ListStackedSectionController)
@interface IGListStackedSectionController : IGListSectionController

/**
A bitmask of experiments to conduct on the section controller.
*/
@property (nonatomic, assign) IGListExperiment experiments;

/**
Creates a new stacked section controller.
@@ -417,6 +417,11 @@ - (void)deleteInSectionController:(IGListSectionController *)sectionController a
[self.forwardingBatchContext deleteInSectionController:self atIndexes:itemIndexes];
}

- (void)invalidateLayoutInSectionController:(IGListSectionController *)sectionController atIndexes:(NSIndexSet *)indexes {
NSIndexSet *itemIndexes = [self _itemIndexesForSectionController:sectionController indexes:indexes];
[self.forwardingBatchContext invalidateLayoutInSectionController:sectionController atIndexes:itemIndexes];
}

- (void)moveInSectionController:(IGListSectionController *)sectionController fromIndex:(NSInteger)fromIndex toIndex:(NSInteger)toIndex {
[self reloadData];
const NSInteger fromRelativeIndex = [self _relativeIndexForSectionController:sectionController fromLocalIndex:fromIndex];
@@ -20,6 +20,8 @@
#import "IGTestCell.h"
#import "IGListTestCase.h"
#import "IGTestBindingWithoutDeselectionDelegate.h"
#import "IGTestInvalidateLayoutDataSource.h"
#import "IGTestInvalidateLayoutObject.h"

@interface IGListBindingSectionControllerTests : IGListTestCase

@@ -395,5 +397,73 @@ - (void)test_whenUpdating_withMutableArrayObject_thatViewModelsDontMutate {
XCTAssertNotEqualObjects(initObjects, section.viewModels);
}

- (void)test_whenUpdatingManully_withInvalidateLayoutForUpdates_thatCellSizeUpdatedToLatestSize_usingIGListCollectionViewLayout {
self.dataSource = [[IGTestInvalidateLayoutDataSource alloc] init];
self.adapter.dataSource = self.dataSource;

NSArray<IGTestObject *> *startingSection0 = @[
genInvalidateLayoutObject(@0, CGSizeMake(50, 30)),
genInvalidateLayoutObject(@1, CGSizeMake(50, 40)),
];
NSArray<IGTestObject *> *startingSection1 = @[
genInvalidateLayoutObject(@0, CGSizeMake(50, 50)),
genInvalidateLayoutObject(@1, CGSizeMake(50, 60))
];

IGListCollectionViewLayout *layout = [[IGListCollectionViewLayout alloc] initWithStickyHeaders:NO topContentInset:0 stretchToEdge:NO];
self.collectionView = [[UICollectionView alloc] initWithFrame:self.frame collectionViewLayout:layout];
[(IGListAdapterUpdater *)self.adapter.updater setAllowsBackgroundReloading:NO];
self.adapter.experiments |= IGListExperimentInvalidateLayoutForUpdates;

[self setupWithObjects:@[
[[IGTestInvalidateLayoutObject alloc] initWithKey:@0 objects:startingSection0],
[[IGTestInvalidateLayoutObject alloc] initWithKey:@1 objects:startingSection1]
]];

XCTAssertEqual([self.collectionView numberOfItemsInSection:0], 2);
XCTAssertEqual([self.collectionView numberOfItemsInSection:1], 2);

IGTestCell *cell00 = [self cellAtSection:0 item:0];
IGTestCell *cell01 = [self cellAtSection:0 item:1];
IGTestCell *cell10 = [self cellAtSection:1 item:0];
IGTestCell *cell11 = [self cellAtSection:1 item:1];

IGAssertEqualSize(cell00.frame.size, 50, 30);
IGAssertEqualSize(cell01.frame.size, 50, 40);
IGAssertEqualSize(cell10.frame.size, 50, 50);
IGAssertEqualSize(cell11.frame.size, 50, 60);

NSArray<IGTestObject *> *newSection0 = @[
genInvalidateLayoutObject(@0, CGSizeMake(45, 30)), // Width: 50 -> 45
genInvalidateLayoutObject(@1, CGSizeMake(50, 55)), // Height: 40 -> 55
];
NSArray<IGTestObject *> *newSection1 = @[
genInvalidateLayoutObject(@0, CGSizeMake(50, 50)), // No change
genInvalidateLayoutObject(@1, CGSizeMake(20, 30)), // Size: (50, 60) -> (20, 30)
];

self.dataSource.objects = @[
[[IGTestInvalidateLayoutObject alloc] initWithKey:@0 objects:newSection0],
[[IGTestInvalidateLayoutObject alloc] initWithKey:@1 objects:newSection1]
];

XCTestExpectation *expectation = [self expectationWithDescription:NSStringFromSelector(_cmd)];
[self.adapter performUpdatesAnimated:YES completion:^(BOOL finished) {
IGTestCell *updatedCell00 = [self cellAtSection:0 item:0];
IGTestCell *updatedCell01 = [self cellAtSection:0 item:1];
IGTestCell *nochangedCell10 = [self cellAtSection:1 item:0];
IGTestCell *updatedCell11 = [self cellAtSection:1 item:1];

IGAssertEqualSize(updatedCell00.frame.size, 45, 30);
IGAssertEqualSize(updatedCell01.frame.size, 50, 55);
IGAssertEqualSize(nochangedCell10.frame.size, 50, 50);
IGAssertEqualSize(updatedCell11.frame.size, 20, 30);

[expectation fulfill];
}];

[self waitForExpectationsWithTimeout:30 handler:nil];
}

@end

@@ -0,0 +1,20 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import <Foundation/Foundation.h>

#import <IGListKit/IGListAdapterDataSource.h>

#import "IGListTestCase.h"

@class IGTestInvalidateLayoutObject;

@interface IGTestInvalidateLayoutDataSource : NSObject <IGListTestCaseDataSource>

@property (nonatomic, strong) NSArray<IGTestInvalidateLayoutObject *> *objects;

@end
@@ -0,0 +1,27 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import "IGTestInvalidateLayoutDataSource.h"

#import "IGTestInvalidateLayoutObject.h"
#import "IGTestInvalidateLayoutSectionController.h"

@implementation IGTestInvalidateLayoutDataSource

#pragma mark - IGListAdapterDataSource

- (NSArray<id<IGListDiffable>> *)objectsForListAdapter:(IGListAdapter *)listAdapter {
return self.objects;
}

- (IGListSectionController *)listAdapter:(IGListAdapter *)listAdapter sectionControllerForObject:(id)object {
return [IGTestInvalidateLayoutSectionController new];
}

- (UIView *)emptyViewForListAdapter:(IGListAdapter *)listAdapter { return nil; }

@end

0 comments on commit 08bf69c

Please sign in to comment.
You can’t perform that action at this time.