[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

BeMacized · 2021-05-21T10:48:16Z

As of May 5th, the Google Play store requires all apps that target Android 11+ to use Scoped Storage, and as such they can no longer use the requestLegacyExternalStorage flag. image_picker required this as it stores camera captures in external storage.

This PR changes the storage location for camera captures to the internal cache. The internal cache is part of the app's internal storage and therefore requires no permissions, which meant the WRITE_EXTERNAL_STORAGE permission could be dropped for API 29+.
This should allow developers using the image picker plugin to target Android 11+ again.

Note: While theoretically this should solve the issue, I am not a 100% sure as I could not find a way to test whether the issue was resolved. The Google Play Console does not give a warning when uploading an apk/appbundle of the current example app, nor for the new version in this PR, so I do not have any results to compare. A sanity check would be appreciated.

Relevant Issue:

[image_picker] Image picker is incompatible with new Google Play's storage requirement flutter#80502

Pre-launch Checklist

mvanbeusekom

Besides from some nits, I have two questions:

Should this be considered a breaking change?
If it necessary to update the targetSdkVersion of the Android example App?

packages/image_picker/image_picker/android/src/main/AndroidManifest.xml

...ages/image_picker/image_picker/example/android/app/src/test/resources/robolectric.properties

packages/image_picker/image_picker/android/src/main/res/xml/flutter_image_picker_file_paths.xml

.../example/android/app/src/test/java/io/flutter/plugins/imagepicker/ImagePickerPluginTest.java

packages/image_picker/image_picker/example/android/app/build.gradle

mvanbeusekom · 2021-05-21T11:31:20Z

packages/image_picker/image_picker/CHANGELOG.md

+* Changed storage location for captured images and videos to internal cache on Android,
+to comply with new Google Play storage requirements.


I am wondering if this should be considered as a breaking change. As the API itself doesn't change, but the locations where images from the camera are stored is changed to scoped storage instead of the Environment.DIRECTORY_PICTURES external file location.
Meaning it will be more difficult for other Apps to reach the images. @stuartmorgan or @cyanglaz can you possibly advice on this?

It's probably better to err on the side of making it a breaking change since we don't know if people are relying on the location, even if it's not something that's guaranteed by the API.

@blasten any preference on this?

"If you require your picked image to be stored permanently, it is your responsibility to move it to a more permanent location."

I agree the plugin never said that this was the case in the first place, but I'm inclined to think that given the nature of the issue, making it a major release sounds good.

The changelog and pubspec have been updated to show this as a breaking change.

…ifest.xml Co-authored-by: Maurits van Beusekom <[email protected]>

…t/resources/robolectric.properties Co-authored-by: Maurits van Beusekom <[email protected]>

…utter_image_picker_file_paths.xml Co-authored-by: Maurits van Beusekom <[email protected]>

blasten · 2021-05-25T19:42:53Z

.../example/android/app/src/test/java/io/flutter/plugins/imagepicker/ImagePickerPluginTest.java

+    assertThat(
+        "Delegate uses cache directory for storing camera captures",
+        delegate.externalFilesDirectory,
+        equalTo(mockDirectory));


It would be great if we also check (in another unit test) that calling delegate.takeImageWithCamera writes a file to the mock directory.

Fair point! A test for this has been added.

mvanbeusekom

LGTM. Just had one suggestion to improve the CHANGELOG entry. The current entry explains what had been changed, however it doesn't explicitly explain the consequence of the change.

packages/image_picker/image_picker/CHANGELOG.md

Co-authored-by: Maurits van Beusekom <[email protected]>

packages/image_picker/image_picker/pubspec.yaml

stuartmorgan-g · 2021-05-27T14:32:13Z

@blasten Did you want to give this a final review?

blasten

LGTM

btrautmann · 2021-06-01T08:02:33Z

packages/image_picker/image_picker/android/src/main/AndroidManifest.xml

-   <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
+    package="io.flutter.plugins.imagepicker">
+
+    <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />


Can someone point me towards documentation that talks about why we have either of these permissions declared? As mentioned here (see the first row in the table), no permissions are needed for accessing the cache directory. I think these permissions are over-reaching and it'd be nice to drop them.

Right, the same goes for any API level of course, not just 29+. WRITE_EXTERNAL_STORAGE should not be needed at all anymore.

I cannot actually find any evidence that the READ_EXTERNAL_STORAGE permission is required for Intent.ACTION_GET_CONTENT, but it seems like a logical permission to request as we are reading images written by other apps, from external storage.
I have tried removing the permission, but it ends up throwing PlatformException(photo_access_denied, The user did not allow photo access., null, null), without prompting the user.

Therefore I would propose just removing the WRITE_EXTERNAL_STORAGE permission entirely and leaving READ_EXTERNAL_STORAGE as is.

Can you tell me more about your issue with that platform exception? I put together this sample to prove you don't need that permission (in the sample I use the get content action successfully without it). If I can reproduce the issue it may help me understand why it's needed.

Sorry @BeMacized I just had another look at the code. You're running into that Exception because you likely removed the permission declaration from the manifest but didn't remove the request for it from the code, which isn't allowed. If you remove it in both places you should be able to fetch the image without issue.

Ah I see! that makes things a lot more clear, thanks for the hint.
I've made the changes so that the read permission is no longer required either.

I know I'm probably being a pest @BeMacized but it's worth noting the plugin also doesn't need to request the camera permission because it's using the ACTION_VIDEO_CAPTURE and ACTION_IMAGE_CAPTURE intent actions. You can see that explained here. I figure since this is now a major release and the PR addresses permissions, it may be nice to do a sweep and remove them all?

FWIW, the reason I'm calling these all out is because it's nice for client apps to not request these permissions needlessly. It helps build trust with our users, so I'd love to see these go away if possible :)

If we could drop that one too that would indeed be excellent! I'll definitely take a look at it, although I can't guarantee it will also land together with this PR.

btrautmann · 2021-06-01T14:04:53Z

Firstly, thank you @BeMacized for the work here. I do think it's worth calling out though that this PR doesn't really address a scoped storage issue (at least one introduced in API29). That's evidenced by the fact that getExternalFilesDir(Environment.DIRECTORY_PICTURES) (the method removed in this PR) is actually the recommended way to store app-specific media files that provide value only to the user without any special permissions on API19+. What this PR does is take it one step further and simply add them to the cache, which (as far as I can tell from the documentation) makes it more likely for these files to be deleted when cache space is low (whereas, again, as far as I can tell, files in the path returned from getExternalFilesDir(...) will only be deleted upon app uninstall). One other positive impact of this change is that the need to request WRITE_EXTERNAL_STORAGE permission on <API19 is dropped, since getCacheDir requires no permissions since the files live in the private app storage.

All that to say, I think this is a good step in the right direction but I'm not sure it warrants specific Scoped Storage language (though the call out for the migration of where media files are being saved is great!).

BeMacized · 2021-06-01T14:27:22Z

@btrautmann Thank you for your thoughts. Using the cache directory was actually a deliberate choice, as this should remove the responsibility for developers using the plugin to clean up the captured image files. This way, it puts the responsibility on the developer to make sure their files are stored in a more permanent location, if they wish to do so. In my personal opinion this is preferable to the end-user's device filling up because the app developer did not take their use of storage into account.

That said, if this is not preferred by the community, using the app-specific folder in external storage is always still an option.

As a bit of a sidenote, one of the things that was discussed internally was that this change introduces a regression, where other apps are no longer able to access the image_picker captures. While it was decided that (for this PR) it is is out of scope, it was proposed that in the case demand for it was made apparent, an optional flag could be added for the plugin to more permanently save its captures using the media store api.

Perhaps this could also act as a solution for offering more permanent storage, while keeping the saved captures temporarily in the cache by default.

btrautmann · 2021-06-01T14:38:56Z

As a bit of a sidenote, one of the things that was discussed internally was that this change introduces a regression, where other apps are no longer able to access the image_picker captures. While it was decided that (for this PR) it is is out of scope, it was proposed that in the case demand for it was made apparent, an optional flag could be added for the plugin to more permanently save its captures using the media store api.

Perhaps this could also act as a solution for offering more permanent storage, while keeping the saved captures temporarily in the cache by default.

Absolutely! I think the behavior introduced in this PR should have been the default, with a flag available to "contribute to the MediaStore" added at a later date. 100% agree. In fact I think that would be a fun PR to work on, and was planning on investigating adding that behavior at a later date.

Again, thank you for your work here. It's very much appreciated and not at all taken for granted!

… internal cache on Android, to comply with new Google Play storage requirements. (flutter/plugins#3956)

* master: (131 commits) [in_app_purchase] fix "autoConsume" param in "buyConsumable" (flutter#3957) [video_player_web] fix: set autoplay to false during initialization (flutter#3985) [multiple_web] Adapt web PlatformView widgets to work slotted. (flutter#3964) [url_launcher] Add iOS unit and UI tests (flutter#3987) [image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. (flutter#3956) [script/tool] Use 'dart pub' instead of deprecated 'pub' (flutter#3991) [video_player] Add iOS unit and UI tests (flutter#3986) Add pubspec convention checks (flutter#3984) Enable pubspec dependency sorting lint (flutter#3983) [image_picker] Migrate maven repo from jcenter to mavenCentral (flutter#3915) [video_player] Update README.md (flutter#3975) [script/tool] speed up the pub get portion of the analyze command (flutter#3982) Revert commit e742a7b (flutter#3976) Added support to request list of purchases (flutter#3944) [google_maps_flutter] Add iOS unit and UI tests (flutter#3978) Added Windows to the description (flutter#3936) use 'flutter pub get' for both dart and flutter packages (flutter#3973) [camera] android-rework part 3: Android exposure related features (flutter#3797) Remove exoplayer workaround from everything but video_player (flutter#3980) Allow reverts when checking versions (flutter#3981) ...

…l cache on Android, to comply with new Google Play storage requirements. (flutter#3956)

BeMacized added 5 commits May 21, 2021 09:14

Switch to app-specific folder for writing camera captures.

7be1c7c

Merge branch 'master' into issue/80502

d79bac3

Update documentation

df306e2

Update targetSdkVersion for example. Update tests.

467bb5c

Updated changelog and pubspec version

b2e902e

BeMacized requested a review from cyanglaz as a code owner May 21, 2021 10:48

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

github-actions bot added p: image_picker platform-android labels May 21, 2021

mvanbeusekom suggested changes May 21, 2021

View reviewed changes

BeMacized and others added 4 commits May 21, 2021 14:49

Update packages/image_picker/image_picker/android/src/main/AndroidMan…

84a5ad1

…ifest.xml Co-authored-by: Maurits van Beusekom <[email protected]>

Update packages/image_picker/image_picker/example/android/app/src/tes…

46df81f

…t/resources/robolectric.properties Co-authored-by: Maurits van Beusekom <[email protected]>

Update packages/image_picker/image_picker/android/src/main/res/xml/fl…

15d841d

…utter_image_picker_file_paths.xml Co-authored-by: Maurits van Beusekom <[email protected]>

Implemented PR feedback

e1cb24e

blasten reviewed May 25, 2021

View reviewed changes

BeMacized added 2 commits May 26, 2021 07:47

Add test to check if file is created in cache directory

eafa023

Upgrade PR to breaking change

aa3c24d

mvanbeusekom approved these changes May 26, 2021

View reviewed changes

packages/image_picker/image_picker/CHANGELOG.md Outdated Show resolved Hide resolved

BeMacized and others added 4 commits May 26, 2021 14:01

Update packages/image_picker/image_picker/CHANGELOG.md

a3c737c

Co-authored-by: Maurits van Beusekom <[email protected]>

Update CHANGELOG.md

bc3e4d5

Merge branch 'master' into issue/80502

121a7ca

Removed redundant robolectric.properties

1143108

stuartmorgan-g reviewed May 26, 2021

View reviewed changes

packages/image_picker/image_picker/pubspec.yaml Outdated Show resolved Hide resolved

Fix merge issue

bfb2b6a

blasten approved these changes May 27, 2021

View reviewed changes

btrautmann reviewed Jun 1, 2021

View reviewed changes

mvanbeusekom mentioned this pull request Jun 1, 2021

[image_picker] Image picker is incompatible with new Google Play's storage requirement flutter/flutter#80502

Closed

BeMacized added 4 commits June 1, 2021 12:31

Removed WRITE_EXTERNAL_STORAGE permission completely

e973353

Remove READ_EXTERNAL_STORAGE permission checks

ca846ba

Ran format and removed redundant unit tests

fa3c577

Merge branch 'master' into issue/80502

80906ca

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 Jun 1, 2021

fluttergithubbot merged commit bb539fb into flutter:master Jun 1, 2021

github-actions bot added the needs-publishing label Jun 1, 2021

engine-flutter-autoroll mentioned this pull request Jun 1, 2021

Roll Plugins from 4bea573ee8ce to bb539fb06e55 (2 revisions) flutter/flutter#83725

Merged

This was referenced Jun 2, 2021

[image_picker] Remove redundant request for camera permission flutter/flutter#83789

Closed

[image_picker] Removed redundant request for camera permission #4001

Merged

stuartmorgan-g removed the needs-publishing label Jun 3, 2021

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

[image_picker] Change storage location for camera captures to interna…

2ec3f47

…l cache on Android, to comply with new Google Play storage requirements. (flutter#3956)

mvanbeusekom deleted the issue/80502 branch September 21, 2021 09:33

darshankawar mentioned this pull request Jan 18, 2023

[image_picker] Created image is not temporary but stored on the device flutter/flutter#118416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

BeMacized commented May 21, 2021 •

edited by mvanbeusekom

Loading

mvanbeusekom left a comment

mvanbeusekom May 21, 2021

stuartmorgan-g May 25, 2021

blasten May 25, 2021

BeMacized May 26, 2021

blasten May 25, 2021

BeMacized May 26, 2021

mvanbeusekom left a comment

stuartmorgan-g commented May 27, 2021

blasten left a comment

btrautmann Jun 1, 2021

BeMacized Jun 1, 2021

btrautmann Jun 1, 2021

btrautmann Jun 1, 2021

BeMacized Jun 1, 2021 •

edited

Loading

btrautmann Jun 1, 2021

BeMacized Jun 1, 2021

btrautmann commented Jun 1, 2021 •

edited

Loading

BeMacized commented Jun 1, 2021 •

edited

Loading

btrautmann commented Jun 1, 2021

		* Changed storage location for captured images and videos to internal cache on Android,
		to comply with new Google Play storage requirements.

[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

[image_picker] Change storage location for camera captures to internal cache on Android, to comply with new Google Play storage requirements. #3956

Conversation

BeMacized commented May 21, 2021 • edited by mvanbeusekom Loading

Pre-launch Checklist

mvanbeusekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanbeusekom left a comment

Choose a reason for hiding this comment

stuartmorgan-g commented May 27, 2021

blasten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeMacized Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btrautmann commented Jun 1, 2021 • edited Loading

BeMacized commented Jun 1, 2021 • edited Loading

btrautmann commented Jun 1, 2021

BeMacized commented May 21, 2021 •

edited by mvanbeusekom

Loading

BeMacized Jun 1, 2021 •

edited

Loading

btrautmann commented Jun 1, 2021 •

edited

Loading

BeMacized commented Jun 1, 2021 •

edited

Loading