From 6b3f0ada120bd1fad355a59ce893a62c83b8cf0b Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 24 Nov 2015 18:10:59 -0800 Subject: [PATCH 1/6] Fix table view change coalescing bug --- AsyncDisplayKit/Private/_ASHierarchyChangeSet.m | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m index ca6040f18c..d35748c849 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m @@ -219,9 +219,11 @@ __block ASDataControllerAnimationOptions currentOptions = 0; __block NSMutableIndexSet *currentIndexes = nil; - NSUInteger lastIndex = allIndexes.lastIndex; - - NSEnumerationOptions options = type == _ASHierarchyChangeTypeDelete ? NSEnumerationReverse : kNilOptions; + + BOOL reverse = (type == _ASHierarchyChangeTypeDelete); + NSEnumerationOptions options = reverse ? NSEnumerationReverse : kNilOptions; + NSUInteger endIndex = reverse ? allIndexes.firstIndex : allIndexes.lastIndex; + [allIndexes enumerateIndexesWithOptions:options usingBlock:^(NSUInteger idx, __unused BOOL * stop) { ASDataControllerAnimationOptions options = [animationOptions[@(idx)] integerValue]; BOOL endingCurrentGroup = NO; @@ -237,7 +239,7 @@ endingCurrentGroup = YES; } - BOOL endingLastGroup = (currentIndexes != nil && lastIndex == idx); + BOOL endingLastGroup = (currentIndexes != nil && endIndex == idx); if (endingCurrentGroup || endingLastGroup) { _ASHierarchySectionChange *change = [[_ASHierarchySectionChange alloc] initWithChangeType:type indexSet:currentIndexes animationOptions:currentOptions]; From 2d633f118bddd4b9abeefc87dcac41122d97a8ed Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 24 Nov 2015 18:44:10 -0800 Subject: [PATCH 2/6] Fix it better --- .../Private/_ASHierarchyChangeSet.m | 91 +++++++++---------- 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m index d35748c849..c4d9af059f 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m @@ -217,38 +217,35 @@ // Create new changes by grouping sorted changes by animation option NSMutableArray *result = [NSMutableArray new]; - __block ASDataControllerAnimationOptions currentOptions = 0; - __block NSMutableIndexSet *currentIndexes = nil; + __block ASDataControllerAnimationOptions currentOptions = NSUIntegerMax; + __block NSMutableIndexSet *currentIndexes = [NSMutableIndexSet indexSet]; - BOOL reverse = (type == _ASHierarchyChangeTypeDelete); - NSEnumerationOptions options = reverse ? NSEnumerationReverse : kNilOptions; - NSUInteger endIndex = reverse ? allIndexes.firstIndex : allIndexes.lastIndex; + NSEnumerationOptions options = type == _ASHierarchyChangeTypeDelete ? NSEnumerationReverse : kNilOptions; [allIndexes enumerateIndexesWithOptions:options usingBlock:^(NSUInteger idx, __unused BOOL * stop) { ASDataControllerAnimationOptions options = [animationOptions[@(idx)] integerValue]; - BOOL endingCurrentGroup = NO; - - if (currentIndexes == nil) { - // Starting a new group - currentIndexes = [NSMutableIndexSet indexSetWithIndex:idx]; - currentOptions = options; - } else if (options == currentOptions) { - // Continuing the current group - [currentIndexes addIndex:idx]; - } else { - endingCurrentGroup = YES; - } - - BOOL endingLastGroup = (currentIndexes != nil && endIndex == idx); - - if (endingCurrentGroup || endingLastGroup) { - _ASHierarchySectionChange *change = [[_ASHierarchySectionChange alloc] initWithChangeType:type indexSet:currentIndexes animationOptions:currentOptions]; + + // End the previous group if needed. + if (options != currentOptions && currentIndexes.count > 0) { + _ASHierarchySectionChange *change = [[_ASHierarchySectionChange alloc] initWithChangeType:type indexSet:[currentIndexes copy] animationOptions:currentOptions]; [result addObject:change]; - currentOptions = 0; - currentIndexes = nil; + [currentIndexes removeAllIndexes]; } + + // Start a new group if needed. + if (currentIndexes.count == 0) { + currentOptions = options; + } + + [currentIndexes addIndex:idx]; }]; - + + // Finish up the last group. + if (currentIndexes.count > 0) { + _ASHierarchySectionChange *change = [[_ASHierarchySectionChange alloc] initWithChangeType:type indexSet:[currentIndexes copy] animationOptions:currentOptions]; + [result addObject:change]; + } + [changes setArray:result]; } @@ -303,36 +300,34 @@ // Create new changes by grouping sorted changes by animation option NSMutableArray *result = [NSMutableArray new]; - + ASDataControllerAnimationOptions currentOptions = 0; - NSMutableArray *currentIndexPaths = nil; - NSIndexPath *lastIndexPath = allIndexPaths.lastObject; - + NSMutableArray *currentIndexPaths = [NSMutableArray array]; + for (NSIndexPath *indexPath in allIndexPaths) { ASDataControllerAnimationOptions options = [animationOptions[indexPath] integerValue]; - BOOL endingCurrentGroup = NO; - - if (currentIndexPaths == nil) { - // Starting a new group - currentIndexPaths = [NSMutableArray arrayWithObject:indexPath]; - currentOptions = options; - } else if (options == currentOptions) { - // Continuing the current group - [currentIndexPaths addObject:indexPath]; - } else { - endingCurrentGroup = YES; - } - - BOOL endingLastGroup = (currentIndexPaths != nil && (NSOrderedSame == [lastIndexPath compare:indexPath])); - if (endingCurrentGroup || endingLastGroup) { - _ASHierarchyItemChange *change = [[_ASHierarchyItemChange alloc] initWithChangeType:type indexPaths:currentIndexPaths animationOptions:currentOptions presorted:YES]; + // End the previous group if needed. + if (options != currentOptions && currentIndexPaths.count > 0) { + _ASHierarchyItemChange *change = [[_ASHierarchyItemChange alloc] initWithChangeType:type indexPaths:[currentIndexPaths copy] animationOptions:currentOptions presorted:YES]; [result addObject:change]; - currentOptions = 0; - currentIndexPaths = nil; + [currentIndexPaths removeAllObjects]; } + + // Start a new group if needed. + if (currentIndexPaths.count == 0) { + currentOptions = options; + } + + [currentIndexPaths addObject:indexPath]; } - + + // Finish up the last group. + if (currentIndexPaths.count > 0) { + _ASHierarchyItemChange *change = [[_ASHierarchyItemChange alloc] initWithChangeType:type indexPaths:[currentIndexPaths copy] animationOptions:currentOptions presorted:YES]; + [result addObject:change]; + } + [changes setArray:result]; } From b5e19a24a916c740da19c59350bf2cf316403d9b Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 24 Nov 2015 18:48:07 -0800 Subject: [PATCH 3/6] Consistency --- AsyncDisplayKit/Private/_ASHierarchyChangeSet.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m index c4d9af059f..cc5203f01d 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m @@ -217,7 +217,7 @@ // Create new changes by grouping sorted changes by animation option NSMutableArray *result = [NSMutableArray new]; - __block ASDataControllerAnimationOptions currentOptions = NSUIntegerMax; + __block ASDataControllerAnimationOptions currentOptions = 0; __block NSMutableIndexSet *currentIndexes = [NSMutableIndexSet indexSet]; NSEnumerationOptions options = type == _ASHierarchyChangeTypeDelete ? NSEnumerationReverse : kNilOptions; From 4bfdd8256891fa84d0336dad7ca8e1eb57d9890e Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Wed, 25 Nov 2015 17:18:58 -0800 Subject: [PATCH 4/6] _ASHierarchyChangeSet: Exclude item-level changes that are redundant with section-level changes --- .../Private/_ASHierarchyChangeSet.h | 7 ---- .../Private/_ASHierarchyChangeSet.m | 41 +++++++++++-------- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h index fa67235957..8c3ece4a17 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h @@ -34,13 +34,6 @@ typedef NS_ENUM(NSInteger, _ASHierarchyChangeType) { @interface _ASHierarchyChangeSet : NSObject -@property (nonatomic, strong, readonly) NSIndexSet *deletedSections; -@property (nonatomic, strong, readonly) NSIndexSet *insertedSections; -@property (nonatomic, strong, readonly) NSIndexSet *reloadedSections; -@property (nonatomic, strong, readonly) NSArray *insertedItems; -@property (nonatomic, strong, readonly) NSArray *deletedItems; -@property (nonatomic, strong, readonly) NSArray *reloadedItems; - @property (nonatomic, readonly) BOOL completed; /// Call this once the change set has been constructed to prevent future modifications to the changeset. Calling this more than once is a programmer error. diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m index cc5203f01d..9407eca0d1 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m @@ -17,6 +17,9 @@ Assumes: `changes` is [_ASHierarchySectionChange] all with the same changeType */ + (void)sortAndCoalesceChanges:(NSMutableArray *)changes; + +/// Returns all the indexes from all the `indexSet`s of the given `_ASHierarchySectionChange` objects. ++ (NSMutableIndexSet *)allIndexesInChanges:(NSArray *)changes; @end @interface _ASHierarchyItemChange () @@ -40,26 +43,12 @@ @end -@implementation _ASHierarchyChangeSet { - NSMutableIndexSet *_deletedSections; - NSMutableIndexSet *_insertedSections; - NSMutableIndexSet *_reloadedSections; - NSMutableArray *_insertedItems; - NSMutableArray *_deletedItems; - NSMutableArray *_reloadedItems; -} +@implementation _ASHierarchyChangeSet - (instancetype)init { self = [super init]; if (self) { - _deletedSections = [NSMutableIndexSet new]; - _insertedSections = [NSMutableIndexSet new]; - _reloadedSections = [NSMutableIndexSet new]; - - _deletedItems = [NSMutableArray new]; - _insertedItems = [NSMutableArray new]; - _reloadedItems = [NSMutableArray new]; _insertItemChanges = [NSMutableArray new]; _deleteItemChanges = [NSMutableArray new]; @@ -172,9 +161,16 @@ [_ASHierarchySectionChange sortAndCoalesceChanges:_deleteSectionChanges]; [_ASHierarchySectionChange sortAndCoalesceChanges:_insertSectionChanges]; [_ASHierarchySectionChange sortAndCoalesceChanges:_reloadSectionChanges]; - [_ASHierarchyItemChange sortAndCoalesceChanges:_deleteItemChanges ignoringChangesInSections:_deletedSections]; - [_ASHierarchyItemChange sortAndCoalesceChanges:_reloadItemChanges ignoringChangesInSections:_reloadedSections]; - [_ASHierarchyItemChange sortAndCoalesceChanges:_insertItemChanges ignoringChangesInSections:_insertedSections]; + + // Item deletes in deleted sections need not be reported. + NSIndexSet *deletedSections = [_ASHierarchySectionChange allIndexesInChanges:_deleteSectionChanges]; + [_ASHierarchyItemChange sortAndCoalesceChanges:_deleteItemChanges ignoringChangesInSections:deletedSections]; + + // Item reloads/inserts in reloaded/inserted sections need not be reported. + NSMutableIndexSet *reloadedAndInsertedSections = [_ASHierarchySectionChange allIndexesInChanges:_reloadSectionChanges]; + [reloadedAndInsertedSections addIndexes:[_ASHierarchySectionChange allIndexesInChanges:_insertSectionChanges]]; + [_ASHierarchyItemChange sortAndCoalesceChanges:_reloadItemChanges ignoringChangesInSections:reloadedAndInsertedSections]; + [_ASHierarchyItemChange sortAndCoalesceChanges:_insertItemChanges ignoringChangesInSections:reloadedAndInsertedSections]; } } @@ -249,6 +245,15 @@ [changes setArray:result]; } ++ (NSMutableIndexSet *)allIndexesInChanges:(NSArray *)changes +{ + NSMutableIndexSet *indexes = [NSMutableIndexSet indexSet]; + for (_ASHierarchySectionChange *change in changes) { + [indexes addIndexes:change.indexSet]; + } + return indexes; +} + @end @implementation _ASHierarchyItemChange From 41ecaa7bb23be2f0e0e13e88fb51423fbf299d58 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Thu, 26 Nov 2015 10:42:35 -0800 Subject: [PATCH 5/6] _ASHierarchyChangeSet: add section change mapping methods, fix section exclusions --- .../Private/_ASHierarchyChangeSet.h | 15 +++++++ .../Private/_ASHierarchyChangeSet.m | 43 +++++++++++++++---- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h index 8c3ece4a17..b1f39cbf15 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.h @@ -34,6 +34,21 @@ typedef NS_ENUM(NSInteger, _ASHierarchyChangeType) { @interface _ASHierarchyChangeSet : NSObject +/// @precondition The change set must be completed. +@property (nonatomic, strong, readonly) NSIndexSet *deletedSections; +/// @precondition The change set must be completed. +@property (nonatomic, strong, readonly) NSIndexSet *insertedSections; +/// @precondition The change set must be completed. +@property (nonatomic, strong, readonly) NSIndexSet *reloadedSections; + +/** + Get the section index after the update for the given section before the update. + + @precondition The change set must be completed. + @returns The new section index, or NSNotFound if the given section was deleted. + */ +- (NSInteger)newSectionForOldSection:(NSInteger)oldSection; + @property (nonatomic, readonly) BOOL completed; /// Call this once the change set has been constructed to prevent future modifications to the changeset. Calling this more than once is a programmer error. diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m index 9407eca0d1..877413c649 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m @@ -99,6 +99,17 @@ } } +- (NSInteger)newSectionForOldSection:(NSInteger)oldSection +{ + [self _ensureCompleted]; + if ([_deletedSections containsIndex:oldSection]) { + return NSNotFound; + } + + NSInteger indexAfterDeletes = oldSection - [_deletedSections countOfIndexesInRange:NSMakeRange(0, oldSection)]; + return indexAfterDeletes + [_insertedSections countOfIndexesInRange:NSMakeRange(0, indexAfterDeletes)]; +} + - (void)deleteItems:(NSArray *)indexPaths animationOptions:(ASDataControllerAnimationOptions)options { [self _ensureNotCompleted]; @@ -162,15 +173,31 @@ [_ASHierarchySectionChange sortAndCoalesceChanges:_insertSectionChanges]; [_ASHierarchySectionChange sortAndCoalesceChanges:_reloadSectionChanges]; - // Item deletes in deleted sections need not be reported. - NSIndexSet *deletedSections = [_ASHierarchySectionChange allIndexesInChanges:_deleteSectionChanges]; - [_ASHierarchyItemChange sortAndCoalesceChanges:_deleteItemChanges ignoringChangesInSections:deletedSections]; + _deletedSections = [[_ASHierarchySectionChange allIndexesInChanges:_deleteSectionChanges] copy]; + _insertedSections = [[_ASHierarchySectionChange allIndexesInChanges:_insertSectionChanges] copy]; + _reloadedSections = [[_ASHierarchySectionChange allIndexesInChanges:_reloadSectionChanges] copy]; - // Item reloads/inserts in reloaded/inserted sections need not be reported. - NSMutableIndexSet *reloadedAndInsertedSections = [_ASHierarchySectionChange allIndexesInChanges:_reloadSectionChanges]; - [reloadedAndInsertedSections addIndexes:[_ASHierarchySectionChange allIndexesInChanges:_insertSectionChanges]]; - [_ASHierarchyItemChange sortAndCoalesceChanges:_reloadItemChanges ignoringChangesInSections:reloadedAndInsertedSections]; - [_ASHierarchyItemChange sortAndCoalesceChanges:_insertItemChanges ignoringChangesInSections:reloadedAndInsertedSections]; + // These are invalid old section indexes. + NSMutableIndexSet *deletedOrReloaded = [_deletedSections mutableCopy]; + [deletedOrReloaded addIndexes:_reloadedSections]; + + // These are invalid new section indexes. + NSMutableIndexSet *insertedOrReloaded = [_insertedSections mutableCopy]; + + // Get the new section that each reloaded section index corresponds to. + [_reloadedSections enumerateIndexesUsingBlock:^(NSUInteger oldIndex, __unused BOOL * stop) { + NSUInteger newIndex = [self newSectionForOldSection:oldIndex]; + if (newIndex != NSNotFound) { + [insertedOrReloaded addIndex:newIndex]; + } + }]; + + // Ignore item reloads/deletes in reloaded/deleted sections. + [_ASHierarchyItemChange sortAndCoalesceChanges:_deleteItemChanges ignoringChangesInSections:deletedOrReloaded]; + [_ASHierarchyItemChange sortAndCoalesceChanges:_reloadItemChanges ignoringChangesInSections:deletedOrReloaded]; + + // Ignore item inserts in reloaded(new)/inserted sections. + [_ASHierarchyItemChange sortAndCoalesceChanges:_insertItemChanges ignoringChangesInSections:insertedOrReloaded]; } } From d273cda2b0b93d76b58fb0f9760952c3ab6c2df7 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Thu, 26 Nov 2015 10:46:26 -0800 Subject: [PATCH 6/6] Remove unneeded _block specifier --- AsyncDisplayKit/Private/_ASHierarchyChangeSet.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m index 877413c649..bd820e9cd6 100644 --- a/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m +++ b/AsyncDisplayKit/Private/_ASHierarchyChangeSet.m @@ -241,7 +241,7 @@ NSMutableArray *result = [NSMutableArray new]; __block ASDataControllerAnimationOptions currentOptions = 0; - __block NSMutableIndexSet *currentIndexes = [NSMutableIndexSet indexSet]; + NSMutableIndexSet *currentIndexes = [NSMutableIndexSet indexSet]; NSEnumerationOptions options = type == _ASHierarchyChangeTypeDelete ? NSEnumerationReverse : kNilOptions;