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

Utterance.canAnnounceProperty should interrupt the Announcer when it becomes false #77

Closed
jessegreenberg opened this issue Apr 12, 2022 · 5 comments
Assignees
Labels

Comments

@jessegreenberg
Copy link
Contributor

An extension and enhancement of phetsims/scenery/issues/1300, when the Utterance.canSpeakProperty becomes false it should interrupt the Announcer if it is currently being announced.

@jessegreenberg jessegreenberg added the dev:enhancement New feature or request label Apr 12, 2022
@jessegreenberg jessegreenberg self-assigned this Apr 12, 2022
@jessegreenberg jessegreenberg changed the title Utterance.canSpeakProperty should interrupt the Announcer when it becomes false Utterance.canAnnounceProperty should interrupt the Announcer when it becomes false Apr 13, 2022
jessegreenberg added a commit that referenced this issue Apr 13, 2022
@jessegreenberg
Copy link
Contributor Author

This was simple enough, the change is working nicely. Tested with Dialogs and ReadingBlocks which now interrupt when closed.

@jessegreenberg
Copy link
Contributor Author

This is to be reviewed as part of the set from phetsims/scenery#1300 by @zepumph, reopening. And thank you!

@zepumph
Copy link
Member

zepumph commented Jun 9, 2022

This looks really nice, thanks! Instead of creating a new closure each time we speak, can you attach a bound prototype method, or even a member variable that is set once, and then have a second value like this.canAnnouncePropertyLinked:boolean that you check instead of checking if the listener itself is null? I think that would be slightly cleaner. If you disagree that is fine too. Feel free to close.

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Jun 9, 2022
@jessegreenberg
Copy link
Contributor Author

Great idea thanks. I went with a bound method. While I was testing this I realized that after #82 we need to do this for both utterance.canAnnounceProperty and utterance.voicingCanAnnounceProperty so I did that as well in the above commit. I verified that it was still working and sims passed fuzzing.

@zepumph can you take a look at this again?

@zepumph
Copy link
Member

zepumph commented Jun 30, 2022

Looks great, thanks!

@zepumph zepumph closed this as completed Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants