Remove use of LiveData for fetching DirectionsRoute and updating Location #894

danesfeder · 2018-04-27T18:43:58Z

Closes #750 Closes #877

We currently use LiveData to update new DirectionsRoutes (such as an off-route event) and Location updates (previously RouteViewModel and LocationViewModel).

This PR removes these vm's and replaces them with classes that has callbacks and can operate in the background without issue.

TODO:

Unit tests
Fix leak in NavigationCamera

danesfeder · 2018-04-27T18:44:19Z

@devotaaabel @Guardiola31337 created a new pull request after my git fail

Guardiola31337

Nice cleanup and test coverage 👏

Some comments/questions before getting in.

Guardiola31337 · 2018-04-30T20:55:15Z

...ox/services/android/navigation/testapp/activity/navigationui/EmbeddedNavigationActivity.java

+  @Override
+  public void onStart() {
+    super.onStart();
+    navigationView.onStart();


Wondering if we could use Lifecycle Architecture Component https://developer.android.com/topic/libraries/architecture/lifecycle here to remove ☝️ boilerplate code from all activities containing a NavigationView. Saw that @OnLifecycleEvents were removed as part of this PR, wondering why 🤔

Discussed this with @Guardiola31337 - making it manual for now is fine, noting that we should look into how to properly handle the Fragment architecture with the annotations.

Guardiola31337 · 2018-04-30T22:08:37Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

+   *
+   * @param onNavigationReadyCallback to be set to this view
+   */
+  public void getNavigationAsync(OnNavigationReadyCallback onNavigationReadyCallback) {


Could we revisit the method's name? We're not getting anything actually (note void). BTW, this method is doing 2 things, setting onNavigationReadyCallback and a callback object that will be triggered when the map is ready to be used. Shouldn't the latter managed somewhere else?

Guardiola31337 · 2018-04-30T22:32:29Z

...rc/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewEventDispatcher.java

+   * @param navigationViewOptions that contains all listeners to attach
+   */
+  void initializeListeners(NavigationViewOptions navigationViewOptions, MapboxNavigation navigation) {
+    setFeedbackListener(navigationViewOptions.feedbackListener());


get/set nomenclature is hijacked by the Java language and should be only used when defining POJOs/DTOs. Especially here that the methods are package-private.

Guardiola31337 · 2018-04-30T22:42:01Z

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

-  /**
-   * Adds this class as a listener for progress,
-   * milestones, and off route events.
-   */
  private void addNavigationListeners() {
    if (navigation != null) {


How could navigation be null? I guess that's not possible because we're calling addNavigationListeners from initNavigation which initializes it. Am I missing something? If not, this check could be removed.

Nope, good 👁 here @Guardiola31337

Guardiola31337 · 2018-04-30T22:46:05Z

...ain/java/com/mapbox/services/android/navigation/ui/v5/location/NavigationLocationEngine.java

+
+  private void initLocationEngine(Context context, boolean simulateRoute) {
+    if (simulateRoute) {
+      locationEngine = new MockLocationEngine(1000, 30, false);


Magic numbers.

Guardiola31337 · 2018-04-30T22:52:01Z

...src/test/java/com/mapbox/services/android/navigation/ui/v5/NavigationLocationEngineTest.java

+    NavigationLocationEngineListener mockCallback = mock(NavigationLocationEngineListener.class);
+    NavigationLocationEngine navigationLocationEngine = new NavigationLocationEngine(mockCallback);
+
+    navigationLocationEngine.initializeLocationEngine(createMockContext(), true);


IMO this is part of the Arrange part.

We should be explicit with the true that we're passing in because that's key for what we're testing here. What about extracting it to a explaining variable and name it simulationEnabled for example?

Guardiola31337 · 2018-04-30T23:02:37Z

...rc/test/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewRouteEngineTest.java

+  }
+
+  @Test
+  public void onSecondResponseReceived_routesAreCompared() throws Exception {


We should be explicit on onSecondResponseReceived somehow to know what's going on here in a glance. Are we enabling some option in buildNavigationEngineWithOptions? Wondering how this test differs from onResponseReceived_routeIsProcessed one 🤔

Guardiola31337 · 2018-04-30T23:08:02Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/route/NavigationRouteEngine.java

+   * @since 0.13.0
+   */
+  @Override
+  public void findRouteFromRouteProgress(Location location, RouteProgress routeProgress) {


We could refactor findRouteFromRouteProgress (decompose conditionals, extract blocks of code into well-named methods, etc.). We should keep public methods as clean as possible so they read like pseudocode (i.e. no conditionals, fors, etc.).

Guardiola31337 · 2018-04-30T23:09:39Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/route/NavigationRouteEngine.java

+  }
+
+  @Nullable
+  private static NavigationRoute.Builder buildRouteRequestFromCurrentLocation(Point origin, Double bearing,


Why buildRouteRequestFromCurrentLocation needs to be static? Same for addRouteProfile, addDestination, retrieveDestinationWaypoint and addWaypoints methods.

devotaaabel · 2018-05-01T17:26:26Z

...ain/java/com/mapbox/services/android/navigation/ui/v5/location/NavigationLocationEngine.java

+import com.mapbox.api.directions.v5.models.DirectionsRoute;
+import com.mapbox.services.android.navigation.v5.location.MockLocationEngine;
+
+public class NavigationLocationEngine {


Isn't this name a little consfusing? Maybe it could be NavigationLocationEngineManager or something

devotaaabel · 2018-05-01T17:29:31Z

.../main/java/com/mapbox/services/android/navigation/ui/v5/route/NavigationViewRouteEngine.java

+import java.util.List;
+import java.util.Locale;
+
+public class NavigationViewRouteEngine extends NavigationRouteEngine implements RouteEngineListener {


Again is Engine the right word here? Maybe Manager? And I'm not sure I understand this whole hierarchy (NavigationViewRouteEngine -> NavigationRouteEngine -> RouteEngine)

Guardiola31337 · 2018-05-01T17:46:07Z

...rc/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewEventDispatcher.java

+   * @param navigationViewOptions that contains all listeners to attach
+   */
+  void initializeListeners(NavigationViewOptions navigationViewOptions, MapboxNavigation navigation) {
+    updateFeedbackListener(navigationViewOptions.feedbackListener());


Are we updating the listeners? What about using assign instead?

BTW, for consistency we should use Listener or Callback across the base code. Saw that sometimes we name callbacks as Listener and sometimes as Callback. This is something that we could revisit later.

Guardiola31337 · 2018-05-01T17:54:37Z

...rc/test/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewRouteEngineTest.java

+  }
+
+  @Test
+  public void onSecondRouteResponseReceived_routesAreComparedWithDamerauLevenshteinAlgorithm() throws Exception {


👀 #894 (comment)

We should be explicit on onSecondResponseReceived somehow to know what's going on here in a glance. Are we enabling some option in buildNavigationEngineWithOptions? Wondering how this test differs from onResponseReceived_routeIsProcessed one 🤔

Guardiola31337 · 2018-05-01T17:58:04Z

...ion/src/main/java/com/mapbox/services/android/navigation/v5/route/NavigationRouteEngine.java

+    return remainingWaypoints.remove(lastWaypoint);
+  }
+
+  private static void addWaypoints(List<Point> remainingCoordinates, NavigationRoute.Builder builder) {


👀 #894 (comment)

It seems you missed this one. Any reasons?

Guardiola31337

Let's 🚢

Well done @danesfeder 🙇

Guardiola31337 · 2018-05-01T18:32:03Z

...x/services/android/navigation/testapp/activity/navigationui/fragment/NavigationFragment.java

@@ -36,7 +36,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat
    super.onViewCreated(view, savedInstanceState);
    navigationView = view.findViewById(R.id.navigation_view_fragment);
    navigationView.onCreate(savedInstanceState);
-    navigationView.getNavigationAsync(this);
+    navigationView.initialize(this);


danesfeder added bug Defect to be fixed. navigation-ui labels Apr 27, 2018

danesfeder added this to the v0.13.0 milestone Apr 27, 2018

danesfeder self-assigned this Apr 27, 2018

danesfeder requested review from Guardiola31337 and devotaaabel April 27, 2018 18:43

danesfeder force-pushed the dan-vm-arch branch from 96cd589 to e3c64bd Compare April 27, 2018 18:45

danesfeder added 2 commits April 30, 2018 13:26

Create working first pass, consolidating view models

f08ba92

Refactor NavigationRouteEngine for easier testing

012b52f

danesfeder force-pushed the dan-vm-arch branch 2 times, most recently from fad1b4c to 14d6ff2 Compare April 30, 2018 17:42

Guardiola31337 reviewed Apr 30, 2018

View reviewed changes

danesfeder force-pushed the dan-vm-arch branch from 14d6ff2 to 78bc870 Compare May 1, 2018 17:22

devotaaabel reviewed May 1, 2018

View reviewed changes

Guardiola31337 reviewed May 1, 2018

View reviewed changes

danesfeder force-pushed the dan-vm-arch branch from 78bc870 to aec328e Compare May 1, 2018 18:27

Update test app lifecycle and allow navigation to start multiple times

70f649d

danesfeder force-pushed the dan-vm-arch branch from aec328e to 70f649d Compare May 1, 2018 18:34

devotaaabel approved these changes May 1, 2018

View reviewed changes

Guardiola31337 approved these changes May 1, 2018

View reviewed changes

danesfeder merged commit e746a46 into master May 1, 2018

This was referenced May 2, 2018

Changed livedata for non-ui elements to callbacks to fix the bug with… #871

Closed

Release v0.13.0 #909

Closed

danesfeder deleted the dan-vm-arch branch June 12, 2018 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove use of LiveData for fetching DirectionsRoute and updating Location #894

Remove use of LiveData for fetching DirectionsRoute and updating Location #894

danesfeder commented Apr 27, 2018 •

edited

Loading

danesfeder commented Apr 27, 2018

Guardiola31337 left a comment

Guardiola31337 Apr 30, 2018

danesfeder May 1, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

danesfeder May 1, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

Guardiola31337 Apr 30, 2018

devotaaabel May 1, 2018

devotaaabel May 1, 2018

Guardiola31337 May 1, 2018

Guardiola31337 May 1, 2018

Guardiola31337 May 1, 2018

Guardiola31337 left a comment

Guardiola31337 May 1, 2018

Remove use of LiveData for fetching DirectionsRoute and updating Location #894

Remove use of LiveData for fetching DirectionsRoute and updating Location #894

Conversation

danesfeder commented Apr 27, 2018 • edited Loading

danesfeder commented Apr 27, 2018

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danesfeder commented Apr 27, 2018 •

edited

Loading