addressed several review comments related to adding support for globa…

…l sound controls, see #497
phetsims · Jul 19, 2018 · bf62c79 · bf62c79
1 parent 7569848
commit bf62c79
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 19 deletions.
diff --git a/js/NavigationBar.js b/js/NavigationBar.js
@@ -41,6 +41,7 @@ define( function( require ) {
   var PhetFont = require( 'SCENERY_PHET/PhetFont' );
   var platform = require( 'PHET_CORE/platform' );
   var Rectangle = require( 'SCENERY/nodes/Rectangle' );
+  var soundManager = require( 'TAMBO/soundManager' );
   var StringUtils = require( 'PHETCOMMON/util/StringUtils' );
   var Text = require( 'SCENERY/nodes/Text' );
 
@@ -126,18 +127,21 @@ define( function( require ) {
 
     // @public (joist-internal) - PhET button. The transform of this is tracked, so we can mirror it over to the
     // homescreen's button. See https://github.com/phetsims/joist/issues/304.
-    this.phetButton = new PhetButton( sim, sim.lookAndFeel.navigationBarFillProperty, sim.lookAndFeel.navigationBarTextFillProperty, tandem.createTandem( 'phetButton' ) );
+    this.phetButton = new PhetButton(
+      sim,
+      sim.lookAndFeel.navigationBarFillProperty,
+      sim.lookAndFeel.navigationBarTextFillProperty,
+      tandem.createTandem( 'phetButton' )
+    );
     this.barContents.addChild( this.phetButton );
 
     // list of optional buttons added for a11y
     var a11yButtons = [];
 
     // only put the sound on/off button on the nav bar if the sound library is enabled
-    if ( phet.chipper.tambo ) {
+    if ( sim.supportsSound ) {
       var soundOnOffButton = new NavigationBarSoundToggleButton(
-        // REVIEW: I saw code in another file that provided another BooleanProperty if the soundManager wasn't defined
-        // REVIEW: How does that relate to this code?
-        sim.soundManager.enabledProperty,
+        soundManager.enabledProperty,
         sim.lookAndFeel,
         tandem.createTandem( 'soundOnOffButton' )
       );
@@ -149,7 +153,6 @@ define( function( require ) {
     if ( phet.chipper.accessibility && sim.keyboardHelpNode && !platform.mobileSafari ) {
 
       // @public (joist-internal, read-only) - Pops open a dialog with information about keyboard navigation
-      // REVIEW: Why is this a property and not a local var?  It seems like it can be a local var
       this.keyboardHelpButton = new KeyboardHelpButton(
         sim.keyboardHelpNode,
         sim.lookAndFeel,

diff --git a/js/PhetMenu.js b/js/PhetMenu.js
@@ -29,6 +29,7 @@ define( function( require ) {
   var Rectangle = require( 'SCENERY/nodes/Rectangle' );
   var ScreenshotGenerator = require( 'JOIST/ScreenshotGenerator' );
   var Shape = require( 'KITE/Shape' );
+  var soundManager = require( 'TAMBO/soundManager' );
   var StringUtils = require( 'PHETCOMMON/util/StringUtils' );
   var Text = require( 'SCENERY/nodes/Text' );
   var UpdateCheck = require( 'JOIST/UpdateCheck' );
@@ -275,13 +276,10 @@ define( function( require ) {
       // "Enhanced Sound" menu item
       {
         text: menuItemEnhancedSoundString,
-        present: phet.chipper.supportsEnhancedSound,
-
-        // REVIEW: It is odd that one property reference is used for checkedProperty, and a different one is used for
-        // REVIEW: the callback
-        checkedProperty: sim.soundManager.enhancedSoundEnabledProperty,
+        present: sim.supportsEnhancedSound,
+        checkedProperty: soundManager.enhancedSoundEnabledProperty,
         callback: function() {
-          sim.soundManager.enhancedSoundEnabledProperty.set( !sim.soundManager.enhancedSoundEnabledProperty.get() );
+          soundManager.enhancedSoundEnabledProperty.set( !soundManager.enhancedSoundEnabledProperty.get() );
         },
         tandem: tandem.createTandem( 'enhancedSoundMenuItem' ),
         phetioInstanceDocumentation: 'This menu item toggles between basic and enhanced sound modes.',

diff --git a/js/Sim.js b/js/Sim.js
@@ -267,21 +267,20 @@ define( function( require ) {
     // Set up accessibility features for the sim.
     phet.chipper.accessibility && initializeAccessibility();
 
-    // @public ( joist-internal, read-only )
+    // @public (joist-internal, read-only)
     this.keyboardHelpNode = options.keyboardHelpNode;
 
     // Set/update global flag values that enable and configure the sound library.  These can be controlled through sim
     // flags or query params.
 
-    // REVIEW: It seems awkward for these to be globals.  Can they be converted to non-globals that are passed where appropriate?
-    phet.chipper.tambo = phet.chipper.tambo || options.tambo;
-    phet.chipper.supportsEnhancedSound = phet.chipper.supportsEnhancedSound || options.supportsEnhancedSound;
+    // @public (joist-internal, read-only) {boolean} - true if the simulation uses the tambo sound library
+    this.supportsSound = phet.chipper.queryParameters.tambo || options.tambo;
 
-    // @public (read-only) {soundManager}
-    this.soundManager = soundManager;
+    // @public (joist-internal, read-only) {boolean} - true if the simulation supports enhanced sound
+    this.supportsEnhancedSound = phet.chipper.queryParameters.supportsEnhancedSound || options.supportsEnhancedSound;
 
     // Initialize the sound library if enabled.
-    if ( phet.chipper.tambo ) {
+    if ( this.supportsSound ) {
       soundManager.initialize( this.browserTabVisibleProperty );
     }