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

Some preferences maintained when opening standard wrapper #520

Closed
Nancy-Salpepi opened this issue Sep 14, 2022 · 5 comments
Closed

Some preferences maintained when opening standard wrapper #520

Nancy-Salpepi opened this issue Sep 14, 2022 · 5 comments
Assignees
Labels

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air (m1 chip)

Operating System
MacOS 12.5.1

Browser
Safari 15.6.1

Problem description
For phetsims/qa#831 and related to phetsims/joist#744
STUDIO: In the audio tab of the preferences menu, the following states carry over to the standard wrapper:
- Audio features off
- Sounds off
- Extra sounds on

Should any of the preferences carry over to the standard wrapper?

From slack:
Nancy Salpepi
2:51 PM
Hi Michael!
Quick question about the preferences menu and studio….what settings should be maintained when I open the standard wrapper? For example, extra sounds will stay checked, but interactive highlights won’t.

Michael Kauzmann
🏡 4:11 PM
No, we specifically decided against it. Please make an issue with the confusion/worry and assign to Amy to note, confirm and to close. Also please tag phetsims/joist#744

@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Sep 14, 2022
@zepumph
Copy link
Member

zepumph commented Sep 14, 2022

We spoke about this during PhET-iO meeting today, and confirmed that "preferences" in general should not be stateful (persisting through to the standard wrapper). General audio (audioManager.enabledProperty) should be stateful. We will make sound and extraSound phetioState:false.

@zepumph zepumph assigned zepumph and unassigned arouinfar Sep 16, 2022
@zepumph
Copy link
Member

zepumph commented Sep 16, 2022

Just to confirm steps here, please apply this patch and regenerate API files,

Index: js/soundManager.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/soundManager.ts b/js/soundManager.ts
--- a/js/soundManager.ts	(revision ffd8068d7888fdd28420a78fd944253f65b266de)
+++ b/js/soundManager.ts	(date 1663344496555)
@@ -114,11 +114,13 @@
 
     this.enabledProperty = new BooleanProperty( phet.chipper.queryParameters.supportsSound, {
       tandem: tandem.createTandem( 'enabledProperty' ),
+      phetioState: false, // This is a preference, global sound control is handled by the audioManager
       phetioDocumentation: 'Determines whether sound is enabled.'
     } );
 
     this.extraSoundEnabledProperty = new BooleanProperty( phet.chipper.queryParameters.extraSoundInitiallyEnabled, {
       tandem: tandem.createTandem( 'extraSoundEnabledProperty' ),
+      phetioState: false, // This is a preference, global sound control is handled by the audioManager
       phetioDocumentation: 'Determines whether extra sound is enabled. Extra sound is additional sounds that ' +
                            'can serve to improve the learning experience for individuals with visual disabilities. ' +
                            'Note that not all simulations that support sound also support extra sound. Also note ' +

@samreid
Copy link
Member

samreid commented Sep 16, 2022

I applied the patch and regenerated API files, closing.

@samreid samreid closed this as completed Sep 16, 2022
@samreid samreid reopened this Sep 16, 2022
@samreid
Copy link
Member

samreid commented Sep 16, 2022

Or perhaps a review phase would be good if there's time?

@zepumph
Copy link
Member

zepumph commented Sep 17, 2022

Yes great thanks.

@zepumph zepumph closed this as completed Sep 17, 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

4 participants