mapbox-navigation-android
mapbox-navigation-android copied to clipboard
Merge guidance view sub-type components into a single one
Refs https://github.com/mapbox/mapbox-navigation-android/pull/6239#pullrequestreview-1091711679.
This ticket tracks deprecating the MapboxSignboardApi/View
, MapboxJunctionApi/View
, and MapboxSapaApi/View
(if introduced) classes, in favor of a MapboxImageGuidanceApi
and MapboxImageGuidanceView
(or something along these lines) that would expose distinct methods to download any of the sub-types and reuse the rest of the setup.
cc @abhishek1508 @kmadsen @tomaszrybakiewicz @cafesilencio
I brought this up here https://github.com/mapbox/navigation-sdks/issues/1412#issuecomment-1187719221
I would say that we should still have the API(s) and Views individually, but have one Processor, Action and Result as the major chunk of business logic is in these classes. All these API(s) can then make use of this Processor/Action/Result internally thereby reducing LOCs and still bifurcate features into there separate APIs. This will make it easy to manage the constructors as all the API(s) have different requirements in terms of styling and options without the need to introduce nullability for one or the other feature
@LukasPaczos
Thanks @abhishek1508, I somehow missed that comment focusing on the data side of things.
Let's copy the arguments here and continue!
From @cafesilencio: I disagree. These are completely different things. As with every API the developer uses the ones they need. There's too much an assumption these are all implemented the same. As these features expand over time the single class will get bloated and overly complex. Also conceptually a signboard and CHM are completely different things. If you want to create a facade that gives the appearance these are all the same it could be done as a compromise but to me these API's conceptually aren't more related that any other API's.
From @abhishek1508:
As with every API the developer uses the ones they need. There's too much an assumption these are all implemented the same.
I don't agree that there is an assumption that these are all implemented the same but rather a fact although CHM can be a bit of an exception. CHM map would still be rendered the same way with the difference being overlaying a puck. All these features have or will have the same Mapbox-Agent
that is required to make a network request and have the same return type.
However, now looking at MapboxSignboardAPI
in details I see the following:
class MapboxSignboardApi @JvmOverloads constructor(
private val accessToken: String,
private val parser: SvgToBitmapParser,
private val options: MapboxSignboardOptions = MapboxSignboardOptions.Builder().build()
)
which we would have to account for in this single MapboxVisualGuidanceAPI
which might make it a bit messy as MapboxSignboardOptions
will not be applicable for MapboxJunctionAPI
.
So taking back my suggestion in favor of individual API(s).
From @cafesilencio: I think a better approach might be to abstract anything that's common in order to reuse code and limit duplication.
From @abhishek1508:
Yeah, we have already abstracted out the common logic between these features and that is making the network request to fetch the image. Now it's super simple and we just need to use ResourceLoadRequest
. The logic to convert SVG to Bitmap is also abstracted out.
From my side I'd like to add that these features certainly are related and that's enforced by the Navigation APIs - they all come as URIs under a guidance view banner type. This already limits our options and possible implementations.
I think that a single class with functions that take options
as parameters instead of options
being static in constructor would make sense, especially with options
including things like width
or style
which can be mutable and dynamic. The constructor injection for them doesn't seem like a good choice anyway.