-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[image_picker] Add the ability to pick multiple videos #9775
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: main
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request adds the pickMultiVideo
feature, allowing users to select multiple videos from their gallery. The changes span across the platform interface and all platform implementations (Android, iOS, Web, Linux, macOS, Windows). The example apps have also been updated to demonstrate this new functionality, along with some general cleanup.
My review focuses on ensuring the new feature is implemented correctly across all platforms, checking for potential issues in the example apps, and verifying that dependency configurations are correct for merging.
Overall, the implementation is solid and the refactoring on the iOS side is a great improvement. I've identified a few issues, mainly in the example applications and pubspec.yaml
files, that should be addressed before merging.
packages/image_picker/image_picker_windows/example/lib/main.dart
Outdated
Show resolved
Hide resolved
packages/image_picker/image_picker_android/example/lib/main.dart
Outdated
Show resolved
Hide resolved
packages/image_picker/image_picker_windows/example/lib/main.dart
Outdated
Show resolved
Hide resolved
...picker_android/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java
Outdated
Show resolved
Hide resolved
For review context, this PR started as an experiment with Jules using the new agents.md file. Jules wrote:
I then wrote all the example app changes (I forgot to ask Jules for those, and didn't feel like going back and doing another round once I realized I needed them when manually testing), did the iOS refactoring, did some minor Android tweaks, added some native unit tests, and fixed all the changelogs and versions (Jules got confused and bumped the version of every package twice, possibly using the repo tooling). |
…va/io/flutter/plugins/imagepicker/ImagePickerDelegate.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@@ -121,12 +122,19 @@ - (void)launchUIImagePickerWithSource:(nonnull FLTSourceSpecification *)source | |||
UIImagePickerController *imagePickerController = [self createImagePickerController]; | |||
imagePickerController.modalPresentationStyle = UIModalPresentationCurrentContext; | |||
imagePickerController.delegate = self; | |||
NSMutableArray<NSString *> *mediaTypes = [[NSMutableArray alloc] init]; | |||
if (context) { | |||
[mediaTypes addObject:(NSString *)kUTTypeImage]; |
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 like kUTTypeImage
is included regardless of context.includeImages
. Is that intentional? Shouldn't it not be included for pickMultiVideoWithMaxDuration
?
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 that intentional?
Nope, that was me somehow forgetting to type .includeImages
on that condition.
I missed it in manual testing because I forgot to do the test I had intended to do where I forced the UIImagePickerController codepaths; I've now done that.
I also looked at why no existing unit tests failed, and it looks like unfortunately the native unit tests aren't currently set up to force codepaths, so we aren't getting good coverage of the older codepath. It may not be worth investing time into addressing that at this point though, given that it's only for 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.
iOS side LGTM
Adds
pickMultiVideo
, to allow picking multiple videos. This fills the gap in our current support matrix, which allows single or multiple images, single or multiple image-or-video, but only single video-only.Other changes:
Fixes flutter/flutter#102283
Fixes flutter/flutter#146400
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3