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

Request READ_EXTERNAL_STORAGE permission on older android versions #6071

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Dec 1, 2023

Reported in https://discord.com/channels/188630481301012481/1097318920991559880/1179849444032790589.
Regressed in #5973.

Since the READ_EXTERNAL_STORAGE permission is not declared, access to storage wouldn't work on older devices. The new permissions used in #5973 only work for newer devices.

Documented (badly) in https://developer.android.com/reference/android/Manifest.permission#READ_MEDIA_IMAGES

For apps with a targetSdkVersion of Build.VERSION_CODES.S_V2 or lower, the READ_EXTERNAL_STORAGE permission is required, instead, to read image files.

They actually meant to say "for apps that will run on Build.VERSION_CODES.S_V2 or lower [...]"

Documented (badly) in https://developer.android.com/reference/android/Manifest.permission#READ_MEDIA_IMAGES
> For apps with a targetSdkVersion of Build.VERSION_CODES.S_V2 or lower, the READ_EXTERNAL_STORAGE permission is required, instead, to read image files.

They actually meant to say "for apps that will run on Build.VERSION_CODES.S_V2 or lower [...]"
@bdach
Copy link
Collaborator

bdach commented Dec 1, 2023

Was any testing done on this? I want to say I'm pretty sure I tested the original change on an android 7 device and saw nothing like what's being reported there.

As much as I dislike android I would like to think they are still able to write accurate documentation.

@Susko3
Copy link
Member Author

Susko3 commented Dec 1, 2023

Tested on an Android 11 device. Current osu! and tests without the change show "No permissions requested" in system settings (file selector doesn't show any files). With the change, the storage permissions is available and granting it makes the file selector work.

@bdach
Copy link
Collaborator

bdach commented Dec 1, 2023

Can this come with an inline comment in the manifest at least then? We're likely never getting rid of that thing anyway but at least that would help explain things given that android docs cannot.

Includes a note for future implementations of runtime permissions checks/requests.
Only relevant for the framework, as consumers will use abstractions.
@peppy peppy merged commit a025d23 into ppy:master Dec 4, 2023
19 of 21 checks passed
@Susko3 Susko3 deleted the fix-older-android-storage branch December 4, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants