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

Fix incorrect predicate in PermissionControllerImpl on Android #137

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

MrPranklin
Copy link

In PR #105 permission state NotGranted is introduced and the negation when checking shouldShowRequestPermissionRationale is removed.

The desired result in that change is to return PermissionState.Denied in case that shouldShowRequestPermissionRationale returns true for all permissions, and PermissionState.NotGranted otherwise. That is correct behavior according to the documentation

However, in PR #115, possibly due to conflict resolution, the negation is brought back, while the return statement stays as it was in #105, resulting in the default state of the permission to be Denied (instead of NotGranted), which is incorrect.

@MrPranklin MrPranklin changed the title Fix incorrect condition check Fix incorrect condition check in PermissionControllerImpl on Android Oct 28, 2024
@MrPranklin MrPranklin changed the title Fix incorrect condition check in PermissionControllerImpl on Android Fix incorrect predicate in PermissionControllerImpl on Android Oct 28, 2024
@Alex009
Copy link
Member

Alex009 commented Nov 4, 2024

@MrPranklin hi. why default state should be Denied? at first run user should receive some UI for NotGranted state. but if we will have Denied - user will see "You denied access, please give us permissions".

@MrPranklin
Copy link
Author

Hi @Alex009, in version 0.18.1 I'm getting Denied as the default state and then NotGranted after denying once, which is incorrect.

I found the reason for that to be the series of changes I described with the 2 pull requests.

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

Successfully merging this pull request may close these issues.

2 participants