phetsims / utterance-queue

Alerting library powered by aria-live
MIT License
0 stars 2 forks source link

Only one instance of SpeechSynthesisAnnouncer and UtteranceQueue allowed #108

Closed chrisklus closed 1 year ago

chrisklus commented 1 year ago

Over in https://github.com/phetsims/number-suite-common/issues/47, a solution we came up with for a speech synthesis problem was to have two instances of an announcer and an utterance queue. @jessegreenberg and I made a few small changes to get two instances of these working in an unbuilt sim. Then, at some point later, @zepumph recommended to try to not have two instances of these. For sake of time, I proceeded with the original design of having two, but then in https://github.com/phetsims/qa/issues/917, we immediately discovered that the two instances were not working in a built version.

Because there were some unknowns related to what might happen with multiple instances of these types, and because @zepumph noted that using one instance of each felt like a more appropriate use, as soon as this issue in the dev test came up, it seems best to me and @jessegreenberg to punt on investigating the problem and reverting back to using one instance, and noting in this repo that we don't support using more than one instance per sim. As soon as I make the refactor in number-suite-common, I'll notify @jessegreenberg so he can create a flag in SpeechSynthesisAnnouncer that only allows it to be instantiated once.

chrisklus commented 1 year ago

I still have one thing to look into before passing this issue back to @jessegreenberg

chrisklus commented 1 year ago

Okay so actually it looks like multiple instances of announcers and utterance queues CAN work properly in a built version. I'm very sorry I didn't look into it more, I was just assuming that because the second-instance speech synthesis was not working it must have been the problem. Turns out, the string that it was trying to read was being stripped out on build, see https://github.com/phetsims/number-compare/issues/20. So speech synthesis had nothing to read out.

I downloaded the debug version of dev.4 (the dev version that had the missing string AND the double instance announcer) and edited it to give it a dummy string. This allowed be to confirm that speech synthesis was actually working correctly.

So, I'm not sure where that leaves this issue. I'm still glad to have refactored Number Play and Compare to use one announcer over in https://github.com/phetsims/number-suite-common/issues/55, I think those were nice changes. @jessegreenberg if you feel okay about it, perhaps we can close this issue without adding a guard in SpeechSynthesisAnnouncer. Or, if we don't want to deal with any potential problems that could come up from this in the future, we could still add it since we currently don't need multiple instances in any sims.

jessegreenberg commented 1 year ago

OK, awesome, thanks for the update @chrisklus. I think we should still lock this down to one instance. It was assumed to be a singleton when we created the type and even though it worked in a build I am afraid of problems that may waiting for us.

Like #104 for instance.

jessegreenberg commented 1 year ago

OK, done. I realized the check needs to be in the initialize call instead of the constructor because the singleton voicingManager is constructed on first import. unit tests are still passing and sims that support Voicing are fuzzing.

@chrisklus can you please review this and confirm this changes works for number-suite-common sims?

chrisklus commented 1 year ago

Thanks @jessegreenberg! Sorry for the delay here. Changes look great and this is working well for number-suite sims.

The only thing I noticed is that when @zepumph and I tried to turn on voicing to test something unrelated/random in Number Play, we hit this assertion - so worth noting that we then cannot have a custom implementation like Number Play AND have standard voicing on the same sim. I wouldn't expect that to be an issue in the near future but thought it was worth documenting. I think we are good to close here!