Skip to content

Conversation

@GitToTheHub
Copy link
Contributor

@GitToTheHub GitToTheHub commented Dec 3, 2025

  • Apple recommends to use PHPickerViewController to select images, which also does not need any permissions for reading images:
  • May resolve issues when selecting images, especially on the iPhone SE
  • The PHPickerViewController 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
  • Generated-by: Visual Studio Code, Copilot, GPT -5

Platforms affected

iOS

Motivation and Context

Description

Testing

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

…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`
@GitToTheHub
Copy link
Contributor Author

@dpogue Can you review this?

@GitToTheHub GitToTheHub requested a review from dpogue December 4, 2025 00:01
@GitToTheHub GitToTheHub changed the title feat(ios): integrate PHPickerViewController for iOS 14+ feat(ios): use PhotosUI for iOS 14+ Dec 4, 2025
- 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]
@GitToTheHub GitToTheHub changed the title feat(ios): use PhotosUI for iOS 14+ feat(ios): use PHPickerViewController for iOS 14+ Dec 4, 2025
@GitToTheHub GitToTheHub changed the title feat(ios): use PHPickerViewController for iOS 14+ feat(ios): use PHPickerViewController for iOS 14+ Dec 4, 2025
Copy link
Member

@dpogue dpogue left a 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
@GitToTheHub
Copy link
Contributor Author

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.

You are right, there are already HTML specifications to access the camera and photo library. <input type="file" accept="image/png, image/jpeg" /> would give the possibility to access the photo library. Maybe this should be added to the documentation. Is this plugin redundant, when there are already specifications to access these things? I mean, it would be good, if we could maintain less.

@GitToTheHub GitToTheHub requested a review from dpogue December 6, 2025 15:16
@GitToTheHub
Copy link
Contributor Author

I made changes which could be reviewed again

@dpogue
Copy link
Member

dpogue commented Dec 6, 2025

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 PHPickerViewController classes do not exist in the SDK. That's why everything that uses it needs to be wrapped in a #if __IPHONE_OS_VERSION_MAX_ALLOWED >= 140000 check.

API_AVAILABLE(ios(14)) is sufficient for a newer SDK targeting an older device, but not for an older SDK.

@GitToTheHub
Copy link
Contributor Author

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 PHPickerViewController classes do not exist in the SDK. That's why everything that uses it needs to be wrapped in a #if __IPHONE_OS_VERSION_MAX_ALLOWED >= 140000 check.

API_AVAILABLE(ios(14)) is sufficient for a newer SDK targeting an older device, but not for an older SDK.

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 API_AVAILABLE(ios(14))? How do I know, when I can use API_AVAILABLE(ios(14))?

@dpogue
Copy link
Member

dpogue commented Dec 8, 2025

The plugin requires now minimum cordova-ios 7.0.0. Is this sufficient to use API_AVAILABLE(ios(14))?

cordova-ios 7.x supports Xcode 11 as a minimum, which ships with the iOS 13 SDK. So you'll still need those __IPHONE_OS_VERSION_MAX_ALLOWED checks.

@GitToTheHub
Copy link
Contributor Author

Ok, and API_AVAILABLE(ios(14)) can be removed? Because otherwise it would be double guarded or?

@dpogue
Copy link
Member

dpogue commented Dec 8, 2025

Ok, and API_AVAILABLE(ios(14)) can be removed? Because otherwise it would be double guarded or?

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.

@GitToTheHub
Copy link
Contributor Author

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
@GitToTheHub
Copy link
Contributor Author

@dpogue I created some test methods with Copilot in CameraTest.m. Do you think they are worth to keep or should we just concentrate on implementing PHPickerViewController in CDVCamera.m? I don't have any experience with the extra CDVCameraTest project.

@dpogue
Copy link
Member

dpogue commented Dec 17, 2025

@dpogue I created some test methods with Copilot in CameraTest.m. Do you think they are worth to keep or should we just concentrate on implementing PHPickerViewController in CDVCamera.m? I don't have any experience with the extra CDVCameraTest project.

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 Generated-by: trailer in the commit message.

@GitToTheHub
Copy link
Contributor Author

Make sure that any AI-generated code is compliant with the ASF Generative Tooling policy and includes the Generated-by: trailer in the commit message.

I didn't know that. I will read the Generative Tooling policy.

@GitToTheHub
Copy link
Contributor Author

GitToTheHub commented Dec 29, 2025

I read the ASF Generative Tooling policy and will add the Generated-by: trailer to the commit message. Is this ready to merge?

EDIT: Sorry, I didn't see that IOS_SDK_14_AVAILABLE has to be removed still.

- 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.
@GitToTheHub
Copy link
Contributor Author

@dpogue This is ready to merge

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the API for the actual PHPickerViewController stuff, but this looks okay to me as far as structure goes

- Document `takePicture` and `showCameraPicker`
- Remove documentation for `takePicture` in header file. The documentation is done in the `.m` file
- Remove unnecessry `dispatch_async(dispatch_get_main_queue() ...` in `takePicture` before calling `showCameraPicker`. This is already done in `showCameraPicker`.
- Source out code for permission denied alert dialog when accessing the camera or UIImagePickerController on iOS < 14 for picking images
- Better docuent usage descriptions
- `NSPhotoLibraryUsageDescription` not needed for iOS 14+ when only picking images
- Improve formatting for xml, js
- sourceType `SAVEDPHOTOALBUM` is the same as `PHOTOLIBRARY` on Android and iOS 14+
- Use `PHOTOLIBRARY` as sourceType instead of `SAVEDPHOTOALBUM` in  photo picker example
- Make clear that `SAVEDPHOTOALBUM` is the same like `PHOTOLIBRARY` and has only an effect on iOS < 14
- Format code when creating image chooser and document the request code parameter
@GitToTheHub
Copy link
Contributor Author

@dpogue Thanks for reviewing this. There were some changes still necessary i noticed later:

  • sourceType SAVEDPHOTOALBUM opens also the PHPicker now
  • Documentation of takePicture and showCameraPicker in CDVCamera.m
  • Removed documentation for takePicture in header file. The documentation is done in the .m file
  • Removed unnecessry dispatch_async(dispatch_get_main_queue() ... in takePicture before calling showCameraPicker. This is already done in showCameraPicker.
  • Source out code for permission denied alert dialog when accessing the camera or UIImagePickerController on iOS < 14 for picking images
  • I updated the readme
    • Better document usage descriptions
    • NSPhotoLibraryUsageDescription not needed for iOS 14+ when only picking images
    • Improve formatting for xml, js
    • sourceType SAVEDPHOTOALBUM is the same as PHOTOLIBRARY on Android and iOS 14+
    • Use PHOTOLIBRARY as sourceType instead of SAVEDPHOTOALBUM in photo picker example
  • I made subtle changes on the Android side in CameraLauncher.java
    • Make clear that SAVEDPHOTOALBUM is the same like PHOTOLIBRARY and has only an effect on iOS < 14
    • Format code when creating image chooser and document the request code parameter

@GitToTheHub GitToTheHub requested a review from dpogue January 5, 2026 08:56
@GitToTheHub
Copy link
Contributor Author

@dpogue I would merge this. Is this ok?

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks okay, but again I have no experience with the camera/picker APIs so I can't spot any issues in terms of how it's being used or whether we're calling the right methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants