Skip to content
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

Jump to bottom

Move faster route out of mapbox navigation #2577

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Move faster route out of mapbox navigation #2577

merged 4 commits into from
Mar 13, 2020

Conversation

kmadsen
Copy link
Contributor

@kmadsen kmadsen commented Mar 11, 2020

Description

MapboxNavigation is not going to scale with features like Faster Route added to the middle of it. This builds on top of the MapboxTimer to move faster route out of the core class.
#2575

  • 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

Testing

Please describe the manual tests that you ran to verify your changes

  • 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: @abhishek1508 @olegzil @Guardiola31337

@kmadsen kmadsen mentioned this pull request Mar 11, 2020
Merged
10 tasks
@kmadsen kmadsen force-pushed the km-decouple-faster-route branch from 385f84a to a7eae5e Compare March 11, 2020 23:32
@codecov-io
Copy link

codecov-io commented Mar 12, 2020 •
edited
Loading

Codecov Report

Merging #2577 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #2577   +/-   ##
=========================================
  Coverage     29.16%   29.16%           
  Complexity     1061     1061           
=========================================
  Files           281      281           
  Lines         10731    10731           
  Branches        870      870           
=========================================
  Hits           3130     3130           
  Misses         7258     7258           
  Partials        343      343

abhishek1508
abhishek1508 suggested changes Mar 12, 2020
View reviewed changes
...avigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteController.kt Outdated Show resolved Hide resolved
...avigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteController.kt Show resolved Hide resolved
...avigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteController.kt Outdated
private val tripSession: TripSession
) {
private val fasterRouteTimer = MapboxTimer()
private val fasterRouteObservers = CopyOnWriteArrayList<FasterRouteObserver>()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one class will have only one instance of FasterRouteTimer; hence does this need to be a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree with this. but also out of scope of this refactor because many of our interfaces support multiple subscribers

mapbox-navigation-android/examples/src/main/java/com/mapbox/navigation/examples/core/SimpleMapboxNavigationKt.kt

Lines 379 to 383 in d08f1e5

mapboxNavigation.registerLocationObserver(locationObserver)
mapboxNavigation.registerRouteProgressObserver(routeProgressObserver)
mapboxNavigation.registerRoutesObserver(routesObserver)
mapboxNavigation.registerTripSessionStateObserver(tripSessionStateObserver)
mapboxNavigation.registerFasterRouteObserver(fasterRouteObserver)

would prefer to separate that refactor

Copy link
Contributor

Choose a reason for hiding this comment

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

Other interfaces support multiple subscribers because a client can do his own implementation along with MapboxNavigation doing one itself. Whereas it is different for FasterRoute because only MapboxNavigation can register for it, whereas others will observe it. There is no need to have a list of subscribers.

Copy link
Contributor Author

@kmadsen kmadsen Mar 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I was refactoring a few things, and considered this part out of scope. Do you mind if I make that part a separate pull request?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to do it in the same PR given the fact it's just a couple of line changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok cool, pushed an update ❤️

...avigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteController.kt Show resolved Hide resolved
Guardiola31337
Guardiola31337 reviewed Mar 13, 2020
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Love this kind of PRs, refactoring stuff around so entities live where they should, decluttering other classes (like MapboxNavigation in this particular case) so they don't keep growing and growing. Great work @kmadsen ❤️

Left a minor question / comment.

...gation-core/src/main/java/com/mapbox/navigation/core/directions/session/DirectionsSession.kt Outdated Show resolved Hide resolved
@kmadsen kmadsen force-pushed the km-update-mapbox-timer branch 3 times, most recently from ba6799c to fab3dcd Compare March 13, 2020 16:02
@kmadsen kmadsen force-pushed the km-decouple-faster-route branch 2 times, most recently from 486a1e6 to 8e2120a Compare March 13, 2020 16:56
@kmadsen kmadsen force-pushed the km-update-mapbox-timer branch from 6c83e38 to fb755b7 Compare March 13, 2020 18:24
@kmadsen kmadsen changed the base branch from km-update-mapbox-timer to master March 13, 2020 18:45
@kmadsen kmadsen force-pushed the km-decouple-faster-route branch from 8e2120a to 57280b6 Compare March 13, 2020 18:51
@kmadsen kmadsen force-pushed the km-decouple-faster-route branch from 1ad3fdb to 9abe006 Compare March 13, 2020 20:07
@kmadsen kmadsen merged commit 017d4cc into master Mar 13, 2020
@kmadsen kmadsen deleted the km-decouple-faster-route branch March 13, 2020 22:17
kmadsen added a commit that referenced this pull request Mar 16, 2020
@kmadsen
Move faster route out of mapbox navigation (#2577)
a76fa22 
* Move faster route out of mapbox navigation

* rename

* Move to separate class

* update test

Co-authored-by: Kyle Madsen <>
@kmadsen kmadsen mentioned this pull request Mar 25, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guardiola31337 Guardiola31337 Guardiola31337 left review comments

@abhishek1508 abhishek1508 abhishek1508 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kmadsen @codecov-io @Guardiola31337 @abhishek1508