-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add spoken locale #235
Add spoken locale #235
Conversation
MapboxDirections/MBRoute.swift
Outdated
/** | ||
The locale to use for spoken instructions. | ||
*/ | ||
@objc open var spokenLocale: Locale? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this property to speechLocale
or voiceLocale
to better emphasize that this locale gets plugged into MapboxSpeech or the Voice API.
MapboxDirections/MBDirections.swift
Outdated
@@ -173,6 +173,9 @@ open class Directions: NSObject { | |||
route.accessToken = self.accessToken | |||
route.apiEndpoint = self.apiEndpoint | |||
route.routeIdentifier = json["uuid"] as? String | |||
if let vLocale = json["voiceLocale"] as? String { | |||
route.spokenLocale = Locale(identifier: vLocale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about also copying this locale into each SpokenInstruction, so that the navigation SDK’s voice controller classes only need to work with individual SpokenInstruction objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that works
@@ -194,6 +197,11 @@ open class Route: NSObject, NSSecureCoding { | |||
Each route produced by a single call to `Directions.calculate(_:completionHandler:)` has the same route identifier. | |||
*/ | |||
@objc open var routeIdentifier: String? | |||
|
|||
/** | |||
The locale to use for spoken instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain that this locale is specifically meant for use with the Mapbox Voice API. Add a note about what nil
means.
MapboxDirectionsTests/V5Tests.swift
Outdated
@@ -58,6 +58,7 @@ class V5Tests: XCTestCase { | |||
XCTAssertEqual(route!.accessToken, BogusToken) | |||
XCTAssertEqual(route!.apiEndpoint, URL(string: "https://api.mapbox.com")) | |||
XCTAssertEqual(route!.routeIdentifier, "cj725hpi30yp2ztm2ehbcipmh") | |||
XCTAssertEqual(route!.spokenLocale!.description, "en-US") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description
is a free-form, human-readable string that may contain extra details beyond the identifier. Use identifier
instead.
This is ready for review @mapbox/navigation-ios. |
The Mapbox Directions API will soon tell the developer in which locale to speak the instructions in. This is helpful for in situations where there is a mismatch between the written instructions the API supports and the spoken voice locales as well.
Looks like this is failing with the error
Any ideas @1ec5 about this? The test fixture addition adds
voiceLocale: "en-us"
. Is this a case issue?/cc @mapbox/navigation-ios