mapbox-navigation-android icon indicating copy to clipboard operation
mapbox-navigation-android copied to clipboard

Add route selection by index

Open kmadsen opened this issue 3 years ago • 6 comments

Mapbox Navigation SDK version: 2.1.0

EDIT: grammar

Follow up work from https://github.com/mapbox/mapbox-navigation-android/pull/4892. Anticipating this issue will require refactoring and testing in order to get it all just right. It is considered out of scope of the initial integration of continuous alternatives because we may not want to do this. Re-ordering the list and assuming route index zero also works.

Here is the existing way to change the route leg in MapboxNavigation

    fun navigateNextRouteLeg(callback: LegIndexUpdatedCallback) {
        arrivalProgressObserver.navigateNextRouteLeg(callback)
    }

So here we're proposing adding and/or changing the functions in MapboxNavigation

// Adding routeIndex may be semver, since it changes the position of the setRoute arguments
    fun setRoutes(routes: List<DirectionsRoute>, routeIndex: Int = 0, initialLegIndex: Int = 0) {

// After routes have been set, be able to change the route index with something like this.
    fun setPrimaryRoute(routeIndex: Int)
    fun setPrimaryRoute(route: DirectionsRoute)

Ui code to select a route

Before

    private val mapClickListener = OnMapClickListener { point ->
        CoroutineScope(Dispatchers.Main).launch {
            val result = routeLineApi.findClosestRoute(
                point,
                binding.mapView.getMapboxMap(),
                routeClickPadding
            )

            val routeFound = result.value?.route
            if (routeFound != null && routeFound != routeLineApi.getPrimaryRoute()) {
                val reOrderedRoutes = routeLineApi.getRoutes()
                    .filter { it != routeFound }
                    .toMutableList()
                    .also {
                        it.add(0, routeFound)
                    }
                mapboxNavigation.setRoutes(reOrderedRoutes)
            }
        }
        false
    }

After

    private val mapClickListener = OnMapClickListener { point ->
        CoroutineScope(Dispatchers.Main).launch {
            val result = routeLineApi.findClosestRoute(
                point,
                binding.mapView.getMapboxMap(),
                routeClickPadding
            )

            result.value?.route?.let { mapboxNavigation.setPrimaryRoute(it)
        }
        false
    }

kmadsen avatar Oct 14 '21 14:10 kmadsen

To avoid semver, why don't we introduce

    fun setRoutes(routes: List<DirectionsRoute>, routeIndex: Int = 0, initialLegIndex: Int = 0)
    fun setPrimaryRoute(routeIndex: Int)
    fun setPrimaryRoute(route: DirectionsRoute)

as extensions?

We discussed this in the past and there is no additional power/flexibility that you gain by specifying the index versus reordering the list, it's just semantics.

LukasPaczos avatar Oct 25 '21 10:10 LukasPaczos

We discussed this in the past and there is no additional power/flexibility that you gain by specifying the index versus reordering the list, it's just semantics.

This is not entirely true. Where this has come up in android auto is when selecting and viewing the route alternatives in a list. For example, the list that displays and allows you to select a route requires an index to indicate selection. When selection is done by re-ordering, the display list will automatically re-order. Re-ordering the list after every selection may not be desirable.

Screen Shot 2022-07-14 at 12 01 00 PM

Further on this, reasonable to duplicate the view model from the data model for this reason (also a typical reason to duplicate data)

kmadsen avatar Jul 14 '22 19:07 kmadsen

Interesting use case! We've never previously had an out-of-the-box route management/preview UI and with these use cases becoming more integrated into the SDK iteslf, an index-based selection could be preferable :+1:

Something to consider for the next major version of the SDK.

cc @VysotskiVadim for visibility

LukasPaczos avatar Jul 26 '22 14:07 LukasPaczos

Something to consider for the next major version of the SDK.

@LukasPaczos , why can't we support index based selection in 2.x? Can't we duplicate methods which accept/return List<NavigationRoute> by methods which accepts NavigationRoutes everywhere

class NavigationRoutes(
   val routes: List<NavigationRoute>,
   val primaryRouteIndex: Int
) {
    val primaryRoute get() = routes[primaryRouteIndex]
    val alternativeRoutes get() = routes.filterIndexed { _, index -> index != primaryRouteIndex }
}

We already did similar change moving from DirectionRoute to NavigationRoute 🤔

VysotskiVadim avatar Jul 26 '22 14:07 VysotskiVadim

Yes, we definitely can if needed! I just wanted to add a semver label to make sure we revisit this topic before v3.

LukasPaczos avatar Jul 26 '22 14:07 LukasPaczos

Fixed with a new label :upside_down_face:

LukasPaczos avatar Jul 26 '22 14:07 LukasPaczos