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

Config AFImageDownloader NSURLCache and ask AFImageRequestCache implementer if an image should be cached #4010

Merged
merged 9 commits into from Dec 15, 2017

Conversation

Projects
None yet
4 participants
@wjehenddher
Contributor

wjehenddher commented Jul 28, 2017

The new feature consists of 2 parts:

  • Allow user to config AFImageDownloader +defaultURLCache. Currently, the values are hardcoded 20Mb mem, 150Mb disk.

  • Provide a hook in AFAutoPurgingImageCache so UIImages can be filtered before cached. Ideally, one could filter out UIImage that exceeds certain raster size.

Use case for this new two features:

A UICollectionView shows thumbnails of JPGs. Tapping any UICollectionViewCell opens a full size UIImageView that displays the full size JPG.

Under thew current implementation, large JPGs boot out thumbnails.
Hence JPGs and thumbnails are downloaded almost always as both caches keep getting recycled.

References

SO: https://stackoverflow.com/questions/41939774/how-to-limit-size-of-image-disk-cache-using-afnetworking/45329948#45329948

See /issues/4009

@wjehenddher

This comment has been minimized.

Show comment
Hide comment
@wjehenddher

wjehenddher Jul 28, 2017

Contributor

Hoping this will fly with you guys ;)

With these new features, we can do this:

AFImageDownloader with only disk NSURLCache

    // Force AFNetworking to NOT use any RAM whatsoever for image downloading
    // (neither backed by NSURLCache nor imageCache). We will solely rely on
    // NSURLCache backed by disk
    // (i.e. won't use AFAutoPurgingImageCache and NSURLCache.memoryCapacity=0 disk=300)
    NSURLCache* imageDownloaderCache = [[NSURLCache alloc] initWithMemoryCapacity:0
                                                                     diskCapacity:300 * 1024 * 1024
                                                                         diskPath:@"com.alamofire.imagedownloader"];
    
    NSURLSessionConfiguration* imageDownloaderSessionConfiguration = [AFImageDownloader defaultURLSessionConfiguration];
    imageDownloaderSessionConfiguration.URLCache = imageDownloaderCache;
    AFImageDownloader* imageDownloader = [[AFImageDownloader alloc] initWithSessionConfiguration:imageDownloaderSessionConfiguration];
    imageDownloader.imageCache = nil;
    // If in the future, we wanted to cache thumbnails in RAM, our own P2AFAutoPurgingImageCache shall do the trick
//    imageDownloader.imageCache = [[P2AFAutoPurgingImageCache alloc] initWithMemoryCapacity:5 * 1024 * 1024
//                                                                   preferredMemoryCapacity:4 * 1024 * 1024];
    [UIImageView setSharedImageDownloader:imageDownloader];

Optional P2AFAutoPurgingImageCache that only caches thumbnails

@interface P2AFAutoPurgingImageCache : AFAutoPurgingImageCache
@end

@implementation P2AFAutoPurgingImageCache

-(BOOL)shouldCacheImage:(UIImage *)image forRequest:(NSURLRequest *)request withAdditionalIdentifier:(NSString *)identifier
{
    // Only cache thumbnails
    return image.size.width <= 200 && image.size.height <= 200;
}

@end
Contributor

wjehenddher commented Jul 28, 2017

Hoping this will fly with you guys ;)

With these new features, we can do this:

AFImageDownloader with only disk NSURLCache

    // Force AFNetworking to NOT use any RAM whatsoever for image downloading
    // (neither backed by NSURLCache nor imageCache). We will solely rely on
    // NSURLCache backed by disk
    // (i.e. won't use AFAutoPurgingImageCache and NSURLCache.memoryCapacity=0 disk=300)
    NSURLCache* imageDownloaderCache = [[NSURLCache alloc] initWithMemoryCapacity:0
                                                                     diskCapacity:300 * 1024 * 1024
                                                                         diskPath:@"com.alamofire.imagedownloader"];
    
    NSURLSessionConfiguration* imageDownloaderSessionConfiguration = [AFImageDownloader defaultURLSessionConfiguration];
    imageDownloaderSessionConfiguration.URLCache = imageDownloaderCache;
    AFImageDownloader* imageDownloader = [[AFImageDownloader alloc] initWithSessionConfiguration:imageDownloaderSessionConfiguration];
    imageDownloader.imageCache = nil;
    // If in the future, we wanted to cache thumbnails in RAM, our own P2AFAutoPurgingImageCache shall do the trick
//    imageDownloader.imageCache = [[P2AFAutoPurgingImageCache alloc] initWithMemoryCapacity:5 * 1024 * 1024
//                                                                   preferredMemoryCapacity:4 * 1024 * 1024];
    [UIImageView setSharedImageDownloader:imageDownloader];

Optional P2AFAutoPurgingImageCache that only caches thumbnails

@interface P2AFAutoPurgingImageCache : AFAutoPurgingImageCache
@end

@implementation P2AFAutoPurgingImageCache

-(BOOL)shouldCacheImage:(UIImage *)image forRequest:(NSURLRequest *)request withAdditionalIdentifier:(NSString *)identifier
{
    // Only cache thumbnails
    return image.size.width <= 200 && image.size.height <= 200;
}

@end
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 28, 2017

Codecov Report

Merging #4010 into master will increase coverage by 0.25%.
The diff coverage is 97.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4010      +/-   ##
==========================================
+ Coverage   82.52%   82.77%   +0.25%     
==========================================
  Files          45       45              
  Lines        5270     5371     +101     
  Branches      439      442       +3     
==========================================
+ Hits         4349     4446      +97     
- Misses        682      685       +3     
- Partials      239      240       +1
Impacted Files Coverage Δ
UIKit+AFNetworking/AFImageDownloader.h 100% <ø> (ø) ⬆️
UIKit+AFNetworking/AFAutoPurgingImageCache.h 100% <ø> (ø) ⬆️
UIKit+AFNetworking/AFAutoPurgingImageCache.m 93.45% <100%> (+0.06%) ⬆️
UIKit+AFNetworking/AFImageDownloader.m 91.54% <100%> (+0.12%) ⬆️
Tests/Tests/AFAutoPurgingImageCacheTests.m 100% <100%> (ø) ⬆️
Tests/Tests/AFImageDownloaderTests.m 98.75% <96.7%> (-0.61%) ⬇️
AFNetworking/AFURLSessionManager.m 59.89% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd59ffa...9b33c16. Read the comment docs.

codecov-io commented Jul 28, 2017

Codecov Report

Merging #4010 into master will increase coverage by 0.25%.
The diff coverage is 97.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4010      +/-   ##
==========================================
+ Coverage   82.52%   82.77%   +0.25%     
==========================================
  Files          45       45              
  Lines        5270     5371     +101     
  Branches      439      442       +3     
==========================================
+ Hits         4349     4446      +97     
- Misses        682      685       +3     
- Partials      239      240       +1
Impacted Files Coverage Δ
UIKit+AFNetworking/AFImageDownloader.h 100% <ø> (ø) ⬆️
UIKit+AFNetworking/AFAutoPurgingImageCache.h 100% <ø> (ø) ⬆️
UIKit+AFNetworking/AFAutoPurgingImageCache.m 93.45% <100%> (+0.06%) ⬆️
UIKit+AFNetworking/AFImageDownloader.m 91.54% <100%> (+0.12%) ⬆️
Tests/Tests/AFAutoPurgingImageCacheTests.m 100% <100%> (ø) ⬆️
Tests/Tests/AFImageDownloaderTests.m 98.75% <96.7%> (-0.61%) ⬇️
AFNetworking/AFURLSessionManager.m 59.89% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd59ffa...9b33c16. Read the comment docs.

@wjehenddher

This comment has been minimized.

Show comment
Hide comment
@wjehenddher

wjehenddher Jul 28, 2017

Contributor

Candidate resolution for /issues/4008 and See /issues/4009

Contributor

wjehenddher commented Jul 28, 2017

Candidate resolution for /issues/4008 and See /issues/4009

@wjehenddher

This comment has been minimized.

Show comment
Hide comment
@wjehenddher

wjehenddher Aug 11, 2017

Contributor

Hello @SlaunchaMan, @TheDom, it seems you were the last people to PR.

Could you help getting this PR reviewed (and hopefully accepted)?

Also, there hasn't been any release since March last year. Are there any plans for a new 3.1.x release?

Contributor

wjehenddher commented Aug 11, 2017

Hello @SlaunchaMan, @TheDom, it seems you were the last people to PR.

Could you help getting this PR reviewed (and hopefully accepted)?

Also, there hasn't been any release since March last year. Are there any plans for a new 3.1.x release?

@wjehenddher

This comment has been minimized.

Show comment
Hide comment
@wjehenddher

wjehenddher Oct 27, 2017

Contributor

🎉🎉🎉🎉🎉

I suppose you'll do the merge, @hemangshah?

Contributor

wjehenddher commented Oct 27, 2017

🎉🎉🎉🎉🎉

I suppose you'll do the merge, @hemangshah?

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan
Member

SlaunchaMan commented Nov 22, 2017

Fixes #4035

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Nov 22, 2017

Member

@wjehenddher I don’t see the withAdditionalIdentifier: parameter being used anywhere in here. Where is this designed to be used? Can you add some tests around its expected behavior?

Member

SlaunchaMan commented Nov 22, 2017

@wjehenddher I don’t see the withAdditionalIdentifier: parameter being used anywhere in here. Where is this designed to be used? Can you add some tests around its expected behavior?

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan
Member

SlaunchaMan commented Nov 22, 2017

Fixes #4008

@wjehenddher

This comment has been minimized.

Show comment
Hide comment
@wjehenddher

wjehenddher Nov 28, 2017

Contributor

HI @SlaunchaMan

About #4010 (comment)

I'm being consistent with the other methods: the param identifier is declared in

- (BOOL)shouldCacheImage:(UIImage *)image forRequest:(NSURLRequest *)request withAdditionalIdentifier:(nullable NSString *)identifier;

within AFAutoPurgingImageCache.h

https://github.com/WJE/AFNetworking/blob/9b33c1699f3f94135fbc35cb06080cbfe9cf11d1/UIKit%2BAFNetworking/AFAutoPurgingImageCache.h#L84

The test case is here: It's a mock that returns YES or NO based on an initial configuration.

https://github.com/WJE/AFNetworking/blob/9b33c1699f3f94135fbc35cb06080cbfe9cf11d1/Tests/Tests/AFImageDownloaderTests.m#L562

https://github.com/WJE/AFNetworking/blob/9b33c1699f3f94135fbc35cb06080cbfe9cf11d1/Tests/Tests/AFImageDownloaderTests.m#L246

Makes sense?

Contributor

wjehenddher commented Nov 28, 2017

HI @SlaunchaMan

About #4010 (comment)

I'm being consistent with the other methods: the param identifier is declared in

- (BOOL)shouldCacheImage:(UIImage *)image forRequest:(NSURLRequest *)request withAdditionalIdentifier:(nullable NSString *)identifier;

within AFAutoPurgingImageCache.h

https://github.com/WJE/AFNetworking/blob/9b33c1699f3f94135fbc35cb06080cbfe9cf11d1/UIKit%2BAFNetworking/AFAutoPurgingImageCache.h#L84

The test case is here: It's a mock that returns YES or NO based on an initial configuration.

https://github.com/WJE/AFNetworking/blob/9b33c1699f3f94135fbc35cb06080cbfe9cf11d1/Tests/Tests/AFImageDownloaderTests.m#L562

https://github.com/WJE/AFNetworking/blob/9b33c1699f3f94135fbc35cb06080cbfe9cf11d1/Tests/Tests/AFImageDownloaderTests.m#L246

Makes sense?

@SlaunchaMan SlaunchaMan merged commit b0d24a0 into AFNetworking:master Dec 15, 2017

3 checks passed

codecov/patch 97.11% of diff hit (target 82.52%)
Details
codecov/project 82.77% (+0.25%) compared to dd59ffa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wjehenddher

This comment has been minimized.

Show comment
Hide comment
@wjehenddher

wjehenddher Dec 15, 2017

Contributor

🎉🎉🎉🎉🎉

Thank you @SlaunchaMan

Do you folks have ETA for new AFN 3.x release?

Contributor

wjehenddher commented Dec 15, 2017

🎉🎉🎉🎉🎉

Thank you @SlaunchaMan

Do you folks have ETA for new AFN 3.x release?

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Dec 15, 2017

Member

Hopefully very soon!

Member

SlaunchaMan commented Dec 15, 2017

Hopefully very soon!

@SlaunchaMan SlaunchaMan added this to the 3.2.0 milestone Dec 15, 2017

@SlaunchaMan SlaunchaMan added the added label Dec 15, 2017

@SlaunchaMan SlaunchaMan added enhancement and removed enhancement labels Dec 15, 2017

@wjehenddher wjehenddher deleted the WJE:config_afimagedownloader_nsurlcache branch Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment