Refactor location snapping #408
Conversation
1ec5
added
bug
| style.addSource(source) | ||
| streetsSources.append(source) | ||
| } | ||
| guard let snappedLocation = routeController.snappedLocation else { |
There was a problem hiding this comment.
Relying on two location managers, route controller being the main source for driving navigation and MGLMapView asking for shouldUpdateTo based on its own location updates feels like it's set up for a race condition. However, #402 should eventually move the data in the other direction and the way CLLocationManager is shared throughout the OS should prevent that but there's no guarantee that the snapped location won't lag 1 location update behind.
There was a problem hiding this comment.
Ah, so should this method be passing location into snappedLocation then?
There was a problem hiding this comment.
I'd say the other way around, the route controller should drive the map view, but that might be out of scope for this PR?
There was a problem hiding this comment.
Yes, I agree. We should have the route controller drive the map view, and #402 will accomplish that pretty cleanly. In the meantime, do you think what’s happening here is any riskier than what’s in master?
|
|
||
| This is a raw location received from `locationManager`. To obtain an idealized location, use the `snappedLocation` property. | ||
| */ | ||
| public var location: CLLocation? |
There was a problem hiding this comment.
Do you think this should be public? If a developer needs the raw user location, shouldn't they just create their own location manager?
There was a problem hiding this comment.
snappedLocation has to be public so that MapboxNavigation can use it. I figured if we expose snappedLocation, we might as well expose the raw location too. But we could instead expose snappedLocation as location and pretend there isn't a raw location to begin with.
Broke up navigationMapView(_:shouldUpdateTo:) into UI and non-UI methods, making it easier to move the non-UI method to Core Navigation. Update the current road name label based on the snapped location, not the raw location.
Also added RouteController.location for convenience.
1ec5
force-pushed
the
1ec5-location-snapping-refactor
branch
from
9eba4ef to
3f9c942
Compare
Hide the raw location from the public. Keep labeling the current road based on the raw location after the user goes off-course or passes the destination.
|
29da08e causes the current road name to continue to be labeled even if the user diverges from the route. |
1ec5
added
the
backwards incompatible
This PR moves the user location snapping code from RouteMapViewController in MapboxNavigation to RouteController in CoreNavigation. Conversely, it moves the
snapsUserLocationAnnotationToRouteproperty from RouteController in CoreNavigation to RouteMapViewController in MapboxNavigation, where it’s actually used.RouteController now has public
locationandproperties, while NavigationViewController has asnappedLocationsnapsUserLocationAnnotationToRouteproperty.Along the way, the current road name label now respects course snapping, and various NavigationMapViewDelegate methods have proper Objective-C names. (Previously, selector pieces ended in dangling prepositions instead of nouns.)
#402 partly refactors the location snapping code as well. I think this PR should land before that one to keep things simple.
To summarize:
snapsUserLocationAnnotationToRouteproperty to NavigationViewController (to fix snapsUserLocationAnnotationToRoute has no effect #406)/ref #321
/cc @ericrwolfe @frederoni @bsudekum