GH-7: Geolocation API's #132

Redth · 2018-03-29T14:08:20Z

Description of Change

Adds Geolocation API's

API Changes

public static Task<Location> GetLastKnownLocationAsync();
public static Task<Location> GetLocationAsync();
public static Task<Location> GetLocationAsync(GeolocationRequest request);
public static Task<Location> GetLocationAsync(GeolocationRequest request, CancellationToken cancelToken);

public enum GeolocationAccuracy
{
    // iOS:     ThreeKilometers         (3000m)
    // Android: ACCURACY_LOW, POWER_LOW (500m)
    // UWP:     3000                    (1000-5000m)
    Lowest,

    // iOS:     Kilometer               (1000m)
    // Android: ACCURACY_LOW, POWER_MED (500m)
    // UWP:     1000                    (300-3000m)
    Low,

    // iOS:     HundredMeters           (100m)
    // Android: ACCURACY_MED, POWER_MED (100-500m)
    // UWP:     100                     (30-500m)
    Medium,

    // iOS:     NearestTenMeters        (10m)
    // Android: ACCURACY_HI, POWER_MED  (0-100m)
    // UWP:     High                    (<=10m)
    High,

    // iOS:     Best                    (0m)
    // Android: ACCURACY_HI, POWER_HI   (0-100m)
    // UWP:     High                    (<=10m)
    Best
}

public class GeolocationRequest
{
    public GeolocationRequest();
    public GeolocationRequest(GeolocationAccuracy accuracy);
    public GeolocationRequest(GeolocationAccuracy accuracy, TimeSpan timeout);
    
    public TimeSpan? Timeout { get; set; }
    public GeolocationAccuracy DesiredAccuracy { get; set; }
}

PR Checklist

Has tests (if omitted, state reason in description)
Has samples (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Updated documentation (see walkthrough)

… mor than a few seconds

mattleibow · 2018-04-05T17:58:37Z

Caboodle/Permissions/Permissions.ios.cs

@@ -9,6 +9,10 @@ namespace Microsoft.Caboodle
 {
    internal static partial class Permissions
    {
+        static readonly object locationLocker = new object();
+        static CLLocationManager locationManager;
+        static TaskCompletionSource<PermissionStatus> locationTcs;


This needs a set of eyes here 👀 - and on all the changes to make the permissions not just disappear in a second.

I am locking and making sure that there is only a single location manager for all permissions, but is this necessary?

I also saw a constructor for TaskCompletionSource that takes object state... I have never used that, but maybe there is some way to use that to keep the listener alive... will investigate, but comments will help.

mattleibow · 2018-04-05T17:59:31Z

Caboodle/Types/Shared/Utils.shared.cs

@@ -31,5 +32,18 @@ internal static string Md5Hash(string input)
            return hash.ToString();
 #endif
        }
+
+        internal static CancellationToken TimeoutToken(CancellationToken cancellationToken, TimeSpan timeout)


A util function that will auto-cancel a token after a timeout

mattleibow · 2018-04-05T18:01:20Z

Caboodle/Geolocation/Geolocation.ios.cs

+        {
+            await Permissions.RequireAsync(PermissionType.LocationWhenInUse).ConfigureAwait(false);
+
+            var manager = new CLLocationManager();


This location manager is different to the one used in permissions. Need to review if this gets GC'ed unexpectedly.

mattleibow · 2018-04-05T18:14:23Z

Caboodle/Geolocation/Geolocation.ios.cs

+
+            var manager = new CLLocationManager();
+
+            var tcs = new TaskCompletionSource<CLLocation>(manager);


Keep a reference to the manager for the duration of the task... this needs 👀 and 💭

mattleibow · 2018-04-05T18:14:54Z

Caboodle/Permissions/Permissions.ios.cs

-            var locationManager = new CLLocationManager();
+            var manager = new CLLocationManager();
+
+            var tcs = new TaskCompletionSource<PermissionStatus>(manager);


Keep a reference to the manager for the duration of the task... this needs 👀 and 💭

mattleibow · 2018-04-05T20:08:23Z

With regards to a fail-fast case, this is a bit harder to implement. To be realistic, almost all devices nowadays have a GPS or some basic location system. In most cases there are 2 alternatives: no permission and disabled.

UWP is fun with a more async model. The locator has to init and then updates a status property during that process. Also, when the GPS is off, the RequestAccessAsync returns Denied. In addition, we have to first get permission and then attach to an event to watch for the status to become favourable or not. In fact, if you request location (with GetGeopositionAsync), and then disable the GPS, an UnauthorizedAccessException is thrown.

iOS appears to always have a GPS, and they have a property for detecting whether it enabled or not.

Android appears to be like UWP in that you have to get permission in order to see what GPS/location devices are available. If there are no device available, then all have been disabled and thus no location data. If there are no devices, it could mean that all are disabled, or that there just simply are none.

mattleibow · 2018-04-05T20:24:54Z

Just looking at the implementations, we have to handle the cases of the location devices being disabled or permissions revoked during a request.

Redth · 2018-04-05T20:34:40Z

Honestly this is more reason to consider play services location since it helps deal with getting the user to enable location. Not to mention it's a much better API to deal with. I'm not excited about the dependency it would add and we would still need to fall back on the old api but something to consider!

jamesmontemagno · 2018-04-05T21:27:18Z

I have a version 5 of the geolocator plugin, it does help a bit on Android that is sure, but yeah some tricky parts with google play as a dependency :(

What I did on Android was detect if it had any providers available to determine if it was active or not. The gotcha is that you need to have permissions set to query providers :)

iOS is easy peasy:

 public bool IsGeolocationEnabled
        {
            get
            {         
                var status = CLLocationManager.Status;
                return CLLocationManager.LocationServicesEnabled;
            }
        }

UWP is a bundle of terrible to be honest with you see here: jamesmontemagno/GeolocatorPlugin#144

jamesmontemagno · 2018-04-05T21:29:12Z

I am wondering if it is best just to assume always on... make the request and throw the exception if it fails... we are exception first.. the question is does that work properly.

On iOS/Android we can check permissions first... UWP is a pain in the butt a bit.

jamesmontemagno · 2018-04-05T21:30:01Z

Caboodle/Geolocation/Geolocation.ios.cs

+
+        static async Task<Location> PlatformLastKnownLocationAsync()
+        {
+            await Permissions.RequireAsync(PermissionType.LocationWhenInUse).ConfigureAwait(false);


Can we remove the configureawait for now throughout. I am still worried about that stuff.

jamesmontemagno · 2018-04-05T21:30:19Z

Caboodle/Geolocation/Geolocation.ios.cs

+            var manager = new CLLocationManager();
+            var location = manager.Location;
+
+            return new Location


Can you make this an extension method to convert like i do for geocoding. It may already be there actually.

jamesmontemagno · 2018-04-05T21:31:12Z

Caboodle/Geolocation/Geolocation.ios.cs

+        {
+            try
+            {
+                return new DateTimeOffset((DateTime)timestamp);


We should put these in platform helpers

jamesmontemagno · 2018-04-05T21:32:01Z

Caboodle/Geolocation/Geolocation.uwp.cs

+            if (location?.Coordinate == null)
+                return null;
+
+            return new Location


Same here for extension methods

jamesmontemagno · 2018-04-05T21:33:24Z

Caboodle/Platform/Platform.android.cs

@@ -84,6 +85,9 @@ public static void BeginInvokeOnMainThread(Action action)

        internal static ClipboardManager ClipboardManager =>
            Application.Context.GetSystemService(Context.ClipboardService) as ClipboardManager;
+
+        internal static LocationManager LocationManager =>
+            (LocationManager)Application.Context.GetSystemService(Context.LocationService);


can we use "as" for null checks along the way.

jamesmontemagno · 2018-04-05T21:33:56Z

Samples/Caboodle.Samples.iOS/Info.plist

@@ -53,5 +53,7 @@
 	<string>Caboodle.Samples</string>
 	<key>CFBundleShortVersionString</key>
 	<string>1.0</string>
+	<key>NSLocationWhenInUseUsageDescription</key>
+    <string>Access to your location is required for cool things to happen!</string>


tab this puppy over

Redth · 2018-04-05T21:34:42Z

Yeah I say we just check for hardware and not try and query enabled status here. I think it's not expected to have a UI asking for permission when checking if it's supported. Also we don't really expose the is supported yet anyway so there really is no code path that wouldn't check for permission anyway.

Tl;dr let's check for hardware only.

mattleibow · 2018-04-09T14:20:17Z

There is something I noticed on iOS is that the test will fail because the location manager has some arb requirements about where locations are started from:

https://developer.apple.com/documentation/corelocation/cllocationmanagerdelegate
The methods of your delegate object are called from the thread in which you started the corresponding location services. That thread must itself have an active run loop, like the one found in your application’s main thread.

If the location is requested from a background thread, then the task will wait indefinitely since there is no feedback whatsoever.

I have updated the tests to call the location manager from the UI thread and I throw an exception in the request code, but this requirement may/may not be acceptable for us. There are some steps that we can take:

just keep the exception for iOS only
a bit nasty, but super easy
automatically jump to the main/UI thread
this will cause confusion since the task was called from a background thread, but now we are on a new thread
call from the main thread, and then marshal back to the original thread
this will make the threading appear normal, but there may be consequences later on

…riginal thread

mattleibow · 2018-04-09T14:41:03Z

I went with option 3.

mattleibow · 2018-04-09T14:42:37Z

Xamarin.Essentials/Geolocation/Geolocation.ios.cs

+            // so just use the main loop
+            Platform.BeginInvokeOnMainThread(() =>
+            {
+                var manager = new CLLocationManager();


We create and start the location manger on the main thread:

because we need an active run loop

it is non-blocking

it does not affect the actual call threading system

If we are on the main thread already, this basically is just a straight execution as if nothing special was happening.

mattleibow · 2018-04-09T14:43:31Z

Xamarin.Essentials/Geolocation/Geolocation.ios.cs

+            });
+
+            // still wait and return on the background thread
+            var clLocation = await tcs.Task;


We await the location manager:

we still wait the current thread until the main thread gets back to us

we continue on the background thread

mattleibow · 2018-04-09T14:48:16Z

build

jamesmontemagno · 2018-04-09T18:57:09Z

DeviceTests/DeviceTests.Android/Properties/AndroidManifest.xml

+	<uses-permission android:name="android.permission.CAMERA" />
+	<uses-permission android:name="android.permission.FLASHLIGHT" />
+	<uses-permission android:name="android.permission.INTERNET" />
+	<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />


Do we need Read/Write for anything?

jamesmontemagno · 2018-04-09T18:59:08Z

Xamarin.Essentials/Geolocation/Geolocation.android.cs

+            {
+                try
+                {
+                    locationManager.RemoveUpdates(listener);


Should we really ignore this?

jamesmontemagno · 2018-04-09T18:59:42Z

Xamarin.Essentials/Geolocation/Geolocation.android.cs

+                    break;
+            }
+
+            return locationManager.GetBestProvider(criteria, true) ?? locationManager.GetProviders(true).First();


Should this be FirstorDefault?

jamesmontemagno · 2018-04-09T19:00:13Z

Xamarin.Essentials/Geolocation/Geolocation.ios.cs

+        {
+            await Permissions.RequireAsync(PermissionType.LocationWhenInUse);
+
+            var manager = new CLLocationManager();


Should we have this on the main thread or doesn't matter??

It does not appear to matter.

jamesmontemagno · 2018-04-09T19:01:16Z

Xamarin.Essentials/Geolocation/Geolocation.ios.cs

+
+                wasRaised = true;
+
+                var location = locations.LastOrDefault();


Are we assuming these are ordered by priority? Accuracy?

According to the docs:

The objects in the array are organized in the order in which they occurred. Therefore, the most recent location update is at the end of the array.
https://developer.apple.com/documentation/corelocation/cllocationmanagerdelegate/1423615-locationmanager

…location

Made changes

First pass at Android Geolocation impl

bf34c12

Redth changed the title ~~Geolocation API's~~ [WIP] Geolocation API's Mar 29, 2018

Redth added the DO-NOT-MERGE Don't merge it.... don't do it! label Mar 29, 2018

Redth self-assigned this Mar 29, 2018

Redth added this to the Preview-1 milestone Mar 29, 2018

mattleibow changed the title ~~[WIP] Geolocation API's~~ GH-7: Geolocation API's Mar 29, 2018

mattleibow added 8 commits March 29, 2018 18:52

No need for nullables

9f65284

Added the geolocation sample

135500c

Added the UWP geolocation implementation

a55fd48

Getting some of the iOS in

bfdc140

Use a global location manager so that the permissions popup lives for…

ed4014d

… mor than a few seconds

Check before asking on iOS

398d802

Merge branch 'master' into geolocation

39450a1

Finished the iOS implementation and tweak a few things

23331de

mattleibow requested a review from jamesmontemagno April 5, 2018 17:53

mattleibow self-assigned this Apr 5, 2018

Remove the unsused using

760c1b9

mattleibow reviewed Apr 5, 2018

View reviewed changes

mattleibow added 2 commits April 5, 2018 20:10

Don't use locking and static fields, use the TCS state

b766f4f

Keep the manager alive for the duration of the method

351383f

mattleibow reviewed Apr 5, 2018

View reviewed changes

Use platform specific accuracy

8a6c740

jamesmontemagno suggested changes Apr 5, 2018

View reviewed changes

Merge branch 'master' into geolocation

ad22bf6

mattleibow added 5 commits April 6, 2018 23:08

Merge branch 'master' into geolocation

95e8bad

Merge branch 'master' into geolocation

bfe601b

Merge branch 'master' into geolocation

ee54c8a

Enabling the tests for CI after adding attributes

ddb16ea

Added the iOS permissions text to the tests

984fe2d

mattleibow added 2 commits April 9, 2018 16:25

iOS has opinions when it comes to locations

bfc0c26

Starting the location manager on the main thread, but return on the o…

596aff7

…riginal thread

mattleibow reviewed Apr 9, 2018

View reviewed changes

mattleibow added 4 commits April 9, 2018 17:30

We just need to call the constructor in the main thread

4a3d901

Added all the permissions to the manifest for the samples and the tests

961d635

Android has looper issues

89a2fce

Location is now a declared permission

24f24f7

jamesmontemagno previously requested changes Apr 9, 2018

View reviewed changes

mattleibow and others added 7 commits April 9, 2018 21:21

Changes based on feedback.

c77ac43

Merge branch 'master' into geolocation

f2675be

Merge branch 'master' into geolocation

edcd6d8

Merge branch 'master' into geolocation

3b72e0d

Merge branch 'master' into geolocation

d68c156

Cleanup iOS Permissions. Must have static location manager around.

03e260c

Merge branch 'geolocation' of https://github.com/xamarin/f50 into geo…

edaa86f

…location

The stylish copper got us again

d1f9615

jamesmontemagno approved these changes Apr 10, 2018

View reviewed changes

jamesmontemagno merged commit f185ef4 into master Apr 10, 2018

mattleibow deleted the geolocation branch April 10, 2018 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-7: Geolocation API's #132

GH-7: Geolocation API's #132

Redth commented Mar 29, 2018 •

edited by mattleibow

Loading

mattleibow Apr 5, 2018 •

edited

Loading

mattleibow Apr 5, 2018

mattleibow Apr 5, 2018

mattleibow Apr 5, 2018

mattleibow Apr 5, 2018

mattleibow Apr 5, 2018 •

edited

Loading

mattleibow Apr 5, 2018 •

edited

Loading

mattleibow commented Apr 5, 2018 •

edited

Loading

mattleibow commented Apr 5, 2018

Redth commented Apr 5, 2018

jamesmontemagno commented Apr 5, 2018

jamesmontemagno commented Apr 5, 2018

jamesmontemagno Apr 5, 2018

jamesmontemagno Apr 5, 2018

jamesmontemagno Apr 5, 2018

jamesmontemagno Apr 5, 2018

jamesmontemagno Apr 5, 2018

jamesmontemagno Apr 5, 2018

Redth commented Apr 5, 2018

mattleibow commented Apr 9, 2018 •

edited

Loading

mattleibow commented Apr 9, 2018

mattleibow Apr 9, 2018 •

edited

Loading

mattleibow Apr 9, 2018 •

edited

Loading

mattleibow commented Apr 9, 2018

jamesmontemagno Apr 9, 2018

jamesmontemagno Apr 9, 2018

jamesmontemagno Apr 9, 2018

jamesmontemagno Apr 9, 2018

mattleibow Apr 9, 2018

jamesmontemagno Apr 9, 2018

mattleibow Apr 9, 2018


		var manager = new CLLocationManager();

		var tcs = new TaskCompletionSource<CLLocation>(manager);

GH-7: Geolocation API's #132

GH-7: Geolocation API's #132

Conversation

Redth commented Mar 29, 2018 • edited by mattleibow Loading

Description of Change

API Changes

PR Checklist

mattleibow Apr 5, 2018 • edited Loading

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

mattleibow Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

mattleibow Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

mattleibow commented Apr 5, 2018 • edited Loading

mattleibow commented Apr 5, 2018

Redth commented Apr 5, 2018

jamesmontemagno commented Apr 5, 2018

jamesmontemagno commented Apr 5, 2018

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

Redth commented Apr 5, 2018

mattleibow commented Apr 9, 2018 • edited Loading

mattleibow commented Apr 9, 2018

mattleibow Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

mattleibow Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

mattleibow commented Apr 9, 2018

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

Redth commented Mar 29, 2018 •

edited by mattleibow

Loading

mattleibow Apr 5, 2018 •

edited

Loading

mattleibow Apr 5, 2018 •

edited

Loading

mattleibow Apr 5, 2018 •

edited

Loading

mattleibow commented Apr 5, 2018 •

edited

Loading

mattleibow commented Apr 9, 2018 •

edited

Loading

mattleibow Apr 9, 2018 •

edited

Loading

mattleibow Apr 9, 2018 •

edited

Loading