phetsims / number-suite-common

"Number Suite Common" contains code for use by sims that are part of the Number Suite
GNU General Public License v3.0
0 stars 0 forks source link

Use `UtteranceQueue.announcer` to access `numberSuiteCommonAnnouncer` #58

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Since https://github.com/phetsims/utterance-queue/commit/7d6aa7440daf6ef38e9c66cee9257547e07d6930, we can provide a more specific Announcer subclass. This will let us get rid of the variable: numberSuiteCommonAnnouncer. Over to @chrisklus for the minor cleanup.

chrisklus commented 1 year ago

@marlitas and I took a go a this and right away needed to convert announcer to protected to use it in NumberSuiteCommonUtteranceQueue, which seemed fine, and I previously have asked @jessegreenberg if that is okay. Then, we discovered I needed to reference it publicly in LanguageAndVoiceControl and we weren't sure if that was okay or if I should revert to a subclass-specific reference for this case. @zepumph what do you think? Okay to make it public readonly?

Here is a working patch for all changes needed for this issue:

```diff Index: number-suite-common/js/common/view/NumberSuiteCommonUtteranceQueue.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-suite-common/js/common/view/NumberSuiteCommonUtteranceQueue.ts b/number-suite-common/js/common/view/NumberSuiteCommonUtteranceQueue.ts --- a/number-suite-common/js/common/view/NumberSuiteCommonUtteranceQueue.ts (revision 5bb1473da3ad1cda94c7adf758b705165567852d) +++ b/number-suite-common/js/common/view/NumberSuiteCommonUtteranceQueue.ts (date 1679509724121) @@ -25,7 +25,7 @@ // the sim is running in), we can't use this string Property like normal. NumberSuiteCommonStrings.oneTwoThreeStringProperty; -export default abstract class NumberSuiteCommonUtteranceQueue extends UtteranceQueue { +export default abstract class NumberSuiteCommonUtteranceQueue extends UtteranceQueue { // Data that can be spoken to the user. The data comes from the screen that is currently being interacted with. private speechDataProperty: TReadOnlyProperty | null; @@ -33,9 +33,6 @@ // Whether this class has been initialized. private initialized: boolean; - // The SpeechSynthesisAnnouncer used for this UtteranceQueue. - public readonly numberSuiteCommonAnnouncer: NumberSuiteCommonSpeechSynthesisAnnouncer; - // See doc in NumberSuiteCommonPreferences. private readonly readAloudProperty: TReadOnlyProperty; @@ -56,8 +53,6 @@ this.speechDataProperty = null; this.initialized = false; - - this.numberSuiteCommonAnnouncer = numberSuiteCommonAnnouncer; this.readAloudProperty = readAloudProperty; this.speechDataUtterance = new Utterance( { @@ -77,7 +72,7 @@ assert && assert( this.initialized && this.speechDataProperty, 'Cannot speak before initialization' ); const speechData = this.speechDataProperty!.value; - speechData && this.numberSuiteCommonAnnouncer.voiceProperty.value && this.speak( speechData, this.speechDataUtterance ); + speechData && this.announcer.voiceProperty.value && this.speak( speechData, this.speechDataUtterance ); } /** @@ -85,8 +80,8 @@ * string, and then resets the voice to what it was before starting the test read. */ public testVoiceBySpeaking( voiceToTest: SpeechSynthesisVoice, locale: Locale ): void { - const currentVoice = this.numberSuiteCommonAnnouncer.voiceProperty.value; - this.numberSuiteCommonAnnouncer.voiceProperty.value = voiceToTest; + const currentVoice = this.announcer.voiceProperty.value; + this.announcer.voiceProperty.value = voiceToTest; // Indicate when we are speaking with the test voice so we know if we should set the voice back to the non-testing // voice or not. @@ -96,21 +91,21 @@ const resetVoiceListener = () => { if ( !this.isTestVoiceSpeaking ) { - this.numberSuiteCommonAnnouncer.voiceProperty.value = currentVoice; + this.announcer.voiceProperty.value = currentVoice; } - this.numberSuiteCommonAnnouncer.announcementCompleteEmitter.removeListener( resetVoiceListener ); + this.announcer.announcementCompleteEmitter.removeListener( resetVoiceListener ); }; // When the test speech is complete, set the voice back to what it was before testing, unless we have already // started speaking for a different test. - this.numberSuiteCommonAnnouncer.announcementCompleteEmitter.addListener( resetVoiceListener ); + this.announcer.announcementCompleteEmitter.addListener( resetVoiceListener ); } /** * Speaks the provided string. */ private speak( string: string, utterance: Utterance ): void { - const voice = this.numberSuiteCommonAnnouncer.voiceProperty.value; + const voice = this.announcer.voiceProperty.value; assert && assert( voice, 'No voice set for voiceProperty: ', voice ); utterance.alert = string; @@ -145,8 +140,7 @@ // Speak the speechData if readAloud is turned on or the speechData changes. Also check that the announcer has a // voice because even if the voiceProperty is set to null, the browser still speaks with a default voice. Multilink.lazyMultilink( - [ this.readAloudProperty, this.speechDataProperty ], - ( readAloud ) => readAloud && this.speakSpeechData() + [ this.readAloudProperty, this.speechDataProperty ], readAloud => readAloud && this.speakSpeechData() ); this.initialized = true; Index: utterance-queue/js/UtteranceQueue.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/utterance-queue/js/UtteranceQueue.ts b/utterance-queue/js/UtteranceQueue.ts --- a/utterance-queue/js/UtteranceQueue.ts (revision 9427ace6db1442425e1b65853747f88bce203338) +++ b/utterance-queue/js/UtteranceQueue.ts (date 1679509998163) @@ -48,7 +48,7 @@ // Sends browser requests to announce either through aria-live with a screen reader or // SpeechSynthesis with Web Speech API (respectively), or any method that implements this interface. Use with caution, // and only with the understanding that you know what Announcer this UtteranceQueue instance uses. - private readonly announcer: MyAnnouncer; + public readonly announcer: MyAnnouncer; // Initialization is like utteranceQueue's constructor. No-ops all around if not // initialized (cheers). See constructor() Index: number-suite-common/js/common/view/LanguageAndVoiceControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-suite-common/js/common/view/LanguageAndVoiceControl.ts b/number-suite-common/js/common/view/LanguageAndVoiceControl.ts --- a/number-suite-common/js/common/view/LanguageAndVoiceControl.ts (revision 5bb1473da3ad1cda94c7adf758b705165567852d) +++ b/number-suite-common/js/common/view/LanguageAndVoiceControl.ts (date 1679509998170) @@ -113,12 +113,12 @@ // Rebuild the voiceCarousel with the available voices when the locale changes or when voices become available Multilink.multilink( - [ localeProperty, utteranceQueue.numberSuiteCommonAnnouncer.voicesProperty ], + [ localeProperty, utteranceQueue.announcer.voicesProperty ], ( locale, voices ) => { if ( voices.length ) { - utteranceQueue.numberSuiteCommonAnnouncer.setFirstAvailableVoiceForLocale( locale, voiceProperty ); + utteranceQueue.announcer.setFirstAvailableVoiceForLocale( locale, voiceProperty ); - const availableVoicesForLocale = utteranceQueue.numberSuiteCommonAnnouncer.getPrioritizedVoicesForLocale( locale ); + const availableVoicesForLocale = utteranceQueue.announcer.getPrioritizedVoicesForLocale( locale ); if ( availableVoicesForLocale.length ) { const voiceCarouselItems: VoiceCarouselItem[] = availableVoicesForLocale.map( ```
chrisklus commented 1 year ago

@zepumph @marlitas and I discussed on Zoom and @zepumph gave the okay for public readonly. Committed above, closing!