[ios] Add delegate method to specify the user location annotation’s position #12907

captainbarbosa · 2018-09-17T22:52:59Z

This PR makes three changes:

Fixes Add delegate method for the user location annotation’s position #12203 / [ios] Added 2 new mid alignment position in MGLAnnotationVerticalAlignment #11342 by providing a new delegate method that enables the developer to a CGPoint where the user location annotation should be anchored, relative to the map view’s origin after applying the its content insets.
Deprecates -[MGLMapView setUserLocationVerticalAlignment:] and the associated MGLAnnotationVerticalAlignment enumeration.
Exposes -[MGLMapView updateUserLocationAnnotationView] and -[MGLMapView updateUserLocationAnnotationView:animated:] to programmatically force the user location annotation to update.

1ec5 · 2018-09-20T00:55:11Z

platform/ios/src/MGLMapView.h

@@ -416,7 +416,7 @@ MGL_EXPORT IB_DESIGNABLE
 transition. If you don’t want to animate the change, use the
 `-setUserLocationVerticalAlignment:animated:` method instead.
 */
-@property (nonatomic, assign) MGLAnnotationVerticalAlignment userLocationVerticalAlignment;
+@property (nonatomic, assign) MGLAnnotationVerticalAlignment userLocationVerticalAlignment __attribute__((deprecated("Use -userLocationAnchorForMapView delegate method instead.")));


Qualify the method name with the protocol name for discoverability.

1ec5 · 2018-09-20T00:57:01Z

platform/ios/src/MGLMapViewDelegate.h

+
+ @param mapView The map view that is tracking the user's location.
+ */
+- (CGPoint)userLocationAnchorForMapView:(MGLMapView *)mapView;


By convention, the first selector part begins with the first parameter’s type. If the return type is a CGPoint, describe it as an “anchor point” rather than an “anchor”, which typically describes an NSLayoutAnchor. Thus: -mapViewUserLocationAnchor:.

1ec5 · 2018-09-20T01:03:05Z

platform/ios/src/MGLMapViewDelegate.h

@@ -308,6 +308,17 @@ NS_ASSUME_NONNULL_BEGIN
 */
 - (void)mapView:(MGLMapView *)mapView didChangeUserTrackingMode:(MGLUserTrackingMode)mode animated:(BOOL)animated;

+/**
+ Returns a CGPoint value to offset the user location annotation, relative to
+ the camera's center coordinate.


A screen point can only be relative to another screen point, not a geographic coordinate. I think the intention is to say that it’s relative to the center of the map view. Note that the navigation SDK’s analogous NavigationMapViewDelegate.navigationMapViewUserAnchorPoint(_:) method is expressed relative to the map view’s origin, not its center. There are pros and cons to either approach, so it’s good that the frame of reference is being called out here.

A screen point can only be relative to another screen point, not a geographic coordinate.

Does this mean the camera's center can also relate to a CGPoint instead of a coordinate?

I think I'm going to base my implementation off of the map view's origin as well, to keep things consistent with the implementation in the Navigation SDK.

1ec5 · 2018-09-20T01:04:39Z

platform/ios/src/MGLMapView.mm

+    {
+        // Get whatever value is returned from the delegate method that responds to
+        // the selector and assign it to the center
+        center = [self.delegate userLocationAnchorForMapView:self];


This line sets center from scratch, effectively making it relative to the origin rather than the view’s center, which is stored in center up to this point.

If we choose to set the anchor point based off the the origin, should we adjust the original center (CGPointMake(CGRectGetMidX(contentFrame), CGRectGetMidY(contentFrame))) to do the same?

No, because the legacy userLocationVerticalAlignment code depends on that initial value, whereas the new -mapViewUserLocationAnchorPoint: code does not.

platform/ios/src/MGLMapView.h

1ec5 · 2018-09-24T05:41:49Z

platform/ios/src/MGLMapView.mm

+
+    if ([self.delegate respondsToSelector:@selector(mapViewUserLocationAnchorPoint:)])
+    {
+        center = [self.delegate mapViewUserLocationAnchorPoint:self];


The delegate should be expressing the anchor point in relation to the content frame, so offset the anchor point by the content insets.

platform/ios/src/MGLMapView.mm

1ec5 · 2018-09-24T05:43:58Z

platform/ios/src/MGLMapView.mm

+    {
+        // Get whatever value is returned from the delegate method that responds to
+        // the selector and assign it to the center
+        center = [self.delegate userLocationAnchorForMapView:self];


No, because the legacy userLocationVerticalAlignment code depends on that initial value, whereas the new -mapViewUserLocationAnchorPoint: code does not.

platform/ios/src/MGLMapView.h

1ec5 · 2018-09-24T23:34:09Z

platform/ios/src/MGLMapViewDelegate.h

@@ -310,7 +310,7 @@ NS_ASSUME_NONNULL_BEGIN

 /**
 Returns a CGPoint value to offset the user location annotation, relative to
- the the map view's origin.
+ the the map view's content insets.


relative to the map view’s origin after applying the map view’s content insets.

1ec5 · 2018-09-24T23:36:31Z

platform/ios/src/MGLMapView.mm

@@ -5997,7 +5997,8 @@ - (CGPoint)userLocationAnnotationViewCenter

    if ([self.delegate respondsToSelector:@selector(mapViewUserLocationAnchorPoint:)])
    {
-        center = [self.delegate mapViewUserLocationAnchorPoint:self];
+        CGPoint anchorPoint = [self.delegate mapViewUserLocationAnchorPoint:self];
+        center = CGPointMake((contentFrame.origin.x + anchorPoint.x), (contentFrame.origin.y + anchorPoint.y));


By this point, contentFrame has already been inset by -edgePaddingForFollowingWithCourse, which might not be quite correct. That method does account for the size of the puck, which is probably a good thing, but it also applies MGLUserLocationAnnotationViewInset.

1ec5 · 2018-09-26T17:31:40Z

An idea for an integration test:

Create a map view with a specific frame.
Set the map view’s content insets.
Configure a custom location manager that immediately feeds the map view a specific coordinate.
Set the map view’s userTrackingMode without animation.
Get the user location annotation view and check its frame.

platform/ios/src/MGLMapView.mm

captainbarbosa · 2018-10-01T20:48:44Z

I think this is ready for a preliminary review 👀

julianrex

Is there a branch where I can check this out in iosapp?

julianrex · 2018-10-01T21:06:17Z

platform/ios/Integration Tests/MGLTestLocationManager.m

+
+@synthesize delegate;
+
+- (instancetype)init


This init method isn't needed here since it doesn't do anything in addition to super.

julianrex · 2018-10-01T21:08:09Z

platform/ios/Integration Tests/MGLTestLocationManager.m

+    return self;
+}
+
+- (CLAuthorizationStatus)authorizationStatus { return [CLLocationManager authorizationStatus]; }


Since this is a test manager, how about returning kCLAuthorizationStatusAuthorized here?

captainbarbosa · 2018-10-02T20:41:30Z

@julianrex Here's a branch you can use to test this out in iosapp: https://github.com/mapbox/mapbox-gl-native/tree/user-location-delegate-demo

captainbarbosa · 2018-10-02T21:28:19Z

Rebased on master & added changelog entry.

platform/ios/src/MGLMapView.h

1ec5 · 2018-10-02T21:29:03Z

platform/ios/src/MGLMapViewDelegate.h

@@ -308,6 +308,17 @@ NS_ASSUME_NONNULL_BEGIN
 */
 - (void)mapView:(MGLMapView *)mapView didChangeUserTrackingMode:(MGLUserTrackingMode)mode animated:(BOOL)animated;

+/**
+ Returns a CGPoint value to offset the user location annotation, relative to the


CGPoint is already present in the declaration, so I’d maybe describe it here in lay terms. It’s an absolute screen coordinate, not an offset (which would be CGVector):

Returns a screen coordinate at which to position the user location annotation…

1ec5 · 2018-10-02T21:30:37Z

platform/ios/src/MGLMapViewDelegate.h

+ map view’s origin after applying the map view’s content insets.
+
+ This method will offset the user location annotation by the specified
+ X and Y values determined by the CGPoint.


What is the default behavior when this method goes unimplemented? How does this method interact with the MGLMapView.userLocationVerticalAlignment property when it is set and this method is also implemented?

1ec5 · 2018-10-02T21:31:51Z

platform/ios/CHANGELOG.md

@@ -11,6 +11,11 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
 * Fixed a crash when a style layer `*-pattern` property evaluates to nil for a particular feature. ([#12896](https://github.com/mapbox/mapbox-gl-native/pull/12896))
 * Fixed an issue with view annotations (including the user location annotation) and the GL map lagging relative to each other. ([#12895](https://github.com/mapbox/mapbox-gl-native/pull/12895))

+### User location
+* Added `-[MGLMapViewDelegate mapViewUserLocationAnchorPoint:]`  to offset the user location annotation with a custom `CGPoint`. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))


…to customize the location of the user location annotation.

1ec5 · 2018-10-02T21:32:35Z

platform/ios/CHANGELOG.md

+### User location
+* Added `-[MGLMapViewDelegate mapViewUserLocationAnchorPoint:]`  to offset the user location annotation with a custom `CGPoint`. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))
+* Marked `-[MGLMapView setUserLocationVerticalAlignment:]` as deprecated. Use `-[MGLMapViewDelegate mapViewUserLocationAnchorPoint:]` instead. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))
+* Added `-[MGLMapView updateUserLocationAnnotationView]` and `-[MGLMapView updateUserLocationAnnotationView:animated:]` to public API to update the position of the user location annotation. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))


As far as the user is concerned, these methods never existed in the first place, so “to public API” is redundant.

Added the -[MGLMapView updateUserLocationAnnotationView] and -[MGLMapView updateUserLocationAnnotationView:animated:] methods to update the position of the user location annotation between location updates.

platform/ios/src/MGLMapView.mm

1ec5

Good to merge once the documentation feedback in #12907 (review) is addressed.

…osition Update method name More API drafting Add deprecation flag Add Swift delegate integration test Update method name and documentation Update deprecation notices Update method name Offset anchor point relative to contentFrame Update docs Only run through switch statement if delegate is unimplemented Account for content inset + refactor logic Adjust edgePaddingForFollowing Fix Swift delegate integration test Set up integration test Set up test location manager . Remove unused file reference from test Return CGPoint value from delegate method within integration test setup Test anchor points Make updateUserLocationAnnotationView public Refactor test Update test location manager Changelog entry Doc fixes

friedbunny · 2018-10-02T23:27:34Z

platform/ios/CHANGELOG.md

+* Added `-[MGLMapViewDelegate mapViewUserLocationAnchorPoint:]` to customize the position of the user location annotation.. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))
+* Marked `-[MGLMapView setUserLocationVerticalAlignment:]` as deprecated. Use `-[MGLMapViewDelegate mapViewUserLocationAnchorPoint:]` instead. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))
+* Added the `-[MGLMapView updateUserLocationAnnotationView]` and `-[MGLMapView updateUserLocationAnnotationView:animated:]` methods to update the position of the user location annotation between location updates. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907))
+* Fixed an issue where the user location annotation was positioned incorrectly when the map view had a left or right content inset. ([#12907](https://github.com/mapbox/mapbox-gl-native/pull/12907)) inset


There’s an extra “inset” at the end of this sentence.

captainbarbosa added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Sep 17, 2018

captainbarbosa added this to the ios-v4.5.0 milestone Sep 17, 2018

captainbarbosa self-assigned this Sep 17, 2018

1ec5 reviewed Sep 20, 2018

View reviewed changes

1ec5 reviewed Sep 24, 2018

View reviewed changes

platform/ios/src/MGLMapView.h Outdated Show resolved Hide resolved

1ec5 reviewed Sep 24, 2018

View reviewed changes

1ec5 added the navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general label Sep 25, 2018

captainbarbosa commented Sep 29, 2018

View reviewed changes

platform/ios/src/MGLMapView.mm Show resolved Hide resolved

captainbarbosa force-pushed the user-location-delegate-method branch 3 times, most recently from eb690a0 to 53b6b47 Compare October 1, 2018 20:47

captainbarbosa requested review from 1ec5 and julianrex October 1, 2018 20:48

julianrex reviewed Oct 1, 2018

View reviewed changes

captainbarbosa force-pushed the user-location-delegate-method branch from d6be976 to 664a3ba Compare October 2, 2018 20:40

captainbarbosa force-pushed the user-location-delegate-method branch from 664a3ba to e267524 Compare October 2, 2018 20:42

captainbarbosa removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Oct 2, 2018

captainbarbosa force-pushed the user-location-delegate-method branch from e267524 to 6dc7d5b Compare October 2, 2018 21:27

1ec5 reviewed Oct 2, 2018

View reviewed changes

1ec5 approved these changes Oct 2, 2018

View reviewed changes

captainbarbosa force-pushed the user-location-delegate-method branch from 6dc7d5b to db1d530 Compare October 2, 2018 22:02

captainbarbosa merged commit 7b24339 into master Oct 2, 2018

captainbarbosa deleted the user-location-delegate-method branch October 2, 2018 22:28

friedbunny reviewed Oct 2, 2018

View reviewed changes

friedbunny mentioned this pull request Oct 3, 2018

[ios] Ignore deprecation warnings for MGLMapView.userLocationVerticalAlignment #13029

Merged

1ec5 mentioned this pull request Jan 12, 2019

Incorrect userLocation annotation position after zoomLevel is changed #10108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Add delegate method to specify the user location annotation’s position #12907

[ios] Add delegate method to specify the user location annotation’s position #12907

captainbarbosa commented Sep 17, 2018 •

edited

Loading

1ec5 Sep 20, 2018

1ec5 Sep 20, 2018

1ec5 Sep 20, 2018

captainbarbosa Sep 20, 2018

captainbarbosa Sep 20, 2018

1ec5 Sep 20, 2018

captainbarbosa Sep 20, 2018

1ec5 Sep 24, 2018

1ec5 Sep 24, 2018

1ec5 Sep 24, 2018

1ec5 Sep 24, 2018

1ec5 Sep 24, 2018

1ec5 commented Sep 26, 2018

captainbarbosa commented Oct 1, 2018

julianrex left a comment

julianrex Oct 1, 2018

julianrex Oct 1, 2018

captainbarbosa commented Oct 2, 2018

captainbarbosa commented Oct 2, 2018

1ec5 Oct 2, 2018

1ec5 Oct 2, 2018

1ec5 Oct 2, 2018

1ec5 Oct 2, 2018

1ec5 left a comment

friedbunny Oct 2, 2018

[ios] Add delegate method to specify the user location annotation’s position #12907

[ios] Add delegate method to specify the user location annotation’s position #12907

Conversation

captainbarbosa commented Sep 17, 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

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

1ec5 commented Sep 26, 2018

captainbarbosa commented Oct 1, 2018

julianrex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainbarbosa commented Oct 2, 2018

captainbarbosa commented Oct 2, 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

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainbarbosa commented Sep 17, 2018 •

edited

Loading