Remove stopTripSession from ReplayRouteSession

[kmadsen]

Description

Resolves https://mapbox.atlassian.net/browse/NAVAND-320
Resolves #6056

There is an internal requirement to first call stopTripSession before you can call startReplayTripSession. This pull request is fixing that. This will make it so the caller can convert a regular trip session into a replay session.

I'm finding that this issue needs to be resolved before resolving this issue #6794. Anything that observes the trip session state receives rapid changes when enabling replay.

[github-actions]

Changelog

Features

• Introduced ViewStyleCustomization.infoPanelGuidelineMaxPosPercent that allows customization of the NavigationView InfoPanel bottom guideline maximum position. Increased default value to 50%. #6792

Bug fixes and improvements

• Improved MapboxNavigation#startReplayTripSession and ReplayRouteSession so that the previous trip session does not need to be stopped. ⚠️ ReplayRouteSession#onDetached removed the call to stopTripSession. #6817

• Each newly instantiated MapboxRouteArrowView class will initialize the layers with the provided options on the first render call. Previously this would only be done if the layers hadn't already been initialized. #6466

• ⚠️ Updated the NavigationView default navigation puck asset. #6678

  Previous puck can be restored by injecting LocationPuck2D with the bearingImage set to com.mapbox.navigation.ui.maps.R.drawable.mapbox_navigation_puck_icon drawable:

  navigationView.customizeViewStyles {
      locationPuckOptions = LocationPuckOptions.Builder(context)
          .defaultPuck(
              LocationPuck2D(
                  bearingImage = ContextCompat.getDrawable(
                      context,
                      com.mapbox.navigation.ui.maps.R.drawable.mapbox_navigation_puck_icon,
                  )
              )
          )
          .idlePuck(regularPuck(context))
          .build()
  }

• Fixed an issue with NavigationView that caused info panel to shrink in landscape mode with a full screen theme. #6780

• Fixed standalone MapboxManeuverView appearance when the app also integrates Drop-In UI. #6774

• Added guarantees that route progress with RouteProgress#currentState == OFF_ROUTE arrives earlier than NavigationRerouteController#reroute is called. #6764

• Introduced NavigationViewListener.onSpeedInfoClicked that would be triggered when MapboxSpeedInfoView is clicked upon. #6770

• Fixed a rare java.lang.NullPointerException: Attempt to read from field 'SpeechAnnouncement PlayCallback.announcement' on a null object reference crash in PlayCallback.getAnnouncement. #6760

• Fixed an issue where the first voice instruction might have been played twice. #6766

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

[codecov]

Codecov Report

Merging #6817 (370f0ab) into main (61415ea) will decrease coverage by 0.00%.
The diff coverage is 86.95%.

@@             Coverage Diff              @@
##               main    #6817      +/-   ##
============================================
- Coverage     72.64%   72.63%   -0.01%     
+ Complexity     5569     5568       -1     
============================================
  Files           780      780              
  Lines         30104    30101       -3     
  Branches       3553     3556       +3     
============================================
- Hits          21869    21864       -5     
- Misses         6808     6809       +1     
- Partials       1427     1428       +1     

Impacted Files	Coverage Δ
...navigation/core/replay/route/ReplayRouteSession.kt	70.78% <ø> (-0.65%)	⬇️
...al/controller/TripSessionStarterStateController.kt	86.66% <ø> (ø)
...ion/core/trip/session/TripSessionLocationEngine.kt	81.81% <71.42%> (-5.69%)	⬇️
.../navigation/core/trip/session/MapboxTripSession.kt	86.57% <100.00%> (-0.08%)	⬇️
...igation/dropin/tripsession/TripSessionComponent.kt	85.71% <100.00%> (+1.26%)	⬆️

[VysotskiVadim]

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

[kmadsen]

🤔 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 {

[dzinad]

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.

[kmadsen]

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?

[dzinad]

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.