Add dynamic offline routing to NavigationView #1829

danesfeder · 2019-03-20T18:22:05Z

Description

This PR adds two NavigationViewOptions that will allow for initialization of offline routing within the NavigationView. The view will then detect connectivity when it's time to find a route and use either online or offline based on the current state of the device.

Fixes Added API to set offline in NavigationView #1292

What's the goal?

The goal is to encapsulate the offline logic within the NavigationView so RouteListener#allowRerouteFrom is no longer the go-to for offline with the NavigationView.

How is it being implemented?

For this initial implementation, the view will not worry about downloading data. The two view options are Strings: routing tile offline path and the version of that data. This means this data will already be on the device prior to initialization of the view.

When searching for a new route, the logic is as follows:

    if (connectivityStatus.isConnectedFast()) {
      onlineRouter.findRouteWith(builder);
      ...
    } else if (offlineRouter != null) {
      offlineRouter.findRouteWith(builder);
      ...
    } else if (connectivityStatus.isConnected()) {
      onlineRouter.findRouteWith(builder);
      ...
    }

So we will fallback to offline if we don't find a fast connection. Otherwise, we will try the slow connection with online as a last resort.

connectivityStatus.isConnectedFast() is based on the mobile connection subtype (EDGE, LTE for example). WIFI always returns true.

How has this been tested?

Unit tests added
This absolutely needs field testing.

Checklist

I have tested locally / staging (including SNAPSHOT upstream dependencies if needed)
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have added an Activity example in the test app showing the new feature implemented

codecov-io · 2019-03-20T19:00:48Z

Codecov Report

Merging #1829 into master will increase coverage by 1.46%.
The diff coverage is 71.17%.

@@             Coverage Diff              @@
##             master    #1829      +/-   ##
============================================
+ Coverage     32.14%   33.61%   +1.46%     
- Complexity      964     1015      +51     
============================================
  Files           248      254       +6     
  Lines          8577     8696     +119     
  Branches        651      660       +9     
============================================
+ Hits           2757     2923     +166     
+ Misses         5572     5513      -59     
- Partials        248      260      +12

Guardiola31337

This absolutely needs field testing.

Yeah, this definitely needs a bunch of testing.

I've left some comments to discuss / look at before merging here.

Guardiola31337 · 2019-03-20T19:54:47Z

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

+
+import java.util.HashMap;
+
+class ConnectivityStatusMap extends HashMap<Integer, Boolean> {


What about adding a HashMap as a property / create a wrapper so creating a custom one extending from HashMap is not necessary?

This is to be internal, right? Can this be refactored later if/as needed?

Straightforward refactor, took care of it 👍

Guardiola31337 · 2019-03-20T20:08:11Z

...i/src/main/java/com/mapbox/services/android/navigation/ui/v5/ConnectivityStatusProvider.java

+  }
+
+  private boolean isConnectionFast(int type, int subType) {
+    if (type == ConnectivityManager.TYPE_WIFI) {


We should be careful here because you could be connected to a Wi-Fi but that doesn't mean to have connectivity e.g. you could be connected to a hotspot and don't have connection until you log in.

I believe we should implement something similar as with mobile connectivity. Maybe 👀 Wi-Fi strength?

This is good to think about as an improvement but can possibly be deferred.

I added a wifi strength "checker" to address this

danesfeder · 2019-03-21T13:19:57Z

Per conversation with @kevinkreiser, our own timeout mechanism may make sense when offline is present with online calls. I.e. wait no longer than 5 seconds before cancelling the online call and looking at offline for the route.

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

devotaaabel · 2019-03-21T22:26:04Z

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

@@ -377,11 +369,9 @@ public void onProgressChange(Location location, RouteProgress routeProgress) {
  private OffRouteListener offRouteListener = new OffRouteListener() {
    @Override
    public void userOffRoute(Location location) {
-      if (hasNetworkConnection()) {


Is the offRouteListener only getting fired now when the device is online? This doesn't cause any issues?

Connectivity checks are now done in NavigationViewRouter with the ConnectivityStatusProvider, so this check was no longer necessary / would block the new logic.

devotaaabel · 2019-03-21T22:29:19Z

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

+
+  void findRouteWith(NavigationRoute.Builder builder) {
+    if (!isConfigured) {
+      return;


Should we give an error if not configured?

Yeah, good catch

Should we throw an exception here? As it is, this would be a somewhat silent failure.

We actually won't call this method if this class isn't configured https://github.com/mapbox/mapbox-navigation-android/pull/1829/files#diff-29009b5750e50557cd5e8a7fd79a46f7R80. So we may not need this check at all?

Yeah, let's remove it altogether then.

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

devotaaabel · 2019-03-21T22:45:59Z

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

+class RouteComparator {
+
+  private static final int FIRST_ROUTE = 0;
+  private static final int ONE_ROUTE = 1;


adjust naming for consistency

Consistent with what exactly? Sorry not following the comment

Just understood what that meant, sorry I thought they were both referring to indexes. Maybe SIZE_ONE_ROUTE would be more explicit for the second field?

devotaaabel · 2019-03-21T22:48:23Z

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

+  void compare(@NonNull DirectionsResponse response, @Nullable DirectionsRoute chosenRoute) {
+    if (isValidRoute(response)) {
+      List<DirectionsRoute> routes = response.routes();
+      DirectionsRoute bestRoute = routes.get(FIRST_ROUTE);


You could move the initialization to the call to obtainMostSimilarRoute

Adjusted the naming in RouterComparator to be more clear 👍

devotaaabel · 2019-03-21T22:50:05Z

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

+    }
+  }
+
+  private DirectionsRoute obtainMostSimilarRoute(List<DirectionsRoute> routes, DirectionsRoute currentBestRoute,


This feels redundant with findMostSimilarRoute, if you don't pass in the first route you could just get rid of this method and call the other one.

devotaaabel · 2019-03-21T22:52:22Z

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

+    int routeIndex = 0;
+    String chosenRouteLegDescription = obtainRouteLegDescriptionFrom(chosenRoute);
+    int minSimilarity = Integer.MAX_VALUE;
+    for (int index = 0; index < routes.size(); index++) {


You could make this a for each (for(DirectionsRoute route: routes)) and instead of routeIndex keep track of the route

devotaaabel · 2019-03-25T16:20:14Z

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

+
+import timber.log.Timber;
+
+class NavigationViewOfflineRouter {


We have a lot of objects called routers, since this seems to only take care of making sure that the offline router is properly configured, can we try to name it to make that apparent?

Not sure I completely follow regarding sole responsibility of configuration, findRouteWith is also finding new offline routes

Ok I understand what you are saying. Can we call this a wrapper or something? Since it is acting mostly as a wrapper for the routers.

devotaaabel · 2019-03-26T14:03:59Z

@danesfeder can you describe the architecture of the Routers/RouteFetchers? That might make it easier to understand some of your choices

danesfeder · 2019-03-27T13:02:30Z

@devotaaabel sure thing!

NavigationViewModel holds:
- NavigationViewRouter holds:
  - RouteFetcher for online routing -> onlineRouter (from libandroid-navigation)
  - NavigationViewOfflineRouter for offline routing -> offlineRouter
  - ConnectivityStatusProvider for deciding which router to use (as described in the original PR post)

devotaaabel

Looks good, my existing comments are style NITs I guess. Will approve after field testing

Guardiola31337

Other than a minor comment I think this is good to go. Let's test it out and 👀 how it looks like.

Great work here @danesfeder 💪

Guardiola31337 · 2019-03-27T20:24:35Z

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

+
+  void findRouteWith(NavigationRoute.Builder builder) {
+    if (!isConfigured) {
+      return;


Yeah, let's remove it altogether then.

danesfeder added ✓ ready for review backwards incompatible Requires a SEMVER major version change. labels Mar 20, 2019

danesfeder added this to the v0.34.0 milestone Mar 20, 2019

danesfeder self-assigned this Mar 20, 2019

danesfeder requested review from Guardiola31337 and devotaaabel March 20, 2019 18:22

danesfeder force-pushed the dan-navigationview-offline branch from d3e64e0 to 655e842 Compare March 20, 2019 18:47

Guardiola31337 reviewed Mar 20, 2019

View reviewed changes

devotaaabel reviewed Mar 21, 2019

View reviewed changes

danesfeder force-pushed the dan-navigationview-offline branch 2 times, most recently from 392d0a7 to 1df0c85 Compare March 25, 2019 15:21

devotaaabel reviewed Mar 25, 2019

View reviewed changes

This was referenced Mar 25, 2019

Check for device connectivity prior to voice instruction #1842

Closed

Allow option for offline Maps sideload with dynamic tile fetching based on route line #1843

Closed

danesfeder force-pushed the dan-navigationview-offline branch from 1df0c85 to 10deec9 Compare March 25, 2019 19:04

devotaaabel reviewed Mar 27, 2019

View reviewed changes

Guardiola31337 approved these changes Mar 27, 2019

View reviewed changes

danesfeder force-pushed the dan-navigationview-offline branch from 10deec9 to 636a03e Compare March 28, 2019 19:47

Add dynamic offline routing to NavigationView

27053bb

danesfeder force-pushed the dan-navigationview-offline branch from 636a03e to 27053bb Compare March 28, 2019 20:32

danesfeder merged commit e7a9e42 into master Mar 28, 2019

danesfeder deleted the dan-navigationview-offline branch March 28, 2019 20:48

Guardiola31337 removed this from the v0.34.0 milestone Apr 2, 2019

Guardiola31337 added this to the v0.35.0 milestone Apr 2, 2019

Guardiola31337 pushed a commit that referenced this pull request Apr 2, 2019

Add dynamic offline routing to NavigationView (#1829)

4a397a6

Guardiola31337 pushed a commit that referenced this pull request Apr 2, 2019

Add dynamic offline routing to NavigationView (#1829)

bf9e8d6

Guardiola31337 mentioned this pull request Apr 3, 2019

Add offline options to Navigation Launcher #1862

Merged

6 tasks

Guardiola31337 mentioned this pull request Apr 12, 2019

Release v0.35.0 #1882

Closed

11 tasks

Guardiola31337 modified the milestones: v0.35.0, v0.36.0 Apr 12, 2019

danesfeder mentioned this pull request Apr 15, 2019

Create NavigationViewRouter timeout to unblock routing state #1888

Merged

6 tasks

Guardiola31337 mentioned this pull request Apr 17, 2019

Release 0.36.0 #1897

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic offline routing to NavigationView #1829

Add dynamic offline routing to NavigationView #1829

danesfeder commented Mar 20, 2019 •

edited

Loading

codecov-io commented Mar 20, 2019 •

edited

Loading

Guardiola31337 left a comment

Guardiola31337 Mar 20, 2019

akitchen Mar 22, 2019

danesfeder Mar 22, 2019

Guardiola31337 Mar 20, 2019

akitchen Mar 22, 2019

danesfeder Mar 22, 2019

danesfeder commented Mar 21, 2019

devotaaabel Mar 21, 2019

danesfeder Mar 22, 2019

devotaaabel Mar 21, 2019

danesfeder Mar 22, 2019

devotaaabel Mar 25, 2019

danesfeder Mar 25, 2019

Guardiola31337 Mar 27, 2019

devotaaabel Mar 21, 2019

danesfeder Mar 22, 2019

devotaaabel Mar 22, 2019

devotaaabel Mar 21, 2019

danesfeder Mar 22, 2019

devotaaabel Mar 21, 2019

devotaaabel Mar 21, 2019

devotaaabel Mar 25, 2019

danesfeder Mar 25, 2019

devotaaabel Mar 26, 2019

devotaaabel commented Mar 26, 2019

danesfeder commented Mar 27, 2019

devotaaabel left a comment

Guardiola31337 left a comment

Guardiola31337 Mar 27, 2019


		import java.util.HashMap;

		class ConnectivityStatusMap extends HashMap<Integer, Boolean> {


		import timber.log.Timber;

		class NavigationViewOfflineRouter {

Add dynamic offline routing to NavigationView #1829

Add dynamic offline routing to NavigationView #1829

Conversation

danesfeder commented Mar 20, 2019 • edited Loading

Description

What's the goal?

How is it being implemented?

How has this been tested?

Checklist

codecov-io commented Mar 20, 2019 • edited Loading

Codecov Report

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

danesfeder commented Mar 21, 2019

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

devotaaabel commented Mar 26, 2019

danesfeder commented Mar 27, 2019

devotaaabel left a 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 Mar 20, 2019 •

edited

Loading

codecov-io commented Mar 20, 2019 •

edited

Loading