Skip to content

Enable proactive rerouting by default#1986

Merged
frederoni merged 6 commits into
masterfrom
fred/proactive-rerouting
Feb 26, 2019
Merged

Enable proactive rerouting by default#1986
frederoni merged 6 commits into
masterfrom
fred/proactive-rerouting

Conversation

@frederoni
Copy link
Copy Markdown
Contributor

@frederoni frederoni commented Feb 21, 2019

Fixes #1902 - Enable proactive rerouting by default

  • Reuse proactive rerouting from LegacyRouteController
  • Add tests

@frederoni frederoni requested review from 1ec5 and JThramer February 21, 2019 19:24
@frederoni frederoni changed the title Reuse proactive rerouting from LegacyRouteController Enable proactive rerouting by default Feb 21, 2019
Comment thread MapboxCoreNavigation/Router.swift Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We’re getting close to being able to refresh the existing route on a timer as well. Should we control both behaviors through a single, more generically named property, on an interval defined by a single, more generically named constant? Or should we keep the two complementary behaviors separate?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's OK to keep a separate flag for "reroutesProactively" (although the naming leaves a little to be desired). Faster route polling should happen directly after a directions refresh response, so it's possible we'll be reworking this code again soon.

I don't think we should combine them into one option, to directly answer your question.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what does 'refresh the existing route' mean? separate i reckon, by the sound of it.

@frederoni frederoni force-pushed the fred/proactive-rerouting branch from a536fc1 to 43c761a Compare February 22, 2019 13:23
@frederoni frederoni force-pushed the fred/proactive-rerouting branch from 43c761a to 618a7c8 Compare February 22, 2019 13:26
Copy link
Copy Markdown
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Suggested changelog entry:

  • Restored the RouteController.reroutesProactively property. (#1986)
  • Added a RouteControllerMinimumDurationRemainingForProactiveRerouting global variable to customize when RouteController stops looking for more optimal routes as the user nears the destination. (#1986)

}
}

extension LegacyRouteController: InternalRouter { }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This line introduces a deprecation warning. Not sure how to work around that 🤔

@frederoni frederoni force-pushed the fred/proactive-rerouting branch from 9e0764e to c0a2a93 Compare February 25, 2019 11:04
@frederoni frederoni marked this pull request as ready for review February 25, 2019 11:05
@frederoni frederoni force-pushed the fred/proactive-rerouting branch from a8b61e2 to 685ed96 Compare February 26, 2019 12:16
@frederoni frederoni merged commit 64f07b7 into master Feb 26, 2019
@frederoni frederoni deleted the fred/proactive-rerouting branch February 26, 2019 14:58
@1ec5 1ec5 added this to the v0.30.0 milestone Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants