feat override synthesizer options from config #590
Conversation
|
Tested on jambonz.me |
lib/tasks/say.js
Outdated
| @@ -68,6 +68,12 @@ class TaskSay extends Task { | |||
| const salt = cs.callSid; | |||
|
|
|||
| let credentials = cs.getSpeechCredentials(vendor, 'tts', label); | |||
|
|
|||
| // override Synthesizer options from config verb | |||
| if (cs.synthesizer && cs.synthesizer.options) { | |||
There was a problem hiding this comment.
@xquanluu I would add these conditions:
- call session synthesizer options are only applied, when there is no say.synthesizer.options
- at least one "key" must be present in call session synthesizer options
if (Object.keys(this.options).length === 0 && cs.synthesizer && cs.synthesizer.options && Object.keys(cs.synthesizer.options).length > 0) {
There was a problem hiding this comment.
Thanks @Catharsis68 for the review:
1/ call session synthesizer options are only applied, when there is no say.synthesizer.options
--> This seem to me we violate the rule for config verb to override configuration from other verb.
2/ at least one "key" must be present in call session synthesizer options
--> Do you know any case that user use config verb to put {} empty configuration, IMHO I think there is 2 normal case there is synthesizer.options and there is no synthesizer.options?
There was a problem hiding this comment.
-
I was thinking more like this:
- If there is a global config - options will be applied to say task synthAudio
- If there is a specific options property directly in the say task - this will "win" and overwrite the global config. e.g you want to change the synthAudio options just for one say task, you would otherwise send a new config with te changes and then an additional config to change it back. So there would be no benefit of "say.synthesizer.options" in that case
The more specific options (directly provided in the say task) should overwrite the global one.
-
This condition is just there, that the "this.options" should not get overwritten by an empty options object.
There was a problem hiding this comment.
I agree with @Catharsis68 on this one -- the config provides session-level settings, but if a verb specifies something different, that should take preference for that verb. Given that, I think we can merge this @xquanluu ?
lib/tasks/say.js
Outdated
| @@ -70,7 +70,10 @@ class TaskSay extends Task { | |||
| let credentials = cs.getSpeechCredentials(vendor, 'tts', label); | |||
|
|
|||
| // override Synthesizer options from config verb | |||
| if (cs.synthesizer && cs.synthesizer.options) { | |||
| if (!Object.keys(this.options).length != 0 && | |||
There was a problem hiding this comment.
what exactly are we saying here?
There was a problem hiding this comment.
if not length does not equal zero.....can't that be made into a simple test: if length equals zero?
#589