SoundDragListener and SoundKeyboardDragListener problems.

[pixelzoom]

While working on https://github.com/phetsims/equality-explorer/issues/215, I had to dive into SoundDragListener.

In SoundDragListener and SoundKeyboardDragListener:

    // Apply the sound options to the drag listener, wrapping start/end callbacks, wrapping start/end callbacks with
    // functions that will play sounds on grab and release.
    const soundClips = SoundRichDragListener.wireSoundsToDragCallbacks(
      SoundRichDragListener.DEFAULT_SOUND_OPTIONS,
      providedOptions,
      providedOptions
    );

    super( providedOptions );

It was not immediately obvious how sounds clips were being wired up. But looking into SoundRichDragListener.wireSoundsToDragCallbacks, I see that the 3rd argument is modified as a side effect. Besides being obfuscated, this is very dangerous. If the same providedOptions value is passed to 2 different instances of SoundDragListener or SoundKeyboardDragListener, then you may get subtly bad behavior, because both instances will be modifying the value.

In general:

• Avoid modifying arguments as a side-effect, especially in a method/function that returns a value.
• Never modify providedOptions.

Highly recommended to change this implementation.

[pixelzoom]

This comment in SoundDragListener also seems to be repeating itself:

    // Apply the sound options to the drag listener, wrapping start/end callbacks, wrapping start/end callbacks with
    // functions that will play sounds on grab and release.

[pixelzoom]

Instead of wrapping start/end callbacks in functions that play the sounds, why not just add a listener to DragListener's isPressedProperty? And perhaps add something like isDraggingProperty to KeyboardDragListener?

[pixelzoom]

SoundDragListener and SoundKeyboardDragListener also contain:

    // When this listener is disposed, sound clips must be removed from the soundManager.
    SoundRichDragListener.wireDisposeListener( this, soundClips[ 0 ], soundClips[ 1 ] );

There is no assertion that soundClips.length === 2. And why not just pass the entire array to wireDisposeListener and have it iterate over the array?

[jessegreenberg]

Instead of wrapping start/end callbacks in functions that play the sounds, why not just add a listener to DragListener's isPressedProperty

That is a great idea, done in the above commit. I accidentally gave it the wrong commit message, but that commit plays sounds by linking to the isPressedProperty of DragListener and KeyboardDragListener instead of wrapping the options callbacks.

For the other comments, I am going to work on https://github.com/phetsims/scenery-phet/issues/889 first because I think that could totally change wireDisposeListener.

[pixelzoom]

For the other comments, I am going to work on https://github.com/phetsims/scenery-phet/issues/889 first because I think that could totally change wireDisposeListener.

I think #889 will make wireDisposeListener unnecessary, as disposal of grabSoundPlayer: TSoundPlayer and releaseSoundPlayer: TSoundPlayer will be the responsibility of whoever created them -- and undesirable in the case of those obtained via sharedSoundPlayers.get.

[jessegreenberg]

wireDisposeListener was removed in #889, so I think this issue is complete as well. @pixelzoom can you please review?

[pixelzoom]

SoundDragListener and SoundKeyboardDragListener look nice. Review of #889 is in https://github.com/phetsims/scenery-phet/issues/889#issuecomment-2494426852.

I'll go ahead and close this. Thanks for the quick turnaround!