Short circuit perms request at root method #76

SUPERCILEX · 2017-01-19T01:03:28Z

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com> (cherry picked from commit e6e7c57) Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

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

SUPERCILEX · 2017-01-19T01:04:52Z

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

@@ -352,7 +352,7 @@ private static Activity getActivity(@NonNull Object object) {
        } else if (object instanceof android.app.Fragment) {
            return ((android.app.Fragment) object).getActivity();
        } else {
-            return null;
+            throw new IllegalArgumentException("Context cannot be null.");


I changed it to be IllegalArgumentException since it is now called immediately using the method's argument instead of further down the stack.

Wouldn't this message be more accurate as "Object must be an Activity or a Fragment"? This else does not imply nullity.

Woops, thanks!

samtstern

LGTM in general, a few comments.

samtstern · 2017-01-19T16:03:53Z

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

                                          @StringRes int positiveButton, @StringRes int negativeButton,
                                          final int requestCode, @NonNull final String... perms) {
+        if (hasPermissions(getActivity(object))) return false;


nit: prefer using brackets even for a one-line if statement.

samtstern · 2017-01-19T16:05:17Z

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

@@ -352,7 +352,7 @@ private static Activity getActivity(@NonNull Object object) {
        } else if (object instanceof android.app.Fragment) {
            return ((android.app.Fragment) object).getActivity();
        } else {
-            return null;
+            throw new IllegalArgumentException("Context cannot be null.");


Wouldn't this message be more accurate as "Object must be an Activity or a Fragment"? This else does not imply nullity.

samtstern · 2017-01-19T16:05:56Z

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

     */
    @SuppressLint("NewApi")
-    public static void requestPermissions(@NonNull final Object object, @NonNull String rationale,
+    public static boolean requestPermissions(@NonNull final Object object, @NonNull String rationale,


I'm unsure about changing this method to be non-void. What would you expect an app developer to do in the case where this returns false?

I meant to include checking for permissions in the if statement (I fixed that in my latest commit). This leaves us with two questions: should we be checking for permissions as well and then does it make sense to return the boolean if we are also checking for permissions?

Although I see both sides, I like the explicitness of the current model where you say if(!hasPermissions()) { requestPermissions() } in your app code since then you know exactly when you're about to launch UI. I think it also matches the simplest mental model.

@samtstern, so is issue #62 no longer relevant? (hasPermissions() already checks for the API level)

The essence of #62 was tracking all the technical debt that has built up over time. This library made some bad initial decisions (using Object all over the place rather than overloads) and with the complexity of the support library there are just a ton of branches and checks that I'd like to refactor to make the code overall smaller and more readable.

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

SUPERCILEX

@samtstern Whew! I get what you mean by technical debt, EasyPermissions.java was a jumbled mess! The new EasyPermissions.java class has more lines of code than the old one, but that's only because it has to triplicate methods and documentation. Otherwise, things are so easy to understand now! If you should show a rationale, then show it; else, just request the permissions with all of that in one method related to the activity or fragment being passed. It's beautiful, enjoy! 😄

PS: The diff is garbage so I would recommend just reading through the new class. (It shouldn't be too difficult now!)

SUPERCILEX · 2017-01-20T02:32:30Z

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

@@ -76,7 +78,7 @@ public void onClick(DialogInterface dialog, int which) {
        mAlertDialog = dialogBuilder.create();
    }

-    @TargetApi(11)
+    @RequiresApi(api = Build.VERSION_CODES.HONEYCOMB)


TargetApi is technically deprecated.

From the RequiresApi docs:

This is similar in purpose to the older @TargetApi annotation, but more clearly expresses that this is a requirement on the caller, rather than being used to "suppress" warnings within the method that exceed the minSdkVersion.

SUPERCILEX · 2017-01-20T02:33:57Z

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

+            } else { // mHost instance of FragmentActivity
+                ActivityCompat.requestPermissions(
+                        (FragmentActivity) mHost, mConfig.permissions, mConfig.requestCode);
+            }


mHost = dialogFragment.getParentFragment() != null ? dialogFragment.getParentFragment() : dialogFragment.getActivity();

SUPERCILEX · 2017-01-20T02:40:03Z

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

-                                                      @NonNull String deniedPermission) {
-        return !shouldShowRequestPermissionRationale(object, deniedPermission);
-    }
+    @RequiresApi(api = Build.VERSION_CODES.M)


Not using Object allows us to do this! (Yay to lint and IDE smarts!)

SUPERCILEX · 2017-01-20T02:40:21Z

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

+     *                          PermissionCallbacks#onPermissionsDenied(int, List)}
+     * @return {@code true} if at least one permission in the list was permanently denied.
+     */
+    @RequiresApi(api = Build.VERSION_CODES.M)


Again, yay!

SUPERCILEX · 2017-01-20T03:16:20Z

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

+                                          @NonNull String... perms) {
+        if (hasPermissions(activity, perms)) {
+            notifyAlreadyHasPermissions(activity, requestCode, perms);
+            return;
        }


This is the new model you'll see everywhere: if we already have the permissions or api level< M, then we're done.

We also hope that the api consumer has their implements PermissionCallbacks or annotations in the same object they pass into requestPermissions. If they do, we notify them which provides nice api consistency.

New model is great! Will try to make sense of the diff / new file and comment.

Ok so I think the use of notifyAlreadyHasPermissions could be a breaking change for some developers.

Let's say you have this code

private foid foo() { if (!EasyPermissions.hasPermissions(this, "BAR")) { EasyPermissions.requestPermissions(this, "BAR"); return; } // Do some important stuff } @Override public void onPermissionsGranted(List<String> perms) { if (perms.contains("BAR")) { foo(); } }

The problem is that by notifying them of already having the permission, this could result in foo() being called twice. For some operations, this could be very bad (anything that's not idempotent).

So I think instead of notifying listeners when they already have permissions, we may have to just Log a warning and then drop the whole thing in order to preserve current behavior.

@samtstern I got confused by this too at first, but think of it this way: anytime you call requestPermissions, you can expect to get a callback. In your example the flow is still the same: no permissions -> request permissions -> foo gets called because user accepted permissions -> has permissions -> don't call request permissions -> continue as usual. The only case I can think of that results in the method being recursively called infinity is if you do this which is wrong anyway:

@AfterPermissionsGranted(foo) public void bar() { EasyPermissions.requestPermissions(this, ...); // Boom! }

The usage I can see for getting a callback when you already have the permissions would be:

@Override public void onCreate(instance) { EasyPermissions.requestPermissions(this, ...); } @AfterPermissionsGranted(foo) public void bar() { // Happily do stuff }

That's a good point. I am convinced :-)

SUPERCILEX · 2017-01-20T03:17:19Z

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

        }

-        new AlertDialog.Builder(activity)


This was used for api level < something lower than M which means it's useless because we already have the permissions if we are lower than M.

SUPERCILEX · 2017-01-20T03:24:52Z

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

+    }
+
+    private static boolean shouldShowRequestPermissionRationale(@NonNull Object object,
+                                                                @NonNull String perm) {


This is the only one I left with Object because it helps reduce triplicating logic. (I didn't know triplicating was a word, lol 😄)

samtstern · 2017-01-20T17:36:47Z

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

-     *         request code to track this request, must be < 256.
-     * @param perms
-     *         a set of permissions to be requested.
+     * @param activity    Activity requesting permissions. Should implement {@link


Since we're now going to have a huge number of methods and overloads (which is good!) this file is going to be overloaded with duplicate docs.

In general, for overloads we should avoid duplicating the whole docstring and just say something short and then use {@see} to point to the method we delegate to which contains the full docstring.

"Requests permissions with default values for rationale dialog buttons.

{@see #requestPermissions(...)}"

Done in b4f4ad9.

samtstern · 2017-01-20T17:42:39Z

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

-        RationaleDialogFragment fragment = RationaleDialogFragment
-                .newInstance(positiveButton, negativeButton, rationale, requestCode, perms);
-        fragment.show(fragmentManager, DIALOG_TAG);
+    public static void requestPermissions(@NonNull Fragment fragment,


Although it's verbose, it may be clearer to always use fully qualified names for fragments (so android.support.v4.app.Fragment here. I think people come to the code with different assumptions about what an unqualified reference to Fragment means.

Even if we don't do that in code, we should definitely mention it in the JavaDoc.

I agree with you that this can be confusing so I'm open to adding the full qualifier, but for the moment I just made everything a link in 941c08f. I really hope that's good enough because it kills me to add the full qualifier.

samtstern · 2017-01-20T17:52:44Z

@SUPERCILEX thanks for taking this on! Sorely needed.

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

SUPERCILEX · 2017-01-21T01:23:57Z

@samtstern I took advantage of us guarding our permission requests with #hasPermissions(...) to lower the api level needed to call #requestPermissions(android.app.Fragment, ...) in ddd3f74. Is that appropriate?

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

samtstern · 2017-01-23T16:19:27Z

@SUPERCILEX this all LGTM, thank you! Going to squash and merge it.

SUPERCILEX · 2017-01-23T17:55:46Z

Awesome, glad to have been able to clean it up!

SUPERCILEX added 2 commits January 18, 2017 17:00

Short-circuit perms request at root method

f309e44

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com> (cherry picked from commit e6e7c57) Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Change exception type

682b43e

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

SUPERCILEX commented Jan 19, 2017

View reviewed changes

samtstern reviewed Jan 19, 2017

View reviewed changes

SUPERCILEX added 3 commits January 19, 2017 09:57

Fix mistakes and nits

caed95c

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

Initial refactor

b67e0e8

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

Cleanup

eb99a50

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

SUPERCILEX commented Jan 20, 2017

View reviewed changes

samtstern reviewed Jan 20, 2017

View reviewed changes

SUPERCILEX added 3 commits January 20, 2017 16:53

Add links to documentation

941c08f

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

Remove triplication of documentation

b4f4ad9

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

Use android.app.Fragment#getActivity() for context instead of getContext

ddd3f74

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

SUPERCILEX added 3 commits January 21, 2017 16:26

Remove unnecessary abstraction

733043e

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

Argh Intellij! I finally updated my code style to put interfaces on top

35f8d63

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

Cleanup

c097b4c

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

samtstern merged commit 209a517 into googlesamples:master Jan 23, 2017

SUPERCILEX deleted the short-circuit branch January 23, 2017 17:55

samtstern mentioned this pull request Jan 24, 2017

Short-circuit the library much earlier on API < 23 #62

Closed

SUPERCILEX mentioned this pull request Mar 14, 2017

New public method to verify if we should show the rationale dialog #92

Merged

Short circuit perms request at root method #76

Short circuit perms request at root method #76

Uh oh!

Conversation

SUPERCILEX commented Jan 19, 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!

samtstern left a comment

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!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jan 20, 2017

Uh oh!

SUPERCILEX commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern commented Jan 23, 2017

Uh oh!

SUPERCILEX commented Jan 23, 2017

Uh oh!

Uh oh!

SUPERCILEX commented Jan 21, 2017 •

edited

Loading