StopwatchNode units do not support dynamic locale

[AgustinVallejo]

Looking at keplers-laws/js/common/view/KeplersLawsScreenView.ts:284 the pattern is technically correct but when running the string tests, it still says 'years'. Not sure what to do about it

[pixelzoom]

Usage in KeplersLawsScreenView.ts is correct, see line 292:

      const stopwatchNode = new StopwatchNode(
      model.stopwatch, {
        dragBoundsProperty: this.visibleBoundsProperty,
        visibleProperty: model.stopwatchVisibleProperty,
        numberDisplayOptions: {
          numberFormatter: StopwatchNode.createRichTextNumberFormatter( {
            showAsMinutesAndSeconds: false,
            numberOfDecimalPlaces: 2,
292         units: KeplersLawsStrings.units.yearsStringProperty
          } )
        }
      }
    );

The problem is in StopwatchNode createRichTextNumberFormatter. It creates a formatter that only updates when the time value changes. When the stopwatch is running, you'll see "years" update properly. For example, running with `stringTest=dynamic" and starting the stopwatch, I see:

I wouldn't worry about this for Kepler's Law. The stopwatch units will update properly when the stopwatch starts running. Not great, but not blocking.

I'll transfer this issue to scenery-phet, and assign to @samreid -- it looks like he added this function in 21f6e5a.

[pixelzoom]

Also noting that the root of this problem is actually in NumberDisplay, and is related to #812. Imo NumberDisplay needs a rewrite. It has gotten too complicated, and it's difficult to support dynamic locale with the current API and implementation.

[samreid]

@matthew-blackman and I reviewed this patch in the context of the Sound simulation.

Subject: [PATCH] R
---
Index: js/NumberDisplay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/NumberDisplay.ts b/js/NumberDisplay.ts
--- a/js/NumberDisplay.ts	(revision 5a5256f83d89cecad948aee7d203d5ae0bf3a8eb)
+++ b/js/NumberDisplay.ts	(date 1692213571005)
@@ -22,6 +22,7 @@
 import PhetFont from './PhetFont.js';
 import sceneryPhet from './sceneryPhet.js';
 import DerivedStringProperty from '../../axon/js/DerivedStringProperty.js';
+import SceneryPhetStrings from './SceneryPhetStrings.js';
 
 // constants
 const DEFAULT_FONT = new PhetFont( 20 );
@@ -209,7 +210,8 @@
       numberProperty,
       noValuePatternProperty,
       valuePatternProperty,
-      numberFormatterProperty
+      numberFormatterProperty,
+      SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty
     ], ( value, noValuePattern, valuePatternValue, numberFormatter ) => {
       const valuePattern = ( value === null && noValuePattern ) ? noValuePattern : valuePatternValue;
       // NOTE: this.numberFormatter could change, so we support a recomputeText() below that recomputes this derivation

This experiment demonstrates that the stopwatch can be made to update the text on that string change, even when paused. However, baking this specific stopwatch-related string into NumberDisplay is not practical. I agree with @pixelzoom remarks above and not sure what, if anything, should be done in the meantime.

[samreid]

We realized this means the units wouldn't change when changing English to another language, so we investigated adding another dependency to trigger the update. This is working in the sound sim.

Subject: [PATCH] hello
---
Index: babel/_generated_development_strings/sound_all.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/babel/_generated_development_strings/sound_all.json b/babel/_generated_development_strings/sound_all.json
--- a/babel/_generated_development_strings/sound_all.json	(revision 55ecfede633a975340d4d7632c0f5832574a4231)
+++ b/babel/_generated_development_strings/sound_all.json	(date 1692214722485)
@@ -89,6 +89,9 @@
     },
     "frequency": {
       "value": "Frequency"
+    },
+    "milliseconds": {
+      "value": "ms"
     }
   }
 }
\ No newline at end of file
Index: sound/js/sound-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sound/js/sound-main.ts b/sound/js/sound-main.ts
--- a/sound/js/sound-main.ts	(revision 191076a39a91b2b44aa139d1025f3d1fe15b2744)
+++ b/sound/js/sound-main.ts	(date 1692214744969)
@@ -50,4 +50,8 @@
   } );
 
   sim.start();
+
+  setInterval( () => {
+    SoundStrings.millisecondsStringProperty.value = Date.now() + '';
+  }, 1000 );
 } );
\ No newline at end of file
Index: scenery-phet/js/StopwatchNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/StopwatchNode.ts b/scenery-phet/js/StopwatchNode.ts
--- a/scenery-phet/js/StopwatchNode.ts	(revision 5a5256f83d89cecad948aee7d203d5ae0bf3a8eb)
+++ b/scenery-phet/js/StopwatchNode.ts	(date 1692214434355)
@@ -70,7 +70,7 @@
   smallNumberFont?: number;
   unitsFont?: number;
   units?: string | TReadOnlyProperty<string>;
-  valueUnitsPattern?: string | TReadOnlyProperty<string>;
+  // valueUnitsPattern?: string | TReadOnlyProperty<string>;
 };
 
 export default class StopwatchNode extends InteractiveHighlighting( Node ) {
@@ -131,6 +131,7 @@
       xMargin: 8,
       yMargin: 8,
       numberDisplayOptions: {
+        // valuePattern: SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty,
         numberFormatter: StopwatchNode.RICH_TEXT_MINUTES_AND_SECONDS,
         useRichText: true,
         textOptions: {
@@ -140,7 +141,10 @@
         cornerRadius: 4,
         xMargin: 4,
         yMargin: 2,
-        pickable: false // allow dragging by the number display
+        pickable: false, // allow dragging by the number display
+        additionalPropertiesForUpdate: [
+          SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty
+        ]
       },
       dragBoundsProperty: null,
       dragListenerOptions: {
@@ -379,10 +383,7 @@
       bigNumberFont: 20,
       smallNumberFont: 14,
       unitsFont: 14,
-      units: '',
-
-      // Units cannot be baked into the i18n string because they can change independently
-      valueUnitsPattern: SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty
+      units: ''
     }, providedOptions );
 
     return ( time: number ) => {
@@ -392,7 +393,7 @@
 
       // Single quotes around CSS style so the double-quotes in the CSS font family work. Himalaya doesn't like &quot;
       // See https://github.com/phetsims/collision-lab/issues/140.
-      return StringUtils.fillIn( options.valueUnitsPattern, {
+      return StringUtils.fillIn( SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty, {
         value: `<span style='font-size: ${options.bigNumberFont}px; font-family:${StopwatchNode.NUMBER_FONT_FAMILY};'>${minutesAndSeconds}</span><span style='font-size: ${options.smallNumberFont}px;font-family:${StopwatchNode.NUMBER_FONT_FAMILY};'>${centiseconds}</span>`,
         units: `<span style='font-size: ${options.unitsFont}px; font-family:${StopwatchNode.NUMBER_FONT_FAMILY};'>${units}</span>`
       } );
Index: sound/js/measure/MeasureView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sound/js/measure/MeasureView.ts b/sound/js/measure/MeasureView.ts
--- a/sound/js/measure/MeasureView.ts	(revision 191076a39a91b2b44aa139d1025f3d1fe15b2744)
+++ b/sound/js/measure/MeasureView.ts	(date 1692214791899)
@@ -18,6 +18,7 @@
 import sound from '../sound.js';
 import MeasureModel from '../measure/MeasureModel.js';
 import SoundScreenView from '../common/view/SoundScreenView.js';
+import SoundStrings from '../SoundStrings.js';
 
 export default class MeasureView extends SoundScreenView {
   public constructor( model: MeasureModel ) {
@@ -50,15 +51,15 @@
     this.addChild( movableRuler );
 
     // Stopwatch
-    const createFormatter = ( units: string ) => StopwatchNode.createRichTextNumberFormatter( {
-      showAsMinutesAndSeconds: false,
-      units: units
-    } );
 
     const stopwatchNode = new StopwatchNode( model.stopwatch, {
       dragBoundsProperty: this.visibleBoundsProperty,
       numberDisplayOptions: {
-        numberFormatter: createFormatter( 'ms' )
+        numberFormatter: StopwatchNode.createRichTextNumberFormatter( {
+          showAsMinutesAndSeconds: false,
+          units: SoundStrings.millisecondsStringProperty
+        } ),
+        additionalPropertiesForUpdate: [ SoundStrings.millisecondsStringProperty ]
       },
       dragListenerOptions: {
         start: () => {
Index: sound/sound-strings_en.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sound/sound-strings_en.json b/sound/sound-strings_en.json
--- a/sound/sound-strings_en.json	(revision 191076a39a91b2b44aa139d1025f3d1fe15b2744)
+++ b/sound/sound-strings_en.json	(date 1692214708377)
@@ -88,5 +88,8 @@
   },
   "frequency": {
   	"value": "Frequency"
+  },
+  "milliseconds": {
+  	"value": "ms"
   }
 }
\ No newline at end of file
Index: keplers-laws/js/common/view/KeplersLawsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/keplers-laws/js/common/view/KeplersLawsScreenView.ts b/keplers-laws/js/common/view/KeplersLawsScreenView.ts
--- a/keplers-laws/js/common/view/KeplersLawsScreenView.ts	(revision e7fbab5d7f0f33dda454482ecf33d6ba78d3536a)
+++ b/keplers-laws/js/common/view/KeplersLawsScreenView.ts	(date 1692214187847)
@@ -289,7 +289,6 @@
           numberFormatter: StopwatchNode.createRichTextNumberFormatter( {
             showAsMinutesAndSeconds: false,
             numberOfDecimalPlaces: 2,
-            valueUnitsPattern: KeplersLawsStrings.pattern.valueUnitsStringProperty,
             units: KeplersLawsStrings.units.yearsStringProperty
           } )
         }
Index: sound/js/SoundStrings.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sound/js/SoundStrings.ts b/sound/js/SoundStrings.ts
--- a/sound/js/SoundStrings.ts	(revision 191076a39a91b2b44aa139d1025f3d1fe15b2744)
+++ b/sound/js/SoundStrings.ts	(date 1692214722454)
@@ -65,6 +65,7 @@
   };
   'amplitudeStringProperty': LocalizedStringProperty;
   'frequencyStringProperty': LocalizedStringProperty;
+  'millisecondsStringProperty': LocalizedStringProperty;
 };
 
 const SoundStrings = getStringModule( 'SOUND' ) as StringsType;
Index: scenery-phet/js/NumberDisplay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts
--- a/scenery-phet/js/NumberDisplay.ts	(revision 5a5256f83d89cecad948aee7d203d5ae0bf3a8eb)
+++ b/scenery-phet/js/NumberDisplay.ts	(date 1692214466666)
@@ -75,6 +75,8 @@
   noValueString?: string; // default is the PhET standard, do not override lightly.
   noValueAlign?: NumberDisplayAlign | null; // see ALIGN_VALUES. If null, defaults to options.align
   noValuePattern?: string | TReadOnlyProperty<string> | null; // If null, defaults to options.valuePattern
+
+  additionalPropertiesForUpdate?: TReadOnlyProperty<unknown>[];
 };
 export type NumberDisplayOptions = SelfOptions & StrictOmit<NodeOptions, 'children'>;
 
@@ -120,6 +122,8 @@
       noValueAlign: null,
       noValuePattern: null,
 
+      additionalPropertiesForUpdate: [],
+
       // phet-io
       phetioType: NumberDisplay.NumberDisplayIO
     }, providedOptions );
@@ -205,12 +209,19 @@
     // value
     const Constructor = options.useRichText ? RichText : Text;
     const valueTextTandem = options.tandem?.createTandem( 'valueText' );
-    const valueStringProperty = new DerivedStringProperty( [
+    const valueStringProperty = DerivedStringProperty.deriveAny( [
       numberProperty,
       noValuePatternProperty,
       valuePatternProperty,
-      numberFormatterProperty
-    ], ( value, noValuePattern, valuePatternValue, numberFormatter ) => {
+      numberFormatterProperty,
+      ...options.additionalPropertiesForUpdate
+    ], () => {
+
+      const value = numberProperty.value;
+      const noValuePattern = noValuePatternProperty.value;
+      const valuePatternValue = valuePatternProperty.value;
+      const numberFormatter = numberFormatterProperty.value;
+
       const valuePattern = ( value === null && noValuePattern ) ? noValuePattern : valuePatternValue;
       // NOTE: this.numberFormatter could change, so we support a recomputeText() below that recomputes this derivation
       const stringValue = valueToString( value, options.noValueString, numberFormatter );

[samreid]

@DianaTavares or @AgustinVallejo or @matthew-blackman can you clarify if this is required for Sound Waves or Kepler's Law?

[AgustinVallejo]

@samreid Not sure if 'needed' but there's definitely a Stopwatch Node in Kepler's.

[pixelzoom]

I've played around with changing the locale, and the stopwatch definitely feels a little confusing and janky. My opinion in #814 (comment) was that it's not blocking. But it's not my call.

[AgustinVallejo]

Agree with CM. This is not blocking in Kepler. Unassigning Diana and I.

[pixelzoom]

This was fixed by @samreid in #824. Closing.