[ios] Mapbox's Location Manager new API. by fabian-guerra · Pull Request #12013 · mapbox/mapbox-gl-native

fabian-guerra · 2018-05-25T19:24:00Z

This PR fixes #11983 and adds a new API to implement a custom location manager.

This API follows Apple's rule that is possible to have only one location manager at a given time, but opens the option to have different technologies that provides location updates such as WIFI or iBeacons.

Create MGLLocationManager and MGLLocationManagerDelegate.
Change MGLMapView to use the new API.
Add tests.
- Memory usage.
- Course following.
- Single LM multiple usages.
Document how to use the API and make an example.

simonseyer · 2018-06-11T14:28:17Z

+
+@optional
+
+@end


I would suggest removing the usage of all CoreLocation types (including CLAuthorizationStatus , etc. — so that you can remove the CoreLocation import above). This has the following reasons:

Some of them are difficult to initialize. CLHeading has no public initializer so that I was forced to create a subclass where I was overriding the trueHeading property. This was hacky and not obvious.

You can only reuqest the values you really need. Instead of CLHeading cou can ask for the trueHeading:(double):trueHeading. This makes the API easier to integrate and simulate.

You are getting completely independent of the CoreLocation framework

Hi, @simonseyer. Thank you for your feedback, and agree with you. When I was designing the API I consider removing completely the dependency on CoreLocation (provide our version of CLLocation, CLLocationCoordinate2D,CLHeading, etc). Although making that change will require a new semver, and migrating code.

I will follow up with a ticket related to remove CoreLocation dependency.

julianrex · 2018-06-26T18:18:46Z

+@required
+
+/**
+ The delegate to recive updates.


Typo: recive -> receive

julianrex · 2018-06-26T18:22:46Z

+
+- (void)setHeadingOrientation:(CLDeviceOrientation)headingOrientation
+{
+    _locationManager.headingOrientation = headingOrientation;


Why not self.locationManager... here?

julianrex · 2018-06-26T18:33:15Z

-        self.locationManager = [[CLLocationManager alloc] init];
+        // If no custom location manager is provided will use the internal implementation.
+        if (!self.locationManager) {
+            self.locationManager = [[MGLCLLocationManager alloc] init];


This will call -validateLocationServices again, won't it?

It won't. https://github.com/mapbox/mapbox-gl-native/pull/12013/files#diff-20502b6bc02986a4398d0da84b2d4b94R4673

Perfect :) Looks like I was commenting on an earlier version.

julianrex · 2018-06-26T18:47:20Z

Just picking up from @simonseyer's comment (#12013 (comment)) about removing CoreLocation references (I'm assuming here that you mean only removing CoreLocation in terms of the location manager, not from the wider SDK).

This should be our ideal goal, so

what code would need to change that requires a major version change?
if we can't go straight to a CoreLocation-less option, what are our options to help get us there?
for example, should we consider typedefs or subclassing?

In addition, I wonder if it's worth discussing if and how we could improve the user-authorization dance that developers have to go through.

fabian-guerra · 2018-06-26T19:55:01Z

about removing CoreLocation references (I'm assuming here that you mean only removing CoreLocation in terms of the location manager, not from the wider SDK).

I meant the wider SDK.

what code would need to change that requires a major version change?

If we replace CLXXXX with our own versions — MGLLocation, MGLHeading, MGLLocationCoordinate — will break the current apps and will require a simple(?) migration.

if we can't go straight to a CoreLocation-less option, what are our options to help get us there?

We may not want to go full without CL but we can replace some classes/structs/enums that makes sense in the line of allowing customization — MGLHeading? MGLAuthorizationStatus? etc —

for example, should we consider typedefs or subclassing?

As I outlined above. I am thinking redefinition.

In addition, I wonder if it's worth discussing if and how we could improve the user-authorization dance that developers have to go through.

We can make a ticket to follow along the discussion just as I am waiting to release this to followup with a ticket for going full and replace CL.

julianrex · 2018-06-26T21:32:09Z

We may not want to go full without CL but we can replace some classes/structs/enums that makes sense in the line of allowing customization — MGLHeading? MGLAuthorizationStatus? etc —

As I outlined above. I am thinking redefinition.

Sounds good. Protocols for the classes (e.g CLLocation), re-def for the structs (oh where are you Swift), and some lightweight "conversions".

If we drop the CL dependency for this location manager protocol - is it worth considering tweaking the interface?

fabian-guerra · 2018-06-28T22:31:36Z

I performed an allocations and time profiler test iterating over the options in

mapbox-gl-native/platform/ios/src/MGLMapView.h

Line 52 in acbcf7e

typedef NS_ENUM(NSUInteger, MGLUserTrackingMode) {

I used an iPhone X iOS 11.4.

Allocations.

CLLocationManager (used master as benchmark)

MGLCLLocationManager (Internal API).

The internal implementation added 32 bytes to the memory growth.

Time Profiler.

CLLocationManager (used master as benchmark)

MGLCLLocationManager (Internal API).

It was almost the same time of execution. I performed more tests with 1-2 s deltas. I think is due to the inconsistent location updates I get at our San Francisco office.

1ec5

This PR takes a first step towards making the map view’s built-in location-related features more customizable, which is a laudable goal.

However, I’m uneasy with the idea of eliminating our dependency on Core Location. While doing so could be a boon for a relative few developers with specialized needs, it could potentially complicate most developers’ ability to use the SDK. That would be akin to Apple’s new Natural Language framework shipping with a custom NLString type that you have to use instead of String.

Even with just this more limited refactoring of CLLocationManager usage, I think we should give a lot of thought to the tradeoffs involved and whether the use cases we’d be unblocking would be worth the trouble we’d cause the general user base. Consider the impact these changes will have on someone migrating from MapKit or the Google Maps SDK.

What do these changes mean for MGLMapViewDelegate’s location-related methods? Do we need to document any changes there?

What do these changes mean for telemetry? Would it be problematic if the map view uses one location manager while the telemetry subsystem uses another? On the flip side, would it be problematic to allow the developer to override the location manager used for telemetry?

Remember to update the iOS changelog to note the addition of MGLMapView.locationManager.

1ec5 · 2018-06-29T22:01:36Z

+
+@interface MGLCLLocationManager()<CLLocationManagerDelegate>
+
+@property (nonatomic) CLLocationManager *locationManager;


Why create a default location manager that owns a CLLocationManager? Why not extend CLLocationManager to conform to MGLLocationManager?

Te reason for MGLCLLocationManager to implement MGLLocationManager is that MGLMapView uses the latter for handling authorization/start/stop events and breaks the dependency on CLLocationManager.

Suppose we instead declare a category on CLLocationManager that conforms to MGLLocationManager. Wouldn’t that accomplish the same thing as this class, except with less indirection?

You're right. I changed the implementation accordingly.

1ec5 · 2018-06-29T22:03:57Z

 @interface MGLMapView () <UIGestureRecognizerDelegate,
                          GLKViewDelegate,
-                          CLLocationManagerDelegate,
+                          MGLLocationManagerDelegate,


Note that the navigation SDK currently depends on MGLMapView conforming to CLLocationManagerDelegate. I’m unsure whether the navigation SDK still takes advantage of that assumption following mapbox/mapbox-navigation-ios#402.

/cc @bsudekum @frederoni

The navigation SDK’s redeclaration of this conformance was vestigial: mapbox/mapbox-navigation-ios#1549.

1ec5 · 2018-06-29T22:06:13Z

+ events.
+
+ If no custom manager is provided or setting this property to `nil` will default
+ to the internal `CLLocationManager` based implementation.


If the default use of CLLocationManager is an internal implementation detail, then we shouldn’t mention it.

How can we communicate this behavior? We should let devs know that this is a specialized API and if nothing is provided then the default option is used.

We can certainly say that a typical implementation comes with MGLMapView by default, but if the goal is to reduce dependency on CLLocationManager, then we need to couple any mention of CLLocationManager in the documentation with a note that the default location manager may change in the future.

1ec5 · 2018-06-29T22:08:06Z

+- (void)setLocationManager:(id<MGLLocationManager>)locationManager
+{
+    _locationManager = locationManager;
+    _locationManager.delegate = self;


The documentation comment for this property states that setting the property to nil causes the map view to use the CLLocationManager implementation. Does that happen immediately, or upon the next location update?

When the location manager is set to nil is going to use the default option the next time the showsUserLocation is set to yes.

So MGLMapView would continue to serve as the old location manager’s delegate until then, without stopping that location manager? That could result in MGLMapView and MGLMapViewDelegate responding to doubled-up or contradictory location updates.

1ec5 · 2018-06-29T22:09:08Z

+
+ Set the custom location manager before calling `showUserLocation`.
+ */
+@property (nonatomic, nullable) id<MGLLocationManager> locationManager;


If the developer sets this property to nil or leaves it as nil, should the getter return nil or should it return the default implementation? If the latter, then the property should be null_resettable instead of nullable.

Most of the time we’ve received requests to expose the location manager, it wasn’t because the developer wanted to replace the location manager with their own implementation. Rather, it was because they wanted to tweak the built-in location manager’s settings. For that to be possible, this property would always need to return a non-nil value.

fabian-guerra · 2018-06-29T23:39:27Z

@1ec5

However, I’m uneasy with the idea of eliminating our dependency on Core Location. While doing so could be a boon for a relative few developers with specialized needs, it could potentially complicate most developers’ ability to use the SDK. That would be akin to Apple’s new Natural Language framework shipping with a custom NLString type that you have to use instead of String.

I understand your worries. Reducing our direct dependency on Core Location may bring new problems. In one of my previous comments I stated that we may not replace entirely our dependency but that is up to a conscious analysis. I also don't believe that radical changes are good for devs but I do believe on evolutionary changes. Enabling greater customization will require some evolutionary changes. It's familiar for devs to remove prefixes from Classes and what would be the worst scenario besides renaming CLLocation to MGLLocation? — please help me understand —

Even with just this more limited refactoring of CLLocationManager usage, I think we should give a lot of thought to the tradeoffs involved and whether the use cases we’d be unblocking would be worth the trouble we’d cause the general user base. Consider the impact these changes will have on someone migrating from MapKit or the Google Maps SDK.

What kind of problems do you think it may arise? MGLLocationManager and MGLLocationManagerDelegate are thought for customers with different location providers needs. We do provide a default implementation for the ones that doesn't care about location.

What do these changes mean for MGLMapViewDelegate’s location-related methods? Do we need to document any changes there?

This does not affect MGLMapViewDelegate beyond the calling stack.

What do these changes mean for telemetry? Would it be problematic if the map view uses one location manager while the telemetry subsystem uses another? On the flip side, would it be problematic to allow the developer to override the location manager used for telemetry?

I don't have a clear answer for this. The telemetry parts that are used inside MGLMapView remind unaffected but I will sync with @rclee and @bsudekum to determine the impact of this change.

1ec5 · 2018-06-30T01:00:12Z

It's familiar for devs to remove prefixes from Classes and what would be the worst scenario besides renaming CLLocation to MGLLocation? — please help me understand —

That change alone is benign, given that we don’t currently use CLLocation anywhere in the public API. I was responding to the suggestion that we remove the dependency on Core Location, which would require replacing important types like CLLocationCoordinate2D and CLLocationDirection with types specific to this SDK. Unlike on Android, our various iOS libraries don’t share a common dependency that vends core types the way the Mapbox Java SDK does: mapbox/mapbox-events-ios#1.

What kind of problems do you think it may arise? MGLLocationManager and MGLLocationManagerDelegate are thought for customers with different location providers needs. We do provide a default implementation for the ones that doesn't care about location.

My understanding is that the set of developers who’d need to use this SDK with a different location manager is a relatively small subset. They may be an important subset, but perhaps that’s a discussion to have elsewhere. Most developers would at most need to configure a few CLLocation parameters rather than creating their own. I think making the locationManager property null-resettable could address that need without unnecessary complexity.

…ng updates after set to nil.

…tion.

* [ios] Cherry-pick 12013 * [ios] Updated Changelog.md * [ios] Fix MBXCustomLocationViewController iOS 8 compatibility.

fabian-guerra added the iOS Mapbox Maps SDK for iOS label May 25, 2018

fabian-guerra self-assigned this May 25, 2018

fabian-guerra mentioned this pull request Jun 1, 2018

Map Location Update with External Coordinates Source #12037

Closed

fabian-guerra force-pushed the fabian-location-manager-11983 branch from 4529f05 to 4891635 Compare June 1, 2018 20:34

akitchen mentioned this pull request Jun 7, 2018

Global app state manipulation mapbox/mapbox-navigation-ios#1474

Closed

simonseyer suggested changes Jun 11, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-location-manager-11983 branch from 4891635 to 91bc574 Compare June 13, 2018 15:28

fabian-guerra changed the base branch from master to release-chai June 13, 2018 15:29

fabian-guerra requested review from 1ec5, friedbunny and julianrex June 25, 2018 19:41

julianrex reviewed Jun 26, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-location-manager-11983 branch from d03fa07 to e4465d1 Compare June 28, 2018 22:51

fabian-guerra changed the base branch from release-chai to master June 28, 2018 22:52

fabian-guerra force-pushed the fabian-location-manager-11983 branch from e4465d1 to 5af4080 Compare June 28, 2018 23:07

1ec5 reviewed Jun 29, 2018

View reviewed changes

fabian-guerra requested review from bsudekum and rclee July 2, 2018 17:01

1ec5 mentioned this pull request Jul 2, 2018

Allow plugins to transform location updates #12280

Closed

fabian-guerra force-pushed the fabian-location-manager-11983 branch from 5af4080 to cc4a2b2 Compare July 3, 2018 22:07

fabian-guerra force-pushed the fabian-location-manager-11983 branch 2 times, most recently from 6221531 to 38306ba Compare July 11, 2018 22:31

fabian-guerra added feature needs changelog Indicates PR needs a changelog entry prior to merging. labels Jul 11, 2018

fabian-guerra added 16 commits July 20, 2018 12:58

[ios] Clarify location manager usage.

49078d9

[ios] Add a custom location manager example.

7d826ec

[ios] Fix location manager doc typo.

428a99a

[ios] Fix internal location manager invocation.

5ff6d16

[ios] Fix resetting the location manager blocks updates.

acf3eaf

[ios] Change locationManager to null resettable.

ba2a196

[ios] Clarify map view's locationManager property usage.

65a788d

[ios] Fix an issue where a custom location manager may continue getti…

3bec307

…ng updates after set to nil.

[ios] Change the location manager internal implementation to a category.

efad42d

[ios] Clarify location manager's API usage.

fbd8066

[ios] Rever the default location manager to the wrap based implementa…

f1445cb

…tion.

[ios] Clarify location manager API use.

6ffc828

[ios] Add the location API to jazzy.

2d60067

[ios] Update changelog.

a6cd73c

[ios] Fix documentation format.

741dd3c

[ios] Add location manager's error handler.

f01e324

fabian-guerra force-pushed the fabian-location-manager-11983 branch from 09aaa6b to f01e324 Compare July 20, 2018 20:00

[ios] Update location manager API documentation order.

08b4727

friedbunny removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Jul 20, 2018

friedbunny approved these changes Jul 20, 2018

View reviewed changes

fabian-guerra merged commit 681e014 into master Jul 20, 2018

fabian-guerra deleted the fabian-location-manager-11983 branch July 20, 2018 21:14

fabian-guerra mentioned this pull request Jul 21, 2018

[ios] Analyze map's SDK Apple's Core Location dependency. #12454

Open

friedbunny added this to the ios-v4.3.0 milestone Jul 26, 2018

friedbunny mentioned this pull request Jul 26, 2018

[ios, macos] Custom Location Manager #11893

Closed

julianrex mentioned this pull request Aug 1, 2018

Refactor timer code into separate utility class mapbox/mapbox-events-ios#53

Closed

jmkiley mentioned this pull request Aug 13, 2018

Cherry-pick #12013 into release-boba #12620

Merged

jmkiley added a commit that referenced this pull request Aug 14, 2018

Cherry-pick #12013 into release-boba (#12620)

1dafca1

* [ios] Cherry-pick 12013 * [ios] Updated Changelog.md * [ios] Fix MBXCustomLocationViewController iOS 8 compatibility.

1ec5 mentioned this pull request Sep 14, 2018

MGLMapView location/course tracking should be overrideable #6867

Closed


		@interface MGLCLLocationManager()<CLLocationManagerDelegate>

		@property (nonatomic) CLLocationManager *locationManager;


		@optional

		@end

Conversation

fabian-guerra commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianrex commented Jun 26, 2018

Uh oh!

fabian-guerra commented Jun 26, 2018

Uh oh!

julianrex commented Jun 26, 2018

Uh oh!

fabian-guerra commented Jun 28, 2018

Allocations.

Time Profiler.

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian-guerra commented Jun 29, 2018

Uh oh!

1ec5 commented Jun 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fabian-guerra commented May 25, 2018 •

edited

Loading

1ec5 Jun 29, 2018 •

edited

Loading