Add a method for setting preconfigured PINRemoteImageManager #1124

ernestmama · 2018-09-17T18:29:19Z

Add a method for setting preconfigured PINRemoteImageManager instead of using the self-created ASPINRemoteImageManager.

In this way, we can add custom configuration to the PINRemoteImageManager such as request configuration/url configuration.

…of using the self-created ASPINRemoteImageManager

CLAassistant · 2018-09-17T18:29:26Z

All committers have signed the CLA.

maicki · 2018-09-17T19:48:38Z

Source/Details/ASPINRemoteImageDownloader.m

@@ -155,9 +159,18 @@ + (PINRemoteImageManager *)sharedPINRemoteImageManagerWithConfiguration:(NSURLSe

 - (PINRemoteImageManager *)sharedPINRemoteImageManager
 {
+  if (_preconfiguredPINRemoteImageManager) {


Is this thread safe? Do we have to lock this?

maicki · 2018-09-17T19:49:15Z

Source/Details/ASPINRemoteImageDownloader.m

  return [ASPINRemoteImageDownloader sharedPINRemoteImageManagerWithConfiguration:nil];
 }

+- (PINRemoteImageManager *)setPreconfiguredPINRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager
+{
+  _preconfiguredPINRemoteImageManager = preconfiguredPINRemoteImageManager;


I think we should add a lock here. Furthermore why do we return the same object here and od not make it a regular setter?

Actually, I was going to make it return a void since it is setting.

Let me make it thread safe.

nguyenhuy · 2018-09-17T20:15:18Z

It's a valid concern that the new addition should be thread-safe. However, I'm not sure about adding a lock around -sharedPINRemoteImageManager. Almost every method of the downloader hits it eventually, and since the downloader is shared between all nodes, it's a very hot code path and there'll be lock contention.

How about we use the same pattern as -setSharedImageManagerWithConfiguration: to allow users to set the class of sharedPINRemoteImageManager? The class is required to be set very early on, even before +setSharedImageManagerWithConfiguration: which then will be picked up by +sharedPINRemoteImageManagerWithConfiguration:.

maicki · 2018-09-17T20:19:27Z

The suggestion from @nguyenhuy sounds good to me.

ghost · 2018-09-17T20:36:21Z

🚫 CI failed with log

ernestmama · 2018-09-17T20:38:18Z

@nguyenhuy Shouldn't Texture knows nothing about the custom image manager except it is a subclass of PINRemoteImageMager to keep the abstraction? The subclass is in a higher level app/library.

nguyenhuy · 2018-09-17T20:55:09Z

@ernestmama Yes, and with the new API, it still wouldn't know anything, does it?

ernestmama · 2018-09-17T21:12:26Z

@nguyenhuy How about this update? This will essentially change that the RemoteImageManager can only be set once (same as previous with configuration) instead of able to swapping it in runtime. It will need to be set before the first time ImageManager is being used.

nguyenhuy

Yeah, that's a better idea.

nguyenhuy · 2018-09-17T21:12:44Z

Source/Details/ASPINRemoteImageDownloader.h

+ * Sets a custom perconconfigured PINRemoteImageManager that will be used by @c ASNetworkImageNodes and @c ASMultiplexImageNodes
+ * while loading images off the network. This must be specified early in the application lifecycle before
+ * `sharedDownloader` is accessed. If nil is passed in as the PINRemoteImageManager, it will call
+ * setSharedImageManagerWithConfiguration with nil configuration.


it will call setSharedImageManagerWithConfiguration with nil configuration.

Does it? How about:

it will create a default image manager with a nil session configuration.

You are rrright.

…image manager can be set at a time

nguyenhuy

Nearly there.

Also, this diff is not trivial. It introduces a new API, please update changelog.

nguyenhuy · 2018-09-17T22:40:41Z

Source/Details/ASPINRemoteImageDownloader.m

+
+ (PINRemoteImageManager *)sharedPINRemoteImageManagerWithConfiguration:(NSURLSessionConfiguration *)configuration preconfiguredPINRemoteImageManager:(PINRemoteImageManager *)preconfiguredPINRemoteImageManager
+{
+  NSAssert(configuration != nil && preconfiguredPINRemoteImageManager != nil, @"Either configuration or preconfigured image manager can be set at a time.");


This assertion means both configuration and preconfigured manager must not be nil. I don't think it's right.

nguyenhuy · 2018-09-17T22:41:37Z

Source/Details/ASPINRemoteImageDownloader.m

  dispatch_once(&onceToken, ^{

+    if (preconfiguredPINRemoteImageManager) {
+      sharedPINRemoteImageManager = preconfiguredPINRemoteImageManager;
+    } else {
 #if PIN_ANIMATED_AVAILABLE
    // Check that Carthage users have linked both PINRemoteImage & PINCache by testing for one file each
    if (!(NSClassFromString(@"PINRemoteImageManager"))) {


The indentation seems to be off. The whole else body here should be indented.

…RemoteImageManager (TextureGroup#1124) * add a method for setting preconfigured PINRemoteImageManager instead of using the self-created ASPINRemoteImageManager * update preconfigured image manager where it can only be set once * fix spacing in downloader * Fix doc/comments on new api * adding assertion to ensure either only configuration or preconfigure image manager can be set at a time * adding assertion to ensure either only configuration or preconfigure image manager can be set at a time * fix assertion condition * Update CHANGELOG.md * Remove unnecessary change

add a method for setting preconfigured PINRemoteImageManager instead …

e3ca0dc

…of using the self-created ASPINRemoteImageManager

Adlai-Holler requested a review from garrettmoon September 17, 2018 18:35

nguyenhuy requested a review from maicki September 17, 2018 19:44

maicki reviewed Sep 17, 2018

View reviewed changes

ernestmama added 2 commits September 17, 2018 14:07

update preconfigured image manager where it can only be set once

f189a83

fix spacing in downloader

9919bdb

ernestmama changed the title ~~add a method for setting preconfigured PINRemoteImageManager~~ add a method for setting preconfigured PINRemoteImageManager #trivial Sep 17, 2018

nguyenhuy approved these changes Sep 17, 2018

View reviewed changes

ernestmama added 4 commits September 17, 2018 14:19

Fix doc/comments on new api

8430060

adding assertion to ensure either only configuration or preconfigure …

b23daf6

…image manager can be set at a time

adding assertion to ensure either only configuration or preconfigure …

683ca29

…image manager can be set at a time

adding assertion to ensure either only configuration or preconfigure …

31a6fbb

…image manager can be set at a time

nguyenhuy requested changes Sep 17, 2018

View reviewed changes

fix assertion condition

a2bdf7b

nguyenhuy approved these changes Sep 17, 2018

View reviewed changes

nguyenhuy changed the title ~~add a method for setting preconfigured PINRemoteImageManager #trivial~~ Add a method for setting preconfigured PINRemoteImageManager Sep 17, 2018

Update CHANGELOG.md

2dbef33

maicki approved these changes Sep 17, 2018

View reviewed changes

Remove unnecessary change

e19bb45

nguyenhuy merged commit 1452b31 into TextureGroup:master Sep 18, 2018

ernestmama deleted the injectedRemoteImageManager branch September 18, 2018 00:22

nguyenhuy mentioned this pull request Sep 21, 2018

Allow creating a ASPINRemoteImageDownloader that ignores caches #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a method for setting preconfigured PINRemoteImageManager #1124

Add a method for setting preconfigured PINRemoteImageManager #1124

ernestmama commented Sep 17, 2018

CLAassistant commented Sep 17, 2018 •

edited

Loading

maicki Sep 17, 2018

maicki Sep 17, 2018

ernestmama Sep 17, 2018

ernestmama Sep 17, 2018

nguyenhuy commented Sep 17, 2018 •

edited

Loading

maicki commented Sep 17, 2018

ghost commented Sep 17, 2018

ernestmama commented Sep 17, 2018

nguyenhuy commented Sep 17, 2018

ernestmama commented Sep 17, 2018 •

edited

Loading

nguyenhuy left a comment •

edited

Loading

nguyenhuy Sep 17, 2018

ernestmama Sep 17, 2018

nguyenhuy left a comment

nguyenhuy Sep 17, 2018

nguyenhuy Sep 17, 2018

Add a method for setting preconfigured PINRemoteImageManager #1124

Add a method for setting preconfigured PINRemoteImageManager #1124

Conversation

ernestmama commented Sep 17, 2018

CLAassistant commented Sep 17, 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

nguyenhuy commented Sep 17, 2018 • edited Loading

maicki commented Sep 17, 2018

ghost commented Sep 17, 2018

ernestmama commented Sep 17, 2018

nguyenhuy commented Sep 17, 2018

ernestmama commented Sep 17, 2018 • edited Loading

nguyenhuy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Sep 17, 2018 •

edited

Loading

nguyenhuy commented Sep 17, 2018 •

edited

Loading

ernestmama commented Sep 17, 2018 •

edited

Loading

nguyenhuy left a comment •

edited

Loading