[3.x] Add partial support for Android scoped storage

[m4gr3d]

This is done by restricting read/write operations to the app specific directories. With that out of the way, we're also able to bump the target sdk version to 30!

With this change, the behavior leans closer to iOS since there, everything is saved to an app specific directory and get removed on uninstall. This may constitute a breaking change since the location of saved contents has been moved, so I'm open to feedback whether this should be cherry-picked for 3.3.x.

In addition, we're also bumping the min sdk version to 19 as version 18 is no longer supported by Google Play Services and only add an additional 0.3% of device coverage: 98.4% -> 98.1%.

Fix #38913, #45467
Revert #49436
Supersede #50332

[m4gr3d]

@HEAVYPOLY Seems like we may need to follow the iOS approach here and only use app specific directories.
There seem to be a way to keep the saved data even after app uninstall, so let me know if you think that may make things easier and I'll create a separate PR for that option.

[madmiraal]

To support the Android storage updates (which will include enforcing scoped storage with SDK 30, and will be required from 21 August 2021) will require a lot more work than this PR.

Scoped storage basically means that applications (that are not data management applications) will no longer be granted WRITE_EXTERNAL_STORAGE permissions. To make this distinction clear, it has been replaced with the new MANAGE_EXTERNAL_STORAGE permission in SDK 30. With SDK 29 the revoking of WRITE_EXTERNAL_STORAGE permission could be delayed by setting the requestLegacyExternalStorage attribute which we did with Godot. However, with SDK 30, scoped storage will be enforced and Godot applications will no longer be granted WRITE_EXTERNAL_STORAGE permissions even with the requestLegacyExternalStorage attribute. Although, we can still set the new preserveLegacyExternalStorage permission to allow previously installed applications to continue to access data they previously had access to, which I suggest we do. Note: It will have no effect on new installs, which will be forced to use scoped storage.

With scoped storage Godot applications will be restricted to writing to the four application specific directories:

1. User files directory: currently obtained using get_user_data_dir(). This location and data will always be available to the application. It counts towards the application data size, is inaccessible to other applications and will be encrypted on Android 10 and above.
2. User cache directory: currently not obtainable, but should be implemented in Android now that Expose OS data directory getter methods #48544 has exposed OS.get_cache_dir(). This location is always available to the application, but Android may delete the data if available storage is low. As with the user data directory, it also counts towards the application data size, is inaccessible to other applications and will be encrypted on Android 10 and above.
3. User external files directory: currently obtained using OS.get_external_data_dir(); therefore we should definitely not be reverting Add OS.get_external_data_dir() to get Android external directory #49435. This location may not be available, but if it is, the data stored there will always be available to the application. Other applications with the required permissions can access this data.
4. User external cache directory: currently not obtainable, but should be added using, for example, OS.get_external_cache_dir(). This location may not be available, and if it is, Android may delete the data if available storage is low.

This has all been available since API level 8, and has not been affected by scoped storage. Note: external storage permissions were required to access the external storage locations before API 19, but setting minimumSDK to 19 will solve this. All these application specific data locations are associated with the application and will be deleted when the application is uninstalled.

The big challenge with scoped storage that needs to be addressed is making data available to other applications i.e. the problem raised with #45467 (changing it from just requiring documentation to a bug again) and I assume the driver behind #38913. Before SDK 29, media files could be shared with other applications by saving them to the media specific sub-directories under the directory returned from getExternalStoragePublicDirectory() and currently accessed using OS.get_system_dir(). This required the WRITE_EXTERNAL_DATA permission that is being revoked going forward. There was another problem that was never addressed (or even raised in #45467): once the data is saved there it requires the media to be scanned (using MediaScannerConnection.scanFile()) before it is available to other applications. Fortunately, this is done automatically by the device when it is rebooted, which provided a workaround, albeit a unsatisfactory one.

With scoped storage, the only way to save a file so it is available to other applications (and ensure it is not deleted when the application is uninstalled) we need to use the MediaStore API. With SDK 29, this is no longer restricted to just media files (Audio, Images and Video), but any type of file can now be saved to the MediaStore.Downloads collection.

Fortunately, Godot currently only provides the ability to save one time of media file: Images. An image can be saved as a PNG file using Image.save_png(). Unfortunately, the core code that enables this, currently uses the base FileAccess class to open the file that is written to:

godot/drivers/png/resource_saver_png.cpp

Lines 51 to 70 in 9d4afa8

	Error ResourceSaverPNG::save_image(const String &p_path, const Ref<Image> &p_img) {
	Vector<uint8_t> buffer;
	Error err = PNGDriverCommon::image_to_png(p_img, buffer);
	ERR_FAIL_COND_V_MSG(err, err, "Can't convert image to PNG.");
	FileAccess *file = FileAccess::open(p_path, FileAccess::WRITE, &err);
	ERR_FAIL_COND_V_MSG(err, err, vformat("Can't save PNG at path: '%s'.", p_path));
	const uint8_t *reader = buffer.ptr();
	file->store_buffer(reader, buffer.size());
	if (file->get_error() != OK && file->get_error() != ERR_FILE_EOF) {
	memdelete(file);
	return ERR_CANT_CREATE;
	}
	file->close();
	memdelete(file);
	return OK;
	}

So it can’t be simply overridden in the Android inherited version:

godot/platform/android/file_access_android.cpp

Lines 44 to 61 in 9d4afa8

	Error FileAccessAndroid::_open(const String &p_path, int p_mode_flags) {
	String path = fix_path(p_path).simplify_path();
	if (path.begins_with("/"))
	path = path.substr(1, path.length());
	else if (path.begins_with("res://"))
	path = path.substr(6, path.length());
	ERR_FAIL_COND_V(p_mode_flags & FileAccess::WRITE, ERR_UNAVAILABLE); //can't write on android..
	a = AAssetManager_open(asset_manager, path.utf8().get_data(), AASSET_MODE_STREAMING);
	if (!a)
	return ERR_CANT_OPEN;
	//ERR_FAIL_COND_V(!a,ERR_FILE_NOT_FOUND);
	len = AAsset_getLength(a);
	pos = 0;
	eof = false;
	return OK;
	}

Note: The Android inherited version currently doesn’t even support opening the file for writing:

godot/platform/android/file_access_android.cpp

Lines 51 to 54 in 9d4afa8

	ERR_FAIL_COND_V(p_mode_flags & FileAccess::WRITE, ERR_UNAVAILABLE); //can't write on android..
	a = AAssetManager_open(asset_manager, path.utf8().get_data(), AASSET_MODE_STREAMING);
	if (!a)
	return ERR_CANT_OPEN;

To support saving files that can be shared with other applications, we need to:

1. Create the Java files to write the file to the appropriate MediaStore depending on the MIME type.
2. Allow the Godot Android FileAccess class to call this method when saving a file that should be publicly accessible.
3. Create the Godot interface to be able to save files to the public locations (across platforms).
4. Update the ResourceSaverPNG code to not use the base FileAccess class so that platforms can override it.
5. Optionally, create similar code to enable saving of other media files both privately and publicly.

[m4gr3d]

To support the Android storage updates (which will include enforcing scoped storage with SDK 30, and will be required from 21 August 2021) will require a lot more work than this PR.

Scoped storage basically means that applications (that are not data management applications) will no longer be granted WRITE_EXTERNAL_STORAGE permissions. To make this distinction clear, it has been replaced with the new MANAGE_EXTERNAL_STORAGE permission in SDK 30. With SDK 29 the revoking of WRITE_EXTERNAL_STORAGE permission could be delayed by setting the requestLegacyExternalStorage attribute which we did with Godot. However, with SDK 30, scoped storage will be enforced and Godot applications will no longer be granted WRITE_EXTERNAL_STORAGE permissions even with the requestLegacyExternalStorage attribute.

Yes, I'm aware of the changes brought on by scoped storage. The current approach is a pragmatic one based on the fact the other supported mobile platform (iOS) behave similarly. As such with this approach, the mobile platforms have the same behavior, which is that files are read/written from app specific storage and removed on uninstall.

It is possible to support writing/reading from shared storage (it's obviously more work :)) so the approach here is to align the two mobile platforms first, then start the discussion on whether supporting writes/reads to shared storage is desirable. Since Godot's primary audience is game developers (with games having little need to access shared contents), having that discussion allows us to see if there's a need for this functionality, and what additional requirements it should fulfill.

Although, we can still set the new preserveLegacyExternalStorage permission to allow previously installed applications to continue to access data they previously had access to, which I suggest we do. Note: It will have no effect on new installs, which will be forced to use scoped storage.

This is not as straightforward as this is allowed temporarily (I suspect it'll be disabled in upcoming Android versions as well) to allow apps to migrate their data. As such we'd need to provide the ability for that data migration as well. Hence, as I mentioned, the need to discuss to see if it's needed, and figure out its requirements.

With scoped storage Godot applications will be restricted to writing to the four application specific directories:

1. User files directory: currently obtained using get_user_data_dir(). This location and data will always be available to the application. It counts towards the application data size, is inaccessible to other applications and will be encrypted on Android 10 and above.
2. User cache directory: currently not obtainable, but should be implemented in Android now that Expose OS data directory getter methods #48544 has exposed OS.get_cache_dir(). This location is always available to the application, but Android may delete the data if available storage is low. As with the user data directory, it also counts towards the application data size, is inaccessible to other applications and will be encrypted on Android 10 and above.

This is added in this PR.

3. User external files directory: currently obtained using OS.get_external_data_dir(); therefore we should definitely not be reverting Add OS.get_external_data_dir() to get Android external directory #49435. This location may not be available, but if it is, the data stored there will always be available to the application. Other applications with the required permissions can access this data.

This is also exposed in this PR using the get_system_dir(SystemDir) method, hence why the get_external_data_dir() method is no longer needed.

One option here to support shared storage access would be to add additional SystemDir enums and support them (e.g: SYSTEM_DIR_SHARED_PICTURES, SYSTEM_DIR_SHARED_DOCUMENTS, etc...).

4. User external cache directory: currently not obtainable, but should be added using, for example, OS.get_external_cache_dir(). This location may not be available, and if it is, Android may delete the data if available storage is low.

I don't really see a need for this one, since the get_cache_dir() should do the job.

This has all been available since API level 8, and has not been affected by scoped storage. Note: external storage permissions were required to access the external storage locations before API 19, but setting minimumSDK to 19 will solve this. All these application specific data locations are associated with the application and will be deleted when the application is uninstalled.

hasFragileUserData allows the user to keep the app's data, albeit this only available for Android 10 and above.

The big challenge with scoped storage that needs to be addressed is making data available to other applications i.e. the problem raised with #45467 (changing it from just requiring documentation to a bug again) and I assume the driver behind #38913. Before SDK 29, media files could be shared with other applications by saving them to the media specific sub-directories under the directory returned from getExternalStoragePublicDirectory() and currently accessed using OS.get_system_dir(). This required the WRITE_EXTERNAL_DATA permission that is being revoked going forward. There was another problem that was never addressed (or even raised in #45467): once the data is saved there it requires the media to be scanned (using MediaScannerConnection.scanFile()) before it is available to other applications. Fortunately, this is done automatically by the device when it is rebooted, which provided a workaround, albeit a unsatisfactory one.

With scoped storage, the only way to save a file so it is available to other applications (and ensure it is not deleted when the application is uninstalled) we need to use the MediaStore API. With SDK 29, this is no longer restricted to just media files (Audio, Images and Video), but any type of file can now be saved to the MediaStore.Downloads collection.

Fortunately, Godot currently only provides the ability to save one time of media file: Images. An image can be saved as a PNG file using Image.save_png(). Unfortunately, the core code that enables this, currently uses the base FileAccess class to open the file that is written to:

godot/drivers/png/resource_saver_png.cpp

Lines 51 to 70 in 9d4afa8

	Error ResourceSaverPNG::save_image(const String &p_path, const Ref<Image> &p_img) {
	Vector<uint8_t> buffer;
	Error err = PNGDriverCommon::image_to_png(p_img, buffer);
	ERR_FAIL_COND_V_MSG(err, err, "Can't convert image to PNG.");
	FileAccess *file = FileAccess::open(p_path, FileAccess::WRITE, &err);
	ERR_FAIL_COND_V_MSG(err, err, vformat("Can't save PNG at path: '%s'.", p_path));
	const uint8_t *reader = buffer.ptr();
	file->store_buffer(reader, buffer.size());
	if (file->get_error() != OK && file->get_error() != ERR_FILE_EOF) {
	memdelete(file);
	return ERR_CANT_CREATE;
	}
	file->close();
	memdelete(file);
	return OK;
	}

So it can’t be simply overridden in the Android inherited version:

It can be overridden. FileAccess (and DirAccess) creates platform specific handles to open a file.

godot/platform/android/file_access_android.cpp

Lines 44 to 61 in 9d4afa8

	Error FileAccessAndroid::_open(const String &p_path, int p_mode_flags) {
	String path = fix_path(p_path).simplify_path();
	if (path.begins_with("/"))
	path = path.substr(1, path.length());
	else if (path.begins_with("res://"))
	path = path.substr(6, path.length());
	ERR_FAIL_COND_V(p_mode_flags & FileAccess::WRITE, ERR_UNAVAILABLE); //can't write on android..
	a = AAssetManager_open(asset_manager, path.utf8().get_data(), AASSET_MODE_STREAMING);
	if (!a)
	return ERR_CANT_OPEN;
	//ERR_FAIL_COND_V(!a,ERR_FILE_NOT_FOUND);
	len = AAsset_getLength(a);
	pos = 0;
	eof = false;
	return OK;
	}

Note: The Android inherited version currently doesn’t even support opening the file for writing:

The current Android version only handles res:// paths, which by being in the Android assets directory are immutable at runtime.
If we decide to provide support for shared storage, then we'll need a new (java) class that'll handle all supported Godot's access type.

godot/platform/android/file_access_android.cpp

Lines 51 to 54 in 9d4afa8

	ERR_FAIL_COND_V(p_mode_flags & FileAccess::WRITE, ERR_UNAVAILABLE); //can't write on android..
	a = AAssetManager_open(asset_manager, path.utf8().get_data(), AASSET_MODE_STREAMING);
	if (!a)
	return ERR_CANT_OPEN;

To support saving files that can be shared with other applications, we need to:

1. Create the Java files to write the file to the appropriate MediaStore depending on the MIME type.
2. Allow the Godot Android FileAccess class to call this method when saving a file that should be publicly accessible.
3. Create the Godot interface to be able to save files to the public locations (across platforms).
4. Update the ResourceSaverPNG code to not use the base FileAccess class so that platforms can override it.
5. Optionally, create similar code to enable saving of other media files both privately and publicly.

To recap:

• This PR is a pragmatic one that adds support for scoped storage primarily for apps/games that don't need to read/write to shared storage and allows convergence of behavior between mobile platforms
• It's possible through get_system_dir(SystemDir) to add support for shared storage. If we decide that's a need, this can be done in a separate PR after we nail down the requirements.

Thoughts and feedback welcomed!

[madmiraal]

Saying it's not needed is effectively saying: "won't fix" #38913, #39414 and #45467.

[m4gr3d]

Saying it's not needed is effectively saying: "won't fix" #38913, #39414 and #45467.

I'm not seeing where I stated that. Again, (constructive) thoughts and feedback are welcomed, (invalid) blanket statements about contributors' intentions are not.

We all contribute to this project with good faith in mind, so I'll go ahead and assume you had a more constructive feedback in mind when you wrote that response. If that's the case, please lay it out.

[madmiraal]

@m4gr3d Saying that my statements are "invalid" is an insult and is not in line with the Godot's code of conduct: "Always assume positive intent from others." and is definitely not assuming I had more constructive feedback in mind.

I made no statements about your intentions. I merely pointed out the effect statements like these:

get_external_data_dir() method is no longer needed.

and

If we decide to provide support for shared storage

would have in a PR that would revert the fix for #39414 and close #38913 and #45467 without properly fixing them.

I clearly laid out my thoughts on why this PR does not fix #38913 and #45467, and reverts the fix for #39414, and provided constructive feedback on how to properly fix #38913 and #45467 in my review. This review and its suggestions remain valid.

[akien-mga]

I clearly laid out my thoughts on why this PR does not fix #38913 and #45467, and reverts the fix for #39414, and provided constructive feedback on how to properly fix #38913 and #45467 in my review. This review and its suggestions remain valid.

And you haven't addressed any of @m4gr3d's detailed answer to your review, and chose instead to discard it all with (indeed) a blanket statement. Whether it's valid or not is up for discussion and saying that it's invalid (i.e. that @m4gr3d's proposed pragmatic solution does not "won't fix" the referenced issues) is not an insult nor a breach of Code of Conduct.

Attempting to construe it as such seems disingenuous - and sadly it's not a first occurrence coming from you. I have to ask you once again to stop using this approach - which has blocked countless PRs - of giving contributors silent treatment after they replied in-depth to your own in-depth negative review.

[madmiraal]

Both #38913, as detailed in this comment and #45467, as detailed in this comment and the following one describe the need for saving files to shared storage. However, this PR closes those two issues without providing a solution to saving files to shared, scoped storage. In my review, I outlined what I think needs to be done to enable saving files to shared, scoped storage, but this was dismissed with: "If we decide that's a need, this can be done in a separate PR". Is it unreasonable to conclude that a decision has been made to not fix these issues? How is pointing this out an "invalid, blanket statement"?

Regarding @m4gr3d's other responses to my review: I fully understand the urgent need to update the Android builds to use SDK 30. I don't agree that this PR "Adds support for Android scoped storage" i.e. it does not fix #38913 and #45467. I agree that get_system_dir() should (somehow, but I don't yet know how) be used for saving to shared storage as it does for all other platforms. However, I don't agree that it should also be used to provide access to the apps private, external data location (or that the fix for #39414 should be reverted). The user's external cache directory would be used for saving large temporary files e.g. play recordings. And, finally, I know there is an Android override for FileAccess::open(), I pointed to it in my review, but ResourceSaverPNG::save_image() does not use it, which is what I tried to highlight in my review.

@akien-mga I find it unfair to assume that I can provide a prompt, detailed responses to every statement, when I do this voluntarily in my spare time; especially when I have already provided detailed feedback and feel that I would only be repeating myself. My own comments and responses are often ignored for months by people paid to work on this project. Suggesting that I've "blocked countless PRs" is absurd. I have always acted in the best interests of this project. However, there is something seriously wrong with a culture where negative reviews or disagreeing with certain people is discouraged; a culture where I'm somehow expected not to be upset when accused of being disingenuous; or a culture where anyone is ever discouraged from calling out other people's violations to the code of conduct. For the record, even if it was okay to tell people not to call out other people's violations of the code of conduct, you've never asked me not to do so before, so saying "once again" is not true. The only other time I've pointed out a violation of the code of conduct, was to complain about bullying, which, according to your comment appears to be not only condoned, but endemic in the culture.

[m4gr3d]

To follow up on the points raised in the discussion here:

• Yes, this PR does not fully resolve scoped storage support for Godot on Android.
• Yes, we're still planning to add full support for it, however the implementation (and subsequent testing) is expected to take some time as it's a large refactor (anyone with a better implementation is welcome to contribute), so in the meantime, we're going with this approach in order to unblock support for SDK 30.
• Feedback and comments are welcomed so long as the above two points are taken into consideration.

Thanks!

[m4gr3d]

@godotengine/android @akien-mga I'm looking for feedback on this approach to easily provide support for shared and app specific storage.
For most platform, this argument doesn't make a difference (hence default to true). On Android (and maybe iOS), we send different paths depending on whether this is true or false.

The alternatives to this approach were either:

• Create an app specific dir enum that mirrored the SystemDir enum
• Or add app specific dir values to the SystemDir enum

[akien-mga]

I think the boolean option is fine for 3.x (and can be forward ported to master too for consistency initially).

We might want to take time to rethink this API for 4.0 though, get_system_dir() is a very old method and it might not expose the best API to handle current features and requirements of modern OSes.

But it's also a fairly niche method that only a handful of users will need to call explicitly, which is why I think adding an Android-specific boolean flag in the short term is OK.

[akien-mga]

Code looks good to me.

[akien-mga]

To follow up on the points raised in the discussion here:

• Yes, this PR does not fully resolve scoped storage support for Godot on Android.
• Yes, we're still planning to add full support for it, however the implementation (and subsequent testing) is expected to take some time as it's a large refactor (anyone with a better implementation is welcome to contribute), so in the meantime, we're going with this approach in order to unblock support for SDK 30.

That's also my understanding and the approach I recommend as we urgently need support for SDK 30 (some users are already stuck publishing new apps due to the new requirements).

I think most of the discussion on technical points boiled down to the fact that this PR says that it "adds support for Android scoped storage" and aims at closing issues which request "support for Android scoped storage". While this PR adds support for (as I understand it) scoped storage for app-specific storage, but not the full blown API.

So it's mostly a question of semantics IMO, whether we believe that supporting scoped storage means "porting existing features to use the scoped storage API" (what this PR does) or "implementing new features offered by the scoped storage API" (what should be done eventually).

I would suggest making sure that we have an issue or proposal that describes what is left to be done after the initial steps taken in this PR. I don't see any harm in doing things progressively (and that's what I advocated for both publicly and in private discussions), but I understand Marcel's disagreement with the fact that this fully implements scoped storage support for all use cases.

[akien-mga]

I made a 3.3 backport of this PR in my fork, to use as a base for test builds of 3.3.3 RC with support for SDK 30: https://github.com/akien-mga/godot/tree/3.3-scoped-storage

[akien-mga]

I made a 3.3 backport of this PR in my fork, to use as a base for test builds of 3.3.3 RC with support for SDK 30: https://github.com/akien-mga/godot/tree/3.3-scoped-storage

Haven't heard back on my testing build in #48636 (comment), so I'll go ahead and merge this in 3.x and 3.3 (will do a backport PR) so we can get it tested more in 3.3.3 RC 2 today/tomorrow.

[akien-mga]

Thanks!

[ondesic]

Just a clarification. When Scoped Storage if fully supported, we should be able to save files to, for instance, the android "Documents" folder and be able to access this app folder from inside the app as well? I have been watching Scoped storage support closely and want to make sure I understand the end result.

[m4gr3d]

@ondesic Yes access to the Documents directory will be feasible.

Here's the WIP PR for full support, but it still needs to go through testing and validation.

[ghost]

Hey sir. Can you please implement in app rating system for Android.