Add support for providing additional info to network image node delegate #775
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // | ||
| // ASNetworkImageLoadInfo.h | ||
| // AsyncDisplayKit | ||
| // | ||
| // Created by Adlai on 1/30/18. | ||
| // Copyright © 2018 Facebook. All rights reserved. | ||
| // | ||
|
|
||
| #import <Foundation/Foundation.h> | ||
| #import <AsyncDisplayKit/ASBaseDefines.h> | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| typedef NS_ENUM(NSInteger, ASNetworkImageSourceType) { | ||
| ASNetworkImageSourceUnspecified = 0, | ||
| ASNetworkImageSourceSynchronousCache, | ||
| ASNetworkImageSourceAsynchronousCache, | ||
| ASNetworkImageSourceFileURL, | ||
| ASNetworkImageSourceDownload, | ||
| }; | ||
|
|
||
| AS_SUBCLASSING_RESTRICTED | ||
| @interface ASNetworkImageLoadInfo : NSObject <NSCopying> | ||
|
|
||
| /// The type of source from which the image was loaded. | ||
| @property (readonly) ASNetworkImageSourceType sourceType; | ||
|
|
||
| /// The image URL that was downloaded. | ||
| @property (readonly) NSURL *url; | ||
|
|
||
| /// The download identifier, if one was provided. | ||
| @property (nullable, readonly) id downloadIdentifier; | ||
|
|
||
| /// The userInfo object provided by the downloader, if one was provided. | ||
| @property (nullable, readonly) id userInfo; | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // | ||
| // ASNetworkImageLoadInfo.m | ||
| // AsyncDisplayKit | ||
| // | ||
| // Created by Adlai on 1/30/18. | ||
| // Copyright © 2018 Facebook. All rights reserved. | ||
| // | ||
|
|
||
| #import <AsyncDisplayKit/ASNetworkImageLoadInfo.h> | ||
| #import <AsyncDisplayKit/ASNetworkImageLoadInfo+Private.h> | ||
|
|
||
| @implementation ASNetworkImageLoadInfo | ||
|
|
||
| - (instancetype)initWithURL:(NSURL *)url sourceType:(ASNetworkImageSourceType)sourceType downloadIdentifier:(id)downloadIdentifier userInfo:(id)userInfo | ||
| { | ||
| if (self = [super init]) { | ||
| _url = [url copy]; | ||
| _sourceType = sourceType; | ||
| _downloadIdentifier = downloadIdentifier; | ||
| _userInfo = userInfo; | ||
| } | ||
| return self; | ||
| } | ||
|
|
||
| #pragma mark - NSCopying | ||
|
|
||
| - (id)copyWithZone:(NSZone *)zone | ||
| { | ||
| return self; | ||
| } | ||
|
|
||
| @end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| #import <AsyncDisplayKit/ASImageNode+AnimatedImagePrivate.h> | ||
| #import <AsyncDisplayKit/ASImageContainerProtocolCategories.h> | ||
| #import <AsyncDisplayKit/ASLog.h> | ||
| #import <AsyncDisplayKit/ASNetworkImageLoadInfo+Private.h> | ||
|
|
||
| #if AS_PIN_REMOTE_IMAGE | ||
| #import <AsyncDisplayKit/ASPINRemoteImageDownloader.h> | ||
|
|
@@ -334,8 +335,9 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously | |
| if (asynchronously == NO && _cacheFlags.cacheSupportsSynchronousFetch) { | ||
| ASDN::MutexLocker l(__instanceLock__); | ||
|
|
||
| NSURL *url = _URL; | ||
| if (_imageLoaded == NO && url && _downloadIdentifier == nil) { | ||
| UIImage *result = [[_cache synchronouslyFetchedCachedImageWithURL:url] asdk_image]; | ||
|
There was a problem hiding this comment. We read |
||
| if (result) { | ||
| [self _locked_setCurrentImageQuality:1.0]; | ||
| [self _locked__setImage:result]; | ||
|
|
@@ -344,8 +346,7 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously | |
| // Call out to the delegate. | ||
| if (_delegateFlags.delegateDidLoadImageWithInfo) { | ||
| ASDN::MutexUnlocker l(__instanceLock__); | ||
| auto info = [[ASNetworkImageLoadInfo alloc] initWithURL:url sourceType:ASNetworkImageSourceSynchronousCache downloadIdentifier:nil userInfo:nil]; | ||
| [_delegate imageNode:self didLoadImage:result info:info]; | ||
| } else if (_delegateFlags.delegateDidLoadImage) { | ||
| ASDN::MutexUnlocker l(__instanceLock__); | ||
|
|
@@ -553,7 +554,7 @@ - (void)_locked_cancelImageDownloadWithResumePossibility:(BOOL)storeResume | |
| _cacheUUID = nil; | ||
| } | ||
|
|
||
| - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> imageContainer, NSError*, id downloadIdentifier, id userInfo))finished | ||
| { | ||
| ASPerformBlockOnBackgroundThread(^{ | ||
| NSURL *url; | ||
|
|
@@ -572,9 +573,9 @@ - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> ima | |
| downloadIdentifier = [_downloader downloadImageWithURL:url | ||
| callbackQueue:dispatch_get_main_queue() | ||
| downloadProgress:NULL | ||
| completion:^(id <ASImageContainerProtocol> _Nullable imageContainer, NSError * _Nullable error, id _Nullable downloadIdentifier, id _Nullable userInfo) { | ||
| if (finished != NULL) { | ||
| finished(imageContainer, error, downloadIdentifier, userInfo); | ||
| } | ||
| }]; | ||
| as_log_verbose(ASImageLoadingLog(), "Downloading image for %@ url: %@", self, url); | ||
|
|
@@ -629,15 +630,15 @@ - (void)_lazilyLoadImageIfNecessary | |
| } | ||
|
|
||
| if (_shouldCacheImage) { | ||
| [self _locked__setImage:[UIImage imageNamed:URL.path.lastPathComponent]]; | ||
| } else { | ||
| // First try to load the path directly, for efficiency assuming a developer who | ||
| // doesn't want caching is trying to be as minimal as possible. | ||
| UIImage *nonAnimatedImage = [UIImage imageWithContentsOfFile:URL.path]; | ||
| if (nonAnimatedImage == nil) { | ||
| // If we couldn't find it, execute an -imageNamed:-like search so we can find resources even if the | ||
| // extension is not provided in the path. This allows the same path to work regardless of shouldCacheImage. | ||
| NSString *filename = [[NSBundle mainBundle] pathForResource:URL.path.lastPathComponent ofType:nil]; | ||
| if (filename != nil) { | ||
| nonAnimatedImage = [UIImage imageWithContentsOfFile:filename]; | ||
| } | ||
|
|
@@ -646,7 +647,7 @@ - (void)_lazilyLoadImageIfNecessary | |
| // If the file may be an animated gif and then created an animated image. | ||
| id<ASAnimatedImageProtocol> animatedImage = nil; | ||
| if (_downloaderFlags.downloaderImplementsAnimatedImage) { | ||
| NSData *data = [NSData dataWithContentsOfURL:URL]; | ||
| if (data != nil) { | ||
| animatedImage = [_downloader animatedImageWithData:data]; | ||
|
|
||
|
|
@@ -669,8 +670,7 @@ - (void)_lazilyLoadImageIfNecessary | |
|
|
||
| if (_delegateFlags.delegateDidLoadImageWithInfo) { | ||
| ASDN::MutexUnlocker u(__instanceLock__); | ||
| auto info = [[ASNetworkImageLoadInfo alloc] initWithURL:URL sourceType:ASNetworkImageSourceFileURL downloadIdentifier:nil userInfo:nil]; | ||
|
There was a problem hiding this comment. Since we already checked above that |
||
| [delegate imageNode:self didLoadImage:self.image info:info]; | ||
| } else if (_delegateFlags.delegateDidLoadImage) { | ||
| ASDN::MutexUnlocker u(__instanceLock__); | ||
|
|
@@ -679,7 +679,7 @@ - (void)_lazilyLoadImageIfNecessary | |
| }); | ||
| } else { | ||
| __weak __typeof__(self) weakSelf = self; | ||
| auto finished = ^(id <ASImageContainerProtocol>imageContainer, NSError *error, id downloadIdentifier, ASNetworkImageSourceType imageSource, id userInfo) { | ||
| ASPerformBlockOnBackgroundThread(^{ | ||
| __typeof__(self) strongSelf = weakSelf; | ||
| if (strongSelf == nil) { | ||
|
|
@@ -718,6 +718,9 @@ - (void)_lazilyLoadImageIfNecessary | |
| strongSelf->_downloadIdentifier = nil; | ||
| strongSelf->_cacheUUID = nil; | ||
|
|
||
| // TODO: Why dispatch to main here? | ||
| // The docs say the image node delegate methods | ||
| // are called from background. | ||
|
There was a problem hiding this comment. Context here. Per the API contract, I think it's ok to not dispatch. However, I'm not sure if it won't cause any client issues, since we have always called the delegate methods on main, and people may assume that (and many don't pay attention to the docs). Also, from an API design point of view, delegate methods are usually called on main. We may want to do that for consistency and ease of use (i.e people don't need to worry about thread safety)? There was a problem hiding this comment. Thanks for the context. Agree with the above, we should probably update the docs instead of updating the code. |
||
| ASPerformBlockOnMainThread(^{ | ||
| __typeof__(self) strongSelf = weakSelf; | ||
| if (strongSelf == nil) { | ||
|
|
@@ -730,8 +733,7 @@ - (void)_lazilyLoadImageIfNecessary | |
| if (imageContainer != nil) { | ||
| if (strongSelf->_delegateFlags.delegateDidLoadImageWithInfo) { | ||
| ASDN::MutexUnlocker u(strongSelf->__instanceLock__); | ||
| auto info = [[ASNetworkImageLoadInfo alloc] initWithURL:URL sourceType:imageSource downloadIdentifier:downloadIdentifier userInfo:userInfo]; | ||
| [delegate imageNode:strongSelf didLoadImage:strongSelf.image info:info]; | ||
| } else if (strongSelf->_delegateFlags.delegateDidLoadImage) { | ||
| ASDN::MutexUnlocker u(strongSelf->__instanceLock__); | ||
|
|
@@ -766,20 +768,20 @@ - (void)_lazilyLoadImageIfNecessary | |
| } | ||
|
|
||
| if ([imageContainer asdk_image] == nil && _downloader != nil) { | ||
| [self _downloadImageWithCompletion:^(id<ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) { | ||
| finished(imageContainer, error, downloadIdentifier, ASNetworkImageSourceDownload, userInfo); | ||
| }]; | ||
| } else { | ||
| as_log_verbose(ASImageLoadingLog(), "Decached image for %@ img: %@ url: %@", self, [imageContainer asdk_image], URL); | ||
| finished(imageContainer, nil, nil, ASNetworkImageSourceAsynchronousCache, nil); | ||
| } | ||
| }; | ||
| [_cache cachedImageWithURL:URL | ||
| callbackQueue:dispatch_get_main_queue() | ||
| completion:completion]; | ||
| } else { | ||
| [self _downloadImageWithCompletion:^(id<ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) { | ||
| finished(imageContainer, error, downloadIdentifier, ASNetworkImageSourceDownload, userInfo); | ||
| }]; | ||
| } | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these all atomic on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I'm trying to figure out if there's a way to document a readonly property as
atomicbut get a synthesized getter that behaves likenonatomic(since we don't need a lock/barrier.)For the time being, I figure simplicity and correct documentation are key. We won't spend much time in the spinlock/unfair-lock that Obj-C uses for these.