-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Remove suitability check for calling object after permission result callback #39
Conversation
I am open to allowing objects not in the Do you have any alternative way to achieve this without complicating the API surface? After all it is called easy permissions and this is a somewhat advanced use case. |
You reflected my concern to the point - what makes it easy but not complicated? When thinking about it, I believe making the I could make an edit to this PR in order to create an example, if that's OK with you. As a side question, do you have a roadmap or any rough plans with this lib? It might be beneficial for you and the lib to create some ground rules over which we can discuss further development. |
I took the liberty to create an example of what I meant. The following changes are relevant:
EasyPermissions.onRequestPermissionsResult(
requestCode,
permissions,
grantResults,
activity, fragment, featureComponent // vararg
) It looks really simple and it does not break any existing usage. I do hope it does not clash with some of your plans. |
distributionBase=GRADLE_USER_HOME | ||
distributionPath=wrapper/dists | ||
zipStoreBase=GRADLE_USER_HOME | ||
zipStorePath=wrapper/dists | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-2.10-all.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revert this back? EasyPermissions has some outstanding issues with 2.14.1
@jmarkovic with the varargs param this looks much better! I am happy to merge it, given my comments above. Let's just delete the sample pieces and then this is good to go. |
Awesome! Thanks for sticking through this. I am going to use the 'squash and merge' option here to make the 6 commits into one, so you may have to update your fork. |
@samtstern I believe I reverted everything you asked me. It might be wise for you to squash this. |
@jmarkovic you read my mind, thanks again! |
No problem. I find this lib very useful so I plan to stick with it for quite some time :) |
With the current implementation, EasyPermissions library assumes the calling object (actually, the object provided in
EasyPermissions.onRequestPermissionsResult
method) is of typeActivity
orFragment
. While the assumption may be valid, it can be beneficial for the user to forward the response to another component that is not anActivity
nor aFragment
.This PR removes the call to
EasyPermissions#checkCallingObjectSuitability(Object object);
fromonRequestPermissionsResult
. I have not noticed the need to perform this check at this callback method since the object is never cast to Activity. With this, any object can be provided as long as it either has a method withAfterPermissionGranted
annotation or it implementsPermissionCallbacks
interface.There's also a question - is there a plan for a feature/fix that would require this object to be an
Activity
inside this method? I fail to see a good reason for that.Usecase
I've put a very trivial example of how this could be used. There's a new class called
FeatureComponent
which requires a permission to run some logic. Traditionally, this is communicated to theActivity
ofFragment
. With EasyPermissions, this isn't required, except the permission callback receiver must be anActivity
which would then forward all events back to the component. With this check removed, this is no longer required - component itself can take these callbacks.A few issues I've noticed
With this approach, the object which calls the request for permissions still has to override
onRequestPermissionsResult
, which isn't surprising. But, instead of specifyingthis
, it has to provide the exact component which expects the result. For example:Example app also shows that the call to
EasyPermissions.onRequestPermissionsResult
must be repeated with different objects supplied. Example is below:While this isn't really an issue in this example, having more than two components might not be pretty. Could we consider having a
vararg
parameter for objects instead?Another issue - when a component implements
PermissionCallbacks
, they need to implement a method fromActivityCompat.OnRequestPermissionsResultCallback
. Is this extension really needed for this interface? I've checked, its method is never called in the library.