Route Refresh #420
Route Refresh #420
Conversation
Udumft
added
platform parity
op-ex
| let route = Route(legs: updatedLegs, | ||
| shape: route.shape, | ||
| distance: route.distance, | ||
| expectedTravelTime: route.expectedTravelTime) |
There was a problem hiding this comment.
Instead of creating a brand-new Route object based on the old one, can we mutate the existing route object? I’m just concerned that some SDK or application code may have stored a reference to the old Route that’s now outdated. It would be fine to make additional properties read-write in order to reuse the existing data structures. If we’re concerned about changing data out from behind the client’s back, then I suppose we could create a parallel MutableRoute class instead, but that could be a lot to maintain. In the future, once we find a way to convert these types to structs, we’ll be able to make mutability more explicit anyways.
There was a problem hiding this comment.
Making Route mutable should still be done explicitly outside Directions.refresh because (1) users should be aware of the update and have control over it , (2) refreshing route also involves updating RouteProgress (and potentially anything else) and doing some calculations, so it is safer be done by user's completion.
There was a problem hiding this comment.
users should be aware of the update and have control over it
Developers can observe changes to the Route and RouteLeg properties using key-value observation. The refreshable properties are already individually mutable, so there’s already an expectation that they could change behind the developer’s back. What the interface doesn’t explicitly call out is that refreshing the route silently makes any existing route references become stale; client code might not be in a good position to obtain the refreshed route in that case.
1ec5
force-pushed
the
2284-route-refresh-directions
branch
from
0cf550b to
73f6e42
Compare
chezzdev
force-pushed
the
2284-route-refresh-directions
branch
from
73f6e42 to
26c760f
Compare
|
For context, the Route Refresh API only refreshes a single leg at a time, and it doesn’t accept the route object itself as input, so instead it requires us to pass in the “route” UUID (which is actually the same across all the routes in a single response), a route index (to figure out which of those routes we mean), and a leg index. The Directions API doesn’t return route indices; that was something the PR originally calculated based on each route’s index in the array. The motivation behind #420 (comment) is that we’re trying to move away from stashing ad-hoc stuff inside Route and Match objects, in part because MapboxNavigationNative will soon handle postprocessing the route response for MapboxCoreNavigation and may lose or barf on any nonstandard properties we add. |
| var params: [URLQueryItem] = [] | ||
| params += [URLQueryItem(name: "access_token", value: credentials.accessToken)] | ||
|
|
||
| var unparameterizedURL = URL(string: "directions-refresh/v1/\(DirectionsProfileIdentifier.automobileAvoidingTraffic.rawValue)", relativeTo: credentials.host)! |
There was a problem hiding this comment.
The automobileAvoidingTraffic profile identifier is hard-coded here because the directions refresh API endpoint only supports refreshing routes calculated using this profile. (The other profiles have no traffic congestion data to refresh.) There is code in RouteOptions to avoid enabling route refreshing for the other profiles, but should we also check that the RouteLeg.profileIdentifier is .automobileAvoidingTraffic, in case the developer enabled route refreshing on a route of the route profile?
| segmentDistances = annotation?.segmentDistances | ||
| expectedSegmentTravelTimes = annotation?.expectedSegmentTravelTimes | ||
| segmentSpeeds = annotation?.segmentSpeeds | ||
| segmentCongestionLevels = annotation?.segmentCongestionLevels | ||
| segmentMaximumSpeedLimits = annotation?.segmentMaximumSpeedLimits |
There was a problem hiding this comment.
I think we can live with this repetitiveness for now, but long term I’m concerned about potentially adding more attribute options and forgetting to copy one of them over here. To avoid that problem, and also to clean up the RouteLeg interface a bit, we could make RouteLegAnnotation public and replace the various segment* properties on RouteLeg with a single attributes property that we’d simply replace when refreshing.
We can take care of this refactoring as tail work in a future release, preserving backwards compatibility by leaving the segment* properties as computed properties.
There was a problem hiding this comment.
I moved this copying to a more obvious location in the setter of the attributes property, taking advantage of copy-by-value for structs.
| extension RouteLeg: NSCopying { | ||
| public func copy(with zone: NSZone? = nil) -> Any { |
There was a problem hiding this comment.
Since #382, we’ve avoided implementing NSCopying, because in the past, we’ve frequently forgotten to add to copy methods when adding new properties, resulting in silent data loss. The hope expressed in #381 was that we’d convert the classes to structs, which have built-in copying functionality. In the meantime, the navigation SDK worked around the lack of NSCopying conformance in RouteOptions by round-tripping through JSON:
But I can see how that approach would be wildly unperformant for copying an entire leg. Do we really need to copy each leg and create a new route when refreshing it? By making the segment* properties mutable, we’ve already set the expectation that they can be changed out from under client code. I think it would be simpler to just set those properties on the original object.
There was a problem hiding this comment.
Do we really need to copy each leg and create a new route when refreshing it?
Looks like we previously discussed this point in #420 (comment).
1ec5
force-pushed
the
2284-route-refresh-directions
branch
from
7c1d5ff to
fa18db8
Compare
| - parameter completionHandler: The closure (block) to call with the resulting skeleton route data. This closure is executed on the application’s main thread. | ||
| - parameter startLegIndex: The index of the leg in the route at which to begin refreshing. If this argument is omitted, the entire route is refreshed. |
There was a problem hiding this comment.
I think startLegIndex should go before completionHandler in comment.
| return | ||
| } | ||
|
|
||
| DispatchQueue.global(qos: .userInitiated).async { |
There was a problem hiding this comment.
Is there any specific reason we use global queue here? Since task is running on background thread I think it can be skipped.
There was a problem hiding this comment.
This is consistent with Directions.calculate(_:completionHandler:). If I’m not mistaken, the idea is to perform deserialization on a dedicated thread other than the CFNetwork thread.
…ctures for en(de)coding responses, refactored out RouteLegAnnotation. Added routeIndex to be stored in Route or Match objects. Added "enable_refresh" key for automobile-traffic profile requests.
Co-authored-by: Minh Nguyễn <[email protected]>
1ec5
force-pushed
the
2284-route-refresh-directions
branch
from
bf1a130 to
2dc18b9
Compare
…s and outputs as the API, leaving the task of merging the route to client code. Made the API response model types fully public and documented them. Removed unnecessary optionality and accompanying errors for nil checks. Grouped the various per-segment attribute properties into a single computed property on RouteLeg, as a first step toward consolidating attribute storage into one property. Renamed annotations to attributes for consistency. Renamed route identifier to response identifier to reflect the actual meaning of the identifier. Documented new public APIs.
This implementation was correct but prone to getting outdated as additional properties get added. Client code can still effectively copy a RouteLeg either by inlining this code or round-tripping it through JSON using JSONEncoder and JSONDecoder.
1ec5
force-pushed
the
2284-route-refresh-directions
branch
from
2dc18b9 to
ef1ca77
Compare
The Swift compiler in Xcode 10.2 is unable to infer CongestionLevel as the type of one argument of the autosynthesized initializer for RouteLeg.Attributes.
1ec5
force-pushed
the
2284-route-refresh-directions
branch
from
6d6ad36 to
06a85e7
Compare
It is the application’s or the navigation SDK’s responsibility to enable this feature for turn-by-turn navigation use cases. No need to enable this feature for use cases that will never end up refreshing this particular route response.
mapbox/mapbox-navigation-ios#2284
Work-In-Progress.
Added Public method to call a Route Refresh.
Refactored Route Coding a bit to re-use some parts in Refresh Response.
Looking for some better way for updating Route contents...