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.

Sign up for GitHub

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

Jump to bottom

Fix downloads not working below Android Q by properly requesting storage permissions #91

Merged
merged 5 commits into from
Sep 4, 2020
Merged

Fix downloads not working below Android Q by properly requesting storage permissions #91

merged 5 commits into from
Sep 4, 2020

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented Sep 4, 2020 •
edited by Maxr1998
Loading

Fixes #72

@nielsvanvelzen nielsvanvelzen mentioned this pull request Sep 4, 2020
Closed
@Maxr1998
Copy link
Member

Maxr1998 commented Sep 4, 2020 •
edited
Loading

The tools:node="remove" part will remove this permission when compiling, idk why shouldn't do that.

May fix #72 , didn't test

Oh that's dumb, I didn't think using the download manager requires storage permissions. I mean, normally it doesn't, but apparently some OEMs still require it.
EDIT: it's isn't required from Android Q onwards, but necessary before that (source). I'll fix that later.

I originally added the tools:remove because I didn't want the permission declared by LeakCanary when the app itself doesn't need it.

Regardless, only declaring the permission isn't enough on Android M and above, we need another permission prompt. I'd prefer though to only ask when the app actually needs it, maybe with a try/catch for the SecurityException.
I need to investigate this issue a bit more.

@nielsvanvelzen
Copy link
Member Author

So the proper fix is to check if the permission is granted when the download button is pressed and if it's not, ask for it first?

I can do that

@Maxr1998
Copy link
Member

Maxr1998 commented Sep 4, 2020

So the proper fix is to check if the permission is granted when the download button is pressed and if it's not, ask for it first?

I can do that

Yep, but only below Android Q.

Maxr1998
Maxr1998 requested changes Sep 4, 2020
View reviewed changes
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
@nielsvanvelzen nielsvanvelzen force-pushed the nielsvanvelzen-storage-permission branch from cfffa92 to 75f2943 Compare September 4, 2020 17:02
@nielsvanvelzen nielsvanvelzen marked this pull request as draft September 4, 2020 17:03
@nielsvanvelzen nielsvanvelzen force-pushed the nielsvanvelzen-storage-permission branch from 75f2943 to 0b9dc23 Compare September 4, 2020 17:18
@nielsvanvelzen nielsvanvelzen marked this pull request as ready for review September 4, 2020 17:18
@nielsvanvelzen
Copy link
Member Author

Redid the PR to actually requets the permission.

Maxr1998
Maxr1998 requested changes Sep 4, 2020
View reviewed changes
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/mobile/utils/SystemUtils.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/mobile/utils/PermissionRequestHelper.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/mobile/ApplicationModule.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/mobile/MainActivity.kt Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/mobile/utils/SystemUtils.kt Outdated Show resolved Hide resolved
@Maxr1998
Improve PermissionsHelper
8bb987c 
Use typealias for callback, use SparseArray instead of Map for callback storage, make request code generation atomic
@Maxr1998
Remove unused import
8b854e1
@Maxr1998
Improve download code
ae040df 
Set timeout for permission acceptance, ensure dialog coroutine always finishes (by adding a dismiss listener)
@nielsvanvelzen
Copy link
Member Author

I can't approve since I opened the PR but your changes look good to me @Maxr1998

@Maxr1998 Maxr1998 changed the title Fix storage permissions in manifest Fix downloads not working below Android Q by properly requesting storage permissions Sep 4, 2020
@Maxr1998 Maxr1998 merged commit 932e9f8 into master Sep 4, 2020
@Maxr1998 Maxr1998 deleted the nielsvanvelzen-storage-permission branch September 4, 2020 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maxr1998 Maxr1998 Maxr1998 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Downloads broken on Android versions below 10
2 participants
@nielsvanvelzen @Maxr1998