-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(ios): use PHPickerViewController for iOS 14+
#937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…S 14+ - Does not need any permissions for reading images - The PHPickerViewController class is an alternative to UIImagePickerController. PHPickerViewController improves stability and reliability, and includes several benefits to developers and users, such as the following: - Deferred image loading and recovery UI - Reliable handling of large and complex assets, like RAW and panoramic images - User-selectable assets that aren’t available for UIImagePickerController - Configuration of the picker to display only Live Photos - Availability of PHLivePhoto objects without library access - Stricter validations against invalid inputs - See documentation of PHPickerViewController: https://developer.apple.com/documentation/photosui/phpickerviewcontroller?language=objc - Added tests for PHPickerViewController in `CameraTest.m`
|
@dpogue Can you review this? |
PHPickerViewController for iOS 14+- It was wrongly checked against the availability of PhotosUI, which is already available since iOS 8, but PHPickerViewControllerDelegate is only available since iOS 14 - Removed all `#if __has_include(<PhotosUI/PhotosUI.h>)` which is not necessary - Added a missing `if (@available(iOS 14, *)) {` when calling [self showPHPicker:withOptions]
PHPickerViewController for iOS 14+
PHPickerViewController for iOS 14+PHPickerViewController for iOS 14+
dpogue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly okay to me, with the caveat that I know nothing about any of these iOS APIs or how to use them properly.
Speaking personally, I'm not a fan of the camera plugin and I wish people would just use <input type="file" accept="image/*" capture> and make the browser handle it instead of us.
- Removed unnecessary import `PhotosUI.h`. This already imported via `CDVCamera.h`. - Removed `if (@available(iOS 14, *))` checks. The methods are already declared with `API_AVAILABLE(ios(14))` which is sufficient
- This separates the PHPickerViewControllerDelegate methods with a line when viewing the methods list in XCode
- A pragmas for UIImagePickerControllerDelegate methods and CLLocationManager methods - Format some long methods declarations to multi-line instead single-line for better readability - Format code in [navigationController:willShowViewController:animated:] to be better aligned and rename variable `cameraPicker` to `imagePicker`
- Make some long method declarations multi-line instead single-line
You are right, there are already HTML specifications to access the camera and photo library. |
|
I made changes which could be reviewed again |
|
This plugin says it supports cordova-ios >=5.1.0 which means it needs to support building with Xcode 10, which means the code needs to compile even when the
|
Thanks about the clarification, I was not aware about this. The plugin requires now minimum cordova-ios 7.0.0. Is this sufficient to use |
cordova-ios 7.x supports Xcode 11 as a minimum, which ships with the iOS 13 SDK. So you'll still need those |
|
Ok, and |
No, you still need that, because you can be building with a newer SDK but supporting older OS versions. cordova-ios 7.x supports as far back as iOS 11.0, cordova-ios 8.x supports as far back as iOS 13.0. |
|
Thank you, now I understand it better :) |
- The methods are not private and declared in CDVCamera.h
- The method is not related to PHPicker tests
- On XCode 11 the iOS SDK 14 is not available and compiling would fail
|
@dpogue I created some test methods with Copilot in |
If the tests are related to the code being changed here, I think it's fine to include them. Make sure that any AI-generated code is compliant with the ASF Generative Tooling policy and includes the |
| #import <PhotosUI/PhotosUI.h> | ||
|
|
||
| // Define a macro to indicate that iOS SDK 14+ is available for PHPicker related things | ||
| #define IOS_SDK_14_AVAILABLE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a publicly visible define in the header file is essentially expanding the public API of the plugin. People will start using this outside of this plugin when they shouldn't, and if we ever want to clean up code by removing this it will be a breaking change.
I know it's more verbose, but it's probably better in the long run to just use the __IPHONE_OS_VERSION_MAX_ALLOWED >= 140000 check everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok understand, I will change this
I didn't know that. I will read the Generative Tooling policy. |
CameraTest.mPlatforms affected
iOS
Motivation and Context
Description
Testing
Checklist
(platform)if this change only applies to one platform (e.g.(android))