[ASImageNode] Add handling of setting .image in specific ASImageNode subclasses #2708
Conversation
AsyncDisplayKit/ASImageNode.mm
Outdated
| { | ||
| [self __setImage:image]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add a comment: "This is exposed to library subclasses, i.e. ASNetworkImageNode, ASMultiplexImageNode and ASVideoNode"
|
|
||
| - (void)setImage:(UIImage *)image | ||
| { | ||
| ASDisplayNodeAssert(NO, @"Setting the image directly to an ASMultiplexImageNode is not allowed."); |
There was a problem hiding this comment.
Should the assert explain why?
"Setting the image directly on an ASMultiplexImageNode is unsafe. It will be cleared in didExitPreloadRange and will have no way to restore in didEnterPreloadRange"
|
As we discussed, let's remove the assert and do one of the following:
I advocate for the first one, but I don't feel strongly about it. I think it's reasonable to say that if a URL and an image are set, the node should prefer the remote image. |
|
Actually on second thought, I advocate for the second option. If someone sets an image on the node externally, we should prefer that and perhaps not even both loading the image from URL. This way the developer has finer control over how the node appears. If I set an image on it, that image will get shown full stop. |
|
Ok let's roll with option two for now. |
|
|
||
| - (void)setURL:(NSURL *)URL | ||
| { | ||
| [self setURL:URL resetToDefault:YES]; |
There was a problem hiding this comment.
Should setURL clear out _imageWasSetExternally?
There was a problem hiding this comment.
It will clear it out now yes
maicki
changed the title
garrettmoon
left a comment
garrettmoon
left a comment
There was a problem hiding this comment.
What are the video node changes hoping to achieve overall?
| * ASNetworkImageNode to set the image. | ||
| * | ||
| * This is exposed to library subclasses, i.e. ASNetworkImageNode, ASMultiplexImageNode and ASVideoNode for setting | ||
| * the image directly without going throug the setter of the superclass |
AsyncDisplayKit/ASVideoNode.mm
Outdated
|
|
||
| { | ||
| ASDN::MutexLocker l(__instanceLock__); | ||
| _asset = asset; |
There was a problem hiding this comment.
Ha, nice, fix some threading issues while you're here :)
|
@garrettmoon @appleguy Ready for another round. As already mentioned not really beautiful and unfortunately adds some clutter to the core classes :( |
|
|
||
| #ifdef DEBUG | ||
| if (_URL != nil) { | ||
| NSLog(@"Setting the image directly on an %@ and setting and setting an URL is not supported. If you want to use a placeholder image please use defaultImage .", NSStringFromClass([self class])); |
There was a problem hiding this comment.
Is this necessary? If not, I think we should support this condition. Think for example of using a network image node in a transition, and so you might want to override its image with some other one temporarily, to achieve a visual effect.
| } | ||
| __instanceLock__.unlock(); | ||
|
|
||
| [self _setImage:image]; |
There was a problem hiding this comment.
I think we may need to hold this lock while calling _setImage:, because we need to be sure that _imageWasSetExternally lines up with super.image.
Think if thread A calls this (image != nil), then B calls it just after (image == nil). A finishes the critical section having set _imageWasSetExternally = YES. Then A goes to sleep. Then B finishes the critical sections having set _imageWasSetExternally = NO, then B calls _setImage:nil, then A wakes up and calls _setImage:(nonnil). Now our flag doesn't match our image.
|
@garrettmoon @Adlai-Holler @appleguy Ready for review. |
4 participants
We had some confusion in our Slack channel as well as a P1 bug in Pinterest due to setting an image directly to the
imageproperty of anASNetworkImageNodethat get's blown away if the node get's into thedidExitPreloadState. This PR addresses this issue forASNetworkImageNodein there we make the following behavior changes:_imageWasSetExternallyand the node will act like a plainASImageNodedidExitPreloadStatewe check if the_imageWasSetExternally == trueand just return and don't blow away the image anymore. Same as theASImageNodedefaultImageand the image that was fetched based on theURLproperty.I tried to reduce the changes within the core classes as much as possible ... but unfortunately we still have to add some clutter ...
Resolves: #2699