-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Have ASNetworkImageNode report whether images were cached or not #639
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,15 +202,11 @@ - (void)cachedImageWithURL:(NSURL *)URL | |
callbackQueue:(dispatch_queue_t)callbackQueue | ||
completion:(ASImageCacherCompletion)completion | ||
{ | ||
// We do not check the cache here and instead check it in downloadImageWithURL to avoid checking the cache twice. | ||
// If we're targeting the main queue and we're on the main thread, complete immediately. | ||
if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { | ||
completion(nil); | ||
} else { | ||
dispatch_async(callbackQueue, ^{ | ||
completion(nil); | ||
}); | ||
} | ||
[[self sharedPINRemoteImageManager] imageFromCacheWithURL:URL processorKey:nil options:PINRemoteImageManagerDownloadOptionsSkipDecode completion:^(PINRemoteImageManagerResult * _Nonnull result) { | ||
[ASPINRemoteImageDownloader _performWithCallbackQueue:callbackQueue work:^{ | ||
completion(result.image); | ||
}]; | ||
}]; | ||
} | ||
|
||
- (void)cachedImageWithURLs:(NSArray <NSURL *> *)URLs | ||
|
@@ -256,51 +252,38 @@ - (nullable id)downloadImageWithURLs:(NSArray <NSURL *> *)URLs | |
downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress | ||
completion:(ASImageDownloaderCompletion)completion | ||
{ | ||
PINRemoteImageManagerProgressDownload progressDownload = ^(int64_t completedBytes, int64_t totalBytes) { | ||
if (downloadProgress == nil) { return; } | ||
|
||
/// If we're targeting the main queue and we're on the main thread, call immediately. | ||
if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { | ||
downloadProgress(completedBytes / (CGFloat)totalBytes); | ||
} else { | ||
dispatch_async(callbackQueue, ^{ | ||
downloadProgress(completedBytes / (CGFloat)totalBytes); | ||
}); | ||
} | ||
}; | ||
|
||
PINRemoteImageManagerImageCompletion imageCompletion = ^(PINRemoteImageManagerResult * _Nonnull result) { | ||
/// If we're targeting the main queue and we're on the main thread, complete immediately. | ||
if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { | ||
#if PIN_ANIMATED_AVAILABLE | ||
if (result.alternativeRepresentation) { | ||
completion(result.alternativeRepresentation, result.error, result.UUID); | ||
} else { | ||
completion(result.image, result.error, result.UUID); | ||
} | ||
#else | ||
completion(result.image, result.error, result.UUID); | ||
#endif | ||
} else { | ||
dispatch_async(callbackQueue, ^{ | ||
PINRemoteImageManagerProgressDownload progressDownload = ^(int64_t completedBytes, int64_t totalBytes) { | ||
if (downloadProgress == nil) { return; } | ||
|
||
[ASPINRemoteImageDownloader _performWithCallbackQueue:callbackQueue work:^{ | ||
downloadProgress(completedBytes / (CGFloat)totalBytes); | ||
}]; | ||
}; | ||
|
||
PINRemoteImageManagerImageCompletion imageCompletion = ^(PINRemoteImageManagerResult * _Nonnull result) { | ||
[ASPINRemoteImageDownloader _performWithCallbackQueue:callbackQueue work:^{ | ||
#if PIN_ANIMATED_AVAILABLE | ||
if (result.alternativeRepresentation) { | ||
completion(result.alternativeRepresentation, result.error, result.UUID); | ||
} else { | ||
completion(result.image, result.error, result.UUID); | ||
} | ||
if (result.alternativeRepresentation) { | ||
completion(result.alternativeRepresentation, result.error, result.UUID); | ||
} else { | ||
completion(result.image, result.error, result.UUID); | ||
} | ||
#else | ||
completion(result.image, result.error, result.UUID); | ||
completion(result.image, result.error, result.UUID); | ||
#endif | ||
}); | ||
} | ||
}; | ||
|
||
return [[self sharedPINRemoteImageManager] downloadImageWithURLs:URLs | ||
options:PINRemoteImageManagerDownloadOptionsSkipDecode | ||
progressImage:nil | ||
progressDownload:progressDownload | ||
completion:imageCompletion]; | ||
}]; | ||
}; | ||
|
||
// add "IgnoreCache" option since we have a caching API so we already checked it, not worth checking again. | ||
// PINRemoteImage is responsible for coalescing downloads, and even if it wasn't, the tiny probability of | ||
// 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. | ||
return [[self sharedPINRemoteImageManager] downloadImageWithURLs:URLs | ||
options:PINRemoteImageManagerDownloadOptionsSkipDecode | PINRemoteImageManagerDownloadOptionsIgnoreCache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly I don't think we should skip the cache because the synchronous cache check only checks the memory cache not the disk cache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we'll check the disk cache in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I take this back, didn't see that the cache check above was implemented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For history, @Adlai-Holler and I discussed and we think this is safe. He pointed out the only case where this could cause an extra download is:
This seems very unlikely in practice. |
||
progressImage:nil | ||
progressDownload:progressDownload | ||
completion:imageCompletion]; | ||
} | ||
|
||
- (void)cancelImageDownloadForIdentifier:(id)downloadIdentifier | ||
|
@@ -369,5 +352,29 @@ - (id)alternateRepresentationWithData:(NSData *)data options:(PINRemoteImageMana | |
return nil; | ||
} | ||
|
||
#pragma mark - Private | ||
|
||
/** | ||
* If on main thread and queue is main, perform now. | ||
* If queue is nil, assert and perform now. | ||
* Otherwise, dispatch async to queue. | ||
*/ | ||
+ (void)_performWithCallbackQueue:(dispatch_queue_t)queue work:(void (^)())work | ||
{ | ||
if (work == nil) { | ||
// No need to assert here, really. We aren't expecting any feedback from this method. | ||
return; | ||
} | ||
|
||
if (ASDisplayNodeThreadIsMain() && queue == dispatch_get_main_queue()) { | ||
work(); | ||
} else if (queue == nil) { | ||
ASDisplayNodeFailAssert(@"Callback queue should not be nil."); | ||
work(); | ||
} else { | ||
dispatch_async(queue, work); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this method because I added another callback path and it felt silly to keep duplicating code. Sorry, dear reviewer! |
||
|
||
@end | ||
#endif |
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.
We previously weren't calling out to the delegate at all in this synchronous fetch scenario. I think we should.
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.
Good catch!