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

Delete joistVoicingUtteranceQueue and replace with voicingVisibleProperty #800

Open
jessegreenberg opened this issue Apr 11, 2022 · 6 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

Now that voicingVisibleProperty is ready for use I think it can be used instead of the joistVoicingUtteranceQueue. The joistVoicingUtteranceQueue is problematic because priorityProperty doesn't work well with multiple UtteranceQueues (see #752 (comment)).

@jessegreenberg
Copy link
Contributor Author

At this time the only usage of joistVoicingUtteranceQueue is in VoicingToolbarItem, other than controlling its enabled state in audioManager and a phet-io specific thing in Sim.js.

@jessegreenberg
Copy link
Contributor Author

joistVoicingVisible has been removed. I added a function to Sim.js to set components other than the toolbar to voicingVisible: false so that they are not heard. It is called when voicingFullyEnabledProperty changes (which is what previously controlled the enabled state of joistVoicingUtteranceQueue.

It isn't working quite yet but I expect that it will. We need to do phetsims/scenery#1403 to verify that voicingVisibleProperty is correctly preventing speech when voicingFullyEnabledProperty if false.

@jessegreenberg
Copy link
Contributor Author

voicingVisibleProperty is working as a replacement of the joistVoicingUtteranceQueue. The feature of the "Sim Voicing" toggle switch in the toolbar isn't working everywhere yet because we still have decisions to make in phetsims/scenery#1403. Nothing further to be done in this issue.

@jessegreenberg
Copy link
Contributor Author

This should be reviewed as part of phetsims/scenery#1300, assigning to @zepumph as part of that. And thanks!

@jessegreenberg
Copy link
Contributor Author

One improvement is to rename setSimVoicingVisible to setSimContentVoicingVisible.

We need to update voicingVisible somehow in setAccessibleViewsVisible. We also need explicit control over how voicingVisible is enabled for the Toolbar and topLayer (dialogs) we thought it would be good to have two functions for each of those so that setting is clear.

Also, the call to setSimVoicingVisible is in a listener added in Sim.js if the Toolbar is provided. It would be better in Toolbar. Can add this listener when sim.isConstructionCompleteProperty fires so that ScreenViews are initialized.

Back to me for these things.

@zepumph
Copy link
Member

zepumph commented Jun 9, 2022

Ok sounds good. Let me know if I can assist at all.

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