[Connectivity] add a method to request location on iOS (for iOS 13) #1999

cyanglaz · 2019-08-21T22:38:44Z

Description

Starting from iOS 13. Certain apps need to have user authorized location service to get WIFI information using CNCopyCurrentNetworkInfo. Detailed in flutter/flutter#37804

We add a helper method for the plugin users to request or get location service authorization.
Also updated the REAMDE to explain the iOS 13 changes.

Related Issues

flutter/flutter#37804

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

collinjackson

Long term I'm hoping this will move to a shared plugin, so I'm fine with whatever you want to do here.

However, it feels like we're conflating requesting permission and checking what the current status is. I'm wondering if there should be separate APIs for each. What do you think?

Compare with the React Native implementation

packages/connectivity/README.md

collinjackson · 2019-08-23T20:22:18Z

packages/connectivity/example/ios/Runner/Info.plist

+	<key>NSLocationAlwaysAndWhenInUseUsageDescription</key>
+	<string>Location service</string>
+	<key>NSLocationWhenInUseUsageDescription</key>
+	<string>locaiton service in use</string>


Typo, also I think you should put an English sentence here

collinjackson · 2019-08-23T20:22:37Z

packages/connectivity/example/ios/Runner/Info.plist

@@ -22,6 +22,10 @@
 	<string>1</string>
 	<key>LSRequiresIPhoneOS</key>
 	<true/>
+	<key>NSLocationAlwaysAndWhenInUseUsageDescription</key>
+	<string>Location service</string>


I think you should put an appropriate English sentence here since it's an example app.

packages/connectivity/README.md

collinjackson · 2019-08-23T20:29:07Z

packages/connectivity/README.md


-You can follow issue [#37804](https://github.com/flutter/flutter/issues/37804) for the changes required to return valid SSID and BSSID values with iOS 13.
+* `NSLocationAlwaysAndWhenInUseUsageDescription` - describe why the app needs access to the user’s location information all the time (foreground and background). _Privacy - Location Always and When In Use Usage Description_ in the visual editor.


Suggested change

* `NSLocationAlwaysAndWhenInUseUsageDescription` - describe why the app needs access to the user’s location information all the time (foreground and background). _Privacy - Location Always and When In Use Usage Description_ in the visual editor.

* `NSLocationAlwaysAndWhenInUseUsageDescription` - describe why the app needs access to the user’s location information all the time (foreground and background). This is called _Privacy - Location Always Usage Description_ in the visual editor.

I might be wrong here but I think it should just be Privacy - Location Always Usage Description

That's what I thought before writing this PR too :) Appearently the name has been changed.
https://developer.apple.com/documentation/bundleresources/information_property_list/nslocationalwaysandwheninuseusagedescription?language=objc

Can you add "This is called" from my suggestion above?

collinjackson · 2019-08-23T20:33:33Z

packages/connectivity/ios/Classes/ConnectivityPlugin.m

+  CLAuthorizationStatus status = CLLocationManager.authorizationStatus;
+  switch (status) {
+    case kCLAuthorizationStatusNotDetermined: {
+      self.result = result;


I think there's a race condition here where two calls to requestLocationServiceAuthorizationIfUndetermined in quick succession could lead to result being clobbered before being invoked. Consider returning an error if self.result != null or handling the case gracefully.

collinjackson · 2019-08-23T20:36:41Z

packages/connectivity/ios/Classes/ConnectivityPlugin.m

+}
+
+- (void)locationManager:(CLLocationManager *)manager didChangeAuthorizationStatus:(CLAuthorizationStatus)status {
+  [self requestLocationServiceAuthorizationIfUndetermined:self.result always:nil];


Should probably set self.result to nil here so we don't accidentally notify it more than once.

I added a new class FLTConnectivityLocationHandler to handle all the location related stuff so now the result is inline.
FLTConnectivityLocationHandler handles the race condition that you mentioned by returning notDetermined if a request is in process.

cyanglaz · 2019-08-24T00:27:36Z

However, it feels like we're conflating requesting permission and checking what the current status is. I'm wondering if there should be separate APIs for each. What do you think?

Sure. My initial implementation did actually have these separated. I can add it back.

Co-Authored-By: Collin Jackson <jackson@google.com>

cyanglaz · 2019-08-27T00:05:04Z

@collinjackson It is ready for another review.

collinjackson

LGTM with some minor nits.

Might want to integration test the default value of getLocationServiceAuthorization even if the infrastructure's not quite there yet to do requestLocationServiceAuthorization yet.

collinjackson · 2019-08-28T00:03:21Z

packages/connectivity/README.md


-You can follow issue [#37804](https://github.com/flutter/flutter/issues/37804) for the changes required to return valid SSID and BSSID values with iOS 13.
+* `NSLocationAlwaysAndWhenInUseUsageDescription` - describe why the app needs access to the user’s location information all the time (foreground and background). _Privacy - Location Always and When In Use Usage Description_ in the visual editor.


Can you add "This is called" from my suggestion above?

collinjackson · 2019-08-28T00:04:01Z

packages/connectivity/example/ios/Runner/Info.plist

@@ -22,6 +22,10 @@
 	<string>1</string>
 	<key>LSRequiresIPhoneOS</key>
 	<true/>
+	<key>NSLocationAlwaysAndWhenInUseUsageDescription</key>
+	<string>This app requires accessing your location information all the time to get WIFI information.</string>


Check caps on WIFI, consider wi-fi

collinjackson · 2019-08-28T00:04:30Z

packages/connectivity/example/ios/Runner/Info.plist

+	<key>NSLocationAlwaysAndWhenInUseUsageDescription</key>
+	<string>This app requires accessing your location information all the time to get WIFI information.</string>
+	<key>NSLocationWhenInUseUsageDescription</key>
+	<string>This app requires accessing your location information when the app is in foreground to get WIFI information.</string>


Check caps on WIFI, consider wi-fi

collinjackson · 2019-08-28T00:05:57Z

packages/connectivity/example/lib/main.dart

+                status == LocationAuthorizationStatus.authorizedWhenInUse) {
+              wifiName = await _connectivity.getWifiName();
+            } else {
+              print(


I'm not thrilled about this print statement, it seems like it could clutter the logs. Do you think it's necessary?

collinjackson · 2019-08-28T00:07:51Z

packages/connectivity/ios/Classes/FLTConnectivityLocationHandler.m

+  }
+
+  if (self.completion) {
+    // If a request is still in process, immdediately return.


Suggested change

// If a request is still in process, immdediately return.

// If a request is still in process, immediately return.

collinjackson · 2019-08-28T00:08:17Z

packages/connectivity/lib/connectivity.dart

+  ///
+  /// Returns a [LocationAuthorizationStatus] after user authorized or denied the location on this request.
+  ///
+  /// if the location information needs to be accessible all the time, set `requestAlwaysLocationUsage` to true. If user has


Suggested change

/// if the location information needs to be accessible all the time, set `requestAlwaysLocationUsage` to true. If user has

/// If the location information needs to be accessible all the time, set `requestAlwaysLocationUsage` to `true`. If user has

collinjackson · 2019-08-28T00:09:44Z

packages/connectivity/lib/connectivity.dart

+  ///
+  /// See also [requestLocationServiceAuthorization] for requesting a location service authorization.
+  Future<LocationAuthorizationStatus> getLocationServiceAuthorization() async {
+    //Just `assert(Platform.isIOS)` will disable us to do dart side unit testing.


Suggested change

//Just `assert(Platform.isIOS)` will disable us to do dart side unit testing.

// Just `assert(Platform.isIOS)` will prevent us from doing Dart side unit testing.

collinjackson · 2019-08-28T00:14:55Z

packages/connectivity/lib/connectivity.dart

@@ -93,6 +94,125 @@ class Connectivity {
  Future<String> getWifiIP() async {
    return await methodChannel.invokeMethod<String>('wifiIPAddress');
  }
+
+  /// Request to authorize the location service. Only on iOS.


Suggested change

/// Request to authorize the location service. Only on iOS.

/// Request to authorize the location service (only on iOS).

I think Dartdoc comments are supposed to have tight first sentence with no other sentences on the first line. https://dart.dev/guides/language/effective-dart/documentation

collinjackson · 2019-08-28T00:15:19Z

packages/connectivity/lib/connectivity.dart

+    return _convertLocationStatusString(result);
+  }
+
+  /// Get the current location service authorization. Only on iOS.


Suggested change

/// Get the current location service authorization. Only on iOS.

/// Get the current location service authorization (only on iOS).

cyanglaz · 2019-08-28T21:50:51Z

@collinjackson Thanks for the review. I updated the code with your review suggests; also added a driver test to test the getLocationServiceAuthorization. The other method requestLocationServiceAuthorization is probably not testable as it for now since it will show a pop up and I'm not sure if there is a way to handle system pop ups from flutter driver.
Im going to merge this on green if you don't have any objections.

collinjackson

It looks like you have some test failures but I'm LGTM on this once you've resolved them.

…lutter#1999)

Chris Yang added 4 commits August 8, 2019 16:09

draft

5c8d7f3

add a method to request location

7201c6f

cleanup

bb199d4

update version

0fd8cd9

googlebot added the cla: yes label Aug 21, 2019

cyanglaz changed the title ~~Connectivity add a method to request location on iOS (for iOS 13)~~ [Connectivity] add a method to request location on iOS (for iOS 13) Aug 21, 2019

assert

17f69cf

collinjackson self-requested a review August 23, 2019 20:19

collinjackson reviewed Aug 23, 2019

View reviewed changes

Chris Yang and others added 10 commits August 26, 2019 13:32

Update packages/connectivity/README.md

69ab075

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/README.md

50a0ffb

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/README.md

169c5e6

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/README.md

782c472

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/lib/connectivity.dart

0634d62

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/lib/connectivity.dart

c7666d6

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/lib/connectivity.dart

3a68b15

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/lib/connectivity.dart

ef2dcfb

Co-Authored-By: Collin Jackson <jackson@google.com>

Update packages/connectivity/README.md

c0d9783

Co-Authored-By: Collin Jackson <jackson@google.com>

review fixes

26b2874

remove unnecesary result property

666821b

collinjackson approved these changes Aug 28, 2019

View reviewed changes

review fixes and add driver test for

e577181

collinjackson self-requested a review August 28, 2019 22:47

collinjackson approved these changes Aug 28, 2019

View reviewed changes

Chris Yang added 2 commits August 29, 2019 13:49

fix tests

12efe99

Merge branch 'master' into connectivity_ios13

f908024

cyanglaz merged commit 90aaca8 into flutter:master Aug 29, 2019

cyanglaz deleted the connectivity_ios13 branch August 29, 2019 21:24

cyanglaz mentioned this pull request Aug 29, 2019

[connectivity] iOS 13, add location authorization request to enable getWifiName and getWifiBSSID to return valid values flutter/flutter#37804

Closed

mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019

[Connectivity] add a method to request location on iOS (for iOS 13) (f…

1a05843

…lutter#1999)

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019

[Connectivity] add a method to request location on iOS (for iOS 13) (f…

5f08a02

…lutter#1999)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connectivity] add a method to request location on iOS (for iOS 13) #1999

[Connectivity] add a method to request location on iOS (for iOS 13) #1999

cyanglaz commented Aug 21, 2019 •

edited

Loading

collinjackson left a comment

collinjackson Aug 23, 2019

collinjackson Aug 23, 2019

collinjackson Aug 23, 2019

collinjackson Aug 23, 2019

cyanglaz Aug 26, 2019

collinjackson Aug 28, 2019

collinjackson Aug 23, 2019 •

edited

Loading

collinjackson Aug 23, 2019

cyanglaz Aug 27, 2019

cyanglaz commented Aug 24, 2019

cyanglaz commented Aug 27, 2019

collinjackson left a comment

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

collinjackson Aug 28, 2019

cyanglaz commented Aug 28, 2019

collinjackson left a comment


		You can follow issue [#37804](https://github.com/flutter/flutter/issues/37804) for the changes required to return valid SSID and BSSID values with iOS 13.
		* `NSLocationAlwaysAndWhenInUseUsageDescription` - describe why the app needs access to the user’s location information all the time (foreground and background). _Privacy - Location Always and When In Use Usage Description_ in the visual editor.

	// If a request is still in process, immdediately return.
	// If a request is still in process, immediately return.

	/// if the location information needs to be accessible all the time, set `requestAlwaysLocationUsage` to true. If user has
	/// If the location information needs to be accessible all the time, set `requestAlwaysLocationUsage` to `true`. If user has

	//Just `assert(Platform.isIOS)` will disable us to do dart side unit testing.
	// Just `assert(Platform.isIOS)` will prevent us from doing Dart side unit testing.

	/// Request to authorize the location service. Only on iOS.
	/// Request to authorize the location service (only on iOS).

	/// Get the current location service authorization. Only on iOS.
	/// Get the current location service authorization (only on iOS).

[Connectivity] add a method to request location on iOS (for iOS 13) #1999

[Connectivity] add a method to request location on iOS (for iOS 13) #1999

Conversation

cyanglaz commented Aug 21, 2019 • edited Loading

Description

Related Issues

Checklist

Breaking Change

collinjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collinjackson Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Aug 24, 2019

cyanglaz commented Aug 27, 2019

collinjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Aug 28, 2019

collinjackson left a comment

Choose a reason for hiding this comment

cyanglaz commented Aug 21, 2019 •

edited

Loading

collinjackson Aug 23, 2019 •

edited

Loading