Route tile download #1559

devotaaabel · 2018-11-27T18:14:53Z

I am still implementing changes we talked about including the update to navigator 3.4.3, adding better error reporting, and changing the public API to not include RoutingTileDownloadManager and instead add a startDownload method in MapboxOfflineRouter. Also going to add some simple unit tests.

… example

codecov-io · 2018-11-28T00:43:06Z

Codecov Report

Merging #1559 into master will decrease coverage by 0.19%.
The diff coverage is 3.84%.

@@             Coverage Diff             @@
##             master    #1559     +/-   ##
===========================================
- Coverage     22.75%   22.56%   -0.2%     
- Complexity      689      694      +5     
===========================================
  Files           182      189      +7     
  Lines          8102     8230    +128     
  Branches        599      606      +7     
===========================================
+ Hits           1844     1857     +13     
- Misses         6075     6190    +115     
  Partials        183      183

Guardiola31337 · 2018-11-28T14:45:44Z

During local testing, I run into the following 💥

    --------- beginning of crash
11-28 09:40:34.869 17608-17608/com.mapbox.services.android.navigation.testapp E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.mapbox.services.android.navigation.testapp, PID: 17608
    java.lang.RuntimeException: Unable to destroy activity {com.mapbox.services.android.navigation.testapp/com.mapbox.services.android.navigation.testapp.example.ui.ExampleActivity}: java.lang.ClassCastException: com.mapbox.services.android.navigation.v5.navigation.camera.SimpleCamera cannot be cast to com.mapbox.services.android.navigation.ui.v5.camera.DynamicCamera
        at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:3831)
        at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:3849)
        at android.app.ActivityThread.-wrap5(ActivityThread.java)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1398)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:148)
        at android.app.ActivityThread.main(ActivityThread.java:5417)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
     Caused by: java.lang.ClassCastException: com.mapbox.services.android.navigation.v5.navigation.camera.SimpleCamera cannot be cast to com.mapbox.services.android.navigation.ui.v5.camera.DynamicCamera
        at com.mapbox.services.android.navigation.testapp.example.ui.ExampleViewModel.shutdown(ExampleViewModel.kt:164)
        at com.mapbox.services.android.navigation.testapp.example.ui.ExampleViewModel.onCleared(ExampleViewModel.kt:97)
        at android.arch.lifecycle.ViewModelStore.clear(ViewModelStore.java:56)
        at android.support.v4.app.FragmentActivity.onDestroy(FragmentActivity.java:387)
        at android.support.v7.app.AppCompatActivity.onDestroy(AppCompatActivity.java:209)
        at com.mapbox.services.android.navigation.testapp.example.ui.ExampleActivity.onDestroy(ExampleActivity.kt:86)
        at android.app.Activity.performDestroy(Activity.java:6407)
        at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1142)
        at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:3818)
        at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:3849) 
        at android.app.ActivityThread.-wrap5(ActivityThread.java) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1398) 
        at android.os.Handler.dispatchMessage(Handler.java:102) 
        at android.os.Looper.loop(Looper.java:148) 
        at android.app.ActivityThread.main(ActivityThread.java:5417) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

Nexus 5 - Android 6.0

Also I want to note that we're not requesting the location updates after granting the permissions - there's no location icon in the status bar as shown above.

devotaaabel · 2018-11-28T14:54:00Z

@Guardiola31337 can you put that in a separate ticket? That looks unrelated to anything in this PR.

Guardiola31337 · 2018-11-28T15:14:18Z

@devotaaabel

can you put that in a separate ticket? That looks unrelated to anything in this PR.

I brought this up because it's working in master. My guess is that we're messing up with the storage permissions added as part of this PR.

danesfeder

@devotaaabel this is looking great! 👍 I left some comments for updates / questions that we can discuss.

One thing I noticed regarding the test app, it seems the app isn't currently aware of changes from settings when resuming. So going from online to offline results in no route found:

I think this is due to the offline router being created as null during initialization:

danesfeder · 2018-11-28T19:46:18Z

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

@@ -452,6 +454,11 @@ private void generateFeatureCollectionList(List<DirectionsRoute> directionsRoute
    // Each route contains traffic information and should be recreated considering this traffic
    // information.
    for (int i = 0; i < directionsRoutes.size(); i++) {
+
+      Timber.d("DirectionsRoutes.size " + directionsRoutes.size());
+      Timber.d("direction route " + directionsRoutes.get(i));


Can we remove these logs?

libandroid-navigation/src/main/AndroidManifest.xml

danesfeder · 2018-11-28T19:52:46Z

...vigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/OfflineData.java

+    CONFIGURE_ROUTER_RESULT_SUCCESS(true), CONFIGURE_ROUTER_RESULT_FAILURE(false);
+
+    private final boolean success;
+    Status(boolean success) {


Is the success boolean needed here? It seems you may be able to infer success or failure based on the Status value itself.

...ain/java/com/mapbox/services/android/navigation/v5/navigation/OfflineRouteRetrievalTask.java

danesfeder · 2018-11-28T20:04:49Z

...n/src/main/java/com/mapbox/services/android/navigation/v5/navigation/RouteFoundCallback.java

+   */
+  void routesFound(List<DirectionsRoute> routes);
+
+  void onError(OfflineData offlineData);


We renamed this to RouteFoundCallback but it looks like the the error callback here is specific to offline with OfflineData. Are we using this callback anywhere that isn't for an offline purpose? (necessitating the change to a more generic name).

It's being used here https://github.com/mapbox/mapbox-navigation-android/blob/devota-route-tile-download/app/src/main/java/com/mapbox/services/android/navigation/testapp/example/ui/navigation/ExampleRouteFinder.kt#L46 I think for consistency and being able to implement one callback for both online and offline routes it is nice to have one for both, maybe we could also put RouteFoundCallback here? https://github.com/mapbox/mapbox-navigation-android/blob/devota-route-tile-download/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRoute.java#L79 But about the OfflineData, I'd like to talk about that, so we can hopefully come to a consensus about the best way to track errors, that was just my first attempt at an error reporting object.

.../src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxOfflineRouter.java

danesfeder · 2018-11-28T20:47:21Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/TileUnpacker.java

+    NavigationLibraryLoader.load();
+  }
+
+  TileUnpacker() {


Does it make sense to pass the Navigator created in the MapboxOfflineRouter to the RouteTileDownloader and then ultimately here in the TileUnpacker?

danesfeder · 2018-11-28T20:47:55Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/TileUnpacker.java

+   */
+  void unpack(File src, String destPath, UnpackUpdateTask.ProgressUpdateListener updateListener) {
+    Log.d("source path", "" + src.getAbsolutePath());
+    Log.d("dest path", "" + destPath);


Should we remove these logs?

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

danesfeder · 2018-11-28T21:38:32Z

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

+    long size = tar.length();
+
+    while (tar.length() > 0) {
+      publishProgress((((tar.length() / size)) * 100));


I noticed that the published progress here is either 0 or 100 - was this expected behavior?

@Guardiola31337 said he had some ideas about a buffer to monitor this? @kevinkreiser any ideas what the issue is with monitoring the tar unpacking? Thanks!

im going to take a stab at fixing this!

Guardiola31337

@devotaaabel this is looking good - some questions and comments to discuss 👍

.../src/main/java/com/mapbox/services/android/navigation/v5/navigation/ConfigureRouterTask.java

Guardiola31337 · 2018-11-28T22:08:06Z

.../src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxOfflineRouter.java

   *                 can be called safely.
   */
-  public void initializeOfflineData(String tilesDirPath, OnOfflineDataInitialized callback) {
-    offlineNavigator.configure(tilesDirPath, callback);
+  public void initializeOfflineData(String version, OnOfflineDataInitialized callback) {


Minor comment / question
Have we considered using initializeData instead of initializeOfflineData? We're in the context of MapboxOfflineRouter so adding Offline sounds redundant to me.

@Guardiola31337 I actually think configureRouter would make the most sense here, because you are configuring it be ready to use the version you specified. Do we know why we went with initializeData?

@devotaaabel yeah! In that case I'd name it configure (initialize also could work) because we're in the context of MapboxOfflineRouter, no need to add Router again IMO.

Guardiola31337 · 2018-11-28T22:08:26Z

.../src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxOfflineRouter.java

@@ -33,8 +48,17 @@ public void initializeOfflineData(String tilesDirPath, OnOfflineDataInitialized
   * @return the offline {@link DirectionsRoute}
   */
  @Nullable
-  public void findOfflineRoute(@NonNull OfflineRoute route,
-                               OfflineRouteFoundCallback callback) {
+  public void findOfflineRoute(@NonNull OfflineRoute route, RouteFoundCallback callback) {


Same here re: Offline redundancy

.../src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxOfflineRouter.java

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

...main/java/com/mapbox/services/android/navigation/v5/navigation/OnOfflineDataInitialized.java

Guardiola31337 · 2018-11-28T22:51:42Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/TileUnpacker.java

+  private final Navigator navigator;
+
+  static {
+    NavigationLibraryLoader.load();


I believe this is re-loading NN unnecessarily because it's already done when instantiating OfflineNavigator which happens when creating a MapboxOfflineRouter. I guess it's safe to remove it here.

Guardiola31337 · 2018-11-28T22:56:29Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/UnpackerTask.java

+
+  @Override
+  protected File doInBackground(String... strings) {
+    navigator.unpackTiles(strings[0], strings[1]);


Shouldn't navigator.unpackTiles be synchronized?

@Guardiola31337 nope I asked, @kevinkreiser said it can be considered a static method

Guardiola31337 · 2018-11-28T22:58:29Z

...d-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/DownloadTask.java

+        outputStream.flush();
+        return file;
+
+      } catch (IOException exception) {


We're currently catching IOException twice 👇 I believe that's not necessary.

@Guardiola31337 the finally block throws an IOException

Guardiola31337 · 2018-11-28T23:01:59Z

...d-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/DownloadTask.java

+public class DownloadTask extends AsyncTask<ResponseBody, Void, File> {
+
+  private static final int END_OF_FILE_DENOTER = -1;
+  private static int instructionNamingInt = 0;


Have we considered making instructionNamingInt non static?

* fix progressUpdateListener for tile unpacking * lint

…ctivity

…1571)

…-navigation-android into devota-route-tile-download

danesfeder · 2018-11-30T01:34:09Z

Amazing work here @devotaaabel, this was no small effort. Huge milestone for the SDK 🚀

Guardiola31337 · 2018-11-30T01:36:16Z

Amazing work here @devotaaabel, this was no small effort. Huge milestone for the SDK 🚀

I second that. Kudos @devotaaabel

Added offline tile downloading and added offline capabilities to main…

fc31f9c

… example

devotaaabel added the feature New feature request. label Nov 27, 2018

devotaaabel added this to the v0.24.0 milestone Nov 27, 2018

devotaaabel self-assigned this Nov 27, 2018

devotaaabel requested review from Guardiola31337 and danesfeder November 27, 2018 18:14

Devota Aabel added 6 commits November 27, 2018 17:03

asdf

e98c8d0

Removed unused changes

b88f25b

Refactoring

77c02b9

Added request for External Storage Permissions

407b264

Refactoring and cleanup

2e1d401

Updated snapshot

8d0734b

Merge branch 'master' into devota-route-tile-download

a331cf1

Devota Aabel added 2 commits November 28, 2018 13:06

asdf

a75920f

Cleanup

b58c1b9

danesfeder suggested changes Nov 28, 2018

View reviewed changes

Guardiola31337 reviewed Nov 28, 2018

View reviewed changes

Devota Aabel and others added 9 commits November 29, 2018 08:32

Merge branch 'master' into devota-route-tile-download

787a955

Cleanup and responded to PR comments

b8787f6

Merge branch 'master' into devota-route-tile-download

397f80b

Update MapboxOfflineRoute APIs and callback naming (#1569)

ee99426

Fix the progressUpdateListener for Tile Pack Unpacking (#1567)

21e491d

* fix progressUpdateListener for tile unpacking * lint

Added permissions check to OfflineRegionDownloadActivity and ExampleA…

45ef43d

…ctivity

Update OfflineTileVersions visibility and add to MapboxOfflineRouter (#…

b502f54

…1571)

Merge branch 'devota-route-tile-download' of github.com:mapbox/mapbox…

911f402

…-navigation-android into devota-route-tile-download

Added permissions check to SettingsActivity

336290e

Devota Aabel and others added 10 commits November 29, 2018 19:01

Fixed checkstyle and crash

a083240

Add error handling for MapboxOfflineRouter callbacks (#1572)

5f2c53c

cleanup download task (#1573)

582573b

Updated navigator version and fixed crash and route fetching bugs

7752c95

Merge branch 'devota-route-tile-download' of github.com:mapbox/mapbox…

9e39fb6

…-navigation-android into devota-route-tile-download

revert static unique id changes from download task

8925aba

remove unnecessary debugging logs

6a55c84

Fix camera tracking in ExampleActivity

4b1ee56

Merge branch 'devota-route-tile-download' of github.com:mapbox/mapbox…

d607017

…-navigation-android into devota-route-tile-download

fix checkstyle

8aa538e

danesfeder approved these changes Nov 30, 2018

View reviewed changes

devotaaabel merged commit b46b7b7 into master Nov 30, 2018

devotaaabel deleted the devota-route-tile-download branch November 30, 2018 01:51

Guardiola31337 mentioned this pull request Nov 30, 2018

Release v0.24.0 #1574

Closed

12 tasks

Guardiola31337 mentioned this pull request Dec 10, 2018

Add support for offline region downloading #1485

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route tile download #1559

Route tile download #1559

devotaaabel commented Nov 27, 2018

codecov-io commented Nov 28, 2018 •

edited

Loading

Guardiola31337 commented Nov 28, 2018 •

edited

Loading

devotaaabel commented Nov 28, 2018

Guardiola31337 commented Nov 28, 2018

danesfeder left a comment

danesfeder Nov 28, 2018

danesfeder Nov 28, 2018

danesfeder Nov 28, 2018

devotaaabel Nov 29, 2018

danesfeder Nov 28, 2018

danesfeder Nov 28, 2018

danesfeder Nov 28, 2018

kevinkreiser Nov 29, 2018

Guardiola31337 left a comment

Guardiola31337 Nov 28, 2018

devotaaabel Nov 29, 2018

Guardiola31337 Nov 29, 2018

Guardiola31337 Nov 28, 2018

Guardiola31337 Nov 28, 2018

Guardiola31337 Nov 28, 2018

devotaaabel Nov 29, 2018

Guardiola31337 Nov 28, 2018

devotaaabel Nov 29, 2018

Guardiola31337 Nov 28, 2018

danesfeder commented Nov 30, 2018

Guardiola31337 commented Nov 30, 2018

Route tile download #1559

Route tile download #1559

Conversation

devotaaabel commented Nov 27, 2018

codecov-io commented Nov 28, 2018 • edited Loading

Codecov Report

Guardiola31337 commented Nov 28, 2018 • edited Loading

devotaaabel commented Nov 28, 2018

Guardiola31337 commented Nov 28, 2018

danesfeder 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

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

danesfeder commented Nov 30, 2018

Guardiola31337 commented Nov 30, 2018

codecov-io commented Nov 28, 2018 •

edited

Loading

Guardiola31337 commented Nov 28, 2018 •

edited

Loading