Skip to content

Remove stopTripSession from ReplayRouteSession #6817

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

Merged
kmadsen merged 1 commit into from
Jan 11, 2023
Merged

Remove stopTripSession from ReplayRouteSession #6817

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/6817.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Improved `MapboxNavigation#startReplayTripSession` and `ReplayRouteSession` so that the previous trip session does not need to be stopped. :warning: `ReplayRouteSession#onDetached` removed the call to `stopTripSession`.
2 changes: 0 additions & 2 deletions ...vigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ class ReplayRouteSession : MapboxNavigationObserver {
override fun onAttached(mapboxNavigation: MapboxNavigation) {
this.replayRouteMapper = ReplayRouteMapper(options.replayRouteOptions)
this.mapboxNavigation = mapboxNavigation
mapboxNavigation.stopTripSession()
mapboxNavigation.startReplayTripSession()
mapboxNavigation.registerRouteProgressObserver(routeProgressObserver)
mapboxNavigation.registerRoutesObserver(routesObserver)
Expand Down Expand Up @@ -139,7 +138,6 @@ class ReplayRouteSession : MapboxNavigationObserver {
mapboxNavigation.mapboxReplayer.unregisterObserver(replayEventsObserver)
mapboxNavigation.mapboxReplayer.stop()
mapboxNavigation.mapboxReplayer.clearEvents()
mapboxNavigation.stopTripSession()
this.mapboxNavigation = null
this.currentRoute = null
}
Expand Down
61 changes: 29 additions & 32 deletions ...avigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,28 @@ internal class MapboxTripSession(
}
}

private val onRawLocationUpdate: (Location) -> Unit = { rawLocation ->
val locationHash = rawLocation.hashCode()
logD(
"updateRawLocation; system elapsed time: ${System.nanoTime()}; " +
"location ($locationHash) elapsed time: ${rawLocation.elapsedRealtimeNanos}",
LOG_CATEGORY
)
this.rawLocation = rawLocation
locationObservers.forEach { it.onNewRawLocation(rawLocation) }
mainJobController.scope.launch(start = CoroutineStart.UNDISPATCHED) {
logD(
"updateRawLocation; notify navigator for ($locationHash) - start",
LOG_CATEGORY
)
navigator.updateLocation(rawLocation.toFixLocation())
logD(
"updateRawLocation; notify navigator for ($locationHash) - end",
LOG_CATEGORY
)
}
}

init {
navigator.setNativeNavigatorRecreationObserver {
if (fallbackVersionsObservers.isNotEmpty()) {
Expand Down Expand Up @@ -297,39 +319,14 @@ internal class MapboxTripSession(
* Start MapboxTripSession
*/
override fun start(withTripService: Boolean, withReplayEnabled: Boolean) {
if (state == TripSessionState.STARTED) {
return
}
navigator.addNavigatorObserver(navigatorObserver)
if (withTripService) {
tripService.startService()
}
tripSessionLocationEngine.startLocationUpdates(withReplayEnabled) {
updateRawLocation(it)
}
state = TripSessionState.STARTED
}

private fun updateRawLocation(rawLocation: Location) {
val locationHash = rawLocation.hashCode()
logD(
"updateRawLocation; system elapsed time: ${System.nanoTime()}; " +
"location ($locationHash) elapsed time: ${rawLocation.elapsedRealtimeNanos}",
LOG_CATEGORY
)
this.rawLocation = rawLocation
locationObservers.forEach { it.onNewRawLocation(rawLocation) }
mainJobController.scope.launch(start = CoroutineStart.UNDISPATCHED) {
logD(
"updateRawLocation; notify navigator for ($locationHash) - start",
LOG_CATEGORY
)
navigator.updateLocation(rawLocation.toFixLocation())
logD(
"updateRawLocation; notify navigator for ($locationHash) - end",
LOG_CATEGORY
)
if (state != TripSessionState.STARTED) {
navigator.addNavigatorObserver(navigatorObserver)
if (withTripService) {
tripService.startService()
}
state = TripSessionState.STARTED
}
tripSessionLocationEngine.startLocationUpdates(withReplayEnabled, onRawLocationUpdate)
Copy link
Contributor

@VysotskiVadim VysotskiVadim Jan 11, 2023

Choose a reason for hiding this comment

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

Maybe we need to call NativeNavigaot#resetRideSession so that native navigator doesn't consider first location updates from new location engine as an error?

Copy link
Contributor Author

@kmadsen kmadsen Jan 11, 2023

Choose a reason for hiding this comment

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

🤔 yeah. A new location engine seems like a good reason to reset. I'm hesitant, should it go inside the startTripSession? Let's consider it in another change. The caller still has the ability to call it or not call it. Right now ReplayRouteSession calls it when the route changes. We can add it there before forcing it everywhere.

mapbox-navigation-android/libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt

Lines 117 to 119 in 61415ea

private fun MapboxNavigation.resetReplayLocation() {
mapboxReplayer.clearEvents()
resetTripSession {

}

/**
Expand Down
54 changes: 29 additions & 25 deletions ...n-core/src/main/java/com/mapbox/navigation/core/trip/session/TripSessionLocationEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,55 +24,60 @@ import java.util.concurrent.TimeUnit
* trip session is not using replay, use the [NavigationOptions.locationEngine].
*/
internal class TripSessionLocationEngine constructor(
val navigationOptions: NavigationOptions
private val navigationOptions: NavigationOptions,
private val replayLocationEngineProvider: (MapboxReplayer) -> LocationEngine = {
ReplayLocationEngine(it)
}
) {

val mapboxReplayer: MapboxReplayer by lazy { MapboxReplayer() }

private val replayLocationEngine: ReplayLocationEngine by lazy {
ReplayLocationEngine(mapboxReplayer)
private val replayLocationEngine: LocationEngine by lazy {
replayLocationEngineProvider.invoke(mapboxReplayer)
}
private var locationEngine: LocationEngine = navigationOptions.locationEngine
private var activeLocationEngine: LocationEngine? = null
private var onRawLocationUpdate: (Location) -> Unit = { }

private val locationEngineCallback = object : LocationEngineCallback<LocationEngineResult> {
override fun onSuccess(result: LocationEngineResult?) {
logD(LOG_CATEGORY) {
"successful location engine callback $result"
}
result?.locations?.lastOrNull()?.let {
logIfLocationIsNotFreshEnough(it)
onRawLocationUpdate(it)
}
}

override fun onFailure(exception: Exception) {
logD("location on failure exception=$exception", LOG_CATEGORY)
}
}

@SuppressLint("MissingPermission")
fun startLocationUpdates(isReplayEnabled: Boolean, onRawLocationUpdate: (Location) -> Unit) {
logD(LOG_CATEGORY) {
"starting location updates for ${if (isReplayEnabled) "replay " else ""}location engine"
}
stopLocationUpdates()
this.onRawLocationUpdate = onRawLocationUpdate
val locationEngine = if (isReplayEnabled) {
activeLocationEngine = if (isReplayEnabled) {
replayLocationEngine
} else {
navigationOptions.locationEngine
}
locationEngine.requestLocationUpdates(
activeLocationEngine?.requestLocationUpdates(
Copy link
Contributor

@dzinad dzinad Jan 16, 2023

Choose a reason for hiding this comment

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

requestLocationUpdates can be invoked multiple times with different callbacks and all of them will receive location updates.
So my question is: is stopping location updates here the right way to go? I mean what if we want to invoke startLocationUpdates twice with different onRawLocationUpdate? Right now the second invocation will cancel the first one.
By the way, having a single locationEngineCallback guarantees that there will be only one location update, but is this designed? Eve if we remove stopLocationUpdates from here the situation won't change: we'll still be getting raw location updates only in the second lambda.
I mean it's not clear from the API that that's the behaviour. So I'm asking whether it's how it's supposed to be.

Copy link
Contributor Author

@kmadsen kmadsen Jan 17, 2023

Choose a reason for hiding this comment

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

is stopping location updates here the right way to go? I mean what if we want to invoke startLocationUpdates twice with different onRawLocationUpdate? Right now the second invocation will cancel the first one.

It is built to manage a single location engine subscription, because it would be sending duplicates if it had more than one subscription. The activeLocationEngine can change based on parameters, so it is unregistering to whatever the previous engine is.

There could be improvements to the logic, but i'm not seeing why it would be needed. Do you see an issue?

Copy link
Contributor

@dzinad dzinad Jan 18, 2023

Choose a reason for hiding this comment

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

I'm only saying that it's not obvious how to use this API. It looks like you can invoke startLocationUpdates twice with different callbacks, receive updates to both callbacks and then stop everything via stopLocationUpdates.
As long as we use the API correctly, there is no issue.

navigationOptions.locationEngineRequest,
locationEngineCallback,
Looper.getMainLooper()
)
locationEngine.getLastLocation(locationEngineCallback)
activeLocationEngine?.getLastLocation(locationEngineCallback)
}

fun stopLocationUpdates() {
onRawLocationUpdate = { }
locationEngine.removeLocationUpdates(locationEngineCallback)
}

private var locationEngineCallback = object : LocationEngineCallback<LocationEngineResult> {
override fun onSuccess(result: LocationEngineResult?) {
logD(LOG_CATEGORY) {
"successful location engine callback $result"
}
result?.locations?.lastOrNull()?.let {
logIfLocationIsNotFreshEnough(it)
onRawLocationUpdate(it)
}
}

override fun onFailure(exception: Exception) {
logD("location on failure exception=$exception", LOG_CATEGORY)
}
activeLocationEngine?.removeLocationUpdates(locationEngineCallback)
activeLocationEngine = null
}

private fun logIfLocationIsNotFreshEnough(location: Location) {
Expand All @@ -88,7 +93,6 @@ internal class TripSessionLocationEngine constructor(
}

private companion object {

private const val DELAYED_LOCATION_WARNING_THRESHOLD_MS = 500 // 0.5s
private const val LOG_CATEGORY = "TripSessionLocationEngine"
}
Expand Down
16 changes: 2 additions & 14 deletions ...tion-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,6 @@ class ReplayRouteSessionTest {
unmockkAll()
}

@Test
fun `onAttached - should stop trip session and start replay session`() {
sut.onAttached(mapboxNavigation)

verifyOrder {
mapboxNavigation.stopTripSession()
mapboxNavigation.startReplayTripSession()
replayer.play()
}
}

@Test
fun `onAttached - should reset trip session and replayer when navigation routes are cleared`() {
val routesObserver = slot<RoutesObserver>()
Expand Down Expand Up @@ -139,13 +128,13 @@ class ReplayRouteSessionTest {
}

@Test
fun `onDetached - should stop trip session and replayer`() {
fun `onDetached - should stop and clear the replayer`() {
sut.onDetached(mapboxNavigation)

verifyOrder {
replayer.unregisterObserver(any())
replayer.stop()
replayer.clearEvents()
mapboxNavigation.stopTripSession()
}
}

Expand Down Expand Up @@ -251,7 +240,6 @@ class ReplayRouteSessionTest {
)

verifyOrder {
mapboxNavigation.stopTripSession()
mapboxNavigation.startReplayTripSession()
replayer.play()
replayer.pushEvents(any())
Expand Down
33 changes: 28 additions & 5 deletions ...ation-core/src/test/java/com/mapbox/navigation/core/trip/session/MapboxTripSessionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,16 +238,39 @@ class MapboxTripSessionTest {
@Test
fun startSessionWithTripServiceCallStartAgain() {
tripSession.start(true)

tripSession.start(false)

every { tripService.hasServiceStarted() } returns true

assertTrue(tripSession.isRunningWithForegroundService())
verify(exactly = 1) { tripService.startService() }
verify(exactly = 1) { tripSessionLocationEngine.startLocationUpdates(any(), any()) }
tripSession.stop()

verifyOrder {
navigator.addNavigatorObserver(any())
tripService.startService()
tripSessionLocationEngine.startLocationUpdates(any(), any())
tripSessionLocationEngine.startLocationUpdates(any(), any())
navigator.removeNavigatorObserver(any())
tripSession.stop()
tripSessionLocationEngine.stopLocationUpdates()
}
}

@Test
fun startSessionWithReplayCallStartAgain() {
tripSession.start(true, withReplayEnabled = false)
tripSession.start(true, withReplayEnabled = true)
tripSession.start(true, withReplayEnabled = false)
tripSession.stop()

verifyOrder {
navigator.addNavigatorObserver(any())
tripService.startService()
tripSessionLocationEngine.startLocationUpdates(false, any())
tripSessionLocationEngine.startLocationUpdates(true, any())
tripSessionLocationEngine.startLocationUpdates(false, any())
navigator.removeNavigatorObserver(any())
tripSession.stop()
tripSessionLocationEngine.stopLocationUpdates()
}
}

@Test
Expand Down
Loading