Add default grab/release sounds to MeasuringTapeNode.

[pixelzoom]

Shouldn't grab/release sounds be included for MeasuringTapeNode in the new PDL sim? See phetsims/projectile-data-lab#270.

Adding support involves a quick conversion to RichDragListener and RichKeyboardDragListener. Patch included below.

Adding options to be propagated to the 4 listeners would be a bit more work, especially given the odd definition of MeasuringTapeNodeOptions.keyboardDragListenerOptions.

MeasuringTapeNode patch

Subject: [PATCH] TODO https://github.com/phetsims/gas-properties/issues/213
---
Index: js/MeasuringTapeNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/MeasuringTapeNode.ts b/js/MeasuringTapeNode.ts
--- a/js/MeasuringTapeNode.ts	(revision f0e0b58e6ca99f49e6f7ec8afc85af96091697c0)
+++ b/js/MeasuringTapeNode.ts	(date 1711058647463)
@@ -35,6 +35,8 @@
 import TProperty from '../../axon/js/TProperty.js';
 import DerivedStringProperty from '../../axon/js/DerivedStringProperty.js';
 import Tandem from '../../tandem/js/Tandem.js';
+import RichKeyboardDragListener from '../../sun/js/RichKeyboardDragListener.js';
+import RichDragListener from '../../sun/js/RichDragListener.js';
 
 export type MeasuringTapeUnits = {
   name: string;
@@ -364,7 +366,7 @@
       };
 
       // Drag listener for base
-      this.baseDragListener = new DragListener( {
+      this.baseDragListener = new RichDragListener( {
         tandem: options.tandem?.createTandem( 'baseDragListener' ),
         start: event => {
           baseStart();
@@ -389,7 +391,7 @@
       this.baseImage.addInputListener( this.baseDragListener );
 
       // Drag listener for base
-      const baseKeyboardDragListener = new KeyboardDragListener( {
+      const baseKeyboardDragListener = new RichKeyboardDragListener( {
         tandem: options.tandem?.createTandem( 'baseKeyboardDragListener' ),
         positionProperty: this.basePositionProperty,
         transform: this.modelViewTransformProperty,
@@ -410,7 +412,7 @@
       let tipStartOffset: Vector2;
 
       // Drag listener for tip
-      const tipDragListener = new DragListener( {
+      const tipDragListener = new RichDragListener( {
         tandem: options.tandem?.createTandem( 'tipDragListener' ),
 
         start: event => {
@@ -437,7 +439,7 @@
       } );
       tip.addInputListener( tipDragListener );
 
-      const tipKeyboardDragListener = new KeyboardDragListener( {
+      const tipKeyboardDragListener = new RichKeyboardDragListener( {
         tandem: options.tandem?.createTandem( 'tipKeyboardDragListener' ),
         positionProperty: this.tipPositionProperty,
         dragBoundsProperty: options.isTipDragBounded ? this.dragBoundsProperty : null,

[pixelzoom]

Adding options to be propagated to the 4 listeners would be a bit more work, especially given the odd definition of MeasuringTapeNodeOptions.keyboardDragListenerOptions.

Suggested improvement to options below. There is 1 usage of keyboardDragListenerOptions for MeasuringTapeNode, in keplers-laws SolarSystemCommonMeasuringTapeNode.

-  keyboardDragListenerOptions?: {
-    baseDragSpeed?: number;
-    baseShiftDragSpeed?: number;
-    tipDragSpeed?: number;
-    tipShiftDragSpeed?: number;
-  };
+  baseDragListenerOptions?: RichDragListenerOptions;
+  tipDragListenerOptions?: RichDragListenerOptions;
+  baseKeyboardDragListenerOptions?: RichKeyboardDragListenerOptions; 
+  tipKeyboardDragListenerOptions?: RichKeyboardDragListenerOptions; 

[pixelzoom]

Work completed in the above commits. @samreid would you mind taking a look? Reminder that this impacts phetsims/projectile-data-lab#270.

[samreid]

This worked well in my testing. In the code review I observed these options:

      baseKeyboardDragListenerOptions: {
        dragSpeed: KEYBOARD_DRAG_SPEED,
        shiftDragSpeed: KEYBOARD_DRAG_SPEED / 2
      },
      tipKeyboardDragListenerOptions: {
        dragSpeed: KEYBOARD_DRAG_SPEED,
        shiftDragSpeed: 150
      }

Should both shiftDragSpeed be KEYBOARD_DRAG_SPEED / 2? I also observed phetsims/scenery#1622 but that is being tracked separately.

[pixelzoom]

Should both shiftDragSpeed be KEYBOARD_DRAG_SPEED / 2?

These are the values that were in the code, I did not change them. I don't know the motivation for having a different default shiftDragSpeed for the base vs tip. You'll need to ask @AgustinVallejo, who set those values in f72cf3e. Unfortunately that commit does not reference a GitHub issue. I suspect that the values are specific to Kepler's Laws, which uses MeasuringTapeNode, and which @AgustinVallejo was working on around that time (7/18/23).

[samreid]

I'll self-unassign until we hear from @AgustinVallejo

[AgustinVallejo]

If I recall correctly, we lowered the default tip drag speed from KEYBOARD_DRAG_SPEED / 2 (300) to 150 for the user to have finer control over the tip. I'd say that for consistency it could be better for both tip and base to be KEYBOARD_DRAG_SPEED / 4. Is that reasonable? Let me know and I could make the change, or feel free as well. @samreid @pixelzoom

I could see reasons not to increase the tip's shiftDragSpeed, but decreasing the base's shiftDragSpeed seems a correct approach.

[pixelzoom]

I don't have a sim that is using MeasuringTapeNode, and I took this issue on as a courtesy when reviewing PDL, which has a MeasuringTapeNode. My task here was to add the grab/release sounds, not decide on whether/how to change the drag speeds, and I don't have time to get involved in that aspect. I think it would be more appropriate to sort this out for PDL, so assigning back to @samreid.

[AgustinVallejo]

Spoke with @samreid on Slack. He said:

1/4 seems better to me. I agree they should be consistent.

Above commit addresses this.

[pixelzoom]

I'll also note that this issue is about sounds, which are independent of drag speeds. So changing shiftDragSpeed feels like it should be in a new issue.

[samreid]

It looks like everything here is completed. Nice work @pixelzoom and @AgustinVallejo. Closing.