RouteResponse usage refactoring by Udumft · Pull Request #3182 · mapbox/mapbox-navigation-ios

Udumft · 2021-07-16T11:40:41Z

Description

This PR replaces usage of Route + Route index in favor of it's original RouteResponse. It also removes usage of Route.routeIdentifier, as this is a redundant property should be removed too.

Implementation

Most of the changes is just a replacing of IndexedRoute with RouteResponse. But it also forced updating some related entities like CPTrip.userInfo, removing RouteProgress.routeIndex or updating SessionStatewith dedicatedrouteIdentifier`. There are many public API changes.

S2Ler · 2021-07-19T07:25:54Z

This PR will clash with #3159

S2Ler · 2021-07-22T09:15:39Z

#2589 mentions:

A convenience initializer for MapboxNavigationService can wrap a Route in a RouteResponse, and a convenience initializer for NavigationViewController can wrap a Route in a RouteResponse and MapboxNavigationService.

Shall we add these convenience inits in this PR? Or what's the reason of not including them?

My motivation not to add these was that all of the routes are supposed to be a result of API calling, and thus are always wrapped in RouteResponse. If, for some uncommon reason, user is creating a Route object differently, he should take care about correctly wrapping it, providing other required fields like options and identifier.

Hi @Udumft ,

I have a use case in which I would prefer injecting a Route and am wondering if I now need to create a convenience init to support? It seems strange to move from a strong required Route, to a model of optionals?

https://github.com/mapbox/straightaway-ios/blob/main/Straightaway/Modules/Map/ContinuousNavigationViewController.swift#L85

Given our UI/ViewModels are intentionally separate from network models, exposing a network response object seemed heavy.

For additional context, in an upcoming feature change, we will be displaying multiple direction options. Which we will display, and begin navigation upon selection of a given route.

cc: @irtemed88

Hi @Udumft ,

I was seeing if I could get your suggestion for how I could approach this? I took a first stab at the upgrade migration.

S2Ler · 2021-07-22T09:33:20Z

I think this code was added to support Storyboards. @MaximAlien please review.

S2Ler · 2021-07-22T09:52:58Z

@1ec5 Your review is required since you created the original ticket.

S2Ler

I think the following should be done before merge:

Verify that Storyboards integration isn't broken cc @MaximAlien
Review from @1ec5

1ec5 · 2021-07-23T16:18:41Z

+    public let routeResponse: RouteResponse
+    public let routeIndex: Int


Remember to add documentation comments about these properties and also the initializer below so that jazzy recognizes them.

1ec5 · 2021-07-23T16:21:41Z

+    /// The `RouteResponse` containing the route along which the user is expected to travel, plus its index in this `RouteResponse`, if applicable.
+    ///
+    /// If you want to update the route use `Router.updateRoute(with:routeOptions:)` method.
+    var indexedRouteResponse: IndexedRouteResponse { get }


If it isn’t valid to set indexedRouteResponse directly, let’s make it publicly read-only.

1ec5

Meant to attach this approval to #3182 (review):

I think it would be quite beneficial to remove some redundancy in a subsequent PR, now that RouteResponse encapsulates more information, but this PR is mergeable as is once the few bits of missing documentation are added.

MaximAlien

This PR brings quite a lot of public API changes. We'll need to create another PR in https://github.com/mapbox/mapbox-navigation-ios-examples to address compilation issues.

MaximAlien · 2021-07-23T16:58:29Z

    /**
     A `Route` object constructed by [MapboxDirections](https://docs.mapbox.com/ios/api/directions/).
     */
    public var route: Route? {


Why NavigationViewController.route, NavigationViewController.routeIndex and NavigationViewController.routeOptions were converted to get-only properties? At this point it breaks functionality, which allows to create NavigationViewController instance via UIStoryboard.

These properties were made read-only to introduce single entry-point method for changing the route: updateRoute(with:routeOptions:). Not sure what to do with storyboard scenario...

#3159 was primarily concerned with changing the route during a trip, but the storyboard code path is about setting up the trip in the first place, at a point when we’re not as concerned about the navigator getting only piecemeal input. Can we introduce something specific to storyboards, perhaps even with “forStoryboard” in the name?

Would be good to have a special method which setups vc after view controller. This method will set appropriate private variables (could be even grouped in a struct) which will be read in the code which setups vc in storyboards case.

When I read this code I found it confusing why route variable has such strange get/set.

MaximAlien · 2021-07-23T21:23:45Z

We'll have to also revise documentation after landing current PR: e.g. in https://docs.mapbox.com/ios/beta/navigation NavigationViewController and related APIs are mentioned quite often.

MaximAlien · 2021-08-03T01:02:34Z

Why do we need to perform check to see if view was really loaded? I recall that in previous implementation in #2974 it wasn't needed. Just want to make sure that this will not cause any side effects.

This check is added since updating routeResponse, routeIndex or routeOptions will not have effect on the contents NavigationViewController if view is already loaded. In this case view - is the Navigation View, and these properties are only stored in NavigationViewController to initialize the NavigationView. So, this check lets user know if this update actually happened or not. Previous implementation did silent update of a local property, but that did not affect the NavigationView if it was already loaded.

MaximAlien · 2021-08-03T01:07:24Z

It'd be great to create a follow-up ticket to update documentation everywhere. In scope of this PR files like README.md and cover.md can be updated as they show how to use NavigationViewController.

I missed that, thanks

…teResponse+RouteIndex. Updated Codebase, Tests, code docs.

…tifier usage from tests.

…uple to struct entity type.

…comments, improved method naming, removed obsolete parts.

…ed, Removed unnecessary usage of routeOptions from CPTrip, NavigationViewController convenience initializer refined.

… method for storyboard scenario. CHANGELOG updated

…ionViewController usage.

MaximAlien

This PR changes so many public APIs that it's hard to see what can be wrong. On high level seems to look good to me. We should review all docs and update examples to make sure that there is not major regression and everything is up-to-date.

Udumft · 2021-08-04T12:03:19Z

Ticket to update examples project : mapbox/mapbox-navigation-ios-examples/issues/135

Udumft added op-ex Refactoring, Tech Debt or any other operational excellence work. backwards incompatible changes that break backwards compatibility of public API labels Jul 16, 2021

Udumft added this to the v2.0.0 (RC) milestone Jul 16, 2021

Udumft self-assigned this Jul 16, 2021

Udumft marked this pull request as ready for review July 16, 2021 12:06

Udumft requested a review from a team July 16, 2021 12:06

Udumft mentioned this pull request Jul 16, 2021

Replace Route with RouteResponse plus route index #2589

Closed

2 tasks

S2Ler reviewed Jul 19, 2021

View reviewed changes

Comment thread Sources/MapboxCoreNavigation/Router.swift Outdated

Udumft force-pushed the vk/2589-route-response-refactor branch 3 times, most recently from 6ef05e4 to 8b8a416 Compare July 21, 2021 09:05

Udumft requested review from a team and S2Ler July 21, 2021 09:14

S2Ler reviewed Jul 22, 2021

View reviewed changes

S2Ler requested a review from 1ec5 July 22, 2021 09:52

jill-cardamon force-pushed the vk/2589-route-response-refactor branch from 8b8a416 to 4ac8389 Compare July 23, 2021 00:28

Udumft requested review from a team and S2Ler July 23, 2021 09:18

S2Ler requested a review from MaximAlien July 23, 2021 09:19

S2Ler reviewed Jul 23, 2021

View reviewed changes

1ec5 reviewed Jul 23, 2021

View reviewed changes

1ec5 linked an issue Jul 23, 2021 that may be closed by this pull request

Replace Route with RouteResponse plus route index #2589

Closed

2 tasks

1ec5 approved these changes Jul 23, 2021

View reviewed changes

MaximAlien reviewed Jul 23, 2021

View reviewed changes

Udumft force-pushed the vk/2589-route-response-refactor branch from 95c2db9 to 0849319 Compare July 26, 2021 16:41

Udumft requested review from MaximAlien and S2Ler July 27, 2021 12:08

Udumft force-pushed the vk/2589-route-response-refactor branch from b0575c3 to 164a251 Compare August 2, 2021 14:57

MaximAlien reviewed Aug 3, 2021

View reviewed changes

Comment thread Sources/MapboxNavigation/NavigationViewController.swift Outdated

MaximAlien reviewed Aug 3, 2021

View reviewed changes

Udumft added 10 commits August 3, 2021 17:24

vk-2589-route-reponse-refactor: refactored Route usage into using Rou…

02fe511

…teResponse+RouteIndex. Updated Codebase, Tests, code docs.

vk-2589-route-response-refactor: removed usage of Route.routeIdentifier.

d3cbe6e

vk-2589-route-response-refactor: CHANGELOG updated. Removed routeIden…

cc6ccf5

…tifier usage from tests.

vk-2589-route-response-refactor: replaced IndexedRouteResponse from t…

4b0d267

…uple to struct entity type.

vk-2589-route-response-refactor: fixed Unit tests compilation

d098027

vk-2589-route-response-refactor: changelog entry updated, fixed code …

0347d3d

…comments, improved method naming, removed obsolete parts.

vk-2589-route-response-refactor: bumped MapboxDirections to beta.7

409a10e

vk-2589-route-response-refactor: fixed code comments, CHANGELOG updat…

ef56b8f

…ed, Removed unnecessary usage of routeOptions from CPTrip, NavigationViewController convenience initializer refined.

vk-2589-route-response-refactor: Added NavigationViewController setup…

02c0bba

… method for storyboard scenario. CHANGELOG updated

vk-2589-route-response-refactor: update doc examples with new Navigat…

fae8fd7

…ionViewController usage.

Udumft force-pushed the vk/2589-route-response-refactor branch from ab51a6d to fae8fd7 Compare August 3, 2021 14:51

vk-2589-route-response-refactor: fixed failing Unit test

34b80d6

MaximAlien approved these changes Aug 4, 2021

View reviewed changes

Comment thread Sources/MapboxCoreNavigation/RouteProgress.swift

Udumft mentioned this pull request Aug 4, 2021

Support RouteResponse usage mapbox/mapbox-navigation-ios-examples#135

Closed

vk-2589-route-response-refactor: RouteProgress code comment corrected.

a29385d

Udumft merged commit 9925bee into main Aug 4, 2021

Udumft deleted the vk/2589-route-response-refactor branch August 4, 2021 12:03

This was referenced Aug 4, 2021

Make NavigationService optional. #3223

Merged

Route preview doesn't show route line on CarPlay. #3228

Closed

1ec5 mentioned this pull request Sep 9, 2021

Fix inconsistent route progress and index after rerouting #3345

Merged

1ec5 mentioned this pull request Sep 24, 2021

NavigationMapView should not assume first route is selected and active #3404

Open

1ec5 mentioned this pull request Oct 18, 2021

Using Map Matching guide uses route instead of route response #3494

Closed

jill-cardamon mentioned this pull request Oct 19, 2021

Using Map Matching guide now uses route response #3497

Merged

		public let routeResponse: RouteResponse
		public let routeIndex: Int

Conversation

Udumft commented Jul 16, 2021

Description

Implementation

Uh oh!

S2Ler commented Jul 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Udumft Jul 23, 2021 • edited by 1ec5 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmiluski Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S2Ler commented Jul 22, 2021

Uh oh!

S2Ler left a comment • edited by MaximAlien Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

1ec5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaximAlien commented Jul 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximAlien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Udumft commented Aug 4, 2021

Uh oh!

Reviewers

Udumft Jul 23, 2021 •

edited by 1ec5

Loading

dmiluski Aug 5, 2021 •

edited

Loading

S2Ler left a comment •

edited by MaximAlien

Loading

1ec5 left a comment •

edited

Loading

1ec5 Jul 27, 2021 •

edited

Loading