[image_picker] add requestFullMetadata for iOS (optional permissions) #4638
Conversation
Updated changelog with description of latest changes
…ch actual changes
…-permissions � Conflicts: � packages/image_picker/image_picker_platform_interface/CHANGELOG.md � packages/image_picker/image_picker_platform_interface/pubspec.yaml
| @@ -191,6 +191,13 @@ class ImagePicker { | |||
| /// the front or rear camera should be opened, this function is not guaranteed | |||
| /// to work on an Android device. | |||
| /// | |||
| /// Use `forceFullMetadata` (defaults to `true`) to control, how much additional information the plugin tries to get on iOS. | |||
There was a problem hiding this comment.
Nit: remove the comma after "control".
| @@ -191,6 +191,13 @@ class ImagePicker { | |||
| /// the front or rear camera should be opened, this function is not guaranteed | |||
| /// to work on an Android device. | |||
| /// | |||
| /// Use `forceFullMetadata` (defaults to `true`) to control, how much additional information the plugin tries to get on iOS. | |||
There was a problem hiding this comment.
Nit: This shouldn't specifically say it's for iOS, as it might apply to other platforms in the future. I would remove iOS here, and add it to the sentence below (e.g., "... which may require extra permission requests on some platforms, such as iOS").
| @@ -1,3 +1,8 @@ | |||
| ## 2.5.0 | |||
|
|
|||
| * Re-adding `forceFullMetadata` option, but as a parameter of a new `pickImageFromSource` | |||
There was a problem hiding this comment.
Re-adds
(Per style guide)
| ## 2.5.0 | ||
|
|
||
| * Re-adding `forceFullMetadata` option, but as a parameter of a new `pickImageFromSource` | ||
| method (non-breaking change approach). |
There was a problem hiding this comment.
You don't need to explain in the changelog that it's non-breaking; that's indicated by the version.
There was a problem hiding this comment.
Okay, I only wanted to explicitly show the difference from previous implementation
| @@ -270,4 +272,24 @@ class MethodChannelImagePicker extends ImagePickerPlatform { | |||
| files: pickedFileList, | |||
| ); | |||
| } | |||
|
|
|||
| @override | |||
| Future<XFile?> getImageFromSource({ | |||
There was a problem hiding this comment.
Please put this next to getImage
| @@ -146,6 +146,8 @@ abstract class ImagePickerPlatform extends PlatformInterface { | |||
| throw UnimplementedError('retrieveLostData() has not been implemented.'); | |||
| } | |||
|
|
|||
| /// This method is deprecated in favor of [getImageFromSource] and will be removed in a future update | |||
There was a problem hiding this comment.
Nit: Missing final period.
Added missing native implementation of the new flag
| @@ -97,7 +97,12 @@ - (void)pickImageWithPHPicker:(int)maxImagesAllowed API_AVAILABLE(ios(14)) { | |||
|
|
|||
| self.maxImagesAllowed = maxImagesAllowed; | |||
|
|
|||
| [self checkPhotoAuthorizationForAccessLevel]; | |||
| BOOL usePhaAsset = [[_arguments objectForKey:@"requestFullMetadata"] boolValue]; | |||
There was a problem hiding this comment.
The type is PHAsset, not PHAAsset, and also ObjC style is to capitalize abbreviations even in variable names, so this, and the similar variables below, should be usePHAsset rather than usePhaAsset.
| @@ -191,6 +191,10 @@ class ImagePicker { | |||
| /// the front or rear camera should be opened, this function is not guaranteed | |||
| /// to work on an Android device. | |||
| /// | |||
| /// Use `requestFullMetadata` (defaults to `true`) to control how much additional information the plugin tries to get on iOS. | |||
There was a problem hiding this comment.
Remove the "on iOS" here.
|
@stuartmorgan can you give me a little hint on the iOS tests? I see two expected results of having the
For now I don't see a way to check if any of those happened in the iOS unit tests. |
Those aren't expected results, they are internal implementation details. Expected results here would be expressed in terms of the iOS APIs that are or are not called, which would be checked by mocking them. |
…ermissions � Conflicts: � packages/image_picker/image_picker/CHANGELOG.md � packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m � packages/image_picker/image_picker/pubspec.yaml
| @override | ||
| Future<XFile?> getImageFromSource({ | ||
| required ImageSource source, | ||
| ImagePickerOptions? options, |
There was a problem hiding this comment.
Can option parameter be non-null and defaults to default options? (const ImagePickerOptions())
| /// If no images were picked, the return value is null. | ||
| Future<XFile?> getImageFromSource({ | ||
| required ImageSource source, | ||
| ImagePickerOptions? options, |
There was a problem hiding this comment.
ditto: can we make options non-null?
| /// | ||
| /// If no images were picked, the return value is null. | ||
| Future<XFile?> getImageFromSource({ | ||
| required ImageSource source, |
There was a problem hiding this comment.
What is the reason behind making source not a part of ImagePickerOptions? Can we also move source to ImagePickerOptions?
There was a problem hiding this comment.
This parameter is needed whenever the picking method is called. All parameters inside ImagePickerOptions are more optional, so the whole options object can be skipped if a caller doesn't need any specific adjustments. It was @stuartmorgan's suggestion in this PR that I've agreed with, as calling the picking method doesn't require creating the options object for every use
There was a problem hiding this comment.
LGTM, @stuartmorgan Do you mind taking another look before we update the path dependency?
There was a problem hiding this comment.
I have some documentation feedback and minor nits, but otherwise this looks good. @PiotrMitkowski please go ahead and open the platform interface PR (with the platform interface parts of my comments below addressed) and we can get that a quick final review and land it to start unwinding the full PR.
| this.requestFullMetadata = true, | ||
| }); | ||
|
|
||
| /// If specified, the image will be at most `maxWidth` wide. Otherwise, |
There was a problem hiding this comment.
You don't need to include the name of a field in the field comment name (as you do in the context of a method comment, where this was taken from). Also, the second sentence isn't true once you split it apart into two different comments for the two fields, because if one maximum is specified, it'll be scaled to fit that max which changes the other dimension. So it should be:
The maximum width of the image, in pixels. If
null, the image will only be resized ifmaxHeightis specified.
| final double? maxWidth; | ||
|
|
||
| /// If specified, the image will be at most`maxHeight` tall. | ||
| /// Otherwise the image will be returned at its original height. |
| final double? maxHeight; | ||
|
|
||
| /// Modifies the quality of the image, ranging from 0-100 where 100 is the original/max | ||
| /// quality. If `imageQuality` is null, the image with the originalquality will |
There was a problem hiding this comment.
"If null, the image will be returned at its original quality."
There was a problem hiding this comment.
Also, lets put a blank comment line after the first sentence here, per https://dart.dev/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph
| /// Specifies options for picking a single image from the device's camera or gallery. | ||
| class ImagePickerOptions { | ||
| /// Creates an instance with the given [maxHeight], [maxWidth], [imageQuality], | ||
| /// [referredCameraDevice] and [requestFullMetadata]. Any of the params may be null. |
There was a problem hiding this comment.
Remove "Any of the params may be null"; nullability is part of types now, and doesn't need to be commented.
| /// The `preferredCameraDevice` is ignored when `source` is [ImageSource.gallery]. | ||
| /// It is also ignored if the chosen camera is not supported on the device. | ||
| /// Defaults to [CameraDevice.rear]. Note that Android has no documented parameter | ||
| /// for an intent to specify if the front or rear camera should be opened, this |
There was a problem hiding this comment.
We try to avoid commenting about specific platforms in the platform interface (other than as illustrative examples). You can just remove this whole sentence since it's a minor detail that callers at the platform interface level aren't going to need to know.
| final int? imageQuality; | ||
|
|
||
| /// Used to specify the camera to use when the `source` is [ImageSource.camera]. | ||
| /// The `preferredCameraDevice` is ignored when `source` is [ImageSource.gallery]. |
There was a problem hiding this comment.
Remove this sentence, and add a blank line.
|
|
||
| /// Used to specify the camera to use when the `source` is [ImageSource.camera]. | ||
| /// The `preferredCameraDevice` is ignored when `source` is [ImageSource.gallery]. | ||
| /// It is also ignored if the chosen camera is not supported on the device. |
There was a problem hiding this comment.
"Ignored if the source is not [ImageSource.camera], or the chosen camera is not supported on the device."
| /// function is not guaranteed to work on an Android device. | ||
| final CameraDevice preferredCameraDevice; | ||
|
|
||
| /// `requestFullMetadata` defaults to `true`, so the plugin tries to get the |
There was a problem hiding this comment.
A field comment should lead with what it's for, not what the default is. How about:
If true, requests full image metadata, which may require extra permissions on some platforms, (e.g.,
NSPhotoLibraryUsageDescriptionon iOS).Defaults to true.
| @ObjCSelector('pickImageWithSource:maxSize:quality:') | ||
| String? pickImage( | ||
| SourceSpecification source, MaxSize maxSize, int? imageQuality); | ||
| @ObjCSelector('pickImageWithSource:maxSize:quality:requestFullMetadata:') |
There was a problem hiding this comment.
Nit: this param would be more idiomatic in ObjC as just fullMetadata:; the "with" is implied to go with following arguments, and "with full metadata" reads much better than "with request full metadata".
…ermissions � Conflicts: � packages/image_picker/image_picker/CHANGELOG.md � packages/image_picker/image_picker/pubspec.yaml � packages/image_picker/image_picker_ios/CHANGELOG.md � packages/image_picker/image_picker_ios/pubspec.yaml � packages/image_picker/image_picker_platform_interface/CHANGELOG.md � packages/image_picker/image_picker_platform_interface/lib/src/platform_interface/image_picker_platform.dart � packages/image_picker/image_picker_platform_interface/lib/src/types/image_picker_options.dart � packages/image_picker/image_picker_platform_interface/test/new_method_channel_image_picker_test.dart
| }) { | ||
| return platform.getImage( | ||
| return platform.getImageFromSource( |
There was a problem hiding this comment.
You need to require the version of the platform interface that has this method in pubspec.yaml.
| maxHeight: maxHeight, | ||
| imageQuality: imageQuality, | ||
| preferredCameraDevice: preferredCameraDevice, | ||
| requestFullMetadata: requestFullMetadata, |
There was a problem hiding this comment.
It would also be better to pull the iOS changes into a separate PR and land it first so that you can require that version, otherwise someone who only updates this package will not actually get a working implementation of the new parameter.
| @@ -57,14 +57,16 @@ void main() { | |||
| 'maxWidth': null, | |||
| 'maxHeight': null, | |||
| 'imageQuality': null, | |||
| 'cameraDevice': 0 | |||
| 'cameraDevice': 0, | |||
| 'requestFullMetadata': true, | |||
There was a problem hiding this comment.
You'll need to reconcile the test changes against #5706 once that lands (which is needed to resolve the current tree breakage, so will be before this PR can land).
| @@ -2,7 +2,7 @@ name: image_picker_ios | |||
| description: iOS implementation of the video_picker plugin. | |||
| repository: https://github.com/flutter/plugins/tree/main/packages/image_picker/image_picker_ios | |||
| issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+image_picker%22 | |||
| version: 0.8.5+2 | |||
| version: 0.8.6 | |||
|
|
|||
There was a problem hiding this comment.
This also needs to require the right version of the platform interface.
…ermissions � Conflicts: � packages/image_picker/image_picker/CHANGELOG.md � packages/image_picker/image_picker/lib/image_picker.dart � packages/image_picker/image_picker/pubspec.yaml � packages/image_picker/image_picker/test/image_picker_deprecated_test.dart � packages/image_picker/image_picker/test/image_picker_test.dart
|
I closed this PR by a mistake today, so I've created a new one: #5915 |
Description
Originally #3264 made by @cpboyd
Make iOS 11+ permissions requests optional per https://developer.apple.com/forums/thread/653414.
PHAssets aren't actually required and there was already some fallback code in place.
Related issues
Fixes flutter/flutter#65995
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).