add image size to onLoad nativeEvent #2364

felixakiragreen · 2015-08-17T17:38:05Z

Issue: #494
Related: #2180

paramaggarwal · 2015-08-17T17:42:52Z

Excellent. :)

felixakiragreen · 2015-08-17T18:16:14Z

@paramaggarwal can you tell me why the checks failed? This is my first PR...

paramaggarwal · 2015-08-17T18:25:21Z

Network failed on the CI server. Try pushing another commit to trigger a new build.

ide · 2015-08-17T23:51:39Z

I think this returns the displayed size, not the intrinsic size. Can you verify?

hayeah · 2015-08-18T02:05:57Z

@ide @DUBERT RCTImageView gets the resized dimensions.

Would need to change downloadImageForURL to return the intrinsic image size.

Relevant code:

react-native/Libraries/Image/RCTImageLoader.m

Line 220 in f0dd9fb

RCTDispatchCallbackOnMainQueue(completionBlock, error, image);

return [_bridge.imageDownloader downloadImageForURL:url size:size scale:scale resizeMode:resizeMode progressBlock:progressBlock completionBlock:^(NSError *error, id image) {
      RCTDispatchCallbackOnMainQueue(completionBlock, error, image);
    }];

felixakiragreen · 2015-08-19T16:40:32Z

I don't know Objective-C, so here's my best shot. I expect there will be a number of revisions necessary to bring it up to snuff.

I believe there is value in providing both the displayed size and intrinsic image size. So I kept size and added dimensions and scale (so that pixel value can be calculated if wanted).

Because I modified the imageLoader function and callback, I had to update it in a bunch of locations, passing nil where dimensions weren't relevant. I didn't know a better way to approach this.

Thoughts @ide, @hayeah ?

paramaggarwal · 2015-08-19T16:56:52Z

Wow, I didn't know that RN resizes the image based on the size it will be used at. Quite a nice memory optimisation. My vote is that only dimensions are needed as size is available on onLayout and scale is available using PixelRatio.

ide · 2015-08-19T20:37:50Z

I'm not sure it's worth showing the displayed size because that should be the size of the Image component. Basically what @paramaggarwal said.

felixakiragreen · 2015-08-20T00:38:28Z

Updated to only show size for network images.

paramaggarwal · 2015-08-20T05:44:31Z

Looks good! Small nitpick - no need for the if (intrinsicSize.height != 0 && intrinsicSize.width != 0) checking - if it 0 it will be sent as 0 - not a problem.

felixakiragreen · 2015-08-20T13:23:22Z

@paramaggarwal I'm checking it so that I don't pass size: { height: 0, width: 0 } for non-network images. That way the user doesn't think there is a bug or something is returning incorrectly.

paramaggarwal · 2015-08-20T15:46:11Z

@DUBERT I don't think that is a problem. Let's wait for others' thoughts.

felixakiragreen · 2015-08-28T14:51:59Z

pinging @ide @hayeah who needs to review this?

ide · 2015-08-28T19:53:03Z

Libraries/Image/RCTImageView.m

@@ -126,7 +126,7 @@ - (void)reloadImage
                                    scale:RCTScreenScale()
                               resizeMode:self.contentMode
                            progressBlock:progressHandler
-                          completionBlock:^(NSError *error, id image) {
+                          completionBlock:^(NSError *error, id image, id dimensions) {


NSValue instead of id

felixakiragreen · 2015-08-31T15:58:29Z

@ide updated and squashed

jnhuynh · 2015-09-09T20:20:52Z

+1

ide · 2015-09-09T21:44:20Z

cc @a2 @nicklockwood for review

felixakiragreen · 2015-09-21T13:56:09Z

@paramaggarwal any idea why the checks have failed? You were able to help me last time

paramaggarwal · 2015-09-21T14:46:33Z

@DUBERT

RCTImageLoaderTests.m:46:33: error: too few arguments to block call, expected 3, have 2
    completionHandler(nil, image);

felixakiragreen · 2015-10-01T17:35:43Z

@ide, @a2, @nicklockwood for review? 😸

ide · 2015-10-07T07:33:20Z

Libraries/Image/RCTImageView.m

@@ -194,7 +194,18 @@ - (void)reloadImage
        }
      } else {
        if (_onLoad) {
-          _onLoad(nil);
+          if (dimensions != nil) {
+            CGSize size = [dimensions CGSizeValue];


~~Could we greatly simplify this PR by using image.size (may need to be careful to account for screen scale -- but it sure would make this PR super short :D )?~~

nvm, that goes back to my original comment on this PR

facebook-github-bot · 2015-10-20T19:16:23Z

@DUBERT updated the pull request.

facebook-github-bot · 2015-10-20T19:33:18Z

@DUBERT updated the pull request.

add dimensions and scale properties to the nativeEvent add comments add check for images without intrinsic size only show size for network images clean up formatting create event first, check for nil change id dimensions to NSValue *dimensions change to NSValue *dimensions catch http images remove file git included add tests fix tests fix errors remove console.log

facebook-github-bot · 2015-10-20T19:49:05Z

@DUBERT updated the pull request.

facebook-github-bot · 2015-10-20T19:50:27Z

@DUBERT updated the pull request.

nicklockwood · 2015-10-21T08:32:01Z

I'm not sure I understand the purpose of this. Why not just get the size from the image directly? (image.size). Why do you care how big the image was originally before we resized it? How would this information be used?

nicklockwood · 2015-10-21T08:44:52Z

OK, sorry, should've read the attached issue properly. I understand the problem, but this is the wrong solution. Loading the image at the wrong size using the <Image> tag and then resizing it to the size you want is pretty hacky, and I don't want to build an API around supporting a hack - if we're going to make API changes then we should support the use case properly, even if it means a bigger change.

I'd be fine with sending the actual loaded image size in the callback. One option would be to do that, then you could load the image at the maximum possible size and then resize it. Still a hack, but at least the API would be logical. You could embed the <Image> inside a smaller view with overflow: hidden, or obscure it behind another view while it's loading to prevent it from messing up the UI before it's resized.

In terms of solving the actual use case, I think we need to add a way to preload images and get their size without having to display the image while its loading. It would be relative easy to expose something like that, but I'll need to discuss with the Android team what the API should look like.

eyaleizenberg · 2015-11-05T14:57:49Z

I tried adding these on RN 0.13 and I couldn't get the project to start... 😢

olofd · 2015-12-03T17:20:22Z

I really need this fix as well. Is there any work going on here?

felixakiragreen · 2015-12-03T20:09:28Z

@olofd Nope. I don't know how to proceed. It sounds like the FB devs don't like the way I solved this. But it's not a feature they are planning to provide themselves. So right now I'm just having to use a forked version of react-native. I'm several versions behind because I'm tired of rebasing :(

olofd · 2015-12-05T12:43:48Z

Ok @DUBERT . for everyone who has a problem with this and want to be able to support remote images with unknown dimensions I made a hack you can use before the react team supports the use-case.

This branch is not suppose to be beautiful, rather to touch as little as possible, so it'll be easy to keep merging, and still give you the functionality.
(Decided to not use your PR as base @DUBERT because it was so 'correct' 😄 , and thereby makes it harder to merge/rebase)

It's based on 0.16.0:
https://github.com/olofd/react-native/tree/Remote-Image-Dimensions

Compare against 0.16.0:
https://github.com/facebook/react-native/compare/0.16-stable...olofd:Remote-Image-Dimensions?expand=1

To use it. Just change your package.json react-native-version to:
"react-native": "git://github.com/olofd/react-native.git#Remote-Image-Dimensions"

I'll try to keep merging it with new releases.

felixakiragreen · 2015-12-07T15:08:23Z

That looks great @olofd, I'll use it in my project.

@nicklockwood Do you have any more thoughts about this PR? Should I go ahead and close it? Or is it worth trying to figure out how to preload images and get their size? Please keep in mind I have very little Objective-C experience.

nicklockwood · 2015-12-07T15:24:06Z

I'm still actively investigating the right API for preloading. I don't want to add something we won't be able to support moving forward. Sorry about the wait.

olofd · 2015-12-12T10:43:28Z

Updated my branch with this feature for v0.17.0-rc:
https://github.com/olofd/react-native/tree/Image-Dimensions-0.17.0-rc

Use this in package.json:
"react-native": "git://github.com/olofd/react-native.git#Image-Dimensions-0.17.0-rc"

Compare-view:
https://github.com/facebook/react-native/compare/0.17-stable...olofd:Image-Dimensions-0.17.0-rc?expand=1

onchainguy-btc · 2015-12-14T18:06:24Z

@olofd using your branch, too. Works like a charm 👍

onchainguy-btc · 2015-12-16T14:01:06Z

@olofd do you have any experience on using this with Android? Seems like the events are not fired there (#4807)

olofd · 2015-12-20T22:01:05Z

Updated my branch with this feature for v0.17.0:
https://github.com/olofd/react-native/tree/Image-Dimensions-0.17.0

Use this in package.json:
"react-native": "git://github.com/olofd/react-native.git#Image-Dimensions-0.17.0"

dbuarque · 2016-03-22T21:05:32Z

So, what's the deal?

facebook-github-bot added the GH Review: review-needed label Aug 17, 2015

felixakiragreen mentioned this pull request Aug 17, 2015

[Image] Getting intrinsic Image size in the onLoad event #2180

Closed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 17, 2015

ide added GH Review: needs-revision and removed GH Review: review-needed labels Aug 18, 2015

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Aug 19, 2015

ide reviewed Aug 28, 2015
View reviewed changes

felixakiragreen force-pushed the network-image-size-onload branch from b4b9c01 to a40f9c1 Compare August 31, 2015 15:19

felixakiragreen force-pushed the network-image-size-onload branch 3 times, most recently from b0ffe88 to fddc263 Compare September 11, 2015 15:26

felixakiragreen force-pushed the network-image-size-onload branch from 77e25fe to 8a3731c Compare October 1, 2015 14:14

ide reviewed Oct 7, 2015
View reviewed changes

felixakiragreen force-pushed the network-image-size-onload branch from 8a3731c to e9f6791 Compare October 20, 2015 19:12

felixakiragreen force-pushed the network-image-size-onload branch from e9f6791 to e7383fe Compare October 20, 2015 19:31

felixakiragreen force-pushed the network-image-size-onload branch from e7383fe to 1e632f1 Compare October 20, 2015 19:47

felixakiragreen force-pushed the network-image-size-onload branch from 1e632f1 to 6544806 Compare October 20, 2015 19:49

felixakiragreen closed this Jan 21, 2016

felixakiragreen deleted the network-image-size-onload branch March 2, 2016 17:25

add image size to onLoad nativeEvent #2364

add image size to onLoad nativeEvent #2364

Conversation

felixakiragreen commented Aug 17, 2015

paramaggarwal commented Aug 17, 2015

felixakiragreen commented Aug 17, 2015

paramaggarwal commented Aug 17, 2015

ide commented Aug 17, 2015

hayeah commented Aug 18, 2015

felixakiragreen commented Aug 19, 2015

paramaggarwal commented Aug 19, 2015

ide commented Aug 19, 2015

felixakiragreen commented Aug 20, 2015

paramaggarwal commented Aug 20, 2015

felixakiragreen commented Aug 20, 2015

paramaggarwal commented Aug 20, 2015

felixakiragreen commented Aug 28, 2015

ide Aug 28, 2015

Choose a reason for hiding this comment

felixakiragreen commented Aug 31, 2015

jnhuynh commented Sep 9, 2015

ide commented Sep 9, 2015

felixakiragreen commented Sep 21, 2015

paramaggarwal commented Sep 21, 2015

felixakiragreen commented Oct 1, 2015

ide Oct 7, 2015

Choose a reason for hiding this comment

facebook-github-bot commented Oct 20, 2015

facebook-github-bot commented Oct 20, 2015

facebook-github-bot commented Oct 20, 2015

facebook-github-bot commented Oct 20, 2015

nicklockwood commented Oct 21, 2015

nicklockwood commented Oct 21, 2015

eyaleizenberg commented Nov 5, 2015

olofd commented Dec 3, 2015

felixakiragreen commented Dec 3, 2015

olofd commented Dec 5, 2015

felixakiragreen commented Dec 7, 2015

nicklockwood commented Dec 7, 2015

olofd commented Dec 12, 2015

onchainguy-btc commented Dec 14, 2015

onchainguy-btc commented Dec 16, 2015

olofd commented Dec 20, 2015

dbuarque commented Mar 22, 2016