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

Add NotGranted to PermissionState for Android #105

Merged
merged 3 commits into from
Apr 14, 2024

Conversation

jacobras
Copy link
Contributor

@jacobras jacobras commented Feb 29, 2024

Here's one idea to fix the issue with [deny and don't ask again] returning an incorrect state (#104), by adding a new state for Android: NotGranted. It's the most accurate description of the situation: it could be NotDetermined or DeniedAlways but there's no way to determine the difference between [deny] and [always deny] when requesting the state. One has to keep track of whether a permission has been requested in the app already, so that's up to consumers.

This change does not affect iOS' behaviour, nor providePermission()/isPermissionGranted() behaviour. Only on for requesting the current state on Android.

Alternative solution would be to add no different state, but make Android return Denied by default. Benefit is that the state enum doesn't change and existing apps would be likely to keep working. Downside is that the default state is Denied and that could look like a bug. But again, a documentation update helps.

Before

See screen capture in #104

After

MokoPermissionStateBugAndroidFixed

Alex009 and others added 3 commits December 7, 2023 20:46
This fixes the issue with [deny and don't ask again] returning an incorrect state. Unfortunately, it's not possible on Android to determine the difference. One has to keep track of whether a permission has been requested in the app already, so that's up to consumers.
@jacobras jacobras force-pushed the bugfix/permission-state-android branch from c528d79 to 1ed2108 Compare March 8, 2024 10:03
@jacobras
Copy link
Contributor Author

jacobras commented Mar 8, 2024

Looking for opinions here on the 3 possible solutions I see :) One issue is that now NotDetermined is never used on Android, making it iOS-only. So the whole thing would look like this:

1️⃣ This PR:

  • NotDetermined iOS-only
  • NotGranted Android-only
  • Granted Both
  • Denied Android-only
  • DeniedAlways Both, but on Android only for push

Can't say that looks ideal.

2️⃣ The alternative approach I mentioned in the description above (returning "denied" by default on Android) would look like this:

  • NotDetermined iOS-only, starting state
  • Denied Both, starting state on Android
  • Granted Both
  • DeniedAlways Both, but on Android only for push

Of course, that changes behaviour for existing apps using this library.

3️⃣ Simplest change would be no behaviour change, but only a documentation update. Then technically it remains as it's currently:

  • NotDetermined Both, but on Android also meaning "denied always"
  • Denied Both
  • Granted Both
  • DeniedAlways Both, but on Android only for push

@Alex009 Alex009 changed the base branch from master to develop April 11, 2024 03:02
@Alex009 Alex009 added this to the 0.18.0 milestone Apr 11, 2024
@Alex009 Alex009 linked an issue Apr 11, 2024 that may be closed by this pull request
@ZiXOps
Copy link

ZiXOps commented Apr 12, 2024

looks good i check with changes on both platforms

@Alex009 Alex009 merged commit 42ade68 into icerockdev:develop Apr 14, 2024
1 check passed
@ZiXOps ZiXOps mentioned this pull request Apr 17, 2024
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.

getPermissionState() is incorrect for [deny & don't ask again]
3 participants