Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 16, 2023
+186 −13
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/unreleased/bugfixes/6974.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- 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](https://github.com/mapbox/mapbox-navigation-android/pull/6974)
19 changes: 14 additions & 5 deletions ...vui-maps/src/main/java/com/mapbox/navigation/ui/maps/route/line/api/MapboxRouteLineApi.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,20 +799,29 @@ class MapboxRouteLineApi(
}
}

// The purpose of this is to provide the correct masking layer data to be rendered in the view.
// Although the primary route line layer data is returned here it shouldn't be rendered by the
// view. It is included because the parameter isn't nullable. If it were the data wouldn't
// be included since it's only the masking layers that should be updated. For this reason
// the variable to ignore the primary route line data is set to true so the view class will
// ignore it.
private fun provideRouteLegUpdate(
routeLineOverlayDynamicData: RouteLineDynamicData?,
consumer: MapboxNavigationConsumer<Expected<RouteLineError, RouteLineUpdateValue>>
) {
ifNonNull(routeLineOverlayDynamicData) { maskingLayerData ->
getRouteDrawData { expected ->
expected.value?.apply {
val updateValue = RouteLineUpdateValue(
this.primaryRouteLineData.dynamicData,
this.alternativeRouteLinesData.map { it.dynamicData },
maskingLayerData
).also {
it.ignorePrimaryRouteLineData = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point, added.

}
consumer.accept(
ExpectedFactory.createValue<RouteLineError, RouteLineUpdateValue>(
RouteLineUpdateValue(
this.primaryRouteLineData.dynamicData,
this.alternativeRouteLinesData.map { it.dynamicData },
maskingLayerData
)
updateValue
)
)
}
Expand Down
10 changes: 6 additions & 4 deletions ...ui-maps/src/main/java/com/mapbox/navigation/ui/maps/route/line/api/MapboxRouteLineView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,10 +370,12 @@ class MapboxRouteLineView(options: MapboxRouteLineOptions) {
jobControl.scope.launch(Dispatchers.Main) {
mutex.withLock {
update.onValue {
primaryRouteLineLayerGroup.map { layerId ->
toExpressionUpdateFun(layerId, it.primaryRouteLineDynamicData)
}.forEach { updateFun ->
updateFun(style)
if (!it.ignorePrimaryRouteLineData) {
primaryRouteLineLayerGroup.map { layerId ->
toExpressionUpdateFun(layerId, it.primaryRouteLineDynamicData)
}.forEach { updateFun ->
updateFun(style)
}
}

ifNonNull(it.routeLineMaskingLayerDynamicData) { overlayData ->
Expand Down
12 changes: 10 additions & 2 deletions ...maps/src/main/java/com/mapbox/navigation/ui/maps/route/line/model/RouteLineUpdateValue.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,18 @@ class RouteLineUpdateValue internal constructor(
val routeLineMaskingLayerDynamicData: RouteLineDynamicData? = null
) {

internal var ignorePrimaryRouteLineData = false

/**
* @return a class with mutable values for replacing.
*/
fun toMutableValue() = MutableRouteLineUpdateValue(
primaryRouteLineDynamicData,
alternativeRouteLinesDynamicData,
routeLineMaskingLayerDynamicData
)
).also {
it.ignorePrimaryRouteLineData = ignorePrimaryRouteLineData
}

/**
* Represents the mutable data for updating the appearance of the route lines.
Expand All @@ -35,14 +39,18 @@ class RouteLineUpdateValue internal constructor(
var routeLineMaskingLayerDynamicData: RouteLineDynamicData? = null
) {

internal var ignorePrimaryRouteLineData = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* @return a RouteLineUpdateValue
*/
fun toImmutableValue() = RouteLineUpdateValue(
primaryRouteLineDynamicData,
alternativeRouteLinesDynamicData,
routeLineMaskingLayerDynamicData
)
).also {
it.ignorePrimaryRouteLineData = ignorePrimaryRouteLineData
}
}

/**
Expand Down
1 change: 1 addition & 0 deletions ...maps/src/test/java/com/mapbox/navigation/ui/maps/route/line/api/MapboxRouteLineApiTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,7 @@ class MapboxRouteLineApiTest {
checkExpression(expectedCasingExpressionContents, casingExpression)
checkExpression(expectedTrailExpressionContents, trailExpression)
checkExpression(expectedTrailCasingExpressionContents, trailCasingExpression)
assertTrue(result.ignorePrimaryRouteLineData)

callbackCalled = true
}
Expand Down
145 changes: 145 additions & 0 deletions ...aps/src/test/java/com/mapbox/navigation/ui/maps/route/line/api/MapboxRouteLineViewTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,151 @@ class MapboxRouteLineViewTest {
unmockkStatic("com.mapbox.maps.extension.style.layers.LayerUtils")
}

@Test
fun renderTraveledRouteLineUpdate_whenIgnorePrimaryRouteLineData() =
coroutineRule.runBlockingTest {
mockkStatic("com.mapbox.maps.extension.style.layers.LayerUtils")
mockkObject(MapboxRouteLineUtils)
val options = MapboxRouteLineOptions.Builder(ctx).build()
val trafficLineExp = mockk<Expression>()
val routeLineExp = mockk<Expression>()
val casingLineEx = mockk<Expression>()
val restrictedRoadExp = mockk<Expression>()
val trailExpression = mockk<Expression>()
val trailCasingExpression = mockk<Expression>()
val state: Expected<RouteLineError, RouteLineUpdateValue> =
ExpectedFactory.createValue(
RouteLineUpdateValue(
primaryRouteLineDynamicData = RouteLineDynamicData(
{ routeLineExp },
{ casingLineEx },
{ trafficLineExp },
{ restrictedRoadExp },
trailExpressionProvider = { trailExpression },
trailCasingExpressionProvider = { trailCasingExpression }
),
alternativeRouteLinesDynamicData = listOf(
RouteLineDynamicData(
{ throw UnsupportedOperationException() },
{ throw UnsupportedOperationException() },
{ throw UnsupportedOperationException() },
{ throw UnsupportedOperationException() }
),
RouteLineDynamicData(
{ throw UnsupportedOperationException() },
{ throw UnsupportedOperationException() },
{ throw UnsupportedOperationException() },
{ throw UnsupportedOperationException() }
)
),
routeLineMaskingLayerDynamicData = RouteLineDynamicData(
{ routeLineExp },
{ casingLineEx },
{ trafficLineExp },
{ restrictedRoadExp },
trailExpressionProvider = { trailExpression },
trailCasingExpressionProvider = { trailCasingExpression }
)
).also {
it.ignorePrimaryRouteLineData = true
}
)
val style = mockk<Style> {
every {
setStyleLayerProperty(any(), any(), any())
} returns ExpectedFactory.createNone()
}.also {
mockCheckForLayerInitialization(it)
}

pauseDispatcher {
val view = MapboxRouteLineView(options)
view.initPrimaryRouteLineLayerGroup(MapboxRouteLineUtils.layerGroup1SourceLayerIds)
view.renderRouteLineUpdate(style, state)
}

verify(exactly = 0) {
style.setStyleLayerProperty(
LAYER_GROUP_1_TRAFFIC,
"line-gradient",
trafficLineExp
)
}
verify(exactly = 0) {
style.setStyleLayerProperty(
LAYER_GROUP_1_MAIN,
"line-gradient",
routeLineExp
)
}
verify(exactly = 0) {
style.setStyleLayerProperty(
LAYER_GROUP_1_CASING,
"line-gradient",
casingLineEx
)
}
verify(exactly = 0) {
style.setStyleLayerProperty(
LAYER_GROUP_1_RESTRICTED,
"line-gradient",
restrictedRoadExp
)
}
verify(exactly = 0) {
style.setStyleLayerProperty(
LAYER_GROUP_1_TRAIL,
"line-gradient",
trailExpression
)
}
verify(exactly = 0) {
style.setStyleLayerProperty(
LAYER_GROUP_1_TRAIL_CASING,
"line-gradient",
trailCasingExpression
)
}
verify {
style.setStyleLayerProperty(
MASKING_LAYER_TRAFFIC,
"line-gradient",
trafficLineExp
)
}
verify {
style.setStyleLayerProperty(
MASKING_LAYER_MAIN,
"line-gradient",
routeLineExp
)
}
verify {
style.setStyleLayerProperty(
MASKING_LAYER_CASING,
"line-gradient",
casingLineEx
)
}
verify {
style.setStyleLayerProperty(
MASKING_LAYER_TRAIL,
"line-gradient",
trailExpression
)
}
verify {
style.setStyleLayerProperty(
MASKING_LAYER_TRAIL_CASING,
"line-gradient",
trailCasingExpression
)
}

unmockkObject(MapboxRouteLineUtils)
unmockkStatic("com.mapbox.maps.extension.style.layers.LayerUtils")
}

@Test
fun renderTraveledRouteLineTrimUpdate() = coroutineRule.runBlockingTest {
mockkStatic("com.mapbox.maps.extension.style.layers.LayerUtils")
Expand Down
11 changes: 9 additions & 2 deletions .../src/test/java/com/mapbox/navigation/ui/maps/route/line/model/RouteLineUpdateValueTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.mapbox.navigation.ui.maps.route.line.model

import io.mockk.mockk
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test

class RouteLineUpdateValueTest {
Expand All @@ -12,7 +13,9 @@ class RouteLineUpdateValueTest {
RouteLineDynamicData(mockk(), mockk(), mockk(), mockk()),
listOf(),
mockk()
)
).also {
it.ignorePrimaryRouteLineData = true
}

val result = original.toMutableValue()

Expand All @@ -25,6 +28,7 @@ class RouteLineUpdateValueTest {
original.routeLineMaskingLayerDynamicData,
result.routeLineMaskingLayerDynamicData
)
assertTrue(result.ignorePrimaryRouteLineData)
}

@Test
Expand All @@ -33,7 +37,9 @@ class RouteLineUpdateValueTest {
RouteLineDynamicData(mockk(), mockk(), mockk(), mockk()),
listOf(),
mockk()
)
).also {
it.ignorePrimaryRouteLineData = true
}
val replacementPrimaryRouteLineDynamicData =
RouteLineDynamicData(mockk(), mockk(), mockk(), mockk())
val replacementList = listOf<RouteLineDynamicData>()
Expand All @@ -48,5 +54,6 @@ class RouteLineUpdateValueTest {
assertEquals(replacementPrimaryRouteLineDynamicData, result.primaryRouteLineDynamicData)
assertEquals(replacementList, result.alternativeRouteLinesDynamicData)
assertEquals(replacementMaskingData, result.routeLineMaskingLayerDynamicData)
assertTrue(result.ignorePrimaryRouteLineData)
}
}