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

Only return iOS voices that work #24

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

talkr-app
Copy link

Only one voice per locale works in iOS (versions 12 & 13 and maybe
earlier versions ). This change brings back the ios.js file and some of
its former code to detect iOS. It deletes the iOS8 & iOS9 voices
from ios.js in the lib folder (was that erroneously left over?) It then
filters the voices returned by iOS and only returns the ones that will
work.

Only one voice per locale works in iOS (versions 12 & 13 and maybe
earlier versions ). This change brings back the ios.js file and some of
its former code to detect iOS.  It deletes the iOS8 & iOS9 voices
from ios.js in the lib folder (was that erroneously left over?) It then
filters the voices returned by iOS and only returns the ones that will
work.
src/ios.js Outdated Show resolved Hide resolved
src/ios.js Outdated Show resolved Hide resolved
Copy link
Owner

@tom-s tom-s left a comment

Choose a reason for hiding this comment

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

I've suggested a few changes, thanks a lot for your PR

@talkr-app talkr-app closed this May 1, 2020
@talkr-app talkr-app reopened this May 1, 2020
lib/speak-tts.js Outdated
@@ -36,6 +36,7 @@ function () {

var listeners = (0, _utils.isNil)(conf.listeners) ? {} : conf.listeners;
var splitSentences = (0, _utils.isNil)(conf.splitSentences) ? true : conf.splitSentences;
var splitPhrases = (0, _utils.isNil)(conf.splitPhrases) ? true : conf.splitPhrases;
Copy link
Owner

@tom-s tom-s Jul 24, 2020

Choose a reason for hiding this comment

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

what is this option for ? How does it compare to splitSentences ?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, looks like I messed up working with this PR and included my internal changes. Not sure how useful this will be to the general public, but I'm trying to sync a moving picture to the TTS and I need to know when there is talking and when there is silence. splitPhrases splits on other punctuation characters that cause a pause in the TTS (like the comma).

Copy link
Owner

@tom-s tom-s Jul 24, 2020

Choose a reason for hiding this comment

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

alright, i'd be grateful if you could remove it. Even better, splitSentences could probably be refactored so that it can serve both purposes (by passing a list of splitting chars for instance) ?

Copy link
Author

Choose a reason for hiding this comment

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

A custom list of splitting chars would work for me and keep me from having to use a separate branch. I'll work on that.

Happy to remove this change, but I'm just not too familiar working with PRs...that's how I ended up with this in here in the first place :)

Copy link
Owner

Choose a reason for hiding this comment

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

I think I 'll do a massive refactor of this lib in the next month anyway, I'll include your changes. In the meantime you can use your own version and close this PR, or update the PR and I'll merge it.
It's up to you, let me know !

Copy link
Author

Choose a reason for hiding this comment

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

ok, removed the splitPhrases option. I'll work on making splitSentences take a custom list of splitting chars

src/ios.js Outdated Show resolved Hide resolved
src/speak-tts.js Outdated
@@ -166,8 +188,8 @@ class SpeakTTS {
if(this.pitch) utterance.pitch = this.pitch //0 to 2
utterance.text = sentence

// Attach event listeners
Object.keys(listeners).forEach(listener => {
// Attach event listeners, and make sure onerror and onend are included
Copy link
Owner

Choose a reason for hiding this comment

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

How is that necessary ?

Copy link
Author

Choose a reason for hiding this comment

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

onerror & onend are used by speaktts to resolve/reject the speak promise, and this should happen even if the user did not pass customized versions of those liseteners into the init function.

Copy link
Owner

Choose a reason for hiding this comment

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

if no customized listener is passed, this is going to attach undefined variables ?

Copy link
Author

Choose a reason for hiding this comment

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

No, newListener is a function that calls the user-provided listener (in this scenerio that is NULL and it won't be called due to "fn && fn(data)" ) but it also checks if the listener is for "onend" or "onerror" in order to resolve/reject the speak promise.

Prior to this change, if no custom listener for onend and onerror is passed, then the speak promise will not resolve or reject.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, thanks for the explanation. I think it could be resolved by giving a noop function as a defaut value for onEnd et onError

return false
}
export const filterIOSVoices = (voices) => {
let selectableVoices = [
Copy link
Owner

Choose a reason for hiding this comment

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

const

src/utils.js Outdated
@@ -5,6 +5,16 @@ export const splitSentences = (text = '') => text.replace(/\.+/g,'.|')
.map(sentence => trim(sentence))
.filter(Boolean)

export const splitPhrases = (text = '') => text.replace(/\.+/g,'.|')
Copy link
Owner

Choose a reason for hiding this comment

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

this can be deleted I think

splitSentences can now be true, false, or set to a string of custom
characters to deliminate the sentences.

An error is thrown if [a-zA-Z] are passed (a user sending in 'true'
could be in for a world of hurt  figuring that out.)

Any number of consecutive splitting characters can be used, and
the sentence will only be split once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants