Offline Tile downloader #303

frederoni · 2018-10-23T12:01:42Z

Introduced Offline Tile downloader

Added two new public methods:

Directions.fetchAvailableVersions(completionHandler:)
Directions.downloadTiles(in:version:completionHandler:)

Documentation
Bundle a small region for unit testing
Unit test downloading
Deploy api-routing-tiles to production
~~Unit test download progress~~ (Add Pause/Resume and progress updates to Directions.downloadTiles #306)
~~Unit test resume download~~ (Add Pause/Resume and progress updates to Directions.downloadTiles #306)

frederoni · 2018-10-23T12:30:46Z

MapboxDirections/OfflineDirections.swift

+}
+
+@objc(MBOfflineDirections)
+public class OfflineDirections: NSObject, URLSessionDownloadDelegate {


We could probably fold this into Directions if we mention offline in the signature and/or documentation.
@1ec5 what’s your take on this?

akitchen · 2018-10-24T15:24:58Z

MapboxDirections/OfflineDirections.swift

+
+        return session.downloadTask(with: url) { [weak self] (tempLocalUrl, response, error) in
+            if let tempLocalUrl = tempLocalUrl, error == nil {
+                // TODO: Move file from temporary location to a provided location


akitchen · 2018-10-26T16:50:29Z

MapboxDirections/OfflineDirections.swift

+    }
+
+    public func urlSession(_ session: URLSession, downloadTask: URLSessionDownloadTask, didResumeAtOffset fileOffset: Int64, expectedTotalBytes: Int64) {
+        // TODO: resume download when range headers are supported


Can we add support for this in advance of the feature availability on the server?

1ec5 · 2018-10-28T04:23:34Z

MapboxDirections/OfflineDirections.swift

+     Fetches the available versions.
+     */
+    @discardableResult
+    func availableOfflineVersions(completionHandler: @escaping OfflineVersionsHandler) -> URLSessionDataTask


Since this method returns a task rather than the offline versions themselves, rename it to getAvailableOfflineVersions(completionHandler:).

get is only used when getting values indirectly (such as UIColor.getRed(_:&green:&blue:&alpha:&)) in Cocoa but I get the point. How about fetch/list/find?

Fetch is fine.

1ec5 · 2018-10-28T04:26:26Z

MapboxDirections/MBBoundingBox.swift

+ A bounding box represents a geographic region.
+ */
+@objc(MBBoundingBox)
+public class BoundingBox: NSObject, Codable {


The map SDK has a similar struct named MGLCoordinateBounds, and MapboxGeocoder.swift has a CLRegion subclass named RectangularRegion. It would be great if we could make this class a struct, for value semantics; otherwise, a subclass of CLRegion would at least make some of the capabilities more discoverable.

Hm, yeah, it's unfortunate that MapboxDirections doesn't depend on those. Would be nice to share a core set of data types, similar to what gl-native is doing.

I went back and forth with CLRegion but didn't find it adding any value. CoordinateBounds makes sense for consistency. How about {MB}/CoordinateBounds?

Swift struct wouldn't bridge to Obj-C.

Turf-swift has a BoundingBox but we are partially striving for turf-js/GeoJSON terminology there.

GeoJSON terminology makes more sense for a library that’s very much about manipulating GeoJSON, such as Turf, but unless we have this library depend on Turf – not necessarily a bad idea – I kind of like CoordinateBounds for consistency with the map SDK. I agree that CLRegion isn’t adding much in this case; MapboxGeocoder.swift has that to be more similar to MapKit’s and Core Location’s geocoders.

Swift struct wouldn't bridge to Obj-C.

A C struct does bridge to Swift, so implement it in an Objective-C header. path could be a C function refined by a Swift-only extension method on this struct.

1ec5 · 2018-10-28T04:29:02Z

MapboxDirections/MBDirections.swift

+    internal var offlineProgressHandler: OfflineDownloaderProgressHandler?
+
+    /// The handler that reports the completion of an offline tile pack
+    internal var offlineCompletionHandler: OfflineDownloaderCompletionHandler?


It’s a bit awkward that these properties are here in the main class definition because of an extension’s conformance to OfflineDirectionsProtocol.

More significantly, these properties make Directions stateful. Directions originally was stateful, going so far as to store the URLSessionTask for the API request, similar to MapKit’s MKDirections. But that made it impossible for a single Directions object to process multiple requests simultaneously, and it also meant that the object had to be held strongly for the duration of the request. #47 moved Directions to a stateless singleton model that has provided a good deal of flexibility to this library. If we’re going to return to the old model, Directions would need to encapsulate the URLSessionTask objects and provide a method for canceling the request.

I agree. Reverted back to a stateless version of Directions with an optional URLSession you can pass in to modify configurations and receive delegate callbacks such as progress updates.

MapboxDirections.swift takes the approach of exposing the request URL publicly, so you can hook the library into any networking library, not just URLSession but also AFNetworking etc.

1ec5 · 2018-10-28T04:38:13Z

MapboxDirections/OfflineDirections.swift

+}
+
+@objc(MBVersion)
+public class Version: NSObject, Codable {


There would be somewhat less boilerplate if we made Version a struct instead. It would have to be defined in Objective-C in order to support both languages.

Good point. The version should be represented as a String according to the api team.

MapboxDirections/OfflineDirections.swift

+    func availableVersionsURL() -> URL {
+
+        let url = apiEndpoint.appendingPathComponent("route-tiles/v1").appendingPathComponent("versions")
+        var components = URLComponents(string: url.absoluteString)


1ec5 · 2018-10-28T04:49:14Z

MapboxDirections/OfflineDirections.swift

+
+    static var dateFormatter: DateFormatter = {
+        let formatter = DateFormatter()
+        formatter.dateFormat = "yyyy-MM-dd"


On the off-chance that there needs to be two versions on the same day, would the API ever assign a more precise timestamp?

1ec5

Mostly stylistic nits remaining, but I do think we should try to use a C struct for the bounding box type, because reference semantics are unexpected for that concept (except for CLRegion).

1ec5 · 2018-10-31T17:54:29Z

MapboxDirections/OfflineDirections.swift

+
+extension Directions: OfflineDirectionsProtocol {
+
+    func availableVersionsURL() -> URL {


Turn this method into a computed variable if possible.

1ec5 · 2018-10-31T17:56:28Z

MapboxDirections/OfflineDirections.swift

+        return components!.url!
+    }
+
+    func tilesURL(for boundingBox: BoundingBox, version: OfflineVersion) -> URL {


Consider replacing for with a more descriptive label, such as covering, or rename the method to url(forTilesIn:version:).

1ec5 · 2018-10-31T17:59:27Z

MapboxDirections/OfflineDirections.swift

+
+    @discardableResult
+    @objc
+    public func downloadTiles(for boundingBox: BoundingBox,


downloadTiles(in:….

1ec5 · 2018-10-31T18:02:21Z

MapboxDirections/OfflineDirections.swift

+    @objc
+    public func downloadTiles(for boundingBox: BoundingBox,
+                              version: OfflineVersion,
+                              session: URLSession? = URLSession.shared,


Even more flexible would be to expose url(forTilesIn:version:) publicly, so that developers can use the URL with either a custom URLSession or another networking library such as AFNetworking.

frederoni added 4 commits October 23, 2018 13:47

Add BoundingBox

cea48d7

Add Offline Directions tests

a4df9b7

Add OfflineDirections and version availability checker

a466868

Add documentation and handle errors more gracefully

09d61c3

frederoni commented Oct 23, 2018

View reviewed changes

frederoni added 2 commits October 23, 2018 14:38

Fix access level

5b65c5f

Update cache key

91f2a21

akitchen requested a review from 1ec5 October 23, 2018 18:38

Refactor

b7bccf8

akitchen reviewed Oct 24, 2018

View reviewed changes

Clean up TODOs and unit test downloading tar

f4733d8

frederoni force-pushed the fred/offline-tile-downloader branch from 66ee09f to f4733d8 Compare October 25, 2018 12:22

Replace bloated fixture

0f44aa7

frederoni mentioned this pull request Oct 26, 2018

Offline routing mapbox/mapbox-navigation-ios#1768

Merged

5 tasks

akitchen reviewed Oct 26, 2018

View reviewed changes

1ec5 reviewed Oct 28, 2018

View reviewed changes

frederoni added 3 commits October 29, 2018 17:05

Version is string, not Date

9e1c3f7

Test suggested filename

6eda225

Resolve against base url

4292f7e

devotaaabel mentioned this pull request Oct 30, 2018

As an end developer I want to choose between online and offline routing in the Navigation View mapbox/mapbox-navigation-android#1285

Closed

Switch to production

4536af7

frederoni force-pushed the fred/offline-tile-downloader branch from 9277b0f to ac1b11e Compare October 30, 2018 21:26

Make Directions stateless

99228a4

frederoni force-pushed the fred/offline-tile-downloader branch from ac1b11e to 99228a4 Compare October 30, 2018 21:31

1ec5 approved these changes Oct 31, 2018

View reviewed changes

frederoni mentioned this pull request Nov 1, 2018

Add Pause/Resume and progress updates to Directions.downloadTiles #306

Closed

frederoni force-pushed the fred/offline-tile-downloader branch from b5b9806 to 08d69de Compare November 1, 2018 15:06

frederoni mentioned this pull request Nov 1, 2018

Offline Example mapbox/mapbox-navigation-ios#1803

Merged

6 tasks

Documentation and naming conventions

fc491a4

frederoni force-pushed the fred/offline-tile-downloader branch from 08d69de to fc491a4 Compare November 1, 2018 15:17

frederoni merged commit 46828bf into master Nov 1, 2018

1ec5 mentioned this pull request Nov 1, 2018

Prepare 0.25.0 release #307

Merged

frederoni mentioned this pull request Nov 1, 2018

Offline routing mapbox/mapbox-navigation-ios#1808

Merged

7 tasks

1ec5 mentioned this pull request Nov 23, 2018

Fix parameter documentation #317

Merged

1ec5 mentioned this pull request Feb 15, 2019

CoordinateBounds mixes up its corners, breaking offline tile downloads #348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offline Tile downloader #303

Offline Tile downloader #303

frederoni commented Oct 23, 2018 •

edited

Loading

frederoni Oct 23, 2018

akitchen Oct 24, 2018

akitchen Oct 26, 2018

1ec5 Oct 28, 2018

frederoni Oct 29, 2018 •

edited

Loading

1ec5 Oct 31, 2018

1ec5 Oct 28, 2018

frederoni Oct 29, 2018 •

edited

Loading

frederoni Oct 30, 2018 •

edited

Loading

1ec5 Oct 31, 2018 •

edited

Loading

1ec5 Oct 28, 2018

frederoni Oct 30, 2018

1ec5 Oct 31, 2018

1ec5 Oct 28, 2018

frederoni Oct 29, 2018

This comment was marked as resolved.

1ec5 Oct 28, 2018

1ec5 left a comment

1ec5 Oct 31, 2018

1ec5 Oct 31, 2018

1ec5 Oct 31, 2018

1ec5 Oct 31, 2018


		extension Directions: OfflineDirectionsProtocol {

		func availableVersionsURL() -> URL {

Offline Tile downloader #303

Offline Tile downloader #303

Conversation

frederoni commented Oct 23, 2018 • edited Loading

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

frederoni Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

frederoni Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

1ec5 Oct 31, 2018 • edited Loading

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

This comment was marked as resolved.

Choose a reason for hiding this comment

1ec5 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

frederoni commented Oct 23, 2018 •

edited

Loading

frederoni Oct 29, 2018 •

edited

Loading

frederoni Oct 29, 2018 •

edited

Loading

frederoni Oct 30, 2018 •

edited

Loading

1ec5 Oct 31, 2018 •

edited

Loading