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

Identify reusable code for speech-synthesis feature. #22

Closed
pixelzoom opened this issue Oct 29, 2024 · 13 comments
Closed

Identify reusable code for speech-synthesis feature. #22

pixelzoom opened this issue Oct 29, 2024 · 13 comments

Comments

@pixelzoom
Copy link
Collaborator

pixelzoom commented Oct 29, 2024

number-pairs is currently not reusing anything from number-suite-common. And we probably need to keep it that way for now, due to PhET-iO support issues. But there is a lot of existing code related to the speech-synthesis feature that we should inspect, especially since the design is the same for number-pairs. Some code we might consider generalizing, adding PhET-iO support, and moving to common code. Other code we might be able to copy and improve.

Here's the existing code that's related to the speech synthesis feature. (This might not be a complete list.)

number-suite-common:

  • LanguageAndVoiceControl - 2 carousels that replace the standard UI in Preferences > Localization
  • SecondLanguageControl - ABSwitch and 1 carousel that appears in Preferences > Simulation
  • AutoHearControl - ABSwitch that appears in Preferences > Audio
  • NumberSuiteCommonSpeechSynthesisAnnouncer (extends utterance-queue.SpeechSynthesisAnnouncer)
  • NumberSuiteCommonUtteranceQueue (abstract, extends utterance-queue.UtteranceQueue)
  • NumberSuiteCommonPreferences
  • SpeechSynthesisButton
  • NoVoiceWarningButton
  • NoVoiceWarningDialog

number-play:

  • LocaleSwitch - ABSwitch between primary and secondary locale
  • numberPlaySpeechSynthesisAnnouncer (instance of NumberSuiteCommonSpeechSynthesisAnnouncer, singleton)
  • numberPlayUtteranceQueue (extends NumberSuiteCommonUtteranceQueue, singleton)
  • numberPlayPreferences (singleton)

number-compare:

  • numberCompareSpeechSynthesisAnnouncer (instance of NumberSuiteCommonSpeechSynthesisAnnouncer, singleton)
  • numberCompareUtteranceQueue (extends NumberSuiteCommonUtteranceQueue, singleton)
  • numberComparePreferences (singleton)
@pixelzoom pixelzoom self-assigned this Oct 29, 2024
@pixelzoom
Copy link
Collaborator Author

LanguageAndVoiceControl and SecondLanguageControl were components that I thought would be good candiates to move to joist. But looking at their current implementations, they are tightly coupled to UtteranceQueue in general, and the NumberSuiteCommonUtteranceQueue subclass of UtteranceQueue in particular. So the first order of business will be to see if they can be decoupled.

@pixelzoom pixelzoom mentioned this issue Nov 5, 2024
12 tasks
@pixelzoom
Copy link
Collaborator Author

@marlitas and I will meet about this feature on Tue 12/3 @ 10:30 MT.

@pixelzoom
Copy link
Collaborator Author

@pixelzoom
Copy link
Collaborator Author

pixelzoom commented Dec 3, 2024

@marlitas and I met, reviewed the state of code in number-suite-common, number-play, number-compare.

We discussed various options for implementing these features. Contrary to a previous decision, we decided that rather than copy code from number-suite-common, we will add it as a dependency and use the code directly.

Next steps:

@pixelzoom:

  • Add number-suite-common (and its dependency counting-common) as dependencies in package.json and tsconfig.json.
  • Create NumberPairsPreferences, ala GeomtricOpticsPreferences.
  • Create singleton numberPairsSpeechSynthesisAnnouncer subclass of NumberSuiteCommonSpeechSynthesisAnnouncer.
  • Create singleton numberPairsUtteranceQueue of NumberSuiteCommonUtteranceQueue.
  • Figure out what allURL is in NumberSuiteCommonPreferences.
  • Remove dependency on NumberSuiteCommonPreferences in SecondLanguageControl.
  • Reuse LanguageAndVoiceControl.
  • Reuse SecondLanguageControl.
  • Reuse AutoHearControl.

@marlitas

  • Switch over to NumberSuiteCommonStrings where possible.
  • Reuse/copy SpeechSynthesisButton, NoVoiceWarningButton, and NoVoiceWarningDialog, and wire them up.
  • Wire up the current "phrase" to the "Automatically Hear Phrase" control in Preferences > Audio.
  • Check on how speech synthesis is expected to work with screen reader. (It looks like speech is addToBack of UtteranceQueue, so should occur after button is read.)

After the above, we can evaluate how to improve the implementation, whether to contribute back to the suite, etc.

pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 3, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 3, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 3, 2024
pixelzoom added a commit that referenced this issue Dec 3, 2024
pixelzoom added a commit that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-compare that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-play that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 9, 2024
pixelzoom added a commit that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/babel that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/babel that referenced this issue Dec 9, 2024
pixelzoom added a commit that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 9, 2024
pixelzoom added a commit to phetsims/number-suite-common that referenced this issue Dec 9, 2024
@pixelzoom
Copy link
Collaborator Author

@marlitas and I reviewed my work on 12/10/24.

I'll look at adding PhET-iO instrumentation to NumberPairsPreferencesNode, AutoHearControl, SecondLanguageControl, and LanguageAndVoiceControl.

@pixelzoom
Copy link
Collaborator Author

In the above commits, I added basic PhET-iO instrumentation for the Preferences controls related to speech synthesis. The controls in number-suite-common default to tandem: Tandem.OPT_OUT, and number-pairs provides valid tandems.

marlitas added a commit to phetsims/number-suite-common that referenced this issue Dec 11, 2024
marlitas added a commit to phetsims/babel that referenced this issue Dec 11, 2024
marlitas added a commit to phetsims/number-suite-common that referenced this issue Dec 11, 2024
marlitas added a commit that referenced this issue Dec 12, 2024
marlitas added a commit to phetsims/number-play that referenced this issue Dec 12, 2024
marlitas added a commit to phetsims/number-play that referenced this issue Dec 12, 2024
marlitas added a commit to phetsims/number-compare that referenced this issue Dec 12, 2024
marlitas added a commit to phetsims/number-suite-common that referenced this issue Dec 12, 2024
marlitas added a commit to phetsims/number-suite-common that referenced this issue Dec 12, 2024
@marlitas
Copy link
Contributor

marlitas commented Dec 13, 2024

Implemented above. I'll send over to @pixelzoom for review, but other than that I think this is good to go.

I created and issue for Check on how speech synthesis is expected to work with screen reader.
#50

p.s. no rush on the review. I'm still parsing through the others.

@pixelzoom
Copy link
Collaborator Author

@marlitas reminder that in #22 (comment), we said:

After the above, we can evaluate how to improve the implementation, whether to contribute back to the suite, etc.

@marlitas
Copy link
Contributor

Mmmm. thanks for that reminder. Okay. Well then let's maybe both review and create a list of suggestions. I do think this issue is nearing the end of it's original intention though.

@pixelzoom
Copy link
Collaborator Author

... I do think this issue is nearing the end of it's original intention though.

Agreed. And I'm OK if we decide that "improve the implementation" and/or "contribute back to the suite" is out of scope. I feel like I made some good improvements (eliminating dependencies) in the commits above.

@pixelzoom
Copy link
Collaborator Author

After the above, we can evaluate how to improve the implementation, whether to contribute back to the suite, etc.

@marlitas and I discussed. I spent 3+ hours this morning on #53, and I don't feel like I have more time to spend on improving number-suite-common. So we're it as is, maybe make a few improvements as we run into problems. We're not going to do anything proactively.

Closing.

@marlitas
Copy link
Contributor

@pixelzoom and I discussed, and we believe this is now ready to close. We will address any improvements as bugs come up or as we interact with the code.

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