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

feat: Allows registering the onRequestPermissionsResult callback. #1818

Merged
merged 6 commits into from
Aug 4, 2019

Conversation

gbm001
Copy link
Contributor

@gbm001 gbm001 commented May 9, 2019

Currently there is no good way to handle runtime permissions in Android in Kivy (unless I've missed it).
You can already check and request runtime permissions; however, you can't receive the callback onRequestPermissionResult which has to be on the Activity.

A number of changes are made to PythonActivity to allow this callback to be accessed. First an interface PermissionsCallback is defined, allowing PyJnius to be used to build a PythonJavaClass to match. This has a single method onRequestPermissionsResult with arguments which match that of the method on Activity. Instances of matching classes can be registered with PythonActivity using the new method addPermissionsCallback.
Finally the method onRequestPermissionsResult is overridden on PythonActivity. This checks if a PermissionCallback has been added using addPermissionsCallback, and if so calls the onRequestPermissionsResult method of that callback, passing all the received arguments. The super method is then called.

Python code in android.permissions then builds a PythonJavaClass to match the interface PermissionsCallback defined in PythonActivity. init accepts a single argument, which should be a python function which will accept three arguments (which will be requestCode, permissions and grantResults). The function register_permissions_callback(callback) provides a simple routine to create an instance of this class, save it in the class itself (to avoid garbage collection) and register the callback function with PythonActivity.

The end user simply needs to create a Python function which will accept requestCode, permissions and grantResults as positional arguments, and then call android.permissions.register_permissions_callback(...) on that function prior to calling android.permissions.request_permissions(...).

One limitation is that if you call request_permissions using SDK_INT < 23, the callback will never happen, but if you do check_permission first (which will return True), and only request permissions if you need to, this will not happen.

Please note I don't know Java so this is potentially hideous code!

@AndreMiras
Copy link
Member

Oh great! I completely missed that one. I'm a bit too busy lately, but I'll try to take a look in the coming weeks.
@Jonast implemented the initial dynamic permission I think so he could provide great feedback too

@ghost
Copy link

ghost commented May 28, 2019

Ok, so request_permissions has this requestCode parameter (in the original java function). Doesn't that allow mapping of the java triggered onRequestPermissionsResult back to the respective request call?

Because if it does, then I really think request_permissions should have a callback= parameter instead of a global register_permissions_callback. The way it's currently set up kind of invites accidentally having two code parts fight over their callbacks or confuse each other with concurrent permission requests.

Other than that this looks really neat! Thanks for exploring how to do this. Do you want to attempt to do a callback-per-request_permissions-call mapping, or should I take a look at how to add that?

@gbm001
Copy link
Contributor Author

gbm001 commented May 28, 2019

There is a requestCode parameter; I originally tried allowing it to be passed but ended up hitting issues because my Java function then had the same name and signature as the original function.

I can have a go at that though? (with passed callbacks rather than just a global callback).

@ghost
Copy link

ghost commented May 28, 2019

@gbm001 I wasn't thinking so much about allowing it to be passed, but rather computing consecutive ids internally and store the respective callback for each id/request sent out in some sort of dict

@inclement inclement changed the base branch from master to develop June 6, 2019 20:40
@gbm001
Copy link
Contributor Author

gbm001 commented Jun 10, 2019

I haven't forgotten about this but probably won't have time to do it for a few weeks unfortunately...

@gbm001 gbm001 force-pushed the gbm_permissions_callback branch from bc608d6 to ecee0e2 Compare July 15, 2019 15:33
Adds an interface in PythonActivity and a method to register a
Python function which will be called when the
onRequestPermissionsResult callback is received.
In android/permissions.py, a new function
'register_permissions_callback' is added to register a Python
function (that takes three arguments) which will receive the
three arguments of onRequestPermissionsResult.
@gbm001 gbm001 force-pushed the gbm_permissions_callback branch from ecee0e2 to 8122126 Compare July 15, 2019 15:46
@gbm001
Copy link
Contributor Author

gbm001 commented Jul 15, 2019

Backend code more complicated now but I think I have preserved backwards compatibility and made it so you can just request permissions and provide a callback?

@ghost
Copy link

ghost commented Jul 15, 2019

@gbm001 oh that looks really nice, well done! Also pretty much exactly how I could see it working in my mind, I like the approach. Maybe you'd still want to rename class onRequestPermissionsManager to class _OnRequestPermissionsManager to make it obvious it's private / not meant to be used from the outside, and to fit more with the general python naming scheme? Otherwise it looks really good, I hope I get around to test it really soon ❤️

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 16, 2019

I renamed the Java callback class to _onRequestPermissionsCallback (since it is mocking a Java instance).
I renamed the Python manager class to _request_permissions_manager since it isn't doing anything with Java and I don't like camelCase in Python.

@ghost
Copy link

ghost commented Jul 16, 2019

I don't like camelCase in Python.

I think camel case is how classes in python are usually named by official style guide, right? I think it makes sense to stick to that. Regarding the java instance and the non-capitalized leading letter, fair enough I suppose that makes sense 😄

Also I don't think this is a merge blocker and I hope this isn't too nitpicky sounding, we just like to have the style somewhat consistent to make it a bit easier to work with for future changes

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 16, 2019

Too much Java has obviously melted my brain; you are quite right (its functions etc. that have camel case names to match the Java that I don't like, but have begrudgingly used where it makes sense). All my own python code has camel case classes...

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 16, 2019

Another thing I should mention is that in order to actually use this I think you have to support pause-resume in your app since requesting permissions seems to pause the app (since it goes to another Android screen to do it). I do it in a 'requesting permissions' screen when my App starts (since I know I will need the permissions) and just wait a second and ask again if the user says 'no' :P

@ghost
Copy link

ghost commented Jul 16, 2019

Oh interesting. So basically when the callback hits the app is still in pause? But then it will resume and the python function will run? Sounds like a weird detail but not like a huge problem

(also kivy may support not blocking in the background/in pause in the future, this is really just a kivy limitation and not python-for-android's issue - e.g. in a custom SDL2 app or using wobblui it is easily possible to keep the python code running while in background)

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 16, 2019

Something like that. And for extra fun the callback is, of course, run on the Java thread and not the Python thread in the same way that app on_pause and on_resume are run on the Java thread (unless it's been changed recently).
My on_resume has a threading lock that is released once the resume has completed, and the permissions_gained callback waits until that is set before calling a second function decorated with @mainthread so it runs in the Python thread. It all gets slightly messy...

@ghost
Copy link

ghost commented Jul 16, 2019

Wait, you have to manually apply that threading lock in your app? Because I can't see what you are mentioning (any on_resume releasing a lock) in the pull request code, so now I'm wondering what will happen if someone just uses it being unaware of that

Edit: oh, you're essentially saying the callback runs in python but per default not on the main thread. That does seem unavoidable with callbacks, so it makes sense that the user would need to deal with it I don't think that can be avoided. I was just confused because you mentioned decorators and I wouldn't have necessarily used them for this, so I was somehow wondering if that was a requirement

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 16, 2019

I do because my 'resume' takes a short while which I need it to finish but I think that's app-specific code. I did debate scheduling, rather than running directly, the passed callback code. Currently though it matches on_resume and on_pause though (unless its been changed recently) in coming from the Java thread not the Python thread. It would certainly need documenting (which sounds like I am volunteering sadly)...

@ghost
Copy link

ghost commented Jul 16, 2019

I did debate scheduling, rather than running directly, the passed callback code

I wouldn't see how one could, the problem is that the main loop is owned by the application in any arbitrary python app. For kivy the main loop is owned by kivy, but python-for-android is used for non-kivy apps, so there is no universal way to schedule anything as far as I am aware 🤷‍♀️

So yeah, it sounds like it needs to be documented. I wouldn't mind doing that actually, I kinda like writing docs. I never tried this but can you try giving me write access to the pull request? I don't know how this works but I think somehow you can enable write access for contributors and then maybe I can possibly commit to it as well. I'll ask the others how this works I think they've tried it 😄

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 17, 2019

I've got the 'allow edits from maintainers' box ticked, so I think you should be able to edit?

AndreMiras
AndreMiras previously approved these changes Jul 17, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work guys!
I still have to test it on device, but it looks neat, thanks

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 17, 2019

I realised I had put nice docstrings on the internal class, and not the public API... so I wrote some. Then I realised the output from the callback is non-Pythonic, so I fixed it...

The callback returns a list of strings (permission strings) and integers (one per permission string). In Android it is 0 if permission is granted and -1 if denied, so I have made it change this to True (for granted) or False (for denied) instead.

I also added a comment with the thread lock which hopefully makes sense.

@Fak3
Copy link
Contributor

Fak3 commented Jul 28, 2019

It does not work on android 5.1 - the callback is never called.
On android 8.1 works fine.

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 29, 2019

It does not work on android 5.1 - the callback is never called.
On android 8.1 works fine.

Hi,

That's expected and described in the docstrings (for SDK_INT < 23) although possibly not enormously logical. On Android < 6 (SDK_INT < 23) run-time permissions don't exist so any app requesting permissions in the manifest should have them. Thus running 'check_permissions' for a permission should return True (i.e. has permissions) and so there is no need to run 'request_permissions'.

I guess an alternative option would be to assume success and call the callback immediately for Android < 6. That would mean constructing a grantResults and permissionsGranted list. It might mean changing the PythonActivity methods slightly so that requestPermissions returned 'true' or 'false' (or similar) to differentiate 'returned immediately for SDK_INT < 23' from 'requested permissions, expect a callback' so that the Python can then directly trigger the callback on < Android 6.

I am assuming the issue is the targeted version of Android, not the version actually running on a phone though - so people building for < Android 6 don't have the run-time permissions problem and people building for > Android 6 will always get a callback?

@ghost
Copy link

ghost commented Jul 29, 2019

I am assuming the issue is the targeted version of Android, not the version actually running on a phone though

It should be the runtime version affecting this, so one way to fix this would be to check the runtime version via pyjnius as e.g. done here: https://github.com/kivy/python-for-android/blob/develop/pythonforandroid/recipes/android/src/android/storage.py#L10 - and then to call the callback immediately directly in the python code if it's smaller than 23. This should be simpler than bothering to handle this on the Java side

@gbm001
Copy link
Contributor Author

gbm001 commented Jul 29, 2019

I can't test < Android 6 easily but hopefully this should call the callback immediately (with a positive response) in that case?

@AndreMiras
Copy link
Member

This is next one I'd like to look into. Hopefully today or tomorrow I'll make the final tests on device and merge it.
Thanks again for the work and patience 🍻

@AndreMiras AndreMiras self-assigned this Aug 4, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Tested successfully on Android 7

@AndreMiras AndreMiras merged commit 588d65d into kivy:develop Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants