diff --git a/CHANGELOG.md b/CHANGELOG.md index 317673a72f5..81e5f3d5b3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Mapbox welcomes participation and contributions from everyone. - Fixed memory leak in `ReplayProgressObserver` which happened after route refresh. [#6691](https://github.com/mapbox/mapbox-navigation-android/pull/6691) - Fixed a rare issue where a reroute relative to old routes might have occurred after setting new routes. [#6693](https://github.com/mapbox/mapbox-navigation-android/pull/6693) - Added experimental `MapboxRouteLineOptions#shareLineGeometrySources` option to enable route line's GeoJson source data sharing between multiple instances of the map. [#6680](https://github.com/mapbox/mapbox-navigation-android/pull/6680) +- Fixed issues in `ReplayRouteSession`. The routes observer was never unregistered. Alternative route selection resets replay to the beginning. DropInUi changing portrait and landscape modes resets replay to the beginning. [#6675](https://github.com/mapbox/mapbox-navigation-android/pull/6675) ## Mapbox Navigation SDK 2.9.4 - 08 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 index f4936abc0d7..d339e355d27 100644 --- 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,15 @@ class ReplayPolylineDecodeStream( } return points } + + /** + * Skip the next [count] points of the geometry. Less points are skipped if there are less than + * [count] points left in the iterator. + * + * @param count the number of points to skip. + */ + fun skip(count: Int) { + var skipped = 0 + while (skipped++ <= count && hasNext()) { next() } + } } 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 index 3f5be86fd37..f8d649d6646 100644 --- 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,6 +18,7 @@ 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 @@ -54,15 +56,29 @@ class ReplayRouteSession : MapboxNavigationObserver { private var options = ReplayRouteSessionOptions.Builder().build() - private lateinit var polylineDecodeStream: ReplayPolylineDecodeStream 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 polylineDecodeStream: ReplayPolylineDecodeStream? = null + private var currentRoute: NavigationRoute? = null + + private val routeProgressObserver = RouteProgressObserver { routeProgress -> + if (currentRoute?.id != routeProgress.navigationRoute.id) { + currentRoute = routeProgress.navigationRoute + onRouteChanged(routeProgress) + } + } + + private val routesObserver = RoutesObserver { result -> + if (result.navigationRoutes.isEmpty()) { + mapboxNavigation?.resetReplayLocation() + currentRoute = null + polylineDecodeStream = null + } + } private val replayEventsObserver = ReplayEventsObserver { events -> - if (isLastEventPlayed(events)) { + if (currentRoute != null && isLastEventPlayed(events)) { pushMorePoints() } } @@ -92,45 +108,44 @@ 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() { mapboxReplayer.clearEvents() - resetTripSession() - if (options.locationResetEnabled) { - val context = navigationOptions.applicationContext - if (PermissionsManager.areLocationPermissionsGranted(context)) { - pushRealLocation(context) - } else { - logW(LOG_CATEGORY) { - "Location permission have not been accepted. If this is intentional, disable" + - " this warning with ReplayRouteSessionOptions.locationResetEnabled." + resetTripSession { + if (options.locationResetEnabled) { + val context = navigationOptions.applicationContext + if (PermissionsManager.areLocationPermissionsGranted(context)) { + pushRealLocation(context) + } else { + logW(LOG_CATEGORY) { + "Location permissions have not been accepted. If this is intentional, " + + "disable this warning with " + + "ReplayRouteSessionOptions.locationResetEnabled." + } } } + mapboxReplayer.play() } - mapboxReplayer.play() } 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 +159,12 @@ class ReplayRouteSession : MapboxNavigationObserver { } return } - currentRouteId = navigationRoute.id polylineDecodeStream = ReplayPolylineDecodeStream(geometry, 6) - mapboxNavigation?.resetTripSession() + + // Skip up to the current geometry index. There is some imprecision here because the + // distance traveled is not equal to a route index. + polylineDecodeStream?.skip(routeProgress.currentRouteGeometryIndex) + pushMorePoints() } @@ -158,7 +176,7 @@ class ReplayRouteSession : MapboxNavigationObserver { } private fun pushMorePoints() { - val nextPoints = polylineDecodeStream.decode(options.decodeMinDistance) + val nextPoints = polylineDecodeStream?.decode(options.decodeMinDistance) ?: return val nextReplayLocations = replayRouteMapper.mapPointList(nextPoints) lastLocationEvent = nextReplayLocations.lastOrNull { it is ReplayEventUpdateLocation } as? ReplayEventUpdateLocation 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 index f4e794b8c7b..9198cbeb513 100644 --- 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,13 +10,17 @@ 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.TripSessionResetCallback 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 io.mockk.every import io.mockk.just @@ -47,9 +51,16 @@ class ReplayRouteSessionTest { private val options: NavigationOptions = mockk { every { applicationContext } returns context } + private val routesObserver = slot() + private val routeProgressObserver = slot() 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 + every { resetTripSession(any()) } answers { + firstArg().onTripSessionReset() + } } private val bestLocationEngine: LocationEngine = mockk { every { getLastLocation(any()) } just runs @@ -102,7 +113,7 @@ class ReplayRouteSessionTest { verifyOrder { replayer.clearEvents() - mapboxNavigation.resetTripSession() + mapboxNavigation.resetTripSession(any()) replayer.play() } } @@ -166,8 +177,6 @@ class ReplayRouteSessionTest { @Test fun `onAttached - should push the initial batch of events`() { - val routesObserver = slot() - every { mapboxNavigation.registerRoutesObserver(capture(routesObserver)) } just runs sut.setOptions( ReplayRouteSessionOptions.Builder() .decodeMinDistance(1.0) @@ -175,7 +184,7 @@ class ReplayRouteSessionTest { ) sut.onAttached(mapboxNavigation) - routesObserver.captured.onRoutesChanged(mockActiveNavigationRoutes()) + routeProgressObserver.captured.onRouteProgressChanged(mockRouteProgress()) val pushedEvents = slot>() verify { replayer.pushEvents(capture(pushedEvents)) } @@ -193,7 +202,7 @@ class ReplayRouteSessionTest { replayEventsObserver.captured.replayEvents(firstArg()) replayer } - val routesUpdatedResult = mockActiveNavigationRoutes() + val routeProgress = mockRouteProgress() sut.setOptions( ReplayRouteSessionOptions.Builder() @@ -201,16 +210,19 @@ class ReplayRouteSessionTest { .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 @@ -218,20 +230,6 @@ class ReplayRouteSessionTest { ) } - @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>() @@ -242,6 +240,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 +264,159 @@ 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() + val routesObserver = slot() + val replayEventsObserver = slot() + 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() + val routesObserver = slot() + 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) + routesObserver.captured.onRoutesChanged(activeRoutes) + progressObserver.captured.onRouteProgressChanged( + mockk { + every { navigationRoute } returns primaryRoute + every { currentRouteGeometryIndex } returns 15 + } + ) + + val pushedEvents = slot>() + verify { replayer.pushEvents(capture(pushedEvents)) } + verifySkipToIndex(pushedEvents.captured, primaryRoute, 15) + } + + @Test + fun `onAttached - should skip to short routeProgress currentRouteGeometryIndex`() { + val progressObserver = slot() + val routesObserver = slot() + 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) + routesObserver.captured.onRoutesChanged(activeRoutes) + progressObserver.captured.onRouteProgressChanged( + mockk { + every { navigationRoute } returns primaryRoute + every { currentRouteGeometryIndex } returns 12 + } + ) + + val pushedEvents = slot>() + verify { replayer.pushEvents(capture(pushedEvents)) } + verifySkipToIndex(pushedEvents.captured, primaryRoute, 12) + } + + @Test + fun `onRouteProgress - will change to new route when the route changes`() { + val progressObserver = slot() + val routesObserver = slot() + every { + mapboxNavigation.registerRouteProgressObserver(capture(progressObserver)) + } just runs + every { mapboxNavigation.registerRoutesObserver(capture(routesObserver)) } just runs + val firstRoutesUpdatedResult = mockActiveRoutesUpdatedResult() + val firstRoute = firstRoutesUpdatedResult.navigationRoutes.first() + every { firstRoute.id } returns "test-first-route-id" + val firstRouteProgress = mockk { + every { navigationRoute } returns firstRoute + every { currentRouteGeometryIndex } returns 12 + } + val secondRoutesUpdatedResult = mockActiveRoutesUpdatedResult() + val secondRoute = secondRoutesUpdatedResult.navigationRoutes.first() + every { secondRoute.id } returns "test-second-route-id" + val secondRouteProgress = mockk { + every { navigationRoute } returns secondRoute + every { currentRouteGeometryIndex } returns 13 + } + + sut.onAttached(mapboxNavigation) + routesObserver.captured.onRoutesChanged(firstRoutesUpdatedResult) + progressObserver.captured.onRouteProgressChanged(firstRouteProgress) + progressObserver.captured.onRouteProgressChanged(firstRouteProgress) + progressObserver.captured.onRouteProgressChanged(firstRouteProgress) + routesObserver.captured.onRoutesChanged(secondRoutesUpdatedResult) + progressObserver.captured.onRouteProgressChanged(secondRouteProgress) + + verify(exactly = 2) { + replayer.clearEvents() + replayer.pushEvents(any()) + } + verifyOrder { + replayer.clearEvents() + replayer.play() + replayer.pushEvents(any()) + replayer.clearEvents() + replayer.play() + replayer.pushEvents(any()) + } + } + + private fun verifySkipToIndex( + pushedEvents: List, + primaryRoute: NavigationRoute, + currentRouteGeometryIndex: Int + ) { + val geometry = primaryRoute.directionsRoute.geometry()!! + val fullRoute = PolylineUtils.decode(geometry, 6) + val expected = fullRoute[currentRouteGeometryIndex] + val firstReplayLocation = (pushedEvents.first() as ReplayEventUpdateLocation).location + val firstReplayPoint = Point.fromLngLat(firstReplayLocation.lon, firstReplayLocation.lat) + + assertEquals(expected, firstReplayPoint) + } + + private fun mockActiveRoutesUpdatedResult(): RoutesUpdatedResult = mockk { + every { navigationRoutes } returns listOf(mockNavigationRoute()) + } + + private fun mockRouteProgress(): RouteProgress = mockk { + every { navigationRoute } returns mockNavigationRoute() + every { currentRouteGeometryIndex } returns 0 + } + + 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 } - ) + } } }