Skip to content

Use last BannerInstruction for arrival event #943

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
danesfeder merged 2 commits into from
May 31, 2018
Merged

Use last BannerInstruction for arrival event #943

danesfeder merged 2 commits into from
May 31, 2018

Conversation

@danesfeder
Copy link
Contributor

@danesfeder danesfeder commented May 15, 2018 •
edited
Loading

Closes #926

TODO:

  • Update failing tests, using production test fixture

cc @akitchen

@danesfeder danesfeder added this to the 0.14.0 milestone May 15, 2018
@danesfeder danesfeder self-assigned this May 15, 2018
@danesfeder danesfeder added the ⚠️ DO NOT MERGE PR should not be merged! label May 15, 2018
@danesfeder danesfeder changed the title Use last BannerInstruction for arrival event [WIP] Use last BannerInstruction for arrival event May 15, 2018
@danesfeder danesfeder force-pushed the dan-arrival-update branch 2 times, most recently from 526fe42 to 5d4efa0 Compare May 30, 2018 13:41
@danesfeder danesfeder changed the title [WIP] Use last BannerInstruction for arrival event Use last BannerInstruction for arrival event May 30, 2018
@danesfeder danesfeder added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels May 30, 2018
@danesfeder danesfeder force-pushed the dan-arrival-update branch 2 times, most recently from 0167cc0 to abfa6de Compare May 30, 2018 14:33
@danesfeder danesfeder modified the milestones: 0.14.0, 0.15.0 May 30, 2018
@danesfeder danesfeder force-pushed the dan-arrival-update branch from abfa6de to db1a0af Compare May 30, 2018 16:51
devotaaabel
devotaaabel reviewed May 30, 2018
View reviewed changes
Copy link
Contributor

@devotaaabel devotaaabel left a comment

Choose a reason for hiding this comment

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

A lot of the refactoring looks good! Though I had a couple of things to bring up, especially about how we're testing.

...ation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewModel.java Outdated
}
}

private void sendEventIsRunning(boolean isRunning) {
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

Should this be sendEventIfRunning? It's kind of a confusing name, maybe sendStatusEvent?

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/RouteUtils.java Outdated
if (!(milestone instanceof BannerInstructionMilestone)) {
return false;
}
if (upcomingStepIsArrival(routeProgress) || currentStepIsArrival(routeProgress)) {
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

Can you explain what this is doing/why? Maybe this can be refactored to make it more clear

libandroid-navigation/src/test/java/com/mapbox/services/android/navigation/v5/BaseTest.java
}

protected RouteProgress buildDefaultTestRouteProgress(DirectionsRoute testRoute) throws Exception {
return routeProgressBuilder.buildDefaultTestRouteProgress(testRoute);
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

I don't like BaseTest being the place to put all of our builders for test objects, can't the test classes call the TestBuilders directly? Also, I think we need to decide between Test..Builder and ...Faker for naming for consistency because they seem to be doing the same thing

...src/test/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationTest.java Outdated
mock(LocationEngine.class));
}

private int searchVoiceInstructionMilestone(MapboxNavigation navigation) {
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

searchForVoiceInstructionMilestone?

...src/test/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationTest.java Outdated
return identifier;
}

private int searchBannerInstructionMilestone(MapboxNavigation navigation) {
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

searchForBannerInstructionMilestone?

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationEventDispatcherTest.java
100, 100, lastStepIndex, 0);
navigationEventDispatcher.onMilestoneEvent(routeProgress, instruction, bannerInstructionMilestone);

verify(arrivalListener, times(0)).onArrival(routeProgress);
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

Shouldn't this be broken into two different tests? Since you're verifying two different things

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationEventDispatcherTest.java Outdated
navigationEventDispatcher.onProgressChange(location, routeProgressDidNotArrive);
verify(eventListeners, times(1)).onRouteProgressUpdate(routeProgressDidNotArrive);
verify(arrivalListener, times(0)).onArrival(location, routeProgressDidNotArrive);
navigationEventDispatcher.onMilestoneEvent(routeProgress, instruction, bannerInstructionMilestone);
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

If you make RouteUtils non-static and "inject" it, then you can mock it and shouldn't have to deal with all of the complexity of creating all of this (i.e., mock routeProgress and routeUtils, and add when(routeUtils.isArrivalEvent(routeProgress)).thenReturn(true), etc.)

...navigation/src/test/java/com/mapbox/services/android/navigation/v5/utils/RouteUtilsTest.java

boolean isArrivalEvent = RouteUtils.isArrivalEvent(theRouteProgress);
public void isArrivalEvent_returnsTrueWhenManeuverTypeIsArrival_andIsLastInstruction() throws Exception {
DirectionsRoute route = buildTestDirectionsRoute();
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

shouldn't these all be mocked instead of test objects? Because really, this is reliant on the other implementations to work (DirectionsRoute, RouteLeg, etc.)

@danesfeder danesfeder force-pushed the dan-arrival-update branch 2 times, most recently from ae988ed to 2a448d1 Compare May 30, 2018 21:08
devotaaabel
devotaaabel approved these changes May 30, 2018
View reviewed changes
Copy link
Contributor

@devotaaabel devotaaabel left a comment

Choose a reason for hiding this comment

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

Looks good, that's exactly what I meant, but see comment.

...ain/java/com/mapbox/services/android/navigation/v5/navigation/NavigationEventDispatcher.java Outdated
progressChangeListeners = new CopyOnWriteArrayList<>();
offRouteListeners = new CopyOnWriteArrayList<>();
fasterRouteListeners = new CopyOnWriteArrayList<>();
routeUtils = new RouteUtils();
Copy link
Contributor

@devotaaabel devotaaabel May 30, 2018

Choose a reason for hiding this comment

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

You can replace the first constructor with NavigationEventDispatcher() { this(new RouteUtils()); } so you avoid the duplicate code and the chance that the two constructors diverge

@danesfeder danesfeder force-pushed the dan-arrival-update branch from 2a448d1 to da26b8c Compare May 31, 2018 12:32
@danesfeder danesfeder merged commit c03867e into master May 31, 2018
@danesfeder danesfeder deleted the dan-arrival-update branch May 31, 2018 13:51
@danesfeder danesfeder mentioned this pull request May 31, 2018
Closed
13 tasks
@danesfeder danesfeder mentioned this pull request Jun 12, 2018
Merged
@akitchen
Copy link

akitchen commented Jul 19, 2018

@dsilvera thanks for the feedback -- could you please provide a real-world example for investigation?

@dsilvera
Copy link

dsilvera commented Jul 19, 2018

@akitchen It works finaly.

@akitchen
Copy link

akitchen commented Jul 19, 2018

We would still like to understand your frustration or what event caused you to reply here. Please open a new ticket with more details if you could, we would really appreciate it!

@dsilvera
Copy link

dsilvera commented Jul 19, 2018

I'm not frustrated, you do an excellent job ;)

@danesfeder danesfeder mentioned this pull request Sep 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Guardiola31337 Guardiola31337 Awaiting requested review from Guardiola31337

1 more reviewer

@devotaaabel devotaaabel devotaaabel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@danesfeder danesfeder

Labels

None yet

Projects

None yet

Milestone

0.15.0

Development

Successfully merging this pull request may close these issues.

Arrival event should be keyed off of arrival instructions from the server

4 participants

@danesfeder @akitchen @dsilvera @devotaaabel