add more delegate methods for monitoring network image node progress #1247
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,10 +49,14 @@ @interface ASNetworkImageNode () | |
| BOOL _shouldRenderProgressImages; | ||
|
|
||
| struct { | ||
| unsigned int delegateWillStartDisplayAsynchronously:1; | ||
| unsigned int delegateWillLoadImageFromCache:1; | ||
| unsigned int delegateWillLoadImageFromNetwork:1; | ||
| unsigned int delegateDidStartFetchingData:1; | ||
| unsigned int delegateDidFailWithError:1; | ||
| unsigned int delegateDidFinishDecoding:1; | ||
| unsigned int delegateDidLoadImage:1; | ||
| unsigned int delegateDidLoadImageFromCache:1; | ||
| unsigned int delegateDidLoadImageWithInfo:1; | ||
| } _delegateFlags; | ||
|
|
||
|
|
@@ -297,10 +301,14 @@ - (void)setDelegate:(id<ASNetworkImageNodeDelegate>)delegate | |
| ASLockScopeSelf(); | ||
| _delegate = delegate; | ||
|
|
||
| _delegateFlags.delegateWillStartDisplayAsynchronously = [delegate respondsToSelector:@selector(imageNodeWillStartDisplayAsynchronously:)]; | ||
| _delegateFlags.delegateWillLoadImageFromCache = [delegate respondsToSelector:@selector(imageNodeWillLoadImageFromCache:)]; | ||
| _delegateFlags.delegateWillLoadImageFromNetwork = [delegate respondsToSelector:@selector(imageNodeWillLoadImageFromNetwork:)]; | ||
| _delegateFlags.delegateDidStartFetchingData = [delegate respondsToSelector:@selector(imageNodeDidStartFetchingData:)]; | ||
| _delegateFlags.delegateDidFailWithError = [delegate respondsToSelector:@selector(imageNode:didFailWithError:)]; | ||
| _delegateFlags.delegateDidFinishDecoding = [delegate respondsToSelector:@selector(imageNodeDidFinishDecoding:)]; | ||
| _delegateFlags.delegateDidLoadImage = [delegate respondsToSelector:@selector(imageNode:didLoadImage:)]; | ||
| _delegateFlags.delegateDidLoadImageFromCache = [delegate respondsToSelector:@selector(imageNodeDidLoadImageFromCache:)]; | ||
| _delegateFlags.delegateDidLoadImageWithInfo = [delegate respondsToSelector:@selector(imageNode:didLoadImage:info:)]; | ||
| } | ||
|
|
||
|
|
@@ -335,6 +343,17 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously | |
| { | ||
| [super displayWillStartAsynchronously:asynchronously]; | ||
|
|
||
| id<ASNetworkImageNodeDelegate> delegate; | ||
| BOOL notifyDelegate; | ||
| { | ||
| ASLockScopeSelf(); | ||
| notifyDelegate = _delegateFlags.delegateWillStartDisplayAsynchronously; | ||
| delegate = _delegate; | ||
| } | ||
| if (notifyDelegate) { | ||
| [delegate imageNodeWillStartDisplayAsynchronously:self]; | ||
| } | ||
|
|
||
| if (asynchronously == NO && _cacheFlags.cacheSupportsSynchronousFetch) { | ||
| ASLockScopeSelf(); | ||
|
|
||
|
|
@@ -350,10 +369,10 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously | |
| if (_delegateFlags.delegateDidLoadImageWithInfo) { | ||
| ASUnlockScope(self); | ||
| let info = [[ASNetworkImageLoadInfo alloc] initWithURL:url sourceType:ASNetworkImageSourceSynchronousCache downloadIdentifier:nil userInfo:nil]; | ||
| [delegate imageNode:self didLoadImage:result info:info]; | ||
| } else if (_delegateFlags.delegateDidLoadImage) { | ||
| ASUnlockScope(self); | ||
| [delegate imageNode:self didLoadImage:result]; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -616,6 +635,9 @@ - (void)_lazilyLoadImageIfNecessary | |
| [self lock]; | ||
| __weak id<ASNetworkImageNodeDelegate> delegate = _delegate; | ||
| BOOL delegateDidStartFetchingData = _delegateFlags.delegateDidStartFetchingData; | ||
| BOOL delegateWillLoadImageFromCache = _delegateFlags.delegateWillLoadImageFromCache; | ||
| BOOL delegateWillLoadImageFromNetwork = _delegateFlags.delegateWillLoadImageFromNetwork; | ||
| BOOL delegateDidLoadImageFromCache = _delegateFlags.delegateDidLoadImageFromCache; | ||
| BOOL isImageLoaded = _imageLoaded; | ||
| NSURL *URL = _URL; | ||
| id currentDownloadIdentifier = _downloadIdentifier; | ||
|
|
@@ -773,18 +795,31 @@ - (void)_lazilyLoadImageIfNecessary | |
| } | ||
|
|
||
| if ([imageContainer asdk_image] == nil && self->_downloader != nil) { | ||
| if (delegateWillLoadImageFromNetwork) { | ||
| [delegate imageNodeWillLoadImageFromNetwork:self]; | ||
|
There was a problem hiding this comment. All of this methods are called with the lock held. This can cause deadlocks. We should at least document this behavior if we cannot get around this. There was a problem hiding this comment. I don't think these are locked. I tried putting a ASUnlockScope which result in an assertion failure. @nguyenhuy There was a problem hiding this comment. This is inside a completion block and the lock is no longer held by the time it's executed. That's why we grabbed the lock before checking the cache sentinel a few line above. So we don't need to unlock here, but we need to grab the lock before accessing |
||
| } | ||
| [self _downloadImageWithCompletion:^(id<ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) { | ||
| finished(imageContainer, error, downloadIdentifier, ASNetworkImageSourceDownload, userInfo); | ||
| }]; | ||
| } else { | ||
| if (delegateDidLoadImageFromCache) { | ||
| [delegate imageNodeDidLoadImageFromCache:self]; | ||
| } | ||
| as_log_verbose(ASImageLoadingLog(), "Decached image for %@ img: %@ url: %@", self, [imageContainer asdk_image], URL); | ||
| finished(imageContainer, nil, nil, ASNetworkImageSourceAsynchronousCache, nil); | ||
| } | ||
| }; | ||
|
|
||
| if (delegateWillLoadImageFromCache) { | ||
| [delegate imageNodeWillLoadImageFromCache:self]; | ||
| } | ||
| [_cache cachedImageWithURL:URL | ||
| callbackQueue:[self callbackQueue] | ||
| completion:completion]; | ||
| } else { | ||
| if (delegateWillLoadImageFromNetwork) { | ||
| [delegate imageNodeWillLoadImageFromNetwork:self]; | ||
| } | ||
| [self _downloadImageWithCompletion:^(id<ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) { | ||
| finished(imageContainer, error, downloadIdentifier, ASNetworkImageSourceDownload, userInfo); | ||
| }]; | ||
|
|
||
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.
It's fine to grab the
delegatein here with the lock held. One problem that will occur, what is not new and in fact we have it in almost all of our unlocked delegate calls, if the delegate needs more information from the node the information could already be out of date. Especially if you use this methods for logging this could introduce inconsistencies.This is not a new problem though and we think about different solutions to find some general solution for this though.
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.
Yeah, it's not a new problem, and I'd say delegates should rely on the data being passed via the delegate method. Not an ideal solution, but it's a fine trade-off.
I'm gonna land this PR now. Thanks for reviewing.