FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,15 +45,15 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| var toSpeak = new SpeechSynthesisUtterance(" "); // short text to attempt to speak | ||
| toSpeak.volume = 0; // mutes the Speech Synthesizer | ||
| var timeout = setTimeout(function () { | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl("cancel", 10); | ||
| promise.reject(); | ||
| }, delay || 1000); | ||
| toSpeak.onend = function () { | ||
| clearTimeout(timeout); | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl("cancel", 10); | ||
| promise.resolve(); | ||
| }; | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl("speak", 10, toSpeak); | ||
| } else { | ||
| setTimeout(promise.reject, 0); | ||
| } | ||
|
|
@@ -72,9 +72,11 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| onSpeechQueued: null | ||
| }, | ||
| members: { | ||
| queue: [], | ||
| // | ||
| currentUtterance: {} | ||
| }, | ||
| // Model paths: speaking, pending, paused, utteranceOpts, pauseRequested, resumeRequested | ||
| model: { | ||
| // Changes to the utteranceOpts will only text that is queued after the change. | ||
| // All of these options can be overriden in the queueSpeech method by passing in | ||
|
|
@@ -83,7 +85,7 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| utteranceOpts: { | ||
| // text: "", // text to synthesize. avoid as it will override any other text passed in | ||
| // lang: "", // the language of the synthesized text | ||
| // voice: {} // a WebSpeechSynthesis object; if not set, will use the default one provided by the browser | ||
| // volume: 1, // a value between 0 and 1 | ||
| // rate: 1, // a value from 0.1 to 10 although different synthesizers may have a smaller range | ||
| // pitch: 1, // a value from 0 to 2 | ||
|
|
@@ -93,10 +95,6 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| "speaking": { | ||
| listener: "fluid.textToSpeech.speak", | ||
| args: ["{that}", "{change}.value"] | ||
| } | ||
| }, | ||
| invokers: { | ||
|
|
@@ -109,12 +107,12 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| args: ["{that}"] | ||
| }, | ||
| pause: { | ||
| funcName: "fluid.textToSpeech.issueControlRequest", | ||
| args: ["{that}", "pauseRequested", "pause", false] | ||
| }, | ||
| resume: { | ||
| funcName: "fluid.textToSpeech.issueControlRequest", | ||
| args: ["{that}", "resumeRequested", "resume", true] | ||
| }, | ||
| getVoices: { | ||
| "this": "speechSynthesis", | ||
|
|
@@ -132,25 +130,55 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| }, | ||
| handleError: "{that}.events.onError.fire", | ||
| handlePause: { | ||
| funcName: "fluid.textToSpeech.handlePause", | ||
| args: ["{that}"] | ||
| }, | ||
| handleResume: { | ||
| funcName: "fluid.textToSpeech.handleResume", | ||
| args: ["{that}"] | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Issue commands to the speechSynthesis interface after a delay; this | ||
| // makes the wrapper behave somewhat better when issuing commands, especially | ||
| // play and pause | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl = function (control, delay, args) { | ||
| setTimeout(function () { | ||
| speechSynthesis[control](args); | ||
| }, delay); | ||
| }; | ||
|
|
||
| fluid.textToSpeech.speak = function (that, speaking) { | ||
| that.events[speaking ? "onStart" : "onStop"].fire(); | ||
| }; | ||
|
|
||
| fluid.textToSpeech.handlePause = function (that) { | ||
| that.applier.change("paused", true); | ||
| that.events.onPause.fire(); | ||
|
Member
There was a problem hiding this comment. No need to duplicate the function of the model events with a regular event
Member
Author
There was a problem hiding this comment. One (possible) argument to make for Second possible point: it's a somewhat fine-grained (and possibly redundant) distinction but the model change represents a change to the component-tracked state of the speech, while the non-model events represent callbacks from the wrapped browser API about the state of the utterance.
Member
There was a problem hiding this comment. I can't go along with either of these arguments :) The whole purpose of having a model idiom is to avoid the proliferation of "uninterpreted instants in time". You have 2 events and 4 invokers, when you could have a simple flag in a model and a call to a standard algorithm. Ideally we never have "non-model event structure", parallel or otherwise.
Member
Author
There was a problem hiding this comment. Point taken - but in this case, we do need to establish some level of communication and sequencing between the wrapper component and the WebSpeechSynthesis API (so that the model doesn't enter a state that doesn't actually reflect what's happening with the browser API, which is where some of the problems have centred working on the issue of irregularly failing tests). But I'll look for ways to simplify.
Member
Author
There was a problem hiding this comment. A further note on the non-model events: the public API as documented at http://docs.fluidproject.org/infusion/development/TextToSpeechAPI.html includes the I'm uncertain how much removing them might impact any downstream projects, if at all. |
||
| // Clear to issue any waiting resume command | ||
| fluid.textToSpeech.clearControlRequest(that, "resumeRequested", "resume"); | ||
| }; | ||
|
|
||
| fluid.textToSpeech.handleResume = function (that) { | ||
|
Member
There was a problem hiding this comment. These two handleXxxx can be eliminated |
||
| that.applier.change("paused", false); | ||
| that.events.onResume.fire(); | ||
| // Clear to issue any waiting pause command | ||
| fluid.textToSpeech.clearControlRequest(that, "pauseRequested", "pause"); | ||
| }; | ||
|
|
||
| fluid.textToSpeech.clearControlRequest = function (that, modelBoolPath, controlName) { | ||
|
Member
There was a problem hiding this comment. This can be replaced with a straightforward modelListener which performs a "debounce" on the model state. Consult @jobara who has a mothballed pull request containing this algorithm. #566 - given we have also ripped it off here - https://github.com/fluid-project/node-jqunit/blob/master/lib/jqUnit-node.js#L158 - our "rule of thumb" suggests that this now needs to go into the core framework
Member
There was a problem hiding this comment.
Member
There was a problem hiding this comment. Ok - the guide is helpful to remind me too - the algorithm we want is actually "throttle" (with trailing) rather than "debounce" :)
Member
Author
There was a problem hiding this comment. Does it make sense per comment above about Rule of Thumb to simply move
Member
Author
There was a problem hiding this comment. I've gone ahead and adapted Underscore's implementation of throttle (with credit and license) in the component itself. |
||
| if(fluid.get(that.model, modelBoolPath)) { | ||
|
Member
Author
There was a problem hiding this comment. I've got my pre-commit linter running now on my Infusion project, so it runs (and aborts the commit on any linting errors) before every commit. Whatever the issue the
Member
There was a problem hiding this comment. Ok, looks like we need to add in this rule to our mix: http://eslint.org/docs/rules/keyword-spacing
Member
Author
There was a problem hiding this comment. I've made a small NOJIRA PR for this (and fixed two files that had the same style issue as part of it): #741 |
||
| that.applier.change(modelBoolPath, false); | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl(controlName, 10); | ||
| } | ||
| }; | ||
|
|
||
| fluid.textToSpeech.issueControlRequest = function (that, modelBoolPath, controlName, invert) { | ||
|
Member
There was a problem hiding this comment. This can be eliminated and the invokers replaced with a "changePath" record - http://docs.fluidproject.org/infusion/development/ChangeApplierAPI.html#declarative-style-for-triggering-a-change |
||
| if(invert ? !that.model.paused : that.model.paused) { | ||
| that.applier.change(modelBoolPath, true); | ||
| } else { | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl(controlName, 10); | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -186,6 +214,12 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| }; | ||
|
|
||
| var toSpeak = new SpeechSynthesisUtterance(text); | ||
| // Store toSpeak additional on the currentUtterance member to help deal with the issue | ||
| // with premature garbage collection described at https://bugs.chromium.org/p/chromium/issues/detail?id=509488#c11 | ||
| // this makes the speech synthesis behave much better in Safari in | ||
| // particular | ||
| // that.currentUtterance = toSpeak; | ||
|
Member
Author
There was a problem hiding this comment. The perils of running 50 differential manual tests at the end of the day is that you do things like this and comment out the actual fix for the problem. |
||
|
|
||
| var eventBinding = { | ||
| onstart: that.handleStart, | ||
| onend: that.handleEnd, | ||
|
|
@@ -197,12 +231,12 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
|
|
||
| that.queue.push(text); | ||
| that.events.onSpeechQueued.fire(text); | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl("speak", 10, toSpeak); | ||
| }; | ||
|
|
||
| fluid.textToSpeech.cancel = function (that) { | ||
| that.queue = []; | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl("cancel", 10); | ||
| }; | ||
|
|
||
| })(jQuery, fluid_2_0_0); | ||
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.
I think my suggestion of sticking this in the queue is better, since otherwise you have to make extra effort to clean it up when the utterance is done.
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.
The
queuemember contains the queue of texts yet to be spoken; it has nothing to do with the queueing behaviour I implemented, which (as you state above) is probably more correctly described as a throttle to prevent more pause/resume commands from being sent via the wrapper while outstanding pause/resume requests are still resolving.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.
Yes, that's right - the queue has nothing to do with your throttling, but it's still an appropriate place for the utterance. Note that it is created one for one with each queue element: https://github.com/fluid-project/infusion/blob/master/src/components/textToSpeech/js/TextToSpeech.js#L188