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 https://github.com/phetsims/scenery-phet/commit/21f6e5a1b20c3e55603acef35711dfbd0d0b1aad.

[pixelzoom]

Also noting that the root of this problem is actually in NumberDisplay, and is related to https://github.com/phetsims/scenery-phet/issues/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.

```diff 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.

```diff 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; - valueUnitsPattern?: string | TReadOnlyProperty; + // valueUnitsPattern?: string | TReadOnlyProperty; }; 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 " // See https://github.com/phetsims/collision-lab/issues/140. - return StringUtils.fillIn( options.valueUnitsPattern, { + return StringUtils.fillIn( SceneryPhetStrings.stopwatchValueUnitsPatternStringProperty, { value: `${minutesAndSeconds}${centiseconds}`, units: `${units}` } ); 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 | null; // If null, defaults to options.valuePattern + + additionalPropertiesForUpdate?: TReadOnlyProperty[]; }; export type NumberDisplayOptions = SelfOptions & StrictOmit; @@ -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 https://github.com/phetsims/scenery-phet/issues/814#issuecomment-1678814420 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 https://github.com/phetsims/scenery-phet/issues/824. Closing.