Skip to content
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] Clean up the APIs to guarantee a better UX #990

Merged
merged 2 commits into from Feb 7, 2022

Conversation

manuelvicnt
Copy link
Collaborator

@manuelvicnt manuelvicnt commented Feb 4, 2022

API breaking changes

  • The isPermissionRequested flag from the PermissionState and MultiplePermissionsState APIs is removed.

    • [Mitigation] If you need this, you can manually keep a mutable state in your composable that you fully control.
    • [Reason 1] The name wasn't clear enough for what it was doing
    • [Reason 2] It also technically got set to true the first time the permission result launcher returns with the user's choice, not upon launchPermissionRequest being called.
  • The PermissionRequired and PermissionsRequired slot APIs have been removed.

    • [Reason] The permissionsNotAvailableContent slot misbehaved in different Android API versions, and just having two slots available doesn't add that much value.
  • Model the permission result as PermissionStatus to avoid state inconsistencies.

  • [New API] New callback in the rememberPermissionState and rememberMultiplePermissionsState for when the activityResultLauncher returns.

    • [Reason] This callback gets called every time the user interacts with the dialog even if the underlying permission states don't change (i.e. don't cause a recomposition), which can be useful in some scenarios.

Changes in samples

  • [NEW] A sample that showcases the coarse/fine permission case introduced in Android 12 where users can request to upgrade to precise location. This is the most complicated permissions workflow by far.
  • All samples are updated to not differentiate between the it's the first time requesting the permission vs the the user doesn't want to be asked again cases. We always leave the button enabled even though it's not the best UX possible but YOLO. Apparently, sending users to the Settings screen is a bad practice.

Misc

This PR fixes: #549 and #819

Copy link
Collaborator

@alexvanyo alexvanyo left a comment

Choose a reason for hiding this comment

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

The PR description is 💯 🤩

Looks good to me, just a couple of nits!

Copy link
Collaborator

@bentrengrove bentrengrove left a comment

Choose a reason for hiding this comment

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

This is really great work. Well done @manuelvicnt

@ln-12
Copy link
Contributor

ln-12 commented Feb 15, 2022

@manuelvicnt Hey, I am wondering if the sample could be improved.

Currently, if I deny all permissions the app tells me This feature requires location permission which is fine. But when I click an Request permission, nothing happens. You state that Apparently, sending users to the Settings screen is a bad practice, but I would disagree with that.

In my opinion it is much better to open the settings that doing nothing. As a user I would wonder why nothing happens. Maybe the user denies the permissions and later, when the app is used again, wonders why it's not working. I know that anyone can implement it in a different way, but I als think it's especially important to consider this case as your sample might be the starting point for many developers.

WDYT?

@manuelvicnt
Copy link
Collaborator Author

Hi @ln-12, thanks for your feedback. We're working closely with UX and PM teams to see what's the best recommendation we can give here. Unfortunately, this is what we can show so far.

If you think your app could benefit from a different UX, go ahead and implement that logic. That's fine too!

Thanks!

@PaulWoitaschek
Copy link

But doesn't this take away the possibility to directly ask the user when entering a composable?
I.e our flow is:
The user clicks on "scan barcode" and then we want to ask for the permission and only show additional explanations after they denied the permission.
In the samples it's simplified as there is now always a button.

@ln-12
Copy link
Contributor

ln-12 commented Aug 2, 2022

@PaulWoitaschek Can't you just do it like this?

// define permission state
val locationPermissionsState = rememberMultiplePermissionsState(
    listOf(
        Manifest.permission.ACCESS_FINE_LOCATION,
        Manifest.permission.ACCESS_COARSE_LOCATION
    )
)

// call launch permission request on first composition
LaunchedEffect(Unit) {
    locationPermissionsState.launchMultiplePermissionRequest()
}

@PaulWoitaschek
Copy link

But that would also show the permission when the user is in a showRationale state right?

@ln-12
Copy link
Contributor

ln-12 commented Aug 3, 2022

Both PermissionState and MultiplePermissionsState contain bools which indicate if you should show a rational (see here. If you first launch the request, shouldShowRationale = false. If you deny it and open the screen again, the dialog is shown and shouldShowRationale = true. If you deny again and come back to the screen, shouldShowRationale = false again and no dialog is shown.

So if I understand you correctly, you can launch the request when you enter your barcode screen and simply check if (permissionState.status.shouldShowRationale) {} else {} in your composable to show additional information.

@PaulWoitaschek
Copy link

PaulWoitaschek commented Aug 3, 2022

Yes but that does not handle the case where the user denies the permission permanently.

To handle the permission, I would need to do sth like this:

  val permissions = rememberPermissions()
  when {
    permissions.granted -> {
      doSomethingWithpermission()
    }
    permissions.deniedAndShowRationale -> {
      Dialog("we need permissions.") {
        permissions.request()
      }
    }
    permissions.deniedAndNotAsked -> {
      permissions.request()
    }
    permissions.deniedButAsked -> {
      Dialog("Enable the permissions in the settings") {
        goToSystemSettings()
      }
    }
  }

So if I understand you correctly, you can launch the request when you enter your barcode screen and simply check if (permissionState.status.shouldShowRationale) {} else {} in your composable to show additional information.

If I were to do it like that, the code in the the second {} would lead the user to the system settings. And that would immediately happen before the user answered the initial permission request.

I don't see how this is possible without adding to the state if the user was actually asked for the permissions.

@ln-12
Copy link
Contributor

ln-12 commented Aug 3, 2022

I can see your problem now. In my project, I don't call goToSystemSettings() without user interaction. So my screen shows a message which says that I need this permission and only after pressing a button, the user is directed to the settings. I think that it is also not the best idea to just open the settings without explaining it first. I would suggest you to simply wrap you goToSystemSettings() in a Button and add some explanation. If instead the permission dialog is triggered, the user won't see that message as it is changed immediately after confirming.

If you directly want to open the settings, you would have to save the state somewhere which might lead to problems after the permissions are manually changed in the settings.

@PaulWoitaschek
Copy link

PaulWoitaschek commented Aug 3, 2022

Yes, that's how we do it too, though through a dialog. I updated my sample to match that request. That doesn't solve the problem though.
The problem is that there is no way to differentiate between "user asked and denied" and "user wasn't asked and not granted yet".

So the only way I can think if to solve it with accompanist is to show a dialog before requesting the permission in the first place.
But that's bad UX, the user knows already what the camera is for so we just want to ask him the for the permission before we show any rationale.

It would be super helpful to add that flow to the samples.

@alexvanyo
Copy link
Collaborator

there is no way to differentiate between "user asked and denied" and "user wasn't asked and not granted yet"

Right, that's unfortunately not something accompanist can solve.

As Manuel mentioned in the PR description, the route the new APIs encourage is always having a button that would request the permission since we can't in general distinguish the "permanently denied" case. There's a possibility that you can still directly request the permission.

You can use the callback in the rememberPermissionState to get a signal to update your own state upon getting a result back from the system when the request has been denied or approved (and that also gets called in the case where the user doesn't see the permission dialog at all because they've permanently denied the permission)

@akardas16
Copy link

Check out https://github.com/akardas16/Compose-Permissions for better implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants