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

Dialog state behavior #324

Closed
Tracked by #899
KatieWoe opened this issue Jan 24, 2023 · 8 comments
Closed
Tracked by #899

Dialog state behavior #324

KatieWoe opened this issue Jan 24, 2023 · 8 comments

Comments

@KatieWoe
Copy link
Contributor

Device
Dell
OS
Win 11
Browser
Chrome
Problem Description
For phetsims/qa#886.
From Slack:

Is the preference dialog or the keyboard nav dialog being open meant to be stateful? I thought it wasn't, but it seems to be in friction.

In addition to this, when the state is set, the open dialog sound plays, which seems wrong.

Steps to Reproduce

  1. Go to the state wrapper
  2. Have the state set rate set to every second or so (default works)
  3. Open the preferences dialog
  4. Observe the downstream sim and note the sound

Visuals

IMG_4588.MOV
@zepumph
Copy link
Member

zepumph commented Jan 25, 2023

I'll take a look before tossing this one to our resident sound expert.

@zepumph
Copy link
Member

zepumph commented Jan 30, 2023

@matthew-blackman and I were able to figure out the problem, but we aren't exactly sure what the best solution is. Basically the sound engine is doing a good job at silencing sound while the state engine is setting state, but in this case, the dialog is being cleared in the "before state set" hook, and that deletion closes dialog, and starts the close sound.

I would like to implicate @samreid to the next part of this conversation, and will likely discuss in about 5 minutes with him and @matthew-blackman.

@zepumph
Copy link
Member

zepumph commented Jan 30, 2023

Fixed by commits over in https://github.com/phetsims/phet-io/issues/1906. Thanks @samreid and @matthew-blackman for helping to get this solved. Commits over in that repo for cherry pick.

@zepumph
Copy link
Member

zepumph commented Jan 30, 2023

Wait. I also still hear another problem with the state engine where I hear the very first open sound. I'll keep investigating.

UPDATE: I heard it while testing an older version of the changes, I cannot reproduce with the new fixes in place. Nevermind!!

@jessegreenberg
Copy link
Contributor

OK, 4 commits were cherry-picked from https://github.com/phetsims/phet-io/issues/1906.

image

I don't know how to test this one, but opened a dialog in studio in the 1.6 SHAs and pressed "test" and nothing was obviously broken.

Ready to verify.

@Nancy-Salpepi
Copy link

I no longer hear sounds in the downstream sim.
However, @jessegreenberg I don't think Katie's original question was ever answered. Is having a dialog open supposed to be stateful? I just compared the behavior in Friction to Beer's Law 1.7.0-rc.2. and it is the same. If that is the intended behavior, then this issue can be closed.

@jessegreenberg
Copy link
Contributor

I tried to find the answer in https://phet-dev.colorado.edu/html/friction/1.6.0-rc.1/phet-io/doc/guides/phet-io-guide.html#preferences-customization but couldn't find anything specific about the dialog state (just individual preferences). So I believe Dialog's isShowingProperty is meant to be stateful and this is correct.

@arouinfar can you please confirm?

@arouinfar
Copy link
Contributor

That's correct @jessegreenberg. Every dialog's isShowingProperty is stateful, regardless of what type of dialog it is. For Preferences, specifically, selectedTabProperty is also stateful. I reviewed the Friction RC and it is behaving as intended, closing.

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

6 participants