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

FLUID-6177: Adds a selection reader to the orator #903

Merged
merged 101 commits into from Jul 20, 2018

Conversation

Projects
None yet
3 participants

jobara added some commits Aug 2, 2017

FLUID-6176: Partial first pass at adding text highlight to Self Voicing.
Still need to strip out non-rendered text from being read, and add unit
tests.
FLUID-6176: Fixing up highlighting and cleanup
The highlighting should be following the utterances to highlight the
word currently being voiced.
FLUID-6175: Refactored selfVoicing enactor to a component.
This is done in prepartion of adding in a ui widget for controlling the
synthesis.
FLUID-6175: Refactoring highlighting.
* renamed from mark to highlight
* added invokers for adding and removing the highlight
* cleaned up the listeners to use the new invokers
FLUID-6175: Split orator into a domReader grade. Started a widget.
Started the widget to control the orator, play/pause. Currently not
wired up or tested.
FLUID-6175: First pass at orator parent component.
Need to modify so that pausing and playing doesn't restart the reading
but continues from where it left off.

jobara added some commits Jul 10, 2018

FLUID-6177: Fixing orator tests for IE 11.
Not running tests that require the speech synthesis api.
@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

@amb26 could you please review this PR. It includes all of the changes to update the self voicing preference.

FLUID-6175: Update the orator icons to include play and pause.
These icons are now used instead of relying on the glyphs in the 
available font.
}
},
listeners: {
"utteranceOnEnd.domReaderStop": [{

This comment has been minimized.

Copy link
@amb26

amb26 Jul 13, 2018

Member

No need for this to be an array since any more than one member of a namespaced listener would be discarded

}
});

fluid.orator.controller.toggleState = function (that, path, state) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Docs for this function given its requirements seem a bit loose - might be helpful to mention it is designed to be invoked directly as a click handler. What is the reason for the odd polymorphism based on accepting the state either from the direct argument or from the model?

args: ["{that}", "{arguments}.0"]
},
play: {
funcName: "fluid.orator.domReader.speak",

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Awkward mismatch here between the name of the invoker and its implementing function.

*
* @return {String} - The parsed text combined into a String.
*/
fluid.orator.domReader.parsedToString = function (parsed) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

This should also be an invoker on domReader since it is the inverse of one of its operations, and invoked that way

* to map the synthesized speech to the location of each word in the DOM; which is useful for highlighting words as
* they are being read.
*
* @typedef {Array} DomWordMappings

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

On the one hand I don't think this needs to be a typedef since it is just an array, and on the other hand I think it could usefully be documented what it is actually an array of - that is, you should use {DomWordMap[]} as the @param type below

* Returns the index of the closest data point from the parseQueue based on the boundary provided.
*
* @param {Component} that - The component
* @param {Integer} boundary - The boundary value used to compare against the blockIndex of the parsed data points.

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Looks like this parameter is optional

This comment has been minimized.

Copy link
@jobara

jobara Jul 16, 2018

Author Member

The function won't throw an error, but it will just return undefined. I've added that to the documentation for boundary. It was already included in the documentation for the return value and I have left it there as well.

fluid.orator.domReader.getClosestIndex = function (that, boundary) {
var parseQueue = that.parseQueue;

if (!parseQueue.length || !fluid.isValue(boundary) && boundary <= 0) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

It can't both be not a value and <= 0

return undefined;
}

while (index >= 0) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Thanks, I think the workflow of this function is much more comprehensible now. Also it will be hugely more efficient :)

return undefined;
};

var maxIndex = Math.max(parseQueue.length - 1, 0);

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Stray space here

speaking: false,
enabled: true
},
parseQueuelength: 0,

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Capitalising this as parseQueueLength feels nicer

}
},
modelListeners: {
"parseIndex": {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

As you mentioned in our call, this might also be invalidated by a change in parseQueueLength that does not at the same time change parseIndex. You can use the "path" member to listen to both at once: https://docs.fluidproject.org/infusion/development/ChangeApplierAPI.html#the-path-entry-in-a-model-listener-declaration

};

fluid.orator.domReader.removeExtraWhiteSpace = function (text) {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

This function could do with some extra whitespace removed :)

*
* @param {Component} that - the component
* @param {Object} utteranceEventMap - a mapping from SpeechSynthesisUtterance events to component events.
* @param {Object} utteranceOpts - options to configure the SpeechSynthesis utterance with.

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Looks like this could usefully be a @typedef since it appears in a few places

invokers: {
renderMarkup: "fluid.identity({that}.options.markup.container)",
renderContainer: "fluid.containerRenderingView.renderContainer({that}, {that}.renderMarkup, {that}.addToParent)",
addToParent: {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 15, 2018

Member

Yup, I think this looks pretty good now and makes a fair amount of sense

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@amb26 thanks for the review. I believe I've addressed all of your comments. The PR is now ready for more review. (NOTE: as of writing this comment, the PR hadn't updated even though the changes have been pushed up to my branch.)

@amb26 amb26 merged commit 68b3845 into fluid-project:master Jul 20, 2018

8 checks passed

buildkite/infusion Build #322 passed (19 minutes, 11 seconds)
Details
buildkite/infusion/browser-tests Passed (8 minutes, 2 seconds)
Details
buildkite/infusion/build Passed (8 minutes, 29 seconds)
Details
buildkite/infusion/cleanup Passed (7 seconds)
Details
buildkite/infusion/code-linter Passed (18 seconds)
Details
buildkite/infusion/node-tests Passed (26 seconds)
Details
buildkite/infusion/pipeline Passed (11 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.