Added flag to ignore route line updates in the view under certain con…

[cafesilencio]

Description

mapbox/mapbox-navigation-android#6742 Introduced a feature to display the active route leg above inactive route legs. A customer found a case in their automated testing that showed an incorrect vanishing point when the route first began after upgrading to the release that included this feature. The customer indicated the issue wasn't observed during real drive testing.

In order to keep from breaking SEMVER the class used for updating the masking layers required a non-null primary route line data object. The implementation in PR 6742 made a call to get the primary route line data and include it which meant re-rendering the primary route line in the view on route progress. This new behavior seems to be involved in the erroneous behavior reported. Since the use case in question doesn't need to update the primary route line data and it was only included because the parameter is non-null the re-rendering was removed by adding a boolean flag indicating if the rendering of the primary route line data should occur. Adding this results in the primary route line data not being rendered in route line progress updates as was the case before the feature described in 6742 was implemented.

Screenshots or Gifs

[github-actions]

Changelog

Features

Bug fixes and improvements

• Fixed route progress vanishing point update issue introduced by feature that displays the active leg of the route line above inactive legs for multi-leg routes. #6974 [#6974](https://github.com/mapbox/mapbox-navigation-android/pull/6974)
• Fixed memory consumption issue on onboard router request cancellation. [#7029](https://github.com/mapbox/mapbox-navigation-android/pull/7029)

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

• Fixed PlacesListOnMapScreen issues when screen changed cause places to disappear [#7021](https://github.com/mapbox/mapbox-navigation-android/pull/7021)
• Fixed an issue that caused route preview screens to display incorrect route distance. [#7031](https://github.com/mapbox/mapbox-navigation-android/pull/7031)

[codecov]

Codecov Report

Merging #6974 (9447ef5) into main (251b7eb) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head 9447ef5 differs from pull request most recent head ce60430. Consider uploading reports for the commit ce60430 to get more accurate results

@@             Coverage Diff              @@
##               main    #6974      +/-   ##
============================================
- Coverage     73.49%   73.26%   -0.23%     
+ Complexity     5796     5730      -66     
============================================
  Files           799      789      -10     
  Lines         31205    30971     -234     
  Branches       3670     3658      -12     
============================================
- Hits          22933    22691     -242     
- Misses         6798     6805       +7     
- Partials       1474     1475       +1     

Impacted Files	Coverage Δ
...ation/ui/maps/route/line/api/MapboxRouteLineApi.kt	83.71% <100.00%> (+0.07%)	⬆️
...tion/ui/maps/route/line/api/MapboxRouteLineView.kt	87.96% <100.00%> (+0.01%)	⬆️
...n/ui/maps/route/line/model/RouteLineUpdateValue.kt	58.53% <100.00%> (+7.10%)	⬆️

... and 22 files with indirect coverage changes

[dzinad]

This can be dangerous:

1. We set this flag to true;
2. We pass RouteLineUpdateValue to the client via consumer;
3. They do something in their consumer which results in passing us not the original object but its copy (toMutableValue/toImmutableValue);
4. We lose the flag.

It sounds artificial but who knows. I think we should transfer the flag value between mutable/immutable (we can do that and it seems like the only way the customer can modify the object).

[cafesilencio]

What you point out is true. However, before beta.3 this data was never returned in this code execution path. The only reason this was introduced was because the masking layers were added in this release and they had to be created and rendered here. The main route line data is included ONLY because the field is non-nullable. If the field had been nullable the main route line parameter would have been null. So even in the possibility you outline the data shouldn't be rendered and wouldn't be if the field had been nullable or could be changed to nullable (without breaking SEMVER).

The flag is introduced solely as a workaround for not being able to exclude the main route line data in a more elegant way and to get this method back to the state it was in prior to beta.3.

[dzinad]

The flag is introduced solely as a workaround for not being able to exclude the main route line data in a more elegant way and to get this method back to the state it was in prior to beta.3.

I get that, no objections to that.
What I mean is imagine the situation:

    private val routeProgressObserver = RouteProgressObserver { routeProgress ->
        routeLineApi.updateWithRouteProgress(routeProgress) { result ->
            val modifiedResult = result.onValue { it.toMutableValue()/** some changes **/.toImmutableValue() }
            mapboxMap.getStyle()?.apply {
                routeLineView.renderRouteLineUpdate(this, modifiedResult)
            }
        }
    }    

This is possible, right? It doesn't have anything to do with beta.3. But with these modifications we lose the flag. Which I think shouldn't be the case. If we add a new property to the model class (even though it's internal), toMutableValue + toImmutableValue should keep it.

[cafesilencio]

Yes good point, added.

[dzinad]

NIT: I'd specify default value only once as a constant.