-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[image_picker] fix camera on Android 11 #3194
[image_picker] fix camera on Android 11 #3194
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Hi, is there anything else I can do here? |
5970e8b
to
0d9c4b9
Compare
6e416e9
to
d29990f
Compare
Rebased on master and fixed conflicts. |
sigh Any chance that someone might look at this? 🤔 |
d29990f
to
2fa3b12
Compare
2fa3b12
to
7f54426
Compare
Any chance to get this looked at? |
7f54426
to
010f3cf
Compare
Thanks for the contribution! We apologize for the long delay in triaging this PR. We’re in the process of overhauling our PR triage system to respond much more quickly, as well as working through the backlog. @mvanbeusekom Could you add this review to your list? |
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 a good change to me, especially because Google also recommends catch (e: ActivityNotFoundException)
in their blog post.
However, I think it would be better to clean the empty file.
...r/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java
Show resolved
Hide resolved
010f3cf
to
c9cdd66
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.
LGTM
@Bubu can you bring your branch up to date with master once more and update the changelog/pubspec? I'm afraid 0.8.1+1 I think there is also 1 broken test (maybe that is new since you fixed the tests).
|
Ugh, there is... It's been so long since I wrote the changes to the tests here... 🙈 . Let me see if I can make it work again :D. |
c9cdd66
to
a2509ed
Compare
@renefloor I can't figure out how to run the tests successfully locally (any pointers for that?). I still pushed a commit that should fix it and also (hopefully) resolved the conflicts. 🎉 |
@Bubu Currently That's because the test doesn't expect a file to be created (because the camera is not available), so the file system was not mocked. I added this after line 183 in that test: MockedStatic<File> mockStaticFile = Mockito.mockStatic(File.class);
mockStaticFile
.when(() -> File.createTempFile(any(), any(), any()))
.thenReturn(new File("/tmpfile")); and I added the following line at the end of the test: mockStaticFile.close(); I cannot really comment this as a suggestion on your PR as these are not lines that changed, but I hope it makes sense. The complete test becomes: @Test
public void
takeImageWithCamera_WhenHasCameraPermission_AndNoActivityToHandleCameraIntent_FinishesWithNoCamerasAvailableError() {
when(mockPermissionManager.isPermissionGranted(Manifest.permission.CAMERA)).thenReturn(true);
MockedStatic<File> mockStaticFile = Mockito.mockStatic(File.class);
mockStaticFile
.when(() -> File.createTempFile(any(), any(), any()))
.thenReturn(new File("/tmpfile"));
doThrow(ActivityNotFoundException.class)
.when(mockActivity)
.startActivityForResult(any(Intent.class), anyInt());
ImagePickerDelegate delegate = createDelegate();
delegate.takeImageWithCamera(mockMethodCall, mockResult);
verify(mockResult)
.error("no_available_camera", "No cameras available for taking pictures.", null);
verifyNoMoreInteractions(mockResult);
mockStaticFile.close();
} It would me nice to also add |
🤞 (I'm flying blind here, when I try to run the tests I get a huge amount of unrelated build errors instead. Let's just hope it works this time :)) |
We are now failing at a different test. I don't quite understand why my MR would be changing the behavior for that test. Did some merge go wrong here? |
I'm at my phone at the moment, but make sure to open image_picker/example/android and not image_picker/android. You might need to run the flutter example once or maybe you are missing parts of the SDK. |
Rebased. Let's see how it goes :). |
@blasten Do you want to do a final review on this? |
Hey everyone, any progress here? :-) |
...r/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java
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.
requires to catch the exception
...r/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerDelegate.java
Show resolved
Hide resolved
Please UPDATE: You can temporally fix this issue targeting your app to sdk 29 :) |
@Bubu Could you resolve the conflicts so we move forward with landing this? |
resolveActivity(intent) might return false on android 11+_even though startActivity(intent) would succeed. Instead of introducing a <queries> entry we can just try to start the activity and catch the exception if that doesn't work. Properly fixes flutter/flutter#62669. Ref: https://cketti.de/2020/09/03/avoid-intent-resolveactivity/
fdb4c24
to
fe5cd5d
Compare
@stuartmorgan hopefully all done correctly. |
There's now conflicts again here now... 🤔 |
That's my mistake for not following up after merging in master to fix the infra issues. |
Description
resolveActivity(intent) might return false on android 11+ even though startActivity(intent) would succeed.
Instead of introducing a entry we can just try to start the
activity and catch the exception if that doesn't work.
See here for a longer form explanation: https://cketti.de/2020/09/03/avoid-intent-resolveactivity/
Related Issues
Fixes flutter/flutter#85599
flutter/flutter#62669
Some more info on the android 11 behaviour change: flutter/flutter#63727 (comment)
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?