Skip to content

Fix Navigator.reset regression#2905

Merged
Guardiola31337 merged 1 commit into
masterfrom
pg-reset-navigator-regression
May 5, 2020
Merged

Fix Navigator.reset regression#2905
Guardiola31337 merged 1 commit into
masterfrom
pg-reset-navigator-regression

Conversation

@Guardiola31337
Copy link
Copy Markdown
Contributor

@Guardiola31337 Guardiola31337 commented May 5, 2020

Description

Fix Navigator#reset regression from #2850

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Goal

The Navigator was reset when stopping a trip session (e.g. stopping from the MapboxTripNotification) removing MapboxTripSession.route Navigators state

Implementation

  • Move navigator.reset out from MapboxTripSession to MapboxNavigation#onDestroy only

Testing

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

cc @Lebedinsky

@Guardiola31337 Guardiola31337 added bug Defect to be fixed. ✓ ready for review Core Work related to core navigation and integrations. labels May 5, 2020
@Guardiola31337 Guardiola31337 added this to the v1.0.0 milestone May 5, 2020
@Guardiola31337 Guardiola31337 self-assigned this May 5, 2020
@Guardiola31337 Guardiola31337 force-pushed the pg-reset-navigator-regression branch from fe2198b to d48bb7e Compare May 5, 2020 17:41
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 5, 2020

Codecov Report

Merging #2905 into master will increase coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff            @@
##             master    #2905   +/-   ##
=========================================
  Coverage     35.46%   35.46%           
  Complexity     2110     2110           
=========================================
  Files           545      545           
  Lines         19571    19575    +4     
  Branches       1846     1846           
=========================================
+ Hits           6940     6942    +2     
- Misses        11804    11806    +2     
  Partials        827      827

kmadsen
kmadsen reviewed May 5, 2020
View reviewed changes
Comment thread libnavigation-core/src/main/java/com/mapbox/navigation/core/NavigationComponentProvider.kt
): DirectionsSession =
MapboxDirectionsSession(router)

fun createNativeNavigator(): MapboxNativeNavigator = MapboxNativeNavigatorImpl
Copy link
Copy Markdown
Contributor

@kmadsen kmadsen May 5, 2020

Choose a reason for hiding this comment

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

why would this change any behavior? MapboxNativeNavigatorImpl is still an object?

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 May 5, 2020

Choose a reason for hiding this comment

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

This is for mocking MapboxNativeNavigator in tests, native code and JUnit don't get along 😬 Some time back I did this https://github.com/mapbox/mapbox-gl-native/pull/8261/files#diff-20a30dbc2607de725d1e4d09e2bd1403 to workaround the issue. Maybe we can bring it to the Navigation SDK

kmadsen
kmadsen reviewed May 5, 2020
View reviewed changes
Comment thread ...ion-core/src/main/java/com/mapbox/navigation/core/internal/trip/session/MapboxTripSession.kt
enhancedLocation = null
routeProgress = null
isOffRoute = false
navigator.reset()
Copy link
Copy Markdown
Contributor

@kmadsen kmadsen May 5, 2020

Choose a reason for hiding this comment

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

is this the fix?

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 May 5, 2020

Choose a reason for hiding this comment

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

Correct so we only reset the Navigator when in onDestroy

kmadsen
kmadsen reviewed May 5, 2020
View reviewed changes
Comment thread libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
tripSession.unregisterAllBannerInstructionsObservers()
tripSession.unregisterAllVoiceInstructionsObservers()
tripSession.route = null
navigator.reset()
Copy link
Copy Markdown
Contributor

@kmadsen kmadsen May 5, 2020

Choose a reason for hiding this comment

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

can we make this fix be, only moving the navigator.reset()?

The NavigationComponentProvider looks like a change that doesn't do anything, right?

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 May 5, 2020

Choose a reason for hiding this comment

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

The NavigationComponentProvider looks like a change that doesn't do anything, right?

See comment above. It's needed to mock MapboxNativeNavigator in order to keep onDestroyCallsNativeNavigatorReset test.

@Guardiola31337 Guardiola31337 merged commit 8d6fe1d into master May 5, 2020
@Guardiola31337 Guardiola31337 deleted the pg-reset-navigator-regression branch May 5, 2020 19:04
@kmadsen kmadsen mentioned this pull request Oct 29, 2020
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lebedinsky Lebedinsky Awaiting requested review from Lebedinsky

1 more reviewer

@kmadsen kmadsen kmadsen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Guardiola31337 Guardiola31337

Labels

bug Defect to be fixed. Core Work related to core navigation and integrations.

Projects

None yet

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Guardiola31337 @codecov-io @kmadsen