Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposes a new option in ASImageDownloaderProtocol to retry image downloads #1948

Merged
merged 3 commits into from
Feb 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Source/ASMultiplexImageNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,12 @@ typedef NS_ENUM(NSUInteger, ASMultiplexImageNodeErrorCode) {
*/
@property (nonatomic) BOOL shouldRenderProgressImages;

/**
* Specifies whether the underlying image downloader should attempt to retry downloading the image if the remote
* host is unreachable. It will have no effect if the downloader does not support retrying. The default is YES.
*/
@property BOOL shouldRetryImageDownload;

/**
* @abstract The image manager that this image node should use when requesting images from the Photos framework. If this is `nil` (the default), then `PHImageManager.defaultManager` is used.

Expand Down
5 changes: 4 additions & 1 deletion Source/ASMultiplexImageNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,14 @@ - (instancetype)initWithCache:(id<ASImageCacheProtocol>)cache downloader:(id<ASI

_downloaderFlags.downloaderImplementsSetProgress = [downloader respondsToSelector:@selector(setProgressImageBlock:callbackQueue:withDownloadIdentifier:)];
_downloaderFlags.downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
_downloaderFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:priority:callbackQueue:downloadProgress:completion:)];
_downloaderFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:shouldRetry:priority:callbackQueue:downloadProgress:completion:)];

_cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)];

_shouldRenderProgressImages = YES;

self.shouldBypassEnsureDisplay = YES;
self.shouldRetryImageDownload = YES;

return self;
}
Expand Down Expand Up @@ -862,6 +863,7 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
*/
ASImageDownloaderPriority priority = ASImageDownloaderPriorityWithInterfaceState(strongSelf.interfaceState);
downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
shouldRetry:[self shouldRetryImageDownload]
priority:priority
callbackQueue:callbackQueue
downloadProgress:downloadProgressBlock
Expand All @@ -877,6 +879,7 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
and their requests are put into the same pool.
*/
downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
shouldRetry:[self shouldRetryImageDownload]
callbackQueue:callbackQueue
downloadProgress:downloadProgressBlock
completion:completion];
Expand Down
6 changes: 6 additions & 0 deletions Source/ASNetworkImageNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ NS_ASSUME_NONNULL_BEGIN
*/
@property BOOL shouldRenderProgressImages;

/**
* Specifies whether the underlying image downloader should attempt to retry downloading the image if the remote
* host is unreachable. It will have no effect if the downloader does not support retrying. The default is YES.
*/
@property BOOL shouldRetryImageDownload;

/**
* The image quality of the current image.
*
Expand Down
8 changes: 5 additions & 3 deletions Source/ASNetworkImageNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,15 @@ - (instancetype)initWithCache:(id<ASImageCacheProtocol>)cache downloader:(id<ASI
_networkImageNodeFlags.downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
_networkImageNodeFlags.downloaderImplementsAnimatedImage = [downloader respondsToSelector:@selector(animatedImageWithData:)];
_networkImageNodeFlags.downloaderImplementsCancelWithResume = [downloader respondsToSelector:@selector(cancelImageDownloadWithResumePossibilityForIdentifier:)];
_networkImageNodeFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:priority:callbackQueue:downloadProgress:completion:)];
_networkImageNodeFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:shouldRetry:priority:callbackQueue:downloadProgress:completion:)];

_networkImageNodeFlags.cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)];
_networkImageNodeFlags.cacheSupportsSynchronousFetch = [cache respondsToSelector:@selector(synchronouslyFetchedCachedImageWithURL:)];

_networkImageNodeFlags.shouldCacheImage = YES;
_networkImageNodeFlags.shouldRenderProgressImages = YES;
self.shouldBypassEnsureDisplay = YES;
self.shouldRetryImageDownload = YES;

return self;
}
Expand Down Expand Up @@ -655,8 +656,8 @@ - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> ima
ASImageDownloaderPriority priority = ASImageDownloaderPriorityWithInterfaceState(interfaceState);

downloadIdentifier = [self->_downloader downloadImageWithURL:url

priority:priority
shouldRetry:[self shouldRetryImageDownload]
priority:priority
callbackQueue:callbackQueue
downloadProgress:downloadProgress
completion:completion];
Expand All @@ -671,6 +672,7 @@ - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> ima
and their requests are put into the same pool.
*/
downloadIdentifier = [self->_downloader downloadImageWithURL:url
shouldRetry:[self shouldRetryImageDownload]
callbackQueue:callbackQueue
downloadProgress:downloadProgress
completion:completion];
Expand Down
2 changes: 1 addition & 1 deletion Source/Details/ASBasicImageDownloader.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ NS_ASSUME_NONNULL_BEGIN
* A shared image downloader which can be used by @c ASNetworkImageNodes and @c ASMultiplexImageNodes.
* The userInfo provided by this downloader is `nil`.
*
* This is a very basic image downloader. It does not support caching, progressive downloading and likely
* This is a very basic image downloader. It does not support caching, retrying, progressive downloading and likely
* isn't something you should use in production. If you'd like something production ready, see @c ASPINRemoteImageDownloader
*
* @note It is strongly recommended you include PINRemoteImage and use @c ASPINRemoteImageDownloader instead.
Expand Down
3 changes: 3 additions & 0 deletions Source/Details/ASBasicImageDownloader.mm
Original file line number Diff line number Diff line change
Expand Up @@ -253,18 +253,21 @@ - (instancetype)_init
#pragma mark ASImageDownloaderProtocol.

- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress
completion:(ASImageDownloaderCompletion)completion
{
return [self downloadImageWithURL:URL
shouldRetry:shouldRetry
priority:ASImageDownloaderPriorityImminent // maps to default priority
callbackQueue:callbackQueue
downloadProgress:downloadProgress
completion:completion];
}

- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
priority:(ASImageDownloaderPriority)priority
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(ASImageDownloaderProgress)downloadProgress
Expand Down
4 changes: 4 additions & 0 deletions Source/Details/ASImageProtocols.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
/**
@abstract Downloads an image with the given URL.
@param URL The URL of the image to download.
@param shouldRetry Whether to attempt to retry downloading if the remote host is currently unreachable.
@param callbackQueue The queue to call `downloadProgressBlock` and `completion` on.
@param downloadProgress The block to be invoked when the download of `URL` progresses.
@param completion The block to be invoked when the download has completed, or has failed.
Expand All @@ -100,6 +101,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
retain the identifier if you wish to use it later.
*/
- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress
completion:(ASImageDownloaderCompletion)completion;
Expand All @@ -117,6 +119,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
/**
@abstract Downloads an image with the given URL.
@param URL The URL of the image to download.
@param shouldRetry Whether to attempt to retry downloading if the remote host is currently unreachable.
@param priority The priority at which the image should be downloaded.
@param callbackQueue The queue to call `downloadProgressBlock` and `completion` on.
@param downloadProgress The block to be invoked when the download of `URL` progresses.
Expand All @@ -127,6 +130,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) {
retain the identifier if you wish to use it later.
*/
- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
priority:(ASImageDownloaderPriority)priority
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress
Expand Down
10 changes: 9 additions & 1 deletion Source/Details/ASPINRemoteImageDownloader.mm
Original file line number Diff line number Diff line change
Expand Up @@ -255,18 +255,21 @@ - (void)clearFetchedImageFromCacheWithURL:(NSURL *)URL
}

- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(ASImageDownloaderProgress)downloadProgress
completion:(ASImageDownloaderCompletion)completion;
{
return [self downloadImageWithURL:URL
shouldRetry:shouldRetry
priority:ASImageDownloaderPriorityImminent // maps to default priority
callbackQueue:callbackQueue
downloadProgress:downloadProgress
completion:completion];
}

- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
priority:(ASImageDownloaderPriority)priority
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(ASImageDownloaderProgress)downloadProgress
Expand Down Expand Up @@ -301,8 +304,13 @@ - (nullable id)downloadImageWithURL:(NSURL *)URL
// extra downloads isn't worth the effort of rechecking caches every single time. In order to provide
// feedback to the consumer about whether images are cached, we can't simply make the cache a no-op and
// check the cache as part of this download.
PINRemoteImageManagerDownloadOptions options = PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache;
if (!shouldRetry) {
options |= PINRemoteImageManagerDownloadOptionsSkipRetry;
}

return [[self sharedPINRemoteImageManager] downloadImageWithURL:URL
options:PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache
options:options
priority:pi_priority
progressImage:nil
progressDownload:progressDownload
Expand Down
2 changes: 2 additions & 0 deletions Tests/ASBasicImageDownloaderTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ - (void)testAsynchronouslyDownloadTheSameURLTwice
subdirectory:@"TestResources"];

[downloader downloadImageWithURL:URL
shouldRetry:YES
callbackQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)
downloadProgress:nil
completion:^(id<ASImageContainerProtocol> _Nullable image, NSError * _Nullable error, id _Nullable downloadIdentifier, id _Nullable userInfo) {
[firstExpectation fulfill];
}];

[downloader downloadImageWithURL:URL
shouldRetry:YES
callbackQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)
downloadProgress:nil
completion:^(id<ASImageContainerProtocol> _Nullable image, NSError * _Nullable error, id _Nullable downloadIdentifier, id _Nullable userInfo) {
Expand Down
6 changes: 3 additions & 3 deletions Tests/ASMultiplexImageNodeTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -222,14 +222,14 @@ - (void)testUncachedDownload

// Mock a 50%-progress URL download.
const CGFloat mockedProgress = 0.5;
OCMExpect([mockDownloader downloadImageWithURL:[self _testImageURL] priority:ASImageDownloaderPriorityPreload callbackQueue:OCMOCK_ANY downloadProgress:[OCMArg isNotNil] completion:[OCMArg isNotNil]])
OCMExpect([mockDownloader downloadImageWithURL:[self _testImageURL] shouldRetry:YES priority:ASImageDownloaderPriorityPreload callbackQueue:OCMOCK_ANY downloadProgress:[OCMArg isNotNil] completion:[OCMArg isNotNil]])
.andDo(^(NSInvocation *inv){
// Simulate progress.
ASImageDownloaderProgress progressBlock = [inv as_argumentAtIndexAsObject:5];
ASImageDownloaderProgress progressBlock = [inv as_argumentAtIndexAsObject:6];
progressBlock(mockedProgress);

// Simulate completion.
ASImageDownloaderCompletion completionBlock = [inv as_argumentAtIndexAsObject:6];
ASImageDownloaderCompletion completionBlock = [inv as_argumentAtIndexAsObject:7];
completionBlock([self _testImage], nil, nil, nil);
});

Expand Down
4 changes: 2 additions & 2 deletions Tests/ASNetworkImageNodeTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ - (void)DISABLED_testThatProgressBlockIsSetAndClearedCorrectlyOnVisibility
node.URL = [NSURL URLWithString:@"http://imageA"];

// Enter preload range, wait for download start.
[[[downloader expect] andForwardToRealObject] downloadImageWithURL:[OCMArg isNotNil] callbackQueue:OCMOCK_ANY downloadProgress:OCMOCK_ANY completion:OCMOCK_ANY];
[[[downloader expect] andForwardToRealObject] downloadImageWithURL:[OCMArg isNotNil] shouldRetry:OCMOCK_ANY callbackQueue:OCMOCK_ANY downloadProgress:OCMOCK_ANY completion:OCMOCK_ANY];
[node enterInterfaceState:ASInterfaceStatePreload];
[downloader verifyWithDelay:5];

Expand Down Expand Up @@ -123,7 +123,7 @@ - (void)cancelImageDownloadForIdentifier:(id)downloadIdentifier
// nop
}

- (id)downloadImageWithURL:(NSURL *)URL callbackQueue:(dispatch_queue_t)callbackQueue downloadProgress:(ASImageDownloaderProgress)downloadProgress completion:(ASImageDownloaderCompletion)completion
- (id)downloadImageWithURL:(NSURL *)URL shouldRetry:(BOOL)shouldRetry callbackQueue:(dispatch_queue_t)callbackQueue downloadProgress:(ASImageDownloaderProgress)downloadProgress completion:(ASImageDownloaderCompletion)completion
{
return @(_currentDownloadID++);
}
Expand Down
1 change: 1 addition & 0 deletions examples_extra/Multiplex/Sample/ScreenNode.m
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ - (void)multiplexImageNode:(ASMultiplexImageNode *)imageNode didFinishDownloadin
#pragma mark ASImageDownloaderProtocol.

- (nullable id)downloadImageWithURL:(NSURL *)URL
shouldRetry:(BOOL)shouldRetry
callbackQueue:(dispatch_queue_t)callbackQueue
downloadProgress:(nullable ASImageDownloaderProgress)downloadProgressBlock
completion:(ASImageDownloaderCompletion)completion
Expand Down