Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use UtteranceQueue.announcer to access numberSuiteCommonAnnouncer #58

Closed
zepumph opened this issue Mar 21, 2023 · 2 comments
Closed

Use UtteranceQueue.announcer to access numberSuiteCommonAnnouncer #58

zepumph opened this issue Mar 21, 2023 · 2 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Mar 21, 2023

Since phetsims/utterance-queue@7d6aa74, 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
Copy link
Contributor

@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:

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<NumberSuiteCommonSpeechSynthesisAnnouncer> {
 
   // Data that can be spoken to the user. The data comes from the screen that is currently being interacted with.
   private speechDataProperty: TReadOnlyProperty<string | null> | 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<boolean>;
 
@@ -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 chrisklus assigned zepumph and unassigned chrisklus Mar 22, 2023
chrisklus added a commit that referenced this issue Mar 22, 2023
chrisklus added a commit to phetsims/utterance-queue that referenced this issue Mar 22, 2023
@chrisklus
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants