-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[image_picker] Image picker phpicker impl #3835
[image_picker] Image picker phpicker impl #3835
Conversation
I have to implement PHPicker to select photos, live photos and videos from the photo library for iOS 14 and higher versions. But I also have to keep old UIImagePickerController for lower iOS versions and to use camera.
I moved the UIImagePickerController implementation into pickImageWithUIImagePicker method for code reusability.
I refactored the checkPhotoAuthorization function to add limited access functionality. Also I implemented showLimitedPhotoLibrary function to call presentLimitedLibraryPickerFromViewController.
I implemented picker function that is came from PHPickerViewControllerDelegate.
I added PHPhotoLibraryPreventAutomaticLimitedAccessAlert = YES into Info.plist to use limited access functionality.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
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.
Thanks for the PR! This is great and it's going to solve lots issues in the plugin! I left some comments :)
packages/image_picker/image_picker/example/ios/Runner/Info.plist
Outdated
Show resolved
Hide resolved
@@ -165,7 +165,7 @@ - (void)launchPickerAndPick { | |||
// Find an image and tap on it. (IOS 14 UI, images are showing directly) | |||
XCUIElement* aImage; | |||
if (@available(iOS 14, *)) { | |||
aImage = self.app.scrollViews.firstMatch.images.firstMatch; | |||
aImage = [self.app.scrollViews.firstMatch.images elementBoundByIndex: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.
Why do we need to select the 2nd element instead of 2? Is this a critical change or just that you think testing the 2nd image is better?
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.
This is because there is a known issue to pick HEIC images with PHPicker implementation. But only for simulators. So we have two options here. Either test with real device or test with non-HEIC images until Apple solves the issue.
...s/image_picker/image_picker/example/ios/RunnerUITests/ImagePickerFromLimitedGalleryUITests.m
Outdated
Show resolved
Hide resolved
__weak typeof(self) weakSelf = self; | ||
[self addUIInterruptionMonitorWithDescription:@"Permission popups" | ||
handler:^BOOL(XCUIElement* _Nonnull interruptingElement) { | ||
if (@available(iOS 14, *)) { |
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.
I think limited photos is only availabe in iOS 14? So maybe we should skip this test for ios 14 and below? This way we don't need to maintain 2 versions for this test.
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.
I am not sure how to do this. Using API_AVAILABLE(ios(14))
on the top of the test class maybe?
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.
Sometime like this?:
if (!(@available(iOS 14, *))) {
return;
}
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.
I used XCTSkip, I think it is done!
packages/image_picker/image_picker/ios/Classes/FLTImagePickerPhotoAssetUtil.m
Outdated
Show resolved
Hide resolved
packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m
Outdated
Show resolved
Hide resolved
_imagePickerController.delegate = self; | ||
_imagePickerController.mediaTypes = @[ (NSString *)kUTTypeImage ]; | ||
|
||
_phPickerFlag = NO; |
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.
Do we really need this state flag? It seems like we can pass this flag along.
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.
I think we need this state flag. Because even the iOS version is 14 or higher we need to use the old UIImagePickerController
when the user wants to use the camera. So I created this flag to make sure PHPicker will be used only if the user pick photo from the photo library and the iOS version is 14 or higher.
NSError *_Nullable error) { | ||
if ([image isKindOfClass:[UIImage class]]) { | ||
if (image != nil) { | ||
NSNumber *maxWidth = [self->_arguments objectForKey:@"maxWidth"]; |
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.
While you are here, could you refactor _arguments into a property then use weak self. :)
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.
Done!
if (image != nil) { | ||
NSNumber *maxWidth = [self->_arguments objectForKey:@"maxWidth"]; | ||
NSNumber *maxHeight = [self->_arguments objectForKey:@"maxHeight"]; | ||
NSNumber *imageQuality = [self->_arguments objectForKey:@"imageQuality"]; | ||
|
||
if (![imageQuality isKindOfClass:[NSNumber class]]) { | ||
imageQuality = @1; | ||
} else if (imageQuality.intValue < 0 || imageQuality.intValue > 100) { | ||
imageQuality = [NSNumber numberWithInt:1]; | ||
} else { | ||
imageQuality = @([imageQuality floatValue] / 100); | ||
} | ||
|
||
if (maxWidth != (id)[NSNull null] || maxHeight != (id)[NSNull null]) { | ||
image = [FLTImagePickerImageUtil scaledImage:image | ||
maxWidth:maxWidth | ||
maxHeight:maxHeight]; | ||
} | ||
|
||
PHAsset *originalAsset = | ||
[FLTImagePickerPhotoAssetUtil getAssetFromPHPickerResult:result]; | ||
|
||
if (!originalAsset) { | ||
// Image picked without an original asset (e.g. User took a photo directly) | ||
[self saveImageWithPickerInfo:nil image:image imageQuality:imageQuality]; | ||
} else { | ||
__weak typeof(self) weakSelf = self; | ||
[[PHImageManager defaultManager] | ||
requestImageDataForAsset:originalAsset | ||
options:nil | ||
resultHandler:^( | ||
NSData *_Nullable imageData, NSString *_Nullable dataUTI, | ||
UIImageOrientation orientation, NSDictionary *_Nullable info) { | ||
// maxWidth and maxHeight are used only for GIF images. | ||
[weakSelf saveImageWithOriginalImageData:imageData | ||
image:image | ||
maxWidth:maxWidth | ||
maxHeight:maxHeight | ||
imageQuality:imageQuality]; | ||
}]; | ||
} | ||
} | ||
} | ||
}]; |
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.
Is this getting the image logic the same as https://github.com/flutter/plugins/blob/master/packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m#L294-L334?
Maybe we can refactor them out to a method and reuse it?
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.
Yes, almost the same. Only important difference is originalAsset
which calls different methods. Do you think we should refactor still?
If the user enabled limited library access, use presentLimitedLibraryPickerFromViewController to present the limited library picker so they may update their selection. But in this case, limited access status will not return with old requestAuthorization implementation in checkPhotoAuthorization method. So this method will not be used.
|
||
@property(copy, nonatomic) FlutterResult result; | ||
|
||
@property(copy, nonatomic) NSDictionary *arguments; | ||
|
||
@property(copy, nonatomic) PHPickerViewController *pickerViewController API_AVAILABLE(ios(14)); |
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.
@property(copy, nonatomic) PHPickerViewController *pickerViewController API_AVAILABLE(ios(14)); | |
@property(strong, nonatomic) PHPickerViewController *pickerViewController API_AVAILABLE(ios(14)); |
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.
done.
@@ -246,12 +290,83 @@ - (void)errorNoPhotoAccess:(PHAuthorizationStatus)status { | |||
} | |||
} | |||
|
|||
- (void)showPhotoLibrary { | |||
- (void)showPhotoLibrary:(int)imagePickerClassType { |
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.
- (void)showPhotoLibrary:(int)imagePickerClassType { | |
- (void)showPhotoLibrary:(ImagePickerClassType)imagePickerClassType { |
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.
done.
[[self viewControllerWithWindow:nil] presentViewController:_imagePickerController | ||
animated:YES | ||
completion:nil]; | ||
if (imagePickerClassType) { |
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.
Use switch
for enums
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.
done.
NSNumber *maxWidth = [self->_arguments objectForKey:@"maxWidth"]; | ||
NSNumber *maxHeight = [self->_arguments objectForKey:@"maxHeight"]; | ||
NSNumber *imageQuality = [self->_arguments objectForKey:@"imageQuality"]; |
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.
NSNumber *maxWidth = [self->_arguments objectForKey:@"maxWidth"]; | |
NSNumber *maxHeight = [self->_arguments objectForKey:@"maxHeight"]; | |
NSNumber *imageQuality = [self->_arguments objectForKey:@"imageQuality"]; | |
NSNumber *maxWidth = [self.arguments objectForKey:@"maxWidth"]; | |
NSNumber *maxHeight = [self.arguments objectForKey:@"maxHeight"]; | |
NSNumber *imageQuality = [self.arguments objectForKey:@"imageQuality"]; |
or
NSNumber *maxWidth = [self->_arguments objectForKey:@"maxWidth"]; | |
NSNumber *maxHeight = [self->_arguments objectForKey:@"maxHeight"]; | |
NSNumber *imageQuality = [self->_arguments objectForKey:@"imageQuality"]; | |
NSNumber *maxWidth = [_arguments objectForKey:@"maxWidth"]; | |
NSNumber *maxHeight = [_arguments objectForKey:@"maxHeight"]; | |
NSNumber *imageQuality = [_arguments objectForKey:@"imageQuality"]; |
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.
done.
I implemented requestAuthorizationForAccessLevel which is the new way to handle the photo library authorization with iOS 14+.
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.
lgtm
Merging this PR while "Windows Plugins" step is red due to infrastructure problems. This PR only contains iOS related code and therefore can be safely merged as mentioned in this comment on Discord. |
if ([image isKindOfClass:[UIImage class]]) { | ||
if (image != nil) { |
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.
Shouldn't the nil
check occur before the isKindOfClass
?
I've encountered a case where the iOS Simulator is returning nil
with a default image and the image_picker is just silently failing.
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.
Good catch, we actually don't need this nil check since [image isKindOfClass:[UIImage class]]
returns false if image is nil.
Looking at our test targets to make sure some tooling changes I'm planning will work with our repo, I came across Apologies if this is in the discussion somewhere above, there was a lot to try to sift through. |
* Add PHPicker libraries and its delegate I have to implement PHPicker to select photos, live photos and videos from the photo library for iOS 14 and higher versions. But I also have to keep old UIImagePickerController for lower iOS versions and to use camera. * Add PHPicker implementation * Refactor UIImagePickerController implementation I moved the UIImagePickerController implementation into pickImageWithUIImagePicker method for code reusability. * Refactor handleMethodCall function * Refactor showPhotoLibrary function * Add the limited access into the photo library I refactored the checkPhotoAuthorization function to add limited access functionality. Also I implemented showLimitedPhotoLibrary function to call presentLimitedLibraryPickerFromViewController. * Add the implementation of picker function I implemented picker function that is came from PHPickerViewControllerDelegate. * Refactor Info.plist file I added PHPhotoLibraryPreventAutomaticLimitedAccessAlert = YES into Info.plist to use limited access functionality. * Add the function to get PHAssetResult * Change the image to bypass HEIC problem * Add UITest to test select photo functionality * Add unit test for PHAssetResult * Format the codes in all files * Fix the unit test * Update the version * Add the description into CHANGELOG file * Fix the license header * Remove the unnecessary if statement * Refactor PHPickerViewController to use property * Refactor phPickerFlag state to use property * Refactor the code to combine lines * Refactor arguments to use property * Revert formatting changes This reverts part of commit 36b6a2a. * Fix the unit test * Format the code * Refactor the UITest to skip lower versions * Fix the property's name * Refactor the properties to pass them via methods * Add the getDesiredImageQuality method * Add API_AVAILABLE for limited access method * Refactor PHPickerController to use as a property * Refactor PHPicker picker method and UITest * Refactor the UITest Move to UITest to new target file to test on iOS 14 and higher versions. * Change the team to None in the RunnerUITestiOS14 * Fix the UITest * Refactor the method to fix dispatch * Change to use self instead of weakSelf * Fix the UITest to use XCTSkip * Add ImagePickerClassType to use enum * Remove unused method If the user enabled limited library access, use presentLimitedLibraryPickerFromViewController to present the limited library picker so they may update their selection. But in this case, limited access status will not return with old requestAuthorization implementation in checkPhotoAuthorization method. So this method will not be used. * Fix property to change copy to strong * Refactor enum * Change argument call * Add checkPhotoAuthorizationForAccessLevel method I implemented requestAuthorizationForAccessLevel which is the new way to handle the photo library authorization with iOS 14+.
* Add PHPicker libraries and its delegate I have to implement PHPicker to select photos, live photos and videos from the photo library for iOS 14 and higher versions. But I also have to keep old UIImagePickerController for lower iOS versions and to use camera. * Add PHPicker implementation * Refactor UIImagePickerController implementation I moved the UIImagePickerController implementation into pickImageWithUIImagePicker method for code reusability. * Refactor handleMethodCall function * Refactor showPhotoLibrary function * Add the limited access into the photo library I refactored the checkPhotoAuthorization function to add limited access functionality. Also I implemented showLimitedPhotoLibrary function to call presentLimitedLibraryPickerFromViewController. * Add the implementation of picker function I implemented picker function that is came from PHPickerViewControllerDelegate. * Refactor Info.plist file I added PHPhotoLibraryPreventAutomaticLimitedAccessAlert = YES into Info.plist to use limited access functionality. * Add the function to get PHAssetResult * Change the image to bypass HEIC problem * Add UITest to test select photo functionality * Add unit test for PHAssetResult * Format the codes in all files * Fix the unit test * Update the version * Add the description into CHANGELOG file * Fix the license header * Remove the unnecessary if statement * Refactor PHPickerViewController to use property * Refactor phPickerFlag state to use property * Refactor the code to combine lines * Refactor arguments to use property * Revert formatting changes This reverts part of commit 36b6a2a. * Fix the unit test * Format the code * Refactor the UITest to skip lower versions * Fix the property's name * Refactor the properties to pass them via methods * Add the getDesiredImageQuality method * Add API_AVAILABLE for limited access method * Refactor PHPickerController to use as a property * Refactor PHPicker picker method and UITest * Refactor the UITest Move to UITest to new target file to test on iOS 14 and higher versions. * Change the team to None in the RunnerUITestiOS14 * Fix the UITest * Refactor the method to fix dispatch * Change to use self instead of weakSelf * Fix the UITest to use XCTSkip * Add ImagePickerClassType to use enum * Remove unused method If the user enabled limited library access, use presentLimitedLibraryPickerFromViewController to present the limited library picker so they may update their selection. But in this case, limited access status will not return with old requestAuthorization implementation in checkPhotoAuthorization method. So this method will not be used. * Fix property to change copy to strong * Refactor enum * Change argument call * Add checkPhotoAuthorizationForAccessLevel method I implemented requestAuthorizationForAccessLevel which is the new way to handle the photo library authorization with iOS 14+. # Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/example/ios/Runner.xcodeproj/project.pbxproj # packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m # packages/image_picker/image_picker/pubspec.yaml
This PR adds the
PHPicker
implementation.PHPicker
is the new approach to pick image from the photo library for iOS 14 and higher versions. But we still need to keepUIImagePickerController
to use camera (PHPicker
does not support) and for the lower iOS versions.PHPicker
implementation fixes the rotation problem which is explaining in the below issue. The issue is based on "Select Photos" option that gives the limited access to the photo library. WithPHPicker
implementation the limited access is also handled and to getPHAsset
will not fail anymore.Related issues:
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format
. See plugin_tool format)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.