Skip to content

Kyv onboard route #2300

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
Merged
Lebedinsky merged 75 commits into from
Jan 13, 2020
Merged

Kyv onboard route #2300

Lebedinsky merged 75 commits into from
Jan 13, 2020

Conversation

@RingerJK
Copy link
Contributor

@RingerJK RingerJK commented Dec 10, 2019 •
edited
Loading

Description

Onboard(offline) Route #issue

  • 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

Implement OnBoard Route.

Implementation

Provide OnBoard Route. Separate Transfer and Present layers.

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

@RingerJK RingerJK added feature New feature request. new API(s) labels Dec 10, 2019
@RingerJK RingerJK added this to the v1.0.0 milestone Dec 10, 2019
@RingerJK RingerJK self-assigned this Dec 10, 2019
RingerJK
RingerJK commented Dec 11, 2019
View reviewed changes
...ion-base/src/main/java/com/mapbox/navigation/base/route/dto/StepIntersectionNavigationDto.kt
bearings = bearings,
classes = classes,
entry = entry,
into = `in`,
Copy link
Contributor Author

@RingerJK RingerJK Dec 11, 2019

Choose a reason for hiding this comment

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

renamed in to into because in is reserved in kotlin. cc @Guardiola31337

@codecov-io
Copy link

codecov-io commented Dec 11, 2019 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2300   +/-   ##
=========================================
  Coverage     32.77%   32.77%           
  Complexity      624      624           
=========================================
  Files           143      143           
  Lines          5547     5547           
  Branches        427      427           
=========================================
  Hits           1818     1818           
  Misses         3533     3533           
  Partials        196      196

Guardiola31337
Guardiola31337 reviewed Dec 12, 2019
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.

This is a great start of the Onboard implementation, great work @RingerJK 💪

I'm missing end-to-end tests / examples verifying that everything is working as expected. Those will help us 👀 how every component plays to each other and if the overall design looks 👌

As it's easy to mess up 😅, I think we should make sure to build examples in Java and in Kotlin very early on, so that we could see how the API changes would impact both languages.

Also I left a bunch of comments / questions 😬

...c/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationWalkingOptions.kt
/**
* Class for specifying options for use with the walking profile.
*/
class NavigationWalkingOptions internal constructor(val walkingOptions: WalkingOptions) {
Copy link
Contributor

@Guardiola31337 Guardiola31337 Dec 12, 2019

Choose a reason for hiding this comment

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

This is already defined in libdirections-offboard and it's never used. Could we reuse the one from Offboard module instead of creating a new duplicated class and remove this?

Copy link
Contributor Author

@RingerJK RingerJK Dec 13, 2019

Choose a reason for hiding this comment

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

They are independent, one in old module, another in new one. As I remember we decided don't remove NavigationWalkingOptions in libdirections-offboard because we'll need it for future features

Copy link

@LukasPaczos LukasPaczos Jan 8, 2020

Choose a reason for hiding this comment

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

If those builders and DTOs are shared, they should live in base.

Copy link
Contributor Author

@RingerJK RingerJK Jan 9, 2020

Choose a reason for hiding this comment

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

they are, ptal at model

...vigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/extensions/Mappers.kt Outdated
.walkwayBias(walkwayBias)
.alleyBias(alleyBias)
.build()

Copy link
Contributor

@Guardiola31337 Guardiola31337 Dec 12, 2019

Choose a reason for hiding this comment

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

WalkingOptionsNavigation.mapToWalkingOptions() ☝️ is never used.

Copy link
Contributor Author

@RingerJK RingerJK Dec 13, 2019

Choose a reason for hiding this comment

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

it's used in my next PR

...tion-base/src/main/java/com/mapbox/navigation/base/route/model/StepIntersectionNavigation.kt
* @return array of lane objects that represent the available turn lanes at the intersection
* @since 1.0
*/
class StepIntersectionNavigation(
Copy link
Contributor

@Guardiola31337 Guardiola31337 Dec 12, 2019

Choose a reason for hiding this comment

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

Same comments as above re: DTOs / models

...vigation-base/src/main/java/com/mapbox/navigation/base/route/model/StepManeuverNavigation.kt Outdated
* rotary - a traffic circle
* @since 1.0
*/
class StepManeuverNavigation(
Copy link
Contributor

@Guardiola31337 Guardiola31337 Dec 12, 2019

Choose a reason for hiding this comment

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

Same comments as above re: DTOs / models

...ion-base/src/main/java/com/mapbox/navigation/base/route/model/VoiceInstructionsNavigation.kt
* Speech Synthesis Markup Language which helps voice synthesiser read information more humanely.
* @since 1.0
*/
class VoiceInstructionsNavigation(
Copy link
Contributor

@Guardiola31337 Guardiola31337 Dec 12, 2019

Choose a reason for hiding this comment

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

Same comments as above re: DTOs / models

...gation-base/src/main/java/com/mapbox/navigation/base/route/model/WalkingOptionsNavigation.kt
* allowed range of values is from -1 to 1, where -1 indicates indicates preference to avoid
* alleys, 1 indicates preference to favor alleys, and 0 indicates no preference (the default).
*/
data class WalkingOptionsNavigation(
Copy link
Contributor

@Guardiola31337 Guardiola31337 Dec 12, 2019

Choose a reason for hiding this comment

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

Same comments as above re: DTOs / models

@Guardiola31337 Guardiola31337 mentioned this pull request Dec 16, 2019
Merged
10 tasks
@RingerJK RingerJK marked this pull request as ready for review December 18, 2019 14:32
korshaknn
korshaknn reviewed Dec 19, 2019
View reviewed changes
RingerJK and others added 23 commits January 13, 2020 14:46
@RingerJK
- Onboard router: sample Activity Java + Kotlin; - make Dto classes p…
5c0eb8d 
…ublic; - fix tests; - minor fixes/refactored.
@RingerJK
Mappers: explicitly specify return types
5d688a7
@RingerJK
Mappers: explicitly specify return types
6af8bf9
@RingerJK
- RouteUrl: test; - lint issues
fa17213
@RingerJK
- Rebase fixes; lint fix
29798f9
@RingerJK
lint issue
95d01ec
@RingerJK
RouteResponse: minor improves
7ea6a79
@RingerJK
pr issues
da535b4
@RingerJK
fix rebase issue
0613138
@RingerJK
- Provide HttpClient for native; - Onboard test activities: comment d…
b4e6207 
…irection session dependencies(because one was moved to code module).
@RingerJK
exclude OnboardRouterActivityJava.java from check style
d03aa2a
@RingerJK
fix pr issue
e607054
@RingerJK
fix rebase issues
9f4f033
@RingerJK
rebase fixes
e8f5a44
@RingerJK
rebase fix
f944279
@RingerJK
rebase fix
be05ab0
@RingerJK
fix pr issues: - moved OnBoard's Activities to examples module; - r…
4220d03 
…emoved new modules dependencies from legacy ones
@RingerJK
Cleanup test and Test Onboard Activities
551be46
@RingerJK
cleanup code; fix pr issues
a4117b3
@RingerJK
pr issue fixes
9bc2ab9
@RingerJK
rebase issues
f103784
@RingerJK
rebase fixes
18df0e7
Fix rebase issues. Remove unused resources. Fix tests and naming
0beeb13
@Lebedinsky Lebedinsky self-assigned this Jan 13, 2020
@Lebedinsky Lebedinsky merged commit 82d9814 into master Jan 13, 2020
@Guardiola31337 Guardiola31337 mentioned this pull request Jan 13, 2020
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@korshaknn korshaknn korshaknn approved these changes

@olegzil olegzil Awaiting requested review from olegzil

@abhishek1508 abhishek1508 Awaiting requested review from abhishek1508

+3 more reviewers

@Lebedinsky Lebedinsky Lebedinsky left review comments

@LukasPaczos LukasPaczos LukasPaczos left review comments

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Lebedinsky Lebedinsky

@RingerJK RingerJK

Labels

feature New feature request.

Projects

None yet

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@RingerJK @codecov-io @Lebedinsky @Guardiola31337 @korshaknn @LukasPaczos