From 77745c55b084c51d57a405073a6e9b00c379f918 Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Sun, 4 Oct 2015 17:11:41 -0700 Subject: [PATCH 01/36] Properly support operating with nil asyncDelegate for Table & Collection. --- AsyncDisplayKit/ASCollectionView.mm | 29 ++++++++++++++++++----------- AsyncDisplayKit/ASTableView.mm | 26 ++++++++++++++++---------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index b1b29335b3..4287e39075 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -61,12 +61,14 @@ static BOOL _isInterceptedSelector(SEL sel) */ @interface _ASCollectionViewProxy : NSProxy - (instancetype)initWithTarget:(id)target interceptor:(ASCollectionView *)interceptor; +@property (nonatomic, weak) id target; @end @implementation _ASCollectionViewProxy { id __weak _target; ASCollectionView * __weak _interceptor; } +@synthesize target = _target; - (instancetype)initWithTarget:(id)target interceptor:(ASCollectionView *)interceptor { @@ -202,6 +204,14 @@ static BOOL _isInterceptedSelector(SEL sel) // and should not trigger a relayout. _ignoreMaxSizeChange = CGSizeEqualToSize(_maxSizeForNodesConstrainedSize, CGSizeZero); + // Set up the delegate / dataSource proxy now, so we recieve key method calls from UITableView even if + // our owner never sets up asyncDelegate (technically the dataSource is required) + _proxyDelegate = [[_ASCollectionViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; + super.delegate = (id)_proxyDelegate; + + _proxyDataSource = [[_ASCollectionViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; + super.dataSource = (id)_proxyDataSource; + self.backgroundColor = [UIColor whiteColor]; [self registerClass:[UICollectionViewCell class] forCellWithReuseIdentifier:@"_ASCollectionViewCell"]; @@ -252,22 +262,21 @@ static BOOL _isInterceptedSelector(SEL sel) // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDataSource in the ViewController's dealloc. In this case our _asyncDataSource // will return as nil (ARC magic) even though the _proxyDataSource still exists. It's really important to nil out - // super.dataSource in this case because calls to _ASTableViewProxy will start failing and cause crashes. + // _proxyDataSource.target in this case because calls to _ASCollectionViewProxy will start failing and cause crashes. if (asyncDataSource == nil) { - super.dataSource = nil; + _proxyDataSource.target = nil; _asyncDataSource = nil; - _proxyDataSource = nil; _asyncDataSourceImplementsConstrainedSizeForNode = NO; } else { + _proxyDataSource.target = asyncDataSource; _asyncDataSource = asyncDataSource; + _asyncDataSourceImplementsConstrainedSizeForNode = ([_asyncDataSource respondsToSelector:@selector(collectionView:constrainedSizeForNodeAtIndexPath:)] ? 1 : 0); + // TODO: Support supplementary views with ASCollectionView. if ([_asyncDataSource respondsToSelector:@selector(collectionView:viewForSupplementaryElementOfKind:atIndexPath:)]) { ASDisplayNodeAssert(NO, @"ASCollectionView is planned to support supplementary views by September 2015. You can work around this issue by using standard items."); } - _proxyDataSource = [[_ASCollectionViewProxy alloc] initWithTarget:_asyncDataSource interceptor:self]; - super.dataSource = (id)_proxyDataSource; - _asyncDataSourceImplementsConstrainedSizeForNode = ([_asyncDataSource respondsToSelector:@selector(collectionView:constrainedSizeForNodeAtIndexPath:)] ? 1 : 0); } } @@ -276,19 +285,17 @@ static BOOL _isInterceptedSelector(SEL sel) // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDelegate in the ViewController's dealloc. In this case our _asyncDelegate // will return as nil (ARC magic) even though the _proxyDelegate still exists. It's really important to nil out - // super.delegate in this case because calls to _ASTableViewProxy will start failing and cause crashes. + // _proxyDelegate.target in this case because calls to _ASCollectionViewProxy will start failing and cause crashes. if (asyncDelegate == nil) { // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes // in UIScrollViewAccessibility. - super.delegate = nil; + _proxyDelegate.target = nil; _asyncDelegate = nil; - _proxyDelegate = nil; _asyncDelegateImplementsInsetSection = NO; } else { + _proxyDelegate.target = asyncDelegate; _asyncDelegate = asyncDelegate; - _proxyDelegate = [[_ASCollectionViewProxy alloc] initWithTarget:_asyncDelegate interceptor:self]; - super.delegate = (id)_proxyDelegate; _asyncDelegateImplementsInsetSection = ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:insetForSectionAtIndex:)] ? 1 : 0); } } diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index 7c36c775a6..6d83de9a95 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -59,12 +59,14 @@ static BOOL _isInterceptedSelector(SEL sel) */ @interface _ASTableViewProxy : NSProxy - (instancetype)initWithTarget:(id)target interceptor:(ASTableView *)interceptor; +@property (nonatomic, weak) id target; @end @implementation _ASTableViewProxy { id __weak _target; ASTableView * __weak _interceptor; } +@synthesize target = _target; - (instancetype)initWithTarget:(id)target interceptor:(ASTableView *)interceptor { @@ -218,6 +220,14 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // If the initial size is 0, expect a size change very soon which is part of the initial configuration // and should not trigger a relayout. _ignoreMaxWidthChange = (_maxWidthForNodesConstrainedSize == 0); + + // Set up the delegate / dataSource proxy now, so we recieve key method calls from UITableView even if + // our owner never sets up asyncDelegate (technically the dataSource is required) + _proxyDelegate = [[_ASTableViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; + super.delegate = (id)_proxyDelegate; + + _proxyDataSource = [[_ASTableViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; + super.dataSource = (id)_proxyDataSource; } - (instancetype)initWithFrame:(CGRect)frame style:(UITableViewStyle)style @@ -277,16 +287,14 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDataSource in the ViewController's dealloc. In this case our _asyncDataSource // will return as nil (ARC magic) even though the _proxyDataSource still exists. It's really important to nil out - // super.dataSource in this case because calls to _ASTableViewProxy will start failing and cause crashes. + // _proxyDataSource.target in this case because calls to _ASTableViewProxy will start failing and cause crashes. if (asyncDataSource == nil) { - super.dataSource = nil; + _proxyDataSource.target = nil; _asyncDataSource = nil; - _proxyDataSource = nil; } else { + _proxyDataSource.target = asyncDataSource; _asyncDataSource = asyncDataSource; - _proxyDataSource = [[_ASTableViewProxy alloc] initWithTarget:_asyncDataSource interceptor:self]; - super.dataSource = (id)_proxyDataSource; } } @@ -295,18 +303,16 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDelegate in the ViewController's dealloc. In this case our _asyncDelegate // will return as nil (ARC magic) even though the _proxyDelegate still exists. It's really important to nil out - // super.delegate in this case because calls to _ASTableViewProxy will start failing and cause crashes. + // _proxyDelegate.target in this case because calls to _ASTableViewProxy will start failing and cause crashes. if (asyncDelegate == nil) { // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes // in UIScrollViewAccessibility. - super.delegate = nil; + _proxyDelegate.target = nil; _asyncDelegate = nil; - _proxyDelegate = nil; } else { + _proxyDelegate.target = asyncDelegate; _asyncDelegate = asyncDelegate; - _proxyDelegate = [[_ASTableViewProxy alloc] initWithTarget:asyncDelegate interceptor:self]; - super.delegate = (id)_proxyDelegate; } } From d6c107a8966b37a8a8cb74505e17565a93a1be90 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 5 Oct 2015 13:26:15 -0700 Subject: [PATCH 02/36] Fix shouldRasterizeSubnodes skipping setting offset and clipping. --- .../Private/ASDisplayNode+AsyncDisplay.mm | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm index 98b1c2b6e6..89115e33a9 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm @@ -117,8 +117,8 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, // Get the display block for this node. asyncdisplaykit_async_transaction_operation_block_t displayBlock = [self _displayBlockWithAsynchronous:NO isCancelledBlock:isCancelledBlock rasterizing:YES]; - // We'll display something if there is a display block and/or a background color. - BOOL shouldDisplay = displayBlock || backgroundColor; + // We'll display something if there is a display block, clipping, translation and/or a background color. + BOOL shouldDisplay = displayBlock || backgroundColor || CGPointEqualToPoint(CGPointZero, frame.origin) == NO || clipsToBounds; // If we should display, then push a transform, draw the background color, and draw the contents. // The transform is popped in a block added after the recursion into subnodes. @@ -131,8 +131,12 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, CGContextTranslateCTM(context, frame.origin.x, frame.origin.y); //support cornerRadius - if (rasterizingFromAscendent && cornerRadius && clipsToBounds) { - [[UIBezierPath bezierPathWithRoundedRect:bounds cornerRadius:self.cornerRadius] addClip]; + if (clipsToBounds) { + if (cornerRadius) { + [[UIBezierPath bezierPathWithRoundedRect:bounds cornerRadius:self.cornerRadius] addClip]; + } else { + [[UIBezierPath bezierPathWithRect:bounds] addClip]; + } } // Fill background if any. From b1d03f1d58e8c38b516d13c3b104cecd55ae6fed Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 5 Oct 2015 14:29:41 -0700 Subject: [PATCH 03/36] Address comments --- AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm index 89115e33a9..e74cd00e35 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm @@ -131,9 +131,9 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, CGContextTranslateCTM(context, frame.origin.x, frame.origin.y); //support cornerRadius - if (clipsToBounds) { + if (rasterizingFromAscendent && clipsToBounds) { if (cornerRadius) { - [[UIBezierPath bezierPathWithRoundedRect:bounds cornerRadius:self.cornerRadius] addClip]; + [[UIBezierPath bezierPathWithRoundedRect:bounds cornerRadius:cornerRadius] addClip]; } else { [[UIBezierPath bezierPathWithRect:bounds] addClip]; } From c5b5bdd29c057268a9a51c60742f9b8e0a8ba450 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 5 Oct 2015 20:34:42 -0700 Subject: [PATCH 04/36] Use deep compare when setting image identifiers in ASMultiplexImageNode --- AsyncDisplayKit/ASMultiplexImageNode.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 4ffb8a1275..7048de4e73 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -268,7 +268,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent { OSSpinLockLock(&_imageIdentifiersLock); - if (_imageIdentifiers == imageIdentifiers) { + if ([_imageIdentifiers isEqual:imageIdentifiers]) { OSSpinLockUnlock(&_imageIdentifiersLock); return; } From ce97b56580eeba6c592d0b9bae64c815cdaa20b9 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 5 Oct 2015 20:41:21 -0700 Subject: [PATCH 05/36] Sugar --- AsyncDisplayKit/ASMultiplexImageNode.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 7048de4e73..3d26fc0610 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -372,7 +372,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent OSSpinLockLock(&_imageIdentifiersLock); // If we've already loaded the best identifier, we've got nothing else to do. - id bestImageIdentifier = ([_imageIdentifiers count] > 0) ? _imageIdentifiers[0] : nil; + id bestImageIdentifier = _imageIdentifiers.firstObject; if (!bestImageIdentifier || [_loadedImageIdentifier isEqual:bestImageIdentifier]) { OSSpinLockUnlock(&_imageIdentifiersLock); return nil; From d94dcdef0fd0b9f5b33a2c67343a5ca3f0ecafeb Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 5 Oct 2015 21:05:27 -0700 Subject: [PATCH 06/36] Only do PHAsset metadata requests one at a time to workaround Photos framework deadlock --- AsyncDisplayKit/ASMultiplexImageNode.mm | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 3d26fc0610..7879970e3f 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -518,17 +518,33 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent ASDisplayNodeAssertNotNil(request, @"request is required"); ASDisplayNodeAssertNotNil(completionBlock, @"completionBlock is required"); + /* + * Locking rationale: + * As of iOS 9, Photos.framework will eventually deadlock if you hit it with concurrent fetch requests. rdar://22984886 + * Image requests are OK, but metadata requests aren't, so we limit ourselves to one at a time. + + * -[PHFetchResult dealloc] plays a role in this deadlock, so we help the fetch result die ASAP by never storing it. + */ + static NSLock *phRequestLock; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + phRequestLock = [NSLock new]; + }); + // This is sometimes called on main but there's no reason to stay there dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ + // Get the PHAsset itself. - PHFetchResult *assetFetchResult = [PHAsset fetchAssetsWithLocalIdentifiers:@[request.assetIdentifier] options:nil]; - if ([assetFetchResult count] == 0) { + [phRequestLock lock]; + PHAsset *imageAsset = [PHAsset fetchAssetsWithLocalIdentifiers:@[request.assetIdentifier] options:nil].firstObject; + [phRequestLock unlock]; + + if (imageAsset == nil) { // Error. completionBlock(nil, nil); return; } - PHAsset *imageAsset = [assetFetchResult firstObject]; PHImageRequestOptions *options = [request.options copy]; // We don't support opportunistic delivery – one request, one image. From f0b7e150cd69a3c1a6f075d2f4a43acc1f7e804f Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 5 Oct 2015 23:01:03 -0700 Subject: [PATCH 07/36] Limit 1 inflight Photos.framework request per multiplex image node --- AsyncDisplayKit/ASMultiplexImageNode.mm | 40 +++++++++++++++++++------ 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 7879970e3f..28b627bb6f 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -66,7 +66,8 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent id _loadedImageIdentifier; id _loadingImageIdentifier; id _displayedImageIdentifier; - + NSOperation *_phImageRequestOperation; + // Networking. id _downloadIdentifier; } @@ -168,12 +169,21 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent return [self initWithCache:nil downloader:nil]; // satisfy compiler } +- (void)dealloc { + [_phImageRequestOperation cancel]; +} + #pragma mark - ASDisplayNode Overrides - (void)clearContents { [super clearContents]; // This actually clears the contents, so we need to do this first for our displayedImageIdentifier to be meaningful. [self _setDisplayedImageIdentifier:nil withImage:nil]; + if (_phImageRequestOperation) { + [_phImageRequestOperation cancel]; + _phImageRequestOperation = nil; + } + if (_downloadIdentifier) { [_downloader cancelImageDownloadForIdentifier:_downloadIdentifier]; _downloadIdentifier = nil; @@ -521,22 +531,33 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent /* * Locking rationale: * As of iOS 9, Photos.framework will eventually deadlock if you hit it with concurrent fetch requests. rdar://22984886 - * Image requests are OK, but metadata requests aren't, so we limit ourselves to one at a time. - - * -[PHFetchResult dealloc] plays a role in this deadlock, so we help the fetch result die ASAP by never storing it. + * Concurrent image requests are OK, but metadata requests aren't, so we limit ourselves to one at a time. */ static NSLock *phRequestLock; + static NSOperationQueue *phImageRequestQueue; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ phRequestLock = [NSLock new]; + phImageRequestQueue = [NSOperationQueue new]; + phImageRequestQueue.maxConcurrentOperationCount = 10; + phImageRequestQueue.name = @"org.AsyncDisplayKit.MultiplexImageNode.phImageRequestQueue"; }); - // This is sometimes called on main but there's no reason to stay there - dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ + // Each ASMultiplexImageNode can have max 1 inflight Photos image request operation + [_phImageRequestOperation cancel]; + + __weak __typeof(self)weakSelf = self; + _phImageRequestOperation = [NSBlockOperation blockOperationWithBlock:^{ + __strong __typeof(weakSelf)strongSelf = weakSelf; + if (strongSelf == nil) { return; } // Get the PHAsset itself. [phRequestLock lock]; - PHAsset *imageAsset = [PHAsset fetchAssetsWithLocalIdentifiers:@[request.assetIdentifier] options:nil].firstObject; + PHAsset *imageAsset; + // -[PHFetchResult dealloc] plays a role in the deadlock mentioned above, so we make sure the PHFetchResult is deallocated inside the critical section + @autoreleasepool { + imageAsset = [PHAsset fetchAssetsWithLocalIdentifiers:@[request.assetIdentifier] options:nil].firstObject; + } [phRequestLock unlock]; if (imageAsset == nil) { @@ -558,7 +579,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent options.synchronous = YES; } - PHImageManager *imageManager = self.imageManager ?: PHImageManager.defaultManager; + PHImageManager *imageManager = strongSelf.imageManager ?: PHImageManager.defaultManager; [imageManager requestImageForAsset:imageAsset targetSize:request.targetSize contentMode:request.contentMode options:options resultHandler:^(UIImage *image, NSDictionary *info) { if (NSThread.isMainThread) { dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ @@ -568,7 +589,8 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent completionBlock(image, info[PHImageErrorKey]); } }]; - }); + }]; + [phImageRequestQueue addOperation:_phImageRequestOperation]; } - (void)_fetchImageWithIdentifierFromCache:(id)imageIdentifier URL:(NSURL *)imageURL completion:(void (^)(UIImage *image))completionBlock From 87caed27e2acf1bb555faad1141b788e631bf655 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 5 Oct 2015 23:48:48 -0700 Subject: [PATCH 08/36] Add more deep compares when setting public properties to the same value --- AsyncDisplayKit/ASEditableTextNode.mm | 2 +- AsyncDisplayKit/ASImageNode.mm | 3 ++- AsyncDisplayKit/ASNetworkImageNode.mm | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/AsyncDisplayKit/ASEditableTextNode.mm b/AsyncDisplayKit/ASEditableTextNode.mm index e6b5399edf..5b929ccfb3 100644 --- a/AsyncDisplayKit/ASEditableTextNode.mm +++ b/AsyncDisplayKit/ASEditableTextNode.mm @@ -217,7 +217,7 @@ - (void)setTypingAttributes:(NSDictionary *)typingAttributes { - if (_typingAttributes == typingAttributes) + if (ASObjectIsEqual(typingAttributes, _typingAttributes)) return; _typingAttributes = [typingAttributes copy]; diff --git a/AsyncDisplayKit/ASImageNode.mm b/AsyncDisplayKit/ASImageNode.mm index f28b743054..fc5aa93dc6 100644 --- a/AsyncDisplayKit/ASImageNode.mm +++ b/AsyncDisplayKit/ASImageNode.mm @@ -18,6 +18,7 @@ #import "ASImageNode+CGExtras.h" #import "ASInternalHelpers.h" +#import "ASEqualityHelpers.h" @interface _ASImageNodeDrawParameters : NSObject @@ -123,7 +124,7 @@ - (void)setImage:(UIImage *)image { ASDN::MutexLocker l(_imageLock); - if (_image != image) { + if (!ASObjectIsEqual(_image, image)) { _image = image; ASDN::MutexUnlocker u(_imageLock); diff --git a/AsyncDisplayKit/ASNetworkImageNode.mm b/AsyncDisplayKit/ASNetworkImageNode.mm index d41fd60976..3d2fad8906 100644 --- a/AsyncDisplayKit/ASNetworkImageNode.mm +++ b/AsyncDisplayKit/ASNetworkImageNode.mm @@ -11,7 +11,7 @@ #import "ASBasicImageDownloader.h" #import "ASDisplayNode+Subclasses.h" #import "ASThread.h" - +#import "ASEqualityHelpers.h" @interface ASNetworkImageNode () { @@ -70,7 +70,7 @@ { ASDN::MutexLocker l(_lock); - if (URL == _URL || [URL isEqual:_URL]) { + if (ASObjectIsEqual(URL, _URL)) { return; } @@ -96,7 +96,7 @@ { ASDN::MutexLocker l(_lock); - if (defaultImage == _defaultImage || [defaultImage isEqual:_defaultImage]) { + if (ASObjectIsEqual(defaultImage, _defaultImage)) { return; } _defaultImage = defaultImage; From ae3eb70f6a584041d5ab283f2926449437b52ba5 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 6 Oct 2015 00:40:33 -0700 Subject: [PATCH 09/36] Public API fast paths in ASTextNode --- AsyncDisplayKit/ASTextNode.mm | 37 ++++++++++++++++------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/AsyncDisplayKit/ASTextNode.mm b/AsyncDisplayKit/ASTextNode.mm index 5023a06f63..319a57b946 100644 --- a/AsyncDisplayKit/ASTextNode.mm +++ b/AsyncDisplayKit/ASTextNode.mm @@ -19,6 +19,7 @@ #import "ASTextNodeRenderer.h" #import "ASTextNodeShadower.h" +#import "ASEqualityHelpers.h" static const NSTimeInterval ASTextNodeHighlightFadeOutDuration = 0.15; static const NSTimeInterval ASTextNodeHighlightFadeInDuration = 0.1; @@ -315,7 +316,7 @@ static NSString *ASTextNodeTruncationTokenAttributeName = @"ASTextNodeTruncation #pragma mark - Modifying User Text - (void)setAttributedString:(NSAttributedString *)attributedString { - if (attributedString == _attributedString) { + if (ASObjectIsEqual(attributedString, _attributedString)) { return; } @@ -349,14 +350,16 @@ static NSString *ASTextNodeTruncationTokenAttributeName = @"ASTextNodeTruncation - (void)setExclusionPaths:(NSArray *)exclusionPaths { - if ((_exclusionPaths == nil && exclusionPaths != nil) || (![_exclusionPaths isEqualToArray:exclusionPaths])) { - _exclusionPaths = exclusionPaths; - [self _invalidateRenderer]; - [self invalidateCalculatedLayout]; - ASDisplayNodeRespectThreadAffinityOfNode(self, ^{ - [self setNeedsDisplay]; - }); + if (ASObjectIsEqual(exclusionPaths, _exclusionPaths)) { + return; } + + _exclusionPaths = exclusionPaths; + [self _invalidateRenderer]; + [self invalidateCalculatedLayout]; + ASDisplayNodeRespectThreadAffinityOfNode(self, ^{ + [self setNeedsDisplay]; + }); } - (NSArray *)exclusionPaths @@ -965,28 +968,22 @@ static NSAttributedString *DefaultTruncationAttributedString() - (void)setTruncationAttributedString:(NSAttributedString *)truncationAttributedString { - // No-op if they're exactly equal (avoid redrawing) - if (_truncationAttributedString == truncationAttributedString) { + if (ASObjectIsEqual(_truncationAttributedString, truncationAttributedString)) { return; } - if (![_truncationAttributedString isEqual:truncationAttributedString]) { - _truncationAttributedString = [truncationAttributedString copy]; - [self _invalidateTruncationString]; - } + _truncationAttributedString = [truncationAttributedString copy]; + [self _invalidateTruncationString]; } - (void)setAdditionalTruncationMessage:(NSAttributedString *)additionalTruncationMessage { - // Short circuit if we're setting to nil (prevent redrawing when we don't need to) - if (_additionalTruncationMessage == additionalTruncationMessage) { + if (ASObjectIsEqual(_additionalTruncationMessage, additionalTruncationMessage)) { return; } - if (![_additionalTruncationMessage isEqual:additionalTruncationMessage]) { - _additionalTruncationMessage = [additionalTruncationMessage copy]; - [self _invalidateTruncationString]; - } + _additionalTruncationMessage = [additionalTruncationMessage copy]; + [self _invalidateTruncationString]; } - (void)setTruncationMode:(NSLineBreakMode)truncationMode From 180162009407d9e248803624d6e7430b79be513d Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 6 Oct 2015 00:46:20 -0700 Subject: [PATCH 10/36] Make ASTextNode copy exclusionPaths and attributedString --- AsyncDisplayKit/ASTextNode.mm | 2 +- AsyncDisplayKit/Details/ASTextNodeCoreTextAdditions.m | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/AsyncDisplayKit/ASTextNode.mm b/AsyncDisplayKit/ASTextNode.mm index 319a57b946..3a9509266c 100644 --- a/AsyncDisplayKit/ASTextNode.mm +++ b/AsyncDisplayKit/ASTextNode.mm @@ -354,7 +354,7 @@ static NSString *ASTextNodeTruncationTokenAttributeName = @"ASTextNodeTruncation return; } - _exclusionPaths = exclusionPaths; + _exclusionPaths = [exclusionPaths copy]; [self _invalidateRenderer]; [self invalidateCalculatedLayout]; ASDisplayNodeRespectThreadAffinityOfNode(self, ^{ diff --git a/AsyncDisplayKit/Details/ASTextNodeCoreTextAdditions.m b/AsyncDisplayKit/Details/ASTextNodeCoreTextAdditions.m index 55efebdce8..5ba091b3ea 100644 --- a/AsyncDisplayKit/Details/ASTextNodeCoreTextAdditions.m +++ b/AsyncDisplayKit/Details/ASTextNodeCoreTextAdditions.m @@ -149,7 +149,7 @@ NSAttributedString *ASCleanseAttributedStringOfCoreTextAttributes(NSAttributedSt return cleanAttributedString; } else { - return dirtyAttributedString; + return [dirtyAttributedString copy]; } } From 1f799a6687cd0995fe52c21f21e3d51c0ce4e8f5 Mon Sep 17 00:00:00 2001 From: Levi McCallum Date: Tue, 6 Oct 2015 09:34:57 -0700 Subject: [PATCH 11/36] Use long-form view block initializer for better Swift support --- AsyncDisplayKit/ASScrollNode.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASScrollNode.m b/AsyncDisplayKit/ASScrollNode.m index ade0ee9e5e..afead9ba8a 100644 --- a/AsyncDisplayKit/ASScrollNode.m +++ b/AsyncDisplayKit/ASScrollNode.m @@ -28,7 +28,7 @@ { return [super initWithViewBlock:^UIView *{ return [[ASScrollView alloc] init]; - }]; + } didLoadBlock:nil]; } @end From 8a70c8d416a73ea4c49543ab3933e828c1d67ea4 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 5 Oct 2015 13:50:43 -0700 Subject: [PATCH 12/36] Adds support for disabling and re-enabling should rasterize. --- AsyncDisplayKit/ASDisplayNode.mm | 60 ++++++- AsyncDisplayKit/Private/_ASPendingState.h | 3 + AsyncDisplayKit/Private/_ASPendingState.m | 208 ++++++++++++++++++++++ 3 files changed, 268 insertions(+), 3 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index f8b5aef2ec..6201fb75ef 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -331,6 +331,40 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) #pragma mark - Core +- (void)__tearDown:(BOOL)tearDown subnodesOfNode:(ASDisplayNode *)node +{ + for (ASDisplayNode *subnode in node.subnodes) { + if (tearDown) { + [subnode __unloadNode]; + } else { + [subnode __loadNode]; + } + } +} + +- (void)__unloadNode +{ + ASDisplayNodeAssertThreadAffinity(self); + ASDN::MutexLocker l(_propertyLock); + + if (_flags.layerBacked) + _pendingViewState = [_ASPendingState pendingViewStateFromLayer:_layer]; + else + _pendingViewState = [_ASPendingState pendingViewStateFromView:_view]; + + [_view removeFromSuperview]; + _view = nil; + if (_flags.layerBacked) + _layer.delegate = nil; + [_layer removeFromSuperlayer]; + _layer = nil; +} + +- (void)__loadNode +{ + [self layer]; +} + - (ASDisplayNode *)__rasterizedContainerNode { ASDisplayNode *node = self.supernode; @@ -340,7 +374,7 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) } node = node.supernode; } - + return nil; } @@ -619,11 +653,22 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) { ASDisplayNodeAssertThreadAffinity(self); ASDN::MutexLocker l(_propertyLock); - + if (_flags.shouldRasterizeDescendants == flag) return; - + _flags.shouldRasterizeDescendants = flag; + + if (self.isNodeLoaded) { + //recursively tear down or build up subnodes + [self recursivelyClearContents]; + [self __tearDown:flag subnodesOfNode:self]; + if (flag == NO) { + [self _addSubnodeViewsAndLayers]; + } + + [self recursivelyDisplayImmediately]; + } } - (CGFloat)contentsScaleForDisplay @@ -653,6 +698,15 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) [[self asyncLayer] displayImmediately]; } +- (void)recursivelyDisplayImmediately +{ + ASDN::MutexLocker l(_propertyLock); + for (ASDisplayNode *child in _subnodes) { + [child recursivelyDisplayImmediately]; + } + [self displayImmediately]; +} + - (void)__setNeedsLayout { ASDisplayNodeAssertThreadAffinity(self); diff --git a/AsyncDisplayKit/Private/_ASPendingState.h b/AsyncDisplayKit/Private/_ASPendingState.h index 904730b93d..4dd1146d24 100644 --- a/AsyncDisplayKit/Private/_ASPendingState.h +++ b/AsyncDisplayKit/Private/_ASPendingState.h @@ -27,4 +27,7 @@ - (void)applyToView:(UIView *)view; - (void)applyToLayer:(CALayer *)layer; ++ (_ASPendingState *)pendingViewStateFromLayer:(CALayer *)layer; ++ (_ASPendingState *)pendingViewStateFromView:(UIView *)view; + @end diff --git a/AsyncDisplayKit/Private/_ASPendingState.m b/AsyncDisplayKit/Private/_ASPendingState.m index b92f97d802..f8c2426971 100644 --- a/AsyncDisplayKit/Private/_ASPendingState.m +++ b/AsyncDisplayKit/Private/_ASPendingState.m @@ -796,4 +796,212 @@ view.accessibilityIdentifier = accessibilityIdentifier; } ++ (_ASPendingState *)pendingViewStateFromLayer:(CALayer *)layer +{ + _ASPendingState *pendingState = [[_ASPendingState alloc] init]; + + pendingState.anchorPoint = layer.anchorPoint; + (pendingState->_flags).setAnchorPoint = YES; + + pendingState.position = layer.position; + (pendingState->_flags).setPosition = YES; + + pendingState.zPosition = layer.zPosition; + (pendingState->_flags).setZPosition = YES; + + pendingState.bounds = layer.bounds; + (pendingState->_flags).setBounds = YES; + + pendingState.contentsScale = layer.contentsScale; + (pendingState->_flags).setContentsScale = YES; + + pendingState.transform = layer.transform; + (pendingState->_flags).setTransform = YES; + + pendingState.sublayerTransform = layer.sublayerTransform; + (pendingState->_flags).setSublayerTransform = YES; + + pendingState.contents = layer.contents; + (pendingState->_flags).setContents = YES; + + pendingState.clipsToBounds = layer.masksToBounds; + (pendingState->_flags).setClipsToBounds = YES; + + pendingState.backgroundColor = layer.backgroundColor; + (pendingState->_flags).setBackgroundColor = YES; + + pendingState.opaque = layer.opaque; + (pendingState->_flags).setOpaque = YES; + + pendingState.hidden = layer.hidden; + (pendingState->_flags).setHidden = YES; + + pendingState.alpha = layer.opacity; + (pendingState->_flags).setAlpha = YES; + + pendingState.cornerRadius = layer.cornerRadius; + (pendingState->_flags).setCornerRadius = YES; + + pendingState.contentMode = ASDisplayNodeUIContentModeFromCAContentsGravity(layer.contentsGravity); + (pendingState->_flags).setContentMode = YES; + + pendingState.shadowColor = layer.shadowColor; + (pendingState->_flags).setShadowColor = YES; + + pendingState.shadowOpacity = layer.shadowOpacity; + (pendingState->_flags).setShadowOpacity = YES; + + pendingState.shadowOffset = layer.shadowOffset; + (pendingState->_flags).setShadowOffset = YES; + + pendingState.shadowRadius = layer.shadowRadius; + (pendingState->_flags).setShadowRadius = YES; + + pendingState.borderWidth = layer.borderWidth; + (pendingState->_flags).setBorderWidth = YES; + + pendingState.borderColor = layer.borderColor; + (pendingState->_flags).setBorderColor = YES; + + pendingState.needsDisplayOnBoundsChange = layer.needsDisplayOnBoundsChange; + (pendingState->_flags).setNeedsDisplayOnBoundsChange = YES; + + pendingState.allowsEdgeAntialiasing = layer.allowsEdgeAntialiasing; + (pendingState->_flags).setAllowsEdgeAntialiasing = YES; + + pendingState.edgeAntialiasingMask = layer.edgeAntialiasingMask; + (pendingState->_flags).setEdgeAntialiasingMask = YES; + + return pendingState; +} + ++ (_ASPendingState *)pendingViewStateFromView:(UIView *)view +{ + _ASPendingState *pendingState = [[_ASPendingState alloc] init]; + + CALayer *layer = view.layer; + + pendingState.anchorPoint = layer.anchorPoint; + (pendingState->_flags).setAnchorPoint = YES; + + pendingState.position = layer.position; + (pendingState->_flags).setPosition = YES; + + pendingState.zPosition = layer.zPosition; + (pendingState->_flags).setZPosition = YES; + + pendingState.bounds = view.bounds; + (pendingState->_flags).setBounds = YES; + + pendingState.contentsScale = layer.contentsScale; + (pendingState->_flags).setContentsScale = YES; + + pendingState.transform = layer.transform; + (pendingState->_flags).setTransform = YES; + + pendingState.sublayerTransform = layer.sublayerTransform; + (pendingState->_flags).setSublayerTransform = YES; + + pendingState.contents = layer.contents; + (pendingState->_flags).setContents = YES; + + pendingState.clipsToBounds = view.clipsToBounds; + (pendingState->_flags).setClipsToBounds = YES; + + pendingState.backgroundColor = layer.backgroundColor; + (pendingState->_flags).setBackgroundColor = YES; + + pendingState.tintColor = view.tintColor; + (pendingState->_flags).setTintColor = YES; + + pendingState.opaque = layer.opaque; + (pendingState->_flags).setOpaque = YES; + + pendingState.hidden = view.hidden; + (pendingState->_flags).setHidden = YES; + + pendingState.alpha = view.alpha; + (pendingState->_flags).setAlpha = YES; + + pendingState.cornerRadius = layer.cornerRadius; + (pendingState->_flags).setCornerRadius = YES; + + pendingState.contentMode = view.contentMode; + (pendingState->_flags).setContentMode = YES; + + pendingState.userInteractionEnabled = view.userInteractionEnabled; + (pendingState->_flags).setUserInteractionEnabled = YES; + + pendingState.exclusiveTouch = view.exclusiveTouch; + (pendingState->_flags).setExclusiveTouch = YES; + + pendingState.shadowColor = layer.shadowColor; + (pendingState->_flags).setShadowColor = YES; + + pendingState.shadowOpacity = layer.shadowOpacity; + (pendingState->_flags).setShadowOpacity = YES; + + pendingState.shadowOffset = layer.shadowOffset; + (pendingState->_flags).setShadowOffset = YES; + + pendingState.shadowRadius = layer.shadowRadius; + (pendingState->_flags).setShadowRadius = YES; + + pendingState.borderWidth = layer.borderWidth; + (pendingState->_flags).setBorderWidth = YES; + + pendingState.borderColor = layer.borderColor; + (pendingState->_flags).setBorderColor = YES; + + pendingState.autoresizingMask = view.autoresizingMask; + (pendingState->_flags).setAutoresizingMask = YES; + + pendingState.autoresizesSubviews = view.autoresizesSubviews; + (pendingState->_flags).setAutoresizesSubviews = YES; + + pendingState.needsDisplayOnBoundsChange = layer.needsDisplayOnBoundsChange; + (pendingState->_flags).setNeedsDisplayOnBoundsChange = YES; + + pendingState.allowsEdgeAntialiasing = layer.allowsEdgeAntialiasing; + (pendingState->_flags).setAllowsEdgeAntialiasing = YES; + + pendingState.edgeAntialiasingMask = layer.edgeAntialiasingMask; + (pendingState->_flags).setEdgeAntialiasingMask = YES; + + pendingState.isAccessibilityElement = view.isAccessibilityElement; + (pendingState->_flags).setIsAccessibilityElement = YES; + + pendingState.accessibilityLabel = view.accessibilityLabel; + (pendingState->_flags).setAccessibilityLabel = YES; + + pendingState.accessibilityHint = view.accessibilityHint; + (pendingState->_flags).setAccessibilityHint = YES; + + pendingState.accessibilityValue = view.accessibilityValue; + (pendingState->_flags).setAccessibilityValue = YES; + + pendingState.accessibilityTraits = view.accessibilityTraits; + (pendingState->_flags).setAccessibilityTraits = YES; + + pendingState.accessibilityFrame = view.accessibilityFrame; + (pendingState->_flags).setAccessibilityFrame = YES; + + pendingState.accessibilityLanguage = view.accessibilityLanguage; + (pendingState->_flags).setAccessibilityLanguage = YES; + + pendingState.accessibilityElementsHidden = view.accessibilityElementsHidden; + (pendingState->_flags).setAccessibilityElementsHidden = YES; + + pendingState.accessibilityViewIsModal = view.accessibilityViewIsModal; + (pendingState->_flags).setAccessibilityViewIsModal = YES; + + pendingState.shouldGroupAccessibilityChildren = view.shouldGroupAccessibilityChildren; + (pendingState->_flags).setShouldGroupAccessibilityChildren = YES; + + pendingState.accessibilityIdentifier = view.accessibilityIdentifier; + (pendingState->_flags).setAccessibilityIdentifier = YES; + + return pendingState; +} + @end From 1a8f66919b91e6892ed446276e601ce9efac08e7 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Tue, 6 Oct 2015 11:35:43 -0700 Subject: [PATCH 13/36] Add tests for enabling / disabling shouldRasterize --- AsyncDisplayKitTests/ASSnapshotTestCase.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/AsyncDisplayKitTests/ASSnapshotTestCase.h b/AsyncDisplayKitTests/ASSnapshotTestCase.h index 054560152c..dae712c29b 100644 --- a/AsyncDisplayKitTests/ASSnapshotTestCase.h +++ b/AsyncDisplayKitTests/ASSnapshotTestCase.h @@ -14,6 +14,12 @@ { \ [ASSnapshotTestCase hackilySynchronouslyRecursivelyRenderNode:node__]; \ FBSnapshotVerifyLayer(node__.layer, identifier__); \ + [node__ setShouldRasterizeDescendants:YES]; \ + [ASSnapshotTestCase hackilySynchronouslyRecursivelyRenderNode:node__]; \ + FBSnapshotVerifyLayer(node__.layer, identifier__); \ + [node__ setShouldRasterizeDescendants:NO]; \ + [ASSnapshotTestCase hackilySynchronouslyRecursivelyRenderNode:node__]; \ + FBSnapshotVerifyLayer(node__.layer, identifier__); \ } @interface ASSnapshotTestCase : FBSnapshotTestCase From fffc76a3533f3d6b0119d2396d0587bb2bc38559 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 6 Oct 2015 13:21:42 -0700 Subject: [PATCH 14/36] Have ASMultiplexImageNode store its image request operation weakly --- AsyncDisplayKit/ASMultiplexImageNode.mm | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 28b627bb6f..07945f98a0 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -66,7 +66,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent id _loadedImageIdentifier; id _loadingImageIdentifier; id _displayedImageIdentifier; - NSOperation *_phImageRequestOperation; + __weak NSOperation *_phImageRequestOperation; // Networking. id _downloadIdentifier; @@ -547,7 +547,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent [_phImageRequestOperation cancel]; __weak __typeof(self)weakSelf = self; - _phImageRequestOperation = [NSBlockOperation blockOperationWithBlock:^{ + NSOperation *newImageRequestOp = [NSBlockOperation blockOperationWithBlock:^{ __strong __typeof(weakSelf)strongSelf = weakSelf; if (strongSelf == nil) { return; } @@ -590,7 +590,8 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent } }]; }]; - [phImageRequestQueue addOperation:_phImageRequestOperation]; + _phImageRequestOperation = newImageRequestOp; + [phImageRequestQueue addOperation:newImageRequestOp]; } - (void)_fetchImageWithIdentifierFromCache:(id)imageIdentifier URL:(NSURL *)imageURL completion:(void (^)(UIImage *image))completionBlock From 6d73cee9fd6fc4ffc669b8a5525ed3aea204b597 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 6 Oct 2015 16:35:50 -0700 Subject: [PATCH 15/36] Give ASMultiplexImageNodeDataSources an opportunity to provide PHAssets quicker --- AsyncDisplayKit/ASMultiplexImageNode.h | 12 ++++++++++++ AsyncDisplayKit/ASMultiplexImageNode.mm | 22 +++++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.h b/AsyncDisplayKit/ASMultiplexImageNode.h index 1956760899..302aa20ef5 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.h +++ b/AsyncDisplayKit/ASMultiplexImageNode.h @@ -212,6 +212,18 @@ didFinishDownloadingImageWithIdentifier:(id)imageIdentifier */ - (NSURL *)multiplexImageNode:(ASMultiplexImageNode *)imageNode URLForImageIdentifier:(id)imageIdentifier; +/** + * @abstract A PHAsset for the specific asset local identifier + * @param imageNode The sender. + * @param assetLocalIdentifier The local identifier for a PHAsset that this image node is loading. + * + * @discussion This optional method can improve image performance if your data source already has the PHAsset available. + * If this method is not implemented, or returns nil, the image node will request the asset from the Photos framework. + * @note This method may be called from any thread. + * @return A PHAsset corresponding to `assetLocalIdentifier`, or nil if none is available. + */ +- (PHAsset *)multiplexImageNode:(ASMultiplexImageNode *)imageNode assetForLocalIdentifier:(NSString *)assetLocalIdentifier; + @end #pragma mark - diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 07945f98a0..081a079524 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -546,19 +546,23 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent // Each ASMultiplexImageNode can have max 1 inflight Photos image request operation [_phImageRequestOperation cancel]; - __weak __typeof(self)weakSelf = self; + __weak __typeof(self) weakSelf = self; NSOperation *newImageRequestOp = [NSBlockOperation blockOperationWithBlock:^{ - __strong __typeof(weakSelf)strongSelf = weakSelf; + __strong __typeof(weakSelf) strongSelf = weakSelf; if (strongSelf == nil) { return; } - // Get the PHAsset itself. - [phRequestLock lock]; - PHAsset *imageAsset; - // -[PHFetchResult dealloc] plays a role in the deadlock mentioned above, so we make sure the PHFetchResult is deallocated inside the critical section - @autoreleasepool { - imageAsset = [PHAsset fetchAssetsWithLocalIdentifiers:@[request.assetIdentifier] options:nil].firstObject; + // Try to get the asset immediately from the data source. + PHAsset *imageAsset = [strongSelf.dataSource multiplexImageNode:strongSelf assetForLocalIdentifier:request.assetIdentifier]; + + // Fall back to locking and getting the PHAsset. + if (imageAsset == nil) { + [phRequestLock lock]; + // -[PHFetchResult dealloc] plays a role in the deadlock mentioned above, so we make sure the PHFetchResult is deallocated inside the critical section + @autoreleasepool { + imageAsset = [PHAsset fetchAssetsWithLocalIdentifiers:@[request.assetIdentifier] options:nil].firstObject; + } + [phRequestLock unlock]; } - [phRequestLock unlock]; if (imageAsset == nil) { // Error. From 89a216b90dff4ef08c641176beceef594ec6ba90 Mon Sep 17 00:00:00 2001 From: ricky cancro Date: Tue, 6 Oct 2015 21:41:39 -0700 Subject: [PATCH 16/36] Fixes to baseline stack alignment 1) Set the ascender/descender of an ASTextNode when the attributeString is set. Previously ascender/descender were only being computed in `setValuesFromLayoutable` and only when the attribute string was not nil. May make sense to remove the computation from `setValuesFromLayoutable` entirely. 2) Remove ability to allow different children of a stack spec to aling to different baselines. This wasn't working before and I'm not convinced it is possible to do properly/useful enough to invest the time. 3) Have all stack spec run `ASStackBaselinePositionedLayout::compute` to compute the stack's ascender and descender. Even if the stack isn't aligning its children to a baseline, the stack itself may be a child of another stack that IS aligning to a baseline. --- AsyncDisplayKit/ASTextNode.mm | 7 +++ AsyncDisplayKit/Layout/ASStackLayoutDefines.h | 8 +-- AsyncDisplayKit/Layout/ASStackLayoutSpec.mm | 16 +++-- .../ASStackBaselinePositionedLayout.mm | 63 ++++++++++--------- .../Private/ASStackLayoutSpecUtilities.h | 4 -- 5 files changed, 56 insertions(+), 42 deletions(-) diff --git a/AsyncDisplayKit/ASTextNode.mm b/AsyncDisplayKit/ASTextNode.mm index 3a9509266c..a88920892e 100644 --- a/AsyncDisplayKit/ASTextNode.mm +++ b/AsyncDisplayKit/ASTextNode.mm @@ -17,6 +17,7 @@ #import #import +#import "ASInternalHelpers.h" #import "ASTextNodeRenderer.h" #import "ASTextNodeShadower.h" #import "ASEqualityHelpers.h" @@ -344,6 +345,12 @@ static NSString *ASTextNodeTruncationTokenAttributeName = @"ASTextNodeTruncation self.isAccessibilityElement = YES; } }); + + if (attributedString.length > 0) { + CGFloat screenScale = ASScreenScale(); + self.ascender = round([[attributedString attribute:NSFontAttributeName atIndex:0 effectiveRange:NULL] ascender] * screenScale)/screenScale; + self.descender = round([[attributedString attribute:NSFontAttributeName atIndex:attributedString.length - 1 effectiveRange:NULL] descender] * screenScale)/screenScale; + } } #pragma mark - Text Layout diff --git a/AsyncDisplayKit/Layout/ASStackLayoutDefines.h b/AsyncDisplayKit/Layout/ASStackLayoutDefines.h index 82d1aa0daf..0ca7bb3c49 100644 --- a/AsyncDisplayKit/Layout/ASStackLayoutDefines.h +++ b/AsyncDisplayKit/Layout/ASStackLayoutDefines.h @@ -45,9 +45,9 @@ typedef NS_ENUM(NSUInteger, ASStackLayoutAlignItems) { ASStackLayoutAlignItemsCenter, /** Expand children to fill cross axis */ ASStackLayoutAlignItemsStretch, - /** Children align to their first baseline. Only available for horizontal stack nodes */ + /** Children align to their first baseline. Only available for horizontal stack spec */ ASStackLayoutAlignItemsBaselineFirst, - /** Children align to their last baseline. Only available for horizontal stack nodes */ + /** Children align to their last baseline. Only available for horizontal stack spec */ ASStackLayoutAlignItemsBaselineLast, }; @@ -66,8 +66,4 @@ typedef NS_ENUM(NSUInteger, ASStackLayoutAlignSelf) { ASStackLayoutAlignSelfCenter, /** Expand to fill cross axis */ ASStackLayoutAlignSelfStretch, - /** Children align to their first baseline. Only available for horizontal stack nodes */ - ASStackLayoutAlignSelfBaselineFirst, - /** Children align to their last baseline. Only available for horizontal stack nodes */ - ASStackLayoutAlignSelfBaselineLast, }; diff --git a/AsyncDisplayKit/Layout/ASStackLayoutSpec.mm b/AsyncDisplayKit/Layout/ASStackLayoutSpec.mm index 3f0abf42fa..a1d27b3c5e 100644 --- a/AsyncDisplayKit/Layout/ASStackLayoutSpec.mm +++ b/AsyncDisplayKit/Layout/ASStackLayoutSpec.mm @@ -101,7 +101,6 @@ std::vector> stackChildren = std::vector>(); for (id child in self.children) { stackChildren.push_back(child); - needsBaselinePass |= child.alignSelf == ASStackLayoutAlignSelfBaselineFirst || child.alignSelf == ASStackLayoutAlignSelfBaselineLast; } const auto unpositionedLayout = ASStackUnpositionedLayout::compute(stackChildren, style, constrainedSize); @@ -109,12 +108,21 @@ CGSize finalSize = CGSizeZero; NSArray *sublayouts = nil; - if (needsBaselinePass) { - const auto baselinePositionedLayout = ASStackBaselinePositionedLayout::compute(positionedLayout, style, constrainedSize); + + // regardless of whether or not this stack aligns to baseline, we should let ASStackBaselinePositionedLayout::compute find the max ascender + // and min descender in case this spec is a child in another spec that wants to align to a baseline. + const auto baselinePositionedLayout = ASStackBaselinePositionedLayout::compute(positionedLayout, style, constrainedSize); + if (self.direction == ASStackLayoutDirectionVertical) { + ASDN::MutexLocker l(_propertyLock); + self.ascender = [[self.children firstObject] ascender]; + self.descender = [[self.children lastObject] descender]; + } else { ASDN::MutexLocker l(_propertyLock); self.ascender = baselinePositionedLayout.ascender; self.descender = baselinePositionedLayout.descender; - + } + + if (needsBaselinePass) { finalSize = directionSize(style.direction, unpositionedLayout.stackDimensionSum, baselinePositionedLayout.crossSize); sublayouts = [NSArray arrayWithObjects:&baselinePositionedLayout.sublayouts[0] count:baselinePositionedLayout.sublayouts.size()]; } else { diff --git a/AsyncDisplayKit/Private/ASStackBaselinePositionedLayout.mm b/AsyncDisplayKit/Private/ASStackBaselinePositionedLayout.mm index 4e81dd60b5..57fd1b4c6d 100644 --- a/AsyncDisplayKit/Private/ASStackBaselinePositionedLayout.mm +++ b/AsyncDisplayKit/Private/ASStackBaselinePositionedLayout.mm @@ -93,7 +93,7 @@ ASStackBaselinePositionedLayout ASStackBaselinePositionedLayout::compute(const A const auto ascenderIt = std::max_element(positionedLayout.sublayouts.begin(), positionedLayout.sublayouts.end(), [&](const ASLayout *a, const ASLayout *b){ return a.layoutableObject.ascender < b.layoutableObject.ascender; }); - const CGFloat maxAscender = baselineIt == positionedLayout.sublayouts.end() ? 0 : (*ascenderIt).layoutableObject.ascender; + const CGFloat maxAscender = ascenderIt == positionedLayout.sublayouts.end() ? 0 : (*ascenderIt).layoutableObject.ascender; /* Step 3: Take each child and update its layout position based on the baseline offset. @@ -103,33 +103,40 @@ ASStackBaselinePositionedLayout ASStackBaselinePositionedLayout::compute(const A spacing between the two nodes is from the baseline, not the bounding box. */ - CGPoint p = CGPointZero; - BOOL first = YES; - auto stackedChildren = AS::map(positionedLayout.sublayouts, [&](ASLayout *l) -> ASLayout *{ - __weak id child = l.layoutableObject; - p = p + directionPoint(style.direction, child.spacingBefore, 0); - if (first) { - // if this is the first item use the previously computed start point - p = l.position; - } else { - // otherwise add the stack spacing - p = p + directionPoint(style.direction, style.spacing, 0); - } - first = NO; - - // Find the difference between this node's baseline and the max baseline of all the children. Add this difference to the child's y position. - l.position = p + CGPointMake(0, baselineOffset(style, l, maxAscender, maxBaseline)); - - // If we are a vertical stack, add the item's descender (it is negative) to the offset for the next node. This will ensure we are spacing - // node from baselines and not bounding boxes. - CGFloat spacingAfterBaseline = 0; - if (style.direction == ASStackLayoutDirectionVertical) { - spacingAfterBaseline = child.descender; - } - p = p + directionPoint(style.direction, stackDimension(style.direction, l.size) + child.spacingAfter + spacingAfterBaseline, 0); - - return l; - }); + std::vector stackedChildren; + // Only change positions of layouts this stackSpec is aligning to a baseline. Otherwise we are only here to + // compute the min/max descender/ascender for this stack spec. + if (style.baselineRelativeArrangement || style.alignItems == ASStackLayoutAlignItemsBaselineFirst || style.alignItems == ASStackLayoutAlignItemsBaselineLast) { + CGPoint p = CGPointZero; + BOOL first = YES; + stackedChildren = AS::map(positionedLayout.sublayouts, [&](ASLayout *l) -> ASLayout *{ + __weak id child = l.layoutableObject; + p = p + directionPoint(style.direction, child.spacingBefore, 0); + if (first) { + // if this is the first item use the previously computed start point + p = l.position; + } else { + // otherwise add the stack spacing + p = p + directionPoint(style.direction, style.spacing, 0); + } + first = NO; + + // Find the difference between this node's baseline and the max baseline of all the children. Add this difference to the child's y position. + l.position = p + CGPointMake(0, baselineOffset(style, l, maxAscender, maxBaseline)); + + // If we are a vertical stack, add the item's descender (it is negative) to the offset for the next node. This will ensure we are spacing + // node from baselines and not bounding boxes. + CGFloat spacingAfterBaseline = 0; + if (style.direction == ASStackLayoutDirectionVertical) { + spacingAfterBaseline = child.descender; + } + p = p + directionPoint(style.direction, stackDimension(style.direction, l.size) + child.spacingAfter + spacingAfterBaseline, 0); + + return l; + }); + } else { + stackedChildren = positionedLayout.sublayouts; + } /* Step 4: Since we have been mucking with positions, there is a chance that our cross size has changed. Imagine a node with a font size of 40 diff --git a/AsyncDisplayKit/Private/ASStackLayoutSpecUtilities.h b/AsyncDisplayKit/Private/ASStackLayoutSpecUtilities.h index 2b073c106a..a61218cfe4 100644 --- a/AsyncDisplayKit/Private/ASStackLayoutSpecUtilities.h +++ b/AsyncDisplayKit/Private/ASStackLayoutSpecUtilities.h @@ -63,10 +63,6 @@ inline ASStackLayoutAlignItems alignment(ASStackLayoutAlignSelf childAlignment, return ASStackLayoutAlignItemsStart; case ASStackLayoutAlignSelfStretch: return ASStackLayoutAlignItemsStretch; - case ASStackLayoutAlignSelfBaselineFirst: - return ASStackLayoutAlignItemsBaselineFirst; - case ASStackLayoutAlignSelfBaselineLast: - return ASStackLayoutAlignItemsBaselineLast; case ASStackLayoutAlignSelfAuto: default: return stackAlignment; From cdfd5c1fddb73742d6bb3968415179a3243dd245 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Wed, 7 Oct 2015 10:08:28 -0700 Subject: [PATCH 17/36] Check whether data source responds to asset method --- AsyncDisplayKit/ASMultiplexImageNode.mm | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 081a079524..06ecd298c2 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -57,6 +57,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent struct { unsigned int image:1; unsigned int URL:1; + unsigned int asset:1; } _dataSourceFlags; // Image flags. @@ -260,6 +261,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent _dataSource = dataSource; _dataSourceFlags.image = [_dataSource respondsToSelector:@selector(multiplexImageNode:imageForImageIdentifier:)]; _dataSourceFlags.URL = [_dataSource respondsToSelector:@selector(multiplexImageNode:URLForImageIdentifier:)]; + _dataSourceFlags.asset = [_dataSource respondsToSelector:@selector(multiplexImageNode:assetForLocalIdentifier:)]; } #pragma mark - @@ -551,8 +553,12 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent __strong __typeof(weakSelf) strongSelf = weakSelf; if (strongSelf == nil) { return; } + PHAsset *imageAsset = nil; + // Try to get the asset immediately from the data source. - PHAsset *imageAsset = [strongSelf.dataSource multiplexImageNode:strongSelf assetForLocalIdentifier:request.assetIdentifier]; + if (_dataSourceFlags.asset) { + imageAsset = [strongSelf.dataSource multiplexImageNode:strongSelf assetForLocalIdentifier:request.assetIdentifier]; + } // Fall back to locking and getting the PHAsset. if (imageAsset == nil) { From cb407367bedb91eef6c4a78ff60b6974421068fe Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Wed, 7 Oct 2015 10:08:54 -0700 Subject: [PATCH 18/36] Cancel image requests in ASMultiplexImageNode.clearFetchedData --- AsyncDisplayKit/ASMultiplexImageNode.mm | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index 06ecd298c2..648c9bb7a0 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -170,7 +170,8 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent return [self initWithCache:nil downloader:nil]; // satisfy compiler } -- (void)dealloc { +- (void)dealloc +{ [_phImageRequestOperation cancel]; } @@ -180,10 +181,7 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent [super clearContents]; // This actually clears the contents, so we need to do this first for our displayedImageIdentifier to be meaningful. [self _setDisplayedImageIdentifier:nil withImage:nil]; - if (_phImageRequestOperation) { - [_phImageRequestOperation cancel]; - _phImageRequestOperation = nil; - } + [_phImageRequestOperation cancel]; if (_downloadIdentifier) { [_downloader cancelImageDownloadForIdentifier:_downloadIdentifier]; @@ -196,6 +194,14 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent [super clearFetchedData]; if ([self _shouldClearFetchedImageData]) { + + [_phImageRequestOperation cancel]; + + if (_downloadIdentifier) { + [_downloader cancelImageDownloadForIdentifier:_downloadIdentifier]; + _downloadIdentifier = nil; + } + // setting this to nil makes the node fetch images the next time its display starts _loadedImageIdentifier = nil; self.image = nil; From 5a2fea7c1d5b9881ae3321c1736f689d0127d8ad Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 7 Oct 2015 21:46:07 +0300 Subject: [PATCH 19/36] Use bounds and position to layout subnodes, instead of frame because it is not safe in case the transform property constains a non-identity transform. --- AsyncDisplayKit/ASDisplayNode.mm | 28 ++++++++++++++++--- .../Private/ASDisplayNode+UIViewBridge.mm | 17 +---------- .../Private/ASDisplayNodeInternal.h | 5 ++++ 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 6201fb75ef..4144c91b8c 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -744,6 +744,26 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) } } +- (void)__setSafeFrame:(CGRect)rect +{ + BOOL useLayer = (_layer && ASDisplayNodeThreadIsMain()); + + CGPoint origin = (useLayer ? _layer.bounds.origin : self.bounds.origin); + CGPoint anchorPoint = (useLayer ? _layer.anchorPoint : self.anchorPoint); + + CGRect bounds = (CGRect){ origin, rect.size }; + CGPoint position = CGPointMake(rect.origin.x + rect.size.width * anchorPoint.x, + rect.origin.y + rect.size.height * anchorPoint.y); + + if (useLayer) { + _layer.bounds = bounds; + _layer.position = position; + } else { + self.bounds = bounds; + self.position = position; + } +} + // These private methods ensure that subclasses are not required to call super in order for _renderingSubnodes to be properly managed. - (void)__layout @@ -1691,10 +1711,10 @@ void recursivelyEnsureDisplayForLayer(CALayer *layer) // Assume that _layout was flattened and is 1-level deep. for (ASLayout *subnodeLayout in _layout.sublayouts) { ASDisplayNodeAssert([_subnodes containsObject:subnodeLayout.layoutableObject], @"Cached sublayouts must only contain subnodes' layout."); - ((ASDisplayNode *)subnodeLayout.layoutableObject).frame = CGRectMake(subnodeLayout.position.x, - subnodeLayout.position.y, - subnodeLayout.size.width, - subnodeLayout.size.height); + [((ASDisplayNode *)subnodeLayout.layoutableObject) __setSafeFrame:CGRectMake(subnodeLayout.position.x, + subnodeLayout.position.y, + subnodeLayout.size.width, + subnodeLayout.size.height)]; } } diff --git a/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm b/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm index 977c98ec40..4909abdd46 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm @@ -143,22 +143,7 @@ ASDisplayNodeAssert(CATransform3DIsIdentity(self.transform), @"-[ASDisplayNode setFrame:] - self.transform must be identity in order to set the frame property. (From Apple's UIView documentation: If the transform property is not the identity transform, the value of this property is undefined and therefore should be ignored.)"); #endif - BOOL useLayer = (_layer && ASDisplayNodeThreadIsMain()); - - CGPoint origin = (useLayer ? _layer.bounds.origin : self.bounds.origin); - CGPoint anchorPoint = (useLayer ? _layer.anchorPoint : self.anchorPoint); - - CGRect bounds = (CGRect){ origin, rect.size }; - CGPoint position = CGPointMake(rect.origin.x + rect.size.width * anchorPoint.x, - rect.origin.y + rect.size.height * anchorPoint.y); - - if (useLayer) { - _layer.bounds = bounds; - _layer.position = position; - } else { - self.bounds = bounds; - self.position = position; - } + [self __setSafeFrame:rect]; } - (void)setNeedsDisplay diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index 8283438272..c2daf155be 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -126,6 +126,11 @@ typedef NS_OPTIONS(NSUInteger, ASDisplayNodeMethodOverrides) { - (ASLayout *)__measureWithSizeRange:(ASSizeRange)constrainedSize; - (void)__setNeedsLayout; +/** + * Sets a new frame to this node by changing its bounds and position. This method can be safely called even if the transform property + * contains a non-identity transform, because bounds and position can be changed in such case. + */ +- (void)__setSafeFrame:(CGRect)rect; - (void)__layout; - (void)__setSupernode:(ASDisplayNode *)supernode; From 8b7dc916f8766a5d88f2fa98a42acd16084573e3 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 7 Oct 2015 22:16:37 +0300 Subject: [PATCH 20/36] Fix indentations in ASDisplayNode:layout --- AsyncDisplayKit/ASDisplayNode.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 4144c91b8c..c8d2e82830 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1712,9 +1712,9 @@ void recursivelyEnsureDisplayForLayer(CALayer *layer) for (ASLayout *subnodeLayout in _layout.sublayouts) { ASDisplayNodeAssert([_subnodes containsObject:subnodeLayout.layoutableObject], @"Cached sublayouts must only contain subnodes' layout."); [((ASDisplayNode *)subnodeLayout.layoutableObject) __setSafeFrame:CGRectMake(subnodeLayout.position.x, - subnodeLayout.position.y, - subnodeLayout.size.width, - subnodeLayout.size.height)]; + subnodeLayout.position.y, + subnodeLayout.size.width, + subnodeLayout.size.height)]; } } From c2fbd651dad74940d7d4aa8e68126778c21415fd Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 7 Oct 2015 22:21:43 +0300 Subject: [PATCH 21/36] Lock the property lock in ASDisplayNode __setSafeFrame --- AsyncDisplayKit/ASDisplayNode.mm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index c8d2e82830..c5beb9347b 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -746,6 +746,9 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) - (void)__setSafeFrame:(CGRect)rect { + ASDisplayNodeAssertThreadAffinity(self); + ASDN::MutexLocker l(_propertyLock); + BOOL useLayer = (_layer && ASDisplayNodeThreadIsMain()); CGPoint origin = (useLayer ? _layer.bounds.origin : self.bounds.origin); From a8435b494c9a103e808039329b4f3a0208681272 Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Wed, 7 Oct 2015 14:48:37 -0700 Subject: [PATCH 22/36] Revert "Properly support operating with nil asyncDelegate for Table & Collection." This reverts commit 77745c55b084c51d57a405073a6e9b00c379f918. Bug was found / reported in https://github.com/facebook/AsyncDisplayKit/issues/721 Attempting resolution here, but need to fix for current clients now: https://github.com/facebook/AsyncDisplayKit/pull/724 --- AsyncDisplayKit/ASCollectionView.mm | 29 +++++++++++------------------ AsyncDisplayKit/ASTableView.mm | 26 ++++++++++---------------- 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index 4287e39075..b1b29335b3 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -61,14 +61,12 @@ static BOOL _isInterceptedSelector(SEL sel) */ @interface _ASCollectionViewProxy : NSProxy - (instancetype)initWithTarget:(id)target interceptor:(ASCollectionView *)interceptor; -@property (nonatomic, weak) id target; @end @implementation _ASCollectionViewProxy { id __weak _target; ASCollectionView * __weak _interceptor; } -@synthesize target = _target; - (instancetype)initWithTarget:(id)target interceptor:(ASCollectionView *)interceptor { @@ -204,14 +202,6 @@ static BOOL _isInterceptedSelector(SEL sel) // and should not trigger a relayout. _ignoreMaxSizeChange = CGSizeEqualToSize(_maxSizeForNodesConstrainedSize, CGSizeZero); - // Set up the delegate / dataSource proxy now, so we recieve key method calls from UITableView even if - // our owner never sets up asyncDelegate (technically the dataSource is required) - _proxyDelegate = [[_ASCollectionViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; - super.delegate = (id)_proxyDelegate; - - _proxyDataSource = [[_ASCollectionViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; - super.dataSource = (id)_proxyDataSource; - self.backgroundColor = [UIColor whiteColor]; [self registerClass:[UICollectionViewCell class] forCellWithReuseIdentifier:@"_ASCollectionViewCell"]; @@ -262,21 +252,22 @@ static BOOL _isInterceptedSelector(SEL sel) // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDataSource in the ViewController's dealloc. In this case our _asyncDataSource // will return as nil (ARC magic) even though the _proxyDataSource still exists. It's really important to nil out - // _proxyDataSource.target in this case because calls to _ASCollectionViewProxy will start failing and cause crashes. + // super.dataSource in this case because calls to _ASTableViewProxy will start failing and cause crashes. if (asyncDataSource == nil) { - _proxyDataSource.target = nil; + super.dataSource = nil; _asyncDataSource = nil; + _proxyDataSource = nil; _asyncDataSourceImplementsConstrainedSizeForNode = NO; } else { - _proxyDataSource.target = asyncDataSource; _asyncDataSource = asyncDataSource; - _asyncDataSourceImplementsConstrainedSizeForNode = ([_asyncDataSource respondsToSelector:@selector(collectionView:constrainedSizeForNodeAtIndexPath:)] ? 1 : 0); - // TODO: Support supplementary views with ASCollectionView. if ([_asyncDataSource respondsToSelector:@selector(collectionView:viewForSupplementaryElementOfKind:atIndexPath:)]) { ASDisplayNodeAssert(NO, @"ASCollectionView is planned to support supplementary views by September 2015. You can work around this issue by using standard items."); } + _proxyDataSource = [[_ASCollectionViewProxy alloc] initWithTarget:_asyncDataSource interceptor:self]; + super.dataSource = (id)_proxyDataSource; + _asyncDataSourceImplementsConstrainedSizeForNode = ([_asyncDataSource respondsToSelector:@selector(collectionView:constrainedSizeForNodeAtIndexPath:)] ? 1 : 0); } } @@ -285,17 +276,19 @@ static BOOL _isInterceptedSelector(SEL sel) // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDelegate in the ViewController's dealloc. In this case our _asyncDelegate // will return as nil (ARC magic) even though the _proxyDelegate still exists. It's really important to nil out - // _proxyDelegate.target in this case because calls to _ASCollectionViewProxy will start failing and cause crashes. + // super.delegate in this case because calls to _ASTableViewProxy will start failing and cause crashes. if (asyncDelegate == nil) { // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes // in UIScrollViewAccessibility. - _proxyDelegate.target = nil; + super.delegate = nil; _asyncDelegate = nil; + _proxyDelegate = nil; _asyncDelegateImplementsInsetSection = NO; } else { - _proxyDelegate.target = asyncDelegate; _asyncDelegate = asyncDelegate; + _proxyDelegate = [[_ASCollectionViewProxy alloc] initWithTarget:_asyncDelegate interceptor:self]; + super.delegate = (id)_proxyDelegate; _asyncDelegateImplementsInsetSection = ([_asyncDelegate respondsToSelector:@selector(collectionView:layout:insetForSectionAtIndex:)] ? 1 : 0); } } diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index 6d83de9a95..7c36c775a6 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -59,14 +59,12 @@ static BOOL _isInterceptedSelector(SEL sel) */ @interface _ASTableViewProxy : NSProxy - (instancetype)initWithTarget:(id)target interceptor:(ASTableView *)interceptor; -@property (nonatomic, weak) id target; @end @implementation _ASTableViewProxy { id __weak _target; ASTableView * __weak _interceptor; } -@synthesize target = _target; - (instancetype)initWithTarget:(id)target interceptor:(ASTableView *)interceptor { @@ -220,14 +218,6 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // If the initial size is 0, expect a size change very soon which is part of the initial configuration // and should not trigger a relayout. _ignoreMaxWidthChange = (_maxWidthForNodesConstrainedSize == 0); - - // Set up the delegate / dataSource proxy now, so we recieve key method calls from UITableView even if - // our owner never sets up asyncDelegate (technically the dataSource is required) - _proxyDelegate = [[_ASTableViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; - super.delegate = (id)_proxyDelegate; - - _proxyDataSource = [[_ASTableViewProxy alloc] initWithTarget:[NSNull null] interceptor:self]; - super.dataSource = (id)_proxyDataSource; } - (instancetype)initWithFrame:(CGRect)frame style:(UITableViewStyle)style @@ -287,14 +277,16 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDataSource in the ViewController's dealloc. In this case our _asyncDataSource // will return as nil (ARC magic) even though the _proxyDataSource still exists. It's really important to nil out - // _proxyDataSource.target in this case because calls to _ASTableViewProxy will start failing and cause crashes. + // super.dataSource in this case because calls to _ASTableViewProxy will start failing and cause crashes. if (asyncDataSource == nil) { - _proxyDataSource.target = nil; + super.dataSource = nil; _asyncDataSource = nil; + _proxyDataSource = nil; } else { - _proxyDataSource.target = asyncDataSource; _asyncDataSource = asyncDataSource; + _proxyDataSource = [[_ASTableViewProxy alloc] initWithTarget:_asyncDataSource interceptor:self]; + super.dataSource = (id)_proxyDataSource; } } @@ -303,16 +295,18 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // Note: It's common to check if the value hasn't changed and short-circuit but we aren't doing that here to handle // the (common) case of nilling the asyncDelegate in the ViewController's dealloc. In this case our _asyncDelegate // will return as nil (ARC magic) even though the _proxyDelegate still exists. It's really important to nil out - // _proxyDelegate.target in this case because calls to _ASTableViewProxy will start failing and cause crashes. + // super.delegate in this case because calls to _ASTableViewProxy will start failing and cause crashes. if (asyncDelegate == nil) { // order is important here, the delegate must be callable while nilling super.delegate to avoid random crashes // in UIScrollViewAccessibility. - _proxyDelegate.target = nil; + super.delegate = nil; _asyncDelegate = nil; + _proxyDelegate = nil; } else { - _proxyDelegate.target = asyncDelegate; _asyncDelegate = asyncDelegate; + _proxyDelegate = [[_ASTableViewProxy alloc] initWithTarget:asyncDelegate interceptor:self]; + super.delegate = (id)_proxyDelegate; } } From e92a6ce9e3440c3dddbbcbc214d4901adb8cb0ee Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 19:37:36 -0700 Subject: [PATCH 23/36] Initial work on measuring loaded cell nodes on the main thread --- AsyncDisplayKit.xcodeproj/project.pbxproj | 12 +++++++ AsyncDisplayKit/ASCellNode.h | 9 ++++- AsyncDisplayKit/Details/ASDataController.mm | 37 ++++++++++++++++++-- AsyncDisplayKit/Private/ASCellNodeInternal.h | 25 +++++++++++++ AsyncDisplayKit/Private/ASCellNodeInternal.m | 25 +++++++++++++ 5 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 AsyncDisplayKit/Private/ASCellNodeInternal.h create mode 100644 AsyncDisplayKit/Private/ASCellNodeInternal.m diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index 1a58fabfe9..d726a29a68 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -379,6 +379,10 @@ CC7FD9DF1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9DD1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m */; settings = {ASSET_TAGS = (); }; }; CC7FD9E11BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9E01BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m */; settings = {ASSET_TAGS = (); }; }; CC7FD9E21BB603FF005CCB2B /* ASPhotosFrameworkImageRequest.h in Headers */ = {isa = PBXBuildFile; fileRef = CC7FD9DC1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.h */; settings = {ATTRIBUTES = (Public, ); }; }; + CCBFF68F1BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */; settings = {ASSET_TAGS = (); }; }; + CCBFF6901BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */; settings = {ASSET_TAGS = (); }; }; + CCBFF6911BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */ = {isa = PBXBuildFile; fileRef = CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */; settings = {ASSET_TAGS = (); }; }; + CCBFF6921BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */ = {isa = PBXBuildFile; fileRef = CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */; settings = {ASSET_TAGS = (); }; }; D785F6621A74327E00291744 /* ASScrollNode.h in Headers */ = {isa = PBXBuildFile; fileRef = D785F6601A74327E00291744 /* ASScrollNode.h */; settings = {ATTRIBUTES = (Public, ); }; }; D785F6631A74327E00291744 /* ASScrollNode.m in Sources */ = {isa = PBXBuildFile; fileRef = D785F6611A74327E00291744 /* ASScrollNode.m */; }; DB7121BCD50849C498C886FB /* libPods-AsyncDisplayKitTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EFA731F0396842FF8AB635EE /* libPods-AsyncDisplayKitTests.a */; }; @@ -627,6 +631,8 @@ CC7FD9DC1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASPhotosFrameworkImageRequest.h; sourceTree = ""; }; CC7FD9DD1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPhotosFrameworkImageRequest.m; sourceTree = ""; }; CC7FD9E01BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPhotosFrameworkImageRequestTests.m; sourceTree = ""; }; + CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASCellNodeInternal.h; sourceTree = ""; }; + CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASCellNodeInternal.m; sourceTree = ""; }; D3779BCFF841AD3EB56537ED /* Pods-AsyncDisplayKitTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AsyncDisplayKitTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-AsyncDisplayKitTests/Pods-AsyncDisplayKitTests.release.xcconfig"; sourceTree = ""; }; D785F6601A74327E00291744 /* ASScrollNode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASScrollNode.h; sourceTree = ""; }; D785F6611A74327E00291744 /* ASScrollNode.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASScrollNode.m; sourceTree = ""; }; @@ -925,6 +931,8 @@ 058D0A01195D050800B7D73C /* Private */ = { isa = PBXGroup; children = ( + CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */, + CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */, 9C65A7291BA8EA4D0084DA91 /* ASLayoutOptionsPrivate.h */, 9C8221931BA237B80037F19A /* ASStackBaselinePositionedLayout.h */, 9C8221941BA237B80037F19A /* ASStackBaselinePositionedLayout.mm */, @@ -1047,6 +1055,7 @@ 058D0A71195D05F800B7D73C /* _AS-objc-internal.h in Headers */, 058D0A68195D05EC00B7D73C /* _ASAsyncTransaction.h in Headers */, 058D0A6A195D05EC00B7D73C /* _ASAsyncTransactionContainer+Private.h in Headers */, + CCBFF68F1BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */, 058D0A6B195D05EC00B7D73C /* _ASAsyncTransactionContainer.h in Headers */, 058D0A6D195D05EC00B7D73C /* _ASAsyncTransactionGroup.h in Headers */, 058D0A72195D05F800B7D73C /* _ASCoreAnimationExtras.h in Headers */, @@ -1150,6 +1159,7 @@ B35062481B010EFD0018CF92 /* _AS-objc-internal.h in Headers */, B350623C1B010EFD0018CF92 /* _ASAsyncTransaction.h in Headers */, B350623E1B010EFD0018CF92 /* _ASAsyncTransactionContainer+Private.h in Headers */, + CCBFF6901BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */, B350623F1B010EFD0018CF92 /* _ASAsyncTransactionContainer.h in Headers */, B35062411B010EFD0018CF92 /* _ASAsyncTransactionGroup.h in Headers */, B35062491B010EFD0018CF92 /* _ASCoreAnimationExtras.h in Headers */, @@ -1442,6 +1452,7 @@ 058D0A23195D050800B7D73C /* _ASAsyncTransactionContainer.m in Sources */, 058D0A24195D050800B7D73C /* _ASAsyncTransactionGroup.m in Sources */, 058D0A26195D050800B7D73C /* _ASCoreAnimationExtras.mm in Sources */, + CCBFF6911BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */, 058D0A18195D050800B7D73C /* _ASDisplayLayer.mm in Sources */, 058D0A19195D050800B7D73C /* _ASDisplayView.mm in Sources */, 058D0A27195D050800B7D73C /* _ASPendingState.m in Sources */, @@ -1552,6 +1563,7 @@ B35062401B010EFD0018CF92 /* _ASAsyncTransactionContainer.m in Sources */, B35062421B010EFD0018CF92 /* _ASAsyncTransactionGroup.m in Sources */, B350624A1B010EFD0018CF92 /* _ASCoreAnimationExtras.mm in Sources */, + CCBFF6921BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */, 2767E9421BB19BD600EA9B77 /* ASViewController.m in Sources */, B35062101B010EFD0018CF92 /* _ASDisplayLayer.mm in Sources */, B35062121B010EFD0018CF92 /* _ASDisplayView.mm in Sources */, diff --git a/AsyncDisplayKit/ASCellNode.h b/AsyncDisplayKit/ASCellNode.h index ae061eb6ca..96970cc479 100644 --- a/AsyncDisplayKit/ASCellNode.h +++ b/AsyncDisplayKit/ASCellNode.h @@ -12,7 +12,9 @@ /** * Generic cell node. Subclass this instead of `ASDisplayNode` to use with `ASTableView` and `ASCollectionView`. */ -@interface ASCellNode : ASDisplayNode +@interface ASCellNode : ASDisplayNode { + BOOL _needsMeasure; +} /** * @abstract When enabled, ensures that the cell is completely displayed before allowed onscreen. @@ -52,6 +54,11 @@ - (void)touchesEnded:(NSSet *)touches withEvent:(UIEvent *)event ASDISPLAYNODE_REQUIRES_SUPER; - (void)touchesCancelled:(NSSet *)touches withEvent:(UIEvent *)event ASDISPLAYNODE_REQUIRES_SUPER; +/* + * + */ +@property (nonatomic) BOOL needsMeasure; + @end diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 7b07ff76ef..251d75ecaf 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -15,6 +15,7 @@ #import "ASDisplayNode.h" #import "ASMultidimensionalArrayUtils.h" #import "ASDisplayNodeInternal.h" +#import "ASCellNodeInternal.h" //#define LOG(...) NSLog(__VA_ARGS__) #define LOG(...) @@ -96,6 +97,23 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; #pragma mark - Cell Layout +/* + * Once nodes have loaded their views, we can't layout in the background so this is a chance + * to do so immediately on the main thread. + */ +- (void)_layoutNodesWithMainThreadAffinity:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths { + NSAssert(NSThread.isMainThread, @"Main thread layout must be on the main thread."); + [indexPaths enumerateObjectsUsingBlock:^(NSIndexPath *indexPath, NSUInteger idx, __unused BOOL * stop) { + ASCellNode *node = nodes[idx]; + if (node.isNodeLoaded && node.needsMeasure) { + ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; + [node measureWithSizeRange:constrainedSize]; + node.frame = CGRectMake(0, 0, node.calculatedSize.width, node.calculatedSize.height); + node.needsMeasure = NO; + } + }]; +} + - (void)_layoutNodes:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions { ASDisplayNodeAssert([NSOperationQueue currentQueue] == _editingTransactionQueue, @"Cell node layout must be initiated from edit transaction queue"); @@ -117,8 +135,12 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; for (NSUInteger k = j; k < j + batchCount; k++) { ASCellNode *node = nodes[k]; ASSizeRange constrainedSize = nodeBoundSizes[k]; - [node measureWithSizeRange:constrainedSize]; - node.frame = CGRectMake(0, 0, node.calculatedSize.width, node.calculatedSize.height); + // Nodes with main thread affinity should all have already been measured. + if (node.needsMeasure) { + [node measureWithSizeRange:constrainedSize]; + node.frame = CGRectMake(0, 0, node.calculatedSize.width, node.calculatedSize.height); + node.needsMeasure = NO; + } } }); } @@ -245,6 +267,8 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSMutableArray *updatedIndexPaths = [NSMutableArray array]; [self _populateFromEntireDataSourceWithMutableNodes:updatedNodes mutableIndexPaths:updatedIndexPaths]; + [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; + [_editingTransactionQueue addOperationWithBlock:^{ LOG(@"Edit Transaction - reloadData"); @@ -399,6 +423,8 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSMutableArray *updatedIndexPaths = [NSMutableArray array]; [self _populateFromDataSourceWithSectionIndexSet:indexSet mutableNodes:updatedNodes mutableIndexPaths:updatedIndexPaths]; + [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; + [_editingTransactionQueue addOperationWithBlock:^{ LOG(@"Edit Transaction - insertSections: %@", indexSet); NSMutableArray *sectionArray = [NSMutableArray arrayWithCapacity:indexSet.count]; @@ -448,6 +474,8 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; // For example, if an initial -reloadData call is quickly followed by -reloadSections, sizing the initial set may not be done // at this time. Thus _editingNodes could be empty and crash in ASIndexPathsForMultidimensional[...] + [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; + [_editingTransactionQueue addOperationWithBlock:^{ NSArray *indexPaths = ASIndexPathsForMultidimensionalArrayAtIndexSet(_editingNodes, sections); @@ -510,6 +538,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:sortedIndexPaths[i]]]; } + [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ LOG(@"Edit Transaction - insertRows: %@", indexPaths); [self _batchLayoutNodes:nodes atIndexPaths:indexPaths withAnimationOptions:animationOptions]; @@ -552,6 +581,9 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:indexPath]]; }]; + // FIXME: Is there any reason I should split the edit transaction queue work into two phases, and do this layout in between rather than before? + [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; + [_editingTransactionQueue addOperationWithBlock:^{ LOG(@"Edit Transaction - reloadRows: %@", indexPaths); [self _deleteNodesAtIndexPaths:indexPaths withAnimationOptions:animationOptions]; @@ -580,6 +612,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); + node.needsMeasure = NO; }]; }]; }]; diff --git a/AsyncDisplayKit/Private/ASCellNodeInternal.h b/AsyncDisplayKit/Private/ASCellNodeInternal.h new file mode 100644 index 0000000000..a0477d1ed7 --- /dev/null +++ b/AsyncDisplayKit/Private/ASCellNodeInternal.h @@ -0,0 +1,25 @@ +// +// ASCellNodeInternal.h +// AsyncDisplayKit +// +// Created by Adlai Holler on 10/9/15. +// Copyright © 2015 Facebook. All rights reserved. +// + +#import + +#import + +@interface ASCellNode (Internal) + +/* + * @abstract Should this node be remeasured when the data controller next adds it? + * + * @discussion If possible, cell nodes should be measured in the background. However, + * we cannot violate a node's thread affinity. When nodes are added in a data controller, + * nodes with main thread affinity will be measured immediately on the main thread and this + * flag will be cleared, so the node will be skipped during the background measurement pass. + */ +@property (nonatomic) BOOL needsMeasure; + +@end diff --git a/AsyncDisplayKit/Private/ASCellNodeInternal.m b/AsyncDisplayKit/Private/ASCellNodeInternal.m new file mode 100644 index 0000000000..eee4e317a8 --- /dev/null +++ b/AsyncDisplayKit/Private/ASCellNodeInternal.m @@ -0,0 +1,25 @@ +// +// ASCellNodeInternal.m +// AsyncDisplayKit +// +// Created by Adlai Holler on 10/9/15. +// Copyright © 2015 Facebook. All rights reserved. +// + +#import "ASCellNodeInternal.h" + +@implementation ASCellNode (Internal) + +// FIXME: Lock this + +- (BOOL)needsMeasure +{ + return _needsMeasure; +} + +- (void)setNeedsMeasure:(BOOL)needsMeasure +{ + _needsMeasure = needsMeasure; +} + +@end From e716ccb580f46115f3cc52506b4b125163cc3b81 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 19:51:12 -0700 Subject: [PATCH 24/36] Don't set cell node frames during measure, set them just before returning the cell size to UITableView/UICollectionView --- AsyncDisplayKit/ASCollectionView.mm | 7 ++++++- AsyncDisplayKit/ASTableView.mm | 10 +++++++--- AsyncDisplayKit/Details/ASDataController.mm | 2 -- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index b1b29335b3..108fc0f16b 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -423,7 +423,12 @@ static BOOL _isInterceptedSelector(SEL sel) - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath { - return [[_dataController nodeAtIndexPath:indexPath] calculatedSize]; + ASCellNode *node = [_dataController nodeAtIndexPath:indexPath]; + CGSize size = node.calculatedSize; + if (!CGSizeEqualToSize(size, node.frame.size)) { + node.frame = CGRectMake(0, 0, size.width, size.height); + } + return size; } - (NSInteger)numberOfSectionsInCollectionView:(UICollectionView *)collectionView diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index 7c36c775a6..c4a65c598b 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -537,7 +537,12 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { - (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath { ASCellNode *node = [_dataController nodeAtIndexPath:indexPath]; - return node.calculatedSize.height; + CGSize size = node.calculatedSize; + // Update node's frame to ensure it will fill its cell. + if (!CGSizeEqualToSize(node.frame.size, size)) { + node.frame = CGRectMake(0, 0, size.width, size.height); + } + return size.height; } - (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView @@ -881,8 +886,7 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // Also, in many cases, some nodes may not need to be re-measured at all, such as when user enters and then immediately leaves editing mode. // To avoid premature optimization and making such assumption, as well as to keep ASTableView simple, re-measurement is strictly done on main. [self beginUpdates]; - CGSize calculatedSize = [[node measureWithSizeRange:constrainedSize] size]; - node.frame = CGRectMake(0, 0, calculatedSize.width, calculatedSize.height); + [node measureWithSizeRange:constrainedSize]; [self endUpdates]; } } diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 7b07ff76ef..2e1f8a9a03 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -118,7 +118,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; ASCellNode *node = nodes[k]; ASSizeRange constrainedSize = nodeBoundSizes[k]; [node measureWithSizeRange:constrainedSize]; - node.frame = CGRectMake(0, 0, node.calculatedSize.width, node.calculatedSize.height); } }); } @@ -579,7 +578,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSIndexPath *indexPath = [NSIndexPath indexPathForRow:rowIndex inSection:sectionIndex]; ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; - node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); }]; }]; }]; From 93cdc0f2f53de0cbdd351b19fc3ec3be62a33b72 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 19:55:14 -0700 Subject: [PATCH 25/36] Don't set cell node frames during measurement --- AsyncDisplayKit/Details/ASDataController.mm | 2 -- 1 file changed, 2 deletions(-) diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 74f5b62bf5..8efd41daf6 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -108,7 +108,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; if (node.isNodeLoaded && node.needsMeasure) { ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; - node.frame = CGRectMake(0, 0, node.calculatedSize.width, node.calculatedSize.height); node.needsMeasure = NO; } }]; @@ -138,7 +137,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; // Nodes with main thread affinity should all have already been measured. if (node.needsMeasure) { [node measureWithSizeRange:constrainedSize]; - node.frame = CGRectMake(0, 0, node.calculatedSize.width, node.calculatedSize.height); node.needsMeasure = NO; } } From 06b7897bc151d8bbfc001e55707784cf31c9a371 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 20:06:59 -0700 Subject: [PATCH 26/36] Finish measuring on main thread when possible --- AsyncDisplayKit/ASCellNode.m | 3 ++- AsyncDisplayKit/Details/ASDataController.mm | 13 +++++++++---- AsyncDisplayKit/Private/ASCellNodeInternal.m | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/AsyncDisplayKit/ASCellNode.m b/AsyncDisplayKit/ASCellNode.m index 80bc34598c..66047d3f46 100644 --- a/AsyncDisplayKit/ASCellNode.m +++ b/AsyncDisplayKit/ASCellNode.m @@ -27,7 +27,8 @@ // use UITableViewCell defaults _selectionStyle = UITableViewCellSelectionStyleDefault; self.clipsToBounds = YES; - + _needsMeasure = YES; + return self; } diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 8efd41daf6..6a0c260463 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -98,11 +98,12 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; #pragma mark - Cell Layout /* - * Once nodes have loaded their views, we can't layout in the background so this is a chance + * Once nodes have loaded their views, we can't measure in the background so this is a chance * to do so immediately on the main thread. */ - (void)_layoutNodesWithMainThreadAffinity:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths { NSAssert(NSThread.isMainThread, @"Main thread layout must be on the main thread."); + [indexPaths enumerateObjectsUsingBlock:^(NSIndexPath *indexPath, NSUInteger idx, __unused BOOL * stop) { ASCellNode *node = nodes[idx]; if (node.isNodeLoaded && node.needsMeasure) { @@ -113,6 +114,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; }]; } +// FIXME: Isn't this name sort of misleading? We don't lay the node out we just measure it. _measureNodes? - (void)_layoutNodes:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions { ASDisplayNodeAssert([NSOperationQueue currentQueue] == _editingTransactionQueue, @"Cell node layout must be initiated from edit transaction queue"); @@ -127,15 +129,18 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSInteger batchCount = MIN(kASDataControllerSizingCountPerProcessor, indexPaths.count - j); for (NSUInteger k = j; k < j + batchCount; k++) { - nodeBoundSizes[k] = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPaths[k]]; + ASCellNode *node = nodes[k]; + if (node.needsMeasure) { + nodeBoundSizes[k] = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPaths[k]]; + } } dispatch_group_async(layoutGroup, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ for (NSUInteger k = j; k < j + batchCount; k++) { ASCellNode *node = nodes[k]; - ASSizeRange constrainedSize = nodeBoundSizes[k]; - // Nodes with main thread affinity should all have already been measured. if (node.needsMeasure) { + ASDisplayNodeAssert(!node.isNodeLoaded, @"Nodes that are loaded should already have been measured on the main thread."); + ASSizeRange constrainedSize = nodeBoundSizes[k]; [node measureWithSizeRange:constrainedSize]; node.needsMeasure = NO; } diff --git a/AsyncDisplayKit/Private/ASCellNodeInternal.m b/AsyncDisplayKit/Private/ASCellNodeInternal.m index eee4e317a8..0926e6f35a 100644 --- a/AsyncDisplayKit/Private/ASCellNodeInternal.m +++ b/AsyncDisplayKit/Private/ASCellNodeInternal.m @@ -10,7 +10,7 @@ @implementation ASCellNode (Internal) -// FIXME: Lock this +// FIXME: Is locking this worth the extra lock? - (BOOL)needsMeasure { From fdb11275db323e947f3cd855ef7f37b6f58875c5 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 20:23:54 -0700 Subject: [PATCH 27/36] Put back the frame-setting behavior when measuring cell nodes --- AsyncDisplayKit/ASCollectionView.mm | 7 +------ AsyncDisplayKit/ASTableView.mm | 7 +------ AsyncDisplayKit/Details/ASDataController.mm | 6 ++++-- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index 108fc0f16b..b1b29335b3 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -423,12 +423,7 @@ static BOOL _isInterceptedSelector(SEL sel) - (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath { - ASCellNode *node = [_dataController nodeAtIndexPath:indexPath]; - CGSize size = node.calculatedSize; - if (!CGSizeEqualToSize(size, node.frame.size)) { - node.frame = CGRectMake(0, 0, size.width, size.height); - } - return size; + return [[_dataController nodeAtIndexPath:indexPath] calculatedSize]; } - (NSInteger)numberOfSectionsInCollectionView:(UICollectionView *)collectionView diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index c4a65c598b..80f1894529 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -537,12 +537,7 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { - (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath { ASCellNode *node = [_dataController nodeAtIndexPath:indexPath]; - CGSize size = node.calculatedSize; - // Update node's frame to ensure it will fill its cell. - if (!CGSizeEqualToSize(node.frame.size, size)) { - node.frame = CGRectMake(0, 0, size.width, size.height); - } - return size.height; + return node.calculatedSize.height; } - (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 6a0c260463..7c0a7311d6 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -98,7 +98,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; #pragma mark - Cell Layout /* - * Once nodes have loaded their views, we can't measure in the background so this is a chance + * Once nodes have loaded their views, we can't layout in the background so this is a chance * to do so immediately on the main thread. */ - (void)_layoutNodesWithMainThreadAffinity:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths { @@ -109,12 +109,12 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; if (node.isNodeLoaded && node.needsMeasure) { ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; + node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); node.needsMeasure = NO; } }]; } -// FIXME: Isn't this name sort of misleading? We don't lay the node out we just measure it. _measureNodes? - (void)_layoutNodes:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions { ASDisplayNodeAssert([NSOperationQueue currentQueue] == _editingTransactionQueue, @"Cell node layout must be initiated from edit transaction queue"); @@ -142,6 +142,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; ASDisplayNodeAssert(!node.isNodeLoaded, @"Nodes that are loaded should already have been measured on the main thread."); ASSizeRange constrainedSize = nodeBoundSizes[k]; [node measureWithSizeRange:constrainedSize]; + node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); node.needsMeasure = NO; } } @@ -614,6 +615,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSIndexPath *indexPath = [NSIndexPath indexPathForRow:rowIndex inSection:sectionIndex]; ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; + node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); node.needsMeasure = NO; }]; }]; From d8e99e00bd8faffbebb4f1bc78a3a7fe10c9c3d8 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 20:27:38 -0700 Subject: [PATCH 28/36] Remove public needsMeasure --- AsyncDisplayKit/ASCellNode.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/AsyncDisplayKit/ASCellNode.h b/AsyncDisplayKit/ASCellNode.h index 96970cc479..ffc3ba1007 100644 --- a/AsyncDisplayKit/ASCellNode.h +++ b/AsyncDisplayKit/ASCellNode.h @@ -54,11 +54,6 @@ - (void)touchesEnded:(NSSet *)touches withEvent:(UIEvent *)event ASDISPLAYNODE_REQUIRES_SUPER; - (void)touchesCancelled:(NSSet *)touches withEvent:(UIEvent *)event ASDISPLAYNODE_REQUIRES_SUPER; -/* - * - */ -@property (nonatomic) BOOL needsMeasure; - @end From 0848aac18600f6917be63825c32ccf0ffb0f7b59 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 20:28:42 -0700 Subject: [PATCH 29/36] Revert prior change --- AsyncDisplayKit/ASTableView.mm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index 80f1894529..7c36c775a6 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -881,7 +881,8 @@ void ASPerformBlockWithoutAnimation(BOOL withoutAnimation, void (^block)()) { // Also, in many cases, some nodes may not need to be re-measured at all, such as when user enters and then immediately leaves editing mode. // To avoid premature optimization and making such assumption, as well as to keep ASTableView simple, re-measurement is strictly done on main. [self beginUpdates]; - [node measureWithSizeRange:constrainedSize]; + CGSize calculatedSize = [[node measureWithSizeRange:constrainedSize] size]; + node.frame = CGRectMake(0, 0, calculatedSize.width, calculatedSize.height); [self endUpdates]; } } From 7f42b37decc82d2f1f461613fb4fbbb051fadc20 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 21:13:00 -0700 Subject: [PATCH 30/36] Remove needsMeasure --- AsyncDisplayKit/ASCellNode.h | 4 +--- AsyncDisplayKit/ASCellNode.m | 1 - AsyncDisplayKit/Details/ASDataController.mm | 10 +++------- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/AsyncDisplayKit/ASCellNode.h b/AsyncDisplayKit/ASCellNode.h index ffc3ba1007..ae061eb6ca 100644 --- a/AsyncDisplayKit/ASCellNode.h +++ b/AsyncDisplayKit/ASCellNode.h @@ -12,9 +12,7 @@ /** * Generic cell node. Subclass this instead of `ASDisplayNode` to use with `ASTableView` and `ASCollectionView`. */ -@interface ASCellNode : ASDisplayNode { - BOOL _needsMeasure; -} +@interface ASCellNode : ASDisplayNode /** * @abstract When enabled, ensures that the cell is completely displayed before allowed onscreen. diff --git a/AsyncDisplayKit/ASCellNode.m b/AsyncDisplayKit/ASCellNode.m index 66047d3f46..df89896ce9 100644 --- a/AsyncDisplayKit/ASCellNode.m +++ b/AsyncDisplayKit/ASCellNode.m @@ -27,7 +27,6 @@ // use UITableViewCell defaults _selectionStyle = UITableViewCellSelectionStyleDefault; self.clipsToBounds = YES; - _needsMeasure = YES; return self; } diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 7c0a7311d6..b41390e6a4 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -15,7 +15,6 @@ #import "ASDisplayNode.h" #import "ASMultidimensionalArrayUtils.h" #import "ASDisplayNodeInternal.h" -#import "ASCellNodeInternal.h" //#define LOG(...) NSLog(__VA_ARGS__) #define LOG(...) @@ -106,11 +105,10 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [indexPaths enumerateObjectsUsingBlock:^(NSIndexPath *indexPath, NSUInteger idx, __unused BOOL * stop) { ASCellNode *node = nodes[idx]; - if (node.isNodeLoaded && node.needsMeasure) { + if (node.isNodeLoaded) { ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); - node.needsMeasure = NO; } }]; } @@ -130,7 +128,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; for (NSUInteger k = j; k < j + batchCount; k++) { ASCellNode *node = nodes[k]; - if (node.needsMeasure) { + if (!node.isNodeLoaded) { nodeBoundSizes[k] = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPaths[k]]; } } @@ -138,12 +136,11 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; dispatch_group_async(layoutGroup, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ for (NSUInteger k = j; k < j + batchCount; k++) { ASCellNode *node = nodes[k]; - if (node.needsMeasure) { + if (!node.isNodeLoaded) { ASDisplayNodeAssert(!node.isNodeLoaded, @"Nodes that are loaded should already have been measured on the main thread."); ASSizeRange constrainedSize = nodeBoundSizes[k]; [node measureWithSizeRange:constrainedSize]; node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); - node.needsMeasure = NO; } } }); @@ -616,7 +613,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; ASSizeRange constrainedSize = [_dataSource dataController:self constrainedSizeForNodeAtIndexPath:indexPath]; [node measureWithSizeRange:constrainedSize]; node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); - node.needsMeasure = NO; }]; }]; }]; From 0eb3490363259497cf05a654c8aaaeedaf5def12 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 21:15:32 -0700 Subject: [PATCH 31/36] Finish removing needsMeasure --- AsyncDisplayKit.xcodeproj/project.pbxproj | 12 ---------- AsyncDisplayKit/ASCellNode.m | 2 +- AsyncDisplayKit/Private/ASCellNodeInternal.h | 25 -------------------- AsyncDisplayKit/Private/ASCellNodeInternal.m | 25 -------------------- 4 files changed, 1 insertion(+), 63 deletions(-) delete mode 100644 AsyncDisplayKit/Private/ASCellNodeInternal.h delete mode 100644 AsyncDisplayKit/Private/ASCellNodeInternal.m diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index d726a29a68..1a58fabfe9 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -379,10 +379,6 @@ CC7FD9DF1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9DD1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m */; settings = {ASSET_TAGS = (); }; }; CC7FD9E11BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9E01BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m */; settings = {ASSET_TAGS = (); }; }; CC7FD9E21BB603FF005CCB2B /* ASPhotosFrameworkImageRequest.h in Headers */ = {isa = PBXBuildFile; fileRef = CC7FD9DC1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.h */; settings = {ATTRIBUTES = (Public, ); }; }; - CCBFF68F1BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */; settings = {ASSET_TAGS = (); }; }; - CCBFF6901BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */; settings = {ASSET_TAGS = (); }; }; - CCBFF6911BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */ = {isa = PBXBuildFile; fileRef = CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */; settings = {ASSET_TAGS = (); }; }; - CCBFF6921BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */ = {isa = PBXBuildFile; fileRef = CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */; settings = {ASSET_TAGS = (); }; }; D785F6621A74327E00291744 /* ASScrollNode.h in Headers */ = {isa = PBXBuildFile; fileRef = D785F6601A74327E00291744 /* ASScrollNode.h */; settings = {ATTRIBUTES = (Public, ); }; }; D785F6631A74327E00291744 /* ASScrollNode.m in Sources */ = {isa = PBXBuildFile; fileRef = D785F6611A74327E00291744 /* ASScrollNode.m */; }; DB7121BCD50849C498C886FB /* libPods-AsyncDisplayKitTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EFA731F0396842FF8AB635EE /* libPods-AsyncDisplayKitTests.a */; }; @@ -631,8 +627,6 @@ CC7FD9DC1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASPhotosFrameworkImageRequest.h; sourceTree = ""; }; CC7FD9DD1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPhotosFrameworkImageRequest.m; sourceTree = ""; }; CC7FD9E01BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASPhotosFrameworkImageRequestTests.m; sourceTree = ""; }; - CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASCellNodeInternal.h; sourceTree = ""; }; - CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASCellNodeInternal.m; sourceTree = ""; }; D3779BCFF841AD3EB56537ED /* Pods-AsyncDisplayKitTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AsyncDisplayKitTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-AsyncDisplayKitTests/Pods-AsyncDisplayKitTests.release.xcconfig"; sourceTree = ""; }; D785F6601A74327E00291744 /* ASScrollNode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASScrollNode.h; sourceTree = ""; }; D785F6611A74327E00291744 /* ASScrollNode.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASScrollNode.m; sourceTree = ""; }; @@ -931,8 +925,6 @@ 058D0A01195D050800B7D73C /* Private */ = { isa = PBXGroup; children = ( - CCBFF68D1BC8A8BA00EF0162 /* ASCellNodeInternal.h */, - CCBFF68E1BC8A8BA00EF0162 /* ASCellNodeInternal.m */, 9C65A7291BA8EA4D0084DA91 /* ASLayoutOptionsPrivate.h */, 9C8221931BA237B80037F19A /* ASStackBaselinePositionedLayout.h */, 9C8221941BA237B80037F19A /* ASStackBaselinePositionedLayout.mm */, @@ -1055,7 +1047,6 @@ 058D0A71195D05F800B7D73C /* _AS-objc-internal.h in Headers */, 058D0A68195D05EC00B7D73C /* _ASAsyncTransaction.h in Headers */, 058D0A6A195D05EC00B7D73C /* _ASAsyncTransactionContainer+Private.h in Headers */, - CCBFF68F1BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */, 058D0A6B195D05EC00B7D73C /* _ASAsyncTransactionContainer.h in Headers */, 058D0A6D195D05EC00B7D73C /* _ASAsyncTransactionGroup.h in Headers */, 058D0A72195D05F800B7D73C /* _ASCoreAnimationExtras.h in Headers */, @@ -1159,7 +1150,6 @@ B35062481B010EFD0018CF92 /* _AS-objc-internal.h in Headers */, B350623C1B010EFD0018CF92 /* _ASAsyncTransaction.h in Headers */, B350623E1B010EFD0018CF92 /* _ASAsyncTransactionContainer+Private.h in Headers */, - CCBFF6901BC8A8BA00EF0162 /* ASCellNodeInternal.h in Headers */, B350623F1B010EFD0018CF92 /* _ASAsyncTransactionContainer.h in Headers */, B35062411B010EFD0018CF92 /* _ASAsyncTransactionGroup.h in Headers */, B35062491B010EFD0018CF92 /* _ASCoreAnimationExtras.h in Headers */, @@ -1452,7 +1442,6 @@ 058D0A23195D050800B7D73C /* _ASAsyncTransactionContainer.m in Sources */, 058D0A24195D050800B7D73C /* _ASAsyncTransactionGroup.m in Sources */, 058D0A26195D050800B7D73C /* _ASCoreAnimationExtras.mm in Sources */, - CCBFF6911BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */, 058D0A18195D050800B7D73C /* _ASDisplayLayer.mm in Sources */, 058D0A19195D050800B7D73C /* _ASDisplayView.mm in Sources */, 058D0A27195D050800B7D73C /* _ASPendingState.m in Sources */, @@ -1563,7 +1552,6 @@ B35062401B010EFD0018CF92 /* _ASAsyncTransactionContainer.m in Sources */, B35062421B010EFD0018CF92 /* _ASAsyncTransactionGroup.m in Sources */, B350624A1B010EFD0018CF92 /* _ASCoreAnimationExtras.mm in Sources */, - CCBFF6921BC8A8BA00EF0162 /* ASCellNodeInternal.m in Sources */, 2767E9421BB19BD600EA9B77 /* ASViewController.m in Sources */, B35062101B010EFD0018CF92 /* _ASDisplayLayer.mm in Sources */, B35062121B010EFD0018CF92 /* _ASDisplayView.mm in Sources */, diff --git a/AsyncDisplayKit/ASCellNode.m b/AsyncDisplayKit/ASCellNode.m index df89896ce9..80bc34598c 100644 --- a/AsyncDisplayKit/ASCellNode.m +++ b/AsyncDisplayKit/ASCellNode.m @@ -27,7 +27,7 @@ // use UITableViewCell defaults _selectionStyle = UITableViewCellSelectionStyleDefault; self.clipsToBounds = YES; - + return self; } diff --git a/AsyncDisplayKit/Private/ASCellNodeInternal.h b/AsyncDisplayKit/Private/ASCellNodeInternal.h deleted file mode 100644 index a0477d1ed7..0000000000 --- a/AsyncDisplayKit/Private/ASCellNodeInternal.h +++ /dev/null @@ -1,25 +0,0 @@ -// -// ASCellNodeInternal.h -// AsyncDisplayKit -// -// Created by Adlai Holler on 10/9/15. -// Copyright © 2015 Facebook. All rights reserved. -// - -#import - -#import - -@interface ASCellNode (Internal) - -/* - * @abstract Should this node be remeasured when the data controller next adds it? - * - * @discussion If possible, cell nodes should be measured in the background. However, - * we cannot violate a node's thread affinity. When nodes are added in a data controller, - * nodes with main thread affinity will be measured immediately on the main thread and this - * flag will be cleared, so the node will be skipped during the background measurement pass. - */ -@property (nonatomic) BOOL needsMeasure; - -@end diff --git a/AsyncDisplayKit/Private/ASCellNodeInternal.m b/AsyncDisplayKit/Private/ASCellNodeInternal.m deleted file mode 100644 index 0926e6f35a..0000000000 --- a/AsyncDisplayKit/Private/ASCellNodeInternal.m +++ /dev/null @@ -1,25 +0,0 @@ -// -// ASCellNodeInternal.m -// AsyncDisplayKit -// -// Created by Adlai Holler on 10/9/15. -// Copyright © 2015 Facebook. All rights reserved. -// - -#import "ASCellNodeInternal.h" - -@implementation ASCellNode (Internal) - -// FIXME: Is locking this worth the extra lock? - -- (BOOL)needsMeasure -{ - return _needsMeasure; -} - -- (void)setNeedsMeasure:(BOOL)needsMeasure -{ - _needsMeasure = needsMeasure; -} - -@end From 1e232561f44e6dfff51cd09322d5c1de90c2ae4b Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 21:16:28 -0700 Subject: [PATCH 32/36] Whitespace --- AsyncDisplayKit/Details/ASDataController.mm | 1 + 1 file changed, 1 insertion(+) diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index b41390e6a4..9757771cc6 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -540,6 +540,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; } [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; + [_editingTransactionQueue addOperationWithBlock:^{ LOG(@"Edit Transaction - insertRows: %@", indexPaths); [self _batchLayoutNodes:nodes atIndexPaths:indexPaths withAnimationOptions:animationOptions]; From 0c361894ac2e966eeff316ed4498a8c42bec4e6a Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 9 Oct 2015 21:17:11 -0700 Subject: [PATCH 33/36] Remove comment --- AsyncDisplayKit/Details/ASDataController.mm | 1 - 1 file changed, 1 deletion(-) diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 9757771cc6..37813a4c51 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -583,7 +583,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:indexPath]]; }]; - // FIXME: Is there any reason I should split the edit transaction queue work into two phases, and do this layout in between rather than before? [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ From 595891629c3f40083f909d603306172671f38e97 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 10 Oct 2015 10:33:41 -0700 Subject: [PATCH 34/36] Remove pointless assertion --- AsyncDisplayKit/Details/ASDataController.mm | 1 - 1 file changed, 1 deletion(-) diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 37813a4c51..4fe4312962 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -137,7 +137,6 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; for (NSUInteger k = j; k < j + batchCount; k++) { ASCellNode *node = nodes[k]; if (!node.isNodeLoaded) { - ASDisplayNodeAssert(!node.isNodeLoaded, @"Nodes that are loaded should already have been measured on the main thread."); ASSizeRange constrainedSize = nodeBoundSizes[k]; [node measureWithSizeRange:constrainedSize]; node.frame = CGRectMake(0.0f, 0.0f, node.calculatedSize.width, node.calculatedSize.height); From d964364cd51c07a766c9c46e06843c116de2d001 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sun, 11 Oct 2015 08:39:29 -0700 Subject: [PATCH 35/36] Add some documentation in DataController, plus use Fast Enumeration more --- AsyncDisplayKit/Details/ASDataController.mm | 24 +++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 4fe4312962..3b39bdb707 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -97,6 +97,8 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; #pragma mark - Cell Layout /* + * FIXME: Shouldn't this method, as well as `_layoutNodes:atIndexPaths:withAnimationOptions:` use the word "measure" instead? + * * Once nodes have loaded their views, we can't layout in the background so this is a chance * to do so immediately on the main thread. */ @@ -136,6 +138,8 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; dispatch_group_async(layoutGroup, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ for (NSUInteger k = j; k < j + batchCount; k++) { ASCellNode *node = nodes[k]; + // Only measure nodes whose views aren't loaded, since we're in the background. + // We should already have measured loaded nodes before we left the main thread, using _layoutNodesWithMainThreadAffinity: if (!node.isNodeLoaded) { ASSizeRange constrainedSize = nodeBoundSizes[k]; [node measureWithSizeRange:constrainedSize]; @@ -267,6 +271,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSMutableArray *updatedIndexPaths = [NSMutableArray array]; [self _populateFromEntireDataSourceWithMutableNodes:updatedNodes mutableIndexPaths:updatedIndexPaths]; + // Measure nodes with [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -510,9 +515,9 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; // update the section of indexpaths NSIndexPath *sectionIndexPath = [[NSIndexPath alloc] initWithIndex:newSection]; NSMutableArray *updatedIndexPaths = [[NSMutableArray alloc] initWithCapacity:indexPaths.count]; - [indexPaths enumerateObjectsUsingBlock:^(NSIndexPath *indexPath, NSUInteger idx, BOOL *stop) { + for (NSIndexPath *indexPath in indexPaths) { [updatedIndexPaths addObject:[sectionIndexPath indexPathByAddingIndex:[indexPath indexAtPosition:indexPath.length - 1]]]; - }]; + } // Don't re-calculate size for moving [self _insertNodes:nodes atIndexPaths:updatedIndexPaths withAnimationOptions:animationOptions]; @@ -538,6 +543,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:sortedIndexPaths[i]]]; } + // Layout nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -557,6 +563,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [_editingTransactionQueue waitUntilAllOperationsAreFinished]; // sort indexPath in order to avoid messing up the index when deleting + // FIXME: Shouldn't deletes be sorted in descending order? NSArray *sortedIndexPaths = [indexPaths sortedArrayUsingSelector:@selector(compare:)]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -577,11 +584,16 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; // Reloading requires re-fetching the data. Load it on the current calling thread, locking the data source. [self accessDataSourceWithBlock:^{ NSMutableArray *nodes = [[NSMutableArray alloc] initWithCapacity:indexPaths.count]; - [indexPaths sortedArrayUsingSelector:@selector(compare:)]; - [indexPaths enumerateObjectsUsingBlock:^(NSIndexPath *indexPath, NSUInteger idx, BOOL *stop) { - [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:indexPath]]; - }]; + // FIXME: This doesn't currently do anything + // FIXME: Shouldn't deletes be sorted in descending order? + [indexPaths sortedArrayUsingSelector:@selector(compare:)]; + + for (NSIndexPath *indexPath in indexPaths) { + [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:indexPath]]; + } + + // Layout nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ From fbd3c77fecb013ece13d1e11a870aad6ea91ab3a Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sun, 11 Oct 2015 08:43:43 -0700 Subject: [PATCH 36/36] Finish that thought --- AsyncDisplayKit/Details/ASDataController.mm | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/AsyncDisplayKit/Details/ASDataController.mm b/AsyncDisplayKit/Details/ASDataController.mm index 3b39bdb707..6a5b361ff0 100644 --- a/AsyncDisplayKit/Details/ASDataController.mm +++ b/AsyncDisplayKit/Details/ASDataController.mm @@ -271,7 +271,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSMutableArray *updatedIndexPaths = [NSMutableArray array]; [self _populateFromEntireDataSourceWithMutableNodes:updatedNodes mutableIndexPaths:updatedIndexPaths]; - // Measure nodes with + // Measure nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -428,6 +428,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; NSMutableArray *updatedIndexPaths = [NSMutableArray array]; [self _populateFromDataSourceWithSectionIndexSet:indexSet mutableNodes:updatedNodes mutableIndexPaths:updatedIndexPaths]; + // Measure nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -479,6 +480,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; // For example, if an initial -reloadData call is quickly followed by -reloadSections, sizing the initial set may not be done // at this time. Thus _editingNodes could be empty and crash in ASIndexPathsForMultidimensional[...] + // Measure nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:updatedNodes atIndexPaths:updatedIndexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -543,7 +545,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:sortedIndexPaths[i]]]; } - // Layout nodes whose views are loaded before we leave the main thread + // Measure nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; [_editingTransactionQueue addOperationWithBlock:^{ @@ -593,7 +595,7 @@ static void *kASSizingQueueContext = &kASSizingQueueContext; [nodes addObject:[_dataSource dataController:self nodeAtIndexPath:indexPath]]; } - // Layout nodes whose views are loaded before we leave the main thread + // Measure nodes whose views are loaded before we leave the main thread [self _layoutNodesWithMainThreadAffinity:nodes atIndexPaths:indexPaths]; [_editingTransactionQueue addOperationWithBlock:^{