[image_picker] Image picker fix camera device #3898

ydag · 2021-05-17T10:44:17Z

This PR fixes an issue where preferredCameraDevice is not working as expected for getVideo method. I also refactor device-only unit tests to make sure we can test camera devices on the CI as well.

Related issues :

Fixes flutter/flutter#74674

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

If you need help, consider asking for advice on the #hackers-new channel on Discord.

# Conflicts: # packages/image_picker/image_picker/pubspec.yaml

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

When camera is chosen the originalAsset will be nil which leads wrong orientation settings. This can be YES all the time because limited access only available with iOS 14+ and it will be handled with PHPicker implementation.

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

stuartmorgan-g

We should have tests covering this. Long-term that will probably require some refactoring for testability, but short term: was this test failing and we just didn't know because it's device-only?

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

ydag · 2021-05-19T11:10:21Z

We should have tests covering this. Long-term that will probably require some refactoring for testability, but short term: was this test failing and we just didn't know because it's device-only?

I think the problem is not this test. Since UIImagePickerControllerCameraDevice _device is a global variable we need to re-assign before we call the showCamera method from both pickImage and pickVideo in handleMethodCall.

But I double-check and I think we even don't need a UIImagePickerControllerCameraDevice _device variable at all if we call the getCameraDevice helper method inside the showCamera method.

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

stuartmorgan-g

We should have tests covering this. Long-term that will probably require some refactoring for testability, but short term: was this test failing and we just didn't know because it's device-only?

I think the problem is not this test. Since UIImagePickerControllerCameraDevice _device is a global variable we need to re-assign before we call the showCamera method from both pickImage and pickVideo in handleMethodCall.

Sorry, I'm not seeing how this answers my question. To clarify, there are two parts:

Where are the test changes that this PR needs to verify the fix, per Flutter policy?
What is the status of the test referenced above, which sounds like it should already have been testing this scenario, but had not alerted us to the failure previously given that the tree is green?

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

This reverts commit 877b0d8.

ydag · 2021-05-27T14:29:37Z

Sorry, I'm not seeing how this answers my question. To clarify, there are two parts:

Where are the test changes that this PR needs to verify the fix, per Flutter policy?

What is the status of the test referenced above, which sounds like it should already have been testing this scenario, but had not alerted us to the failure previously given that the tree is green?

After some tests, I realized that this test is failing as you said and we didn't know because it's device-only.

I was not able to run this test on the real device (because of some weird errors when I was trying to run unit tests of the image_picker on a real device) but after some manipulation on the test and the code, I confirmed that it is failing and this PR fixes the problem.

stuartmorgan-g · 2021-05-27T20:42:34Z

That means we don't have any meaningful test coverage here, since a regression would (as evidenced by this PR) never show up in our CI.

It looks like this should be testable via native unit tests. It would require class mocking of AVCaptureDevice and UIImagePickerController unfortunately, since there are class methods called on them, but class mocks are still better than no test coverage at all. Alternately, you could wrap those calls in a helper that can be instantiated and mocked less invasively via DI.

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/pubspec.yaml

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m # packages/image_picker/image_picker/pubspec.yaml

mvanbeusekom · 2021-06-23T17:19:05Z

That means we don't have any meaningful test coverage here, since a regression would (as evidenced by this PR) never show up in our CI.

It looks like this should be testable via native unit tests. It would require class mocking of AVCaptureDevice and UIImagePickerController unfortunately, since there are class methods called on them, but class mocks are still better than no test coverage at all. Alternately, you could wrap those calls in a helper that can be instantiated and mocked less invasively via DI.

@stuartmorgan could you have another look at this PR? @ydag updated the tests so the device specific features are mocked using OCMock and unit-tests will run correctly on the simulator. Chris already approved but the PR is blocked on your requested changes.

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

packages/image_picker/image_picker/example/ios/Podfile

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

stuartmorgan-g · 2021-06-25T14:10:59Z

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

-  if (![UIImagePickerController isSourceTypeAvailable:UIImagePickerControllerSourceTypeCamera]) {
+#pragma mark - Test camera devices, no op on simulators
+- (void)testPluginPickImageDeviceCancelClickMultipleTimes {
+  if ([UIImagePickerController isSourceTypeAvailable:UIImagePickerControllerSourceTypeCamera]) {


Why are you aborting the test if the camera is available?

Hi Stuart,
This change made by this commit and it already exists in master as well and got merged in my PR. Do you think we should remove it? Or do you think we should ask what was this test about and try to fix it?

@cyanglaz Was this a bug, or is this intentionally only supposed to run on simulator? That seems bizarre.

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

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

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

stuartmorgan-g

LGTM with one comment nit (no need to get another review once fixed)

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

…s#3898)

* upstream_master: (40 commits) [image_picker] Image picker fix camera device (flutter#3898) [flutter_plugin_tools] Improve license-check output (flutter#4154) [webview_flutter] Fix broken keyboard issue link (flutter#3266) [flutter_plugin_tools] Support format on Windows (flutter#4150) [flutter_plugin_tools] Make unit tests pass on Windows (flutter#4149) [image_picker_for_web] Migrate image_picker to package:cross_file (flutter#4083) [various] Prepare plugin repo for binding API improvements (flutter#4148) [quick_actions] Add const constructor (flutter#4131) [in_app_purchase] Add iOS currency symbol to ProductDetails (flutter#4144) [in_app_purchase] Added priceCurrencySymbol to SkuDetailsWrapper (flutter#4114) [image_picker_platform_interface] Add methods that return package:cross_file (flutter#4072) [flutter_plugin_tools] Improve and test 'format' (flutter#4145) [flutter_plugin_tools] Only check target packages in analyze (flutter#4146) [in_app_purchase] Fix crash when retrieveReceiptWithError gives an error. (flutter#4138) [video_player] Pause video when it completes (flutter#3727) [in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115) [in_app_purchase] Add documentation for price change confirmations (flutter#4092) [camera] android-rework part 8: Supporting modules for final implementation (flutter#4054) [plugin_platform_interface] Fix README broken link (flutter#4143) [various] Prepare plugin repo for binding API improvements (flutter#4137) ...

…s#3898)

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/example/ios/Podfile # packages/image_picker/image_picker/pubspec.yaml

ydag requested a review from cyanglaz as a code owner May 17, 2021 10:44

google-cla bot added the cla: yes label May 17, 2021

github-actions bot added p: image_picker platform-ios labels May 17, 2021

ydag added 4 commits May 17, 2021 16:15

Fix handler method to get preferred camera device

fd62211

Update the version number

aaa4a39

# Conflicts: # packages/image_picker/image_picker/pubspec.yaml

Add description into the CHANGELOG.md file

9c021ad

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

Fix an issue when camera is chosen

877b0d8

When camera is chosen the originalAsset will be nil which leads wrong orientation settings. This can be YES all the time because limited access only available with iOS 14+ and it will be handled with PHPicker implementation.

ydag force-pushed the image_picker_fix_camera_device branch from 9df8716 to 877b0d8 Compare May 17, 2021 14:20

ydag added 2 commits May 18, 2021 09:18

Merge branch 'master' into image_picker_fix_camera_device

3a75fe8

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

Update the version number

00213eb

stuartmorgan-g reviewed May 18, 2021

View reviewed changes

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m Outdated Show resolved Hide resolved

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m Outdated Show resolved Hide resolved

ydag added 2 commits May 19, 2021 12:10

Add helper method to get camera device

5b7ff47

Refactor to get preferred camera device

b548492

ydag added 3 commits May 19, 2021 15:59

Fix team selection in XCTest signing

9b8b882

Merge branch 'master' into image_picker_fix_camera_device

4ffe307

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

Update the version number

2313548

stuartmorgan-g suggested changes May 25, 2021

View reviewed changes

ydag added 6 commits May 26, 2021 08:18

Remove unnecessary curly brackets

00ae1ff

Remove unnecessary curly brackets

30d61c7

Add local variable to call method only once

ceb2577

Add a declaration comment

4398ae6

Fix the method call

22b684a

Revert "Fix an issue when camera is chosen"

c93a157

This reverts commit 877b0d8.

ydag added 3 commits June 1, 2021 08:40

Merge branch 'master' into image_picker_fix_camera_device

3760e13

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/pubspec.yaml

Merge branch 'master' into image_picker_fix_camera_device

72bdca4

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/pubspec.yaml

Update the comment

23f4d8a

Merge branch 'master' into image_picker_fix_camera_device

6b82355

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md # packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m # packages/image_picker/image_picker/pubspec.yaml

ydag added 2 commits June 25, 2021 08:54

Merge branch 'master' into image_picker_fix_camera_device

9ffa039

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

Update the version number

bf8bd36

stuartmorgan-g suggested changes Jun 25, 2021

View reviewed changes

ydag added 8 commits June 28, 2021 07:19

Remove tearDown method

5a14280

Convert true to YES

6fe570f

Add blank after pragma mark

4927206

Refactor to return AVAuthorizationStatus constant

84b3e65

Fix the comment

6294377

Add a comment

2800919

Merge branch 'master' into image_picker_fix_camera_device

b0022f6

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

Update the version number

8108389

ydag requested a review from stuartmorgan-g June 30, 2021 08:28

ydag added 3 commits June 30, 2021 21:23

Fix comments

5d7181b

Merge branch 'master' into image_picker_fix_camera_device

e65ce44

# Conflicts: # packages/image_picker/image_picker/CHANGELOG.md

Update version number

8634c68

stuartmorgan-g approved these changes Jul 8, 2021

View reviewed changes

packages/image_picker/image_picker/ios/Classes/FLTImagePickerPlugin.m Outdated Show resolved Hide resolved

ydag added 2 commits July 9, 2021 13:36

Update the comment

307757c

Merge branch 'master' into image_picker_fix_camera_device

a2b22d6

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 Jul 14, 2021

fluttergithubbot merged commit cf80430 into flutter:master Jul 14, 2021

engine-flutter-autoroll mentioned this pull request Jul 14, 2021

Roll Plugins from fbb4b3a85e3a to cf80430e72e1 (1 revision) flutter/flutter#86417

Closed

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

cf80430 [image_picker] Image picker fix camera device (flutter/plugin…

dbc36fb

…s#3898)

engine-flutter-autoroll mentioned this pull request Jul 16, 2021

Roll Plugins from fbb4b3a85e3a to dd2d7397379e (2 revisions) flutter/flutter#86523

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 16, 2021

cf80430 [image_picker] Image picker fix camera device (flutter/plugin…

2f39a07

…s#3898)

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[image_picker] Image picker fix camera device (flutter#3898)

3f412f5

amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021

[image_picker] Image picker fix camera device (flutter#3898)

cf2eb1a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[image_picker] Image picker fix camera device #3898

[image_picker] Image picker fix camera device #3898

ydag commented May 17, 2021 •

edited

Loading

stuartmorgan-g left a comment

ydag commented May 19, 2021 •

edited

Loading

stuartmorgan-g left a comment

ydag commented May 27, 2021

stuartmorgan-g commented May 27, 2021

mvanbeusekom commented Jun 23, 2021

stuartmorgan-g Jun 25, 2021

ydag Jun 28, 2021 •

edited

Loading

stuartmorgan-g Jun 28, 2021

stuartmorgan-g left a comment

[image_picker] Image picker fix camera device #3898

[image_picker] Image picker fix camera device #3898

Conversation

ydag commented May 17, 2021 • edited Loading

Pre-launch Checklist

stuartmorgan-g left a comment

Choose a reason for hiding this comment

ydag commented May 19, 2021 • edited Loading

stuartmorgan-g left a comment

Choose a reason for hiding this comment

ydag commented May 27, 2021

stuartmorgan-g commented May 27, 2021

mvanbeusekom commented Jun 23, 2021

stuartmorgan-g Jun 25, 2021

Choose a reason for hiding this comment

ydag Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

stuartmorgan-g Jun 28, 2021

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

ydag commented May 17, 2021 •

edited

Loading

ydag commented May 19, 2021 •

edited

Loading

ydag Jun 28, 2021 •

edited

Loading