[image_picker] Default to gallery instead of camera when picking multiple images on pre-iOS 14 devices. #4718

mvanbeusekom · 2022-02-01T12:41:25Z

Solves two problems when using the ImagePicker.pickMultiImage method on pre-iOS 14 devices:

Picking multiple images now defaults to selecting images from the gallery instead of opening the camera (this is expected behaviour as it is not possible to select multiple images from the camera, see also issue [image_picker] _picker.pickMultiImage launches camera instead of gallery on IOS 12 flutter#96903);
Make sure the supplied arguments and result callback are made available to the UIImagePicker when picking images on devices running iOS 13 and lower.

Resolves flutter/flutter#96903
Replaces and closes #4706

EDIT: Converted this PR to draft as it depends on changes from PR #4722.

Pre-launch Checklist

packages/image_picker/image_picker/CHANGELOG.md

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

packages/image_picker/image_picker/example/ios/RunnerTests/ImagePickerPluginTests.m

mvanbeusekom · 2022-02-01T14:21:34Z

Hi @stuartmorgan,

I fully agree with your comments and I have been struggling with this too. Ideally we'd separate the method channel implementation from the code that interacts directly with the UIImagePicker and PHPicker controls into separate classes. For example:

A MethodChannelHandler class, responsible for communicating with the Flutter method channel and parsing arguments into strongly typed structures;
A FLTImagePicker class, abstraction around the UIImagePicker and PHPicker controls exposing methods and callbacks accepting only strongly type and clearly named parameters.

This would make testing the API surface between Dart and Native a lot easier. However this is also a large refactor. My reasoning was to solve this bug now by not introducing new parameters, ivars or logic but instead simply fixing the existing implementation (and add some tests to ensure it works) and discuss a bigger refactor with you offline.

stuartmorgan-g · 2022-02-01T15:49:53Z

Ideally we'd separate the method channel implementation from the code that interacts directly with the UIImagePicker and PHPicker controls into separate classes.

Sounds like a good way to go longer term, but it shouldn't actually be necessary for what I'm suggesting. Look at the local_auth iOS tests for instance, which have a single class but still do method-call-to-mock-system-API testing.

My reasoning was to solve this bug now by not introducing new parameters, ivars or logic

Well, there's obviously new logic here. But I'm fine with the short term fix itself being pretty minimal (the non-tests changes I asked for should be quite small), the issue is that the change you made to the tests make the code worse. They make fixing the root problem harder, because they enshrine and encourage an anti-pattern. Small incremental changes are fine but they need to take us in the right direction.

but instead simply fixing the existing implementation (and add some tests to ensure it works)

I think "ensure it works" is highly arguable. As an experiment I tried moving self.arguments = call.arguments; to the end of handleMethodCall:... (where it would do absolutely nothing), and 2/3 of your new tests still pass.

They ensure that part of the anti-pattern is being followed at some point, but not that it's the right point, or that it's used correctly.

Refactored the existing unit tests to expose private interfaces in a separate test header instead of an inline interface. This was first introduced in the google_sign_in plugin (flutter#4157) and after that also applied in the camera plugin (flutter#4426) and the webview_flutter plugin (flutter#4480).

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

packages/image_picker/image_picker/CHANGELOG.md

stuartmorgan-g · 2022-02-09T20:26:16Z

packages/image_picker/image_picker/example/ios/RunnerTests/ImagePickerPluginTests.m

@@ -146,6 +146,32 @@ - (void)testPluginPickVideoDeviceFront {
                 UIImagePickerControllerCameraDeviceFront);
 }

+- (void)testPickMultiImageShouldUseUIImagePickerControllerOnPreiOS14 {
+  if (@available(iOS 14, *)) {


@jmagman Relevant to our discussions about test matrixes in this repository: we won't ever actually test this in CI currently.

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

stuartmorgan-g · 2022-02-09T20:31:04Z

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

-    _imagePickerController.modalPresentationStyle = UIModalPresentationCurrentContext;
-    _imagePickerController.delegate = self;
-    _imagePickerController.mediaTypes = @[
+    UIImagePickerController *imagePickerController = [self createImagePickerController];


Shouldn't this be self.imagePickerController?

If we need a stateful controller, I'm very concerned about our test coverage if this didn't break anything. Does it not actually need to be stateful?

With some refactoring it is not necessary to keep the imagePickerController as state. I did the refactoring necessary which involves passing the current copy of the imagePickerController around to some other methods. Please have a look if you agree with this.

Looks great, thanks! (Now we just need to do the same with arguments, and ideally result, but those are definitely out of scope.)

stuartmorgan-g · 2022-02-09T20:31:33Z

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

    if (image == nil) {
-      image = [info objectForKey:UIImagePickerControllerOriginalImage];
+      image = info[UIImagePickerControllerOriginalImage];


Thanks for fixing this everywhere :)

stuartmorgan-g

LGTM with nits, thanks for all the iteration and test work here!

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

stuartmorgan-g · 2022-02-09T21:29:24Z

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

-    _imagePickerController.modalPresentationStyle = UIModalPresentationCurrentContext;
-    _imagePickerController.delegate = self;
-    _imagePickerController.mediaTypes = @[
+    UIImagePickerController *imagePickerController = [self createImagePickerController];


Looks great, thanks! (Now we just need to do the same with arguments, and ideally result, but those are definitely out of scope.)

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m

…cking multiple images on pre-iOS 14 devices. (flutter/plugins#4718)

Default to image gallery when picking multiple images

b1dfdcf

mvanbeusekom requested review from stuartmorgan-g and BeMacized February 1, 2022 12:41

mvanbeusekom requested a review from cyanglaz as a code owner February 1, 2022 12:41

github-actions bot added p: image_picker platform-ios labels Feb 1, 2022

Add additional unit-tests

9d15c59

mvanbeusekom force-pushed the issue/96903 branch from 284076e to 9d15c59 Compare February 1, 2022 12:44

stuartmorgan-g suggested changes Feb 1, 2022

View reviewed changes

jmagman mentioned this pull request Feb 1, 2022

[image_picker] _picker.pickMultiImage launches camera instead of gallery on IOS 12 flutter/flutter#96903

Closed

mvanbeusekom mentioned this pull request Feb 2, 2022

[image_picker] Refactored tests to expose private interface in separate test header. #4722

Merged

11 tasks

mvanbeusekom added 6 commits February 2, 2022 15:58

Adds declaration comments.

e8333f8

Fix spelling error in changelog

f60f73d

Fix formatting

3921f8d

Merged in unit test refactoring

2b5555a

Applied feedback from PR

8707cd8

mvanbeusekom force-pushed the issue/96903 branch from 09d29ea to 8707cd8 Compare February 3, 2022 16:25

mvanbeusekom marked this pull request as draft February 3, 2022 16:26

mvanbeusekom added 9 commits February 7, 2022 20:41

Adds declaration comments.

6fc92d6

Fix spelling error in changelog

3a6d1b5

Fix formatting

69a41d8

Remove private headers from umbrella

31442f3

Merge branch 'image_picker_test_header' into issue/96903

86a2987

Removes the pickImageWithUIImagePicker trampoline method.

9b71661

Use call.arguments instead of self.arguments.

76131a7

Merge remote-tracking branch 'upstream/main' into issue/96903

84abc6f

mvanbeusekom marked this pull request as ready for review February 9, 2022 10:58

mvanbeusekom requested a review from stuartmorgan-g February 9, 2022 10:58

stuartmorgan-g suggested changes Feb 9, 2022

View reviewed changes

mvanbeusekom added 2 commits February 9, 2022 20:49

Merge remote-tracking branch 'upstream/main' into issue/96903

35dc1ab

Apply feedback on PR

5ea21c7

stuartmorgan-g suggested changes Feb 9, 2022

View reviewed changes

mvanbeusekom added 2 commits February 9, 2022 21:59

Refactored FLTImagePickerPlugin not to be stateful

134ba8b

Added declaration comments as required by style

6bbd53c

mvanbeusekom requested a review from stuartmorgan-g February 9, 2022 21:09

stuartmorgan-g approved these changes Feb 9, 2022

View reviewed changes

Apply fixes for nits

f9bfe66

mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 10, 2022

fluttergithubbot merged commit aa36a8a into flutter:main Feb 10, 2022

engine-flutter-autoroll mentioned this pull request Feb 10, 2022

Roll Plugins from 9d32c83af9af to aa36a8a606e1 (10 revisions) flutter/flutter#98198

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 10, 2022

aa36a8a60 [image_picker] Default to gallery instead of camera when pi…

68f31e7

…cking multiple images on pre-iOS 14 devices. (flutter/plugins#4718)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Plugins from 9d32c83af9af to f6495091d470 (18 revisions) flutter/flutter#98261

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

aa36a8a60 [image_picker] Default to gallery instead of camera when pi…

5eb4262

…cking multiple images on pre-iOS 14 devices. (flutter/plugins#4718)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Plugins from 9d32c83af9af to f45bf32f9f53 (19 revisions) flutter/flutter#98266

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

aa36a8a60 [image_picker] Default to gallery instead of camera when pi…

05e85ed

…cking multiple images on pre-iOS 14 devices. (flutter/plugins#4718)

engine-flutter-autoroll mentioned this pull request Feb 14, 2022

Roll Plugins from 9d32c83af9af to 6b0d4549eaa5 (30 revisions) flutter/flutter#98410

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2022

aa36a8a60 [image_picker] Default to gallery instead of camera when pi…

dbc1218

…cking multiple images on pre-iOS 14 devices. (flutter/plugins#4718)

jonasfranz mentioned this pull request Mar 15, 2022

[image_picker][iOS]: pickMultiImage throws an invalid type cast exception on iOS 12 flutter/flutter#100132

Closed

[image_picker] Default to gallery instead of camera when picking multiple images on pre-iOS 14 devices. #4718

[image_picker] Default to gallery instead of camera when picking multiple images on pre-iOS 14 devices. #4718

Uh oh!

Conversation

mvanbeusekom commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvanbeusekom commented Feb 1, 2022

Uh oh!

stuartmorgan-g commented Feb 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

mvanbeusekom Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mvanbeusekom commented Feb 1, 2022 •

edited

Loading