-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[image_picker] Add limit parameter to pickMultiImage and pickMultipleMedia to ios and Android #6201
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
Conversation
2470dcd
to
3b38ad7
Compare
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.
Seems like a solid change once the method deprecation is changed. Thanks for putting this together, and let me know if you need any help/guidance with anything!
This isn't a full review, I'll double back around once these changes are made.
...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java
Outdated
Show resolved
Hide resolved
...e_picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerPlugin.java
Outdated
Show resolved
Hide resolved
packages/image_picker/image_picker_android/lib/image_picker_android.dart
Outdated
Show resolved
Hide resolved
...picker/image_picker_platform_interface/lib/src/platform_interface/image_picker_platform.dart
Outdated
Show resolved
Hide resolved
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 that this PR could be much simplified if it were: "add a new parameter to MultiImagePickerOptions
, and then use it in iOS and Android". For example, the web package wouldn't need any changes, because it doesn't care about the new parameter of that object at all.
There's a lot of extra code because this is updating a bunch of deprecated methods (as @tarrinneal mentioned in his review above!), which... we might be even able to delete...
...picker/image_picker_platform_interface/lib/src/platform_interface/image_picker_platform.dart
Outdated
Show resolved
Hide resolved
packages/image_picker/image_picker_platform_interface/lib/src/types/media_options.dart
Outdated
Show resolved
Hide resolved
...cker/image_picker_platform_interface/lib/src/method_channel/method_channel_image_picker.dart
Outdated
Show resolved
Hide resolved
I've cleaned up packages which remain unchanged. |
Thanks for reverting the I checked the web APIs again and there's nothing new to We could count how many they've selected and throw an exception, or discard extras if they've picked too many, but that's probably confusing, and can be implemented on the application layer too. |
Yeah, I feel the same way. If we can't limit choice on the UI side, then it's better to do nothing. |
@tarrinneal do you mind look at this again? |
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.
The iOS side looks good, however, the usage of pickMultiImageWithMaxSize
and makeWithMaxSize
within image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m
will need to be updated to use new parameter.
Also, additional tests should be added to test the new limit
parameter correctly sets the maxImageCount
. I've included an example you can use below. You should also add ones where the limit is nil.
Test example
- (void)testPickMultiImageWithLimit {
FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init];
[plugin pickMultiImageWithMaxSize:[[FLTMaxSize alloc] init]
quality:nil
fullMetadata:NO
limit:@(2)
completion:^(NSArray<NSString *> *_Nullable result,
FlutterError *_Nullable error){
}];
XCTAssertEqual(plugin.callContext.maxImageCount, 2);
}
- (void)testPickMediaWithLimitAllowsMultiple {
FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init];
FLTMediaSelectionOptions *mediaSelectionOptions =
[FLTMediaSelectionOptions makeWithMaxSize:[FLTMaxSize makeWithWidth:@(100) height:@(200)]
imageQuality:nil
requestFullMetadata:NO
allowMultiple:YES
limit:@(2)];
[plugin pickMediaWithMediaSelectionOptions:mediaSelectionOptions
completion:^(NSArray<NSString *> *_Nullable result,
FlutterError *_Nullable error){
}];
XCTAssertEqual(plugin.callContext.maxImageCount, 2);
}
- (void)testPickMediaWithLimitMultipleNotAllowed {
FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init];
FLTMediaSelectionOptions *mediaSelectionOptions =
[FLTMediaSelectionOptions makeWithMaxSize:[FLTMaxSize makeWithWidth:@(100) height:@(200)]
imageQuality:nil
requestFullMetadata:NO
allowMultiple:NO
limit:@(2)];
[plugin pickMediaWithMediaSelectionOptions:mediaSelectionOptions
completion:^(NSArray<NSString *> *_Nullable result,
FlutterError *_Nullable error){
}];
XCTAssertEqual(plugin.callContext.maxImageCount, 1);
}
@vashworth thanks for the test examples. I've also added tests for variants where the limit is 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.
The test Linux repo_checks is failing on formatting issues:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8754354673776286193/+/u/Run_package_tests/format/stdout
Looks like a number of files need to be formatted: https://github.com/flutter/packages/blob/main/script/tool/README.md#format-code
2a4b62b
to
3f7062e
Compare
@tarrinneal may you look at this soon? |
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 putting in the time and effort on this pr.
(and sorry for the delays in approval)
…MultipleMedia to ios and Android (flutter/packages#6201)
flutter/packages@90c876d...d39830e 2024-04-16 [email protected] Manual roll Flutter from 2e748e8 to 3882afb (16 revisions) (flutter/packages#6549) 2024-04-16 [email protected] [image_picker] Add limit parameter to pickMultiImage and pickMultipleMedia to ios and Android (flutter/packages#6201) 2024-04-16 [email protected] [camera] Remove iOS thread-safe result class (flutter/packages#6498) 2024-04-16 [email protected] [google_maps_flutter] update ios privacy manifest (flutter/packages#6511) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
flutter/packages@90c876d...d39830e 2024-04-16 [email protected] Manual roll Flutter from 2e748e8 to 3882afb (16 revisions) (flutter/packages#6549) 2024-04-16 [email protected] [image_picker] Add limit parameter to pickMultiImage and pickMultipleMedia to ios and Android (flutter/packages#6201) 2024-04-16 [email protected] [camera] Remove iOS thread-safe result class (flutter/packages#6498) 2024-04-16 [email protected] [google_maps_flutter] update ios privacy manifest (flutter/packages#6511) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Adds limit parameter to `MediaOptions` and `MultiImagePickerOptions`. The `limit` argument defines how many images or media files can be select. Only platform interface package changes taken from: flutter#6201 Fixes: [flutter/flutter#85772](flutter/flutter#85772)
…Media to ios and Android (flutter#6201) Adds limit parameter to `MediaOptions` and `MultiImagePickerOptions` and supports its use on iOS and Android. The `limit` argument defines how many images or media files can be select. Fixes: [flutter/flutter#85772](flutter/flutter#85772)
Adds limit parameter to `MediaOptions` and `MultiImagePickerOptions`. The `limit` argument defines how many images or media files can be select. Only platform interface package changes taken from: flutter#6201 Fixes: [flutter/flutter#85772](flutter/flutter#85772)
…Media to ios and Android (flutter#6201) Adds limit parameter to `MediaOptions` and `MultiImagePickerOptions` and supports its use on iOS and Android. The `limit` argument defines how many images or media files can be select. Fixes: [flutter/flutter#85772](flutter/flutter#85772)
Adds limit parameter to
MediaOptions
andMultiImagePickerOptions
and supports its use on iOS and Android. Thelimit
argument defines how many images or media files can be select.Fixes: flutter/flutter#85772
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.