initializeAsSkelleton - Githubissues

jessegreenberg commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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'.

phetsims / utterance-queue

initializeAsSkelleton #9