Add route overview button and animation to NavigationView by danesfeder · Pull Request #967 · mapbox/mapbox-navigation-android

danesfeder · 2018-05-23T15:14:56Z

Closes #950

TODO:

Add button with chosen asset from @cjballard
Show resume button after overview clicked
Add test for dynamic camera providing list of Point

Animation running wired up to the X button:

cc @brsbl

Cjballard-zz · 2018-05-24T19:46:02Z

routepreview_icons.zip

Attaching light and dark icons for route preview. @danesfeder

brsbl-zz · 2018-05-24T21:06:30Z

Love the dynamic camera.

@danesfeder is it easy enough to adjust the spacing between the sound and feedback icons in this PR as well so they better match iOS? the thinking being grouping things that obscure the map together makes them less intrusive overall -- thoughts @cjballard?

also:

Cut a ticket in /mapbox-navigation-ios to mirror this behavior 🙂

danesfeder · 2018-05-24T21:13:54Z

@brsbl sure thing and thanks! - would like @cjballard 👀 on this as well. I have them spaced a little more to create larger touch points. Hitting that feedback button accidentally when you're trying to mute is pretty jarring

Cjballard-zz · 2018-05-24T21:16:27Z

@brsbl @danesfeder Agreed. I think there's a middle ground we can hit that still reserves those touchpoints while grouping them more tightly and revealing more of the map. Maybe 10-20dp closer together could work.

danesfeder · 2018-06-05T14:57:37Z

@cjballard Here's what the current setup looks like, let me know what you think / want to tweak 👍

`Portrait`

`Landscape`

Cjballard-zz · 2018-06-05T15:17:26Z

@danesfeder Looks good to me! Only adjustment I might make is to make the separating line a bit shorter, closer to the height (but still a little taller) than the route preview icon:

danesfeder · 2018-06-05T20:48:14Z

@cjballard Updated:

Cjballard-zz · 2018-06-05T21:01:51Z

@danesfeder Looks good to me 👍

danesfeder · 2018-06-05T21:32:14Z

@cjballard awesome, thank you!

@Guardiola31337 @devotaaabel will add tests to this PR tomorrow then will be good for 👀

danesfeder · 2018-06-06T14:23:44Z

Tests added - this is good for 👀

danesfeder · 2018-06-07T15:07:56Z

Merge is causing issues with padding adjustments - this PR is no longer ready for review until these issues are fixed.

Guardiola31337

Overall this look awesome 💯 Great job @danesfeder 👏
Really nice test coverage ❤️

Some minor comments.

Guardiola31337 · 2018-06-15T16:33:29Z

        navigationPresenter.onRecenterClick();
      }
    });
+    routeOverviewBtn.setOnClickListener(new OnClickListener() {


What about extracting the listeners into class properties?

Guardiola31337 · 2018-06-15T16:37:53Z

  }

+  @NonNull
+  private RouteInformation buildRouteInformationFromRouteProgress(RouteProgress routeProgress) {


Minor thing - IMO buildRouteInformationFrom(RouteProgress routeProgress) would be clearer and to the point. What do you think?

Guardiola31337 · 2018-06-15T16:44:52Z

+  private void animateCameraForRouteOverview(RouteInformation routeInformation, int[] padding) {
+    Camera cameraEngine = navigation.getCameraEngine();
+
+    CameraPosition resetPosition = new CameraPosition.Builder().tilt(0).bearing(0).build();


We can move these two lines after the guard clause avoiding instantiating them if invalidPoints is true.

Guardiola31337 · 2018-06-15T16:47:54Z

+    if (invalidPoints) {
+      return;
+    }
+    final LatLngBounds routeBounds = convertRoutePointsToLatLngBounds(routePoints);


What about extracting this preparation block of code into a well-named private method? (Including the two lines mentioned above ☝️)

Guardiola31337 · 2018-06-15T17:09:05Z

+    ImageButton routeOverviewBtn = findViewById(R.id.routeOverviewBtn);
+    boolean isDarkThemeEnabled = ThemeSwitcher.isDarkThemeEnabled(getContext());
+    Drawable routeOverviewDrawable;
+    if (isDarkThemeEnabled) {


Could you add a custom attribute to the nav theme so it's just added upon inflation and remove the if else check?

Guardiola31337 · 2018-06-15T17:21:28Z

+      LineString lineString = LineString.fromPolyline(route.geometry(), Constants.PRECISION_6);
+      return lineString.coordinates();
+    }
+    return null;


We should consider returning a Null Object instead or even better try to getting rid of it at all (maybe returning an empty list?). If not, code becomes defensive and full of null checks.

👀 Null References: The Billion Dollar Mistake

Actually if the route in setupLineStringAndBearing is null L#85 will cause a NPE right?

In any case, guard clauses normally are at the beginning of the method representing the unexpected behavior.

Also saw the same exact code in DynamicCameraTest.java (generateRouteCoordinates). Should be abstracted and accessible to the implementation classes?

Guardiola31337 · 2018-06-15T17:34:58Z


+  @Test
+  public void onInformationFromRoute_engineCreatesOverviewPointList() throws Exception {
+    MapboxMap mapboxMap = mock(MapboxMap.class);


Minor thing - Could we extract some shared "arrangement" code into private methods and used it across the tests? This applies to MapWaynameTest as well.

danesfeder · 2018-06-18T20:59:55Z

@Guardiola31337 Updated - I skipped your comment on MapWaynameTest - the variables that are out of the builders are being used to provide context / used in the actual tests so they cannot be pushed into the builders.

Guardiola31337

@danesfeder Some minor new comments from the latest changes.

Guardiola31337 · 2018-06-19T10:49:15Z

+import com.mapbox.mapboxsdk.maps.MapboxMap;
+import com.mapbox.services.android.navigation.ui.v5.utils.ViewUtils;
+
+public class NavigationSnapshotReadyCallback implements MapboxMap.SnapshotReadyCallback {


Access can be package-private

Guardiola31337 · 2018-06-19T10:49:21Z

+  private NavigationView navigationView;
+  private NavigationViewModel navigationViewModel;
+
+  public NavigationSnapshotReadyCallback(NavigationView navigationView, NavigationViewModel navigationViewModel) {


Access can be package-private

Guardiola31337 · 2018-06-19T10:51:15Z

+    boolean darkThemeEnabled = isDarkThemeEnabled(context);
    updatePreferencesDarkEnabled(context, darkThemeEnabled);

    // Check for custom theme from NavigationLauncher


Is this comment necessary?

Guardiola31337 · 2018-06-19T10:59:26Z

  }

+  @NonNull
+  private RouteInformation buildRouteInformationFrom(RouteProgress routeProgress) {


#967 (comment)

Didn't see that we had other buildRouteInformationFrom methods. Maybe we should leave as it was. Sorry for the confusion.

Guardiola31337 · 2018-06-19T11:01:05Z

+    if (invalidPoints) {
+      return;
+    }
+    CameraUpdate resetUpdate = buildResetCameraUpdate();


#967 (comment)

Guardiola31337 · 2018-06-19T11:01:38Z

+    CameraUpdate resetUpdate = buildResetCameraUpdate();
+    final CameraUpdate overviewUpdate = buildOverviewCameraUpdate(padding, routePoints);
+
+    mapboxMap.animateCamera(resetUpdate, 150, new MapboxMap.CancelableCallback() {


What about extracting the listeners into class properties?

Guardiola31337 · 2018-06-19T11:03:09Z

    return response.routes().get(0);
  }
+
+  private List<Point> generateRouteCoordinates(DirectionsRoute route) {


#967 (comment)

Guardiola31337 · 2018-06-19T11:07:03Z

  }

-  public static float dpToPx(Context context, int dp) {
+  public static int[] buildRouteOverviewPadding(Context context) {


This doesn't sound like a View utility. It's only used in NavigationView. IMO, objects and methods should live/be where their context is. From my experience utility classes become a drawer easily and we should treat them carefully.

Guardiola31337 · 2018-06-19T11:11:55Z

  }

+  @Override
+  public List<Point> overview(RouteInformation routeInformation) {


IMO overview is getting too long.
What about extracting blocks of code into well-named private methods and simplifying conditional expressions somehow?

We should keep public methods as clean as possible so they read like pseudocode (i.e. no conditionals, fors, etc.).

Guardiola31337

Well done @danesfeder

danesfeder added ⚠️ DO NOT MERGE PR should not be merged! platform parity Required to keep on par with iOS. topic: camera labels May 23, 2018

danesfeder added this to the 0.14.0 milestone May 23, 2018

danesfeder self-assigned this May 23, 2018

danesfeder requested review from Guardiola31337 and devotaaabel May 23, 2018 15:14

brsbl-zz mentioned this pull request May 24, 2018

Improve route overview button mapbox/mapbox-navigation-ios#1455

Closed

danesfeder modified the milestones: 0.14.0, 0.15.0 May 30, 2018

danesfeder force-pushed the dan-route-overview branch 3 times, most recently from 9528c6b to 7ece39c Compare June 5, 2018 13:07

danesfeder force-pushed the dan-route-overview branch from 2ae7846 to d60a781 Compare June 5, 2018 20:47

danesfeder force-pushed the dan-route-overview branch 3 times, most recently from 386c01d to d0236e2 Compare June 6, 2018 14:23

danesfeder added the ✓ ready for review label Jun 6, 2018

danesfeder removed the ⚠️ DO NOT MERGE PR should not be merged! label Jun 6, 2018

danesfeder changed the title ~~[WIP] Add route overview button and animation to NavigationView~~ Add route overview button and animation to NavigationView Jun 6, 2018

danesfeder added ⚠️ DO NOT MERGE PR should not be merged! and removed ✓ ready for review labels Jun 7, 2018

danesfeder force-pushed the dan-route-overview branch 4 times, most recently from c6c9676 to 35966d2 Compare June 8, 2018 20:11

danesfeder added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jun 11, 2018

danesfeder force-pushed the dan-route-overview branch 2 times, most recently from 1ccb3e9 to 717f146 Compare June 13, 2018 16:23

Guardiola31337 reviewed Jun 15, 2018

View reviewed changes

danesfeder force-pushed the dan-route-overview branch 4 times, most recently from 5cb0ecc to 74d0d02 Compare June 18, 2018 20:58

danesfeder force-pushed the dan-route-overview branch from 74d0d02 to a5af67c Compare June 18, 2018 21:01

Guardiola31337 reviewed Jun 19, 2018

View reviewed changes

danesfeder force-pushed the dan-route-overview branch 2 times, most recently from 63409b0 to 47c98ea Compare June 19, 2018 14:14

Add Route overview button

512390e

danesfeder force-pushed the dan-route-overview branch from 47c98ea to 512390e Compare June 19, 2018 14:24

Guardiola31337 approved these changes Jun 19, 2018

View reviewed changes

danesfeder merged commit abe86a5 into master Jun 19, 2018

danesfeder deleted the dan-route-overview branch June 19, 2018 14:46

danesfeder mentioned this pull request Jun 21, 2018

Release v0.15.0 #1049

Closed

11 tasks

Conversation

danesfeder commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cjballard-zz commented May 24, 2018

Uh oh!

brsbl-zz commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danesfeder commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cjballard-zz commented May 24, 2018

Uh oh!

danesfeder commented Jun 5, 2018

Portrait

Landscape

Uh oh!

Cjballard-zz commented Jun 5, 2018

Uh oh!

danesfeder commented Jun 5, 2018

Uh oh!

Cjballard-zz commented Jun 5, 2018

Uh oh!

danesfeder commented Jun 5, 2018

Uh oh!

danesfeder commented Jun 6, 2018

Uh oh!

danesfeder commented Jun 7, 2018

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danesfeder commented Jun 18, 2018

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

danesfeder commented May 23, 2018 •

edited

Loading

brsbl-zz commented May 24, 2018 •

edited

Loading

danesfeder commented May 24, 2018 •

edited

Loading

`Portrait`

`Landscape`