Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Refactor iOS location authorization delegate method #1598

Closed
wants to merge 3 commits into from
Closed

Refactor iOS location authorization delegate method #1598

wants to merge 3 commits into from

Conversation

friedbunny
Copy link
Contributor

This pull request adds [MGLMapView mapView:didChangeLocationAuthorizationStatus:] as a pass-thru delegate method for [CLLocationManager locationManager:didChangeAuthorizationStatus:].

This PR also removes [MGLMapView mapView:didFailToLocateUserWithError:], because it was never being called when user location permissions changed. In my testing on iOS 7 & 8, on device and in sim, I never got this method to fire. It may still be useful for other purposes or catastrophic cases that I can't easily test for, but not for reliably indicating location permission authorization.

I didn't deprecate [MGLMapView mapView:didFailToLocateUserWithError:] — should we? Update: readded mapView:didFailToLocateUserWithError: for MapKit parity.

/cc @1ec5 @incanus @bleege

This new delegate method passes along the `didChangeAuthorizationStatus` CoreLocation delegate method.
This method was not being called when location authorization status was changing, making it unnecessary.
@1ec5
Copy link
Contributor

1ec5 commented May 19, 2015

-[MGLMapView mapView:didFailToLocateUserWithError:] is our pass-through for -[CLLocationManagerDelegate locationManager:didFailWithError:]:

The location manager calls this method when it encounters an error trying to get the location or heading data. If the location service is unable to retrieve a location right away, it reports a kCLErrorLocationUnknown error and keeps trying. In such a situation, you can simply ignore the error and wait for a new event. If a heading could not be determined because of strong interference from nearby magnetic fields, this method returns kCLErrorHeadingFailure.

If the user denies your application’s use of the location service, this method reports a kCLErrorDenied error. Upon receiving such an error, you should stop the location service.

It’s also worth noting that MapKit provides for an -[MKMapViewDelegate mapView:didFailToLocateUserWithError:], which probably does the same thing. We should try to match that unless there’s a strong reason not to.

@1ec5
Copy link
Contributor

1ec5 commented May 19, 2015

Did you try sticking the phone next to a stereo or in a refrigerator, @friedbunny? 😉

@friedbunny
Copy link
Contributor Author

@1ec5 I don't know why locationManager:didFailWithError: isn't offering kCLErrorDenied like the docs say it should, seems like that never got hooked up in CoreLocation or it refers to something else?

This does sound like it should work, though:

typedef NS_ENUM(NSInteger, CLError) {
    ...
    kCLErrorDenied, // Access to location or ranging has been denied by the user
    ...
}

In any case, we weren't passing anything besides kCLErrorDenied along in our didFailToLocateUserWithError delegate method, so it feels safe to kill it.

@friedbunny
Copy link
Contributor Author

It’s also worth noting that MapKit provides for an -[MKMapViewDelegate mapView:didFailToLocateUserWithError:], which probably does the same thing. We should try to match that unless there’s a strong reason not to.

Parity is a good reason to keep this, but we'll have to change it to pass everything through.

@friedbunny
Copy link
Contributor Author

As much as I wanted to kill mapView:didFailToLocateUserWithError: for betraying me, I've added it back but decoupled the method from kCLErrorDenied to make it transparently pass everything along.

@incanus
Copy link
Contributor

incanus commented May 19, 2015

Yeah we should keep it for parity.

@incanus incanus added iOS Mapbox Maps SDK for iOS refactor labels May 19, 2015
@friedbunny
Copy link
Contributor Author

MapKit's mapView:didFailToLocateUserWithError: does work as expected, first throwing a MapKit error with a nice recovery suggestion, then throwing kCLErrorDenied:

didFailToLocateUserWithError: Error Domain=MKLocationErrorDomain Code=0 "The operation couldn’t be completed. (MKLocationErrorDomain error 0.)" UserInfo=0x1850a030 {NSLocalizedRecoverySuggestion=Turn On Location Services to Allow (null) to Determine Your Location}
didFailToLocateUserWithError: Error Domain=kCLErrorDomain Code=1 "The operation couldn’t be completed. (kCLErrorDomain error 1.)"

I'll keep looking into why we aren't getting kCLErrorDenied.

@friedbunny
Copy link
Contributor Author

OK, figured it out: we were turning off the location manager before it got to didFailWithError:.

By setting showsUserLocation = NO in didChangeAuthorizationStatus:, we accidentally prevent subsequent delegate messages from being sent:

- (void)locationManager:(__unused CLLocationManager *)manager didChangeAuthorizationStatus:(CLAuthorizationStatus)status
{
    if (status == kCLAuthorizationStatusDenied || status == kCLAuthorizationStatusRestricted)
    {
        self.userTrackingMode  = MGLUserTrackingModeNone;
        self.showsUserLocation = NO;
    }
}

Using didChangeAuthorizationStatus: and didFailWithError: is redundant, as far as I can tell. MapKit does not expose authorization status.

I think we should get rid of the checks in didChangeAuthorizationStatus: and rely solely on didFailWithError: and our delegate method mapView:didFailToLocateUserWithError:.

@friedbunny
Copy link
Contributor Author

Closed in favor of #1608.

@friedbunny friedbunny closed this May 20, 2015
@friedbunny friedbunny deleted the didChangeAuthorizationStatus-delegate-method branch May 25, 2015 06:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants