search

PhotoBackup / client-android

The Android PhotoBackup client, made to free your pictures from your device
https://photobackup.github.io/
GNU General Public License v2.0
48 stars 15 forks source link

Not functioning properly on Android 6 #8

Closed stephanepechard closed 9 years ago

stephanepechard commented 9 years ago

On Android 6 (Marshmallow), the service starts and the test to the server is ok, but the upload journal is not accessible after launch and taking a picture does not trigger the service.

lsowen commented 9 years ago

@stephanepechard Is this due to the new permissions in Android 6? I got a stacktrace like:

11-01 14:24:43.004 5248-5266/? W/System.err: java.lang.SecurityException: Permission Denial: reading com.android.providers.media.MediaProvider uri content://media/external/images/media from pid=5248, uid=10113 requires android.permission.READ_EXTERNAL_STORAGE, or grantUriPermission()

The docs indicate that "dangerous" permissions aren't granted anymore on install, and rather need to use requestPermissions.

I started working to add this, but didn't know what you would think would be the best location:

  1. Request when the user turns on the service in the preference screen
  2. When it first attempts to access the external storage (in PBMediaStore.doInBackground)
  3. Someplace else entirely
stephanepechard commented 9 years ago

Yeah, the problem is probably due to the new permissions system. For me, the best location would be 1. as it does not interfere with the nominal action of taking a picture (in 2.). The thing is, if the user does not give permission, the PB service should not even run, so we should ask for it at the first launch of the service. Don't you think?

lsowen commented 9 years ago

That is what I was thinking as well.

However, what do you think about the corner case when the user goes into settings and revokes the storage permission? Should we then "re-ask" for permission?

stephanepechard commented 9 years ago

Yes, we should. In fact, we should check permission each time the service is started. This is just another check, like checking if server address is set, password is set, etc.

lsowen commented 9 years ago

I created PR #9 to start addressing this. I'm not a java programmer, and even less an Android one, so keep that in mind while reviewing it.

I think we will still need to address checking for permissions in PBMediaStore.doInBackground to make sure the user hasn't revoked the permission, but at least this will prompt them to grant READ_EXTERNAL_STORAGE when attempting to start the service for the first time.

lsowen commented 9 years ago

@stephanepechard Have you had a chance to look over the PR? I only ask because I will have some time this weekend to make any suggested changes/improvements.

stephanepechard commented 9 years ago

Yes, I took a look and it helped me implementing the bugfix. I made it a bit simpler, that's why I didn't pull your contribution. I want to test a bit before commiting, I will to do it this week-end, or even today if I can. Thanks again!

lsowen commented 9 years ago

Excellent, glad I could be of some help!

stephanepechard commented 9 years ago

Fixed in v0.6.5