Conversation
| locationDictionary["lat"] = location.coordinate.latitude | ||
| locationDictionary["lng"] = location.coordinate.latitude | ||
| locationDictionary["altitude"] = location.altitude | ||
| locationDictionary["timestamp"] = location.timestamp |
There was a problem hiding this comment.
For some reason, I'm unable to todo location.timestamp.ISO8601 here even though location.timestamp is of type Date
There was a problem hiding this comment.
Drop static from ISO8601 and return formatter.string(from: self) should work.
| modifiedEventDictionary["platform"] = UIDevice.current.systemName | ||
| modifiedEventDictionary["operatingSystemVersion"] = "\(UIDevice.current.systemName)-\(UIDevice.current.systemVersion)" | ||
| modifiedEventDictionary["sdkIdentifier"] = "mapbox-navigation-ios" | ||
| modifiedEventDictionary["sdkVersion"] = "0.4.0" |
There was a problem hiding this comment.
There should be a better way to do this.
There was a problem hiding this comment.
Something like: Bundle.navigationUI.object(forInfoDictionaryKey: "CFBundleShortVersionString"). There should be a constant for CFBundleShort… IIRC
| class func addDefaultEvents(routeProgress: RouteProgress, sessionIdentifier: UUID) -> [String: Any] { | ||
| var modifiedEventDictionary: [String: Any] = [:] | ||
|
|
||
| modifiedEventDictionary["platform"] = UIDevice.current.systemName |
There was a problem hiding this comment.
UIDevice should be part of UIKit which this library doesn't depend on. 😕
We should adapt this for watchOS later.
There was a problem hiding this comment.
Are there any alternatives?
There was a problem hiding this comment.
We can wrap this in a compiler flag by platform and by default we can set this to unknown
E.g.
#if os(iOS)
UIApplication.shared.isIdleTimerDisabled = true
#endifThere was a problem hiding this comment.
This library is currently only being built for iOS, so #if os(iOS) is unnecessary. The issue is actually that Core Navigation avoids depending on UIKit. We should add a hook for MapboxNavigation to add to this dictionary.
|
|
||
| userLocationForRerouteEvent.append(location) | ||
| // Keep double the amount needed for reroute event since we keep collecting after the reroute | ||
| if userLocationForRerouteEvent.count >= rerouteLocationArraySize * 20 { |
| class func addDefaultEvents(routeProgress: RouteProgress, sessionIdentifier: UUID) -> [String: Any] { | ||
| var modifiedEventDictionary: [String: Any] = [:] | ||
|
|
||
| modifiedEventDictionary["platform"] = UIDevice.current.systemName |
There was a problem hiding this comment.
We can wrap this in a compiler flag by platform and by default we can set this to unknown
E.g.
#if os(iOS)
UIApplication.shared.isIdleTimerDisabled = true
#endif|
|
||
| modifiedEventDictionary["platform"] = UIDevice.current.systemName | ||
| modifiedEventDictionary["operatingSystemVersion"] = "\(UIDevice.current.systemName)-\(UIDevice.current.systemVersion)" | ||
| modifiedEventDictionary["sdkIdentifier"] = "mapbox-navigation-ios" |
There was a problem hiding this comment.
Would there be an easy way to differentiate this between MapboxCoreNavigation (mapbox-navigation-ios) and MapboxNavigation (mapbox-navigation-ios-ui)?
There was a problem hiding this comment.
This gets tricky. We could pass the event object along in the didreroute method and then have UI send off the event. But it's impossible for core to know whether UI is actually used. I think we might have to punt on this for now.
There was a problem hiding this comment.
It isn’t impossible: see the ideas in mapbox/mapbox-directions-swift#136, specifically NSClassFromString(_:) and Bundle(for:).
There was a problem hiding this comment.
Even more straightforward would be for Core Navigation to expose a hook for MapboxNavigation to tell Core Navigation that it’s present.
| var newDictionary: [String: Any] = [:] | ||
|
|
||
| for key in Array(eventDictionary.keys) { | ||
| newDictionary["\(eventPrefix)"] = eventDictionary[key] |
There was a problem hiding this comment.
I'm not sure I follow the purpose or logic of this function? It looks like here the resulting dictionary will always have only a single key:
[
"eventPrefix": <random value>
]| var countdownToPushEvent = 20 | ||
| var distanceCompleted: CLLocationDistance = 0 | ||
| var distanceRemaining: CLLocationDistance? | ||
| var durationRemaining: TimeInterval? |
There was a problem hiding this comment.
Should we rename these to lastRerouteDistanceRemaining or something to clarify that these properties are stored state from the last reroute? Or should we put these all into a single struct property for the last reroute state?
Something like
struct LastRerouteState {
var distanceRemaining: CLLocationDistance
var durationRemaining: CLLocationDistance
...etc...
}| eventDictionary["feedbackType"] = "reroute" | ||
| eventDictionary["locationsBefore"] = convertLocastionsObject(locations: Array(userLocationForRerouteEvent.prefix(rerouteIndexLocationBefore))) | ||
| eventDictionary["locationsAfter"] = convertLocastionsObject(locations: Array(userLocationForRerouteEvent.suffix(from: rerouteIndexLocationBefore))) | ||
| eventDictionary["userLocatioBeforeAfterReroute"] = userLocationForRerouteEvent |
There was a problem hiding this comment.
This property isn't in the spec so the event may be discarded by the API. What's the thought behind including this?
There was a problem hiding this comment.
Oh this is old code prior to spec outline.
| eventDictionary["distanceRemaining"] = distanceRemaining ?? nil | ||
| eventDictionary["durationRemaining"] = durationRemaining ?? nil | ||
|
|
||
| let eventDictionaryWithPrefix = MGLMapboxEvents.addEventPrefix(eventDictionary: eventDictionary, eventPrefix: "navigation.reroute") |
There was a problem hiding this comment.
We shouldn't need to prefix the event attribute keys with the event name.
| incrementRouteProgress(alertLevel, location: location, updateStepIndex: updateStepIndex) | ||
| } | ||
|
|
||
| func convertLocastionsObject(locations: [CLLocation]) -> [[String: Any]] { |
There was a problem hiding this comment.
Typo convertLocastionsObject
MapboxCoreNavigation/String.swift
Outdated
| @@ -0,0 +1,20 @@ | |||
| import UIKit | |||
There was a problem hiding this comment.
@frederoni you think this okay? I can't use UIDevice.current.systemName without it. You think hardcoding strings would be okay here?
There was a problem hiding this comment.
You should do something like:
#if os(iOS)
import UIKit
#elseif os(watchOS)
import WatchKitHowever, that could be tail work or part of #305
| import Polyline | ||
|
|
||
| extension MGLMapboxEvents { | ||
| public class func addDefaultEvents(routeProgress: RouteProgress, sessionIdentifier: UUID, sessionNumberOfReroutes: Int = 0) -> [String: Any] { |
There was a problem hiding this comment.
Is there any way we can share this function with MapboxNavigation without making it public? We need it for navigation.cancel
There was a problem hiding this comment.
Not really, because MapboxCoreNavigation and MapboxNavigation are separate modules. If you were to declare this method in an Objective-C header but implement it in Swift, it would be possible to give the header “private” membership in the MapboxCoreNavigation target, which would make it accessible to other targets in the same Xcode project, like MapboxNavigation. At least that’s true in Xcode and Carthage land. I don’t think that would work with CocoaPods.
| /** | ||
| IOS 8601 timestamp of when the `RouteController` was initialized. | ||
| */ | ||
| public let sessionStartTimestamp: String = Date().ISO8601 |
There was a problem hiding this comment.
Same question as in https://github.com/mapbox/mapbox-navigation-ios/pull/304/files#r123724999, can make this sharable with MapboxNavigation but not public?
|
Added remaining events, cancel, arrive, depart in 9a65dbf |
|
Looks good. It's mostly the private communication between core and the ui that we probably should solve. Perhaps a test case to verify the circular buffer and break out the collection of locations from |
| @@ -0,0 +1,9 @@ | |||
| extension Date { | |||
| var ISO8601: String { | |||
There was a problem hiding this comment.
Too bad ISO8601DateFormatter is only in iOS 10.0 and above.
MapboxCoreNavigation/Feedback.swift
Outdated
| import Foundation | ||
|
|
||
| public enum FeedbackType: String { | ||
| case general = "general" |
There was a problem hiding this comment.
As of Swift 3.1, string enumerations only bridge from Objective-C to Swift, not the other way around. Since Objective-C clients don’t need to access the underlying strings, the usual pattern is to have the enumeration be backed by Int and conform to CustomStringConvertible (example).
There was a problem hiding this comment.
Since this enum will not public, is it required to make it bridge-compatible?
There was a problem hiding this comment.
It’s currently impossible for an Objective-C application to call RouteController.recordFeedback(type:description:) et al., because this public type doesn’t bridge to Objective-C.
| NSLog("Sending \(eventName)") | ||
|
|
||
| event.eventDictionary["locationsBefore"] = sessionState.pastLocations.allObjects.filter({$0.timestamp <= event.timestamp }).map({$0.dictionary}) | ||
| event.eventDictionary["locationsAfter"] = sessionState.pastLocations.allObjects.filter({$0.timestamp > event.timestamp }).map({$0.dictionary}) |
There was a problem hiding this comment.
Nit: Use trailing closure syntax.
| } | ||
| } | ||
|
|
||
| class CoreFeedbackEvent: Hashable { |
There was a problem hiding this comment.
If neither this class nor any of its subclasses need to be public (to Objective-C code), we should make them structs. Swift will synthesize initializers for these structs, they’ll take up less memory, and they’re just easier to work with than value classes.
There was a problem hiding this comment.
CoreFeedbackEvent was originally a struct until we split out feedback into two separate event types. Structs don't support inheritance, thus the switch.
There was a problem hiding this comment.
Ah, in that case protocols are typically used for code sharing between struct types. However, there’s no structural difference between the two subclasses. If we expect the two types of events to diverge in the future, we can have two structs conform to a single protocol, or we can use an enumeration with associated values instead of a struct. Otherwise, a simple enumeration-typed property inside the struct would suffice.
|
|
||
| eventDictionary["secondsSinceLastReroute"] = sessionState.lastReroute != nil ? round(timestamp.timeIntervalSince(sessionState.lastReroute!)) : -1 | ||
|
|
||
| // These are placeholders until the |
| if let geometry = route.coordinates { | ||
| lastReroute.eventDictionary["newGeometry"] = Polyline(coordinates: geometry).encodedPolyline | ||
| lastReroute.eventDictionary["newDistanceRemaining"] = round(route.distance) | ||
| lastReroute.eventDictionary["newDurationRemaining"] = round(route.expectedTravelTime) |
There was a problem hiding this comment.
A [String: Any] using string literals for the keys can be rather fragile, especially for code that lacks unit tests. Let’s replace eventDictionary with strongly-typed properties for each of the expected fields, then add a method to CoreFeedbackEvent or RerouteEvent that converts to an attributes dictionary.
There was a problem hiding this comment.
Would it be possible to add some tests for at least the event generation? The spec may change a lot still so relying on tests may actually make more sense at this point (or always).
There was a problem hiding this comment.
Using stronger typing with FeedbackEvent will ensure better test coverage. In essence, we need to centralize the logic that converts from native types like Polyline into JSON types, rather than scattering that logic in multiple places, as is the case right now.
There was a problem hiding this comment.
There still aren’t any tests of the new feature. Some of the other feedback, such as #304 (comment), would no doubt be caught by basic tests.
|
|
||
| let eventName = event.eventDictionary["event"] as! String | ||
|
|
||
| NSLog("Sending \(eventName)") |
There was a problem hiding this comment.
Nit: print() is more idiomatic than NSLog() in Swift.
|
|
||
| let eventName = event.eventDictionary["event"] as! String | ||
|
|
||
| NSLog("Sending \(eventName)") |
There was a problem hiding this comment.
Remove any print statements before merging. If these print statements are really needed for debugging, we should make them debug-only, either using a compiler flag or perhaps a user default.
| extension String { | ||
| static var systemName: String { | ||
| #if os(iOS) || os(tvOS) | ||
| return UIDevice.current.systemName |
There was a problem hiding this comment.
Doesn’t this just return “iOS” or “tvOS”?
| var systemInfo = utsname() | ||
| uname(&systemInfo) | ||
| let machineMirror = Mirror(reflecting: systemInfo.machine) | ||
| let identifier = machineMirror.children.reduce("") { identifier, element in |
There was a problem hiding this comment.
How does this dark magic differ from the telemetry library implementation, which is also currently used in the map SDK?
There was a problem hiding this comment.
It's basically the same. Many of these duplicated class extensions are just stopgaps until we can leverage the full standalone telemetry framework.
|
@frederoni @1ec5 this is ready for another round of review. |
| sdkIdentifier = routeController.usesDefaultUserInterface ? "mapbox-navigation-ui-ios" : "mapbox-navigation-ios" | ||
| sdkVersion = String(describing: Bundle(for: RouteController.self).object(forInfoDictionaryKey: "CFBundleShortVersionString")!) | ||
|
|
||
| eventVersion = 2 |
There was a problem hiding this comment.
All the events produced by a particular version of this library should have the same version, right? Let’s make sure that’s the case by pulling this value out into a constant. No need for a property on the struct; just use the constant in convertedToDictionary().
There was a problem hiding this comment.
Also, I don't think this value is actually used currently. convertedToDictionary() currently sets the value with a literal value.
|
|
||
| platform = ProcessInfo.systemName | ||
| operatingSystem = "\(ProcessInfo.systemName) \(ProcessInfo.systemVersion)" | ||
| device = UIDevice.current.machine |
There was a problem hiding this comment.
platform, operatingSystem, sdkVersion, etc. can never change throughout the session, so set them in convertedToDictionary() rather than increasing the size of the struct. The fewer properties in the struct, the smaller each instance of the struct and the higher likelihood that Swift will use an efficient memory representation for it.
| var estimatedDuration: TimeInterval? | ||
| var stepCount: Int? | ||
| var created: Date | ||
| var startTimestamp: String? |
There was a problem hiding this comment.
This should be a Date, like created. Wait until convertedToDictionary() to lazily convert it to a string.
| applicationState = UIApplication.shared.applicationState.telemetryString | ||
| } | ||
|
|
||
| func convertedToDictionary() -> [String: Any] { |
There was a problem hiding this comment.
var eventDictionary or var jsonDictionary would be more descriptive.
| var screenBrightness: Int | ||
| var batteryPluggedIn: Bool | ||
| var batteryLevel: Float | ||
| var applicationState: String |
There was a problem hiding this comment.
This should be a UIApplicationState. Wait until convertedToDictionary() to lazily convert it to a string.
| */ | ||
| public func updateLastFeedback(type: FeedbackType, description: String?) { | ||
| if let lastFeedback = outstandingFeedbackEvents.filter({$0 is FeedbackEvent}).last { | ||
| lastFeedback.eventDictionary["feedbackType"] = type.rawValue |
There was a problem hiding this comment.
FeedbackType.rawValue is an integer (e.g., 0 for general). Did you mean to set this item to type.description instead?
| if let geometry = route.coordinates { | ||
| lastReroute.eventDictionary["newGeometry"] = Polyline(coordinates: geometry).encodedPolyline | ||
| lastReroute.eventDictionary["newDistanceRemaining"] = round(route.distance) | ||
| lastReroute.eventDictionary["newDurationRemaining"] = round(route.expectedTravelTime) |
There was a problem hiding this comment.
There still aren’t any tests of the new feature. Some of the other feedback, such as #304 (comment), would no doubt be caught by basic tests.
| var currentRoute: Route! | ||
| var currentRequestIdentifier: String? | ||
|
|
||
| var originalRoute: Route! |
There was a problem hiding this comment.
Implicitly unwrapped optionals are unsafe. Make this property and currentRoute non-optional and pass them in when initializing the SessionState below.
| struct SessionState { | ||
| let identifier = UUID() | ||
| var departureTimestamp: Date? | ||
| var arrivalTimestamp: Date? |
There was a problem hiding this comment.
Can we call these dates instead of timestamps? They are Dates, after all.
| var totalDistanceCompleted: CLLocationDistance = 0 | ||
|
|
||
| var numberOfReroutes = 0 | ||
| var lastReroute: Date? |
There was a problem hiding this comment.
lastRerouteDate would be clearer; otherwise, one could reasonably see lastReroute somewhere in the code and think it refers to a coordinate.
|
|
||
| if let mapboxAccessToken = mapboxAccessToken { | ||
| events.isDebugLoggingEnabled = true | ||
| events.isMetricsEnabledInSimulator = true |
There was a problem hiding this comment.
I don't think we should ship this with metrics enabled in simulator set to true. Maybe this library can define a new user default key (it would not need to be documented) that could be checked and this value could default to false and and use the value in user defaults if available?
|
@bsudekum good for a final review then I think we're ready to merge |
|
This is blocked by putting out a new release of MapboxDirections.swift which includes mapbox/mapbox-directions-swift#155. Waiting on mapbox/mapbox-directions-swift#154 also. |
Cartfile
Outdated
| @@ -1,5 +1,5 @@ | |||
| binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 3.5 | |||
| github "mapbox/MapboxDirections.swift" ~> 0.10.0 | |||
| github "mapbox/MapboxDirections.swift" ~> 0.10 | |||
There was a problem hiding this comment.
Since MapboxDirections.swift hasn’t yet reached v1.0, anything could technically change at any time. We do try to avoid backwards-compatible changes in patch releases, but we make no such guarantees about minor releases.
We should pin to a specific patch version of the library (with the tadpole operator) to avoid surprises. ~> 0.10.1 will require v0.10.x except v0.10.0.
| return EventDetails(routeController: routeController, session: routeController.sessionState).eventDictionary | ||
| } | ||
|
|
||
| func navigationDepartEvent(routeController: RouteController) -> [String: Any] { |
There was a problem hiding this comment.
Instead of a series of separate methods that each sets a different event value, we should have a single method that takes an MMEEventType as a parameter; depending on the passed-in eventType, we can add additional items to eventDictionary as needed.
| */ | ||
| public var sendNotifications: Bool = true | ||
|
|
||
| public var showsReportFeedback: Bool = false { |
There was a problem hiding this comment.
This method needs documentation.
Telem events manager needs to be made public first: https://github.com/mapbox/mapbox-gl-native/tree/bs-telem-api-> https://github.com/mapbox/mapbox-telemetry-iosThis PR adds the ground work for telemetry events and also adds the first event, rerouting. On every reroute, an event will be pushed to the event manager containing the last 20 locations prior to the reroute and up to 20 locations after the reroute. The event manager in the iOS SDK will then push all events in a single network request to the server.
The goal here is help improve all aspects of the navigation SKD and also the directions API which this SDK relies on.
/cc @1ec5 @ericrwolfe @frederoni