Support for `PHPickerViewController` #1654

JanGorman · 2021-03-22T10:14:03Z

Motivation

This PR adds support for the iOS 14 PHPickerViewController to address Issue #1428

Test Plan

Tested using the included example app to load images/videos and verify that the output matches that of the original.

JanGorman · 2021-03-22T10:15:03Z

ios/ImagePickerManager.m


 @import MobileCoreServices;

 @interface ImagePickerManager ()

-@property (nonatomic, strong) UIImagePickerController *picker;


The presenting view controller retains the picker so there's no requirement to hold on to it

JanGorman · 2021-03-22T10:16:36Z

ios/ImagePickerManager.m

@@ -198,12 +215,14 @@ - (void)checkCameraPermissions:(void(^)(BOOL granted))callback
    if (status == AVAuthorizationStatusAuthorized) {
        callback(YES);
        return;
-    } else if (status == AVAuthorizationStatusNotDetermined){
+    }
+    else if (status == AVAuthorizationStatusNotDetermined){


There was a mix of formatting

Johan-dutoit · 2021-03-22T11:09:23Z

Thanks for the PR, I'll see if I can squeeze in some testing soon and revert back.

ios/ImagePickerManager.m

ravirajn22 · 2021-03-22T17:59:43Z

@JanGorman I have added few comments, can you check it and revert with changes or your POV

JanGorman · 2021-03-23T07:39:03Z

@ravirajn22 Thanks for the comments. Agreed! I factored out the common pieces

ios/ImagePickerManager.m

ravirajn22 · 2021-03-23T09:22:52Z

Need to check the PR again, will update and push before this weekend.

Johan-dutoit · 2021-03-25T06:39:06Z

I did some testing and mostly works okay.

I get the following error when quickly tapping an image twice

JanGorman · 2021-03-25T06:56:48Z

@Johan-dutoit Thanks for the feedback. I'll see how to work around that

JanGorman · 2021-03-25T07:31:25Z

ios/ImagePickerManager.m

+    }
+
+    // Immediately dismiss the controller to prevent calling the completion handler twice
+    [picker dismissViewControllerAnimated:YES completion:nil];


@Johan-dutoit This change here fixes the double tap issue in my tests

It does a better job, but still fails if you click really fast.

You really want to try to break it 😅 I would suggest you make a call on how realistic this is (double tap speed vs. mouse clicks…). I don't see what other way there is to prevent this.

What's interesting is that it's reproducible (at least for me) only on the first double click. Subsequent attempts fail until you relaunch it. I'm able to reproduce this in a bare bones example and will report it as a bug to Apple.

class ViewController: UIViewController { @IBAction func pick(_ sender: Any) { let picker = PHPickerViewController(configuration: .init()) picker.delegate = self present(picker, animated: true) } } extension ViewController: PHPickerViewControllerDelegate { func picker(_ picker: PHPickerViewController, didFinishPicking results: [PHPickerResult]) { picker.dismiss(animated: true) // Prints twice the first time you double click really quickly print("Called") } }

Reported to Apple as FB9059292

I totally agree with your sentiments above. At first I didn't even want to mention it, but decided that it's good to have a trail.

Shouldn't the call to dismissViewControllerAnimated be made before we check for results? That's what's suggested in the WWDC video about PHPicker, I'm wondering if that has incidence in the bug you're facing with double/triple taps.

@flochtililoch Thanks for the comment. I made this change specifically to attempt to work around the mentioned bug. In my tests it didn't matter whether you call the completion block in the dismiss completion block or outside. It was always reproducible the first time that you present the picker but not on the second call which to me suggest that there is an issue.

Oh, what I meant was:

// First, dismiss picker [picker dismissViewControllerAnimated:YES completion:nil]; // Then, check results if (results.count == 0) { dispatch_async(dispatch_get_main_queue(), ^{ [picker dismissViewControllerAnimated:YES completion:^{ self.callback(@[@{@"didCancel": @YES}]); }]; }); return; } ...

Another thing worthwhile to explore, is around setting the flag preferredAssetRepresentationMode on the configuration object to PHPickerConfigurationAssetRepresentationModeCurrent. This blog article suggests that not setting it can have a performance impact on the import process.

Let's give this a go, would love to not face this :D

I moved it to the first line of the delegate callback. The double tap problem persists but in terms of consistency I guess this is better (only one dismiss instead of separate handling for the "cancel" case).
I also applied the suggestion to set the preferredAssetRepresentationMode

Johan-dutoit · 2021-03-29T07:42:32Z

if @ravirajn22 is happy with the code, I'm happy to give the thumbs up.

ravirajn22 · 2021-03-30T18:46:11Z

Sorry for the delay, haven't looked into the code. Give me few more days.

Sven65 · 2021-04-07T09:39:17Z

Does this PR allow users of the library to set the selection limit, in order to select multiple images at once?

JanGorman · 2021-04-07T10:33:34Z

@Sven65 It does not. The library currently doesn't offer this option either. It's straightforward to add that option but IMO better done in a separate PR

JanGorman · 2021-04-16T06:30:42Z

@ravirajn22 Have you had chance to review? If there's anything I can do to help unblock the PR please let me know 🙂

Johan-dutoit · 2021-04-26T07:53:39Z

@ravirajn22 last chances for a review, going to merge this today, unless there are any objections.

ravirajn22 · 2021-04-26T12:24:22Z

@Johan-dutoit I will review and tell before tomorrow. This change affects many lines including styling and refactor of unrelated lines. I don't know if its the right way to add feature + refactor together. Just give some time.

ravirajn22 · 2021-04-26T19:42:27Z

@JanGorman I have refactored your code, it now uses one function to process image (same for video), reducing duplicate codes and fixed few issues. Your code did not handle image types other than jpg and png. Please review this.

@Johan-dutoit Test this code and release a beta version if everything ok.

ravirajn22 · 2021-04-26T19:47:03Z

I did some testing and mostly works okay.

I get the following error when quickly tapping an image twice

Regarding this error, we can handle this in another PR. Just setting the callback to nil after sending response and checking if callback != nil before sending the response should fix this issue. This issue exist even in current production build I think.

JanGorman · 2021-04-27T06:18:55Z

ios/ImagePickerManager.h


 typedef NS_ENUM(NSInteger, RNImagePickerTarget) {
  camera = 1,
  library
 };

-@interface ImagePickerManager : NSObject <RCTBridgeModule, UINavigationControllerDelegate, UIActionSheetDelegate, UIImagePickerControllerDelegate>
+@interface ImagePickerManager : NSObject <RCTBridgeModule, UINavigationControllerDelegate, UIActionSheetDelegate, UIImagePickerControllerDelegate, PHPickerViewControllerDelegate>


@ravirajn22 Out of curiosity, why do you prefer declaring this on the public interface? For one, the outside world doesn't need to know that it conforms to the interface (personally I'd move UIActionSheetDelegate, UIImagePickerControllerDelegate, UINavigationControllerDelegate into the .m file too). This was one of the benefits that Obj-C has over Swift, that you can move the #import into the .m file making the link phase faster

I am no expert in objective c, so l like consistency over others. Since all previous protocols have been done on the public interface as you said, I thought its better if we can declare PHPickerViewControllerDelegate the same way.

Refactors can go in another PR or with smaller PR, not with big PR like this.

Understood! Then I'll gladly change this in a follow-up PR 👍

JanGorman · 2021-04-27T06:21:40Z

ios/ImagePickerManager.m


+- (void)onImageObtained:(UIImage*)image data:(NSData*)data


Come to think of it, we don't necessarily need to pass the UIImage here. We could

UIImage *image = [[UIImage alloc] initWithData:data];

Fine to put this into a follow-up PR as well imo

Yes ideally it should be done as you said. But in UIImagePickerController there is something called edited UIIImage and original UIImage. But the data we obtain is only from original image, you cannot get edited image data. So if the user edited the image then we need to process further using the edited UIImage and use the data from original image to just obtain image type and save it directly for GIF type. I am not sure if I am explaining properly.

This is how previous code has been working so I just used it.

I'll look into it. Should be possible to figure out the type and do the editing as you said

JanGorman · 2021-04-30T07:07:36Z

LGTM. As discussed in the PR I'll follow up with a refactoring to move protocol conformance into categories akin to class extensions in Swift

Johan-dutoit · 2021-04-30T07:08:23Z

Thanks @JanGorman, I'll do some basic testing locally and get this merged.

ios/ImagePickerManager.m

JanGorman · 2021-05-04T07:14:35Z

@Johan-dutoit Is there anything left that I can help with to merge? :)

# [3.6.0](v3.5.0...v3.6.0) (2021-05-08) ### Features * add support for PHPickerViewController ([#1654](#1654)) ([26eee5d](26eee5d))

Johan-dutoit · 2021-05-08T10:22:07Z

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [3.6.0](react-native-image-picker/react-native-image-picker@v3.5.0...v3.6.0) (2021-05-08) ### Features * add support for PHPickerViewController ([#1654](react-native-image-picker/react-native-image-picker#1654)) ([26eee5d](react-native-image-picker/react-native-image-picker@26eee5d))

gabrieldonadel · 2022-03-30T19:16:02Z

Does this support the durationLimit param?

# [3.6.0](react-native-image-picker/react-native-image-picker@v3.5.0...v3.6.0) (2021-05-08) ### Features * add support for PHPickerViewController ([#1654](react-native-image-picker/react-native-image-picker#1654)) ([0822468](react-native-image-picker/react-native-image-picker@0822468))

JanGorman added 7 commits March 22, 2021 07:14

Update ObjC syntax

7c1b650

Don't retain response

8f88b11

Don't retain picker

948daeb

More consistent formatting

29223a5

Use modern picker

d67e5e0

Select image

f400024

Select video

9ac1c89

JanGorman requested a review from Johan-dutoit as a code owner March 22, 2021 10:14

JanGorman commented Mar 22, 2021

View reviewed changes

ravirajn22 reviewed Mar 22, 2021

View reviewed changes

ios/ImagePickerManager.m Outdated Show resolved Hide resolved

ravirajn22 reviewed Mar 22, 2021

View reviewed changes

ios/ImagePickerManager.m Outdated Show resolved Hide resolved

JanGorman added 3 commits March 23, 2021 08:02

Move declarations closer to where they're used

61f8466

Factor out common image picker code

8b2adb3

Factor out common video picker code

57c2550

JanGorman commented Mar 23, 2021

View reviewed changes

ios/ImagePickerManager.m Outdated Show resolved Hide resolved

Johan-dutoit linked an issue Mar 25, 2021 that may be closed by this pull request

Support for PHPicker in iOS 14 #1428

Closed

Dismiss the picker controller right away

8c0d886

JanGorman commented Mar 25, 2021

View reviewed changes

ashwinibm mentioned this pull request Apr 21, 2021

Support for PHPicker in iOS 14 #1428

Closed

JanGorman added 2 commits April 26, 2021 09:39

Dismiss the picker controller even earlier

069afb0

Set preferredAssetRepresentationMode

ef9d14c

Refactor

cda44d7

JanGorman commented Apr 27, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main'

ff17120

Johan-dutoit reviewed Apr 30, 2021

View reviewed changes

ios/ImagePickerManager.m Outdated Show resolved Hide resolved

Update ImagePickerManager.m

37be8e3

ravirajn22 reviewed Apr 30, 2021

View reviewed changes

ios/ImagePickerManager.m Outdated Show resolved Hide resolved

Update ImagePickerManager.m

196220b

Johan-dutoit approved these changes May 8, 2021

View reviewed changes

Johan-dutoit merged commit 26eee5d into react-native-image-picker:main May 8, 2021

Johan-dutoit pushed a commit that referenced this pull request May 8, 2021

chore(release): 3.6.0 [skip ci]

86d713f

# [3.6.0](v3.5.0...v3.6.0) (2021-05-08) ### Features * add support for PHPickerViewController ([#1654](#1654)) ([26eee5d](26eee5d))

Johan-dutoit added the released label May 8, 2021

rpander93 mentioned this pull request May 8, 2021

[🌟] Multiple image selection #1695

Closed

JanGorman mentioned this pull request May 13, 2021

Re-organize iOS code into class-continuation categories #1702

Merged

Support for PHPickerViewController #1654

Support for PHPickerViewController #1654

Conversation

JanGorman commented Mar 22, 2021

Motivation

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johan-dutoit commented Mar 22, 2021

ravirajn22 commented Mar 22, 2021

JanGorman commented Mar 23, 2021

ravirajn22 commented Mar 23, 2021

Johan-dutoit commented Mar 25, 2021

JanGorman commented Mar 25, 2021

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

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

Johan-dutoit Apr 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johan-dutoit commented Mar 29, 2021

ravirajn22 commented Mar 30, 2021

Sven65 commented Apr 7, 2021

JanGorman commented Apr 7, 2021

JanGorman commented Apr 16, 2021

Johan-dutoit commented Apr 26, 2021 • edited

ravirajn22 commented Apr 26, 2021

ravirajn22 commented Apr 26, 2021

ravirajn22 commented Apr 26, 2021

JanGorman Apr 27, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanGorman commented Apr 30, 2021 • edited

Johan-dutoit commented Apr 30, 2021

JanGorman commented May 4, 2021

Johan-dutoit commented May 8, 2021

gabrieldonadel commented Mar 30, 2022

Support for `PHPickerViewController` #1654

Support for `PHPickerViewController` #1654

Johan-dutoit Apr 26, 2021 •

edited

Johan-dutoit commented Apr 26, 2021 •

edited

JanGorman Apr 27, 2021 •

edited

JanGorman commented Apr 30, 2021 •

edited