announceImmediately adds low priority Utterances to the queue

[jessegreenberg]

Failing test is

    firstUtterance.priorityProperty.value = 2;
    thirdUtterance.priorityProperty.value = 1;

    testVoicingUtteranceQueue.addToBack( firstUtterance );
    await timeout( timeForFirstUtterance / 2 );
    testVoicingUtteranceQueue.announceImmediately( thirdUtterance );

    assert.ok( !testVoicingUtteranceQueue.hasUtterance( thirdUtterance ), 'lower priority thirdUtterance should be dropped from the queue' );

And there is a line of documentation that says

A provided Utterance with a priority equal to or lower than what is being announced will not interrupt and will never be announced. If an Utterance at the front of the queue has a higher priority than the provided Utterance, the provided Utterance will never be announced.

That is not what the implementation does, the implementation of announceImmediately does not attempt to remove thirdUtterance. I am assumign this is a bug and that we didn't forget to update the documentation.

[jessegreenberg]

IT seems like this should have been an issue since we first did https://github.com/phetsims/utterance-queue/commit/2291f6831431562cae28cd734df0981c7e627a9b, but that issue indicates that unit tests for this were passing.

[jessegreenberg]

announceImmediately uses prioritizeUtterances.

prioritizeUtterances at the end uses this to determine whetner the new Utterances should cancel what is being announced.

    // Let the Announcer know that priority has changed so that it can do work such as cancel the currently speaking
    // utterance if it has become low priority
    if ( this.queue.length > 0 ) {
      this.announcer.onUtterancePriorityChange( this.queue[ 0 ].utterance );
    }

There is nothing further in announceImmediately to remove the Utterance passed to the function if it has lower priority than what is being announced.

[jessegreenberg]

OK I changed the behavior to match the documentation for announceImmediately. I also had to change a unit test. We had some inconsistency because 1) The documentation or announceImmediately described that a new lower priority Utterance would not interrupt the announcing utterance and never be added to the queue. 2) We had one test that verified that the lower priority utterance would never interrupt the Announcer and never be announced. 3) We also had a test that verified that when a lower priority Utterance was added with announceImmediately it remained in the queue without interrupting the announcing utterance.

When I changed the behavior to fix the test in 2) it broke the test 3). So I changed test 3) to match the documentation and test 2). I believe this was the intent. I don't know how all the tests were passing before. They are all passing now.

@zepumph would you like to review this? No worries if short on time, feel free to assign back to me.

[zepumph]

Should we instead use shouldUtteranceCancelOther, it seems like potentially a bit of duplication otherwise. Is there a reason it shouldn't use the announcer-specific implementation just always check priorityProperty?

[jessegreenberg]

That is a great improvement, thanks. Anything else?