Skip to content

Consume banner instructions from NN #1543

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
merged 1 commit into from
Jan 16, 2019
Merged

Consume banner instructions from NN #1543

merged 1 commit into from
Jan 16, 2019

Conversation

@Guardiola31337
Copy link
Contributor

@Guardiola31337 Guardiola31337 commented Nov 16, 2018

Fixes #1271

  • Updates RouteProgress and BannerInstructionMilestone for banner data from Navigator

@Guardiola31337 Guardiola31337 self-assigned this Nov 16, 2018
@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch 2 times, most recently from 7f58a03 to 719f58a Compare November 27, 2018 23:46
@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch from 719f58a to fa4fc60 Compare December 11, 2018 19:15
@Guardiola31337 Guardiola31337 added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Dec 11, 2018
@codecov-io
Copy link

codecov-io commented Dec 11, 2018 •
edited
Loading

Codecov Report

Merging #1543 into master will increase coverage by 0.09%.
The diff coverage is 82.25%.

@@             Coverage Diff              @@
##             master    #1543      +/-   ##
============================================
+ Coverage     26.69%   26.79%   +0.09%     
+ Complexity      823      799      -24     
============================================
  Files           202      202              
  Lines          8533     8533              
  Branches        627      622       -5     
============================================
+ Hits           2278     2286       +8     
+ Misses         6042     6035       -7     
+ Partials        213      212       -1

@Guardiola31337 Guardiola31337 added the ⚠️ DO NOT MERGE PR should not be merged! label Dec 11, 2018
@Guardiola31337
Copy link
Contributor Author

Guardiola31337 commented Dec 11, 2018 •
edited
Loading

TODO

  • Bump mapboxNavigator version when components fix is released and available

@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch from fa4fc60 to bd8f933 Compare December 12, 2018 20:18
@Guardiola31337 Guardiola31337 added this to the v0.27.0 milestone Dec 20, 2018
@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch from bd8f933 to ea59bca Compare January 9, 2019 12:53
@Guardiola31337 Guardiola31337 mentioned this pull request Jan 11, 2019
Closed
@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch 4 times, most recently from 3be48d9 to 6439493 Compare January 16, 2019 17:53
@Guardiola31337 Guardiola31337 changed the title [WIP] Consume banner instructions from NN Consume banner instructions from NN Jan 16, 2019
@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch 2 times, most recently from 505469c to 502d046 Compare January 16, 2019 18:52
@Guardiola31337
Copy link
Contributor Author

Guardiola31337 commented Jan 16, 2019

mapboxNavigator 4.0.0 was released 🎉

@danesfeder This is updated and ready for a final round of 👀

@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch from 502d046 to 2d8fef2 Compare January 16, 2019 19:27
danesfeder
danesfeder reviewed Jan 16, 2019
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@Guardiola31337 this looks good, just a couple minor questions.

Also when testing, I noticed a delay with the banner showing at startup. Is this a behavior introduced by Navigator pushing the data only when tracking?

ezgif com-video-to-gif

...ain/java/com/mapbox/services/android/navigation/v5/milestone/BannerInstructionMilestone.java Outdated
private BannerInstructions instructions;
private RouteUtils routeUtils;

BannerInstructionMilestone(Builder builder) {
Copy link
Contributor

@danesfeder danesfeder Jan 16, 2019

Choose a reason for hiding this comment

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

This can be private, do we want to keep as-is for testing or can we still test with it marked as private?

danesfeder
danesfeder reviewed Jan 16, 2019
View reviewed changes
...ain/java/com/mapbox/services/android/navigation/v5/milestone/BannerInstructionMilestone.java
public class BannerInstructionMilestone extends Milestone {

private BannerInstructions instructions;
private RouteUtils routeUtils;
Copy link
Contributor

@danesfeder danesfeder Jan 16, 2019

Choose a reason for hiding this comment

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

It looks like there are a few unused methods in RouteUtils / methods only being used by tests. Can we cleanup and remove them?

danesfeder
danesfeder approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

Thanks for running with this @Guardiola31337 and the help working through the banner objects @kevinkreiser

@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch 2 times, most recently from c2c9f3f to 27397fb Compare January 16, 2019 21:09
@Guardiola31337
Copy link
Contributor Author

Guardiola31337 commented Jan 16, 2019

Also when testing, I noticed a delay with the banner showing at startup. Is this a behavior introduced by Navigator pushing the data only when tracking?

That's correct! Adding a TODO here to remove the workaround added to fix ☝️ once a fix lands in NN

  • Don't pass MapboxNavigator in NavigationRouteProcessor#buildNewRouteProgress

@danesfeder Thanks for the feedback, this is updated!

@Guardiola31337 Guardiola31337 force-pushed the pg-1271-banner-instructions-nn branch from 27397fb to 73b3675 Compare January 16, 2019 21:18
@danesfeder danesfeder added the backwards incompatible Requires a SEMVER major version change. label Jan 16, 2019
@Guardiola31337 Guardiola31337 merged commit 927e923 into master Jan 16, 2019
@Guardiola31337 Guardiola31337 deleted the pg-1271-banner-instructions-nn branch January 16, 2019 22:17
@danesfeder danesfeder mentioned this pull request Jan 16, 2019
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@danesfeder danesfeder danesfeder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Guardiola31337 Guardiola31337

Labels

backwards incompatible Requires a SEMVER major version change.

Projects

None yet

Milestone

v0.27.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Guardiola31337 @codecov-io @danesfeder