Add new builder API and support customizing rationale dialog theme #180

SUPERCILEX · 2017-12-03T01:30:27Z

@samtstern What do you think of this PR? I'm using our new builder style to add the theme option. If you think the idea is good, I'll add documentation and all that good stuff and prep this PR for merging.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-12-04T23:33:29Z

@SUPERCILEX this is a nice API but I am wary of making a breaking change for this (relatively) minor feature request of theme changing. The API right now is simple and has a lot of users.

Is there anything else you think we'd add to the API in the future if it was Builder-style?

SUPERCILEX · 2017-12-05T01:11:05Z

@samtstern Nope, I can't think of anything. However, here's my reasoning:

requestPermissions has way too many parameters: a common limit I've seen is 3 and then you consider a different way of passing in parameters. In our case, we can go up to 5 plus the varargs one which is unlimited. When you consider adding yet another parameter (now 6), things get tearfully messy.
We already have way too many int parameters: the signature = requestPermissions(Host, String, int, int, int, String...). Adding the theme int turns those 3 into now 4 ints in a row!
Overloads: it's getting ridiculous. To not break backwards compatibility, we need to add at least another 3 overloads. If we want to improve flexibility, we'd need to add 6 to the existing 6!
Current API limitations: a minor pet peeve of mine has been the inability to pass in a string resource int for the rationale and vice versa for the yes and no buttons. The new builder has String/int overloads for each of those options.
I don't see any reason to release a v2.0 anytime soon. I was basically thinking we could leave people a year or more to migrate away from the old API which should be more than enough time to migrate away from a method that's only used once per set of permissions.

@samtstern Maybe I've changed your mind? 😄 If not, we need to decide which overloads to add.

samtstern · 2017-12-05T17:33:08Z

Ok I agree with you completely, I hate all the overloads and they're not scalable.

So maybe let's take a middle-road approach:

Keep supporting the EasyPermissions.requestPermissions(context, rationaleString, requestCode, perms) method call indefinitely. That's the most common use case (99%) and it doesn't require too many overloads.
Introduce a new method: EasyPermissions.requestPermissions(PermissionsRequest) which uses the builder.
Mark all other overloads of EasyPermissions.requestPermissions() as @Deprecated so this can be just a minor version bump and won't break anyone.

SUPERCILEX · 2017-12-05T21:38:11Z

@samtstern I like it, that's a perfect compromise! 😄

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-12-05T21:57:12Z

easypermissions/src/main/java/pub/devrel/easypermissions/PermissionRequest.java

+import pub.devrel.easypermissions.helper.PermissionHelper;
+
+/**
+ * An immutable model representing a permission request.


nit: we can probably say something more descriptive here like "A model object that holds all of the parameters associate with a permission request, such as the request code or the rationale"

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-12-05T23:03:35Z

easypermissions/src/main/java/pub/devrel/easypermissions/helper/PermissionHelper.java

-        } else {
+        if (host instanceof AppCompatActivity)
+            //noinspection unchecked Hmmm... not sure what's going on here
+            return (PermissionHelper<Activity>) (Object)


What's up with the double cast? (PermissionHelper<Activity>) (Object)

Not sure, here's the error I get otherwise:

It works just fine at runtime so I'd guess we need to use some sort of type variance.

Figured it out.

samtstern · 2017-12-05T23:06:03Z

easypermissions/src/main/java/pub/devrel/easypermissions/helper/PermissionHelper.java

        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
-            return new LowApiPermissionsHelper(host);
+            return new LowApiPermissionsHelper<>(host);


Surprised to see the <> here, do we get a warning without that?

Yeah, I added a type T to the helper.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-12-05T23:17:31Z

@SUPERCILEX awesome! I think all that's left is the README, right?

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-12-05T23:32:25Z

Sweet! (Thanks for reminding me, I'd forgotten 😄)

samtstern · 2017-12-06T01:18:26Z

README.md

@@ -66,6 +66,14 @@ private void methodRequiresTwoPermission() {
        // Do not have permissions, request them now
        EasyPermissions.requestPermissions(this, getString(R.string.camera_and_location_rationale),
                RC_CAMERA_AND_LOCATION, perms);
+        // OR for finer control over the rationale dialog


One last comment:
Developers tend to copy-and-paste so I'd avoid putting both of these in the same snippet as developers will end up with double requests.

I'd say:

// old snippet here

Or for finer control over the rationale dialog, use PermissionRequest:

// new snippet here

Raises hand. Haha, yeah, I've done that too. It should be fixed now.

Signed-off-by: Alex Saveau <[email protected]>

Add new builder API and support customizing rationale dialog theme

23d7019

Signed-off-by: Alex Saveau <[email protected]>

samtstern added this to the 1.1.0 milestone Dec 5, 2017

SUPERCILEX added 2 commits December 5, 2017 13:38

Add documentation

2395282

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

0c182ec

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Dec 5, 2017

View reviewed changes

Clarify PermissionRequest docs

6a1dafc

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Dec 5, 2017

View reviewed changes

Fix type invariance

5cf11c3

Signed-off-by: Alex Saveau <[email protected]>

Add README example

5de5e18

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Dec 6, 2017

View reviewed changes

Fix README

c704289

Signed-off-by: Alex Saveau <[email protected]>

samtstern merged commit 49b6867 into googlesamples:master Dec 6, 2017

SUPERCILEX deleted the builder-theme branch December 6, 2017 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add new builder API and support customizing rationale dialog theme #180

Add new builder API and support customizing rationale dialog theme #180

Uh oh!

SUPERCILEX commented Dec 3, 2017

Uh oh!

samtstern commented Dec 4, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

samtstern commented Dec 5, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

samtstern Dec 5, 2017

Uh oh!

SUPERCILEX Dec 5, 2017

Uh oh!

samtstern Dec 5, 2017

Uh oh!

SUPERCILEX Dec 5, 2017

Uh oh!

SUPERCILEX Dec 5, 2017

Uh oh!

samtstern Dec 5, 2017

Uh oh!

SUPERCILEX Dec 5, 2017

Uh oh!

samtstern commented Dec 5, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

samtstern Dec 6, 2017

Uh oh!

SUPERCILEX Dec 6, 2017

Uh oh!

Uh oh!

Add new builder API and support customizing rationale dialog theme #180

Add new builder API and support customizing rationale dialog theme #180

Uh oh!

Conversation

SUPERCILEX commented Dec 3, 2017

Uh oh!

samtstern commented Dec 4, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

samtstern commented Dec 5, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Dec 5, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!