Feature/ak #220 faster route callback

[abhishek1508]

Description

Implement callback to allow user to accept or dismiss faster route (https://github.com/mapbox/navigation-sdks/issues/220)

• 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

Implement callback to allow user to accept or dismiss faster route

Implementation

Implement callback to allow user to accept or dismiss faster route

Screenshots or Gifs

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

[Guardiola31337]

Could you @designevokes @map-angela @noracalabrese take a look at this when you have a chance? It'd be 💯 if we can start getting input from you on the design side of things.

[Guardiola31337]

Left some minor comments / questions to discuss as a team @abhishek1508

[Guardiola31337]

Do we need this as part of this PR? Currently faster route is disable by default downstream

mapbox-navigation-android/libnavigation-core/src/main/java/com/mapbox/navigation/core/directions/session/MapboxDirectionsSession.kt

Lines 116 to 119 in fbbeee4

	private fun isRouteFaster(newRoute: DirectionsRoute): Boolean {
	// TODO: Implement the logic to check if the route is faster
	return false
	}

[abhishek1508]

Yes. I would like to stick it as a part of this PR because that's the objective of this ticket. To be able to provide a callback for user to act on faster route. The part where we fix the above isRouteFaster is going to come up here

[Guardiola31337]

This should be customizable from a client point of view as well as enabling / disabling faster functionality completely.

[abhishek1508]

Coming up as a part of this

[LukasPaczos]

Tag the team for a review next time around @abhishek1508, I completely missed the PR! :)

The idea LGTM, we're only missing the docs. The UI lives only in an example so I'm happy to merge and we can keep the design discussion going for the refactor of the UI lib.

[LukasPaczos]

How about we provide a list of routes from the response here?

[abhishek1508]

private fun requestFasterRoute(routeOptions: RouteOptions) {
       if (routes.isEmpty()) {
           return
       }
       router.getRoute(routeOptions, object : Router.Callback {
           override fun onResponse(routes: List<DirectionsRoute>) {
               val route = routes[0]
               if (isRouteFaster(route)) {
                   fasterRouteObservers.forEach { it.onFasterRouteAvailable(route) }
               }
           }

           override fun onFailure(throwable: Throwable) {
               // do nothing
           }

           override fun onCanceled() {
               // do nothing
           }
       })
   }

If you look at the onResponse we use the route at index 0 to check if the route is faster than the current route. If true only then we broadcast a faster route. The question, I would like to ask here is do we want to emit a list of routes because then how does the end client know which route is faster in that list? There are 2 solutions to this:

• We emit the list of route and say that route at index 0 in this list has been verified to be faster; so use it - so then in that case why to even send a list since the 0th route is always going to be faster.
• Send a list to the user and ask him to find out which route is faster

I don't see either of the solution that we would probably want to extend from SDK perspective.

Faster route in itself is a very specific feature that says we found 1 faster route rather than multiple faster routes. In that case I think we should only emit one route.
Comments/Thoughts?

[LukasPaczos]

It'd be great to add unit tests as well.

[codecov-io]

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2455   +/-   ##
=========================================
  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

[LukasPaczos]

As mentioned in #2462, if MapboxNavigation is going to be responsible for making the decision if the route is faster, it could also notify the users back and maintain the observers' list. We don't need to return the control back to the DirectionsSession.

Otherwise, I'd prefer MapboxDirectionsSession to have a constructor injected FasterRouteDetector from #2462 instead of the FasterRouteRequestCallback mechanism. We ultimately don't need to worry about whether or not the requested succeeded as the session's client.

[LukasPaczos]

Let's merge and iterate in #2462 👍