navigation specific route options builder

[VysotskiVadim]

Problem

Developers use RouteObject from the mapbox-java directly. They can easily make a mistake requesting a route: just forget to add any required parameter and Android Navigation SDK may crash.

Imagine a team that is trying out different Nav SDK to compare and select for their application. They may didn't notice applyDefaultNavigationOptions and make a request with coordinates and profile only, which leads to the crash. It's hard to understand that you're getting an IndexOutOfBounds error because you forgot to include steps=true in the request. Would you pick Mapbox Navigation SDK if had such problems during the first use?

Solution

Create such an API for requesting routes so that it isn't possible to make an error.

Changelog

<changelog>Added `MapboxNavigation:buildRouteOptions` that is an experimentation API to safely request a route.</changelog>

TODO:

• cancel request
• java support
• default profile?
• mix profiles: error or override ?
• reconsider API
• compare with swift solution
• add instrumentation test
• manual testing
• Can silent waypoint have bearing and zLevel? I think they should, but we need to test

Do in the context of the following tickets

• approaches
• round about exists?
• driving specific params: depart at, arrive at, max height, max width, alley_bias
• driving-traffic specific params depart at, max height, max width, snapping_include_closures
• API for language\units
• Compare API's capabilities with swift solution
• How to safely share routes?
• experiment with different API formats: compile safe, runtime safe, etc (see more )

[LukasPaczos]

The proposal looks good to me in general - have an opinionated, higher-level API that allows us to build routes while taking advantage of all the onboard features (matched location, bearing, z-level, etc) without developers having to dive deep into documentation to build a good route request manually.

The problem that might arise is maintenance, we might get more and more request to expose features through this NavRouteOptionsBuilder that are already available via RouteOptionsBuilder from mapbox-java. We need to have a clear set of defined rules for what goes into this higher level API and what doesn't.

Then is the question of compatibility, we also need to define a clear path for developers to start with higher-level API and transition to RouteOptiosnBuilder to tweak a parameter that wasn't exposed via NavRouteOptionsBuilder, maybe additional extension that works directly on RouteOptiosnBuilder or something along these lines.

And lastly I'd like to discuss platform parity - are there any measures in the iOS SDK that would apply the runtime properties (like current location, bearing, z-level, etc) without developers having to do that themselves when building a route request? cc @1ec5 @Udumft

[RingerJK]

is the PR ready for review?

[VysotskiVadim]

is the PR ready for review?

yes.

[tomaszrybakiewicz]

This class and LocationProvider interface can be replaced by an extension method on TripSessionLocationProvider

internal suspend fun TripSessionLocationProvider.currentLocation() = suspendCancellableCoroutine<CurrentLocation> { continuation ->
    // return immediately if locationMatcherResult has already been set
    locationMatcherResult?.also {
        continuation.resume(it.toCurrentLocation())
        return@suspendCancellableCoroutine
    }

    // subscribe for the next locationMatcherResult
    val observer = object : LocationObserver {
        override fun onNewRawLocation(rawLocation: Location) = Unit

        override fun onNewLocationMatcherResult(locationMatcherResult: LocationMatcherResult) {
            unregisterLocationObserver(this)
            continuation.resume(locationMatcherResult.toCurrentLocation())
        }
    }
    registerLocationObserver(observer)
    continuation.invokeOnCancellation {
        unregisterLocationObserver(observer)
    }
}

private fun LocationMatcherResult.toCurrentLocation() = CurrentLocation(
    point = enhancedLocation.toPoint(),
    bearing = enhancedLocation.bearing.toDouble(),
    zLevel = zLevel
)

[VysotskiVadim]

Thank you. I agree it can. I'm not sure I understand why an extension function is better than a wrapper class. Please share your thoughts about exception function vs wrapper class .

Not sure if the provided code is expected to be fully functional but it will crash on unregisterLocationObserver(this). You can reed and share your opinion about the crash here

[tomaszrybakiewicz]

Please share your thoughts about exception function vs wrapper class .

There is nothing wrong with wrapper classes. They are extremely useful in languages that doesn't provide any other mechanism to add new functionality to an existing class or when we need to store some state.

In this specific case, we can extend TripSessionLocationProvider functionality by utilizing Kotlin extensions.

Not sure if the provided code is expected to be fully functional but it will crash on unregisterLocationObserver(this).

I am assuming the crash is caused by ConcurrentModificationException. We should create new ticket for patching unregisterLocationObserver().

[VysotskiVadim]

If I replace wrapper by an extension function then NavRouteOptionsBuilder will start to depend on the TripSessionLocationProvider directly. But I would prefer NavRouteOptionsBuilder to work with an interface LocationProvider which can be implemented by other classes.

[tomaszrybakiewicz]

Mixing builder and configuration blocks results in an inconsistent api usage which IMHO looks a bit messy.

val routeOptions = mapboxNavigation.buildRouteOptions { builder ->
                builder
                    .fromCurrentLocation()           // builder 
                    .toDestination(destination)      // builder
                    .profileDriving {                            
                        maxWidth(1.42)              // configuration block
                        maxHeight(2.2)              // configuration block
                    }
            }

Maybe we should stick to either builders or configuration blocks, not both.

[VysotskiVadim]

I see what you mean, the API would look less messy if we used one of two approaches. I don't want to get rig of the step builder for now, because it makes the API safe. Anyway let's discuss the step bulder in the different answer, maybe we will decide to get rid of it, who knows 🙂

@tomaszrybakiewicz , would you like the API more if I replaced configuration block by builder?

val routeOptions = mapboxNavigation.buildRouteOptions { builder ->
                builder
                    .fromCurrentLocation()           
                    .toDestination(destination)      
                    .profileDriving { drivingProfileConfig ->                            
                        drivingProfileConfig
                            .maxWidth(1.42)
                            .maxHeight(2.2)             
                    }
            }

[tomaszrybakiewicz]

Still looks awkward. But at least it's consistent with buildRouteOptions.

[VysotskiVadim]

Thank you for feedback. I understand what you feel and I tend to agree with you. I will think about other way to structure API. 👍
But I think that current API is okay for the first iteration. As the feature is experimental, I will be able to change API in the future.

[tomaszrybakiewicz]

I think this API design introduces a lot of rigidity. Builders are meant to be flexible and allow developers to call any setter method in any order before calling build().

Building route options like will work:

val routeOptions = mapboxNavigation.buildRouteOptions { builder ->
    builder
        .fromCurrentLocation()
        .toDestination(destination)
        .profileDriving {
            maxWidth(1.42)
            maxHeight(2.2)
        }        
}

But like this won't work:

val routeOptions = mapboxNavigation.buildRouteOptions { builder ->
    builder
        .profileDriving {
            maxWidth(1.42)
            maxHeight(2.2)
        }
        .toDestination(destination)
        .fromCurrentLocation()
}

Also it won't allow developers to re-use of RouteOptions Builder between calls.

[VysotskiVadim]

Builders are meant to be flexible and allow developers to call any setter method in any order before calling build()

The builder pattern is just a pattern, i.e. I think it meant to be as the majority of developers think it is 🙂
And I've seen recently a description of a step builder pattern in AutoValue docs, which I think is quite popular. So I can't agree with "Builders are meant to be flexible" for now.

I agree that the "step builder" pattern doesn't allow re-use the builder between calls, because it changes the interface on each call.

I see a tradeoff here. From one side of the tradeoof is flexibility, from the other side is safety. The "step builder" pattern ensures that all the required fields are filled in. With current approach we get safety sacrificing flexibility.

@tomaszrybakiewicz, what do you think about it? Do you think it better to pick flexibility over safety?

[tomaszrybakiewicz]

Both are not mutually exclusive. You can offer a flexible API to developers and enforce its safety on runtime via assertions/preconditions. Lots of libraries does that. Take firebase-android-sdk for example. Even though they use @NonNull annotations and explicitly mention argument non nullability requirement in documentation, they still use preconditions to validate assumptions and enforce correct api usage.

[VysotskiVadim]

This discussion is very similar to the discussion above.
I will try different API formats before making this API not experimental, added note for myself and I will create an issue once PR is merged. But I think results of the first iteration is good enough to be merged.
Anyway feel free to say that the API is too bad to be merged. I will try to change it in the context of this PR if you think so.

[tomaszrybakiewicz]

I don't think we should be starting trip session here.
Doings so will introduce a side effect that will lead to unexpected behaviour and very hard to detect bugs.

[VysotskiVadim]

@tomaszrybakiewicz , what do you think we should do? Throw an exceptions if trip session hasn't been started yet?

[LukasPaczos]

I agree with @tomaszrybakiewicz. As things stand, the session would never be started by itself, it's always initiated by developers, especially since it has billing consequences, next to functional consequences like starting all the observers.

Not only that but also the fact that we are not stopping the session that's started here, nor are we monitoring if it's been changed by developer which could result in the buildRouteOptions never returning because it would get stuck in LocationFromTripSessionProvider.

Let's start small and require the trip session to be started before someone calls buildRouteOptions or the proposed above MapboxNavigation#requestRoutes(NavRouteOptionsBuilder). We'll still need to register a TripSessionStateObserver in the LocationFromTripSessionProvider to abort if the session was stopped before we got a location sample.

This will lest us experiment with the ergonomics without being blocked. As a next step, we can consider looking for a way to either bypass or initialize the session more ergonomically/silently when trying to obtain the location sample.

[VysotskiVadim]

Now I just throw an exception if trip session isn't started.

[VysotskiVadim]

We'll still need to register a TripSessionStateObserver in the LocationFromTripSessionProvider to abort if the session was stopped before we got a location sample.

Added timeout for waining for location update. it's easier and handles more cases

[tomaszrybakiewicz]

Builder a suspend method? I understand it's because of locationProvider.getCurrentLocation() but I don't think builders should ever be suspend methods.

To eliminate the need to call getCurrentLocation(), we could set a placeholder object that will tell MapboxNavigation to use current location when requestRoutes() is called.

eg. placeholder object could look like this

data class Waypoint(
    val coordinate: Point,
    val name: String?,
    val bearing: Double?,
    val zLevel: Int?,
    val isSilent: Boolean = false,
    val targetCoordinate: Point?
) {
    companion object {
        val CURRENT_LOCATION = Waypoint(
            coordinate = Point.fromLngLat(Double.NaN, Double.NaN),
            name = "CURRENT_LOCATION",
            bearing = null,
            zLevel = null,
            isSilent = false,
            targetCoordinate = null
        )
    }
}

The current location retrieval could be customized by additionally passing LocationProvider to RouteOptions and updating either MapboxNavigation.requestRoutes() or MapboxDirectionsSession.requestRoutes() to launch coroutine and acquiring waypoint location before forwarding it to the router.

[VysotskiVadim]

I don't think builders should ever be suspend methods.

Why do you think so? Is there any unexpected consequences I can get violating "build should be synchronous" rule?

[VysotskiVadim]

Just a note. I don't like current API in terms of asynchronously. Currently, the API makes a user to asynchronously build a RouteOptions object first and then request again asynchronously a route. It would be a callback hell without coroutines 🙂
I don't know good solution for this right now, so I decided to leave it for the next iterations.

[LukasPaczos]

I'd like to propose a simple solution for this problem - expose a new MapboxNavigation#requestRoutes(NavRouteOptionsBuilder) which will build the options and request a route in one asynchronous call.

We should still leave the buildRouteOptions public for developers that would like to mutate the predefined options but the basic flow would be not to build the options manually.

[VysotskiVadim]

@LukasPaczos , I agree thatMapboxNavigation#requestRoutes(NavRouteOptionsBuilder) is the best option. The only reason I didn't do that is that because it requires a tricky cancellation implementation. I wanted the first iteration to be shorter and postponed cancellation to the next iterations.

But actually, cancellation won't be complex if MapboxNavigation#requestRoutes(NavRouteOptionsBuilder) will be a suspend function. Will try it.

[VysotskiVadim]

Implemented MapboxNavigation#requestRoutes(NavRouteOptionsBuilder) as suspend function

[VysotskiVadim]

@tomaszrybakiewicz , @LukasPaczos , thank you for feedback. I fixed or replied to your suggestions. In general I think that PR is good enough to be merged, but I need a few more iterations before new API can become not experimental. If you don't have strong opinion about some of the new changes and approve it, I will create tickets for the next iterations based on list in the PR description, see "Do in the context of the following tickets".

[LukasPaczos]

Then is the question of compatibility, we also need to define a clear path for developers to start with higher-level API and transition to RouteOptiosnBuilder to tweak a parameter that wasn't exposed via NavRouteOptionsBuilder, maybe additional extension that works directly on RouteOptiosnBuilder or something along these lines.

Did you have a chance to think about this? This might be an important element for a transition/experimental period for developers who would like to try the API which doesn't yet offer everything.

[LukasPaczos]

Suggested change

	): RequestRoutesResult {
	): Expected<RequestRoutesFailure, RequestRoutesSuccess> {

Let's keep it consistent with other APIs that are in the Nav SDK.

[LukasPaczos]

Suggested change

	interface NoWaypointsOptionsBuilder {
	sealed interface NoWaypointsOptionsBuilder {

Same everywhere below.

[LukasPaczos]

This shouldn't throw but rather return an error message.

[LukasPaczos]

Suggested change

	zLevel: Int?
	zLevel: Int? = null,

[LukasPaczos]

We cannot define interfaces with default arguments because we won't be able to extend the parameter set for Java consumers. Adding a parameter to an interface function will always be a breaking change since @JvmOverloads is not available in interfaces.

Unfortunately it looks like we'll need to split NavRouteOptionsBuilder into 3 separate classes that pass the data between each other in a chain, unless you have other ideas?

[LukasPaczos]

If we can do that, it might actually make the experience more consistent with the rest of the build methods that do use functional extensions as arguments.

[RingerJK]

Suggested change

	fun fromStartLocation(
	point: Point,
	bearing: Double? = null,
	zLevel: Int?
	): WaypointsInProgressBuilder
	fun fromStartLocation(
	navigationLocation: NavigationLocation,
	): WaypointsInProgressBuilder
	class NavigationLocation private constrictor(
	val point: Point,
	val bearing: Double? = null,
	val zLevel: Int?
	){
	Builder...
	}

This is more flexible and covers java devs needs. Suppose we must use wrappers for all methods

[RingerJK]

where are we here? @VysotskiVadim

[VysotskiVadim]

@RingerJK , I don't work on it anymore because there is always something more important than this. Do you think I should reconsider priority?