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

Get rid of VoicingManager.mainWindowVoicingEnabledProperty #1343

Open
zepumph opened this issue Jan 28, 2022 · 7 comments
Open

Get rid of VoicingManager.mainWindowVoicingEnabledProperty #1343

zepumph opened this issue Jan 28, 2022 · 7 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jan 28, 2022

While trying to consolidate voicing-control Properties into a more solid structure over in phetsims/joist#743, @jessegreenberg and I realized that we can get rid of the purposes of the mainWindow Property and instead of voicingVisible. This will need to wait until that feature is complete in #1300.

Once that is ready, we can toggle voicingVisible when turning sim voicing on/off in joist. On hold until that is complete.

@jessegreenberg
Copy link
Contributor

I hope that work in #1300 will make mainWindowVoicingEnabledProperty unnecessary, lets come back to this issue once that issue is done and make sure there is nothing else to do here.

@jessegreenberg
Copy link
Contributor

#1300 is ready, no longer on hold.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Apr 13, 2022

There is a TODO in FocusManager that I don't fully understand

    // TODO: perhaps remove once reading blocks are set up to listen instead to Node.canSpeakProperty (voicingVisible), https://github.com/phetsims/scenery/issues/1343
    voicingManager.voicingFullyEnabledProperty.link( enabled => {
      this.readingBlockHighlightsVisibleProperty.value = enabled;
    } );

EDIT: This can be moved out of FocusManager and into joist, but I don't think it can be removed entirely. We don't want to display reading block highlights at all when "Sim Voicing" is not enabled.

@jessegreenberg
Copy link
Contributor

@zepumph and I reviewed this together and we are excited to remove mainWindowVoicingEnabledProperty, and also move Properties form SpeechSynthesisAnnouncer to joist like voicingFullyEnabledProperty and voicingFullyEnabledProperty.
Probably rename voicingFullyEnabledProperty to contentAndToolbarVoicingEnabledProperty so it is more clear.

The only Property that seemed challenging to move out of scenery was voicingManager.speechAllowedAndFullyEnabledProperty in its usage in ReadingBlock.ts. But we decided we can replace it with Voicing.canAnnounceProperty, because that Property indicates whether the ReadingBlock can announce and should receive input via voicingVisibleProperty. Hooray!

@jonathanolson
Copy link
Contributor

jonathanolson commented Jul 29, 2022

Patched a memory leak above (related to this issue?), can you verify? It was leaking on every new Display.

@zepumph
Copy link
Member Author

zepumph commented Jul 29, 2022

Looks great. Thanks! Did this cause trouble somewhere? I'm curious if there is a use case where we are creating that many Displays?

@jonathanolson
Copy link
Contributor

Yes, a memory leak in Density (there are a number of 3d sims that create Displays often, since they need to contain, size and rasterize content and update the content's rasterization when needed).

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

3 participants