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

initializeAsSkelleton #9

Closed
jessegreenberg opened this issue Nov 21, 2019 · 7 comments
Closed

initializeAsSkelleton #9

jessegreenberg opened this issue Nov 21, 2019 · 7 comments

Comments

@jessegreenberg
Copy link
Contributor

Noticed while reviewing #6 - There is an optional arg to UtteranceQueue called implementAsSkeleton, which creates the UtteranceQueue but with all alerts disabled. Currently default for this is true. Since this repo might stand on its own, I am wondering if the default should be false?

Then a client can just do const utteranceQueue = new phet.utteranceQueue.UtteranceQueue();

@zepumph @twant what do you think?

@jessegreenberg
Copy link
Contributor Author

Alternatively, maybe an initialize() function that is always required may be more easy to understand if we still want the default behavior to be disabled.

@jessegreenberg
Copy link
Contributor Author

Wait.. sorry - the default is false. I haven't been able to get alerts in #6 yet and I thought this was the reason but it is not. Reassigning to myself.

@zepumph
Copy link
Member

zepumph commented Nov 21, 2019

You bring up a good point though. I think that this is still a good issue. I also don't like this option. Perhaps at the very least we can turn that into an options object so that the skeleton function isn't so "front and center."

Here is an alternative. What if we create it always, but then in display just disable it if interactive descriptions isn't enabled?

@jessegreenberg
Copy link
Contributor Author

The reason it wasn't working is that I forgot to step axon.timer.

What if we create it always, but then in display just disable it if interactive descriptions isn't enabled?

That sounds really nice.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Nov 21, 2019

I noticed two things when I tried to do this

  • implementAsSkelleton also controls phetio options and tandem, if always enabled will these always be passed through? Is that OK?
  • If PhET uses disabled to enable/disable at the Display level, disabled probably shouldn't also be controlled at the sim level. It may be beneficial to have both enabled state (which can within sim code) and initialized which is to be set globally at the Display level.

zepumph added a commit that referenced this issue Jan 28, 2020
zepumph added a commit that referenced this issue Jan 28, 2020
@zepumph
Copy link
Member

zepumph commented Jan 28, 2020

implementAsSkelleton also controls phetio options and tandem, if always enabled will these always be passed through? Is that OK?

After looking at it with semi fresh eyes, I can't see an obvious way to improve it, and it doesn't feel terribly confusing to me currently.

If PhET uses disabled to enable/disable at the Display level, disabled probably shouldn't also be controlled at the sim level. It may be beneficial to have both enabled state (which can within sim code) and initialized which is to be set globally at the Display level.

I don't understand this sorry. I don't see any enabled/disable/initialize in Display. So far there has not been a need to enabled/disable the Display beyond when constructing it (options.interactive), so I don't see a reason why we would want to add anything, even for utteranceQueue.

@jessegreenberg
Copy link
Contributor Author

implementAsSkelleton also controls phetio options and tandem, if always enabled will these always be passed through? Is that OK?

After looking at it with semi fresh eyes, I can't see an obvious way to improve it, and it doesn't feel terribly confusing to me currently.

OK sounds good - I mostly wanted to see if it was an issue for PhET-iO.

If PhET uses disabled to enable/disable at the Display level,...

I don't understand this sorry....

I no longer understand this either. initialized and enabled are implemented as I would expect in utteranceQueue and Display.

Im going to go ahead and close this issue since after review we are OK with the original point of the ticket, 'initializeAsSkelleton'.

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