Add logic to determine faster route

[abhishek1508]

Description

Add logic to use user current location to request for faster route. Compare new route with current route to determine if it is faster #221

• I have added any issue links
• I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
• I have added the appropriate milestone and project boards

Goal

Add logic to use user current location to request for faster route. Compare new route with current route to determine if it is faster.

Implementation

Add logic to use user current location to request for faster route. Compare new route with current route to determine if it is faster.

Testing

Please describe the manual tests that you ran to verify your changes

• I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
• I have tested via a test drive, or a simulation/mock location app
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have updated the CHANGELOG including this PR

[LukasPaczos]

Good progress 👍

How about DirectionsSession exposes a callback like in #2455, but instead of calling it FaterRouteCallback, we call it AlternativeRouteObserver.
The session still maintains the timer and makes periodic requests that invoke the observers.

MapboxNavigation can subscribe to this event and based on the route progress determine whether the alternative route is actually faster. If it is, it notifies FaterRouteObservers that now live and are maintained in MapboxNavigation.

This way, we are decreasing the coupling between the 2 classes and simplifying the whole flow.

[abhishek1508]

Good progress 👍

How about DirectionsSession exposes a callback like in #2455, but instead of calling it FaterRouteCallback, we call it AlternativeRouteObserver.
The session still maintains the timer and makes periodic requests that invoke the observers.

MapboxNavigation can subscribe to this event and based on the route progress determine whether the alternative route is actually faster. If it is, it notifies FaterRouteObservers that now live and are maintained in MapboxNavigation.

This way, we are decreasing the coupling between the 2 classes and simplifying the whole flow.

I had a great discussion with @Guardiola31337 last week on 2 different approaches to implement faster route detection. We explored the possibility of having the Faster Route Timer in DirectionsSession. The problem is to make a faster route request you use the same API as route request. The API needs RouteOptions which needs updated user current location. DirectionsSession doesn't have access to users current location. We don't want to expose methods to DirectionSession that maintains users current location or make DirectionsSession observe location updates. The architecture was implemented to make sure that MapboxNavigation will be the intermediary b/w Directions and Trips and any feature that needs access to properties from these 2 different modules will be handled in MapboxNavigation. Hence, the timer cannot live in DirectionsSession.

The implementation above is the first iteration towards FasterRoute. I plan to take out the implementation details from MapboxNavigation so that it is light in the next iteration. Also, I would rather not use the term AlternativeRouteObserver as I feel that may be confusing from client's perspective. I say this because there is already this feature where we allow users to specify if they want alternative routes. and draw them accordingly. We then have offRoute observers and callbacks that inform users about onRoutesChanged. Having specific terms for features makes it easy to understand what is one doing. Wdyt?

[LukasPaczos]

The location samples are the tiebreaker here. If we don't want to push them to the directions session, which we shouldn't, the timer has to live in MapboxNavigation and exposing additional route request method might be the only way 👍

[abhishek1508]

I don't see any other outstanding comments here.
@Guardiola31337 @LukasPaczos would be great if you could approve. I would like to merge this so that this code is then accessible in this ticket and it makes it easy for everyone to understand how is the entire faster route tied together

[LukasPaczos]

Capturing from multiple threads above:

• I'm okay with exposing the states if you feel like it's the right thing to do now. I can see the value, we'll just need to be extra careful.
• I still think that the faster route timer should run even if we are not in active guidance and leave the decision of whether or not to update the route to the user.

[abhishek1508]

• I'm okay with exposing the states if you feel like it's the right thing to do now. I can see the value, we'll just need to be extra careful.

@LukasPaczos I would like to give some time to think on what are the possible other ways to expose navigationState in general. I think this is good for first iteration. We should definitely have a broader discussion around this in a separate follow up ticket that is more targeted to NavigationState rather than faster route. Will create a ticket for it.

[LukasPaczos]

Sounds good, thanks @abhishek1508.

[codecov-io]

Codecov Report

Merging #2462 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2462   +/-   ##
=========================================
  Coverage     32.29%   32.29%           
  Complexity      621      621           
=========================================
  Files           143      143           
  Lines          5605     5605           
  Branches        434      434           
=========================================
  Hits           1810     1810           
  Misses         3595     3595           
  Partials        200      200

[Guardiola31337]

Capturing from internal discussion with @abhishek1508 that we're going to try the following:

• Move NavigationSession back to libnavigation-core (for now)
• Remove unnecessary back and forth cycle between MapboxNavigation and DirectionsSession
• Reuse RoutesRequestCallback
• Run faster route timer independently of the NavigationSession.State
• Link faster route timer lifecycle to observer register / unregister

cc @LukasPaczos

[LukasPaczos]

Looks good! Minor comments and should be good to merge (when tests are ready).

[Guardiola31337]

This is looking pretty good, great work @abhishek1508

Adding to @LukasPaczos's comments I left some other minor ones.

[Guardiola31337]

Thanks for addressing the feedback @abhishek1508

Left some minor comments. Also it seems that we lost some tests (e.g. fasterRoute_timerStartedOnce and fasterRoute_canceledByNewRequest) 🤔 Not sure if covered by others, could you confirm?

Other than that, this looks good to me 🚀

[Guardiola31337]

Minor thing

Normally is good to structure the tests following the triple A 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=40 http://wiki.c2.com/?ArrangeActAssert
If the Arrange part gets too long we should create factory or builder methods.

This applies to the rest of the tests 👇

[Guardiola31337]

NIT This is still valid

[abhishek1508]

@Guardiola31337 fasterRoute_timerStartedOnce should not be concern of MapboxNavigation or MapboxDirectionSession. All they should do is call start on the timer and timer should be responsible to check if it is being started once or multiple times. MapboxTimerTest doesn't exist at the moment, hence I am going to cut a ticket for it.

[Guardiola31337]

👀 @cafesilencio @evabishchevich @JunDai this PR brings some insights and thoughts on what we were discussing today. Please read the threads comments when you have a chance.

[Guardiola31337]

Let's cut follow up tickets for https://github.com/mapbox/mapbox-navigation-android/pull/2462/files#r380339602 and #2462 (comment) and we should be good to go.

Thanks for addressing the feedback @abhishek1508 🚀

[Guardiola31337]

NIT This is still valid