Add silent waypoints support into NavigationRoute #1733

Guardiola31337 · 2019-02-08T23:58:38Z

Edited:

Adds ~~via~~ silent waypoints support into NavigationRoute

Note: current branch is using MAS SNAPSHOT - MAS 4.4.0 needs to be released and the SNAPSHOT dependency removed before this PR can merge.

cc @osana

danesfeder

@Guardiola31337 some minor comments, thanks for running with this!

danesfeder · 2019-02-11T14:56:20Z

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRoute.java


+      String waypoints = options.waypoints();
+      if (!TextUtils.isEmpty(waypoints)) {
+        Integer[] splittedWaypointIndices = parseWaypointIndices(waypoints);


NIT splitted > split

danesfeder · 2019-02-11T14:57:58Z

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRoute.java

+     * @param waypoints integer array of coordinate indices to be used as waypoints
+     * @return this builder for chaining options together
+     */
+    public Builder addWaypoints(@Nullable @IntRange(from = 0) Integer... waypoints) {


It may be better to be more specific with our naming here, as we already have a addWaypoint. Maybe addSilentWaypoint or addViaWaypoint?

Yeah, I couldn't agree more. Will fix 🚀

danesfeder · 2019-02-11T18:20:13Z

@Guardiola31337 any ideas what's going on with proguard in CI? 🤔

codecov-io · 2019-02-12T00:30:33Z

Codecov Report

Merging #1733 into master will increase coverage by 0.09%.
The diff coverage is 89.47%.

@@             Coverage Diff              @@
##             master    #1733      +/-   ##
============================================
+ Coverage     29.91%   30.01%   +0.09%     
  Complexity      843      843              
============================================
  Files           220      220              
  Lines          8134     8147      +13     
  Branches        627      629       +2     
============================================
+ Hits           2433     2445      +12     
  Misses         5471     5471              
- Partials        230      231       +1

Guardiola31337 · 2019-02-12T05:12:03Z

Thanks for the feedback @danesfeder this is updated and ready for another round of 👀

any ideas what's going on with proguard in CI? 🤔

It seems something weird happened with the previous build. It's working now ¯\_(ツ)_/¯

danesfeder · 2019-02-12T12:53:19Z

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRoute.java

      if (!TextUtils.isEmpty(waypointTargets)) {
-        Point[] splittedWaypointTargets = parseWaypointTargets(waypointTargets);
-        directionsBuilder.addWaypointTargets(splittedWaypointTargets);
+        Point[] splitdWaypointTargets = parseWaypointTargets(waypointTargets);


NIT splitd is not a word: split I think is what you're looking for

BTW, good catch! Will fix 👍

danesfeder

Thanks for these updates @Guardiola31337, let's merge when 4.4.0 is good to go 🚢

Guardiola31337 · 2019-02-13T00:46:47Z

Found 👇 during CI Robo Tests

java.lang.NullPointerException: Attempt to invoke virtual method 'java.util.List com.mapbox.api.directions.v5.models.DirectionsResponse.routes()' on a null object reference
FATAL EXCEPTION: ControllerMessenger
Process: com.mapbox.services.android.navigation.app, PID: 13547
java.lang.NullPointerException: Attempt to invoke virtual method 'java.util.List com.mapbox.api.directions.v5.models.DirectionsResponse.routes()' on a null object reference
	at com.mapbox.services.android.navigation.v5.navigation.NavigationRoute$1.onResponse(NavigationRoute.java:98)
	at com.mapbox.api.directions.v5.MapboxDirections$1.onResponse(MapboxDirections.java:129)
	at retrofit2.ExecutorCallAdapterFactory$ExecutorCallbackCall$1$1.run(ExecutorCallAdapterFactory.java:70)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at androidx.test.espresso.base.Interrogator.a(Interrogator.java:19)
	at androidx.test.espresso.base.UiControllerImpl.a(UiControllerImpl.java:164)
	at androidx.test.espresso.base.UiControllerImpl.a(UiControllerImpl.java:156)
	at androidx.test.espresso.base.UiControllerImpl.a(UiControllerImpl.java:34)
	at androidx.test.espresso.action.MotionEvents.a(MotionEvents.java:76)
	at androidx.test.espresso.action.MotionEvents.a(MotionEvents.java:51)
	at androidx.test.espresso.action.Tap.c(Tap.java:8)
	at androidx.test.espresso.action.Tap.a(Tap.java:18)
	at androidx.test.espresso.action.Tap$1.b(Tap.java:3)
	at androidx.test.espresso.action.GeneralClickAction.perform(GeneralClickAction.java:22)
	at androidx.test.espresso.ViewInteraction$SingleExecutionViewAction.perform(ViewInteraction.java:9)
	at androidx.test.espresso.ViewInteraction.a(ViewInteraction.java:79)
	at androidx.test.espresso.ViewInteraction.a(ViewInteraction.java:96)
	at androidx.test.espresso.ViewInteraction$1.call(ViewInteraction.java:3)
	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:135)
	at android.app.ActivityThread.main(ActivityThread.java:5254)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)

It seems it happened randomly when jumping between activities fastly i.e. Activity A makes a request 👉 navigate to a different Activity B quickly (so the response callback didn't have enough time to be fired) 👉 when the response listener is called the body is null 👉 💥

Should we add a null check in NavigationRoute to avoid this? It seems like a race condition because of the async requests' nature and how fast Robo tests jump through the different activities. I wasn't able to reproduce this locally.

Guardiola31337 · 2019-02-13T01:01:34Z

Should we add a null check in NavigationRoute to avoid this?

Actually, I think it's even a better approach because routes is marked as @NonNull so we don't need to check if response.body().routes() is null.

@danesfeder @devotaaabel have you seen a response with null routes before? Couldn't find anything but I wanted to double check with you that the changes make sense before merging here.

danesfeder

@Guardiola31337 thanks for the updates here and the logic adjustment for the valid route check 🚢

Guardiola31337 added ⚠️ DO NOT MERGE PR should not be merged! ✓ ready for review feature New feature request. labels Feb 8, 2019

Guardiola31337 added this to the v0.29.0 milestone Feb 8, 2019

Guardiola31337 self-assigned this Feb 8, 2019

Guardiola31337 requested review from danesfeder and devotaaabel February 8, 2019 23:58

danesfeder reviewed Feb 11, 2019

View reviewed changes

Guardiola31337 mentioned this pull request Feb 11, 2019

Fix via waypoints APIs naming mapbox/mapbox-java#961

Merged

Guardiola31337 force-pushed the pg-1730-via-waypoints branch from a02d79e to 37ed39e Compare February 12, 2019 00:10

danesfeder reviewed Feb 12, 2019

View reviewed changes

Guardiola31337 force-pushed the pg-1730-via-waypoints branch from 37ed39e to 3c2371c Compare February 12, 2019 14:23

Guardiola31337 changed the title ~~Add via waypoints support into NavigationRoute~~ Add silent waypoints support into NavigationRoute Feb 12, 2019

Guardiola31337 force-pushed the pg-1730-via-waypoints branch from 3c2371c to f3bd299 Compare February 12, 2019 18:15

danesfeder approved these changes Feb 12, 2019

View reviewed changes

Guardiola31337 force-pushed the pg-1730-via-waypoints branch from f3bd299 to b3b9184 Compare February 12, 2019 23:24

Guardiola31337 removed blocked: upstream ⚠️ DO NOT MERGE PR should not be merged! labels Feb 12, 2019

add waypoints support into navigation route

e76d3ce

Guardiola31337 force-pushed the pg-1730-via-waypoints branch from b3b9184 to e76d3ce Compare February 13, 2019 00:48

danesfeder approved these changes Feb 13, 2019

View reviewed changes

Guardiola31337 merged commit 6bf87c7 into master Feb 13, 2019

Guardiola31337 deleted the pg-1730-via-waypoints branch February 13, 2019 16:47

Guardiola31337 mentioned this pull request Feb 13, 2019

Release v0.29.0 #1737

Closed

12 tasks

Guardiola31337 mentioned this pull request Mar 1, 2019

Feature Request - Waypoints as "Vias" #875

Closed

1ec5 mentioned this pull request Mar 4, 2019

Silent waypoints lost when rerouting #1781

Closed

Guardiola31337 mentioned this pull request Apr 5, 2019

How do i use silent Waypoints with mapmatching #1851

Closed

Add silent waypoints support into NavigationRoute #1733

Add silent waypoints support into NavigationRoute #1733

Uh oh!

Conversation

Guardiola31337 commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

danesfeder Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

danesfeder Feb 11, 2019 • edited by akitchen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

danesfeder commented Feb 11, 2019

Uh oh!

codecov-io commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Guardiola31337 commented Feb 12, 2019

Uh oh!

danesfeder Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Guardiola31337 commented Feb 13, 2019

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Guardiola31337 commented Feb 8, 2019 •

edited

Loading

danesfeder Feb 11, 2019 •

edited by akitchen

Loading

codecov-io commented Feb 12, 2019 •

edited

Loading

Guardiola31337 commented Feb 13, 2019 •

edited

Loading