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

readAloud design questions #52

Closed chrisklus closed 1 year ago

chrisklus commented 1 year ago

From changes in https://github.com/phetsims/number-suite-common/issues/51, the readAloud ("Hear Number"/"Hear Number Sentence") feature has basically been entirely re-implemented. This has brought up some behavior questions that I don't think we've talked about.

  1. The sim doesn't immediately read the state when turning on readAloud in the preferences menu. This has always been the behavior, and I think it should stay because the menu covers up the screen you're on. But just noting that to hear the starting state, you do have to change the number and then go back.
  2. When readAloud is on and you navigate to a screen that has voicing capabilities, the current state is read out. This is new and I think is fine because the state is possibly different than where you're coming from, and it seems nice to speak what you're starting at on that screen.
  3. When readAloud is on, the sim reads whenever we are using the primary language and we change that language or voice (same thing with second language), and when switching between the primary voice and secondary voice. I think this is nice. One symptom of this I am noticing is that if you're using the second language and then you turn off second language in preferences, the sim automatically switches to the primary language, which cause a read out. Not sure if this is desirable.
  4. Should turning on the second language toggle automatically toggle the sim to use the second language, or should we let the user leave the preferences menu to make the switch like the current behavior? It's maybe a little weird that if readAloud is on, turning on the secondLanguage in the preference doesn't speak when selecting a second language for the first time.

Also tagging @pixelzoom if you want to weigh in on any of these.

amanda-phet commented 1 year ago
  1. The sim doesn't immediately read the state when turning on readAloud in the preferences menu. This has always been the behavior, and I think it should stay because the menu covers up the screen you're on. But just noting that to hear the starting state, you do have to change the number and then go back.

I think it could be nice to have the sim actually read the state of the sim when you toggle this, but I'm also ok leaving it as-is. Reading through all of these questions is what makes me think it would be nice to have that immediate feedback, since there are a lot of actions that could prompt the read aloud feedback.

  1. When readAloud is on and you navigate to a screen that has voicing capabilities, the current state is read out. This is new and I think is fine because the state is possibly different than where you're coming from, and it seems nice to speak what you're starting at on that screen.

This seems fine, and again is what makes me think (1) above could be changed.

  1. When readAloud is on, the sim reads whenever we are using the primary language and we change that language or voice (same thing with second language), and when switching between the primary voice and secondary voice. I think this is nice. One symptom of this I am noticing is that if you're using the second language and then you turn off second language in preferences, the sim automatically switches to the primary language, which cause a read out. Not sure if this is desirable.

Seems ok to me if we change (1) above.

  1. Should turning on the second language toggle automatically toggle the sim to use the second language, or should we let the user leave the preferences menu to make the switch like the current behavior? It's maybe a little weird that if readAloud is on, turning on the secondLanguage in the preference doesn't speak when selecting a second language for the first time.

I think this toggle should just control the visibility of the dual-language toggle on the sim screen. But happy to discuss this further.

pixelzoom commented 1 year ago

My 2 cents... The general goal here is to always provide auditory feedback when the student changes locale, voice, or the thing that is being read (total or sentence).

Read Aloud feature:

Primary & Secondary Launguage feature:

chrisklus commented 1 year ago

From design meeting with @pixelzoom and @amanda-phet:

Discussing https://github.com/phetsims/number-suite-common/issues/52#issuecomment-1434966437:

If you turn on the "Hear..." preference and are on a screen that has something readable (total or sentence), it should be read in the selected language + voice.

👍

When a language or voice is selected for Second Language, it should always read something (like "one") in that language and voice. Don't try to make it conditional based on which screen is selected, or specific to what would be read for the current screen. Always "say" sometime, regardless of screen.

We like this, and also mentioned that it would speak regardless of if "Hear... " is turned on. Start with "one two three" and see how we like it. @pixelzoom also mentioned that using three numbers here could avoid confusion because the demo voice content won't overlap with what can be read out from a screen

When a language or voice is selected on the Localization tab, it should always read something in that language and voice. Again, don't try to make it conditional based on which screen is selected, or specific to what would be read for the current screen. Always "say" sometime, regardless of screen.

We will do this following the same pattern as the Second Language behavior.

Turning on "Second Language" preference, or changing which language or voice is selected, does not change the toggle on the screen. Neither does changing the primary language or voice in the Localization tab.

Sounds good.

chrisklus commented 1 year ago

This issue is ready for review by @pixelzoom and @amanda-phet. I believe I handled all the cases described so far in this issue. I also added one that wasn't mentioned: When "Hear..." is turned on and the comparison signs are turned off, if you turn comparison signs back on, the sim should read out the current state. EDIT: I discovered this was discussed and implemented back in https://github.com/phetsims/number-suite-common/issues/46#issuecomment-1421204189, so my changes here maintained that behavior.

@pixelzoom doesn't have the hours for this issue until Wednesday or Thursday so it would be great if @amanda-phet could take a look before then to review the design behavior in case I should make any changes before code review.

For code review, I think reviewing the above commits is sufficient. Main affected files are:

amanda-phet commented 1 year ago

Reviewed and for the most part, the behavior is working well.

Discovered one bug when changing locales (in localization tab) when Hear Total was 'on.' The sim reads out the total/sentence followed by "one two three." @chrisklus is on it.

pixelzoom commented 1 year ago

This is looking really great. The code is exceptional -- clean, well-documented, well-organized.

In addition to the bug the @amanda-phet noted in https://github.com/phetsims/number-suite-common/issues/52#issuecomment-1448885718, the only thing I noticed is that the list of Languages is not presented in alphabetical order. (Note: ..., Bengali, Tibetan, Bosnian,...)

screenshot_2325

But I now see that is also an issue with joist.LocalePanel. It's probably ordered by locale code, rather than the displayed locale name. I've reported this in https://github.com/phetsims/joist/issues/911.

screenshot_2326

To fix the order for Number Play/Compare, you'll need to make a change in LanguageAndVoiceControl.ts. Here's the relevant line:

    const languageCarouselItems: LanguageCarouselItem[] = localeProperty.validValues!.map(

Before doing the map, sort localeProperty.validValues. I'm not sure whether they should be ordered by Locale.name or Locale.localizedName -- consult with @jessegreenberg.

chrisklus commented 1 year ago

Thanks @pixelzoom!

I worked on the bug @amanda-phet found more with @zepumph yesterday. We ended up needing to cancel the utterance queue instead of disabling the announcer. This added what feels like more of a workaround than the original intended solution. @zepumph also expressed not loving the 2x announcer + utteranceQueue pattern, as well as the singleton pattern for both of those. I may consider refactoring but everything is working nicely as is. Those changes are committed above.

Thanks for the note about the alphabetization, I was noticing that was more annoying in the Carousel. I'll ask @jessegreenberg about which name would be best to use.

Leaving self-assigned to me.

chrisklus commented 1 year ago

With the sorting of languages over in https://github.com/phetsims/joist/issues/911, I think this issue is ready to close.