Add sounds for ABSwitch

[jbphet]

The parent issue for this is https://github.com/phetsims/joist/issues/803.

The ABSwitch doesn't currently produce any sound, but it is now used in a number of simulations the do have sound because it appears in the Preferences dialog. I started process already, so there are some commits associated with the issue linked above, but they are all about adding ABSwitches to the tambo demo and creating the sound players. I thought I should have a separate issue for the changes to the code in this repo, so here it is.

[jbphet]

@pixelzoom - Could you please review how sound support was added to these components? There were a number of ways that it could be done, and thought at least one other developer should have a look at what I chose to do, and you're the primary author. One of the things I'm most curious about is passing options that go to the ABSwitch through to the ToggleSwitch, which is used via composition.

There is a test/demo harness in the middle screen of the tambo demo through which you can exercise the new feature. You can see a more 'real world' example by running John Travoltage and interacting with the Preferences dialog.

[pixelzoom]

ToggleSwitch.ts looks good.

But in ABSwitch.ts, I would definitely NOT use the current implementation because:

• switchToASoundPlayer and switchToBSoundPlayer duplicates what is already provided via nested options toggleSwitchOptions. You only need one 1 of setting sound. Having 2 ways is more complicated for implementer and the caller.
• The caller needs to be familiar with ToggleSwitchOptions regardless of sounds, so it's reasonable to expect them to know about switchToLeftSoundPlayer and switchToRightSoundPlayer.
• The documentation clearly states that "Choice 'A' is to the left of the switch, choice 'B' is to the right." So I don't see a problem with requiring the caller to know that "A" is toggleSwitchOptions.switchToLeftSoundPlayer and "B" is toggleSwitchOptions.switchToRightSoundPlayer.
• You haven't added (and should not add) switchToOnSoundPlayer and switchToOffSoundPlayer to OnOffSwitch, so why add class-specific options to ABSwitch?
• toggleSwitchOptions.switchToLeftSoundPlayer and switchToASoundPlayer should be mutually exclusive. Similarly, toggleSwitchOptions.switchToRightSoundPlayer and switchToBSoundPlayer should be mutually exclusive. Your current implementation does not verify those relationships, and silently ignores the latter in each case.

[pixelzoom]

@jbphet Another thing to be aware of going forward is WHERE to add defaults for options. ABSwitch was organized like this:

 const options = optionize<ABSwitchOptions, SelfOptions, NodeOptions>()( {

      // SelfOptions
      ...

      // NodeOptions
      ...
    }, providedOptions );

... and you put the defaults for your new options (which are SelfOptions) under NodeOptions. You did something similar in ToggleSwitch.ts.

I've used this pattern of organizing option defaults throughout common code (and my sim code), so you'll undoubtedly see it elsewhere.

[pixelzoom]

Thinking about this some more... If you prefer having sound options whose names are more aligned with the class names (like switchToASoundPlayer and switchToBSoundPlayer), then another way to do that in ABSwitch to narrow the interface for toggleSwitchOptions like this:

toggleSwitchOptions?: Omit<ToggleSwitchOptions, 'switchToLeftPlayer' | 'switchToRightPlayer'>;

then:

// Use class-specific sounds for the ToggleSwitch.
options.toggleSwitchOptions.switchToLeftSoundPlayer = options.switchToASoundPlayer;
options.toggleSwitchOptions.switchToRightSoundPlayer = options.switchToBSoundPlayer;

The patch below shows the full (untested) solution, and shows how the same pattern would be applied to OnOffSwitch. (Note that in OnOffSwitch, I have imported sound files that do not currently exist. You'd need to create them.)

Let me know if you'd like to discuss.

Index: js/OnOffSwitch.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/OnOffSwitch.ts b/js/OnOffSwitch.ts
--- a/js/OnOffSwitch.ts (revision 52f0492f8ce06e3c9a68bbba628217009387d6e8)
+++ b/js/OnOffSwitch.ts (date 1654042661921)
@@ -11,20 +11,36 @@
 import optionize from '../../phet-core/js/optionize.js';
 import sun from './sun.js';
 import ToggleSwitch, { ToggleSwitchOptions } from './ToggleSwitch.js';
+import switchToOnSoundPlayer from '../../tambo/js/shared-sound-players/switchToOnSoundPlayer.js';
+import switchToOffSoundPlayer from '../../tambo/js/shared-sound-players/switchToOffSoundPlayer.js';

-type SelfOptions = {};
+type SelfOptions = {

-export type OnOffSwitchOptions = SelfOptions & ToggleSwitchOptions;
+  // sound generation, use nullSoundPlayer to turn off
+  switchToOnSoundPlayer?: ISoundPlayer;
+  switchToOffSoundPlayer?: ISoundPlayer;
+};
+
+export type OnOffSwitchOptions = SelfOptions & Omit<ToggleSwitchOptions, 'switchToLeftPlayer' | 'switchToRightPlayer'>;

 export default class OnOffSwitch extends ToggleSwitch<boolean> {

   constructor( property: IProperty<boolean>, providedOptions: OnOffSwitchOptions ) {

     const options = optionize<OnOffSwitchOptions, SelfOptions, ToggleSwitchOptions>()( {
+
+      // SelfOptions
+      switchToOnSoundPlayer: switchToOnSoundPlayer,
+      switchToOffSoundPlayer: switchToOffSoundPlayer,
+
+      // ToggleSwitchOptions
       trackFillLeft: 'white', // track fill when property.value === false
       trackFillRight: 'rgb( 0, 200, 0 )' // track fill when property.value === true
     }, providedOptions );

+    options.switchToLeftPlayer = options.switchToOnSoundPlayer;
+    options.switchToRightPlayer = options.switchToOffSoundPlayer;
+
     super( property, false, true, options );
   }
 }
Index: js/ABSwitch.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/ABSwitch.ts b/js/ABSwitch.ts
--- a/js/ABSwitch.ts    (revision 52f0492f8ce06e3c9a68bbba628217009387d6e8)
+++ b/js/ABSwitch.ts    (date 1654042661924)
@@ -30,7 +30,7 @@
 type SelfOptions = {

   // options passed to ToggleSwitch
-  toggleSwitchOptions?: ToggleSwitchOptions;
+  toggleSwitchOptions?: Omit<ToggleSwitchOptions, 'switchToLeftPlayer' | 'switchToRightPlayer'>;

   // space between labels and switch
   xSpacing?: number;
@@ -78,24 +78,22 @@
       xSpacing: 8,
       setEnabled: DEFAULT_SET_ENABLED,
       centerOnButton: false,
+      switchToASoundPlayer: switchToASoundPlayer,
+      switchToBSoundPlayer: switchToBSoundPlayer,

       // NodeOptions
       cursor: 'pointer',
       disabledOpacity: SceneryConstants.DISABLED_OPACITY,

-      // sound generation
-      switchToASoundPlayer: switchToASoundPlayer,
-      switchToBSoundPlayer: switchToBSoundPlayer,
-
       // phet-io
       tandem: Tandem.REQUIRED,
       visiblePropertyOptions: { phetioFeatured: true },
       phetioEnabledPropertyInstrumented: true // opt into default PhET-iO instrumented enabledProperty
     }, providedOptions );

-    // If no sound players were explicitly specified for the ToggleSwitch, use the ones for the ABSwitch.
-    options.toggleSwitchOptions.switchToLeftSoundPlayer ||= options.switchToASoundPlayer;
-    options.toggleSwitchOptions.switchToRightSoundPlayer ||= options.switchToBSoundPlayer;
+    // Use class-specific sounds for the ToggleSwitch.
+    options.toggleSwitchOptions.switchToLeftSoundPlayer = options.switchToASoundPlayer;
+    options.toggleSwitchOptions.switchToRightSoundPlayer = options.switchToBSoundPlayer;

     super();

[pixelzoom]

If you chose to use the above patch, note that https://github.com/phetsims/phet-core/issues/119 now requires use of OmitStrict instead of Omit.

[jbphet]

Thanks for the thorough review @pixelzoom!

One of my motivations for having sound player options in both ABSwitch and ToggleSwitch was that ABSwitch needs to play sounds when users click on its labels. While it could reach into ToggleSwitch to do this, either via methods or by exposing the sound players publicly, that didn't initially seem great to me. It also seemed like a better API to have these options at the UI component level instead of somewhat buried and, as you observed, I was trying to have the sound option names be more aligned with the class names. However, I'm rethinking this. I now think it would be okay to have the sound player options go through toggleSwitchOptions. Doing otherwise seems to create more complexity than it's worth. I think I would just expose the sound players as public on ToggleSwitch and call them from ABSwitch. OnOffSwitch doesn't look like it has the need to call them, at least not as it's currently written.

So, what would you think of me basically removing the sound player options from ABSwitch, renaming the shared sound players using the terms 'left' and 'right', and not having separate options for the AB and OnOff switches?

[pixelzoom]

I think that's a fine solution. As you noted, it's less complex. And in the future, if anyone wants to use the approach outlined in https://github.com/phetsims/sun/issues/762#issuecomment-1142844044, it won't be difficult.

[jbphet]

I backed out most of the changes to ABSwitch and am now passing the options through to the ToggleSwitch in all cases. I also added an OnOfSwitch to the tambo demo.

@pixelzoom - Care to have one more look?

[pixelzoom]

Looks good to me. Not sure if you want me to close, so back to @jbphet.

[jbphet]

Cool, closing. Note that there may be some follow up on the sounds used in https://github.com/phetsims/joist/issues/803.