Fix ReplayRouteSession route state

kmadsen · kmadsen · commit 970bd392c686 · 2022-12-01T18:49:50.000-08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@ Mapbox welcomes participation and contributions from everyone.
 ## Unreleased
 #### Features
 #### Bug fixes and improvements
+- Fixed issues in `ReplayRouteSession`. The routes observer was never unregistered. Alternative route selection resets at the beginning. DropInUi changing Portrait and Landscape modes resets at the beginning. [#6675](https://github.com/mapbox/mapbox-navigation-android/pull/6675)
 
 ## Mapbox Navigation SDK 2.10.0-beta.2 - 01 December, 2022
 ### Changelog
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayPolylineDecodeStream.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayPolylineDecodeStream.kt
@@ -94,4 +94,21 @@ class ReplayPolylineDecodeStream(
         }
         return points
     }
+
+    /**
+     * Skip the next [minDistance] of the geometry. Similar to decode except that this does not
+     * hold any points in memory.
+     */
+    fun skip(
+        minDistance: Double,
+        @TurfConstants.TurfUnitCriteria units: String = TurfConstants.UNIT_KILOMETERS
+    ) {
+        var travelled = 0.0
+        current?.let { if (minDistance > 0.0 && hasNext()) next() }
+        while (travelled < minDistance && hasNext()) {
+            val previous = current
+            val next = next()
+            travelled += previous?.let { TurfMeasurement.distance(previous, next, units) } ?: 0.0
+        }
+    }
 }
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt
@@ -9,6 +9,7 @@ import com.mapbox.android.core.permissions.PermissionsManager
 import com.mapbox.api.directions.v5.DirectionsCriteria
 import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.base.route.NavigationRoute
+import com.mapbox.navigation.base.trip.model.RouteProgress
 import com.mapbox.navigation.core.MapboxNavigation
 import com.mapbox.navigation.core.directions.session.RoutesObserver
 import com.mapbox.navigation.core.lifecycle.MapboxNavigationApp
@@ -17,8 +18,10 @@ import com.mapbox.navigation.core.replay.MapboxReplayer
 import com.mapbox.navigation.core.replay.history.ReplayEventBase
 import com.mapbox.navigation.core.replay.history.ReplayEventUpdateLocation
 import com.mapbox.navigation.core.replay.history.ReplayEventsObserver
+import com.mapbox.navigation.core.trip.session.RouteProgressObserver
 import com.mapbox.navigation.utils.internal.logW
-import java.util.Collections
+import com.mapbox.turf.TurfConstants
+import java.util.*
 
 /**
  * Used to create a replay trip session. Continue to use [MapboxNavigation.setNavigationRoutes] to
@@ -58,11 +61,23 @@ class ReplayRouteSession : MapboxNavigationObserver {
     private lateinit var replayRouteMapper: ReplayRouteMapper
     private var mapboxNavigation: MapboxNavigation? = null
     private var lastLocationEvent: ReplayEventUpdateLocation? = null
-    private var routesObserver: RoutesObserver? = null
-    private var currentRouteId: String? = null
+    private var currentRoute: NavigationRoute? = null
+
+    private val routeProgressObserver = RouteProgressObserver { routeProgress ->
+        if (currentRoute != routeProgress.navigationRoute) {
+            currentRoute = routeProgress.navigationRoute
+            onRouteChanged(routeProgress)
+        }
+    }
+
+    private val routesObserver = RoutesObserver { result ->
+        if (result.navigationRoutes.isEmpty()) {
+            mapboxNavigation?.resetReplayLocation()
+        }
+    }
 
     private val replayEventsObserver = ReplayEventsObserver { events ->
-        if (isLastEventPlayed(events)) {
+        if (currentRoute != null && isLastEventPlayed(events)) {
             pushMorePoints()
         }
     }
@@ -92,17 +107,10 @@ class ReplayRouteSession : MapboxNavigationObserver {
         this.mapboxNavigation = mapboxNavigation
         mapboxNavigation.stopTripSession()
         mapboxNavigation.startReplayTripSession()
-
-        routesObserver = RoutesObserver { result ->
-            if (result.navigationRoutes.isEmpty()) {
-                currentRouteId = null
-                mapboxNavigation.resetReplayLocation()
-            } else if (result.navigationRoutes.first().id != currentRouteId) {
-                onRouteChanged(result.navigationRoutes.first())
-            }
-        }.also { mapboxNavigation.registerRoutesObserver(it) }
+        mapboxNavigation.registerRouteProgressObserver(routeProgressObserver)
+        mapboxNavigation.registerRoutesObserver(routesObserver)
         mapboxNavigation.mapboxReplayer.registerObserver(replayEventsObserver)
-        mapboxNavigation.resetReplayLocation()
+        mapboxNavigation.mapboxReplayer.play()
     }
 
     private fun MapboxNavigation.resetReplayLocation() {
@@ -123,14 +131,18 @@ class ReplayRouteSession : MapboxNavigationObserver {
     }
 
     override fun onDetached(mapboxNavigation: MapboxNavigation) {
-        this.mapboxNavigation = null
+        mapboxNavigation.unregisterRoutesObserver(routesObserver)
+        mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
         mapboxNavigation.mapboxReplayer.unregisterObserver(replayEventsObserver)
         mapboxNavigation.mapboxReplayer.stop()
         mapboxNavigation.mapboxReplayer.clearEvents()
         mapboxNavigation.stopTripSession()
+        this.mapboxNavigation = null
+        this.currentRoute = null
     }
 
-    private fun onRouteChanged(navigationRoute: NavigationRoute) {
+    private fun onRouteChanged(routeProgress: RouteProgress) {
+        val navigationRoute = routeProgress.navigationRoute
         val mapboxReplayer = mapboxNavigation?.mapboxReplayer ?: return
         mapboxReplayer.clearEvents()
         mapboxReplayer.play()
@@ -144,9 +156,9 @@ class ReplayRouteSession : MapboxNavigationObserver {
             }
             return
         }
-        currentRouteId = navigationRoute.id
+        val distanceTraveled = routeProgress.distanceTraveled.toDouble()
         polylineDecodeStream = ReplayPolylineDecodeStream(geometry, 6)
-        mapboxNavigation?.resetTripSession()
+        polylineDecodeStream.skip(distanceTraveled, TurfConstants.UNIT_METERS)
         pushMorePoints()
     }
 
diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt
@@ -10,14 +10,19 @@ import com.mapbox.geojson.Point
 import com.mapbox.geojson.utils.PolylineUtils
 import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.base.options.NavigationOptions
+import com.mapbox.navigation.base.route.NavigationRoute
+import com.mapbox.navigation.base.trip.model.RouteProgress
 import com.mapbox.navigation.core.MapboxNavigation
 import com.mapbox.navigation.core.directions.session.RoutesObserver
 import com.mapbox.navigation.core.directions.session.RoutesUpdatedResult
 import com.mapbox.navigation.core.replay.MapboxReplayer
 import com.mapbox.navigation.core.replay.history.ReplayEventBase
 import com.mapbox.navigation.core.replay.history.ReplayEventUpdateLocation
 import com.mapbox.navigation.core.replay.history.ReplayEventsObserver
+import com.mapbox.navigation.core.trip.session.RouteProgressObserver
 import com.mapbox.navigation.testing.LoggingFrontendTestRule
+import com.mapbox.turf.TurfConstants
+import com.mapbox.turf.TurfMeasurement
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
@@ -47,9 +52,13 @@ class ReplayRouteSessionTest {
     private val options: NavigationOptions = mockk {
         every { applicationContext } returns context
     }
+    private val routesObserver = slot<RoutesObserver>()
+    private val routeProgressObserver = slot<RouteProgressObserver>()
     private val mapboxNavigation: MapboxNavigation = mockk(relaxed = true) {
         every { mapboxReplayer } returns replayer
         every { navigationOptions } returns options
+        every { registerRoutesObserver(capture(routesObserver)) } just runs
+        every { registerRouteProgressObserver(capture(routeProgressObserver)) } just runs
     }
     private val bestLocationEngine: LocationEngine = mockk {
         every { getLastLocation(any()) } just runs
@@ -166,16 +175,14 @@ class ReplayRouteSessionTest {
 
     @Test
     fun `onAttached - should push the initial batch of events`() {
-        val routesObserver = slot<RoutesObserver>()
-        every { mapboxNavigation.registerRoutesObserver(capture(routesObserver)) } just runs
         sut.setOptions(
             ReplayRouteSessionOptions.Builder()
                 .decodeMinDistance(1.0)
                 .build()
         )
 
         sut.onAttached(mapboxNavigation)
-        routesObserver.captured.onRoutesChanged(mockActiveNavigationRoutes())
+        routeProgressObserver.captured.onRouteProgressChanged(mockRouteProgress(0.0))
 
         val pushedEvents = slot<List<ReplayEventBase>>()
         verify { replayer.pushEvents(capture(pushedEvents)) }
@@ -193,45 +200,34 @@ class ReplayRouteSessionTest {
             replayEventsObserver.captured.replayEvents(firstArg())
             replayer
         }
-        val routesUpdatedResult = mockActiveNavigationRoutes()
+        val routeProgress = mockRouteProgress(0.0)
 
         sut.setOptions(
             ReplayRouteSessionOptions.Builder()
                 .decodeMinDistance(0.001)
                 .build()
         )
         sut.onAttached(mapboxNavigation)
-        routesObserver.captured.onRoutesChanged(routesUpdatedResult)
+        routeProgressObserver.captured.onRouteProgressChanged(routeProgress)
 
         // Verify every point in the geometry was simulated
         val pushedPoints = pushedEvents.flatten().toList().map {
             val location = (it as ReplayEventUpdateLocation).location
             Point.fromLngLat(location.lon, location.lat)
         }
-        val geometry = routesUpdatedResult.navigationRoutes.first().directionsRoute.geometry()!!
+        val geometry = routeProgress.navigationRoute.directionsRoute.geometry()!!
         val geometryPoints = PolylineUtils.decode(geometry, 6)
-        assertTrue(pushedPoints.size > geometryPoints.size)
+        assertTrue(
+            "${pushedPoints.size} > ${geometryPoints.size}",
+            pushedPoints.size > geometryPoints.size
+        )
         assertTrue(
             geometryPoints.all { lhs ->
                 pushedPoints.firstOrNull { rhs -> lhs.equals(rhs) } != null
             }
         )
     }
 
-    @Test
-    fun `onAttached - should request gps location when resetLocationEnabled is true`() {
-        every { PermissionsManager.areLocationPermissionsGranted(any()) } returns true
-        sut.setOptions(ReplayRouteSessionOptions.Builder().locationResetEnabled(true).build())
-        sut.onAttached(mapboxNavigation)
-
-        verifyOrder {
-            mapboxNavigation.stopTripSession()
-            mapboxNavigation.startReplayTripSession()
-            bestLocationEngine.getLastLocation(any())
-            replayer.play()
-        }
-    }
-
     @Test
     fun `onAttached - should push gps location when route is not set`() {
         val locationCallbackSlot = slot<LocationEngineCallback<LocationEngineResult>>()
@@ -242,6 +238,9 @@ class ReplayRouteSessionTest {
 
         sut.setOptions(ReplayRouteSessionOptions.Builder().locationResetEnabled(true).build())
         sut.onAttached(mapboxNavigation)
+        routesObserver.captured.onRoutesChanged(
+            mockk { every { navigationRoutes } returns emptyList() }
+        )
         locationCallbackSlot.captured.onSuccess(
             mockk {
                 every { lastLocation } returns mockk(relaxed = true) {
@@ -263,21 +262,115 @@ class ReplayRouteSessionTest {
         assertEquals(-2.0, capturedLocation.location.lon, 0.0)
     }
 
-    private fun mockActiveNavigationRoutes(): RoutesUpdatedResult = mockk {
+    @Test
+    fun `onAttached registered listeners should be unregistered onDetached`() {
+        val progressObserver = slot<RouteProgressObserver>()
+        val routesObserver = slot<RoutesObserver>()
+        val replayEventsObserver = slot<ReplayEventsObserver>()
+        every { mapboxNavigation.registerRoutesObserver(capture(routesObserver)) } just runs
+        every {
+            mapboxNavigation.registerRouteProgressObserver(capture(progressObserver))
+        } just runs
+        every { replayer.registerObserver(capture(replayEventsObserver)) } just runs
+
+        sut.onAttached(mapboxNavigation)
+        sut.onDetached(mapboxNavigation)
+
+        verifyOrder {
+            mapboxNavigation.registerRouteProgressObserver(any())
+            mapboxNavigation.registerRoutesObserver(any())
+            replayer.registerObserver(any())
+            mapboxNavigation.unregisterRoutesObserver(routesObserver.captured)
+            mapboxNavigation.unregisterRouteProgressObserver(progressObserver.captured)
+            replayer.unregisterObserver(replayEventsObserver.captured)
+        }
+    }
+
+    @Test
+    fun `onAttached - should skip to the current routeProgress distanceTraveled`() {
+        val progressObserver = slot<RouteProgressObserver>()
+        val routesObserver = slot<RoutesObserver>()
+        every {
+            mapboxNavigation.registerRouteProgressObserver(capture(progressObserver))
+        } just runs
+        every { mapboxNavigation.registerRoutesObserver(capture(routesObserver)) } just runs
+        val activeRoutes = mockActiveRoutesUpdatedResult()
+        val primaryRoute = activeRoutes.navigationRoutes.first()
+
+        sut.onAttached(mapboxNavigation)
+        progressObserver.captured.onRouteProgressChanged(
+            mockk {
+                every { navigationRoute } returns primaryRoute
+                every { distanceTraveled } returns 100.0f
+            }
+        )
+        routesObserver.captured.onRoutesChanged(activeRoutes)
+
+        val pushedEvents = slot<List<ReplayEventBase>>()
+        verify { replayer.pushEvents(capture(pushedEvents)) }
+        verifyDistanceTraveled(pushedEvents.captured, primaryRoute, 100.0)
+    }
+
+    @Test
+    fun `onAttached - should skip to short routeProgress distanceTraveled`() {
+        val progressObserver = slot<RouteProgressObserver>()
+        val routesObserver = slot<RoutesObserver>()
+        every {
+            mapboxNavigation.registerRouteProgressObserver(capture(progressObserver))
+        } just runs
+        every { mapboxNavigation.registerRoutesObserver(capture(routesObserver)) } just runs
+        val activeRoutes = mockActiveRoutesUpdatedResult()
+        val primaryRoute = activeRoutes.navigationRoutes.first()
+
+        sut.onAttached(mapboxNavigation)
+        progressObserver.captured.onRouteProgressChanged(
+            mockk {
+                every { navigationRoute } returns primaryRoute
+                every { distanceTraveled } returns 40.0f
+            }
+        )
+        routesObserver.captured.onRoutesChanged(activeRoutes)
+
+        val pushedEvents = slot<List<ReplayEventBase>>()
+        verify { replayer.pushEvents(capture(pushedEvents)) }
+        verifyDistanceTraveled(pushedEvents.captured, primaryRoute, 40.0)
+    }
+
+    private fun verifyDistanceTraveled(
+        pushedEvents: List<ReplayEventBase>,
+        primaryRoute: NavigationRoute,
+        distanceTraveled: Double
+    ) {
+        val geometry = primaryRoute.directionsRoute.geometry()!!
+        val fullRoute = PolylineUtils.decode(geometry, 6)
+        val expected = TurfMeasurement.along(fullRoute, distanceTraveled, TurfConstants.UNIT_METERS)
+        val firstReplayLocation = (pushedEvents.first() as ReplayEventUpdateLocation).location
+        val firstReplayPoint = Point.fromLngLat(firstReplayLocation.lon, firstReplayLocation.lat)
+        val distanceToPoint = TurfMeasurement.distance(expected, firstReplayPoint)
+
+        assertTrue("1.0 > $distanceToPoint", 1.0 > distanceToPoint)
+    }
+
+    private fun mockActiveRoutesUpdatedResult(): RoutesUpdatedResult = mockk {
+        every { navigationRoutes } returns listOf(mockNavigationRoute())
+    }
+
+    private fun mockRouteProgress(traveled: Double): RouteProgress = mockk {
+        every { navigationRoute } returns mockNavigationRoute()
+        every { distanceTraveled } returns traveled.toFloat()
+    }
+
+    private fun mockNavigationRoute(): NavigationRoute = mockk {
         val geometry = "_kmbgAppafhFwXaOuC}ApAoEbNqe@jAaEhEcOtAwEdAoD`DaLnCiJ|M_e@`Je[rAyEnEgO" +
             "tGiUxByHlDjBp@^zKdG`Ah@`HtDx@d@rGlDl@\\pAp@dAl@p@^nItEpQvJfAh@fDjB`D`Br@`@nKpFbDhB" +
             "~KlGtDvBvAwE|EqPzFeSvHaXtA{ElAiE|@_D"
-        every { navigationRoutes } returns listOf(
-            mockk {
-                every { id } returns "test-navigation-route-id"
-                every { routeOptions }
-                every { directionsRoute } returns mockk {
-                    every { routeOptions() } returns mockk {
-                        every { geometries() } returns "polyline6"
-                        every { geometry() } returns geometry
-                    }
-                }
+        every { id } returns "test-navigation-route-id"
+        every { routeOptions }
+        every { directionsRoute } returns mockk {
+            every { routeOptions() } returns mockk {
+                every { geometries() } returns "polyline6"
+                every { geometry() } returns geometry
             }
-        )
+        }
     }
 }
