Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,7 @@ @interface ASPINRemoteImageManager : PINRemoteImageManager | |
| @implementation ASPINRemoteImageManager | ||
|
|
||
| //Share image cache with sharedImageManager image cache. | ||
| + (id <PINRemoteImageCaching>)defaultImageCache | ||
| { | ||
| static dispatch_once_t onceToken; | ||
| static id <PINRemoteImageCaching> cache = nil; | ||
|
|
@@ -106,7 +106,6 @@ @implementation ASPINRemoteImageDownloader | |
|
|
||
| + (ASPINRemoteImageDownloader *)sharedDownloader NS_RETURNS_RETAINED | ||
| { | ||
| static dispatch_once_t onceToken = 0; | ||
| dispatch_once(&onceToken, ^{ | ||
| sharedDownloader = [[ASPINRemoteImageDownloader alloc] init]; | ||
|
|
@@ -116,56 +115,67 @@ + (ASPINRemoteImageDownloader *)sharedDownloader NS_RETURNS_RETAINED | |
|
|
||
| + (void)setSharedImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration | ||
| { | ||
| PINRemoteImageManager *sharedManager = [self PINRemoteImageManagerWithConfiguration:configuration imageCache:nil]; | ||
| [self setSharedPreconfiguredRemoteImageManager:sharedManager]; | ||
| } | ||
|
|
||
| + (void)setSharedImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration | ||
| imageCache:(nullable id<PINRemoteImageCaching>)imageCache | ||
| { | ||
| PINRemoteImageManager *sharedManager = [self PINRemoteImageManagerWithConfiguration:configuration imageCache:imageCache]; | ||
| [self setSharedPreconfiguredRemoteImageManager:sharedManager]; | ||
| } | ||
|
|
||
| static dispatch_once_t shared_init_predicate; | ||
|
|
||
| + (void)setSharedPreconfiguredRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager | ||
| { | ||
| NSAssert(sharedDownloader == nil, @"Singleton has been created and session can no longer be configured."); | ||
| NSAssert1(sharedPINRemoteImageManager == nil, @"An instance of %@ has been set. Either configuration or preconfigured image manager can be set at a time and only once.", [[sharedPINRemoteImageManager class] description]); | ||
wiseoldduck marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| dispatch_once(&shared_init_predicate, ^{ | ||
| sharedPINRemoteImageManager = preconfiguredPINRemoteImageManager; | ||
| }); | ||
| } | ||
|
|
||
| + (PINRemoteImageManager *)PINRemoteImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration imageCache:(nullable id<PINRemoteImageCaching>)imageCache | ||
| { | ||
| PINRemoteImageManager *manager = nil; | ||
| // Check that Carthage users have linked both PINRemoteImage & PINCache by testing for one file each | ||
| if (!(NSClassFromString(@"PINRemoteImageManager"))) { | ||
|
There was a problem hiding this comment. We should put that behind a debug flag. There was a problem hiding this comment. I think we need to put these checks behind the There was a problem hiding this comment. I wondered as I looked over it the first time why this was the case? cc @garrettmoon There was a problem hiding this comment. I think I hit the error condition once, and didn't get the exception, and saw that, and thought it would be helpful, and it didn't have anything really to do with I can see putting it behind There was a problem hiding this comment. Of course now that I've done it, it seems like it might as well be an assertion instead if it's only on I agree that I'm interested in Garret's take here as well. |
||
| NSException *e = [NSException | ||
| exceptionWithName:@"FrameworkSetupException" | ||
| reason:@"Missing the path to the PINRemoteImage framework." | ||
| userInfo:nil]; | ||
| @throw e; | ||
| } | ||
| if (!(NSClassFromString(@"PINCache"))) { | ||
| NSException *e = [NSException | ||
| exceptionWithName:@"FrameworkSetupException" | ||
| reason:@"Missing the path to the PINCache framework." | ||
| userInfo:nil]; | ||
| @throw e; | ||
| } | ||
| #if PIN_ANIMATED_AVAILABLE | ||
| sharedPINRemoteImageManager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration | ||
wiseoldduck marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| alternativeRepresentationProvider:[self sharedDownloader] | ||
| imageCache:imageCache]; | ||
| #else | ||
| manager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration | ||
|
There was a problem hiding this comment. Why did this change to manager instead of There was a problem hiding this comment. because it's not shared - I funneled all the set-shared logic into as few places (two, unfortunately couldn't get it to one without changing the API of the getter) as possible to reduce complexity. This method just configures a manager and returns it to you. There was a problem hiding this comment.
This method is no longer a setter but a builder. There was a problem hiding this comment. Cool than we should change the other |
||
| alternativeRepresentationProvider:nil | ||
| imageCache:imageCache]; | ||
| #endif | ||
| return manager; | ||
| } | ||
|
|
||
| - (PINRemoteImageManager *)sharedPINRemoteImageManager | ||
| { | ||
| if (!sharedPINRemoteImageManager) { | ||
|
There was a problem hiding this comment. Any reason we have to have this check here? There was a problem hiding this comment. Give me a minute to try to remember lol! If I can't, I will write it off as a brain fart... There was a problem hiding this comment. OK, I took nine, and I got nothing! I am pretty sure I had worked myself into some overcomplicated concurrency scenario that cannot actually exist and wouldn't have been completely helped anyway |
||
| dispatch_once(&shared_init_predicate, ^{ | ||
| sharedPINRemoteImageManager = [ASPINRemoteImageDownloader PINRemoteImageManagerWithConfiguration:nil imageCache:nil]; | ||
wiseoldduck marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
| } | ||
| return sharedPINRemoteImageManager; | ||
| } | ||
|
|
||
| - (BOOL)sharedImageManagerSupportsMemoryRemoval | ||
|
|
||
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.
If we remove the nullable here it does not match with the implementation. Any reason we removed it?
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.
I should change the implementation if I didn't. It makes no sense to call this with
nil, to support it would be to add a check right away along the lines ofif (preconfiguredPINRemoteImageManager == nil) return;which is silly.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.
Removing
nullablehere is fine. It doesn't make sense to pass in anilmanager here anyway, and it isn't obvious what the framework would do in that case.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.
Looks like I did actually also change the implementation so I'm happy with this as is.
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.
Please also update the doc comments. The doc still say passing in a nil as the PINRemoteImageManager, it will create a default one.