search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Code Review RichDragListener #858

Open jessegreenberg opened 1 month ago

jessegreenberg commented 1 month ago

In https://github.com/phetsims/scenery/issues/1614 we created a combined listener that supports both pointer and keyboard dragging. The listener is in scenery-phet because it also supports sounds from tambo (see #849).

This would benefit from a code review. In particular

jessegreenberg commented 1 month ago

For the reviewer:

jessegreenberg commented 1 month ago

Over slack @zepumph mentioned he can do this (and thank you!). @zepumph if that doesn't work anymore let me know and I will find another.

zepumph commented 1 month ago

@jessegreenberg and I had a sync conversation that went well. I communicated my initial feedback and I believe he will paste in notes at some point for next steps. Back to him.

jessegreenberg commented 1 month ago

@zepumph reviewed and we met on zoom to discuss next steps.

We also discussed:

- General sentiment of hiding the implementations of the two types of classes - JG: My sentiment is that "pointer" and "keyboard" are inherently different in how they behave and it isn't reasonable to hide their behaviors. - Consider extending one of the listeners, and composing the other. - Does not seem like there is a huge benefit. Complicates the API because we want to support exposing each listener. - We are not thrilled with "Pointer" in the names. "Pointer" has very scenery specific meanings. But "Pointer" is very clear to other devs/devs outside of PhET, so we are going to keep it. We are not thrilled with "Rich" either, but can't think of anything better. "Rich" fits well in scenery.
jessegreenberg commented 2 weeks ago

RichDragListener has been moved to scenery, but here is a patch with changes to scenery-phet that refactor the sound related code. I am happy with the de-duplication but there are still ts-expect-errors in places where I couldn't figure out how to abstract types.

```patch Subject: [PATCH] New strings and keyboard help content for HeaterCoolerNode, see --- Index: scenery/js/listeners/RichDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/listeners/RichDragListener.ts b/scenery/js/listeners/RichDragListener.ts --- a/scenery/js/listeners/RichDragListener.ts (revision 30c177084c155f854b4bd2e9536e0c7b17fd2463) +++ b/scenery/js/listeners/RichDragListener.ts (date 1718920695623) @@ -2,7 +2,8 @@ /** * A drag listener that supports both pointer and keyboard input. It is composed with a DragListener and a - * KeyboardDragListener to support pointer input, alternative input, sounds, and other PhET-specific features. + * KeyboardDragListener to support pointer input and alternative input. In the future it can support other + * input modalities or PhET-specific features. * * Be sure to dispose of this listener when it is no longer needed. * @@ -69,7 +70,7 @@ dragBoundsProperty?: TReadOnlyProperty | null; // If provided, this allows custom mapping from the desired position (i.e. where the pointer is, or where the - // RichKeyboardListener will set the position) to the actual position that will be used. + // KeyboardListener will set the position) to the actual position that will be used. mapPosition?: null | ( ( point: Vector2 ) => Vector2 ); // If true, the target Node will be translated during the drag operation. Index: scenery-phet/js/SoundRichDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/SoundRichDragListener.ts b/scenery-phet/js/SoundRichDragListener.ts --- a/scenery-phet/js/SoundRichDragListener.ts (revision 313e4aa4b886068e04bbc37eca85873134b844ab) +++ b/scenery-phet/js/SoundRichDragListener.ts (date 1718922308772) @@ -1,88 +1,23 @@ // Copyright 2024, University of Colorado Boulder /** - * A drag listener that supports both pointer and keyboard input. It is composed with a RichPointerDragListener and a - * RichKeyboardDragListener to support pointer input, alternative input, sounds, and other PhET-specific features. - * - * Be sure to dispose of this listener when it is no longer needed. - * - * Options that are common to both listeners are provided directly to this listener. Options that are specific to - * a particular listener can be provided through the richPointerDragListenerOptions or richKeyboardDragListenerOptions. - * - * Typical PhET usage will use a position Property in a model coordinate frame and look like this: - * - * // A focusable Node that can be dragged with pointer or keyboard. - * const draggableNode = new Node( { - * tagName: 'div', - * focusable: true - * } ); - * - * const richDragListener = new SoundRichDragListener( { - * positionProperty: someObject.positionProperty, - * transform: modelViewTransform - * } ); - * - * draggableNode.addInputListener( richDragListener ); - * - * This listener works by implementing TInputListener and forwarding input events to the specific listeners. This is - * how we support adding this listener through the scenery input listener API. + * A RichDragListener that supports sounds for grab and release. * * @author Jesse Greenberg */ -import { Hotkey, PressListenerEvent, SceneryEvent, TInputListener } from '../../scenery/js/imports.js'; -import TProperty from '../../axon/js/TProperty.js'; -import Vector2 from '../../dot/js/Vector2.js'; -import Transform3 from '../../dot/js/Transform3.js'; -import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; -import Bounds2 from '../../dot/js/Bounds2.js'; -import RichPointerDragListener, { PressedRichPointerDragListener, RichPointerDragListenerOptions } from './RichPointerDragListener.js'; -import RichKeyboardDragListener, { RichKeyboardDragListenerOptions } from './RichKeyboardDragListener.js'; +import { DragListener, DragListenerOptions, KeyboardDragListener, KeyboardDragListenerOptions, RichDragListener, RichDragListenerOptions } from '../../scenery/js/imports.js'; import optionize, { combineOptions } from '../../phet-core/js/optionize.js'; import WrappedAudioBuffer from '../../tambo/js/WrappedAudioBuffer.js'; -import { SoundClipOptions } from '../../tambo/js/sound-generators/SoundClip.js'; -import { SoundGeneratorAddOptions } from '../../tambo/js/soundManager.js'; +import SoundClip, { SoundClipOptions } from '../../tambo/js/sound-generators/SoundClip.js'; +import soundManager, { SoundGeneratorAddOptions } from '../../tambo/js/soundManager.js'; import grab_mp3 from '../../tambo/sounds/grab_mp3.js'; import release_mp3 from '../../tambo/sounds/release_mp3.js'; import SceneryPhetConstants from './SceneryPhetConstants.js'; import sceneryPhet from './sceneryPhet.js'; -import DerivedProperty from '../../axon/js/DerivedProperty.js'; - -type SelfOptions = { - - // Called when the drag is started, for any input type. If you want to determine the type of input, you can check - // SceneryEvent.isFromPDOM or SceneryEvent.type. If you need a start behavior for a specific form of input, - // provide a start callback for that listener's options. It will be called IN ADDITION to this callback. - start?: ( ( event: SceneryEvent, listener: RichPointerDragListener | RichKeyboardDragListener ) => void ) | null; - - // Called during the drag event, for any input type. If you want to determine the type of input, you can check - // SceneryEvent.isFromPDOM or SceneryEvent.type. If you need a drag behavior for a specific form of input, - // provide a drag callback for that listener's options. It will be called IN ADDITION to this callback. - drag?: ( ( event: SceneryEvent, listener: RichPointerDragListener | RichKeyboardDragListener ) => void ) | null; - - // Called when the drag is ended, for any input type. If you want to determine the type of input, you can check - // SceneryEvent.isFromPDOM or SceneryEvent.type. If you need an end behavior for a specific form of input, - // provide an end callback for that listener's options. It will be called IN ADDITION to this callback. The event - // may be null for cases of interruption. - end?: ( ( event: SceneryEvent | null, listener: RichPointerDragListener | RichKeyboardDragListener ) => void ) | null; - - // If provided, it will be synchronized with the drag position in the model coordinate frame. The optional transform - // is applied. - positionProperty?: Pick, 'value'> | null; - - // If provided, this will be used to convert between the parent (view) and model coordinate frames. Most useful - // when you also provide a positionProperty. - transform?: Transform3 | TReadOnlyProperty | null; - - // If provided, the model position will be constrained to these bounds. - dragBoundsProperty?: TReadOnlyProperty | null; - // If provided, this allows custom mapping from the desired position (i.e. where the pointer is, or where the - // RichKeyboardListener will set the position) to the actual position that will be used. - mapPosition?: null | ( ( point: Vector2 ) => Vector2 ); - - // If true, the target Node will be translated during the drag operation. - translateNode?: boolean; +// Options for drag listeners that are specific to sound. +export type RichDragListenerSoundOptions = { // Grab and release sounds. `null` means no sound. grabSound?: WrappedAudioBuffer | null; @@ -95,260 +30,113 @@ // addSoundGeneratorOptions grabSoundGeneratorAddOptions?: SoundGeneratorAddOptions; releaseSoundGeneratorAddOptions?: SoundGeneratorAddOptions; - - // Additional options for the RichPointerDragListener, OR any overrides for the RichPointerDragListener that should - // be used instead of the above options. For example, if the RichPointerDragListener should have different - // grab/release sounds, you can provide those options here. - richPointerDragListenerOptions?: RichPointerDragListenerOptions; - - // Additional options for the RichKeyboardDragListener, OR any overrides for the RichKeyboardDragListener that should - // be used instead of the above options. For example, if the RichKeyboardDragListener should have different - // grab/release sounds, you can provide those options here. - richKeyboardDragListenerOptions?: RichKeyboardDragListenerOptions; }; -export type RichDragListenerOptions = SelfOptions; - -export default class SoundRichDragListener implements TInputListener { - private readonly richPointerDragListener: RichPointerDragListener; - private readonly richKeyboardDragListener: RichKeyboardDragListener; - - // True if the listener is currently pressed (RichPointerDragListener OR RichKeyboardDragListener). - public readonly isPressedProperty: TReadOnlyProperty; +type SelfOptions = { + keyboardDragListenerSoundOptions?: RichDragListenerSoundOptions; + dragListenerSoundOptions?: RichDragListenerSoundOptions; +} & RichDragListenerSoundOptions; - // Properties for each of the pressed states of the RichPointerDragListener and RichKeyboardDragListener. - public readonly keyboardListenerPressedProperty: TReadOnlyProperty; - public readonly pointerListenerPressedProperty: TReadOnlyProperty; +export type SoundRichDragListenerOptions = RichDragListenerOptions & SelfOptions; - // Implements TInputListener - public readonly hotkeys: Hotkey[]; +export default class SoundRichDragListener extends RichDragListener { + public constructor( providedOptions?: SoundRichDragListenerOptions ) { - public constructor( providedOptions?: RichDragListenerOptions ) { - - const options = optionize()( { - - // RichDragListenerOptions - positionProperty: null, - start: null, - end: null, - drag: null, - transform: null, - dragBoundsProperty: null, - mapPosition: null, - translateNode: false, + const options = optionize()( { grabSound: grab_mp3, releaseSound: release_mp3, grabSoundClipOptions: SceneryPhetConstants.DEFAULT_DRAG_CLIP_OPTIONS, releaseSoundClipOptions: SceneryPhetConstants.DEFAULT_DRAG_CLIP_OPTIONS, grabSoundGeneratorAddOptions: SceneryPhetConstants.DEFAULT_GRAB_SOUND_GENERATOR_ADD_OPTIONS, releaseSoundGeneratorAddOptions: SceneryPhetConstants.DEFAULT_GRAB_SOUND_GENERATOR_ADD_OPTIONS, - richPointerDragListenerOptions: {}, - richKeyboardDragListenerOptions: {} + keyboardDragListenerSoundOptions: {}, + dragListenerSoundOptions: {} }, providedOptions ); - // Options that will apply to both listeners. - const sharedOptions = { - positionProperty: options.positionProperty, - transform: options.transform, - dragBoundsProperty: options.dragBoundsProperty || undefined, - mapPosition: options.mapPosition || undefined, - translateNode: options.translateNode, - grabSound: options.grabSound, - releaseSound: options.releaseSound, - grabSoundClipOptions: options.grabSoundClipOptions, - releaseSoundClipOptions: options.releaseSoundClipOptions, - grabSoundGeneratorAddOptions: options.grabSoundGeneratorAddOptions - }; - - //--------------------------------------------------------------------------------- - // Construct the RichPointerDragListener and combine its options. - //--------------------------------------------------------------------------------- - const wrappedDragListenerStart = ( event: PressListenerEvent, listener: PressedRichPointerDragListener ) => { - - // when the drag listener starts, interrupt the keyboard dragging - this.richKeyboardDragListener.interrupt(); - - options.start && options.start( event, listener ); - options.richPointerDragListenerOptions.start && options.richPointerDragListenerOptions.start( event, listener ); - }; - - const wrappedDragListenerDrag = ( event: PressListenerEvent, listener: PressedRichPointerDragListener ) => { - options.drag && options.drag( event, listener ); - options.richPointerDragListenerOptions.drag && options.richPointerDragListenerOptions.drag( event, listener ); - }; - - const wrappedDragListenerEnd = ( event: PressListenerEvent | null, listener: PressedRichPointerDragListener ) => { - options.end && options.end( event, listener ); - options.richPointerDragListenerOptions.end && options.richPointerDragListenerOptions.end( event, listener ); - }; - - const richPointerDragListenerOptions = combineOptions( - // target object - {}, - // Options that apply to both, but can be overridden by provided listener-specific options - sharedOptions, - // Provided listener-specific options - options.richPointerDragListenerOptions, - // Options that cannot be overridden - see wrapped callbacks above - { - start: wrappedDragListenerStart, - drag: wrappedDragListenerDrag, - end: wrappedDragListenerEnd - } + // Apply overrides for the KeyboardDragListener sounds and forward to the KeyboardDragListenerOptions drag callbacks. + const dragListenerClips = SoundRichDragListener.wireSoundsToDragCallbacks( + options, + options.keyboardDragListenerSoundOptions, + options.keyboardDragListenerOptions ); - this.richPointerDragListener = new RichPointerDragListener( richPointerDragListenerOptions ); - - //--------------------------------------------------------------------------------- - // Construct the RichKeyboardDragListener and combine its options. - //--------------------------------------------------------------------------------- - const wrappedKeyboardListenerStart = ( event: SceneryEvent, listener: RichKeyboardDragListener ) => { - - // when the drag listener starts, interrupt the pointer dragging - this.richPointerDragListener.interrupt(); - - options.start && options.start( event, listener ); - options.richKeyboardDragListenerOptions.start && options.richKeyboardDragListenerOptions.start( event, listener ); - }; - - const wrappedKeyboardListenerDrag = ( event: SceneryEvent, listener: RichKeyboardDragListener ) => { - options.drag && options.drag( event, listener ); - options.richKeyboardDragListenerOptions.drag && options.richKeyboardDragListenerOptions.drag( event, listener ); - }; - - const wrappedKeyboardListenerEnd = ( event: SceneryEvent | null, listener: RichKeyboardDragListener ) => { - options.end && options.end( event, listener ); - options.richKeyboardDragListenerOptions.end && options.richKeyboardDragListenerOptions.end( event, listener ); - }; - - const richKeyboardDragListenerOptions = combineOptions( - // target object - {}, - // Options that apply to both, but can be overridden by provided listener-specific options - sharedOptions, - // Provided listener-specific options - options.richKeyboardDragListenerOptions, - // Options that cannot be overridden - see wrapped callbacks above - { - start: wrappedKeyboardListenerStart, - drag: wrappedKeyboardListenerDrag, - end: wrappedKeyboardListenerEnd - } + // Apply overrides for the DragListener sounds and forward to the DragListenerOptions drag callbacks. + const keyboardDragListenerClips = SoundRichDragListener.wireSoundsToDragCallbacks( + options, + options.dragListenerSoundOptions, + options.dragListenerOptions ); - this.richKeyboardDragListener = new RichKeyboardDragListener( richKeyboardDragListenerOptions ); - - // The hotkeys from the keyboard listener are assigned to this listener so that they are activated for Nodes - // where this listener is added. - this.hotkeys = this.richKeyboardDragListener.hotkeys; + super( options ); - this.isPressedProperty = DerivedProperty.or( [ this.richPointerDragListener.isPressedProperty, this.richKeyboardDragListener.isPressedProperty ] ); - this.keyboardListenerPressedProperty = this.richKeyboardDragListener.isPressedProperty; - this.pointerListenerPressedProperty = this.richPointerDragListener.isPressedProperty; + // Dispose the grab and release clips when the listener is disposed. + SoundRichDragListener.wireDisposeListener( this.dragListener, dragListenerClips[ 0 ], dragListenerClips[ 1 ] ); + SoundRichDragListener.wireDisposeListener( this.keyboardDragListener, keyboardDragListenerClips[ 0 ], keyboardDragListenerClips[ 1 ] ); } - public get isPressed(): boolean { - return this.richPointerDragListener.isPressed || this.richKeyboardDragListener.isPressed; - } + public static wireSoundsToDragCallbacks( + soundOptions: RichDragListenerSoundOptions, + listenerSoundOptions: RichDragListenerSoundOptions | undefined, + listenerOptions: KeyboardDragListenerOptions | DragListenerOptions | undefined + ): [ SoundClip | undefined, SoundClip | undefined ] { - public dispose(): void { - this.isPressedProperty.dispose(); + const dragListenerSoundOptions = combineOptions( {}, soundOptions, listenerSoundOptions ); + listenerOptions = listenerOptions || {}; - this.richPointerDragListener.dispose(); - this.richKeyboardDragListener.dispose(); - } + // Create the grab SoundClip and wire it into the start function for the drag cycle. + let dragClip: SoundClip | undefined; + if ( dragListenerSoundOptions.grabSound ) { + dragClip = new SoundClip( dragListenerSoundOptions.grabSound, dragListenerSoundOptions.grabSoundClipOptions ); + soundManager.addSoundGenerator( dragClip, dragListenerSoundOptions.grabSoundGeneratorAddOptions ); - /** - * ******************************************************************** - * Forward input to both listeners - * ******************************************************************** - */ - public interrupt(): void { - this.richPointerDragListener.interrupt(); - this.richKeyboardDragListener.interrupt(); - } - - /** - * ******************************************************************** - * Forward to the KeyboardListener - * ******************************************************************** - */ - public keydown( event: SceneryEvent ): void { - this.richKeyboardDragListener.keydown( event ); - } - - public focusout( event: SceneryEvent ): void { - this.richKeyboardDragListener.focusout( event ); - } + const previousStart = listenerOptions.start; - public focusin( event: SceneryEvent ): void { - this.richKeyboardDragListener.focusin( event ); - } + // @ts-expect-error - The args have implicit any type because of the union of KeyboardDragListenerOptions and + // DragListenerOptions. I (JG) couldn't figure out how to type this properly, even defining the args. + listenerOptions.start = ( ...args ) => { - public cancel(): void { - this.richKeyboardDragListener.cancel(); - } + // @ts-expect-error - see above note. + previousStart && previousStart( ...args ); + dragClip!.play(); + }; + } - /** - * ******************************************************************** - * Forward to the DragListener - * ******************************************************************** - */ - public click( event: SceneryEvent ): void { - this.richPointerDragListener.click( event ); - } + // Create the release SoundClip and wire it into the end function for the drag cycle. + let releaseClip: SoundClip | undefined; + if ( dragListenerSoundOptions.releaseSound ) { + releaseClip = new SoundClip( dragListenerSoundOptions.releaseSound, dragListenerSoundOptions.releaseSoundClipOptions ); + soundManager.addSoundGenerator( releaseClip, dragListenerSoundOptions.releaseSoundGeneratorAddOptions ); - public touchenter( event: PressListenerEvent ): void { - this.richPointerDragListener.touchenter( event ); - } + const previousEnd = listenerOptions.end; - public touchmove( event: PressListenerEvent ): void { - this.richPointerDragListener.touchmove( event ); - } + // @ts-expect-error - The args have implicit any type because of the union of KeyboardDragListenerOptions and + // DragListenerOptions. I (JG) couldn't figure out how to type this properly, even defining the args. + listenerOptions.end = ( ...args ) => { - public focus( event: SceneryEvent ): void { - this.richPointerDragListener.focus( event ); - } + // @ts-expect-error - see above note. + previousEnd && previousEnd( ...args ); - public blur(): void { - this.richPointerDragListener.blur(); - } + const listener = args[ 1 ]; + !listener.interrupted && releaseClip!.play(); + }; + } - public down( event: PressListenerEvent ): void { - this.richPointerDragListener.down( event ); + return [ dragClip, releaseClip ]; } - public up( event: PressListenerEvent ): void { - this.richPointerDragListener.up( event ); - } + public static wireDisposeListener( listener: DragListener | KeyboardDragListener, grabClip: SoundClip | undefined, releaseClip: SoundClip | undefined ): void { + listener.disposeEmitter.addListener( () => { + if ( grabClip ) { + grabClip.dispose(); + soundManager.removeSoundGenerator( grabClip ); + } - public enter( event: PressListenerEvent ): void { - this.richPointerDragListener.enter( event ); - } - - public move( event: PressListenerEvent ): void { - this.richPointerDragListener.move( event ); + if ( releaseClip ) { + releaseClip.dispose(); + soundManager.removeSoundGenerator( releaseClip ); + } + } ); } - - public exit( event: PressListenerEvent ): void { - this.richPointerDragListener.exit( event ); - } - - public pointerUp( event: PressListenerEvent ): void { - this.richPointerDragListener.pointerUp( event ); - } - - public pointerCancel( event: PressListenerEvent ): void { - this.richPointerDragListener.pointerCancel( event ); - } - - public pointerMove( event: PressListenerEvent ): void { - this.richPointerDragListener.pointerMove( event ); - } - - public pointerInterrupt(): void { - this.richPointerDragListener.pointerInterrupt(); - } } sceneryPhet.register( 'SoundRichDragListener', SoundRichDragListener ); \ No newline at end of file Index: scenery-phet/js/demo/components/demoRichDragListeners.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/demo/components/demoRichDragListeners.ts b/scenery-phet/js/demo/components/demoRichDragListeners.ts --- a/scenery-phet/js/demo/components/demoRichDragListeners.ts (revision 313e4aa4b886068e04bbc37eca85873134b844ab) +++ b/scenery-phet/js/demo/components/demoRichDragListeners.ts (date 1718909724674) @@ -95,7 +95,7 @@ richDragListenerEllipse.addInputListener( new SoundRichDragListener( { dragBoundsProperty: dragBoundsProperty, translateNode: true, - richKeyboardDragListenerOptions: { + keyboardDragListenerOptions: { dragSpeed: RADIUS * 5, shiftDragSpeed: RADIUS }, Index: scenery-phet/js/RichKeyboardDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/RichKeyboardDragListener.ts b/scenery-phet/js/RichKeyboardDragListener.ts --- a/scenery-phet/js/RichKeyboardDragListener.ts (revision 313e4aa4b886068e04bbc37eca85873134b844ab) +++ b/scenery-phet/js/RichKeyboardDragListener.ts (date 1718923318123) @@ -12,90 +12,29 @@ * @author Agustín Vallejo * @author Michael Kauzmann * @author Chris Malley (PixelZoom, Inc.) + * @author Jesse Greenberg */ import sceneryPhet from './sceneryPhet.js'; import { KeyboardDragListener, KeyboardDragListenerOptions } from '../../scenery/js/imports.js'; -import optionize from '../../phet-core/js/optionize.js'; -import SoundClip, { SoundClipOptions } from '../../tambo/js/sound-generators/SoundClip.js'; -import grab_mp3 from '../../tambo/sounds/grab_mp3.js'; -import release_mp3 from '../../tambo/sounds/release_mp3.js'; -import soundManager, { SoundGeneratorAddOptions } from '../../tambo/js/soundManager.js'; -import WrappedAudioBuffer from '../../tambo/js/WrappedAudioBuffer.js'; -import SceneryPhetConstants from './SceneryPhetConstants.js'; - -type SelfOptions = { - - // Grab and release sounds. null means no sound. - grabSound?: WrappedAudioBuffer | null; - releaseSound?: WrappedAudioBuffer | null; - - // Passed to the grab and release SoundClip instances. - grabSoundClipOptions?: SoundClipOptions; - releaseSoundClipOptions?: SoundClipOptions; +import SoundRichDragListener, { RichDragListenerSoundOptions } from './SoundRichDragListener.js'; - // addSoundGeneratorOptions - grabSoundGeneratorAddOptions?: SoundGeneratorAddOptions; - releaseSoundGeneratorAddOptions?: SoundGeneratorAddOptions; -}; - -export type RichKeyboardDragListenerOptions = SelfOptions & KeyboardDragListenerOptions; +export type RichKeyboardDragListenerOptions = KeyboardDragListenerOptions & RichDragListenerSoundOptions; export default class RichKeyboardDragListener extends KeyboardDragListener { - public constructor( providedOptions: RichKeyboardDragListenerOptions ) { - const options = optionize()( { - - // SelfOptions - grabSound: grab_mp3, - releaseSound: release_mp3, - grabSoundClipOptions: SceneryPhetConstants.DEFAULT_DRAG_CLIP_OPTIONS, - releaseSoundClipOptions: SceneryPhetConstants.DEFAULT_DRAG_CLIP_OPTIONS, - grabSoundGeneratorAddOptions: SceneryPhetConstants.DEFAULT_GRAB_SOUND_GENERATOR_ADD_OPTIONS, - releaseSoundGeneratorAddOptions: SceneryPhetConstants.DEFAULT_GRAB_SOUND_GENERATOR_ADD_OPTIONS - }, providedOptions ); - - // Create the grab SoundClip and wire it into the start function for the drag cycle. - let grabClip: SoundClip; - if ( options.grabSound ) { - grabClip = new SoundClip( options.grabSound, options.grabSoundClipOptions ); - soundManager.addSoundGenerator( grabClip, options.grabSoundGeneratorAddOptions ); - - const previousStart = options.start; - options.start = ( ...args ) => { - previousStart && previousStart( ...args ); - grabClip.play(); - }; - } - - // Create the release SoundClip and wire it into the end function for the drag cycle. - let releaseClip: SoundClip; - if ( options.releaseSound ) { - releaseClip = new SoundClip( options.releaseSound, options.releaseSoundClipOptions ); - soundManager.addSoundGenerator( releaseClip, options.releaseSoundGeneratorAddOptions ); + // Apply the sound options to the drag listener, wrapping start/end callbacks with functions that will + // play sounds on grab and release. + const soundClips = SoundRichDragListener.wireSoundsToDragCallbacks( + {}, + providedOptions, + providedOptions + ); - const previousEnd = options.end; - options.end = ( ...args ) => { - previousEnd && previousEnd( ...args ); - !this.interrupted && releaseClip.play(); - }; - } + super( providedOptions ); - super( options ); - - // Clean up SoundClips when this RichKeyboardDragListener is disposed. - this.disposeEmitter.addListener( () => { - if ( grabClip ) { - grabClip.dispose(); - soundManager.removeSoundGenerator( grabClip ); - } - - if ( releaseClip ) { - releaseClip.dispose(); - soundManager.removeSoundGenerator( releaseClip ); - } - } ); + SoundRichDragListener.wireDisposeListener( this, soundClips[ 0 ], soundClips[ 1 ] ); } } Index: scenery-phet/js/RichPointerDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/RichPointerDragListener.ts b/scenery-phet/js/RichPointerDragListener.ts --- a/scenery-phet/js/RichPointerDragListener.ts (revision 313e4aa4b886068e04bbc37eca85873134b844ab) +++ b/scenery-phet/js/RichPointerDragListener.ts (date 1718923332964) @@ -12,95 +12,32 @@ * @author Agustín Vallejo * @author Michael Kauzmann * @author Chris Malley (PixelZoom, Inc.) + * @author Jesse Greenberg */ import sceneryPhet from './sceneryPhet.js'; import { DragListener, DragListenerOptions, PressedDragListener } from '../../scenery/js/imports.js'; -import optionize from '../../phet-core/js/optionize.js'; -import SoundClip, { SoundClipOptions } from '../../tambo/js/sound-generators/SoundClip.js'; -import grab_mp3 from '../../tambo/sounds/grab_mp3.js'; -import release_mp3 from '../../tambo/sounds/release_mp3.js'; -import soundManager, { SoundGeneratorAddOptions } from '../../tambo/js/soundManager.js'; -import WrappedAudioBuffer from '../../tambo/js/WrappedAudioBuffer.js'; -import SceneryPhetConstants from './SceneryPhetConstants.js'; - -type SelfOptions = { - - // Grab and release sounds. null means no sound. - grabSound?: WrappedAudioBuffer | null; - releaseSound?: WrappedAudioBuffer | null; - - // Passed to the grab and release SoundClip instances. - grabSoundClipOptions?: SoundClipOptions; - releaseSoundClipOptions?: SoundClipOptions; - - // addSoundGeneratorOptions - grabSoundGeneratorAddOptions?: SoundGeneratorAddOptions; - releaseSoundGeneratorAddOptions?: SoundGeneratorAddOptions; -}; +import SoundRichDragListener, { RichDragListenerSoundOptions } from './SoundRichDragListener.js'; // Pattern followed from DragListenerOptions. export type PressedRichPointerDragListener = RichPointerDragListener & PressedDragListener; -export type RichPointerDragListenerOptions = SelfOptions & DragListenerOptions; +export type RichPointerDragListenerOptions = DragListenerOptions & RichDragListenerSoundOptions; export default class RichPointerDragListener extends DragListener { - public constructor( providedOptions: RichPointerDragListenerOptions ) { - const options = optionize>()( { - - // SelfOptions - grabSound: grab_mp3, - releaseSound: release_mp3, - grabSoundClipOptions: SceneryPhetConstants.DEFAULT_DRAG_CLIP_OPTIONS, - releaseSoundClipOptions: SceneryPhetConstants.DEFAULT_DRAG_CLIP_OPTIONS, - grabSoundGeneratorAddOptions: SceneryPhetConstants.DEFAULT_GRAB_SOUND_GENERATOR_ADD_OPTIONS, - releaseSoundGeneratorAddOptions: SceneryPhetConstants.DEFAULT_GRAB_SOUND_GENERATOR_ADD_OPTIONS - }, providedOptions ); + // 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( + {}, + providedOptions, + providedOptions + ); + super( providedOptions ); - // Create the grab SoundClip and wire it into the start function for the drag cycle. - let grabClip: SoundClip; - if ( options.grabSound ) { - - grabClip = new SoundClip( options.grabSound, options.grabSoundClipOptions ); - soundManager.addSoundGenerator( grabClip, options.grabSoundGeneratorAddOptions ); - - const previousStart = options.start; - options.start = ( ...args ) => { - previousStart && previousStart( ...args ); - grabClip.play(); - }; - } - - // Create the release SoundClip and wire it into the end function for the drag cycle. - let releaseClip: SoundClip; - if ( options.releaseSound ) { - - releaseClip = new SoundClip( options.releaseSound, options.releaseSoundClipOptions ); - soundManager.addSoundGenerator( releaseClip, options.releaseSoundGeneratorAddOptions ); - - const previousEnd = options.end; - options.end = ( ...args ) => { - previousEnd && previousEnd( ...args ); - !this.interrupted && releaseClip.play(); - }; - } - - super( options ); - - // Clean up SoundClips when this RichPointerDragListener is disposed. - this.disposeEmitter.addListener( () => { - if ( grabClip ) { - grabClip.dispose(); - soundManager.removeSoundGenerator( grabClip ); - } - - if ( releaseClip ) { - releaseClip.dispose(); - soundManager.removeSoundGenerator( releaseClip ); - } - } ); + // Dispose of the SoundClips when the listener is disposed. + SoundRichDragListener.wireDisposeListener( this, soundClips[ 0 ], soundClips[ 1 ] ); } } ```
jessegreenberg commented 1 week ago

OK, summarizing the above commits:

For these parts:

We are interested in making the API simpler by making it so that you ahve to use listener specific options less frequently. Consider how we can pass all options to the top level providedOptions

I don't think that makes the API simpler yet and I prefer the way it is structured now. Would be good to discuss again before working on this more.

alternatively, just delete these and these listeners will be availabel through SoundRichDragListener

I think there is going to be a lot of use cases for SoundDragListener and possibly SoundKeyboardDragListener, giving them their own classes seems most intuitive for other developers so I decided to leave these classes as they are.

Sorry it has been a while @zepumph, but would you mind re-reviewing these changes from what we discussed in https://github.com/phetsims/scenery-phet/issues/858#issuecomment-2153518978?

zepumph commented 6 days ago

I'm a big fan of these changes. Thanks for all your hard work. I updated some of the typing so that more could be shared. I'd worry about the RichDragListener options getting out of date. I have a couple of TODOs pointing to this issue for you to look at, and help review my changes. Let me know what you think. To be clear, everything I did was with Typescript types, not runtime values. The implementation looks great.