[ASNetworkImageNode] Whenever URL is set, prefer it over a manually-set .image.#2774
Conversation
After much discussion I think this is the correct behavior. It seems like it's much more likely to be the expected behavior but still enables you to use the network image node like an regular image node.
|
@maicki @appleguy @Adlai-Holler would appreciate your eyes on this :) |
maicki
left a comment
maicki
left a comment
There was a problem hiding this comment.
After thinking about it I think we should take a step back and let ASNetworkImageNode not let to set any image directly at all. We should assert in this case and in the assertion message we should point to the defaultImage. In production if someone set's the image directly we would just call through to the default image.
| [self _cancelDownloadAndClearImage]; | ||
| _URL = nil; | ||
| } | ||
| _imageWasSetExternally = (image != nil && _URL != nil); |
There was a problem hiding this comment.
I think this change would make the ASNetworkImage back to the old unpredictables behavior. The behavior of the network image node would be based on the order of setting the properties. The behavior would depend on setting the URL before or after setting the image. Without this change it behaved like either an image node or a network image node, now it's kind of a hybrid thing ...
There was a problem hiding this comment.
@maicki I don't think that's quite right, due to the code in setURL, which prevents the problem.
The problem case would seem to be when you do setImage:, and then later do setURL:, this code will do _imageWasSetExternally = YES; however, that's not the case because setURL: will clear the flag.
garrettmoon
changed the title
appleguy
left a comment
appleguy
left a comment
There was a problem hiding this comment.
We may need to change this code further, but I'm going to land this now for a simple reason: the current state in master is not correct, and causes Pinterest to NEVER (100%) load the correct-resolution image.
In other words, today, this NEVER works:
.image = lowerResolutionStarterImage;
.url = URLForActualFullResImage;
In this case, or with the opposite call order, we will never load the image!
That said, we should all agree -- the behavior we land on MUST be the same regardless of the order you set .image and .URL. I believe that is the case now after this patch, but if it isn't, we'll change it again until it is consistent.
By landing this, we'll at least get a Pinterest build to confirm the behavior is improved and from there we can write a unit test to run through all 4 permutations of setting these two properties.
| [self _cancelDownloadAndClearImage]; | ||
| _URL = nil; | ||
| } | ||
| _imageWasSetExternally = (image != nil && _URL != nil); |
There was a problem hiding this comment.
@maicki I don't think that's quite right, due to the code in setURL, which prevents the problem.
The problem case would seem to be when you do setImage:, and then later do setURL:, this code will do _imageWasSetExternally = YES; however, that's not the case because setURL: will clear the flag.
appleguy
changed the title
5 participants
After much discussion I think this is the correct behavior. It seems
like it's much more likely to be the expected behavior but still enables
you to use the network image node like an regular image node.
Resolves #2756