This repository was archived by the owner on Oct 3, 2024. It is now read-only.

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

Merged

samtstern merged 7 commits into googlesamples:master from SUPERCILEX:builder-theme

Dec 6, 2017

Merged

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

samtstern merged 7 commits into googlesamples:master from SUPERCILEX:builder-theme

Dec 6, 2017

Conversation

@SUPERCILEX

Copy link

Contributor

@SUPERCILEX SUPERCILEX commented Dec 3, 2017

#174

@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.

@SUPERCILEX


 Add new builder API and support customizing rationale dialog theme

23d7019

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

@samtstern

Copy link

Contributor

samtstern commented Dec 4, 2017

@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

Copy link

Contributor Author

SUPERCILEX commented Dec 5, 2017

@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

Copy link

Contributor

samtstern commented Dec 5, 2017

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.

@samtstern samtstern added this to the 1.1.0 milestone

Dec 5, 2017

@SUPERCILEX

Copy link

Contributor Author

SUPERCILEX commented Dec 5, 2017

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

SUPERCILEX added 2 commits

December 5, 2017 13:38

@SUPERCILEX


 Add documentation

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

@SUPERCILEX


 Cleanup

0c182ec

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

samtstern

samtstern reviewed

Dec 5, 2017

View reviewed changes

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

import pub.devrel.easypermissions.helper.PermissionHelper;

/**

* An immutable model representing a permission request.

Copy link

Contributor

@samtstern samtstern Dec 5, 2017

Choose a reason for hiding this comment

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

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"

Copy link

Contributor Author

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

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

👍

@SUPERCILEX


 Clarify PermissionRequest docs

6a1dafc

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

samtstern

samtstern reviewed

Dec 5, 2017

View reviewed changes

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

} else {

if (host instanceof AppCompatActivity)

//noinspection unchecked Hmmm... not sure what's going on here

return (PermissionHelper<Activity>) (Object)

Copy link

Contributor

@samtstern samtstern Dec 5, 2017

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

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

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

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

Copy link

Contributor Author

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

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

Figured it out.

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

public static PermissionHelper<Fragment> newInstance(Fragment host) {

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {

return new LowApiPermissionsHelper(host);

return new LowApiPermissionsHelper<>(host);

Copy link

Contributor

@samtstern samtstern Dec 5, 2017

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

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

Yeah, I added a type T to the helper.

@SUPERCILEX


 Fix type invariance

5cf11c3

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

@samtstern

Copy link

Contributor

samtstern commented Dec 5, 2017

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

@SUPERCILEX


 Add README example

5de5e18

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

@SUPERCILEX

Copy link

Contributor Author

SUPERCILEX commented Dec 5, 2017

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

samtstern

samtstern reviewed

Dec 6, 2017

View reviewed changes

README.md Outdated

// 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

Copy link

Contributor

@samtstern samtstern Dec 6, 2017

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@SUPERCILEX SUPERCILEX Dec 6, 2017

Choose a reason for hiding this comment

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

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

@SUPERCILEX


 Fix README

c704289

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

@samtstern samtstern merged commit 49b6867 into googlesamples:master

Dec 6, 2017

@SUPERCILEX SUPERCILEX deleted the builder-theme branch

December 6, 2017 19:48

Labels

None yet

2 participants

@SUPERCILEX @samtstern

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 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 samtstern Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

@samtstern samtstern Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

@samtstern samtstern Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

@SUPERCILEX SUPERCILEX Dec 5, 2017

Choose a reason for hiding this comment

Uh oh!

samtstern commented Dec 5, 2017

Uh oh!

SUPERCILEX commented Dec 5, 2017

Uh oh!

@samtstern samtstern Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

@SUPERCILEX SUPERCILEX Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants