-
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
Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197
Changes from 6 commits
c73e66f
d24eb06
db62c56
66882a0
f1573ae
7b27e9d
c6d0950
b325198
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 |
---|---|---|
|
@@ -75,7 +75,7 @@ @interface ASPINRemoteImageManager : PINRemoteImageManager | |
@implementation ASPINRemoteImageManager | ||
|
||
//Share image cache with sharedImageManager image cache. | ||
- (id <PINRemoteImageCaching>)defaultImageCache | ||
+ (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,68 @@ + (ASPINRemoteImageDownloader *)sharedDownloader NS_RETURNS_RETAINED | |
|
||
+ (void)setSharedImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration | ||
{ | ||
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]); | ||
__unused PINRemoteImageManager *sharedManager = [self sharedPINRemoteImageManagerWithConfiguration:configuration preconfiguredPINRemoteImageManager:nil]; | ||
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]; | ||
} | ||
|
||
+ (void)setSharedPreconfiguredRemoteImageManager:(nullable PINRemoteImageManager *)preconfiguredPINRemoteImageManager | ||
static dispatch_once_t shared_init_predicate; | ||
|
||
+ (void)setSharedPreconfiguredRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager | ||
{ | ||
NSAssert(preconfiguredPINRemoteImageManager != nil, @"setSharedPreconfiguredRemoteImageManager requires a non-nil parameter"); | ||
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. We may should use |
||
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
|
||
__unused PINRemoteImageManager *sharedManager = [self sharedPINRemoteImageManagerWithConfiguration:nil preconfiguredPINRemoteImageManager:preconfiguredPINRemoteImageManager]; | ||
|
||
dispatch_once(&shared_init_predicate, ^{ | ||
sharedPINRemoteImageManager = preconfiguredPINRemoteImageManager; | ||
}); | ||
} | ||
|
||
+ (PINRemoteImageManager *)sharedPINRemoteImageManagerWithConfiguration:(NSURLSessionConfiguration *)configuration preconfiguredPINRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager | ||
+ (PINRemoteImageManager *)PINRemoteImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration imageCache:(nullable id<PINRemoteImageCaching>)imageCache | ||
{ | ||
NSAssert(!(configuration != nil && preconfiguredPINRemoteImageManager != nil), @"Either configuration or preconfigured image manager can be set at a time."); | ||
static dispatch_once_t onceToken; | ||
dispatch_once(&onceToken, ^{ | ||
|
||
if (preconfiguredPINRemoteImageManager) { | ||
sharedPINRemoteImageManager = preconfiguredPINRemoteImageManager; | ||
} else { | ||
PINRemoteImageManager *manager = nil; | ||
#if DEBUG | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should put that behind a debug flag. 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. OK 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 think we need to put these checks behind the 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 wondered as I looked over it the first time why this was the case? cc @garrettmoon 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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; | ||
} | ||
#endif | ||
#if PIN_ANIMATED_AVAILABLE | ||
// Check that Carthage users have linked both PINRemoteImage & PINCache by testing for one file each | ||
if (!(NSClassFromString(@"PINRemoteImageManager"))) { | ||
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; | ||
} | ||
sharedPINRemoteImageManager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration | ||
alternativeRepresentationProvider:[self sharedDownloader]]; | ||
manager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration | ||
alternativeRepresentationProvider:[self sharedDownloader] | ||
imageCache:imageCache]; | ||
#else | ||
sharedPINRemoteImageManager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration]; | ||
manager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration | ||
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. Why did this change to manager instead of 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This method is no longer a setter but a builder. 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. Cool than we should change the other |
||
alternativeRepresentationProvider:nil | ||
imageCache:imageCache]; | ||
#endif | ||
} | ||
}); | ||
return sharedPINRemoteImageManager; | ||
return manager; | ||
} | ||
|
||
- (PINRemoteImageManager *)sharedPINRemoteImageManager | ||
{ | ||
return [ASPINRemoteImageDownloader sharedPINRemoteImageManagerWithConfiguration:nil preconfiguredPINRemoteImageManager:nil]; | ||
dispatch_once(&shared_init_predicate, ^{ | ||
sharedPINRemoteImageManager = [ASPINRemoteImageDownloader PINRemoteImageManagerWithConfiguration:nil imageCache:nil]; | ||
}); | ||
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
nullable
here is fine. It doesn't make sense to pass in anil
manager 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.