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

Replace VoicingManager.speakIgnoringEnabled with a Property controlling speech #69

Open
jessegreenberg opened this issue Oct 22, 2021 · 3 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

It is confusing that the voicingManager.enabledProperty can have a value of false but there can still be speech with speakIgnoringEnabled. A better way would be to provide a separate Property to control speech output while making it so that whenever voicingManager.enabledProperty is false there can be no speech. Here is some scratch illustrating the change we are thinking of.

Index: joist/js/audioManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/audioManager.js b/joist/js/audioManager.js
--- a/joist/js/audioManager.js	(revision ddcffa71993bd7e06725b30f5a394124e7cff693)
+++ b/joist/js/audioManager.js	(date 1634927385639)
@@ -135,6 +135,7 @@
           sim.isConstructionCompleteProperty,
           sim.browserTabVisibleProperty,
           sim.activeProperty,
+          this.simVoicingEnabledProperty, // passed to the PreferenceDialog, field on AudioManager
           sim.isSettingPhetioStateProperty,
           this.audioEnabledProperty
         ], ( simConstructionComplete, simVisible, simActive, simSettingPhetioState, audioEnabled ) => {
Index: scenery/js/accessibility/voicing/voicingManager.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/voicing/voicingManager.js b/scenery/js/accessibility/voicing/voicingManager.js
--- a/scenery/js/accessibility/voicing/voicingManager.js	(revision 414d8cf0bf3cdabc1a9c8e6eb6b7e091ae8369cf)
+++ b/scenery/js/accessibility/voicing/voicingManager.js	(date 1634927277241)
@@ -382,8 +382,8 @@
    *
    * @param {Utterance} utterance
    */
-  speakIgnoringEnabled( utterance ) {
-    if ( this.initialized ) {
+  speakIgnoringSpeechAllowed( utterance ) {
+    if ( this.initialized && this.enabledProperty.value ) {
       this.requestSpeech( utterance );
     }
   }

Likely related to phetsims/joist#743 because that will determine where simVoicingEnabledProperty should live. COuld be on the AudioManager, but maybe it should be on a "PreferencesModel", whatever that ends up looking like.

@jessegreenberg jessegreenberg self-assigned this Oct 22, 2021
@jessegreenberg
Copy link
Contributor Author

This was decided while working on phetsims/gravity-force-lab-basics#303.

@jessegreenberg
Copy link
Contributor Author

This would happen in utterance-queue now that SpeechSynthesisAnnouncer lives there.

@jessegreenberg jessegreenberg transferred this issue from phetsims/scenery Mar 17, 2022
@zepumph
Copy link
Member

zepumph commented Apr 1, 2022

Could we instead handle this by speaking this before turning it off? That would be in joist. I wonder if that would be nicer than another Property.

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

No branches or pull requests

2 participants