From b5aa5e39002854c947e777c11ae241f67f24d19c Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Wed, 26 Oct 2016 11:47:08 -0700 Subject: [PATCH] Replace diff result API for batch updates Summary: Replacing the move+update API with a batch-updates-safe API on the diff results object. This makes using the diff results w/out the rest of IGListKit infra much easier when working with `UITableView` or `UICollectionView`. - Added unit tests - Removed outdated unit tests Reviewed By: dshahidehpour Differential Revision: D4065798 fbshipit-source-id: 30da8a7b483d56d5acc497da9320dc07a6d0b7ad --- CHANGELOG.md | 4 ++ IGListKit.xcodeproj/project.pbxproj | 4 -- Source/IGListDiff.mm | 2 +- Source/IGListIndexPathResult.h | 6 +- Source/IGListIndexPathResult.m | 14 +++- Source/IGListIndexSetResult.h | 6 +- Source/IGListIndexSetResult.m | 14 +++- Tests/IGListDiffTests.m | 62 +++++++++++++++++ Tests/IGListIndexResultTests.m | 100 ---------------------------- 9 files changed, 95 insertions(+), 117 deletions(-) delete mode 100644 Tests/IGListIndexResultTests.m diff --git a/CHANGELOG.md b/CHANGELOG.md index dea0dce88..069f1683a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag This release closes the [2.0.0 milestone](https://github.com/Instagram/IGListKit/milestone/1?closed=1). +### Breaking Changes + +- Diff result method `-resultWithUpdatedMovesAsDeleteInserts` removed and replaced with `-resultForBatchUpdates` + ### Enhancements - Added support for cells created from nibs. [Sven Bacia](https://github.com/svenbacia) [(#56)](https://github.com/Instagram/IGListKit/pull/56) diff --git a/IGListKit.xcodeproj/project.pbxproj b/IGListKit.xcodeproj/project.pbxproj index 11f96c307..37ec7bb69 100644 --- a/IGListKit.xcodeproj/project.pbxproj +++ b/IGListKit.xcodeproj/project.pbxproj @@ -35,7 +35,6 @@ 88144F0B1D870EDC007C7F66 /* IGListDiffSwiftTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 88144EE61D870EDC007C7F66 /* IGListDiffSwiftTests.swift */; }; 88144F0C1D870EDC007C7F66 /* IGListDiffTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 88144EE81D870EDC007C7F66 /* IGListDiffTests.m */; }; 88144F0D1D870EDC007C7F66 /* IGListDisplayHandlerTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 88144EE91D870EDC007C7F66 /* IGListDisplayHandlerTests.m */; }; - 88144F0E1D870EDC007C7F66 /* IGListIndexResultTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 88144EEA1D870EDC007C7F66 /* IGListIndexResultTests.m */; }; 88144F0F1D870EDC007C7F66 /* IGListObjectMapTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 88144EEC1D870EDC007C7F66 /* IGListObjectMapTests.m */; }; 88144F101D870EDC007C7F66 /* IGListSingleItemControllerTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 88144EED1D870EDC007C7F66 /* IGListSingleItemControllerTests.m */; }; 88144F111D870EDC007C7F66 /* IGListStackItemControllerTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 88144EEE1D870EDC007C7F66 /* IGListStackItemControllerTests.m */; }; @@ -148,7 +147,6 @@ 88144EE61D870EDC007C7F66 /* IGListDiffSwiftTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = IGListDiffSwiftTests.swift; sourceTree = ""; }; 88144EE81D870EDC007C7F66 /* IGListDiffTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGListDiffTests.m; sourceTree = ""; }; 88144EE91D870EDC007C7F66 /* IGListDisplayHandlerTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGListDisplayHandlerTests.m; sourceTree = ""; }; - 88144EEA1D870EDC007C7F66 /* IGListIndexResultTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGListIndexResultTests.m; sourceTree = ""; }; 88144EEB1D870EDC007C7F66 /* IGListKitTests-Bridging-Header.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "IGListKitTests-Bridging-Header.h"; sourceTree = ""; }; 88144EEC1D870EDC007C7F66 /* IGListObjectMapTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGListObjectMapTests.m; sourceTree = ""; }; 88144EED1D870EDC007C7F66 /* IGListSingleItemControllerTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = IGListSingleItemControllerTests.m; sourceTree = ""; }; @@ -420,7 +418,6 @@ 88144EE61D870EDC007C7F66 /* IGListDiffSwiftTests.swift */, 88144EE81D870EDC007C7F66 /* IGListDiffTests.m */, 88144EE91D870EDC007C7F66 /* IGListDisplayHandlerTests.m */, - 88144EEA1D870EDC007C7F66 /* IGListIndexResultTests.m */, 88144EEB1D870EDC007C7F66 /* IGListKitTests-Bridging-Header.h */, 88144EEC1D870EDC007C7F66 /* IGListObjectMapTests.m */, 88144EED1D870EDC007C7F66 /* IGListSingleItemControllerTests.m */, @@ -680,7 +677,6 @@ 88144F181D870EDC007C7F66 /* IGTestDelegateController.m in Sources */, 88144F0D1D870EDC007C7F66 /* IGListDisplayHandlerTests.m in Sources */, 88144F1B1D870EDC007C7F66 /* IGTestSingleItemDataSource.m in Sources */, - 88144F0E1D870EDC007C7F66 /* IGListIndexResultTests.m in Sources */, 88144F171D870EDC007C7F66 /* IGTestCell.m in Sources */, 821BC4C01DB8C9D500172ED0 /* IGListSingleStoryboardItemControllerTests.m in Sources */, 88144F141D870EDC007C7F66 /* IGListTestOffsettingLayout.m in Sources */, diff --git a/Source/IGListDiff.mm b/Source/IGListDiff.mm index 8d32df4e5..91dd718e7 100644 --- a/Source/IGListDiff.mm +++ b/Source/IGListDiff.mm @@ -224,7 +224,7 @@ static id IGListDiffing(BOOL returnIndexPaths, } else { // note that an entry can be updated /and/ moved if (record.entry->updated) { - addIndexToCollection(mUpdates, toSection, oldIndex); + addIndexToCollection(mUpdates, fromSection, oldIndex); } // calculate the offset and determine if there was a move diff --git a/Source/IGListIndexPathResult.h b/Source/IGListIndexPathResult.h index 1076cb68c..ac607b822 100644 --- a/Source/IGListIndexPathResult.h +++ b/Source/IGListIndexPathResult.h @@ -64,11 +64,9 @@ NS_ASSUME_NONNULL_BEGIN - (nullable NSIndexPath *)newIndexPathForIdentifier:(id)identifier; /** - Create a new result object transforming index paths that are both moved and updated into delete and inserts. - - @discussion This is a convenience method for using a result object to perform UICollectionView and UITableView updates. + Create a new result object with operations safe for use in UITableView and UICollectionView batch updates. */ -- (IGListIndexPathResult *)resultWithUpdatedMovesAsDeleteInserts; +- (IGListIndexPathResult *)resultForBatchUpdates; - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; diff --git a/Source/IGListIndexPathResult.m b/Source/IGListIndexPathResult.m index 15a0bd12f..9e5f74e0e 100644 --- a/Source/IGListIndexPathResult.m +++ b/Source/IGListIndexPathResult.m @@ -36,7 +36,7 @@ - (BOOL)hasChanges { return self.inserts.count || self.deletes.count || self.updates.count || self.moves.count; } -- (IGListIndexPathResult *)resultWithUpdatedMovesAsDeleteInserts { +- (IGListIndexPathResult *)resultForBatchUpdates { NSMutableSet *deletes = [self.deletes mutableCopy]; NSMutableSet *inserts = [self.inserts mutableCopy]; NSMutableSet *filteredUpdates = [self.updates mutableCopy]; @@ -44,6 +44,7 @@ - (IGListIndexPathResult *)resultWithUpdatedMovesAsDeleteInserts { NSArray *moves = self.moves; NSMutableArray *filteredMoves = [moves mutableCopy]; + // convert move+update to delete+insert, respecting the from/to of the move const NSUInteger moveCount = moves.count; for (NSInteger i = moveCount - 1; i >= 0; i--) { IGListMoveIndexPath *move = moves[i]; @@ -55,9 +56,18 @@ - (IGListIndexPathResult *)resultWithUpdatedMovesAsDeleteInserts { } } + // iterate all new identifiers. if its index is updated, delete from the old index and insert the new index + for (id key in [_oldIndexPathMap keyEnumerator]) { + NSIndexPath *indexPath = [_oldIndexPathMap objectForKey:key]; + if ([filteredUpdates containsObject:indexPath]) { + [deletes addObject:indexPath]; + [inserts addObject:(id)[_newIndexPathMap objectForKey:key]]; + } + } + return [[IGListIndexPathResult alloc] initWithInserts:[inserts allObjects] deletes:[deletes allObjects] - updates:[filteredUpdates allObjects] + updates:[NSArray new] moves:filteredMoves oldIndexPathMap:_oldIndexPathMap newIndexPathMap:_newIndexPathMap]; diff --git a/Source/IGListIndexSetResult.h b/Source/IGListIndexSetResult.h index 883f06034..d6c3dc174 100644 --- a/Source/IGListIndexSetResult.h +++ b/Source/IGListIndexSetResult.h @@ -64,11 +64,9 @@ NS_ASSUME_NONNULL_BEGIN - (NSUInteger)newIndexForIdentifier:(id)identifier; /** - Create a new result object transforming indexes that are both moved and updated into delete and inserts. - - @discussion This is a convenience method for using a result object to perform UICollectionView and UITableView updates. + Create a new result object with operations safe for use in UITableView and UICollectionView batch updates. */ -- (IGListIndexSetResult *)resultWithUpdatedMovesAsDeleteInserts; +- (IGListIndexSetResult *)resultForBatchUpdates; - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; diff --git a/Source/IGListIndexSetResult.m b/Source/IGListIndexSetResult.m index a898235b5..9efe78de5 100644 --- a/Source/IGListIndexSetResult.m +++ b/Source/IGListIndexSetResult.m @@ -38,7 +38,7 @@ - (BOOL)hasChanges { return self.inserts.count || self.deletes.count || self.updates.count || self.moves.count; } -- (IGListIndexSetResult *)resultWithUpdatedMovesAsDeleteInserts { +- (IGListIndexSetResult *)resultForBatchUpdates { NSMutableIndexSet *deletes = [self.deletes mutableCopy]; NSMutableIndexSet *inserts = [self.inserts mutableCopy]; NSMutableIndexSet *filteredUpdates = [self.updates mutableCopy]; @@ -46,6 +46,7 @@ - (IGListIndexSetResult *)resultWithUpdatedMovesAsDeleteInserts { NSArray *moves = self.moves; NSMutableArray *filteredMoves = [moves mutableCopy]; + // convert all update+move to delete+insert const NSUInteger moveCount = moves.count; for (NSInteger i = moveCount - 1; i >= 0; i--) { IGListMoveIndex *move = moves[i]; @@ -57,9 +58,18 @@ - (IGListIndexSetResult *)resultWithUpdatedMovesAsDeleteInserts { } } + // iterate all new identifiers. if its index is updated, delete from the old index and insert the new index + for (id key in [_oldIndexMap keyEnumerator]) { + const NSInteger index = [[_oldIndexMap objectForKey:key] integerValue]; + if ([filteredUpdates containsIndex:index]) { + [deletes addIndex:index]; + [inserts addIndex:[[_newIndexMap objectForKey:key] integerValue]]; + } + } + return [[IGListIndexSetResult alloc] initWithInserts:inserts deletes:deletes - updates:filteredUpdates + updates:[NSIndexSet new] moves:filteredMoves oldIndexMap:_oldIndexMap newIndexMap:_newIndexMap]; diff --git a/Tests/IGListDiffTests.m b/Tests/IGListDiffTests.m index 4359ec528..1e7763bfb 100644 --- a/Tests/IGListDiffTests.m +++ b/Tests/IGListDiffTests.m @@ -366,4 +366,66 @@ - (void)test_whenDiffing_thatNewIndexPathsMatch { XCTAssertEqualObjects([result newIndexPathForIdentifier:@9], [NSIndexPath indexPathForItem:1 inSection:1]); } +- (void)test_whenDiffing_withBatchUpdateResult_thatIndexesMatch { + NSArray *o = @[ + genTestObject(@1, @1), + genTestObject(@2, @1), + genTestObject(@3, @1), + genTestObject(@4, @1), + genTestObject(@5, @1), + genTestObject(@6, @1), + ]; + NSArray *n = @[ + // deleted + genTestObject(@2, @2), // updated + genTestObject(@5, @1), // moved + genTestObject(@4, @1), + genTestObject(@7, @1), // inserted + genTestObject(@6, @2), // updated + genTestObject(@3, @2), // moved+updated + ]; + IGListIndexSetResult *result = [IGListDiff(o, n, IGListDiffEquality) resultForBatchUpdates]; + XCTAssertEqual(result.updates.count, 0); + NSArray *expectedMoves = @[ [[IGListMoveIndex alloc] initWithFrom:4 to:1] ]; + XCTAssertEqualObjects(result.moves, expectedMoves); + NSMutableIndexSet *expectedDeletes = [NSMutableIndexSet indexSetWithIndex:0]; + [expectedDeletes addIndex:1]; + [expectedDeletes addIndex:2]; + [expectedDeletes addIndex:5]; + XCTAssertEqualObjects(result.deletes, expectedDeletes); + NSMutableIndexSet *expectedInserts = [NSMutableIndexSet indexSetWithIndex:0]; + [expectedInserts addIndex:3]; + [expectedInserts addIndex:4]; + [expectedInserts addIndex:5]; + XCTAssertEqualObjects(result.inserts, expectedInserts); +} + +- (void)test_whenDiffing_withBatchUpdateResult_thatIndexPathsMatch { + NSArray *o = @[ + genTestObject(@1, @1), + genTestObject(@2, @1), + genTestObject(@3, @1), + genTestObject(@4, @1), + genTestObject(@5, @1), + genTestObject(@6, @1), + ]; + NSArray *n = @[ + // deleted + genTestObject(@2, @2), // updated + genTestObject(@5, @1), // moved + genTestObject(@4, @1), + genTestObject(@7, @1), // inserted + genTestObject(@6, @2), // updated + genTestObject(@3, @2), // moved+updated + ]; + IGListIndexPathResult *result = [IGListDiffPaths(0, 1, o, n, IGListDiffEquality) resultForBatchUpdates]; + XCTAssertEqual(result.updates.count, 0); + NSArray *expectedMoves = @[ [[IGListMoveIndexPath alloc] initWithFrom:genIndexPath(4, 0) to:genIndexPath(1, 1)] ]; + XCTAssertEqualObjects(result.moves, expectedMoves); + NSArray *expectedDeletes = @[genIndexPath(0, 0), genIndexPath(1, 0), genIndexPath(2, 0), genIndexPath(5, 0)]; + XCTAssertEqualObjects(sorted(result.deletes), expectedDeletes); + NSArray *expectedInserts = @[genIndexPath(0, 1), genIndexPath(3, 1), genIndexPath(4, 1), genIndexPath(5, 1)]; + XCTAssertEqualObjects(sorted(result.inserts), expectedInserts); +} + @end diff --git a/Tests/IGListIndexResultTests.m b/Tests/IGListIndexResultTests.m deleted file mode 100644 index e9728ff25..000000000 --- a/Tests/IGListIndexResultTests.m +++ /dev/null @@ -1,100 +0,0 @@ -/** - * Copyright (c) 2016-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - */ - -#import - -#import "IGListIndexPathResultInternal.h" -#import "IGListIndexSetResultInternal.h" -#import "IGListMoveIndexInternal.h" -#import "IGListMoveIndexPathInternal.h" - -@interface IGListResultTests : XCTestCase - -@end - -@implementation IGListResultTests - -static NSIndexSet *indexSetWithIndexes(NSArray *indexes) { - NSMutableIndexSet *indexset = [NSMutableIndexSet new]; - for (NSNumber *i in indexes) { - [indexset addIndex:i.integerValue]; - } - return indexset; -} - -static NSIndexPath *indexPath(NSUInteger item, NSUInteger section) { - return [NSIndexPath indexPathForItem:item inSection:section]; -} - -- (void)testIndexSetResultConvertingUpdatesAndMoves { - IGListIndexSetResult *initResult = [[IGListIndexSetResult alloc] initWithInserts:indexSetWithIndexes(@[@2, @3]) - deletes:indexSetWithIndexes(@[@0]) - updates:indexSetWithIndexes(@[@1, @4]) - moves:@[ - [[IGListMoveIndex alloc] initWithFrom:1 to:5], - [[IGListMoveIndex alloc] initWithFrom:6 to:7] - ] - oldIndexMap:[NSMapTable new] - newIndexMap:[NSMapTable new] - ]; - IGListIndexSetResult *converted = [initResult resultWithUpdatedMovesAsDeleteInserts]; - NSIndexSet *expectedDeletes = indexSetWithIndexes(@[@0, @1]); - NSIndexSet *expectedInserts = indexSetWithIndexes(@[@2, @3, @5]); - NSIndexSet *expectedUpdates = indexSetWithIndexes(@[@4]); - NSArray *expectedMoves = @[ - [[IGListMoveIndex alloc] initWithFrom:6 to:7] - ]; - XCTAssertEqualObjects(converted.deletes, expectedDeletes); - XCTAssertEqualObjects(converted.inserts, expectedInserts); - XCTAssertEqualObjects(converted.updates, expectedUpdates); - XCTAssertEqualObjects(converted.moves, expectedMoves); -} - -- (void)testIndexPathResultConvertingUpdatesAndMoves { - IGListIndexPathResult *initResult = [[IGListIndexPathResult alloc] initWithInserts:@[ - indexPath(0, 1), - indexPath(1, 2), - ] - deletes:@[ - indexPath(0, 0), - ] - updates:@[ - indexPath(1, 1), - indexPath(2, 4), - ] - moves:@[ - [[IGListMoveIndexPath alloc] initWithFrom:indexPath(1, 1) to:indexPath(5, 1)], - [[IGListMoveIndexPath alloc] initWithFrom:indexPath(6, 0) to:indexPath(7, 0)], - ] - oldIndexPathMap:[NSMapTable new] - newIndexPathMap:[NSMapTable new] - ]; - IGListIndexPathResult *converted = [initResult resultWithUpdatedMovesAsDeleteInserts]; - NSArray *expectedDeletes = @[ - indexPath(0, 0), - indexPath(1, 1), - ]; - NSArray *expectedInserts = @[ - indexPath(0, 1), - indexPath(1, 2), - indexPath(5, 1), - ]; - NSArray *expectedUpdates = @[ - indexPath(2, 4), - ]; - NSArray *expectedMoves = @[ - [[IGListMoveIndexPath alloc] initWithFrom:indexPath(6, 0) to:indexPath(7, 0)], - ]; - XCTAssertEqualObjects([converted.deletes sortedArrayUsingSelector:@selector(compare:)], [expectedDeletes sortedArrayUsingSelector:@selector(compare:)]); - XCTAssertEqualObjects([converted.inserts sortedArrayUsingSelector:@selector(compare:)], [expectedInserts sortedArrayUsingSelector:@selector(compare:)]); - XCTAssertEqualObjects([converted.updates sortedArrayUsingSelector:@selector(compare:)], [expectedUpdates sortedArrayUsingSelector:@selector(compare:)]); - XCTAssertEqualObjects([converted.moves sortedArrayUsingSelector:@selector(compare:)], [expectedMoves sortedArrayUsingSelector:@selector(compare:)]); -} - -@end