[image_picker_android] Adds Android 13 photo picker functionality #7043
Conversation
…rsion 33 or later. Bumps compileSdkVersion from 31 to 33.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
FXschwartz
changed the title
|
@camsim99 Can you provide pointers on doing SDK-version-linked unit tests? I feel like we have examples in other plugins but I'm not sure where. |
That would be great, as well as an idea on how I could identify which style of photo picker is being used. |
ResolutionFeatureTest.java has examples of setting min/max SDK versions, LocalAuthTest.java has examples of setting an exact SDK version. I believe you'll have to configure the test to run with Robolectric for it to work, like in both of those tests, e.g. here. I'm not familiar with this plugin, so I'm not sure what you mean by style of image picker. Can you clarify? |
You don't, you inject mocks to receive the intent calls and validate the arguments. The existing unit tests for the plugin have examples of that. |
But wouldn't it be necessary on the flutter side to verify the correct image_picker is being used based on the SDK version? |
The plugin's unit tests do not need to validate the UI that results from handing an intent, no; the plugin has no control over that, so it's out of scope for the plugin's tests. We just need to validate that the plugin is making the correct intent request based on the SDK version. |
Ahh fair enough. Then I'll add a test case for the two SDK version scenarios. Appreciate your help @camsim99 @stuartmorgan |
|
@camsim99
Disregard this, I misread the above comment. |
…ry, and chooseImageFromGallery operating differently based on current SDK version.
|
@stuartmorgan @GaryQian Test cases were added for all three methods that were updated. |
|
Based on recent offline discussion of image picker functionality: could we just use androidx.activity instead of doing all of this version switching ourselves? |
That would definitely be ideal, sorry I missed this! |
hey @FXschwartz, I'm curious what the state of this is? I have something I'd like to add to this after your pr is landed. Thanks for doing this! |
Hey there! Yeah sorry for the delay, busy couple of weeks. This should be a pretty easy change per the last comment, I don't think I'll even need to remove any of the tests I added. I'll make it a priority for tomorrow 😀 |
|
@stuartmorgan @tarrinneal Looks like in order to switch to using android.x.activity, I'll need to make more changes the originally thought. Can one of you that are more familiar with Android development give me a little guidance with the questions I have below?
I've added the correct android.x.compat dependencies, but |
I'm not familiar enough with Android to know the answer to this. @reidbaker can you advise on whether this is correct? If we can't use it from |
There was a problem hiding this comment.
(Continuing from your question on Discord:) I think you can take advantage of androidx.activity to simplify away these SDK checks just fine, even without AppCompatActivity.
The key is the createIntent method on those "activity result contract" objects. So for example, taking this code from the guide doc:
ActivityResultLauncher<PickVisualMediaRequest> pickMedia =
registerForActivityResult(new PickVisualMedia(), uri -> {
// Callback is invoked after the user selects a media item or closes the
// photo picker.
if (uri != null) {
Log.d("PhotoPicker", "Selected URI: " + uri);
} else {
Log.d("PhotoPicker", "No media selected");
}
});
// Launch the photo picker and allow the user to choose only images.
pickMedia.launch(new PickVisualMediaRequest.Builder()
.setMediaType(PickVisualMedia.ImageOnly.INSTANCE)
.build());you'd use PickVisualMedia(), which constructs an ActivityResultContract, and then you'd call its createIntent method like so:
Intent intent = new PickVisualMedia().createIntent(
activity,
new PickVisualMediaRequest.Builder()
.setMediaType(PickVisualMedia.ImageOnly.INSTANCE)
.build());Then you'd pass that intent to activity.startActivityForResult just like in the existing code. Here's the source of that createIntent implementation:
https://android.googlesource.com/platform/frameworks/support/+/a67808214/activity/activity/src/main/java/androidx/activity/result/contract/ActivityResultContracts.kt#792
You can see that it uses MediaStore.ACTION_PICK_IMAGES if possible, then has several fallbacks to other alternatives, and that it sets type on the intent.
The thing from androidx.activity that you don't get this way is the handy aspect of registerForActivityResult's API where you pass it a callback. Instead, you have to keep using onActivityResult and having that look out for the expected request code.
But that's the part that fundamentally requires the app's activity to subclass something from AndroidX like AppCompatActivity: the Android base system is going to call onActivityResult, so an API like that of registerForActivityResult relies on having an onActivityResult implementation that is going to call something it hooks into. Here's where that hook appears in the AndroidX source, on an ancestor of AppCompatActivity:
https://android.googlesource.com/platform/frameworks/support/+/a67808214/activity/activity/src/main/java/androidx/activity/ComponentActivity.java#818
In a Flutter plugin, the analogous mechanism is ActivityPluginBinding#addActivityResultListener, which is how this plugin's own onActivityResult ends up getting called.
| int requestCode = | ||
| isPhotoPickerAvailable | ||
| ? REQUEST_CODE_CHOOSE_VIDEO_FROM_GALLERY_USING_PHOTO_PICKER | ||
| : REQUEST_CODE_CHOOSE_VIDEO_FROM_GALLERY; |
There was a problem hiding this comment.
No need for these to be different from each other. What is needed is just that they're different from anything else (any request codes anywhere in the app — not sure what the practice is among Flutter plugins for avoiding accidental conflicts between them), and that this plugin's onActivityResult handles them.
There was a problem hiding this comment.
Ok perfect!
I initially made these different so the tests could confirm that the correct intent tag was being used based on the given Android SDK.
There was a problem hiding this comment.
(continued post-migration at https://github.com/flutter/packages/pull/3267/files#r1114883049 )
This makes a ton of sense, really appreciate your help! Then this should be a really easy change and I'll make sure it's on my TODO list tomorrow! |
|
We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages. Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord. Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state! |
Absolutely no apologies necessary! I'll get it moved over today. |
NOTE: This is my first PR in the Flutter repo, I am also not a Java developer so extra scrutiny is likely required :) I'm also not sure what tests should be added. The functionality of the new photo picker is the same as the previous, and all the current tests pass. The only thing I can think of is maybe it would be good to test that devices running < SDK 33 use the old image picker, and devices running > SDK 33 use the new photo picker. I that is the case I'll need a bit of direction as I'm not sure how I can test that.
Description
This PR adds the new Android 13 photo picker functionality to Android devices running SDK version 33 or later as well as bumps the compileSdkVersion from 31 to 33. If an Android device is older, it will use the previous standard image picker. Below are two devices, the left running SDK 30 and the right running SDK 33.
List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#104250
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy. N/A
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto 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.