Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offline routing #1768

Merged
merged 22 commits into from
Oct 29, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cartfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.3
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 3.2
github "mapbox/MapboxDirections.swift" ~> 0.24
github "mapbox/MapboxDirections.swift" ~> 0.24.1
github "mapbox/turf-swift" ~> 0.2
github "mapbox/mapbox-events-ios" ~> 0.6
github "ceeK/Solar" ~> 2.1.0
Expand Down
2 changes: 1 addition & 1 deletion MapboxCoreNavigation.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Pod::Spec.new do |s|
s.module_name = "MapboxCoreNavigation"

s.dependency "MapboxNavigationNative", "~> 3.2.0"
s.dependency "MapboxDirections.swift", "~> 0.24.0" # Always pin to a patch release if pre-1.0
s.dependency "MapboxDirections.swift", "~> 0.24.1" # Always pin to a patch release if pre-1.0
s.dependency "MapboxMobileEvents", "~> 0.6.0" # Always pin to a patch release if pre-1.0
s.dependency "Turf", "~> 0.2.0" # Always pin to a patch release if pre-1.0

Expand Down
7 changes: 7 additions & 0 deletions MapboxCoreNavigation/MBRouteController.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#import <Foundation/Foundation.h>
#import <MapboxNavigationNative/MapboxNavigationNative.h>

/**
Posted when `MBRouteController` receives a user location update representing movement along the expected route.
Expand Down Expand Up @@ -82,3 +83,9 @@ extern const MBRouteControllerNotificationUserInfoKey MBRouteControllerIsProacti
@interface NSString (MD5)
- (NSString * _Nonnull)md5;
@end

@interface MBNavigator (additions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: category names should be in camel case just like class names. A conventional category name would be MBCoreNavigationAdditions (since both MapboxNavigationNative and MapboxCoreNavigation use the same class prefix).


- (NSUInteger)setupRouter:(NSString *)tilesPath translationsPath:(NSString *)translationsPath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: “set up” here is being used as a verb, so it should be setUpRouter:.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation would be great here. It wasn’t obvious to me that the return value is a tile count. From the implementation of this method, I initially got the impression that it was an error code of some sort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not relevant any longer. configureRouter is not wrapped in setUpRouter but instead called directly from an initializer in Swift which is documented.


@end
17 changes: 17 additions & 0 deletions MapboxCoreNavigation/MBRouteController.m
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

NSString *const MBErrorDomain = @"ErrorDomain";

typedef NS_ENUM(NSInteger, MBNavigationError) {
MBErrorCodeUnknown = -1,
};

@implementation NSString (MD5)
- (NSString * _Nonnull)md5 {
const char *cStr = [self UTF8String];
Expand All @@ -30,3 +34,16 @@ - (NSString * _Nonnull)md5 {
return output;
}
@end

@implementation MBNavigator (additions)

- (NSUInteger)setupRouter:(NSString *)tilesPath translationsPath:(NSString *)translationsPath {

@try {
return [self configureRouterForTilesPath:tilesPath translationsPath:translationsPath];
} @catch (NSException *exception) {
return -1;
Copy link
Contributor

@1ec5 1ec5 Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we’re trying to run around unexpected exceptions coming out of MapboxNavigationNative. If -configureRouterForTilesPath:translationsPath: has expected failure modes, it should be the responsibility of MapboxNavigationNative to catch an exception and turn it into a C-style return code and NSError out parameter.

By default, throwing an exception in Objective-C causes a leak, even under ARC, because the expectation is that an exception is a catastrophic error that leads to an immediate crash anyways. That’s one reason Objective-C programmers generally avoid @try-@catch, even in cases where C++ programmers might rely on it for control flow. It doesn’t leak in Objective-C++, but whether the @throw happens in Objective-C or Objective-C++ is none of this SDK’s business.

Copy link
Contributor Author

@frederoni frederoni Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning NSErrors from Obj-C has a really poor bridgeability to Swift’s "throws". Noticed that this try-catch block doesn't suppress the underlying exception so I ended up removing it altogether. It's just a fallback for the kind of resource the offline router is looking for. (1: Uncompress tiles, 2: Compressed tiles, 3: Different structure of compressed tiles). It's not a fatal error but one that gets caught if you have "Catch all Exceptions" enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning NSErrors from Obj-C has a really poor bridgeability to Swift’s "throws".

Unless I’m mistaken, the most idiomatic method signature would include an NSError ** out parameter, which would bridge to Swift as a throws, but it wouldn’t return an error.

Copy link
Contributor Author

@frederoni frederoni Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the most idiomatic method signature would include an NSError ** out parameter

this is correct but there are some limitations, a boolean is the only supported primitive return type:

- (void)voidNoThrow:(int)arg error:(NSError **)error;      // Bridges to NSErrorPointer
- (NSUInteger)intNoThrow:(int)arg error:(NSError **)error; // Bridges to NSErrorPointer
- (BOOL)boolThrow:(int)arg error:(NSError **)error;        // Throws idiomatically

However, there seems to be a consensus about using -fno-exceptions eventually, partially due to bloated binary size but the inevitable leak in Obj-C is also a good point. Either way, this try-catch block is already removed.

}
}

@end
116 changes: 116 additions & 0 deletions MapboxCoreNavigation/OfflineDirections.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import Foundation
import MapboxDirections
import MapboxNavigationNative

public typealias OfflineDirectionsCompletionHandler = (_ numberOfTiles: UInt) -> Void

public enum OfflineRoutingError: Error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this enumeration bridge to Objective-C? If so, it needs a class prefix in Objective-C (and documentation).

Copy link
Contributor Author

@frederoni frederoni Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors are casted to NSErrors but I guess it needs a localized description key? Or does Error bridge fine to Obj-C since Swift 4.0? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, the localized description keys would need to be provided somehow, though I’m not sure exactly how that works when relying on bridging from Error in Swift.

case unexpectedRouteResult
case corruptRouteData
case responseError(String)
}

struct OfflineDirectionsConstants {
static let offlineSerialQueueLabel = Bundle.mapboxCoreNavigation.bundleIdentifier!.appending(".offline")
static let serialQueue = DispatchQueue(label: OfflineDirectionsConstants.offlineSerialQueueLabel)
}

@objc(MBOfflineDirectionsProtocol)

This comment was marked as resolved.

public protocol OfflineDirectionsProtocol {

/**
Initializes a newly created directions object with an optional access token and host.

- parameter accessToken: A Mapbox [access token](https://www.mapbox.com/help/define-access-token/). If an access token is not specified when initializing the directions object, it should be specified in the `MGLMapboxAccessToken` key in the main application bundle’s Info.plist.
- parameter host: An optional hostname to the server API. The [Mapbox Directions API](https://www.mapbox.com/api-documentation/?language=Swift#directions) endpoint is used by default.
- parameter tilesPath: The location where the tiles has been sideloaded to.
- parameter translationsPath: The location where the translations has been sideloaded to.
*/
init(accessToken: String?, host: String?, tilesPath: String, translationsPath: String, completionHandler: @escaping OfflineDirectionsCompletionHandler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s a bit odd that optional arguments come before required arguments. Can tilesPath and translationsPath come first?


/**
Begins asynchronously calculating the route or routes using the given options and delivers the results to a closure.

This method retrieves the routes asynchronously via MapboxNavigationNative.

Routes may be displayed atop a [Mapbox map](https://www.mapbox.com/maps/). They may be cached but may not be stored permanently. To use the results in other contexts or store them permanently, [upgrade to a Mapbox enterprise plan](https://www.mapbox.com/directions/#pricing).

- parameter options: A `RouteOptions` object specifying the requirements for the resulting routes.
- parameter completionHandler: The closure (block) to call with the resulting routes. This closure is executed on the application’s main thread.
*/
func calculateOffline(_ options: RouteOptions, completionHandler: @escaping Directions.RouteCompletionHandler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method name doesn’t quite sound grammatical, since RouteOptions is not an offline. Would it be possible to combine this functionality with the existing method provided by MapboxDirections.swift using a method signature like calculate(_:whileOffline:completionHandler:)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combining sounds good but I think we can be sparse with the prepositions since it doesn't read fluently anyway.

}

@objc(MBMapboxOfflineDirections)
public class MapboxOfflineDirections: Directions, OfflineDirectionsProtocol {

required public init(accessToken: String?, host: String?, tilesPath: String, translationsPath: String, completionHandler: @escaping OfflineDirectionsCompletionHandler) {

super.init(accessToken: accessToken, host: host)

OfflineDirectionsConstants.serialQueue.sync {
let tilesPath = tilesPath.replacingOccurrences(of: "file://", with: "")
let translationsPath = translationsPath.replacingOccurrences(of: "file://", with: "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it look like tilesPath and translationsPath aren’t actually paths but rather file URL strings. If so, we should stick to the URL type rather than String and call them URLs.

let tileCount = self.navigator.setupRouter(tilesPath, translationsPath: translationsPath)

DispatchQueue.main.async {
completionHandler(tileCount)
}
}
}

public func calculateOffline(_ options: RouteOptions, completionHandler: @escaping Directions.RouteCompletionHandler) {

let url = self.url(forCalculating: options)

OfflineDirectionsConstants.serialQueue.sync { [weak self] in

guard let result = self?.navigator.getRouteForDirectionsUri(url.absoluteString) else {
return completionHandler(nil, nil, OfflineRoutingError.unexpectedRouteResult as NSError)
}

guard let data = result.json.data(using: .utf8) else {
return completionHandler(nil, nil, OfflineRoutingError.corruptRouteData as NSError)
}

do {
let json = try JSONSerialization.jsonObject(with: data, options: []) as! [String: Any]
if let errorValue = json["error"] as? String {
DispatchQueue.main.async {
let error = NSError(domain: "..", code: 102, userInfo: [NSLocalizedDescriptionKey: errorValue])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unusual domain. Can it be replaced with a constant that reliably identifies this SDK?

return completionHandler(nil, nil, error)
}
} else {
let response = options.response(from: json)

DispatchQueue.main.async {
return completionHandler(response.0, response.1, nil)
}
}

} catch {
DispatchQueue.main.async {
return completionHandler(nil, nil, error as NSError)
}
}
}
}

var _navigator: MBNavigator!
var navigator: MBNavigator {

assert(currentQueueName() == OfflineDirectionsConstants.offlineSerialQueueLabel,
"The offline navigator must be accessed from the dedicated serial queue")

if _navigator == nil {
self._navigator = MBNavigator()
}

return _navigator
}
}

fileprivate func currentQueueName() -> String? {
let name = __dispatch_queue_get_label(nil)
return String(cString: name, encoding: .utf8)
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PODS:
- Mapbox-iOS-SDK (4.5.0)
- MapboxCoreNavigation (0.23.0):
- MapboxDirections.swift (~> 0.24.0)
- MapboxDirections.swift (~> 0.24.1)
- MapboxMobileEvents (~> 0.6.0)
- MapboxNavigationNative (~> 3.2.0)
- Turf (~> 0.2.0)
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading