Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197

wiseoldduck · 2018-10-27T04:14:37Z

No description provided.

…n initializing ASPINRemoteImageDownloader.

maicki · 2018-11-01T15:12:15Z

@nguyenhuy @ernestmama @garrettmoon Could someone please take a look over this PR. Thanks!

maicki · 2018-11-01T21:15:07Z

Source/Details/ASPINRemoteImageDownloader.m

-    } else {
+  PINRemoteImageManager *manager = nil;
+  // Check that Carthage users have linked both PINRemoteImage & PINCache by testing for one file each
+  if (!(NSClassFromString(@"PINRemoteImageManager"))) {


We should put that behind a debug flag.

I think we need to put these checks behind the PIN_ANIMATED_AVAILABLE flag, just like the previous implementation.

I wondered as I looked over it the first time why this was the case? cc @garrettmoon

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 PIN_ANIMATED_AVAILABLE specifically, so I moved it out. takes a breath

I can see putting it behind #if DEBUG, although it's just going to crash either way and for a check that is guaranteed to only ever run once it doesn't seem like two NSClassFromStrings is that expensive. But it's of limited use without a debugger attached (you would get a friendlier crash report from Apple if your released app hit here, but then presumably your app is crashing every time so ...... you hopefully have it figured out)

Of course now that I've done it, it seems like it might as well be an assertion instead if it's only on DEBUG.

I agree that I'm interested in Garret's take here as well.

maicki · 2018-11-01T21:15:38Z

Source/Details/ASPINRemoteImageDownloader.m

 }

 - (PINRemoteImageManager *)sharedPINRemoteImageManager
 {
-  return [ASPINRemoteImageDownloader sharedPINRemoteImageManagerWithConfiguration:nil preconfiguredPINRemoteImageManager:nil];
+  if (!sharedPINRemoteImageManager) {


Any reason we have to have this check here?

Give me a minute to try to remember lol!

If I can't, I will write it off as a brain fart...

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

maicki · 2018-11-01T21:20:26Z

Source/Details/ASPINRemoteImageDownloader.m

 #else
-      sharedPINRemoteImageManager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration];
+    manager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration


Why did this change to manager instead of sharedPINRemoteImageManager?

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.

Why did this change to manager instead of sharedPINRemoteImageManager?

This method is no longer a setter but a builder.

Cool than we should change the other sharedPINRemoteImageManager to the manager variable too :)

maicki · 2018-11-01T21:22:22Z

Source/Details/ASPINRemoteImageDownloader.h

@@ -47,7 +60,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 * @param preconfiguredPINRemoteImageManager The preconfigured remote image manager that will be used by `sharedDownloader`
 */
-+ (void)setSharedPreconfiguredRemoteImageManager:(nullable PINRemoteImageManager *)preconfiguredPINRemoteImageManager;
+ (void)setSharedPreconfiguredRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager;


If we remove the nullable here it does not match with the implementation. Any reason we removed it?

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 of if (preconfiguredPINRemoteImageManager == nil) return; which is silly.

Removing nullable here is fine. It doesn't make sense to pass in a nil manager here anyway, and it isn't obvious what the framework would do in that case.

Looks like I did actually also change the implementation so I'm happy with this as is.

Please also update the doc comments. The doc still say passing in a nil as the PINRemoteImageManager, it will create a default one.

nguyenhuy · 2018-11-01T21:53:23Z

Source/Details/ASPINRemoteImageDownloader.h

@@ -47,7 +60,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 * @param preconfiguredPINRemoteImageManager The preconfigured remote image manager that will be used by `sharedDownloader`
 */
-+ (void)setSharedPreconfiguredRemoteImageManager:(nullable PINRemoteImageManager *)preconfiguredPINRemoteImageManager;
+ (void)setSharedPreconfiguredRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager;


Removing nullable here is fine. It doesn't make sense to pass in a nil manager here anyway, and it isn't obvious what the framework would do in that case.

Source/Details/ASPINRemoteImageDownloader.m

nguyenhuy · 2018-11-01T21:56:08Z

Source/Details/ASPINRemoteImageDownloader.m

-    } else {
+  PINRemoteImageManager *manager = nil;
+  // Check that Carthage users have linked both PINRemoteImage & PINCache by testing for one file each
+  if (!(NSClassFromString(@"PINRemoteImageManager"))) {


I think we need to put these checks behind the PIN_ANIMATED_AVAILABLE flag, just like the previous implementation.

Source/Details/ASPINRemoteImageDownloader.m

nguyenhuy · 2018-11-01T21:58:02Z

Source/Details/ASPINRemoteImageDownloader.m

 #else
-      sharedPINRemoteImageManager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration];
+    manager = [[ASPINRemoteImageManager alloc] initWithSessionConfiguration:configuration


Why did this change to manager instead of sharedPINRemoteImageManager?

This method is no longer a setter but a builder.

Source/Details/ASPINRemoteImageDownloader.m

Co-Authored-By: wiseoldduck <[email protected]>

maicki · 2018-11-02T15:51:17Z

Source/Details/ASPINRemoteImageDownloader.m

 {
+  NSAssert(preconfiguredPINRemoteImageManager != nil, @"setSharedPreconfiguredRemoteImageManager requires a non-nil parameter");


We may should use NSParameterAssert for the parameter check.

ghost · 2018-11-03T05:46:23Z

🚫 CI failed with log

…ader. (TextureGroup#1197) * Allow configuring imageCache along with NSURLSessionConfiguration when initializing ASPINRemoteImageDownloader. * Update CHANGELOG.md * Update Source/Details/ASPINRemoteImageDownloader.m Co-Authored-By: wiseoldduck <[email protected]> * Put class linkage tests behind #if DEBUG * Remove silly nil check * Add non-nil assert * Update documentation

wiseoldduck and others added 2 commits October 26, 2018 21:08

Allow configuring imageCache along with NSURLSessionConfiguration whe…

c73e66f

…n initializing ASPINRemoteImageDownloader.

Update CHANGELOG.md

d24eb06

maicki requested review from garrettmoon and nguyenhuy October 27, 2018 17:37

maicki requested changes Nov 1, 2018

View reviewed changes

maicki reviewed Nov 1, 2018

View reviewed changes

nguyenhuy requested changes Nov 1, 2018

View reviewed changes

nguyenhuy and others added 4 commits November 1, 2018 17:00

Update Source/Details/ASPINRemoteImageDownloader.m

db62c56

Co-Authored-By: wiseoldduck <[email protected]>

Put class linkage tests behind #if DEBUG

66882a0

Remove silly nil check

f1573ae

Add non-nil assert

7b27e9d

maicki approved these changes Nov 2, 2018

View reviewed changes

Merge branch 'master' into configure-imagecache

c6d0950

nguyenhuy approved these changes Nov 3, 2018

View reviewed changes

Update documentation

b325198

maicki merged commit a3194f8 into TextureGroup:master Nov 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197

Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197

wiseoldduck commented Oct 27, 2018

maicki commented Nov 1, 2018

maicki Nov 1, 2018

wiseoldduck Nov 1, 2018

nguyenhuy Nov 1, 2018

maicki Nov 1, 2018 •

edited

Loading

wiseoldduck Nov 1, 2018

wiseoldduck Nov 2, 2018

maicki Nov 1, 2018 •

edited

Loading

wiseoldduck Nov 2, 2018

wiseoldduck Nov 2, 2018

maicki Nov 1, 2018

wiseoldduck Nov 1, 2018

nguyenhuy Nov 1, 2018

maicki Nov 1, 2018

maicki Nov 1, 2018

wiseoldduck Nov 1, 2018

nguyenhuy Nov 1, 2018

wiseoldduck Nov 2, 2018

ernestmama Nov 2, 2018

nguyenhuy Nov 1, 2018

nguyenhuy Nov 1, 2018

nguyenhuy Nov 1, 2018

maicki Nov 2, 2018

ghost commented Nov 3, 2018

		{
		NSAssert(preconfiguredPINRemoteImageManager != nil, @"setSharedPreconfiguredRemoteImageManager requires a non-nil parameter");

Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197

Allow configuring imageCache when initializing ASPINRemoteImageDownloader. #1197

Conversation

wiseoldduck commented Oct 27, 2018

maicki commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 3, 2018

maicki Nov 1, 2018 •

edited

Loading

maicki Nov 1, 2018 •

edited

Loading