-
Notifications
You must be signed in to change notification settings - Fork 322
Fix ReplayRouteSession route state #6675
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.
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4cea25b
to
ce47aaf
Compare
970bd39
to
3108291
Compare
Codecov Report
@@ Coverage Diff @@
## main #6675 +/- ##
============================================
- Coverage 72.49% 72.48% -0.02%
- Complexity 5417 5420 +3
============================================
Files 764 764
Lines 29484 29497 +13
Branches 3500 3501 +1
============================================
+ Hits 21374 21380 +6
- Misses 6712 6714 +2
- Partials 1398 1403 +5
|
...ion-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayPolylineDecodeStream.kt
Outdated
Show resolved
Hide resolved
kmadsen
commented
Dec 2, 2022
...igation-core/src/test/java/com/mapbox/navigation/core/replay/route/ReplayRouteSessionTest.kt
Show resolved
Hide resolved
dzinad
reviewed
Dec 5, 2022
libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt
Outdated
Show resolved
Hide resolved
7dc2e2f
to
765442f
Compare
kmadsen
referenced
this pull request
Dec 7, 2022
45d7271
to
0755105
Compare
libnavigation-core/src/main/java/com/mapbox/navigation/core/replay/route/ReplayRouteSession.kt
Outdated
Show resolved
Hide resolved
0755105
to
63f80fc
Compare
4e1b977
to
e0065a4
Compare
abhishek1508
approved these changes
Dec 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixing issues in
ReplayRouteSession
.More details
Here is a similar issue where we added Alternative Route support in the
ReplayProgressObserver
. #5586. Essentially, when the route changes it needs to calculate thedistanceTraveled
and skip some replay events. TheReplayProgressObserver
uses an indexAlong function, where the new way can use the new memory efficientReplayPolylineDecodeStream.skip
.There is probably a leaking routeProgressObserver in the last beta for anything using
ReplayRouteSession
. I found it while digging into the issues. I've added a test to cover it. It was registered in onAttached, and not unregistered in onDetached. Simple fix.The DropInUi fix is slightly lucky, because the
ReplayRouteSession
is not meant to carry information fromonDetatched
->onAttached
. TheTripSessionComponent
is tied to theNavigationView
lifecycle, so the configuration changes trigger onDetached/onAttached events. I'm considering the alternative route fix the true fix, and DropInUi a lucky beneficiary. This is also how we discovered theReplayRouteSession
issue. More details below.More more details
The original approach uses the
RoutesObserver
to decide the current route #6636. This new approach uses theRouteProgressObserver
in a way that is similar toReplayProgressObserver
. This decision is to support DropInUi, the behavior of the callbacks act in such a way that information can be carried from onDetatched/onAttached. I am in favor of making it so onDetached does not happen when a configuration changes, but that will require changes toTripSessionComponent
.I have been thinking about how to handle configuration changes with
MapboxNavigationObserver
, there is a draft here #6484. We can design certain observers to be aware when configurations are changing and maintain state while it is happening. I'm not excited by it because a configuration change does not mean the observer will be re-attached, in which case the state can leak. It feels better to attach the observer to a better lifecycle, such as the Application CarAppLifecycle or ViewModel. Or other ideas.