-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change checkPermission method return type in phone log plugin. #4
Conversation
|
||
void isNeverAskBoxChecked() async { | ||
bool res = await phoneLog.isNeverAskChecked(); | ||
print("never ask is checked: " + res.toString()); |
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.
Remove.
print("permission is: " + res.toString()); | ||
} | ||
|
||
void isNeverAskBoxChecked() async { |
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.
Instead of exposing this as a Dart API, could we manage this state entirely in the native side?
For instance, if Dart app sends "requestPermission", we can send back a special return value that says: "NEVER_ASK_AGAIN" if the never ask again box was checked previously.
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.
Do you think it's better to have 3 states {granted, denied, neverAsked} for the requestPermission method? In this case this requestPermission method will not return if the permission is granted or not since it returns isNeverAsk for both granted and denied with the 'never ask' box checked.
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.
Are you trying to customize the Flutter app based on "never ask"? Could we just immediately return an appropriate response if never ask option was checked? "Never ask + granted" always returns granted. "Never ask + deny" always returns denied.
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.
We have a use case that needs to know if it is the first time that this 'never ask' check box is checked or not. That is when the user clicks "enable" button for the first time, we check permission and request for permission, if she/he selects denied with the check box selected, for this case we do nothing. Then later if this user tries to tap the "enable" button again, we check if the permission is granted. If not we check if the check box is selected previously, we show something to let the user know that she/he can only enable this from the system setting.
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.
It is probably a better UX to not show users Enable button at all if they previously denied and said "never ask again".
I really do think we should do this with a single API. That will also save you two roundtrips. It also makes sense since users select both things (yes/no and do not ask) from the same dialog.
So checkPermission() can return a structure:
bool hasPermission;
bool shouldAskAgain;
Then if:
hasPermission -> don't show enable button, you are done.
!hasPermission && shouldAskAgain -> Show enable button
!hasPermission && !shouldAskAgain -> Don't show enable button, show "Go to settings to enable" instead.
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.
I think we have the same us flow as you suggested, except for one case:
!hasPermission && !shouldAskAgain -> Don't show enable button, show "Go to settings to enable" instead.
for this one, by tapping on enable button:
1, if requesting permission dialog pops up (which means the 'never ask' box is not checked previous), and the user checked the 'never ask' checkbox and selected 'deny', we show nothing.
2, if the requesting permission dialog cannot pop up(the 'never ask' box has been checked previous), we show the user a snack bar with ' go to settings page to enable and so on....'
So that's why we want to check if the box is checked or not separately, so we can do something like this:
if (permission not granted && checkbox is checked) => show snackbar
else {request permission}
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.
When do you decide when to show enable button? I assume at some point you are calling checkPermission
?
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.
So, I will change the checkPermission method to return 3 states:
enum PermissionResult {granted, notGranted, cannotRequestPermission}
If it's 'cannotRequestPermission' means it's denied and the check box is checked already.
And for the request permission method, I will leave it to what it was.
Thank you so much for the suggestions!
@@ -44,6 +44,14 @@ class PhoneLog { | |||
} | |||
} | |||
|
|||
enum PermissionStatus {granted, denied, deniedAndCannotRequest} |
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.
Add test for this.
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.
Done.
Future<bool> checkPermission() async { | ||
final bool isGranted = await _channel.invokeMethod("checkPermission", null); | ||
return isGranted; | ||
Future<PermissionStatus> checkPermission() async { |
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.
This is a breaking change. Please document it in CHANGELOG.md and up the version.
Also update the documentation. "return a Future with the result" is not very informative. Thanks!
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.
Done.
@@ -1,3 +1,6 @@ | |||
## [0.0.3] - August 15th, 2018. |
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.
You also need to change the pubspec.yaml file to rev the version to 0.0.3
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.
Done. Thank you for catching this!
No description provided.