-
-
Notifications
You must be signed in to change notification settings - Fork 828
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
iOS: Allow requesting LOCATION_ALWAYS if LOCATION_WHEN_IN_USE is granted. #529
Conversation
Amazing! Thank you so much, working flawlessly! |
@Peeeep happy to help! Make sure you read the limitations in the README! |
@@ -407,6 +407,16 @@ Permission checks and requests resolve into one of these statuses: | |||
| `RESULTS.GRANTED` | The permission is granted | | |||
| `RESULTS.BLOCKED` | The permission is denied and not requestable anymore | | |||
|
|||
### Getting the result of `PERMISSION.IOS.LOCATION_ALWAYS` on iOS 13 and later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but this is a no-go for me. A never ending Promise
could arm a lot of applications, and unfortunately we can't rely on the fact that developers read the documentation before implementing the feature 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you suggest handling it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know yet. But that's precisely why I don't implemented this in the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any way to have this permission work with the v2 API without having a promise that might never resolve.
For v3 where we have a new API we can do something like have this call resolve instantly and then emit an event when the permission is granted, and don't emit any event if the permission was denied -- but v3 will take a while to be released, and for existing v2 users I think it's better to have a solution with a caveat instead of a bug 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a bug, but more of a missing feature. A Promise that never resolve is a bug.
The first one leads to feature requests, the second to issues.
I'd rather see people relying on patch-package
and be sure understand what they're doing than providing a broken (because of the current API design) feature.
Note that it's also doable to create a custom PermissionHandler fork without forking the entire repository :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough -- do you wanna close this PR then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really for now, I'm sure it could be possible in some way to return blocked
in these cases. I just don't have the bandwidth to check this currently (because of #522)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zoontek @tallpants wouldn't it make sense to rely on applicationDidBecomeActive
(or other app lifecycle method?) instead of (or along with) didChangeAuthorizationStatus
as when you prompt the always permissions, app isn't on foreground anymore and changes back when the request dialog closes?
So far this seems to be working fine for me
- (void)requestWithResolver:(void (^ _Nonnull)(RNPermissionStatus))resolve
....
[_locationManager setDelegate:self];
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(handleAppStateDidChange:)
name:UIApplicationDidBecomeActiveNotification
object:nil];
...
- (void)updatePermission {
if(_resolve == nil || _reject == nil) return;
[self checkWithResolver:_resolve rejecter:_reject];
[_locationManager setDelegate:nil];
_resolve = nil;
_reject = nil;
[[NSNotificationCenter defaultCenter] removeObserver:self name:UIApplicationDidBecomeActiveNotification object:nil];
}
- (void)locationManager:(CLLocationManager *)manager didChangeAuthorizationStatus:(CLAuthorizationStatus)status {
if (status != kCLAuthorizationStatusNotDetermined && status != kCLAuthorizationStatusAuthorizedWhenInUse) {
[self updatePermission];
}
}
- (void)handleAppStateDidChange:(NSNotification *)notification {
if([notification.name isEqualToString:UIApplicationDidBecomeActiveNotification]) {
[self updatePermission];
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a bug, but more of a missing feature. A Promise that never resolve is a bug.
The first one leads to feature requests, the second to issues.I'd rather see people relying on
patch-package
and be sure understand what they're doing than providing a broken (because of the current API design) feature.Note that it's also doable to create a custom PermissionHandler fork without forking the entire repository :)
How would I go about only forking the PermissionHandler instead of the entire repository? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would I go about only forking the PermissionHandler instead of the entire repository? Thanks!
I wouldn't recommend that. I believe what you want is https://github.com/ds300/patch-package
I am using a patch and found a good workaround for finding out whether the user granted Always permissions or not. Rather than awaiting the |
Will this be merged any time soon? Could really use this method for my project this week as Location Always is a necessity. |
Tested this change too and it looks pretty necessary. Without it on iOS 13+ we get blocked rather than upgrading the location when in use to always. |
Any updates on this? |
@antonigiske looks like it needs to be updated to current, then the commentary above needs to be handled |
Any updates on this? |
I used https://github.com/ds300/patch-package with this PR. That worked. |
Hey folks, this is open source. In other words, not to be rude, but...get to work? Like, it needs your work to fix it. From a month ago
...and it looks like the magic work fairy hasn't shown up and done anything yet, so it's waiting on you! Someone grab the code fix it up so it doesn't have conflicts and post a new PR. Make sure to address the comments from the review: #529 (review) Until then, use patch-package I guess. But someone needs to do the work, that's the way work gets done |
Like @mikehardy said, this is open source, everyone can contribute! But the points from the review need to be tackled indeed. I'm currently don't use these permission handlers personally and have other projects / libraries, so it's not a priority for me to implement this. But if your company needs this feature, I can take one day of my time to code it (without the never ending Promise, with tests on devices from iOS 10 to 15 and obviously a new release). For this, my freelance daily rate will apply. |
@zoontek would you be okay merging this if I update the PR with @nasmuris's approach here to avoid having the unresolved promise? #529 (comment) Would really like to get this feature in after it's been up for so long |
Hello, my company need this feature, any solution or update about that? Thanks! |
Looks like someone needs to sponsor it or do the updates like @zoontek's comment indicates and propose that |
Summary
LOCATION_ALWAYS
permission if you already hadLOCATION_WHEN_IN_USE
permission.LocationAlways
pod.pod install
in the example project.)Implementation changes
check
:AuthorizedWhenInUse
, and we have not requestedLOCATION_ALWAYS
in the past, we returnNotDetermined
.AuthorizedWhenInUse
and we have requestedLOCATION_ALWAYS
in the past, we returnDenied
, since we're not allowed to prompt for this permission twice.request
:AuthorizedWhenInUse
and we have not requestedLOCATION_ALWAYS
in the past, we callrequestAlwaysAuthorization
and flag the permission as requested.AuthorizedWhenInUse
and we have requestedLOCATION_ALWAYS
in the past, then we return the current authorization result, since we're not allowed to prompt for this permission twice.Test Plan
LOCATION_WHEN_IN_USE
permission.LOCATION_ALWAYS
permission.Compatibility
Checklist
README.md
CHANGELOG.md
] I mentioned this change inCHANGELOG.md
example/App.js
)