Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate to nav-native’s route following #1904

Merged
merged 48 commits into from
Jan 30, 2019
Merged

Migrate to nav-native’s route following #1904

merged 48 commits into from
Jan 30, 2019

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Dec 17, 2018 •
edited
Loading

This PR further adopts NavigationNative for route following.
The old RouteController has been renamed to LegacyRouteController and is now deprecated.

Migrating to MapboxNavigationNative’s route following logic.

  • Refactor RouteController -> LegacyRouteController
  • Refactor PortableRouteController -> RouteController
  • Keep customizable arrival thresholds (incl willArrive and didArrive delegate methods)
  • Enable remaining events manager tests
  • Fix repetetive voice instructions

cc @JThramer

@frederoni frederoni added the platform parity Required to keep on par with Android. label Dec 18, 2018
akitchen
akitchen reviewed Dec 19, 2018
View reviewed changes
MapboxCoreNavigation/PortableRouteController.swift Outdated


@objc(MBPortableRouteController)
open class PortableRouteController: RouteController {
open class PortableRouteController: NSObject {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an exciting change. What is going to happen with the existing RouteController as a part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we would just delete it, but I assume we have to deprecate it. 🤔

@frederoni frederoni force-pushed the fred/nn-route-following branch from be63205 to ff9afb3 Compare December 21, 2018 12:57
@frederoni frederoni force-pushed the fred/nn-route-following branch from 3f03b69 to 19c5c7e Compare January 10, 2019 13:56
@frederoni frederoni force-pushed the fred/nn-route-following branch from f01ee4e to f8ffa59 Compare January 17, 2019 12:42
@frederoni frederoni force-pushed the fred/nn-route-following branch from f8ffa59 to db79d6b Compare January 17, 2019 13:29
@frederoni
This is a combination of 3 commits.
7fb49c2 
Test arrive maneuver icon

Don't annouce spoken instruction if we are going to reroute

Update test fixtures
@frederoni frederoni force-pushed the fred/nn-route-following branch from c7c4a9e to 7fb49c2 Compare January 17, 2019 16:34
@frederoni frederoni force-pushed the fred/nn-route-following branch from 0cecb38 to 40f2e7e Compare January 29, 2019 17:58
JThramer
JThramer approved these changes Jan 30, 2019
View reviewed changes
Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great except for this one thing

MapboxCoreNavigation/Router.swift Outdated
- parameter route: The route to follow.
- parameter directions: The Directions object that created `route`.
- parameter source: The data source for the RouteController.
*/
@objc(initWithRoute:directions:locationManager:)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initWithRoute:directions:dataSource:

@frederoni frederoni force-pushed the fred/nn-route-following branch from 19856e6 to d4a96d1 Compare January 30, 2019 22:00
@frederoni frederoni merged commit 0ffabdd into master Jan 30, 2019
@1ec5 1ec5 added this to the v0.29.0 milestone Feb 1, 2019
@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Feb 1, 2019
@1ec5 1ec5 mentioned this pull request Feb 8, 2019
Closed
@1ec5
Copy link
Contributor

1ec5 commented Feb 9, 2019

The backwards-incompatible label was added in part because the “renaming” of LegacyRouteController to RouteController isn’t a one-for-one replacement. For example, the following change hasn’t been documented:

  • As part of replacing LegacyRouteController (né RouteController) with RouteController (née PortableRouteController), the reroutesProactively option was effectively removed. This is a useful option, one that Enable proactive rerouting by default #1902 would’ve enabled by default. Developers are now forced to choose between MapboxNavigationNative-based route following functionality and the ability to navigate users around flashmobs, so to speak.

@1ec5 1ec5 mentioned this pull request Feb 9, 2019
Closed
@frederoni frederoni mentioned this pull request Feb 18, 2019
Merged
@1ec5 1ec5 deleted the fred/nn-route-following branch December 5, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ec5 1ec5 1ec5 left review comments

@akitchen akitchen akitchen left review comments

@JThramer JThramer JThramer approved these changes

Assignees

@frederoni frederoni

@JThramer JThramer

Labels
backwards incompatible changes that break backwards compatibility of public API platform parity Required to keep on par with Android.
Projects
None yet
Milestone
v0.29.0
Development

Successfully merging this pull request may close these issues.
4 participants
@frederoni @1ec5 @akitchen @JThramer