Skip to content

Conversation

@Iskander508
Copy link
Contributor

Summary

Fix for #608

Compatibility

OS Implemented
iOS no change
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md (where is the changelog 😄 )
  • I updated the typed files (TS and Flow) (no change in js)
  • I added a sample use of the API in the example project (example/App.js)

@Iskander508 Iskander508 requested a review from zoontek as a code owner May 20, 2021 18:38
Copy link

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

interesting - it looks more correct / easier to read by using the necessary / already existing permissionsToCheck collection to also drive the iterations and completion condition...but, I don't understand how this fixes the issue? I am certain I'm just not reading it and seeing it, but how where was the issue in the original extra-counter style leading to the problem, such that using the collection directly fixes it? 🤔

@Iskander508
Copy link
Contributor Author

interesting - it looks more correct / easier to read by using the necessary / already existing permissionsToCheck collection to also drive the iterations and completion condition...but, I don't understand how this fixes the issue? I am certain I'm just not reading it and seeing it, but how where was the issue in the original extra-counter style leading to the problem, such that using the collection directly fixes it? 🤔

The "buggy" part is the last part in the try block. The changes before that are truly only for better readability of the code. The use of the complete permissions list is the mistake. When using the complete list, then later the rationaleStatuses array doesn't match in the callback, where only the items from permissionsToCheck are taken into account.

@zoontek
Copy link
Owner

zoontek commented May 20, 2021

Could you only fix the issue and remove other changes? The android part of this module is a slightly edited version of https://github.com/facebook/react-native/blob/b161241db2ef74d2e4bff36d4972f5f0312dcc44/ReactAndroid/src/main/java/com/facebook/react/modules/permissions/PermissionsModule.java#L143 and I really want to keep it as close as the original for easier updates.

Copy link

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Now the error and correction are obvious! I think the previous version was a bit cleaner but also have a different project that has to track an upstream source so I understand now wanting better code even if it's better.

@zoontek
Copy link
Owner

zoontek commented May 22, 2021

Just tried it, the fix works flawlessly 👍
Thanks @Iskander508

@zoontek zoontek merged commit 09f6f92 into zoontek:master May 22, 2021
@efstathiosntonas
Copy link

thanks guys, this was driving me crazy and i was using workarounds to mitigate it!

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.

4 participants