Assertion: "About to add the priority listener twice and only one should exist on the Utterance...."

[jessegreenberg]

While working on review for #43 I hit this assertion

Assertion failed: About to add the priority listener twice and only one should exist on the Utterance. The listener should have been removed by removeOthersAndUpdateUtteranceWrapper.

I think it is consistently happening in Chrome when pressing the voicing toolbar buttons rapidly.

[jessegreenberg]

OK I identified the case, it is actually pretty simple. We are not handling what should happen when you add an utterance to the queue while the Announcer is announcing that Utterance.

const testUtterance = new phet.utteranceQueue.Utterance( { alert: 'This is a test utterance.', announcerOptions: { cancelSelf: false } } );

phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );

window.setTimeout( () => {
    phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );
}, 500 );

[jessegreenberg]

For behavior, it seems important that if you add the Utterance to the queue while the Announcer is still speaking it it should not interrupt. alertMaximumDelay requires that an Announcer can be speaking an Utterance while that Utterance still sits in the queue.

We cannot simply remove the assertion. If this ran without error, the listener would be removed when the Announcer was done speaking the Utterance the first time. So changing the priorityProperty would have no effect while it was speaking the Utterance the second time.

[jessegreenberg]

Brainstorming fixes:

• Only add the priorityProperty listener to the Utterance if the Announcer is not currently speaking that Utterance. Only remove the priorityProperty listener on end if the Utterance is not still in the queue.
  • Seems possible, but also kind of hacky. We would need to get the announcer's currentlySpeakingUtterance.
• Change utteranceToPriorityListenerMap so that it is not Map<Utterance,function> or change it so that we can have more than one listener per Utterance.
  • I can't think of anything else that would work well to map to the listener so we can find it again to remove.
• Change UtteranceQueue so that it ONLY is responsible for priorityProperty among its own queue. Always remove the priorityProperty listener when the Utterance has been given to the Announcer. Make the Announcer responsible for handling priorityProperty changes while it is announcing an Utterance.
  • This makes sense to me. But the structure isn't quite right. The priorityProperty listener on the Announcer (at least for voicingManager) would need to look at the UtteranceQueue.queue to see if the next item has a higher priority and should interrupt the currentlySpeakingUtterance. Announcer shouldn't be aware of UtteranceQueue (ideally). So maybe instead of having the Announcer manage this listener, it should still be on the UtteranceQueue, but be separate from the listeners in utteranceToPriorityListenerMap.

[zepumph]

Change utteranceToPriorityListenerMap so that it is not Map<Utterance,function> or change it so that we can have more than one listener per Utterance.

I feel like this is the way to go, but I would make it a list of functions, so you can remove the one that applies to you. Sorta like the "count" features of scenery node structures. We just said that a constraint of the system (Announcer + UtteranceQueue) is that one Utterance can be in multiple stages of the system at the same time. If duplicates are allowed, then changing the Map to be more tolerant feels great to me.

Thoughts?

[jessegreenberg]

I like that, but I am struggling with how to find the right listener to remove from the list.
EDIT: I suppose that the listeners are simple enough that it doesn't matter. They all do the same work on the utterance. so the map could look like Map<Utterance,function[]> and whenever it is time to remove a listener from the list, we just remove the first one?

[jessegreenberg]

I found another bug that may mean further changes to utteranceToPriorityListenerMap: #47 Determined to be a non-issue.

[jessegreenberg]

Sorta like the "count" features of scenery node structures.

I liked this idea, this morning I tried an approach where we use counting variables to decide whether to add or remove a priorityProperty listener. It is basically a way to implement

Only add the priorityProperty listener to the Utterance if the Announcer is not currently speaking that Utterance. Only remove the priorityProperty listener on end if the Utterance is not still in the queue.

without looking at the Announcer.

UtteranceWrapper has a counting variable usageCount that counts "usages"

• When an Utterance is added to the queue, increment counting variable.
• When an Utterance is removed from the queue, decrement counting variable.
• When Utterance is announced with announcer.announce() increment counting varaible.
• When an Utterance is finished from announcementCompleteEmitter, decrement counting variable.

If counting variable is greater than zero when it is time to add or remove a priorityProperty listener we do nothing. In that case, the Utterance exists in the queue or is still being spoken by the Announcer.

So far it is working well. I am not seeing this problem anymore and unit tests are passing but it feels pretty complicated and I suspect there is an easier way. Here is the patch with this solution though:

Index: js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/UtteranceQueue.js b/js/UtteranceQueue.js
--- a/js/UtteranceQueue.js	(revision 439ba41f11aca54bd94e0878ccc0ec83518f2730)
+++ b/js/UtteranceQueue.js	(date 1642785722172)
@@ -89,10 +89,18 @@
     // utteranceToPriorityListenerMap.
     this.announcer.announcementCompleteEmitter.addListener( utterance => {
 
+      const utteranceWrapper = Array.from( this.utteranceToPriorityListenerMap.keys() ).find( utteranceWrapperInMap => utteranceWrapperInMap.utterance === utterance );
+
       // TODO: Can we replace this with an assertion to enforce that it exists? It breaks when using voicingManager.speakIgnoringEnabled but it shouldn't. See https://github.com/phetsims/joist/issues/752
       // assert && assert( this.utteranceToPriorityListenerMap.has( utterance ), 'Utterance missing from utteranceToPriorityListenerMap' );
-      if ( this.utteranceToPriorityListenerMap.has( utterance ) ) {
-        this.removePriorityListener( utterance );
+      if ( this.utteranceToPriorityListenerMap.has( utteranceWrapper ) ) {
+        assert && assert( utteranceWrapper.usageCount > 0, 'Utterance was used by Announcer, it should have a usage count' );
+        utteranceWrapper.usageCount--;
+        if ( assert && utteranceWrapper.usageCount > 0 ) {
+          assert( this.queue.includes( utteranceWrapper ), 'Done speaking, only way utteranceWrapper can have usage is if it exists in the queue' );
+        }
+
+        this.removePriorityListener( utteranceWrapper );
       }
     } );
 
@@ -137,6 +145,8 @@
 
     // Remove identical Utterances from the queue and wrap with a class that will manage timing variables.
     const utteranceWrapper = this.prepareUtterance( utterance );
+    assert && assert( utteranceWrapper.usageCount <= 1, 'At this point the Utterance cannot be in the queue because of prepareUtterance but may be in the Announcer, there is at most one usage.' );
+    utteranceWrapper.usageCount++;
 
     // Add to the queue before prioritizing so that we know which Utterances to prioritize against
     this.queue.push( utteranceWrapper );
@@ -179,13 +189,17 @@
    * @param utteranceWrapper {UtteranceWrapper}
    */
   addPriorityListenerAndPrioritizeQueue( utteranceWrapper ) {
-    assert && assert( !this.utteranceToPriorityListenerMap.has( utteranceWrapper.utterance ),
-      'About to add the priority listener twice and only one should exist on the Utterance. The listener should have been removed by removeOthersAndUpdateUtteranceWrapper.' );
-    const priorityListener = () => {
-      this.prioritizeUtterances( utteranceWrapper );
-    };
-    utteranceWrapper.utterance.priorityProperty.lazyLink( priorityListener );
-    this.utteranceToPriorityListenerMap.set( utteranceWrapper.utterance, priorityListener );
+
+    if ( utteranceWrapper.usageCount <= 1 ) {
+      assert && assert( !this.utteranceToPriorityListenerMap.has( utteranceWrapper ),
+        'About to add the priority listener twice and only one should exist on the Utterance. The listener should have been removed by removeOthersAndUpdateUtteranceWrapper.' );
+      const priorityListener = () => {
+        this.prioritizeUtterances( utteranceWrapper );
+      };
+
+      utteranceWrapper.utterance.priorityProperty.lazyLink( priorityListener );
+      this.utteranceToPriorityListenerMap.set( utteranceWrapper, priorityListener );
+    }
 
     this.prioritizeUtterances( utteranceWrapper );
   }
@@ -245,6 +259,13 @@
     // remove all occurrences, if applicable
     const removedUtteranceWrappers = _.remove( this.queue, utteranceWrapperToUtteranceMapper );
 
+    removedUtteranceWrappers.forEach( utteranceWrapper => {
+      utteranceWrapper.usageCount -= removedUtteranceWrappers.length;
+
+      assert && assert( utteranceWrapper.usageCount <= 1, 'The Utterance is only used by the Announcer if at all, should be one or less usages' );
+      assert && assert( utteranceWrapper.usageCount >= 0, 'negative usages??' );
+    } );
+
     if ( options.removePriorityListener ) {
       this.removePriorityListeners( removedUtteranceWrappers );
     }
@@ -331,12 +352,14 @@
     assert && assert( utteranceWrapper instanceof UtteranceWrapper );
 
     const times = [];
+    const usageCounts = [];
 
     // we need all the times, in case there are more than one wrapper instance already in the Queue.
     for ( let i = 0; i < this.queue.length; i++ ) {
       const currentUtteranceWrapper = this.queue[ i ];
       if ( currentUtteranceWrapper.utterance === utteranceWrapper.utterance ) {
         times.push( currentUtteranceWrapper.timeInQueue );
+        usageCounts.push( currentUtteranceWrapper.usageCount );
       }
     }
 
@@ -344,8 +367,18 @@
       utteranceWrapper.timeInQueue = Math.max( ...times );
     }
 
+    // Make sure that the usageCount from the utterances that were removed is propagated to the new utteranceWrapper
+    // so that we know if announcer is still announcing this Utterance.
+    if ( usageCounts.length >= 1 ) {
+      utteranceWrapper.usageCount = Math.max( ...usageCounts );
+    }
+
     // remove all occurrences, if applicable. This side effect is to make sure that the timeInQueue is transferred between adding the same Utterance.
     const removedWrappers = _.remove( this.queue, currentUtteranceWrapper => currentUtteranceWrapper.utterance === utteranceWrapper.utterance );
+    utteranceWrapper.usageCount -= removedWrappers.length;
+    assert && assert( utteranceWrapper.usageCount <= 1, 'Only usage could be for the Announcer, should be one or less Usages' );
+    assert && assert( utteranceWrapper.usageCount >= 0, 'negative usages??' );
+
     this.removePriorityListeners( removedWrappers );
   }
 
@@ -425,21 +458,21 @@
    * @param utteranceWrappers
    */
   removePriorityListeners( utteranceWrappers ) {
-    utteranceWrappers.forEach( utteranceWrapper => this.removePriorityListener( utteranceWrapper.utterance ) );
+    utteranceWrappers.forEach( utteranceWrapper => this.removePriorityListener( utteranceWrapper ) );
   }
 
   /**
    * @private
-   * @param utterance
+   * @param utteranceWrapper
    */
-  removePriorityListener( utterance ) {
-    const listener = this.utteranceToPriorityListenerMap.get( utterance );
+  removePriorityListener( utteranceWrapper ) {
+    const listener = this.utteranceToPriorityListenerMap.get( utteranceWrapper );
 
     // The same Utterance may exist multiple times in the queue if we are removing duplicates from the array,
     // so the listener may have already been removed.
-    if ( listener ) {
-      utterance.priorityProperty.unlink( listener );
-      this.utteranceToPriorityListenerMap.delete( utterance );
+    if ( listener && utteranceWrapper.usageCount === 0 ) {
+      utteranceWrapper.utterance.priorityProperty.unlink( listener );
+      this.utteranceToPriorityListenerMap.delete( utteranceWrapper );
     }
   }
 
@@ -564,6 +597,9 @@
     utteranceWrapper.stableTime = Number.POSITIVE_INFINITY;
     utteranceWrapper.timeInQueue = Number.POSITIVE_INFINITY;
 
+    assert && assert( utteranceWrapper.usageCount <= 1, 'At this point the Utterance cannot be in the queue because of prepareUtterance but may be in the Announcer, there is at most one usage.' );
+    utteranceWrapper.usageCount++;
+
     // addPriorityListenerAndPrioritizeQueue assumes the UtteranceWrapper is in the queue, add first
     this.queue.unshift( utteranceWrapper );
     this.addPriorityListenerAndPrioritizeQueue( utteranceWrapper );
@@ -591,6 +627,10 @@
 
       // only announce the utterance if not muted and the Utterance predicate returns true
       if ( !this._muted && utterance.predicate() && utterance.getAlertText( this.announcer.respectResponseCollectorProperties ) !== '' ) {
+
+        utteranceWrapper.usageCount++;
+        assert && assert( utteranceWrapper.usageCount === 2, 'Utterance in both queue and announcer at this point, should have two usages' );
+
         this.announcer.announce( utterance, utterance.announcerOptions );
         sentToAnnouncer = true;
       }
@@ -604,7 +644,7 @@
 
           // only remove the priority listener if it has not been received by the Announcer, otherwise the Announcer
           // will let us know when it is finished with it and we will remove the listener then
-          removePriorityListener: !sentToAnnouncer
+          removePriorityListener: true
         } );
       }
     }
@@ -672,6 +712,8 @@
     // in this case the time will be since the first time the utterance was added to the queue.
     this.timeInQueue = 0;
 
+    this.usageCount = 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;

[zepumph]

FYI, with this patch, I can test voicing code that doesn't use priority:

Index: js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/UtteranceQueue.js b/js/UtteranceQueue.js
--- a/js/UtteranceQueue.js	(revision 439ba41f11aca54bd94e0878ccc0ec83518f2730)
+++ b/js/UtteranceQueue.js	(date 1642787271019)
@@ -181,11 +181,11 @@
   addPriorityListenerAndPrioritizeQueue( utteranceWrapper ) {
     assert && assert( !this.utteranceToPriorityListenerMap.has( utteranceWrapper.utterance ),
       'About to add the priority listener twice and only one should exist on the Utterance. The listener should have been removed by removeOthersAndUpdateUtteranceWrapper.' );
-    const priorityListener = () => {
-      this.prioritizeUtterances( utteranceWrapper );
-    };
-    utteranceWrapper.utterance.priorityProperty.lazyLink( priorityListener );
-    this.utteranceToPriorityListenerMap.set( utteranceWrapper.utterance, priorityListener );
+    // const priorityListener = () => {
+    //   this.prioritizeUtterances( utteranceWrapper );
+    // };
+    // utteranceWrapper.utterance.priorityProperty.lazyLink( priorityListener );
+    // this.utteranceToPriorityListenerMap.set( utteranceWrapper.utterance, priorityListener );
 
     this.prioritizeUtterances( utteranceWrapper );
   }

[jessegreenberg]

Here is another solution that is more simple. It is also passing unit tests. It works by saving a reference to the UtteranceWrapper and priorityPropertyListener when we call announcer.announce. Then when we receive the announcementCompleteEmitter event, we unlink This way, the utteranceToPriorityListenerMap is only for Utterances that are in the UtteranceQueue.queue. It is fragile because it does not work under this condition (TODO in the patch):

    this.announcer.announcementCompleteEmitter.addListener( utterance => {

      // It is possible that this.announcer is used by a different UtteranceQueue. When the announcementCompleteEmitter
      // announces, it may not be for this queue. Would love the following assertions though.
      // TODO: This would break if both UtteranceQueues that share the same announcer both have the same Utterance at this.announcingUtteranceWrapper.utterance, https://github.com/phetsims/utterance-queue/issues/46
      // assert && assert( this.announcingUtteranceWrapper, 'no announcingUtteranceWrapper' );
      // assert && assert( this.announcingUtterancePriorityListener, 'no announcingUtterancePriorityListener' );
      if ( this.announcingUtteranceWrapper && utterance === this.announcingUtteranceWrapper.utterance ) {
        assert && assert( this.announcingUtteranceWrapper.utterance.priorityProperty.hasListener( this.announcingUtterancePriorityListener ) );
        this.announcingUtteranceWrapper.utterance.priorityProperty.unlink( this.announcingUtterancePriorityListener );

        this.announcingUtteranceWrapper = null;
        this.announcingUtterancePriorityListener = null;
      }
    } );

Full patch:

Index: js/UtteranceQueue.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/UtteranceQueue.js b/js/UtteranceQueue.js
--- a/js/UtteranceQueue.js	(revision 439ba41f11aca54bd94e0878ccc0ec83518f2730)
+++ b/js/UtteranceQueue.js	(date 1642788732997)
@@ -85,14 +85,24 @@
     // removed from the queue.
     this.utteranceToPriorityListenerMap = new Map();
 
+    this.announcingUtteranceWrapper = null;
+    this.announcingUtterancePriorityListener = null;
+
     // When the Announcer is done with an Utterance, remove priority listeners and remove from the
     // utteranceToPriorityListenerMap.
     this.announcer.announcementCompleteEmitter.addListener( utterance => {
 
-      // TODO: Can we replace this with an assertion to enforce that it exists? It breaks when using voicingManager.speakIgnoringEnabled but it shouldn't. See https://github.com/phetsims/joist/issues/752
-      // assert && assert( this.utteranceToPriorityListenerMap.has( utterance ), 'Utterance missing from utteranceToPriorityListenerMap' );
-      if ( this.utteranceToPriorityListenerMap.has( utterance ) ) {
-        this.removePriorityListener( utterance );
+      // It is possible that this.announcer is used by a different UtteranceQueue. When the announcementCompleteEmitter
+      // announces, it may not be for this queue. Would love the following assertions though.
+      // TODO: This would break if both UtteranceQueues that share the same announcer both have the same Utterance at this.announcingUtteranceWrapper.utterance, https://github.com/phetsims/utterance-queue/issues/46
+      // assert && assert( this.announcingUtteranceWrapper, 'no announcingUtteranceWrapper' );
+      // assert && assert( this.announcingUtterancePriorityListener, 'no announcingUtterancePriorityListener' );
+      if ( this.announcingUtteranceWrapper && utterance === this.announcingUtteranceWrapper.utterance ) {
+        assert && assert( this.announcingUtteranceWrapper.utterance.priorityProperty.hasListener( this.announcingUtterancePriorityListener ) );
+        this.announcingUtteranceWrapper.utterance.priorityProperty.unlink( this.announcingUtterancePriorityListener );
+
+        this.announcingUtteranceWrapper = null;
+        this.announcingUtterancePriorityListener = null;
       }
     } );
 
@@ -591,6 +601,17 @@
 
       // only announce the utterance if not muted and the Utterance predicate returns true
       if ( !this._muted && utterance.predicate() && utterance.getAlertText( this.announcer.respectResponseCollectorProperties ) !== '' ) {
+
+        assert && assert( this.announcingUtteranceWrapper === null, 'This should have been cleared' );
+        assert && assert( this.announcingUtterancePriorityListener === null, 'This should have been cleared' );
+
+        this.announcingUtteranceWrapper = utteranceWrapper;
+        this.announcingUtterancePriorityListener = () => {
+          this.prioritizeUtterances( utteranceWrapper );
+        };
+
+        utteranceWrapper.utterance.priorityProperty.link( this.announcingUtterancePriorityListener );
+
         this.announcer.announce( utterance, utterance.announcerOptions );
         sentToAnnouncer = true;
       }
@@ -604,7 +625,7 @@
 
           // only remove the priority listener if it has not been received by the Announcer, otherwise the Announcer
           // will let us know when it is finished with it and we will remove the listener then
-          removePriorityListener: !sentToAnnouncer
+          removePriorityListener: true
         } );
       }
     }

EDIT: THe problem mentioned here is actually a problem for the patch in #46 (comment) as well. Confirmed that this breaks with patch in that comment. This is an issue with master as well.

const testUtterance = new phet.utteranceQueue.Utterance( {
    alert: 'This is a test utterance.',
    alertStableDelay: 0,
    announcerOptions: { cancelSelf: false }
} );
phet.scenery.voicingUtteranceQueue.addToBack( testUtterance )
phet.joist.joistVoicingUtteranceQueue.addToBack( testUtterance )

[jessegreenberg]

@zepumph and I discussed this during a11y dev meeting today and decided that the issue identified in #46 (comment) is a separate issue and also low priority (see #48).

That freed up both solutions in mentioned above. The counting one seemed unnecessarily complicated so we went with #46 (comment). While pairing, we refined some of the checks in the announcementCompleteEmitter listener to make sure that the correct listener was getting removed.

I just committed the refined patch and verified that unit tests are passing. Next I would like to turn the case that identified this into a unit test. Then I think this issue can be closed.

const testUtterance = new phet.utteranceQueue.Utterance( { alert: 'This is a test utterance.', announcerOptions: { cancelSelf: false } } );

phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );

window.setTimeout( () => {
    phet.scenery.voicingUtteranceQueue.addToBack( testUtterance );
}, 500 );

[jessegreenberg]

Unit test added above, it is passing in Chrome and Firefox.

[jessegreenberg]

Options were removed from removeUtterance, that was the final TODO for this issue. Since the strategy and bulk of the patch was reviewed together I think this is ready to close.

[zepumph]

Id love to review the commits. I think there is one more assertion to add reopening.

[zepumph]

Please double check to make sure my changes are correct in your mind.

This looks really really excellent. I want to note an important point of this work, just to make sure I understand it correctly. Let me know if you feel like this needs more explanation in the code. . . . this.announcingUtteranceWrapper is storing a reference to the Utterance while it is being announced, but it is never used to cancel/interrupt the utterance. Instead, the code path is through the listener, which will generally compare the utterance to all items in the queue, and then forward to Announcer.onUtterancePriorityChange.

This is a good pattern because it provides rigitity in how we ensure proper management of the priority listener add/remove calls, but flexibility as to whether or not the announcer actually cares about/handles priority (i.e. a lack of AriaLiveAnnouncer.onUtterancePriorityChange).

Feel free to close.

[jessegreenberg]

I like that assertion a lot, and things are working well with it.

I want to note an important point of this work, just to make sure I understand it correctly

Yes, your description here is correct.

eel free to close.

Sounds good, thank you for reviewing this.