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

Implements #182 #183

Closed
wants to merge 6 commits into from
Closed

Implements #182 #183

wants to merge 6 commits into from

Conversation

bradmartin
Copy link
Contributor

No description provided.

@ghost ghost added the new PR label Jan 8, 2019
@bradmartin
Copy link
Contributor Author

Implements #182

This is an optional setting for the consumer to pass when calling isEnabled(options).

I've mentioned this on the camera plugin, and plan to implement the same flow in a PR shortly as well. @tbozhikov liked the suggestion there.

I've ran into a situation where I'd like to open the settings is the enabled check resolves false and this.

@bradmartin bradmartin changed the title Implements Implements #182 Jan 9, 2019
@bradmartin
Copy link
Contributor Author

Just realized currently android has options for isEnabled so going to change this accordingly and just add new value to the Options interface about iOS doing this when it fails.

@bradmartin
Copy link
Contributor Author

Okay, made the change necessary. So now the Options can be passed to isEnabled on iOS as well, but the only check will be for the new prop value to open the settings if it is disabled.

@bradmartin
Copy link
Contributor Author

Added doc on the typings about iOS using the new value for the isEnabled method.

@bradmartin
Copy link
Contributor Author

Working more on this UX flow today in an app and even this would sometimes be the wrong approach.

Scenario: isEnabled is false and we open the settings app without ever having presented the user with the location permission. At this point Location would not be listed in the settings since the user has not had a chance to Allow or Deny it.

Only when the status is kCLAuthorizationStatusDenied has the user denied it on their device. This would be the scenario where opening the settings on iOS would make sense.

I'm going to rethink this PR a bit and see if I can provide a more elegant approach without breaking anything.

@bradmartin
Copy link
Contributor Author

bradmartin commented Jan 9, 2019

@lini @tbozhikov - not sure who is doing most of the work on this plugin anymore but know you're both contributors 😄.

This PR has changed a bit since I first created it so I want to recap the status and why so you're aware of the details.

I have an app where I've been getting logs with locationManagerDidChangeAuthorizationStatus in it, you can see this Sentry log if you'd like.

So I went digging and my use case was fairly standard, check if the geolocation was enabled and then try to get the current location.

Of course if the user has previously denied the Location permission prompt iOS provides, nothing happens.

So this PR is providing two separate things:

  1. The new iosOpenSettingsIfLocationHasBeenDenied on the Options interface which is already an argument on android. I didn't like using that but I didn't want to break anything, feel free to improve this by redoing some of the current logic if necessary. So when this is true for the isEnabled function, it will check if the Auth status has been denied (user had their chance to allow location permission the one time iOS does) and if so it will open the settings so they can change the location permission then quickly get back to the app and try again.

  2. getIOSLocationManagerStatus new method iOS only that just returns the auth status of the CLLocationManager. I think this is all that is required but I did not test heavily since it requires uninstalling and reinstalling over and over to get the initial permission prompt on iOS. This function can be useful when consumers of the plugin just want to check the status themselves and handle accordingly in their own app.

I believe everything has been documented on the README and that this is not a breaking change since android remains the same and only iOS has been altered with a new configuration option on the isEnabled method.

Copy link
Contributor

@tbozhikov tbozhikov left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @bradmartin! I've made some in-line comments.
In general, I think that the enableLocationRequest() method is a better place to put the logic for opening the Settings app to enable Location services by hand. I am pasting code here since I cannot commit directly in your repo:

  • geolocation.ios.ts
export function enableLocationRequest(always?: boolean, iosOpenSettingsIfLocationHasBeenDenied?: boolean): Promise<void> {
    return new Promise<void>(function (resolve, reject) {
        const locationIsEnabled = _isEnabled();

        if (locationIsEnabled) {
            resolve();
            return;
        } else {
            const status = getIOSLocationManagerStatus();
            if (status === CLAuthorizationStatus.kCLAuthorizationStatusDenied &&
                iosOpenSettingsIfLocationHasBeenDenied) {
                // now open the Settings so the user can toggle the Location permission
                utils.ios.getter(UIApplication, UIApplication.sharedApplication).openURL(NSURL.URLWithString(UIApplicationOpenSettingsURLString));
            } else {
                let listener = LocationListenerImpl.initWithPromiseCallbacks(resolve, reject, always);
                try {
                    let manager = getIOSLocationManager(listener, null);
                    if (always) {
                        manager.requestAlwaysAuthorization();
                    } else {
                        manager.requestWhenInUseAuthorization();
                    }
                } catch (e) {
                    LocationMonitor.stopLocationMonitoring(listener.id);
                    reject(e);
                }
            }
        }
    });
}

... and below is how I imagine the other 3 methods would look like:

function _isEnabled(): boolean {
    if (CLLocationManager.locationServicesEnabled()) {
        const status = getIOSLocationManagerStatus();

        // CLAuthorizationStatus.kCLAuthorizationStatusAuthorizedWhenInUse and
        // CLAuthorizationStatus.kCLAuthorizationStatusAuthorizedAlways are options that are available in iOS 8.0+
        // while CLAuthorizationStatus.kCLAuthorizationStatusAuthorized is here to support iOS 8.0-.
        // const AUTORIZED_WHEN_IN_USE = CLAuthorizationStatus.kCLAuthorizationStatusAuthorizedWhenInUse;
        return (status === CLAuthorizationStatus.kCLAuthorizationStatusAuthorizedWhenInUse
            || status === CLAuthorizationStatus.kCLAuthorizationStatusAuthorizedAlways
            || status === CLAuthorizationStatus.kCLAuthorizationStatusAuthorized);
    }
    return false;
}

export function isEnabled(options: Options): Promise<boolean> {
    return new Promise(function (resolve, reject) {
        const isEnabledResult = _isEnabled();
       
        resolve(isEnabledResult);
    });
}

export function getIOSLocationManagerStatus(): CLAuthorizationStatus {
    return CLLocationManager.authorizationStatus();
}
  • main-page.ts
    add the second parameter to enableLocationRequest() call:
geolocation.enableLocationRequest(true, true)

[optional] Also, you may mention in the README, that the iosOpenSettingsIfLocationHasBeenDenied option is an argument in the enableLocationRequest() method. If you don't I will add this later.

What do you think of these changes?

return new Promise(function (resolve, reject) {
resolve(_isEnabled());
const isEnabledResult = _isEnabled();
const status = CLLocationManager.authorizationStatus();
Copy link
Contributor

Choose a reason for hiding this comment

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

we could reuse the getIOSLocationManagerStatus(), i.e.:

const status = CLLocationManager.authorizationStatus();

@@ -76,6 +76,8 @@ geolocation.getCurrentLocation({ desiredAccuracy: Accuracy.high, maximumAge: 500
| timeout | 5 minutes | How long to wait for a location in ms. |
| iosAllowsBackgroundLocationUpdates | false | If enabled, UIBackgroundModes key in info.plist is required (check the hint below). Allow the application to receive location updates in background (ignored on Android). Read more in [Apple document](https://developer.apple.com/documentation/corelocation/cllocationmanager/1620568-allowsbackgroundlocationupdates?language=objc) |
| iosPausesLocationUpdatesAutomatically | true | Allow deactivation of the automatic pause of location updates (ignored on Android). Read more in [Apple document](https://developer.apple.com/documentation/corelocation/cllocationmanager/1620553-pauseslocationupdatesautomatical?language=objc)|
| iosOpenSettingsIfLocationIsDisabled | false | If true when the `isEnabled` method is invoked, the settings app will open on iOS so the user can change the location services permission. |
Copy link
Contributor

Choose a reason for hiding this comment

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

iosOpenSettingsIfLocationIsDisabled -> iosOpenSettingsIfLocationHasBeenDenied

@bradmartin
Copy link
Contributor Author

I like this better than using isEnabled 👍 - you can ignore my PR and just use your snippet in that case. Works for me, since you already have the code done 😄 .

Thanks for reviewing it.

@tbozhikov
Copy link
Contributor

new PR with some tweaks added -> #185, good job @bradmartin!

@tbozhikov tbozhikov closed this Jan 15, 2019
@ghost ghost removed the new PR label Jan 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants