mapbox / mapbox/mapbox-navigation-ios
NavigationMapView should not assume first route is selected and active
Nobody has claimed this yet.
- Dominant language
- Swift
- Stars
- 918
- Forks
- 326
- Avg merge
- 1h 16m
- Merged PRs (30d)
- 3
Description
NavigationMapView needs to stop assuming that the first route in an array of routes is the selected or active route.
## Background
Since the very beginning, NavigationMapView has always assumed that the first route in an array of routes is the selected route in a preview or the active route during turn-by-turn navigation. This was possible because the application would be expected to move the selected route to the front of the array any time the selection changed, and because NavigationViewController originally only accepted and stored a single route, disregarding any other routes that may have been part of the same route response.
## Problem and diagnosis
#3182 refactored much of the navigation SDK to track a RouteResponse paired with a route index, rather than just a route. #3345 reworked rerouting and route refreshing so that the first route in the new response would not be assumed to be the new active route. But NavigationMapView has been largely untouched by these changes. For example, when showing or showcasing a collection of routes, the first route is assumed to be the selected or active route:
https://github.com/mapbox/mapbox-navigation-ios/blob/8257c395745e18255e391978e387034f00086290/Sources/MapboxNavigation/NavigationMapView.swift#L498-L499
By contrast, in Core Navigation, NavigationService and RouteController expect the route index to correspond to the route’s index in the original, unmodified route response:
https://github.com/mapbox/mapbox-navigation-ios/blob/b3f19116000945769f1957e83e30f64308be46b8/Sources/MapboxCoreNavigation/Router.swift#L15-L26
For example, this is the index that Router passes to the Route Refresh API, which knows nothing about the swapping that took place earlier on the client side:
https://github.com/mapbox/mapbox-navigation-ios/blob/b3f19116000945769f1957e83e30f64308be46b8/Sources/MapboxCoreNavigation/Router.swift#L203
This inconsistency is a trap for developers, who might be tempted to always assume that the first route is the selected route, because NavigationService and RouteController behavior are somewhat hidden from a typical developer. #3403 illustrates the problem that can result.
## Proposed solution
NavigationMapView methods should no longer assume that the first element in an array of Route objects represents the selected or active route. This will require some methods to accept a route index alongside the array of Routes, or perhaps an IndexedRouteResponse instance, a backwards-incompatible change either way.
Every call to `Array.first` needs to be modified to use the route index. For example, the “vanishing route line” feature only updates the first route:
https://github.com/mapbox/mapbox-navigation-ios/blob/8257c395745e18255e391978e387034f00086290/Sources/MapboxNavigation/NavigationMapView.swift#L180-L184
## Alternative solutions
The proposed solution would be a pretty invasive change to a key UI class coming late in the v2.0.0 release cycle. Alternatively, we could clearly document which methods expect the arrays sorted in priority order and which expect the arrays in the original order. At a minimum, the example applications all need to pass in the original route index instead of assuming 0 based on the current index: #3403.
/cc @mapbox/navigation-ios @OttyLab
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by auditing the Array.first usages in Sources/MapboxNavigation/NavigationMapView.swift, especially the cited route-line and route-display code, then compare them with route-index handling in Sources/MapboxCoreNavigation/Router.swift. Inspect issue #3403 and the example applications to identify callers that assume index 0. Done means NavigationMapView and its callers consistently use the original selected route index, with affected tests and examples updated.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- swift
- Domain
- mobile
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100