Skip to content

Comments

Refactor voice controllers#313

Merged
frederoni merged 2 commits intomasterfrom
fred-refactor-voice
Jul 5, 2017
Merged

Refactor voice controllers#313
frederoni merged 2 commits intomasterfrom
fred-refactor-voice

Conversation

@frederoni
Copy link
Contributor

Fixes #280

Split up voice controllers into RouteVoiceController and PollyVoiceController.

Replace AVSpeech with Polly:

navigationViewController.voiceController = PollyVoiceController(identityPoolId: "")

@1ec5 @bsudekum 👀

@frederoni frederoni requested review from 1ec5 and bsudekum June 27, 2017 14:26
@frederoni frederoni force-pushed the fred-refactor-voice branch from 3dc0d78 to 1e6a7af Compare June 27, 2017 14:26
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

With this refactoring, we can modify the podspec to split PollyVoiceController.swift into a subspec: #280 (comment).

We could further streamline the setup process: PollyVoiceController.swift could define a category method on NavigationViewController that sets up the voice controller. That would come at the cost of modularity, I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldSpeak(for:)

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 a very vague name. Could we fold this into alertLevelDidChange(_:) and allow subclasses to override that method?

@frederoni frederoni force-pushed the fred-refactor-voice branch from 1e6a7af to 79c803a Compare June 27, 2017 20:16
@frederoni
Copy link
Contributor Author

frederoni commented Jun 27, 2017

Interesting idea about the category but less obvious to grasp and debug.

We should add a new target for PollyVoiceController when we create the podspec to
provide modularity for both Carthage and CocoaPods users.

@1ec5
Copy link
Contributor

1ec5 commented Jun 27, 2017

If we add a separate target, we'd have to add a separate podspec. Subspecs can't coexist with separate modules.

@frederoni frederoni merged commit 4cf0512 into master Jul 5, 2017
@frederoni frederoni deleted the fred-refactor-voice branch July 5, 2017 16:27
@1ec5 1ec5 added this to the v0.5.0 milestone Jul 9, 2017
@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards incompatible changes that break backwards compatibility of public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants