Skip to content
This repository has been archived by the owner on Oct 3, 2024. It is now read-only.

Added RationaleDialogCallback #208

Merged
merged 5 commits into from
Mar 15, 2018
Merged

Conversation

ernestkamara
Copy link
Contributor

RationaleDialogCallback is an extension of PermissionCallbacks and forwards the rationale
dialog buttons click events to the host that requesting a permission(s). RationaleDialogCallback is useful for events logging and is completely optional.

Check out RationaleFragment in the sample app folder for example.

Ernest Saidu Kamara and others added 2 commits March 14, 2018 10:10
@samtstern
Copy link
Contributor

Hi @ernestkamara. Thanks for sending over a PR!

Can you explain the main use case for this callback? What situation do you have in your app where you want to know if the rationale was denied versus the actual permissions request?

@ernestkamara
Copy link
Contributor Author

Hi @samtstern . We need this callback mainly for analytics logging to track how many denied the rationale dialog. We find this useful and might help us improve our rationale text and other UI states.

@samtstern
Copy link
Contributor

Ok that's a reasonable use case. What about changing the design though:

  • Make RationaleDialogCallbacks its own interface, not an extension of PermissionCallbacks
  • Give it two methods:
    • onRationaleAccepted(int requestCode)
    • onRationaleDenied(int requestCode)
      • This will be called in addition to onPermissionDenied rather than instead of it.

@ernestkamara
Copy link
Contributor Author

Ok. Thanks for your input. I will update it!

@ernestkamara
Copy link
Contributor Author

PR updated @samtstern!

Copy link
Contributor

@SUPERCILEX SUPERCILEX left a comment

Choose a reason for hiding this comment

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

@ernestkamara This looks great, thanks! I just have some code quality and README changes, but besides that, 👍.

README.md Outdated
@@ -152,6 +152,33 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) {
}
```

### RationaleCallbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: new line after. Also, how about calling it something like "Interacting with the rationale dialog"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

README.md Outdated
}
```

Check out [RationaleFragment](https://github.com/googlesamples/blob/master/app/src/main/java/pub/devrel/easypermissions/sample/RationaleFragment.java) in the sample app folder for the full example.
Copy link
Contributor

Choose a reason for hiding this comment

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

The link can be simplified to app/src/main/java/pub/devrel/easypermissions/sample/RationaleFragment.java. GitHub knows the project's directory structure. 👍

Also, it could be reworded to "For a full example, take a look at the sample RationaleFragment."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

README.md Outdated

Note that permissions are not requested when the user denies or cancel the rationale dialog and `onPermissionsDenied` will be invoked.
The user will need to reinvoke the function that requests permissions. This is callbacks can be useful for analytics tracking and UI states
updates.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should give examples of how the callback can be used, thoughts @samtstern? Without that info, the paragraph can be simplified to "Rationale callbacks don't necessarily imply permission changes. To check for those, see the EasyPermissions.PermissionCallbacks."

public View onCreateView(LayoutInflater inflater,
ViewGroup container,
Bundle savedInstanceState) {
super.onCreateView(inflater, container, savedInstanceState);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this super call. 👍

EasyPermissions.RationaleCallbacks {
private static final String TAG = "RationaleFragment";
private static final int RC_STORAGE_PERM = 123;
public static final String PERMISSION_STORAGE = Manifest.permission.WRITE_EXTERNAL_STORAGE;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be private.

*/
public interface RationaleCallbacks {
void onRationaleAccepted(int requestCode);
void onRationaleDenied(int requestCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: new line between these methods and none after.

README.md Outdated
@@ -152,6 +152,33 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) {
}
```

### RationaleCallbacks
Implement this callback if you want to interact with the rationale dialog.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the specific name, so something like "Implement the EasyPermissions.RationaleCallbacks if ..."


import java.util.List;

public class RationaleFragment extends Fragment implements EasyPermissions.PermissionCallbacks,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any need to introduce a whole new Fragment here rather than just adding this to the existing sample?

Would make the diff much smaller to just add the new callbacks to our existing sample code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I added RationaleFragment in connection to my initial implementation of RationaleCallbacks which extends PermissionCallbacks. I guess it's not needed anymore. Thanks for your feedback @SUPERCILEX . Will update this PR shortly.

Copy link
Contributor

@SUPERCILEX SUPERCILEX left a comment

Choose a reason for hiding this comment

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

Sweet! One last README comment.

README.md Outdated

```java
public class RationaleFragment extends Fragment implements EasyPermissions.RationaleCallbacks {
Implement the EasyPermissions.RationaleCallbacks if you want to interact with the rationale dialog.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's make it code font "`EasyPermissions.RationaleCallbacks`"

README.md Outdated
The user will need to reinvoke the function that requests permissions. This is callbacks can be useful for analytics tracking and UI states
updates.

Rationale callbacks don't necessarily imply permission changes. To check for those, see the EasyPermissions.PermissionCallbacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: `...`.

@samtstern samtstern added this to the 1.1.4 milestone Mar 15, 2018
@samtstern
Copy link
Contributor

Thanks @SUPERCILEX for the review and @ernestkamara for the patient updates.

This is good to go. It also means the next release will be 1.2.0 since this is a new feature (woot!)

@samtstern samtstern merged commit 20987dc into googlesamples:master Mar 15, 2018
README.md Outdated
@@ -170,7 +170,7 @@ public void onRationaleDenied(int requestCode) {
}
```

Rationale callbacks don't necessarily imply permission changes. To check for those, see the EasyPermissions.PermissionCallbacks.
Rationale callbacks don't necessarily imply permission changes. To check for those, see the `EasyPermissions.RationaleCallbacks`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, looks like this should stay PermissionCallbacks 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Smile.. Will create a new PR in a min!

Copy link
Contributor

Choose a reason for hiding this comment

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

Alrighty, go for it!

@SUPERCILEX
Copy link
Contributor

@samtstern Or not 😆, PR incoming.

@ernestkamara
Copy link
Contributor Author

Btw.. when is 1.2 scheduled?

@samtstern
Copy link
Contributor

@ernestkamara just happened! https://github.com/googlesamples/easypermissions/releases/tag/1.2.0

Copy link

@Ana120914 Ana120914 left a comment

Choose a reason for hiding this comment

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

Amor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants