Utterance does not support being added to 2+ UtteranceuQueues

[zepumph]

Discovered while problem solving for https://github.com/phetsims/friction/issues/204 with @jessegreenberg. Basically this is how we wanted our voicing response + description alert pattern to be:

      myUtterance.alert = 'oh hello';
      phet.joist.sim.utteranceQueue.addToBack( myUtterance );
      voicingUtteranceQueue.addToBack( myUtterance );

Here are all the reasons why this doesn't work. In general it is because Utterance stores data from being in the queue, and based on how many times it has been sent through a queue.

• Utterance.numberOfTimesAlerted would be incremented by both queues, thus the array feature of Utterance.alert would yield different results for each utterance.
• UtteranceQueue sets timing variables on the Utterance based on time in queue etc (stableTime and timeInQueue)

There may be more!!

Thus, we decided that we should add in an assertion to Utterance that it is only added to a single UtteranceQueue at a time. This will most likely need a fair number of usage changes (for sims supporting voicing).

@jessegreenberg, another though here, could we prototype a subtype of Utterance that would support multiple queues? Or perhaps a way to bolster Utterance so that its timing variables were a map instead. I think if each utteranceQueue had a unique identifier, then it would be pretty easy to support. I'm not ready to implement anything here yet, I'm going to think about it and implement next week.

[jessegreenberg]

Or perhaps a way to bolster Utterance so that its timing variables were a map instead

Yes, that could work well too.

[zepumph]

If we make Utterance supportive of multiple queues by creating a map of data, then perhaps the "numberOfTimesAlerted" can also be kept in that data, since it in a general case it would depend on what the specific queue context is to alert. This seems like it could be weird though:

const x = new Utterance( {
  alert: [ 1, 2, 3, 4, 5, 6, 7 ]
} );

voicingUtterance.addToBack( x );
// announce 1
descriptionUtterance.addToBack( x );
// announce 1

voicingUtterance.addToBack( x );
// announce 2
descriptionUtterance.addToBack( x );
// announce 2

descriptionUtterance.addToBack( x );
// announce 3
descriptionUtterance.addToBack( x );
// announce 4
descriptionUtterance.addToBack( x );
// announce 5
descriptionUtterance.addToBack( x );
// announce 6
descriptionUtterance.addToBack( x );
// announce 7

voicingUtterance.addToBack( x );
// announce 3

I have a hard time feeling like this isn't overly specific to our case. Perhaps that is alright, because in general people shouldn't use the array alert feature unless they are very knowledgeable about why they need that. Also I feel like once we create a map, it would be pretty easy to create an option that asserts that an Utterance's alert is not changed from the time it enters the queue to the time it exists. That is likely quite important.

[zepumph]

Assigning @jessegreenberg for comment.

[jessegreenberg]

I am a bit torn between what might be the best API for UtteranceQueue/Utterance and what is best for PhET's usage. https://github.com/phetsims/utterance-queue/issues/20#issuecomment-889881374 could be confusing, but it seems necessary to support putting an Utterance through more than one UtteranceQueue t a time.

I think ideally, Utterance should not be aware of the UtteranceQueue it is in. And creating a Map for Utterance that goes from UtteranceQueue to timing/alert counters/other variables adds complexity. But the current alternative of creating one Utterance per UtteranceQueue would add extra boilerplate for PhET.

Just brainstorming something else, what if we had a manager class for all UtteranceQueues like phetUtteranceQueueManager with functions like addToBackOfDescriptionUtteranceQueue, addToBackOfVoicingUtteranceQueue, addToBackOfAllUtteranceQueues.

The implementation of addToBackOfAllUtteranceQueues might look like this (messy, untested, just for discussion):

```js addToBackOfAllUtteranceQueues( alertable ) { if ( !( alertable instanceof Utterance ) ) { alertable = new Utterance( alertable ); } let alertableCopy = null; // if we already have a copy of the alertable, use that one so that adding to back of the UtteranceQueue so // that features that use reference checks like `alertStableDelay` will work. if ( this.alertableCopyMap.has( alertable ) ) { alertableCopy = this.alertableCopyMap.get( alertable ) // maybe set other fields as necessary alertableCopy.alert = alertable.alert; } else { // save a reference to the copy so that it can be used again const alertableCopyForVoicingQueue = alertable.copy(); // implement copy in Utterance this.alertableCopyMap.set( alertable, alertableCopy ); } descriptionUtteranceQueue.addToBack( alertable ); voicingUtteranceQueue.addToBack( alertableCopyForVoicing ); } ```

I dunno, I liked it less as I typed it out. Now we need forwarding functions for each UtteranceQueue in phetUtteranceQueueManager. My hope was we had something more simple in utterance-queue while also having less boilerplate at call sites.

[zepumph]

One side note, it seems like the incrementing side effect in getTextToAlert should be instead incremented in UtteranceQueue. That would be good to do no matter the outcome of this issue, but it seems related, so I am tagging a TODO in this issue as I work on https://github.com/phetsims/aqua/issues/127

[zepumph]

@jessegreenberg and I discussed, and we like this sort of method, where instead of the queue objects being Utterances, we wrap them in data that is only for that single queue (and really for that single instance of the Utterance added to that queue.

  UtteranceQueue.wrapUtterance( utterance ) {
    return {
      stableTime: 0,
      timeInQueue: 0,
      utterance: utterance
    }
  }

[zepumph]

I made some good progress on this today, but I had to give up because I ran out of time just when I ran into a mistake I made about if removeUtterance takes an utterance or an utteranceWrapper. Likely I should make two different methods (and only one is public).

```diff Index: utterance-queue/js/UtteranceQueue.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/utterance-queue/js/UtteranceQueue.js b/utterance-queue/js/UtteranceQueue.js --- a/utterance-queue/js/UtteranceQueue.js (revision cb8c9ea5907ccd09524c8e03af0971f87d1aed7a) +++ b/utterance-queue/js/UtteranceQueue.js (date 1628377094452) @@ -5,6 +5,10 @@ * things in a first-in-first-out manner, but it is possible to send an alert directly to the front of * the queue. Items in the queue are sent to AT front to back, driven by AXON/timer. * + * An Utterance instance is used as a unique value to the UtteranceQueue. If you add an Utterance a second time to the, + * queue, the queue will remove the previous instance, and treat the new addition as if the Utterance has been in the + * queue the entire time, but in the new position. + * * AT are inconsistent in the way that they order alerts, some use last-in-first-out order, * others use first-in-first-out order, others just read the last alert that was provided. This queue * manages order and improves consistency. @@ -64,7 +68,8 @@ // initialized (cheers). See initialize(); this._initialized = !options.implementAsSkeleton; - // @public (tests) {Array.} - array of Utterances, spoken in first to last order + // @public (tests) {Array.} - array of UtteranceWrappers, see private class for details. Spoken + // first in first out (fifo). Earlier utterances will be lower in the Array. this.queue = []; // whether or not Utterances moving through the queue are read by a screen reader @@ -117,8 +122,8 @@ return; } - utterance = this.prepareUtterance( utterance ); - this.queue.push( utterance ); + const utteranceWrapper = this.prepareUtterance( utterance ); + this.queue.push( utteranceWrapper ); } /** @@ -147,55 +152,69 @@ return; } - utterance = this.prepareUtterance( utterance ); - this.queue.unshift( utterance ); + const utteranceWrapper = this.prepareUtterance( utterance ); + this.queue.unshift( utteranceWrapper ); } /** - * Create an Utterance for the queue in case of string and clears the queue of duplicate utterances. + * Create an Utterance for the queue in case of string and clears the queue of duplicate utterances. This will also + * remove duplicates in the queue, and update to the most recent timeInQueue variable. * @private * * @param {AlertableDef} utterance - * @returns {Utterance} + * @returns {UtteranceWrapper} */ prepareUtterance( utterance ) { if ( !( utterance instanceof Utterance ) ) { utterance = new Utterance( { alert: utterance } ); } + const utteranceWrapper = new UtteranceWrapper( utterance ); + // If there are any other items in the queue of the same type, remove them immediately because the added // utterance is meant to replace it - this.removeUtterance( utterance, { - assertExists: false + this.removeUtterance( utteranceWrapper, { + assertExists: false, + transferTimeInQueue: true } ); // Reset the time watching utterance stability since it has been added to the queue. - utterance.stableTime = 0; + utteranceWrapper.stableTime = 0; - return utterance; + return utteranceWrapper; } /** * Remove an Utterance from the queue. This function is only able to remove `Utterance` instances, and cannot remove * other AlertableDef types. + * TODO: this has to take an Utterance, not an UtteranceWrapper, so perhaps we need to find another way to support moving over timeInQueue from wrappers, https://github.com/phetsims/utterance-queue/issues/20 * @public - * @param {Utterance} utterance + * @param {UtteranceWrapper} utteranceWrapper * @param {Object} [options] */ - removeUtterance( utterance, options ) { - assert && assert( utterance instanceof Utterance ); + removeUtterance( utteranceWrapper, options ) { + assert && assert( utteranceWrapper instanceof UtteranceWrapper ); options = merge( { // If true, then an assert will make sure that the utterance is expected to be in the queue. - assertExists: true + assertExists: true, + transferTimeInQueue: false }, options ); - assert && options.assertExists && assert( this.queue.indexOf( utterance ) >= 0, + assert && options.assertExists && assert( this.queue.indexOf( utteranceWrapper ) >= 0, 'utterance to be removed not found in queue' ); - // remove all occurrences, if applicable - _.remove( this.queue, currentUtterance => currentUtterance === utterance ); + // remove all occurrences, if applicable. This side effect is to make sure that the timeInQueue is transferred between adding the same Utterance. + _.remove( this.queue, currentUtterance => { + if ( currentUtterance.utterance === utteranceWrapper.utterance ) { + if ( options.transferTimeInQueue ) { + utteranceWrapper.timeInQueue = currentUtterance.timeInQueue; + return true; + } + } + return false; + } ); } /** @@ -232,12 +251,13 @@ // is greater than the amount of time that the utterance has been sitting in the queue let nextUtterance = null; for ( let i = 0; i < this.queue.length; i++ ) { - const utterance = this.queue[ i ]; + const utteranceWrapper = this.queue[ i ]; // if we have waited long enough for the utterance to become "stable" or the utterance has been in the queue // for longer than the maximum delay override, it will be spoken - if ( utterance.stableTime > utterance.alertStableDelay || utterance.timeInQueue > utterance.alertMaximumDelay ) { - nextUtterance = utterance; + if ( utteranceWrapper.stableTime > utteranceWrapper.utterance.alertStableDelay || + utteranceWrapper.timeInQueue > utteranceWrapper.utterance.alertMaximumDelay ) { + nextUtterance = utteranceWrapper.utterance; this.queue.splice( i, 1 ); break; @@ -255,7 +275,14 @@ * @returns {boolean} */ hasUtterance( utterance ) { - return _.includes( this.queue, utterance ); + for ( let i = 0; i < this.queue.length; i++ ) { + const utteranceWrapper = this.queue[ i ]; + if ( utterance === utteranceWrapper.utterance ) { + return true; + } + + } + return false; } /** @@ -348,9 +375,6 @@ this.announcer.announce( nextUtterance, nextUtterance.announcerOptions ); - // after speaking the utterance, reset time in queue for the next time it gets added back in - nextUtterance.timeInQueue = 0; - //this.phetioEndEvent(); } } @@ -424,6 +448,33 @@ } } +// One instance per entry in the Queue +class UtteranceWrapper { + constructor( utterance ) { + + // @public + this.utterance = utterance; + + // @public {number} - In ms, how long this utterance has been in the queue. The + // same Utterance can be in the queue more than once (for utterance looping or while the utterance stabilizes), + // in this case the time will be since the first time the utterance was added to the queue. + this.timeInQueue = 0; + + // @public {number} - in ms, how long this utterance has been "stable", which + // is the amount of time since this utterance has been added to the utteranceQueue. + this.stableTime = 0; + } + + /** + * Reset variables that track instance variables related to time. + * @public + */ + resetTimingVariables() { + this.timeInQueue = 0; + this.stableTime = 0; + } +} + UtteranceQueue.UtteranceQueueIO = new IOType( 'UtteranceQueueIO', { valueType: UtteranceQueue, documentation: 'Manages a queue of Utterances that are read in order by a screen reader.', Index: utterance-queue/js/Utterance.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/utterance-queue/js/Utterance.js b/utterance-queue/js/Utterance.js --- a/utterance-queue/js/Utterance.js (revision cb8c9ea5907ccd09524c8e03af0971f87d1aed7a) +++ b/utterance-queue/js/Utterance.js (date 1628376834929) @@ -88,15 +88,6 @@ // @public (read-only, scenery-phet-internal) this.predicate = options.predicate; - // @public {number} (scenery-phet-internal) - In ms, how long this utterance has been in the queue. The - // same Utterance can be in the queue more than once (for utterance looping or while the utterance stabilizes), - // in this case the time will be since the first time the utterance was added to the queue. - this.timeInQueue = 0; - - // @public (scenery-phet-internal) {number} - in ms, how long this utterance has been "stable", which - // is the amount of time since this utterance has been added to the utteranceQueue. - this.stableTime = 0; - // @public (read-only, scenery-phet-internal) {number} - In ms, how long the utterance should remain in the queue // before it is read. The queue is cleared in FIFO order, but utterances are skipped until the delay time is less // than the amount of time the utterance has been in the queue @@ -177,15 +168,6 @@ this.alertStableDelay = delay; } - /** - * Reset variables that track instance variables related to time. - * @public - */ - resetTimingVariables() { - this.timeInQueue = 0; - this.stableTime = 0; - } - /** * @public * @returns {string} @@ -199,7 +181,6 @@ */ reset() { this.numberOfTimesAlerted = 0; - this.resetTimingVariables(); } } Index: aqua/js/take-snapshot.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/aqua/js/take-snapshot.js b/aqua/js/take-snapshot.js --- a/aqua/js/take-snapshot.js (revision 0f55040a741d7fc635d4a56e352bd294a6b57315) +++ b/aqua/js/take-snapshot.js (date 1628376850832) @@ -173,7 +173,7 @@ concatHash += hashedPDOMHTML; const descriptionUtteranceQueue = iframe.contentWindow.phet.joist.display.utteranceQueue.queue; - const utteranceTexts = descriptionUtteranceQueue.map( utterance => utterance.toString() ); + const utteranceTexts = descriptionUtteranceQueue.map( utteranceWrapper => utteranceWrapper.utterance.toString() ); descriptionAlertData.utterances = utteranceTexts; const utterancesHash = hash( utteranceTexts + '' ); descriptionAlertData.hash = utterancesHash; Index: utterance-queue/js/UtteranceTests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/utterance-queue/js/UtteranceTests.js b/utterance-queue/js/UtteranceTests.js --- a/utterance-queue/js/UtteranceTests.js (revision cb8c9ea5907ccd09524c8e03af0971f87d1aed7a) +++ b/utterance-queue/js/UtteranceTests.js (date 1628376834957) @@ -182,7 +182,8 @@ assert.ok( utteranceQueue.queue.length === 1, 'same Utterance should override in queue' ); await timeout( sleepTiming ); - assert.ok( myUtterance.stableTime >= myUtterance.timeInQueue, 'utterance should be in queue for at least stableDelay' ); + const utteranceWrapper = utteranceQueue.queue[ 0 ]; + assert.ok( utteranceWrapper.stableTime >= utteranceWrapper.timeInQueue, 'utterance should be in queue for at least stableDelay' ); assert.ok( utteranceQueue.queue.length === 1, 'Alert still in queue after waiting less than alertStableDelay but more than stepInterval.' ); await timeout( stableDelay ); ```

[zepumph]

I had more luck this morning on this! I split out the removal methods to two different functions, one for utteranceWrapper and one for utterance.

This was a great first step for this issue, and tests are passing. My testing didn't show any differences (50 fuzzing frames of snapshot in molarity all had identical utterances emitted). From here we can work on the alert as an array/loopAlerts/numberOfTimesAlerted stuff.

[jessegreenberg]

Progress is looking good! With the movement of resetTimingVariables this usage in AccessibleValueHandler https://github.com/phetsims/sun/blob/ca4c2fb861e678adc960a84f67b90875eaab3e1c/js/accessibility/AccessibleValueHandler.js#L414 is causing an error. That is the only usage of the function, is it safe to remove? I would think that alertStableDelay would provide the delay we need each time it is added to back so it shouldn't need to be reset manually. But maybe I don't understand. I am going to remove it for now to suppress the error.

[zepumph]

You did just great. Thanks for catching my error @jessegreenberg.

[zepumph]

I ran into trouble with an Utterance that has an array as an alert (MovementDescriver.alert()) when collecting responses for voicing. I think it is time to support this now.

[zepumph]

I'm so torn about how to proceed here! I thought I would be ok with only supporting array alerts a single Utterance instance per queue (see patch below). It worked great for BookMovementDescriber, but then I got to BorderAlert.alert() and felt again like we should do a count per utteranceQueue, and support adding an Utterance to any queue, and the cycling will be unique to the queue it is added to (proposal from the snippet in https://github.com/phetsims/utterance-queue/issues/20#issuecomment-889881374).

I will need to come back to this, as I am worried I will make the wrong decision.

Index: js/friction/view/describers/BookMovementDescriber.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/friction/view/describers/BookMovementDescriber.js b/js/friction/view/describers/BookMovementDescriber.js
--- a/js/friction/view/describers/BookMovementDescriber.js  (revision ae2f4a941a4c130a4f53323f0b72e4a7fb5656b3)
+++ b/js/friction/view/describers/BookMovementDescriber.js  (date 1628728140842)
@@ -63,7 +63,13 @@
     this.model = model;

     // @private - special verbose alert for the first 2 times, then use the default
-    this.bottomUtterance = new Utterance( {
+    this.bottomDescriptionUtterance = new Utterance( {
+      alert: [ downRubFastOrSlowString, downRubFastOrSlowString, DEFAULT_MOVEMENT_DESCRIPTIONS.DOWN ],
+    } );
+
+    // @private - special verbose alert for the first 2 times, then use the default. A separate Utterance is needed to
+    // support array alerts.
+    this.bottomVoicingUtterance = new Utterance( {
       alert: [ downRubFastOrSlowString, downRubFastOrSlowString, DEFAULT_MOVEMENT_DESCRIPTIONS.DOWN ],
       announcerOptions: {
         cancelOther: false
@@ -132,7 +138,8 @@

     // if contacted and DOWN, we have a special alert
     else if ( this.model.contactProperty.get() && direction === DirectionEnum.DOWN ) {
-      this.alert( this.bottomUtterance );
+      this.alert( this.bottomDescriptionUtterance );
+      this.alert( this.bottomVoicingUtterance );
     }

     // base case
@@ -169,7 +176,8 @@
    */
   reset() {
     super.reset();
-    this.bottomUtterance.reset();
+    this.bottomDescriptionUtterance.reset();
+    this.bottomVoicingUtterance.reset();
     this.contactedAlertPair.reset();
     this.separatedAlertPair.reset();
   }

[zepumph]

@jessegreenberg and I discussed the above. Given BorderAlert.alert and MovementDecriber.alert, it seems like we should try hard to support 1 Utterance in 2 queues.

Some strategies that were hard for us to think through:

• Utterance.perQueueData[utteranceQueue.uniqueID].numberOfTimesAlerted, the biggest problem for this one was that now Utterance.getAlertText() would need to have an instance of the utteranceQueue it applies to.
• UtteranceQueue.everyUtteranceEver[ utterance ].numberOfTimesAlerted. This seems to have a memory leak, and strips array-supported Utterances from being able to access their own alert value without the UtteranceQueue instance at the ready.
• In theory we likely the idea of storing this on the UtteranceWrapper, but it didn't seem to take shape. Right now those don't persist between times called in the utteranceQueue.
• Subtyping utterance to add in this feature of array-support. We want to explore this more, and @zepumph will do this next.
• We also talked more generally about how we might support this if we had had it as a constraint from the very beginning of utteranceQueue. We didn't come to any large breakthroughs though.

[zepumph]

Lots has happened since the last time we had this discussion. I think that all usages of array-supported Utterance alerts has been deleted. I recommend that we get rid of that support. I think it will greatly improve the utterance-queue library. I'll touch base with @jessegreenberg on friday about this.

[zepumph]

I discussed with @jessegreenberg, and we will remove the array-alert support. In the future if this comes up again, perhaps we can subtype or come up with another solution. Then I'll go over this issue, and mark it for review by @jessegreenberg.

[zepumph]

Pinging my dang self on this again. As I hit it in https://github.com/phetsims/sun/issues/742

[zepumph]

I will remove the array Utterance alerts in https://github.com/phetsims/utterance-queue/issues/67. @jessegreenberg this issue is now ready for review.

[jessegreenberg]

I reviewed the three commits in this issue that took the timing variables out of Utterance and put them in UtteranceWrapper so that Utterances can be in multiple queues at once. Changes make sense to me and this has been working well for us for some time.

I reviewed the other instance variables of Utterance and it still seems like all work if in multiple queues.

Only thing I spotted was an option that has since been removed from removeUtterance called assertExists, I removed its usage from MolarityAlertManager.

Otherwise I think this can be closed, thanks for making these improvements @zepumph!

[zepumph]

Excellent! Thanks