Skip to content

Conversation

@RingerJK
Copy link
Contributor

Description

Navigator: adapt start session, stop session, and restore session

@RingerJK RingerJK requested a review from dzinad March 29, 2023 16:09
@RingerJK RingerJK self-assigned this Mar 29, 2023
@RingerJK RingerJK added the skip changelog Should not be added into version changelog. label Mar 29, 2023
runIfNotDestroyed {
latestLegIndex = tripSession.getRouteProgress()?.currentLegProgress?.legIndex
tripSession.stop()
tripSession.stop(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understood from your discussion with @averkhaturau in the NN PR, it would only be true if we haven't arrived yet, right? If so, there's no such check here.

Choose a reason for hiding this comment

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

The idea of the cancelled : bool parameter here is that routing session cancel is not a complete opposite to routing complete. E.g. User may finish the route but navigator will not detect it because the user parks on another side of the building / fence, etc. But actual session is successfully complete and the User shows it by pressing thumb up. From other side, session may be detected is finished by mistake, e.g. wrong map-matching, target point where is impossible to stop, etc., and the user will actually cancel the session.

Choose a reason for hiding this comment

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


fun start(withTripService: Boolean, withReplayEnabled: Boolean = false)
fun stop()
fun stop(canceled: Boolean)
Copy link
Contributor

Choose a reason for hiding this comment

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

You always invoke it with true...

@dzinad
Copy link
Contributor

dzinad commented Mar 29, 2023

Is there a way to test it in integration? Ideally in instrumentation tests, but at least manually.

@RingerJK
Copy link
Contributor Author

@dzinad regarding the canceled flag: I'm not sure why this flag was added, but NN would be able to handle cancel events without that flag. We're gonna discuss that next week. Let's move the PR to draft until the discussion happen.

@RingerJK
Copy link
Contributor Author

Is there a way to test it in integration? Ideally in instrumentation tests, but at least manually.

the only place where we can check that is telemetry, which is part of another PR. We're good to move these changes as they do not affect the SDK logic or current telemetry

@RingerJK RingerJK marked this pull request as draft March 29, 2023 16:31
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #7060 (72acda3) into main (1e91079) will decrease coverage by 0.02%.
The diff coverage is 28.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #7060      +/-   ##
============================================
- Coverage     73.63%   73.61%   -0.02%     
  Complexity     5855     5855              
============================================
  Files           802      802              
  Lines         31419    31430      +11     
  Branches       3735     3735              
============================================
+ Hits          23136    23138       +2     
- Misses         6806     6815       +9     
  Partials       1477     1477              
Impacted Files Coverage Δ
...mapbox/navigation/core/trip/session/TripSession.kt 100.00% <ø> (ø)
...gation/navigator/internal/MapboxNativeNavigator.kt 0.00% <ø> (ø)
...on/navigator/internal/MapboxNativeNavigatorImpl.kt 0.00% <0.00%> (ø)
...ava/com/mapbox/navigation/core/MapboxNavigation.kt 70.55% <66.66%> (ø)
.../navigation/core/trip/session/MapboxTripSession.kt 85.10% <100.00%> (+0.07%) ⬆️

@RingerJK
Copy link
Contributor Author

closing in favor #7145

@RingerJK RingerJK closed this Apr 28, 2023
@RingerJK RingerJK deleted the kyv-NN-adapt-start-and-stop-session- branch April 28, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog Should not be added into version changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants