mapbox / mapbox/mapbox-navigation-ios

NavigationMapView should not assume first route is selected and active

Open
#3,404 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

backwards incompatible bug documentation jira-sync-complete op-ex topic: documentation
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.