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

[feat] Added download progress for NetworkImageNode #1489

Merged
Show file tree
Hide file tree
Changes from 3 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
7 changes: 7 additions & 0 deletions Source/ASNetworkImageNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,13 @@ NS_ASSUME_NONNULL_BEGIN
*/
@property (readonly) CGFloat renderedImageQuality;

/**
* Download progress of the current image.
* When downloading a network image, this value would be updated to track download progress (value between 0 and 1)
* If the URL is unset, this is 1 if defaultImage or image is set to non-nil.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the URL is unset, this is 1 if defaultImage or image is set to non-nil.

This line is not true for download progress because the progress should be 0 (set here).

*/
@property (readonly) CGFloat downloadProgress;

@end


Expand Down
54 changes: 52 additions & 2 deletions Source/ASNetworkImageNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ @interface ASNetworkImageNode ()

CGFloat _currentImageQuality;
CGFloat _renderedImageQuality;
CGFloat _downloadProgress;

// Immutable and set on init only. We don't need to lock in this case.
__weak id<ASImageDownloaderProtocol> _downloader;
Expand Down Expand Up @@ -155,6 +156,7 @@ - (void)_locked_setImage:(UIImage *)image
// If our image is being set externally, the image quality is 100%
if (imageWasSetExternally) {
[self _setCurrentImageQuality:1.0];
[self _setDownloadProgress:1.0];
}

[self _locked__setImage:image];
Expand Down Expand Up @@ -206,7 +208,9 @@ - (void)setURL:(NSURL *)URL resetToDefault:(BOOL)reset
_networkImageNodeFlags.imageWasSetExternally = NO;

[self _locked_cancelImageDownloadWithResumePossibility:NO];


[self _setDownloadProgress:0.0];

_networkImageNodeFlags.imageLoaded = NO;

_URL = URL;
Expand Down Expand Up @@ -277,6 +281,30 @@ - (void)_setCurrentImageQuality:(CGFloat)imageQuality
});
}

- (void)setDownloadProgress:(CGFloat)downloadProgress
{
ASLockScopeSelf();
_downloadProgress = downloadProgress;
}

- (CGFloat)downloadProgress
{
return ASLockedSelf(_downloadProgress);
}

/**
* Always use these methods internally to update the current image quality
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Always use these methods internally to update the current image quality
* Always use these methods internally to update the current download progress

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Thanks! 👍

* We want to maintain the order that downloadProgress is set regardless of the calling thread,
* so we always have to dispatch to the main thread to ensure that we queue the operations in the correct order.
* (see comment in displayDidFinish)
*/
- (void)_setDownloadProgress:(CGFloat)downloadProgress
{
dispatch_async(dispatch_get_main_queue(), ^{
self.downloadProgress = downloadProgress;
});
}

- (void)setRenderedImageQuality:(CGFloat)renderedImageQuality
{
ASLockScopeSelf();
Expand Down Expand Up @@ -366,6 +394,7 @@ - (void)displayWillStartAsynchronously:(BOOL)asynchronously
UIImage *result = [[_cache synchronouslyFetchedCachedImageWithURL:url] asdk_image];
if (result) {
[self _setCurrentImageQuality:1.0];
[self _setDownloadProgress:1.0];
[self _locked__setImage:result];
_networkImageNodeFlags.imageLoaded = YES;

Expand Down Expand Up @@ -441,6 +470,17 @@ + (BOOL)useMainThreadDelegateCallbacks

#pragma mark - Progress

- (void)_updateDownloadedProgress:(CGFloat)progress
downloadIdentifier:(nullable id)downloadIdentifier
{
ASLockScopeSelf();
// Getting a result back for a different download identifier, download must not have been successfully canceled
if (ASObjectIsEqual(_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
return;
}
[self _setDownloadProgress:progress];
}

- (void)handleProgressImage:(UIImage *)progressImage progress:(CGFloat)progress downloadIdentifier:(nullable id)downloadIdentifier
{
ASLockScopeSelf();
Expand Down Expand Up @@ -539,6 +579,7 @@ - (void)_locked_cancelDownloadAndClearImageWithResumePossibility:(BOOL)storeResu

[self _locked_setAnimatedImage:nil];
[self _setCurrentImageQuality:0.0];
[self _setDownloadProgress:0.0];
[self _locked__setImage:_defaultImage];

_networkImageNodeFlags.imageLoaded = NO;
Expand Down Expand Up @@ -596,7 +637,14 @@ - (void)_downloadImageWithCompletion:(void (^)(id <ASImageContainerProtocol> ima
}

dispatch_queue_t callbackQueue = [self callbackQueue];
ASImageDownloaderProgress downloadProgress = NULL;
__weak __typeof__(self) weakSelf = self;
ASImageDownloaderProgress downloadProgress = ^(CGFloat progress){
__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}
[strongSelf _updateDownloadedProgress:progress downloadIdentifier:downloadIdentifier];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit more concise.

if (strongSelf != nil) {
  [strongSelf _updateDownloadedProgress:progress downloadIdentifier:downloadIdentifier];
}

};
ASImageDownloaderCompletion completion = ^(id <ASImageContainerProtocol> _Nullable imageContainer, NSError * _Nullable error, id _Nullable downloadIdentifier, id _Nullable userInfo) {
if (finished != NULL) {
finished(imageContainer, error, downloadIdentifier, userInfo);
Expand Down Expand Up @@ -729,6 +777,7 @@ - (void)_lazilyLoadImageIfNecessary
self->_networkImageNodeFlags.imageLoaded = YES;

[self _setCurrentImageQuality:1.0];
[self _setDownloadProgress:1.0];

if (self->_networkImageNodeFlags.delegateDidLoadImageWithInfo) {
ASUnlockScope(self);
Expand Down Expand Up @@ -768,6 +817,7 @@ - (void)_lazilyLoadImageIfNecessary
UIImage *newImage;
if (imageContainer != nil) {
[strongSelf _setCurrentImageQuality:1.0];
[self _setDownloadProgress:1.0];
Copy link
Member

@nguyenhuy nguyenhuy Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only remaining blocker for this PR!

Suggested change
[self _setDownloadProgress:1.0];
[strongSelf _setDownloadProgress:1.0];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 Seems I just do the copied. Thanks! 👍

NSData *animatedImageData = [imageContainer asdk_animatedImageData];
if (animatedImageData && strongSelf->_networkImageNodeFlags.downloaderImplementsAnimatedImage) {
id animatedImage = [strongSelf->_downloader animatedImageWithData:animatedImageData];
Expand Down