RCTImagePickerManager crash on image from CameraRoll

[doochik]

Image from CameraRoll haven't UIImagePickerControllerReferenceURL. So we need to save it to PhotoAlbums first.

To save image I've used the same method from RCTCameraRollManager.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @nicklockwood, @dvcrn and @yusefnapora to be potential reviewers.

[javache]

Isn't this method already called on the main thread?

[javache]

I'm not sure if this is a good default behaviour. In internal projects, we've used RCTImageStoreManager to (temporarily) store images taken this way.

[doochik]

Yes, this is good point. I'll rewrite my PR to use temporary storage

[facebook-github-bot]

@doochik updated the pull request.

[doochik]

I've updated my PR. Images from CameraRoll now save to the temporary storage.

And about errors. I've fixed this bug #4411. So now i can pass args something like this. Do you like it?

- (void)imagePickerController:(UIImagePickerController *)picker
didFinishPickingMediaWithInfo:(NSDictionary<NSString *, id> *)info
{

  NSString *imageUri = [info[UIImagePickerControllerReferenceURL] absoluteString];

  // Image from PhotoLibrary
  if (imageUri) {
    [self _dismissPicker:picker args:@[imageUri] error:nil];

  } else {
    // Image from CameraRoll hasn't uri.
    // We need to save it to the store first.
    UIImage *originalImage = (UIImage *) [info objectForKey: UIImagePickerControllerOriginalImage];
    [_bridge.imageStoreManager storeImage:originalImage withBlock:^(NSString *tempImageTag) {
      if (!tempImageTag) {
        NSString *errorMessage = @"Error storing image in RCTImageStoreManager";
        RCTLogWarn(@"%@", errorMessage);
        [self _dismissPicker:picker args:nil error:RCTErrorWithMessage(errorMessage)];
        return;
      }
      [self _dismissPicker:picker args:@[tempImageTag] error:nil];
    }];
  }
}

- (void)imagePickerControllerDidCancel:(UIImagePickerController *)picker
{
  [self _dismissPicker:picker args:nil error:RCTErrorWithMessage(@"Cancel")];
}

[nicklockwood]

nit: can you use [...] syntax for accessing keys? Also, no need to cast here:

 UIImage *originalImage = info[UIImagePickerControllerOriginalImage];

[nicklockwood]

One minor style issue, but otherwise this is good to go.

[javache]

Can you add a warning here that this will cause a memory-leak as-is? You need to remove the image from the imageStoreManager once you're done using it. We're looking into providing automatic solutions for this.

[nicklockwood]

@javache do you mean a console warning? Or a warning in the documentation?

[javache]

Just documentation. A warning in the console that you can't fix doesn't sound very useful :)

[javache]

The error API looks good, but I don't think it's necessary in this case since imageStoreManager doesn't explicitly generate errors. (Any error would/should be a fatal runtime error).

[doochik]

I've fixed your comments, squash commits and rebase to master

[facebook-github-bot]

@doochik updated the pull request.

[nicklockwood]

@facebook-github-bot shipit

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/197772487225713/int_phab to review.

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/197772487225713/int_phab to review.

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/197772487225713/int_phab to review.