-
Notifications
You must be signed in to change notification settings - Fork 601
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
[Permissions] Permissions enhancements #1793
base: main
Are you sure you want to change the base?
Conversation
- Also allow rememberPermissionState and rememberMultiplePermissionsState to be used in Compose Preview
Haven't reviewed yet, but please summarise your API changes in the PR as well please. It helps to link back to from the release notes so people can see what changed and why. |
internal var launcher: ActivityResultLauncher<String>? = null | ||
|
||
internal fun refreshPermissionStatus() { | ||
status = getPermissionStatus() | ||
status = getPermissionStatus(isPermissionPostRequest = true) |
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.
refreshPermissionStatus()
is also called from PermissionLifecycleCheckerEffect
, so right now the initial lifecycle event refreshing the status when the permission isn't granted will result in a status of PermanentlyDenied
.
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 can track isPermissionPostRequest
as its own property and set it separately from refreshPermissionStatus
, since once that is set to true
there shouldn't be any scenario that would make it go back to false
.
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.
Is there any ordering guarantees between rememberLauncherForActivityResult
and PermissionLifecycleCheckerEffect
?
when { | ||
isPermissionPostRequest -> when { | ||
shouldShowRationale -> PermissionStatus.NotGranted.Denied | ||
else -> PermissionStatus.NotGranted.PermanentlyDenied |
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.
There's a case here where it's possible to have requested the permission and shouldShowRationale
is false, but without the permission being permanently denied.
If a permission is requested for the first time, and the user uses the back button to exit the permission dialog without selecting any of the options, the permission won't be permanently denied. If the permission is requested again, then the dialog will be shown again normally. Also, since the user did not explicitly deny the permission, shouldShowRationale
will still be false.
With a12ee22 these states should be correct
The only issue as @alexvanyo pointed out, is if the user canceled the dialog:
As a user, this doesn't happen to me often, but it does happen enough times (usually by mistake) that I can see it being a problem. Any ideas on how to handle that case? |
My initial thought is that it's an intractable problem since there is no way to distinguish between these two states:
and a choice has to be made to optimize for one over the other:
This forces the user to perform a click that they didn't need to (to go from
I don't like Update - a0b6b00 |
Using the lifecycle effect, I wonder if it is possible to infer whether the permission dialog was actually shown (in the not requested state) versus no permission dialog shown at all (in the permanently denied state)? This is even more of an edge case, but it would also be possible for the user to cancel the dialog repeatedly forever (request permission -> cancel -> request permission -> request permission -> request permission -> ...) |
I think the OS (at least in later versions) protects against this and always pauses the requesting
That might be the point where a line has to be drawn between supporting that case and supporting a permanently denied state 🤔 I can see situations where the repeated cancelling could come up in the real world, but they'd still have the escape hatch of granting the permission in settings. Maybe it could be configurable, so the dev could choose between handling cancelled requests or a permanently denied status? |
I am actually remembering now, this is why we didn't support this in the first place. Unfortunately, unless we can work this out I don't think we could land this in Accompanist, which I'm sorry for because I told you it would be ok. I think it is probably fine in your library but we can't have inconsistent behaviour in an "official" library. Let me see if I can find an internal expert |
I went over it a few times today, and I think with the public API there's no way around that. If there's a hidden API or some way to tap in to the OS to get that info, or if there's an expert that knows of a non obvious way to detect the difference between the dialog getting canceled and being permanently denied, that would be great! |
So the answer I got is that the activity result contract should be returning an |
Interesting, I'll take a look at that. |
@bentrengrove it looks like the |
What happens if you just always use |
Same result as single:
|
How are you testing? Just in the accompanist sample app? I will give it a try |
Yes, in the sample app. |
Can you push up your changes please? Doesn't look like sample builds right now and I want to make sure we are testing the same thing |
I'm AFK for the next few days. I can do it when I'm back, but the changes were pretty minimal (mostly just revoked -> denied IIRC) |
@bentrengrove updated the branch so that the sample builds and runs |
Thanks for that, I gave it a go and confirmed the same thing. I can't find any way to detect cancellation for permissions. Even the multiple permission request always returns -1 for me, never emptyMap(). I will go back and see what I can find out |
@bentrengrove any update here? |
Not yet sorry, still trying to find a solution but it's not looking good unfortunately |
What works for me is checking the time it takes for the result to come back, whether a permission dialog is shown or not. val context = LocalContext.current.findActivity()
var launchTime by remember { mutableLongStateOf(0L) }
var permissionGranted by remember { mutableStateOf(false) }
val permissionLauncher = rememberLauncherForActivityResult(
contract = ActivityResultContracts.RequestPermission(),
onResult = { isGranted: Boolean ->
permissionGranted = isGranted
val isPermanentlyDenied = System.currentTimeMillis() - launchTime < 100 // normally it takes 30ms on my device for the result to come back
if (isPermanentlyDenied) {
context.startActivity(
Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS)
.putExtra(Settings.EXTRA_APP_PACKAGE, context.packageName)
)
}
}
)
if (!permissionGranted) {
Button(
onClick = {
launchTime = System.currentTimeMillis()
permissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)
}
) {
Text("Grant permission")
}
} |
I think part of the frustration here is that the whole permission system seems inconsistent to developers. My understanding of that is that it's for user safety as well as discouraging certain UX paradigms (e.g. redirecting to app settings), and I'm sure there are other considerations as well. However it leaves us in a situation where the underlying system is perceived as broken (e.g. not being able to differentiate between a permission getting permanently denied and cancelling the permission request). I know that following the permission workflow would make it easier to operate with these constraints, but the honest truth is that I've never worked with a product/design team that found that workflow acceptable. That led me to look for and implement workarounds, which all ultimately ended up not working in all cases. My experience is not unique, and there are countless QAs about this topic, and just as many broken workarounds. @bentrengrove @alexvanyo I'm not directing any of this at you, and some of these issues go back almost a decade. I'm hoping that you can surface this issue internally, and that hopefully there can be some way to address it. |
I agree with you and would really love to have this feature in Accompanist. I am trying to raise it internally but I created a public issue as well that you could cc yourself to and hopefully we can post any results in there as well |
Pardon me for shooting in here, but I have always believed this to be an intentional decision made by the platform team to “protect” users. That not knowing whether the user directly denied a permission or whether the system denied on behalf of the user is actually a feature. Of course, as a developer, I find this incredibly frustrating, because I don't know when to send users to Settings to enable stuff there if the permission has been permanently denied, so I would personally love to have the ability to differentiate between these states built-in. |
Still need to update the tests and docs, but looking to get feedback.