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

PermissionGranter should click *foreground* grant button for location #356

Merged
merged 4 commits into from
Aug 5, 2020

Conversation

growse
Copy link
Contributor

@growse growse commented Jun 25, 2020

On Android Q, "com.android.permissioncontroller:id/permission_allow_button" isn't guaranteed to exist on permission requests where the app is requesting location permissions and only needs them for the foreground activity / service. See this example:

image

Here, com.android.permissioncontroller:id/permission_allow_button doesn't exist.

Looks like a small enhancement is needed to detect asking for location permission and switch to matching the "foreground only" button on the presented dialog.

…rmission request

On Android Q, `"com.android.permissioncontroller:id/permission_allow_button"` isn't guaranteed to exist on permission requests where the app is requesting location permissions and only needs them for the foreground activity / service.

Looks like a small enhancement is needed to detect asking for location permission and switch to matching the "foreground only" button on the presented dialog.
@Sloy
Copy link
Member

Sloy commented Aug 4, 2020

Hello @growse, so sorry for the late reply.
I checked and you're absolutely right, the current PermissionGranter fails when requesting location permissions in Android Q.

I was going to ask you to add a regression test to verify the misbehavior, but I realized that the PermissionGranterTest is far from perfect, so please let me make some quick changes, add the missing test and then I'll merge your PR with the fix.

Copy link
Member

@Sloy Sloy left a comment

Choose a reason for hiding this comment

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

Done! I've merged the changes in PermissionGranterTest and enabled the Location permission test in this PR.
I'll try to make the build pass (the workflow doesn't seem to be very stable) and merge asap.

@growse Thanks a lot for the contribution! We appreciate it :)

@Sloy Sloy added the bug label Aug 5, 2020
@Sloy Sloy merged commit 2860827 into AdevintaSpain:master Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants