search

vanilla-music / vanilla

Vanilla Music Player for Android
GNU General Public License v3.0
1.19k stars 296 forks source link

All files access permission #1164

Closed reactorcoremeltdown closed 1 year ago

reactorcoremeltdown commented 1 year ago

Fixes #1142

adrian-bl commented 1 year ago

How does that work? Don't we have to ask for this permission first?

reactorcoremeltdown commented 1 year ago

@adrian-bl it does not explicitly ask for the permission, but allows for selecting it in the application's properties.

image

If a dialogue for requesting exactly this permission is required, I'll try my best to submit a patch. Just let me know!

adrian-bl commented 1 year ago

Since this is about exporting playlists: Maybe we could add something in the settings?

If the permission is not granted, we could show an option to open the preferences and grant permissions. wdyt? (I doubt that users would figure out that on their own)

reactorcoremeltdown commented 1 year ago

Makes sense! I am mostly a monkeypatcher, I have zero experience in Java, but I am clever enough to see how others do that. I'll try to add a menu item in that activity and get back to you!

reactorcoremeltdown commented 1 year ago

@adrian-bl I finally made it, please review.

screen-20230425-203509 mp4

adrian-bl commented 1 year ago

thanks, lgtm! Lets merge this and give it a go

adrian-bl commented 1 year ago

Well, unfortunately i need to roll this back :-/

Seems that the play store won't allow me to upload an APK which requests this permission:

image

And i don't think we qualify for an exemption: https://support.google.com/googleplay/android-developer/answer/10467955?visit_id=638184408302430276-147505165&rd=1#zippy=%2Cpermitted-uses-of-the-all-files-access-permission

It therefore seems that the only way out here is to use SAF or somehow find another way to write the files without making Android unhappy

xeruf commented 1 year ago

Is there an APK build available for the meantime?

reactorcoremeltdown commented 1 year ago

@adrian-bl I totally understand your concerns and will try my best to address it. Thank you very much for helping me with this journey!

@xeruf while I can easily build an apk and hand it over to you, I think it can cause problems like conflicting signing keys.

I think a middle ground solution could be publishing a nightly apk, or a pre-release to F-Droid. I understand the concerns also here (adding and removing a feature with a potential of breaking it), but at the same time we're introducing a 100% opt-in feature that we can phase out later on. @adrian-bl wdyt?

xeruf commented 1 year ago

That sounds good, I use F-Droid anyways :)

adrian-bl commented 1 year ago

I think there is actually a way to make this all work without requiring any permissions - but playlist export will be restricted to /sdcard/{Music,Recordings,Ringtones}/...

I have a halfway working prototype but wont be able to work on it this week. Maybe next week i'll have something.

On May 1, 2023 12:58:13 PM GMT+02:00, Janek @.***> wrote:

That sounds good, I use F-Droid anyways :)

-- Reply to this email directly or view it on GitHub: https://github.com/vanilla-music/vanilla/pull/1164#issuecomment-1529573956 You are receiving this because you were mentioned.

Message ID: @.***>

adrian-bl commented 1 year ago

https://github.com/vanilla-music/vanilla/commit/76d1afb7cbdab152391256cb428aa7e856ea06b3

This mostly works and uses the fact that Android still allows us to write certain file types to media directories. What doesn't work is overwriting/changing playlist files created by other applications - not sure about MTP .. needs some testing on real hardware but i'm still tempted to merge this as-is

jameskirkwood commented 1 year ago

@adrian-bl I observed the same on hardware with your implementation. It's also not possible to overwrite or delete playlists created through MTP either. However it is possible to create a writable copy by renaming in Vanilla. One can then delete the original externally and use Vanilla to rename the copy back to its original name. Renaming over the top of an externally owned file using Vanilla seems to have no effect other than replacing the Vanilla owned file with a backup.

I can deal with this limitation but it would be ideal if the externally owned or otherwise unwritable playlists were recognised as such, and any actions that would attempt to overwrite or delete them disabled, except perhaps 'rename' which essentially becomes 'make a copy'. Although, the latter would be a handy operation to also allow for Vanilla owned playlists.

If someone tries to modify a playlist and it fails, it would be helpful to prompt for a different name so they can at least save their change and maybe later delete the old one and rename the copy if they want.