Resets route arrow related layers when initialization is done.

[cafesilencio]

Description

When layers are initialized via the MapboxRouteArrowView and MapboxRouteLineView the layers are reset and removed so that they can be reinitialized with the options that may have change so that the layer configuration can change. Previously once the layers were initialized in the map style there was no way to adjust them.

Screenshots or Gifs

[abhishek1508]

We should follow up with an example showing how to do this in our examples repository app module as well as test it in one of the QA activities.

[abhishek1508]

I am assuming we don't need to invoke this function and that it will be invoked internally via render call. If my understanding is correct then we should make it private.

If not, then is the desired use to always call initializeLayers first before calling render(style: Style, expectedValue: Expected<InvalidPointError, ArrowAddedValue>)

[abhishek1508]

If this is meant to be public it would also need appropriate documentation.

[cafesilencio]

The function is an explicit call to initialize the layers if a developer chooses to use it. If a developer wants change the options then they should instantiate a MapboxRouteArrowView with those options and call initializeLayers.

In a more typical usage of MapboxRouteArrowView it's not necessary to call initializeLayers. I'll add the kdoc.

[abhishek1508]

We have nicely created fields that aresetOf different layerIds like MapboxRouteLineView.trailCasingLayerIds and others. We can move these fields as internal to RouteLayerContants and reuse them both in MapboxRouteLineView as well as in the above function call using forEach

fun removeLayers(style: Style) {
    RouteLayerConstants.trailCasingLayerIds.forEach { 
        style.removeStyleImage(it)
    }
    ....
    ....
}

[cafesilencio]

Those collections have a specific function totally unrelated to the code removing the layers.

[LukasPaczos]

@abhishek1508 idea leads in a good direction though - is there a way to build a single collection that we can iterate over both when adding and removing layers? Otherwise it's easy to make a mistake when a new layer is added to the list, we'd have to remember to write a removal unit test as well, there's no automation that would detect the mistake for us.

[tomaszrybakiewicz]

Here's an idea. According to Map Style Specification, Layers have an optional metadata property which can hold any arbitrary data. Why not leverage that and add a "layer-group": "arrows" property which we could use to quickly identify which layers needs to be removed together?

[codecov]

Codecov Report

Merging #6466 (05d3553) into main (0077240) will decrease coverage by 0.01%.
The diff coverage is 88.23%.

❗ Current head 05d3553 differs from pull request most recent head fea8962. Consider uploading reports for the commit fea8962 to get more accurate results

@@             Coverage Diff              @@
##               main    #6466      +/-   ##
============================================
- Coverage     72.51%   72.50%   -0.02%     
+ Complexity     5540     5532       -8     
============================================
  Files           777      774       -3     
  Lines         30038    29960      -78     
  Branches       3546     3540       -6     
============================================
- Hits          21782    21722      -60     
+ Misses         6832     6815      -17     
+ Partials       1424     1423       -1     

Impacted Files	Coverage Δ
.../navigation/ui/maps/route/arrow/RouteArrowUtils.kt	89.32% <ø> (ø)
...ation/ui/maps/route/line/api/VanishingRouteLine.kt	87.12% <ø> (ø)
...tion/ui/maps/route/line/api/MapboxRouteLineView.kt	88.03% <66.66%> (-0.18%)	⬇️
...on/ui/maps/route/arrow/api/MapboxRouteArrowView.kt	83.95% <92.30%> (+0.38%)	⬆️
...igation/ui/maps/internal/ui/RouteArrowComponent.kt	80.95% <100.00%> (ø)
...navigation/ui/speedlimit/api/MapboxSpeedInfoApi.kt	78.57% <0.00%> (-14.29%)	⬇️
...vigation/ui/speedlimit/view/MapboxSpeedInfoView.kt	63.91% <0.00%> (-0.77%)	⬇️
...tion/ui/speedlimit/model/MapboxSpeedInfoOptions.kt	96.49% <0.00%> (-0.53%)	⬇️
...mapbox/navigation/dropin/ViewStyleCustomization.kt	100.00% <0.00%> (ø)
...box/navigation/ui/speedlimit/ComponentInstaller.kt	0.00% <0.00%> (ø)
... and 8 more

[LukasPaczos]

Is the proposed behavior that if I create the first instance of route line components and start drawing I don't need to do anything, but if a create a second instance that has different options, then I have to call initializeLayers explicitly? I think this offers rather poor developer experience. Lifecycle of the route line components in the app cannot be independent anymore, developers would have to maintain a flag of whether they already used the route line components or not, etc.

Have we tried actually comparing the map layers' state with the options? Why don't we re-apply the options on each draw call and ignore initializeLayers at all during progress updates? How expensive would that be to make the developer experience actually seamless?

[cafesilencio]

Why don't we re-apply the options on each draw call and ignore initializeLayers at all during progress updates?

I"m not exactly sure what you mean by this. The options are for the layers, the colors, the arrow head drawable etc. Normally the only interaction with the class is by calling the render methods which will check if the layers are present and if not initialize them. I don't want to remove and re-add the layers on each render call. That would mean recreating the layers on each route progress update. I"m trying to be mindful of this ticket here: #5806 and not degrade the performance.

I"ll look for other options but can you elaborate on what you had in mind?

[abhishek1508]

I am assuming we don't need to invoke this function and that it will be invoked internally via render call

I had the same question and that's why I asked the above. Currently we do not invoke initializeLayers at all when using RouteLine and RouteArrow. All we do is get the expected from RouteLineApi and invoke render using the expected. With the changes in this PR, now it seems we would have to invoke initializeLayers first to apply the options followed by render. This is undesired as now we need to keep track of the APIs and make an additional call to apply options.

I am not aware of a different solution, but just speaking from customer perspective.

[LukasPaczos]

I"m not exactly sure what you mean by this. The options are for the layers, the colors, the arrow head drawable etc. [...] hat would mean recreating the layers on each route progress update.

There are certainly ways we could try optimizing this. For example, we don't need to recreate layers when they already exist, we only need to re-apply the options that have changed or are different when comparing the options and the layer instance. Also, since options are only provided in a constructor, we could always update or create the layers on the first render call (to apply potentially modified options) and then avoid any additional initializeLayers calls.

[cafesilencio]

I did some testing and made the options mutable. They can be updated at any time and the changes applied to the layers.

[LukasPaczos]

The points from #6466 (comment) still stand - we require developers to interact with standalone instances of the views differently depending if they are the first one or not.

For example

val instance1 = MapboxRouteArrowView(options1)
instance1.render(update)
val instance2 = MapboxRouteArrowView(options2)
instance2.render(update)

the options2 will not be applied, we'd have to explicitly call instance2.update (instance1.update for that matter). I'd think this might be hard to understand by a developer. Could we instead re-apply the immutable options on each first render call of a view instance?

[cafesilencio]

If there's a method to update the options why would a developer have more than one instance of MapboxRouteArrowView? There's no benefit. I've experimented with changing the options in the layers rather than removing and re-adding the layers. The tolerance option is part of the source. I think that's a read only property of the source.

[LukasPaczos]

If there's a method to update the options why would a developer have more than one instance of MapboxRouteArrowView? There's no benefit.

MapboxRouteArrowView is not a singleton, we can't expect and should not enforce there being only one instance for the whole process.

I've experimented with changing the options in the layers rather than removing and re-adding the layers. The tolerance option is part of the source. I think that's a read only property of the source.

Let's re-add the sources and layers on the first render call in this case, that sounds like it should work.

[cafesilencio]

@LukasPaczos I believe this has the changes we last talked about.