Expose delegate method for failed voice instructions #800

bsudekum · 2017-11-06T19:12:36Z

This exposes a delegate method for any failure related to voice instructions.

Todo:

Document.
Detect when the audioController is interrupted, send error.

/cc @mapbox/guidance @1ec5 @frederoni @JThramer @ericrwolfe

bsudekum · 2017-11-06T19:17:54Z

MapboxNavigation/PollyVoiceController.swift

+
+extension NSError {
+    static func withCustom(message: String) -> NSError {
+        return NSError(domain: "MBVoiceErrorDomain", code: -1, userInfo: [


Are there any improvements that could be made to this object?

bsudekum · 2017-11-06T19:49:48Z

MapboxNavigation/PollyVoiceController.swift

@@ -29,7 +29,7 @@ public class PollyVoiceController: RouteVoiceController {
    /**
     Number of seconds a Polly request can wait before it is canceled and the default speech synthesizer speaks the instruction.
     */
-    public var timeoutIntervalForRequest:TimeInterval = 2
+    public var timeoutIntervalForRequest:TimeInterval = 5


Also going to bump this up. 2 seconds is just too short.

1ec5 · 2017-11-07T07:33:00Z

MapboxNavigation/RouteVoiceController.swift

@@ -208,3 +210,7 @@ open class RouteVoiceController: NSObject, AVSpeechSynthesizerDelegate, AVAudioP
        speechSynth.speak(utterance)
    }
 }
+
+@objc public protocol VoiceControllerDelegate {
+    @objc optional func spokenInstructionsDidFail(_ voiceController: RouteVoiceController, error: Error)


Is it failure if we fall back to the system voice, or is it a fallback situation? Perhaps we should scope this protocol more tightly to be just about PollyVoiceController: PollyVoiceController failing is the same thing as PollyVoiceController falling back to VoiceController.

Another case to think about though is for overlapping instructions. We're not falling back, but it is an error.

Yeah, i think you're right. Going to move to pollyvoicecontroller

bsudekum · 2017-11-07T18:58:04Z

@1ec5 updated to use custom error codes.

frederoni · 2017-11-07T20:42:01Z

MapboxNavigation/RouteVoiceController.swift

@@ -58,14 +58,19 @@ open class RouteVoiceController: NSObject, AVSpeechSynthesizerDelegate, AVAudioP
    public var bufferBetweenAnnouncements: TimeInterval = 3

    /**
+     Delegate used for getting metadata information about a particular spoken instruction.
+     */
+    public var voiceControllerDelegate: VoiceControllerDelegate?


This should be a weak var to avoid a retain cycle. We can also rename this property to delegate.

1ec5 · 2017-11-07T21:07:41Z

MapboxNavigation/Error.swift

+    /**
+     Creates a custom `Error` object.
+     */
+    public static func withCustom(message: String, errorCode: SpokenVoiceError = .defaultError) -> NSError {


Make this method an initializer instead: NSError(localizedFailureReason:code:)

1ec5 · 2017-11-07T21:08:33Z

MapboxNavigation/Error.swift

+     Creates a custom `Error` object.
+     */
+    public static func withCustom(message: String, errorCode: SpokenVoiceError = .defaultError) -> NSError {
+        return NSError(domain: "MBErrorDomain", code: errorCode.rawValue, userInfo: [


Pull this domain out into a global constant, along the lines of the map SDK’s MGLErrorDomain.

1ec5 · 2017-11-07T21:09:09Z

MapboxNavigation/ErrorCodes.swift

+/**
+ Enum used for indicating the type of error occured while speaking an instruction.
+ */
+public enum SpokenVoiceError: Int {


There should be a single enumeration with all the error codes we may need to use in this library, similar to the map SDK’s MGLErrorCode.

1ec5 · 2017-11-07T21:09:35Z

MapboxNavigation/ErrorCodes.swift

+    /**
+     Default error.
+     */
+    case defaultError


The default case should be unknown, set to −1.

1ec5 · 2017-11-07T21:09:47Z

MapboxNavigation/ErrorCodes.swift

+    /**
+     The audio player failed to play audio data.
+     */
+    case audiPlayerFailedToPlay


Typo: audio.

1ec5 · 2017-11-07T21:11:48Z

MapboxNavigation/ErrorCodes.swift

+    /**
+     An intruction interrupted another instruction.
+     */
+    case overlappingInstruction


Is this really an error? Unlike the other cases, one instruction interrupting another instruction can be completely expected, like when the user triggers a reroute while an instruction is being read aloud.

We need to know when something like this happens, as it shouldn't. Would you rather have this filed under general?

I think I’d rather that overlapping instructions not result in an “error” at all, but it would be fine to have a dedicated delegate method indicating that an instruction was interrupted before completion.

a dedicated delegate method indicating that an instruction was interrupted before completion

For example, voiceController(_:didInterrupt:with:), taking two SpokenInstruction objects.

1ec5 · 2017-11-07T21:12:24Z

MapboxNavigation/ErrorCodes.swift

+    /**
+     The audio player failed to preprare to play audio data.
+     */
+    case audioPlayerFailedToPrepare


Some of these cases are specific to PollyVoiceController (or at least are inapplicable to RouteVoiceController). Perhaps we can generalize these cases so they can be used for more kinds of voice controllers in the future.

1ec5 · 2017-11-07T21:15:50Z

MapboxNavigation/RouteVoiceController.swift

        // Note why it failed
        if let error = error {
-            print(error)
+            voiceControllerDelegate?.spokenInstructionsDidFail?(self, error: error)


We need to reapply the refactor that was proposed in #624: the caller should be responsible for handling errors. Making this method the bottleneck for error reporting is a premature optimization.

@1ec5 I'm not seeing a proposal for fixing this line in #624.

https://github.com/mapbox/mapbox-navigation-ios/pull/624/files#diff-eaaa217c63f0df828e942d653c6994ceL195

1ec5 · 2017-11-07T23:13:17Z

MapboxNavigation/PollyVoiceController.swift

            return
        }

        guard let url = awsTask.result else {
-            callSuperSpeak(fallbackText, error: "No polly response")
+            callSuperSpeak(fallbackText, error: NSError.withCustom(message: "No AWS Task"))


localizedFailureReason is meant to be human-readable and understandable by the end user, in case the developer presents the error via an alert box (not required). The best practice would be to make the NSError’s error code something generic, like spokenInstructionFailed, and its localized failure reason also vague, like “Unable to read aloud the instruction ‘…’.” Then we can add another field to the NSError’s user info dictionary that indicates exactly what went wrong (in this case, an AWS failure).

bsudekum · 2017-11-08T15:54:58Z

MapboxNavigation/Error.swift

+    public convenience init(localizedFailureReason: String, detailedFailureReason: String, code: MapboxNavigationError = .defaultError) {
+        self.init(domain: MGLErrorDomain, code: code.rawValue, userInfo: [
+            NSLocalizedFailureReasonErrorKey: localizedFailureReason,
+            "MapboxErrorKey": detailedFailureReason


@1ec5 What should this key be?

NSLocalizedDescriptionKey

1ec5 · 2017-11-08T21:40:02Z

MapboxNavigation/PollyVoiceController.swift

@@ -189,7 +189,7 @@ public class PollyVoiceController: RouteVoiceController {
        }

        guard let url = awsTask.result else {
-            callSuperSpeak(fallbackText, error: NSError.withCustom(message: "No AWS Task"))
+            callSuperSpeak(fallbackText, error: NSError(localizedFailureReason: "Unable to read instruction aloud", detailedFailureReason: "No AWS Task"))


What I meant in #800 (comment) is that this error would contain:

NSLocalizedFailureReasonErrorKey: Unable to say “In 5 miles, turn left onto Main Street”

SpokenInstructionErrorCodeKey: SpokenInstructionErrorCode.noAwsTask

The application can then do something different by switching on SpokenInstructionErrorCodeKey rather than doing string comparisons.

1ec5 · 2017-11-08T22:51:36Z

MapboxCoreNavigation/Constants.swift

@@ -152,3 +152,6 @@ public var RouteControllerMinimumNumberLocationUpdatesBackwards = 3
 public var RouteControllerNumberOfSecondsForRerouteFeedback: TimeInterval = 10

 let FasterRouteFoundEvent = "navigation.fasterRoute"
+
+let MGLErrorDomain = "MGLErrorDomain"


This constant needs to be public so developers can conditionalize logic in exception handlers based on it. It also needs a little documentation.

1ec5 · 2017-11-08T22:51:58Z

MapboxNavigation/Error.swift

+    public convenience init(localizedFailureReason: String, detailedFailureReason: String, code: MapboxNavigationError = .defaultError) {
+        self.init(domain: MGLErrorDomain, code: code.rawValue, userInfo: [
+            NSLocalizedFailureReasonErrorKey: localizedFailureReason,
+            NSLocalizedDescriptionKey: detailedFailureReason


#800 (comment)

1ec5 · 2017-11-08T22:53:59Z

MapboxNavigation/RouteVoiceController.swift

@@ -207,3 +212,7 @@ open class RouteVoiceController: NSObject, AVSpeechSynthesizerDelegate, AVAudioP
        speechSynth.speak(utterance)
    }
 }
+
+@objc public protocol VoiceControllerDelegate {
+    @objc optional func spokenInstructionsDidFail(_ voiceController: RouteVoiceController, error: Error)


Delegate methods should always accept the delegator object as the first parameter and be named based on the delegator object. So this method should be voiceController(_:spokenInstructionsDidFailWith:). Also, the Objective-C signature needs to include the nouns: -voiceController:spokenInstructionsDidFailWithError:.

1ec5 · 2017-11-08T22:55:01Z

MapboxNavigation/ErrorCodes.swift

+    /**
+     An intruction interrupted another instruction.
+     */
+    case overlappingInstruction


a dedicated delegate method indicating that an instruction was interrupted before completion

For example, voiceController(_:didInterrupt:with:), taking two SpokenInstruction objects.

1ec5 · 2017-11-08T22:58:42Z

MapboxNavigation/RouteVoiceController.swift

-            print("Voice guidance may not work properly. " +
-                  "Add audio to the UIBackgroundModes key to your app’s Info.plist file")
+            let errorMessage = "Voice guidance may not work properly. Add audio to the UIBackgroundModes key to your app’s Info.plist file"
+            voiceControllerDelegate?.spokenInstructionsDidFail?(self, error: NSError(localizedFailureReason: "Unable to read instruction aloud", detailedFailureReason: errorMessage, code: .audioBackgroundModeMissing)) ?? print(errorMessage)


I think we should make this flag a hard requirement for the navigation SDK and maybe have it fail earlier, when creating the NavigationViewController. At least make it an assertion, because the user should be able to expect to hear the same spoken instructions while the application is in the background.

bsudekum · 2017-11-14T01:23:22Z

@1ec5 this is ready for review again.

1ec5 · 2017-11-14T21:10:14Z

MapboxNavigation/Error.swift

+/**
+ Custom Error code key used in returned in `voiceController(_:didInterrupt:with:)`.
+ */
+public let SpokenInstructionErrorCodeKey = "SpokenInstructionErrorCodeKey"


A global constant that’s accessible to Objective-C needs to use the class prefix MB in both its name and value. Additionally, the word “Key” should only appear in the name of the constant, not its value.

1ec5 · 2017-11-14T21:10:33Z

MapboxNavigation/Error.swift

+
+
+/**
+ Custom Error code key used in returned in `voiceController(_:didInterrupt:with:)`.


Fully qualify the reference to this method name so that jazzy can autolink it.

1ec5 · 2017-11-14T21:11:40Z

MapboxNavigation/Error.swift

+    public convenience init(localizedFailureReason: String, detailedFailureReason: String, code: MapboxNavigationError = .defaultError) {
+        self.init(domain: MGLErrorDomain, code: code.rawValue, userInfo: [
+            NSLocalizedFailureReasonErrorKey: localizedFailureReason,
+            SpokenInstructionErrorCodeKey: detailedFailureReason


MBSpokenInstructionErrorCodeKey should be set to an enumeration that a developer can easily switch on, not a string.

/ref #800 (comment)

1ec5 · 2017-11-14T21:12:34Z

MapboxCoreNavigation/Constants.swift

+/**
+ Domain which all errors from delegate methods live under.
+ */
+public let MGLErrorDomain = "MGLErrorDomain"


MGLErrorDomain is the name of the constant used by the map SDK. Our non-GL libraries use MB as the class prefix, not MGL.

1ec5 · 2017-11-14T21:14:07Z

MapboxNavigation/PollyVoiceController.swift

@@ -46,6 +46,8 @@ public class PollyVoiceController: RouteVoiceController {

    var spokenInstructionsForRoute = NSCache<NSString, NSData>()

+    var previousInstruction: String?


Rename this property to lastSpokenInstruction and set it to an Instruction, not just its SSML text.

1ec5 · 2017-11-14T21:22:48Z

MapboxNavigation/RouteVoiceController.swift

        // Note why it failed
        if let error = error {
-            print(error)
+            voiceControllerDelegate?.voiceController?(self, spokenInstructionsDidFailWith: error)


It should be the responsibility of the caller of this method to deal with any error that is being passed in. This method should only be responsible for handling errors that arise in the execution of this method.

/ref #800 (comment)

1ec5 · 2017-11-14T21:25:08Z

MapboxNavigation/PollyVoiceController.swift

@@ -166,7 +172,7 @@ public class PollyVoiceController: RouteVoiceController {
        }
    }

-    func callSuperSpeak(_ text: String, error: String) {
+    func callSuperSpeak(_ text: String, error: Error) {


Rename this method to speakWithoutPolly(_:) and have it take a single SpokenInstruction object.

(As to why, keep reading: #800 (comment).)

1ec5 · 2017-11-14T21:25:46Z

MapboxNavigation/RouteVoiceController.swift

@@ -181,16 +185,16 @@ open class RouteVoiceController: NSObject, AVSpeechSynthesizerDelegate, AVAudioP
        return true
    }

-    func speak(_ text: String, error: String? = nil) {
+    func speak(_ text: String, error: Error?) {


As part of #624, I proposed renaming this method:

mapbox-navigation-ios/MapboxNavigation/RouteVoiceController.swift

Line 196 in 2097f6d

open func speak(for routeProgress: RouteProgress, userDistance: CLLocationDistance) {

and having alertLevelDidChange(_:) set up the input data for this method instead of storing the input data on instance properties:

mapbox-navigation-ios/MapboxNavigation/RouteVoiceController.swift

Lines 160 to 165 in 2097f6d

func alertLevelDidChange(notification: NSNotification) {

guard shouldSpeak(for: notification) else { return }

let routeProgress = notification.userInfo![RouteControllerAlertLevelDidChangeNotificationRouteProgressKey] as! RouteProgress

let userDistance = notification.userInfo![RouteControllerAlertLevelDidChangeNotificationDistanceToEndOfManeuverKey] as! CLLocationDistance

speak(for: routeProgress, userDistance: userDistance)

PollyVoiceController overrides speak(for:userDistance:) instead of alertLevelDidChange(_:):

mapbox-navigation-ios/MapboxNavigation/PollyVoiceController.swift

Line 51 in 2097f6d

public override func speak(for routeProgress: RouteProgress, userDistance: CLLocationDistance) {

Of course, this library no longer generates the utterances directly from the route progress and current user distance, so speak(_:) can take just a SpokenInstruction.

Overall, PollyVoiceController and RouteVoiceController end up being a lot easier to understand to those who haven’t been involved in writing these classes.

1ec5 · 2017-11-14T21:27:12Z

MapboxNavigation/ErrorCodes.swift

+/**
+ Enum used for indicating the type of error occured while speaking an instruction.
+ */
+public enum MapboxNavigationError: Int {


Rename this enumeration to MBErrorCode for consistency with MGLErrorCode in the map SDK.

1ec5 · 2017-11-14T21:27:40Z

MapboxNavigation.xcodeproj/project.pbxproj

@@ -479,6 +482,7 @@
 		C5ADFBF91DDCC9580011824B /* RouteController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RouteController.swift; sourceTree = "<group>"; };
 		C5ADFBFB1DDCC9AD0011824B /* RouteProgress.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RouteProgress.swift; sourceTree = "<group>"; };
 		C5CFE4871EF2FD4C006F48E8 /* MMEEventsManager.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = MMEEventsManager.swift; sourceTree = "<group>"; };
+		C5D1C9931FB236900067C619 /* ErrorCodes.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ErrorCodes.swift; sourceTree = "<group>"; };


Rename this file to ErrorCode.swift for consistency with the MBErrorCode enumeration name.

bsudekum · 2017-11-15T00:10:35Z

@1ec5 refactored a bit with your feedback. I also removed the overlap timers in https://github.com/mapbox/mapbox-navigation-ios/pull/800/commits/8689cc99856e7c90e16fb2f7abe6b8e9fd85f74e- this is the servers job now.

1ec5

Thanks for undertaking the proposed refactor. Some details still need to be refined, particularly around the creation of NSError objects, but this PR is looking much better.

1ec5 · 2017-11-15T05:14:12Z

MapboxCoreNavigation/MBRouteController.m

@@ -17,3 +17,5 @@
 NSString *const MBRouteControllerDidReroute                     = @"RouteControllerDidReroute";
 NSString *const MBRouteControllerDidFailToReroute               = @"RouteControllerDidFailToReroute";
 NSString *const MBRouteControllerDidFindFasterRouteKey          = @"RouteControllerDidFindFasterRoute";
+NSString *const MBErrorDomain = @"ErrorDomain";
+NSString *const MBSpokenInstructionErrorCode = @"SpokenInstructionErrorCodeKey";


Add “Key” to the name of the constant and remove it from the constant’s value.

1ec5 · 2017-11-15T05:16:50Z

MapboxNavigation/Error.swift


 extension NSError {
    /**
     Creates a custom `Error` object.
     */
-    public convenience init(localizedFailureReason: String, detailedFailureReason: String, code: MapboxNavigationError = .defaultError) {
-        self.init(domain: MGLErrorDomain, code: code.rawValue, userInfo: [
+    public convenience init(localizedFailureReason: String, code: MapboxNavigationError = .defaultError) {


This initializer is only useful to this SDK’s code, so make it private if possible.

1ec5 · 2017-11-15T05:25:06Z

MapboxNavigation/Error.swift

            NSLocalizedFailureReasonErrorKey: localizedFailureReason,
-            SpokenInstructionErrorCodeKey: detailedFailureReason
+            MBSpokenInstructionErrorCode: code


Sorry for not being clearer in #800 (comment), #800 (comment), and #800 (comment):

This method should accept a spokenInstructionCode parameter of type SpokenInstructionErrorCode.

SpokenInstructionErrorCode should be an enumeration with the following values: noAwsResult, emptyAwsResponse, and audioPlayerFailedToPlay.

MBErrorCode (renamed from MapboxNavigationError) should have the following values: unknown and spokenInstructionFailed.

One of the call sites in PollyVoiceController should call this initializer passing in spokenInstructionFailed as the code and noAwsResult as the spokenInstructionCode.

1ec5 · 2017-11-15T05:26:21Z

MapboxCoreNavigation/MBRouteController.h

@@ -17,3 +17,5 @@ extern NSString *const MBRouteControllerWillReroute;
 extern NSString *const MBRouteControllerDidReroute;
 extern NSString *const MBRouteControllerDidFailToReroute;
 extern NSString *const MBRouteControllerDidFindFasterRouteKey;
+extern NSString *const MBErrorDomain;
+extern NSString *const MBSpokenInstructionErrorCode;


These constants (like all the others here) need documentation comments so that they appear in the jazzy documentation.

1ec5 · 2017-11-15T05:27:24Z

MapboxNavigation/PollyVoiceController.swift

@@ -46,7 +47,7 @@ public class PollyVoiceController: RouteVoiceController {

    var spokenInstructionsForRoute = NSCache<NSString, NSData>()

-    var previousInstruction: String?
+    var previousInstruction: SpokenInstruction?


Rename this property to lastSpokenInstruction. Between spoken instructions, it makes more sense to talk about the last instruction that was said than the “previous” one.

1ec5 · 2017-11-15T05:28:54Z

MapboxNavigation/PollyVoiceController.swift

            return
        }

        guard let url = awsTask.result else {
-            callSuperSpeak(fallbackText, error: NSError(localizedFailureReason: "Unable to read instruction aloud", detailedFailureReason: "No AWS Task"))
+            speakWithoutPolly(fallbackInstruction, error: NSError(localizedFailureReason: "Unable to read instruction aloud"))


Localized failure reasons should be localizable.

1ec5 · 2017-11-15T05:30:48Z

MapboxNavigation/RouteVoiceController.swift

@@ -180,24 +167,19 @@ open class RouteVoiceController: NSObject, AVSpeechSynthesizerDelegate, AVAudioP
        // Set recentlyAnnouncedRouteStep to the current step
        recentlyAnnouncedRouteStep = routeProgress.currentLegProgress.currentStep

-        fallbackText = routeProgress.currentLegProgress.currentStepProgress.currentSpokenInstruction?.text
+        fallbackInstruction = routeProgress.currentLegProgress.currentStepProgress.currentSpokenInstruction


Remove this line and remove the fallbackInstruction property. From now on, it should be the responsibility of alertLevelDidChange(_:) to pass currentSpokenInstruction directly into speak(_:).

1ec5 · 2017-11-15T05:34:01Z

MapboxNavigation/PollyVoiceController.swift

@@ -68,14 +69,13 @@ public class PollyVoiceController: RouteVoiceController {
        guard shouldSpeak(for: notification) == true else { return }


Move everything below here into speak(_:) and delete this method implementation, including this guard statement. It should no longer be necessary for PollyVoiceController to override this method.

1ec5 · 2017-11-15T05:34:51Z

MapboxNavigation/RouteVoiceController.swift

@@ -207,3 +211,8 @@ open class RouteVoiceController: NSObject, AVSpeechSynthesizerDelegate, AVAudioP
        speechSynth.speak(utterance)
    }
 }
+
+@objc public protocol VoiceControllerDelegate {
+    @objc optional func voiceController(_ voiceController: RouteVoiceController, spokenInstructionsDidFailWith error: Error)


This method still needs a proper Objective-C signature.

bsudekum · 2017-11-16T17:33:51Z

@1ec5 this is ready for review again.

1ec5 · 2017-11-16T21:40:20Z

MapboxCoreNavigation/MBRouteController.h

@@ -17,5 +17,8 @@ extern NSString *const MBRouteControllerWillReroute;
 extern NSString *const MBRouteControllerDidReroute;
 extern NSString *const MBRouteControllerDidFailToReroute;
 extern NSString *const MBRouteControllerDidFindFasterRouteKey;
+
+/**
+ Constant representing the namespace in which errors created in this library will live under.


s/namespace/domain/

1ec5 · 2017-11-16T21:41:16Z

MapboxCoreNavigation/MBRouteController.m

+
+/**
+ Domain for errors created within this framework.
+ */


Merge this comment with the one in the header. A public Objective-C symbol should be documented only on the declaration (in the header, in this case), not on the definition.

1ec5 · 2017-11-16T21:41:58Z

MapboxNavigation/Error.swift

        self.init(domain: MBErrorDomain, code: code.rawValue, userInfo: [
-            NSLocalizedFailureReasonErrorKey: localizedFailureReason,
-            MBSpokenInstructionErrorCode: code
+            NSLocalizedFailureReasonErrorKey: localizedFailureReason


spokenInstructionCode appears to be unused. Add an entry here that sets MBSpokenInstructionErrorCodeKey to spokenInstructionCode, but only if it’s provided.

1ec5 · 2017-11-16T21:44:29Z

MapboxNavigation/ErrorCode.swift

@@ -19,3 +19,18 @@ public enum MapboxNavigationError: Int {
     */
    case noDataInSpokenInstructionResponse


Rename noDataInSpokenInstructionResponse to emptyAwsResponse, since it’s assumed that any case in this enumeration is about spoken instructions.

Previous iterations of this PR also had error codes for noAwsResult and audioPlayerFailedToPlay. Do we still need them?

Yes, .spokenInstructionAudioPlayerFailedToPlay are is used.

1ec5 · 2017-11-16T21:45:11Z

MapboxNavigation/PollyVoiceController.swift

-    var previousInstruction: SpokenInstruction?
+    var lastSpokenInstruction: SpokenInstruction?
+
+    let localizedErrorMessage = NSLocalizedString("FAILED_INSTRUCTION", bundle: .mapboxNavigation, value: "Unable to read instruction aloud", comment: "Unable to read instruction aloud")


The comment should explain to translators how this string is used:

Error message when the SDK is unable to read a spoken instruction.

1ec5 · 2017-11-16T21:46:21Z

MapboxNavigation/PollyVoiceController.swift

@@ -66,17 +68,15 @@ public class PollyVoiceController: RouteVoiceController {
    }

    public override func didPassSpokenInstructionPoint(notification: NSNotification) {


PollyVoiceController should override speak(_:), but should have no reason to override didPassSpokenInstructionPoint(_:). Move everything from this method into speak(_:), except for the statements that are also in RouteVoiceController.didPassSpokenInstructionPoint(_:). Then remove this method override. The implementation in RouteVoiceController.didPassSpokenInstructionPoint(_:) will get called on PollyVoiceController whenever we pass a spoken instruction point.

1ec5 · 2017-11-16T21:48:59Z

MapboxNavigation/PollyVoiceController.swift

@@ -177,23 +178,23 @@ public class PollyVoiceController: RouteVoiceController {
        voiceControllerDelegate?.voiceController?(self, spokenInstructionsDidFailWith: error)

        guard let audioPlayer = audioPlayer else {
-            super.speak(fallbackInstruction)
+            super.speak(lastSpokenInstruction!)


Pass in instruction instead of lastSpokenInstuction. Ditto below.

1ec5 · 2017-11-16T21:49:33Z

MapboxNavigation/Resources/Base.lproj/Localizable.strings

@@ -1,6 +1,9 @@
 /* Dismiss button title on the steps view */
 "DISMISS_STEPS_TITLE" = "Close";

+/* Unable to read instruction aloud */
+"FAILED_INSTRUCTION" = "Unable to read instruction aloud";


A human-readable error message should read like a complete sentence, so it should end with a period.

1ec5 · 2017-11-16T21:51:55Z

MapboxNavigation/RouteVoiceController.swift

-    }
-
-    func shouldSpeak(for notification: NSNotification) -> Bool {
-        guard isEnabled, volume > 0, !NavigationSettings.shared.muted else { return false }


It would’ve been fine to keep shouldSpeak(for:) around, but in any case, make sure these guard statements are in RouteVoiceController.didPassSpokenInstructionPoint(_:), not PollyVoiceController.didPassSpokenInstructionPoint(_:) – the latter method should go away.

1ec5 · 2017-11-16T21:52:46Z

MapboxNavigation/RouteVoiceController.swift

    }

    func speak(_ instruction: SpokenInstruction) {
+        guard isEnabled, volume > 0, !NavigationSettings.shared.muted else { return }


Move this statement up to didPassSpokenInstructionPoint(_:) or keep shouldSpeak(for:) around.

1ec5 · 2017-11-16T21:56:06Z

Following the refactoring, PollyVoiceController should work like this:

RouteVoiceController.didPassSpokenInstructionPoint(_:) gets called. It consults RouteVoiceController.shouldSpeak(for:) as to whether it should continue or bail (or that logic can be inlined in didPassSpokenInstructionPoint(_:) – either way’s fine.
RouteVoiceController.didPassSpokenInstructionPoint(_:) calls speak(_:). Since PollyVoiceController has overridden speak(_:), PollyVoiceController.speak(_:) gets called.
Various code paths in PollyVoiceController.speak(_:) may call RouteVoiceController.speak(_:) via PollyVoiceController.speakWithoutPolly(_:error:).

At no point does PollyVoiceController.didPassSpokenInstructionPoint(_:) ever get called, even for PollyVoiceController.

mcwhittemore · 2017-11-16T21:57:53Z

I keep forgetting to say how excited I am about this. I'm very excited! :)

bsudekum · 2017-11-16T23:28:50Z

@1ec5 alright, I think we're there.

1ec5 · 2017-11-16T23:30:24Z

MapboxNavigation/ErrorCode.swift

+    /**
+     Default error.
+     */
+    case defaultError = -1


unknown, for consistency with MBErrorCode.unknown.

1ec5 · 2017-11-16T23:31:01Z

MapboxNavigation/ErrorCode.swift

+    /**
+     The audio player failed to play audio data.
+     */
+    case spokenInstructionAudioPlayerFailedToPlay


spokenInstruction is redundant here, because it’s part of an enumeration that contains SpokenInstruction in its name. Rename this case to audioPlayerFailedToPlay.

1ec5 · 2017-11-16T23:35:26Z

MapboxNavigation/PollyVoiceController.swift

-        speak(instruction, error: nil)
+        guard isEnabled, volume > 0, !NavigationSettings.shared.muted else { return }
+
+        super.speak(instruction)


This causes RouteVoiceController.speak(_:) to be called, so PollyVoiceController.speak(_:) never gets called.

An override should normally call the superclass’s implementation. So call super.didPassSpokenInstructionPoint(_:). That will allow RouteVoiceController.didPassSpokenInstructionPoint(_:) to determine which instruction to pass into PollyVoiceController.speak(_:).

If that back-and-forth concerns you, bring back RouteVoiceController.shouldSpeak(for:) and move this prefecthing logic to an override of that method, PollyVoiceController.shouldSpeak(for:). (The override can call the super implementation after performing the prefetching.)

Fixed in a84a28f.

1ec5 · 2017-11-16T23:37:14Z

MapboxNavigation/PollyVoiceController.swift

            return
        }

        guard !audioPlayer.isPlaying else { return }

-        super.speak(fallbackText, error: error)
+        super.speak(instruction)
    }

    func handle(_ awsTask: AWSTask<NSURL>) {


Pass instruction into this method, so that it doesn’t need to use lastSpokenInstruction. This keeps the class minimally dependent on instance properties.

Also adhere to best practices around overriding methods.

This method is the main customization hook for custom voice controller subclasses.

bsudekum commented Nov 6, 2017

View reviewed changes

1ec5 reviewed Nov 7, 2017

View reviewed changes

bsudekum force-pushed the voice-fail-delegate branch from 76bece5 to d78edaf Compare November 7, 2017 20:25

frederoni reviewed Nov 7, 2017

View reviewed changes

1ec5 reviewed Nov 7, 2017

View reviewed changes

bsudekum commented Nov 8, 2017

View reviewed changes

1ec5 reviewed Nov 8, 2017

View reviewed changes

bsudekum force-pushed the voice-fail-delegate branch from 9f1f165 to 1095b51 Compare November 8, 2017 22:46

1ec5 reviewed Nov 8, 2017

View reviewed changes

Bobby Sudekum added 3 commits November 13, 2017 17:07

Expose delegate method for failed voice instructions

1b3e1c9

Update

6a34494

add

cccbb22

bsudekum force-pushed the voice-fail-delegate branch from 938dc54 to cccbb22 Compare November 14, 2017 01:11

Key

b0094b5

1ec5 reviewed Nov 14, 2017

View reviewed changes

refactor

8689cc9

Bobby Sudekum added 2 commits November 14, 2017 16:27

Change errors

90ce8a4

make required

652a8c9

1ec5 reviewed Nov 15, 2017

View reviewed changes

Bobby Sudekum added 3 commits November 15, 2017 09:28

More changes

fbdd26b

Update error

bdf2916

remove

ae9458b

Doc

366fb01

1ec5 reviewed Nov 16, 2017

View reviewed changes

1ec5 mentioned this pull request Nov 16, 2017

Supply phonetic road names to AVSpeechSynthesizer #624

Merged

Bobby Sudekum added 3 commits November 16, 2017 14:37

Updates

d7b04d5

Fix

e290112

Super

f2fc361

1ec5 reviewed Nov 16, 2017

View reviewed changes

Bobby Sudekum and others added 4 commits November 16, 2017 15:51

Fix o

a6017b5

Consolidated speaking conditions

a84a28f

Also adhere to best practices around overriding methods.

Made RouteVoiceController.speak(_:) open

a3d3ad3

This method is the main customization hook for custom voice controller subclasses.

Consolidated audio player logic in speak(_:)

1e885bb

1ec5 approved these changes Nov 17, 2017

View reviewed changes

bsudekum merged commit a66e5aa into master Nov 17, 2017

bsudekum deleted the voice-fail-delegate branch November 17, 2017 00:49

bsudekum mentioned this pull request Nov 17, 2017

Improve voice delegates #864

Merged



		/**
		Custom Error code key used in returned in `voiceController(_:didInterrupt:with:)`.

		@@ -46,6 +46,8 @@ public class PollyVoiceController: RouteVoiceController {

		var spokenInstructionsForRoute = NSCache<NSString, NSData>()

		var previousInstruction: String?

	func alertLevelDidChange(notification: NSNotification) {
	guard shouldSpeak(for: notification) else { return }

	let routeProgress = notification.userInfo![RouteControllerAlertLevelDidChangeNotificationRouteProgressKey] as! RouteProgress
	let userDistance = notification.userInfo![RouteControllerAlertLevelDidChangeNotificationDistanceToEndOfManeuverKey] as! CLLocationDistance
	speak(for: routeProgress, userDistance: userDistance)

		@@ -68,14 +69,13 @@ public class PollyVoiceController: RouteVoiceController {
		guard shouldSpeak(for: notification) == true else { return }

		@@ -19,3 +19,18 @@ public enum MapboxNavigationError: Int {
		*/
		case noDataInSpokenInstructionResponse

		@@ -66,17 +68,15 @@ public class PollyVoiceController: RouteVoiceController {
		}

		public override func didPassSpokenInstructionPoint(notification: NSNotification) {

Expose delegate method for failed voice instructions #800

Expose delegate method for failed voice instructions #800

Conversation

bsudekum commented Nov 6, 2017 • 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

bsudekum commented Nov 7, 2017

frederoni Nov 7, 2017 • 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

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

1ec5 Nov 8, 2017 • 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

bsudekum commented Nov 14, 2017

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

1ec5 Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Nov 15, 2017

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

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

bsudekum commented Nov 16, 2017

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

bsudekum Nov 16, 2017 • 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

Choose a reason for hiding this comment

1ec5 commented Nov 16, 2017

mcwhittemore commented Nov 16, 2017

bsudekum commented Nov 16, 2017

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

bsudekum commented Nov 6, 2017 •

edited

Loading

frederoni Nov 7, 2017 •

edited

Loading

1ec5 Nov 8, 2017 •

edited

Loading

1ec5 Nov 14, 2017 •

edited

Loading

1ec5 Nov 14, 2017 •

edited

Loading

bsudekum Nov 16, 2017 •

edited

Loading