phetsims / scenery-phet

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

Add default grab/release sounds to StopwatchNode #845

Closed pixelzoom closed 6 months ago

pixelzoom commented 6 months ago

Required for Gas Properties suite PhET-iO release https://github.com/phetsims/gas-properties/issues/191, and noted in https://github.com/phetsims/gas-properties/issues/214.

  • [ ] Grab/release for StopWatchNode (convert to RichDragListener and RichKeyboardDragListener)
pixelzoom commented 6 months ago

PDL is in dev testing, and that sim does not appear to have addressed grab/release sounds for the stop watch and measuring tape. So I'll hold off on commiting this. The patch below provides the desired behavior.

StopwatchNode patch ```diff Subject: [PATCH] TODO https://github.com/phetsims/gas-properties/issues/213 --- Index: js/StopwatchNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/StopwatchNode.ts b/js/StopwatchNode.ts --- a/js/StopwatchNode.ts (revision f0e0b58e6ca99f49e6f7ec8afc85af96091697c0) +++ b/js/StopwatchNode.ts (date 1711057585612) @@ -18,7 +18,7 @@ import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js'; import optionize, { combineOptions } from '../../phet-core/js/optionize.js'; import StringUtils from '../../phetcommon/js/util/StringUtils.js'; -import { Circle, DragListener, DragListenerOptions, HBox, InteractiveHighlighting, InteractiveHighlightingOptions, KeyboardDragListener, KeyboardDragListenerOptions, Node, NodeOptions, Path, PressedDragListener, PressListenerEvent, TColor, VBox } from '../../scenery/js/imports.js'; +import { Circle, DragListener, HBox, InteractiveHighlighting, InteractiveHighlightingOptions, KeyboardDragListener, KeyboardDragListenerOptions, Node, NodeOptions, Path, PressListenerEvent, TColor, VBox } from '../../scenery/js/imports.js'; import BooleanRectangularToggleButton, { BooleanRectangularToggleButtonOptions } from '../../sun/js/buttons/BooleanRectangularToggleButton.js'; import RectangularPushButton, { RectangularPushButtonOptions } from '../../sun/js/buttons/RectangularPushButton.js'; import Tandem from '../../tandem/js/Tandem.js'; @@ -35,6 +35,8 @@ import TSoundPlayer from '../../tambo/js/TSoundPlayer.js'; import pushButtonSoundPlayer from '../../tambo/js/shared-sound-players/pushButtonSoundPlayer.js'; import DerivedProperty from '../../axon/js/DerivedProperty.js'; +import RichDragListener, { RichDragListenerOptions } from '../../sun/js/RichDragListener.js'; +import RichKeyboardDragListener, { RichKeyboardDragListenerOptions } from '../../sun/js/RichKeyboardDragListener.js'; type SelfOptions = { @@ -57,8 +59,8 @@ dragBoundsProperty?: Property | null; // options propagated to the DragListener - dragListenerOptions?: DragListenerOptions; - keyboardDragListenerOptions?: KeyboardDragListenerOptions; + dragListenerOptions?: RichDragListenerOptions; + keyboardDragListenerOptions?: RichKeyboardDragListenerOptions; // Passed to their respective buttons playPauseButtonOptions?: BooleanRectangularToggleButtonOptions; @@ -355,7 +357,7 @@ } ); // dragging, added to background so that other UI components get input events on touch devices - const dragListenerOptions = combineOptions>( { + const dragListenerOptions = combineOptions( { targetNode: this, positionProperty: stopwatch.positionProperty, dragBoundsProperty: adjustedDragBoundsProperty, @@ -364,14 +366,14 @@ // Add moveToFront to any start function that the client provided. const optionsStart = dragListenerOptions.start!; - dragListenerOptions.start = ( event: PressListenerEvent, listener: PressedDragListener ) => { + dragListenerOptions.start = ( event: PressListenerEvent, listener: RichDragListener ) => { this.moveToFront(); optionsStart( event, listener ); }; // Dragging, added to background so that other UI components get input events on touch devices. // If added to 'this', touchSnag will lock out listeners for other UI components. - this.dragListener = new DragListener( dragListenerOptions ); + this.dragListener = new RichDragListener( dragListenerOptions ); backgroundNode.addInputListener( this.dragListener ); const keyboardDragListenerOptions = combineOptions( { @@ -380,7 +382,7 @@ tandem: options.tandem.createTandem( 'keyboardDragListener' ) }, options.keyboardDragListenerOptions ); - this.keyboardDragListener = new KeyboardDragListener( keyboardDragListenerOptions ); + this.keyboardDragListener = new RichKeyboardDragListener( keyboardDragListenerOptions ); this.addInputListener( this.keyboardDragListener ); // The group focus highlight makes it clear the stopwatch is highlighted even if the children are focused ```
pixelzoom commented 6 months ago

Work completed in https://github.com/phetsims/scenery-phet/commit/9168cf1e8ea8c7b628e6d8fe2868cb87a06d8e70. @samreid would you mind taking a look? Reminder that this impacts https://github.com/phetsims/projectile-data-lab/issues/270.

zepumph commented 6 months ago

I'll review this now as I work on https://github.com/phetsims/scenery-phet/issues/849. One thing I found is that you need to look at all usages of your common code component to see if there are any that are manually calling grab/release sounds. I made this mistake in MeasuringTapeNode, and it looks like wave-interference is doing this for stopwatch node. I'll take a look.

zepumph commented 6 months ago

Everything is looking great here. I found two usages of double sounds, and everything cleaned up really nicely. No other thoughts here. Anything else?

zepumph commented 6 months ago

Oops please hold, I found a few more items (thanks @AgustinVallejo for keeping me honest).

zepumph commented 6 months ago

We found two spots where KeyboardDragListenerOptions were being used where RichKeyboardDragListenerOptions should be used. One will be removed anyways in keplers laws, and one is committed above.

Ok. Anything else?