Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(android): improve error message when Filesystem.copy fails #3148

Merged
merged 5 commits into from
Jul 6, 2020

Conversation

asztal
Copy link
Contributor

@asztal asztal commented Jun 24, 2020

In my case, this failed on Android 10 due to not having android:requestLegacyExternalStorage="true"
in AndroidManifest.xml. At least if there's a useful error message, it's a starting point for debugging.

I noticed the documentation for the FileSystem plugin does mention this for the FileSystemDirectory.Documents
enum member, but actually this can also be required when copying a file produced by the Camera plugin into the Data
directory.

Presumably this was introduced when upgrading the target SDK version from 28 to 29, so it might be worth calling
out in the 2.0 upgrade notes here.

I'm also a little concerned about what I read about Android 10+. If Capacitor doesn't switch to using the Storage Access Framework, will copying photos from the camera stop working entirely at some point?
https://commonsware.com/blog/2019/06/07/death-external-storage-end-saga.html

In my case, this failed on Android 10 due to not having permissions and not having
`android:requestLegacyExternalStorage="true"` in `AndroidManifest.xml`. At least if there's
a useful error message, it's a starting point for debugging (though perhaps this should
be mentioned in the documentation somewhere, either for the FileSystem plugin or for Android
configuration - or even for the default template, since the plugins need it to work?)

https://commonsware.com/blog/2019/06/07/death-external-storage-end-saga.html
@jcesarmobile jcesarmobile changed the title Improve error message when copyRecursively() fails chore(android): improve error message when copy fails Jul 6, 2020
@jcesarmobile jcesarmobile changed the title chore(android): improve error message when copy fails chore(android): improve error message when Filesystem.copy fails Jul 6, 2020
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good and work fine. I don't find the open failed: EACCES (Permission denied) particularly helpful, but it's better than unknown.

About the copying problems you mention, I had no problem copying a photo taken from Camera plugin to Data directory. Data, Cache and External should be safe to use in future Android releases as they belong to your app.
The folders that were restricted in Android 10 and that could go away in a future release are public folders like the ones used by Documents or ExternalStorage.

@jcesarmobile jcesarmobile merged commit 598d7dc into ionic-team:master Jul 6, 2020
@mepc36
Copy link

mepc36 commented Apr 25, 2021

Hey @jcesarmobile! Does your comment quoted below mean that all we have to do to be able to safely remove android:requestLegacyExternalStorage="true" from our AndroidManifest.xml file is save the media files created by our apps to somewhere other than public folders like Documents or ExternalStorage, such as Data or Cache? Thanks for taking the time to answer!

About the copying problems you mention, I had no problem copying a photo taken from Camera plugin to Data directory. Data, Cache and External should be safe to use in future Android releases as they belong to your app.
The folders that were restricted in Android 10 and that could go away in a future release are public folders like the ones used by Documents or ExternalStorage.

Also thanks for all your hard work on the repo...if I haven't supplied you with enough context to answer my question, just let me know and I'll fill it in ASAP 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants