Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to switch locales in a running sim #1302

Closed
samreid opened this issue Aug 11, 2022 · 33 comments
Closed

Make it possible to switch locales in a running sim #1302

samreid opened this issue Aug 11, 2022 · 33 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 11, 2022

As @arouinfar mentioned in phetsims/joist#744 (comment), we would like the simulation to be able to switch languages from within the sim.

Eventually the Localization Settings will include a UI control for users to select the sim locale within the all file.

And in https://github.com/phetsims/studio/issues/192#issuecomment-730570376, where @kathy-phet also said:

KP: Yes, changing language dynamically is highly desirable if we can accomplish it.

This came up during https://github.com/phetsims/studio/issues/268 where @zepumph and I discovered that the locale query parameter wouldn't work out of the box because the text strings are phetioState: true and hence get overwritten by the state object. Uninstrumenting from state would make them non-customizable.

So I wanted to explore a path for changing the locale from within the sim, now that we have a vision for dynamic layout in phetsims/joist#608

I found that including all languages into the Gravity and Orbits all HTML file only increased the size from 2.8MB to 3.9MB or so, and I am not too worried about that additional size. I used this build command, but had to output a non-debug _all version by a change in chipper:

grunt --brands=phet --locales=* --allHTML --lint=false

I experimented with an idea that getStringModule loads strings (for compatibility with existing code), but also loads a corresponding stringProperty (using a copy/paste preloadable TinyProperty). This means existing code will continue to work, but we can opt-in to a changeable string on a case-by-case basis. I tested this out with Gravity and Orbits, and was able to get several strings switching dynamically at runtime in both built and unbuilt mode.

Screen.Recording.2022-08-10.at.9.46.52.PM.mov

Here's the working copy experiment, which also changes the sim title.

Index: main/chipper/js/load-unbuilt-strings.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/chipper/js/load-unbuilt-strings.js b/main/chipper/js/load-unbuilt-strings.js
--- a/main/chipper/js/load-unbuilt-strings.js	(revision a63678534e0fa11d5154b3ffa8f8aa96e0e5fd09)
+++ b/main/chipper/js/load-unbuilt-strings.js	(date 1660188114399)
@@ -114,6 +114,8 @@
   const loadCustomLocale = customLocale && customLocale !== FALLBACK_LOCALE;
   const locales = [
     FALLBACK_LOCALE,
+    'es',
+    'ar',
     ...( loadCustomLocale ? [ customLocale ] : [] ), // e.g. 'zh_CN'
     ...( ( loadCustomLocale && customLocale.length > 2 && customLocale.slice( 0, 2 ) !== FALLBACK_LOCALE ) ? [ customLocale.slice( 0, 2 ) ] : [] ) // e.g. 'zh'
   ];
Index: main/chipper/js/grunt/getStringMap.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/chipper/js/grunt/getStringMap.js b/main/chipper/js/grunt/getStringMap.js
--- a/main/chipper/js/grunt/getStringMap.js	(revision a63678534e0fa11d5154b3ffa8f8aa96e0e5fd09)
+++ b/main/chipper/js/grunt/getStringMap.js	(date 1660187830557)
@@ -215,9 +215,10 @@
             }
           }
         }
-        assert( stringValue !== null, `Missing string information for ${repo} ${partialStringKey}` );
-
-        stringMap[ locale ][ `${requirejsNamespaceMap[ repo ]}/${partialStringKey}` ] = stringValue;
+        if ( !partialStringKey.endsWith( 'Property' ) ) {
+          assert( stringValue !== null, `Missing string information for ${repo} ${partialStringKey}` );
+          stringMap[ locale ][ `${requirejsNamespaceMap[ repo ]}/${partialStringKey}` ] = stringValue;
+        }
       } );
     } );
   } );
Index: main/joist/js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/joist/js/Sim.ts b/main/joist/js/Sim.ts
--- a/main/joist/js/Sim.ts	(revision 2d4bd8098beba3cb65613a546147662aff0da2af)
+++ b/main/joist/js/Sim.ts	(date 1660187744230)
@@ -261,7 +261,10 @@
    * @param allSimScreens - the possible screens for the sim in order of declaration (does not include the home screen)
    * @param [providedOptions] - see below for options
    */
-  public constructor( name: string, allSimScreens: Screen[], providedOptions?: SimOptions ) {
+  public constructor( nameProperty: string, allSimScreens: Screen[], providedOptions?: SimOptions ) {
+
+    // @ts-ignore
+    const name = nameProperty.value;
 
     window.phetSplashScreenDownloadComplete();
 
@@ -319,6 +322,10 @@
                            'and the title text on the home screen, if it exists.'
     } );
 
+
+    // @ts-ignore
+    nameProperty.link( name => { this.simNameProperty.value = name; } );
+
     // playbackModeEnabledProperty cannot be changed after Sim construction has begun, hence this listener is added before
     // anything else is done, see https://github.com/phetsims/phet-io/issues/1146
     phet.joist.playbackModeEnabledProperty.lazyLink( () => {
Index: main/gravity-and-orbits/js/gravity-and-orbits-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/gravity-and-orbits/js/gravity-and-orbits-main.ts b/main/gravity-and-orbits/js/gravity-and-orbits-main.ts
--- a/main/gravity-and-orbits/js/gravity-and-orbits-main.ts	(revision 33960849867f0ae2b9561565231dd1e825993a38)
+++ b/main/gravity-and-orbits/js/gravity-and-orbits-main.ts	(date 1660187885995)
@@ -18,7 +18,10 @@
 import ToScaleScreen from './toScale/ToScaleScreen.js';
 
 // @ts-ignore
-const gravityAndOrbitsTitleString = gravityAndOrbitsStrings[ 'gravity-and-orbits' ].title;
+gravityAndOrbitsStrings[ 'gravity-and-orbits' ].title;
+
+// @ts-ignore
+const gravityAndOrbitsTitleStringProperty = gravityAndOrbitsStrings[ 'gravity-and-orbits' ].titleProperty;
 
 simLauncher.launch( () => {
 
@@ -53,5 +56,5 @@
     backgroundColorProperty: backgroundColorProperty,
     tandem: Tandem.ROOT.createTandem( 'toScaleScreen' )
   } );
-  new Sim( gravityAndOrbitsTitleString, [ modelScreen, toScaleScreen ], simOptions ).start();
+  new Sim( gravityAndOrbitsTitleStringProperty, [ modelScreen, toScaleScreen ], simOptions ).start();
 } );
\ No newline at end of file
Index: main/gravity-and-orbits/js/common/view/GravityControl.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/gravity-and-orbits/js/common/view/GravityControl.ts b/main/gravity-and-orbits/js/common/view/GravityControl.ts
--- a/main/gravity-and-orbits/js/common/view/GravityControl.ts	(revision 33960849867f0ae2b9561565231dd1e825993a38)
+++ b/main/gravity-and-orbits/js/common/view/GravityControl.ts	(date 1660188512301)
@@ -43,8 +43,8 @@
     }, providedOptions );
 
     const gravityTextNode = new Text( gravityString, TEXT_OPTIONS );
-    const onTextNode = new Text( onString, TEXT_OPTIONS );
-    const offTextNode = new Text( offString, TEXT_OPTIONS );
+    const onTextNode = new Text( onString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.onProperty } );
+    const offTextNode = new Text( offString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.offProperty } );
 
     this.addLinkedElement( gravityEnabledProperty, {
       tandem: options.tandem.createTandem( 'gravityEnabledProperty' )
@@ -60,5 +60,10 @@
   }
 }
 
-gravityAndOrbits.register( 'GravityControl', GravityControl );
+gravityAndOrbits
+  .register(
+    'GravityControl'
+    ,
+    GravityControl
+  );
 export default GravityControl;
\ No newline at end of file
Index: main/scenery-phet/js/TimeSpeedRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/scenery-phet/js/TimeSpeedRadioButtonGroup.ts b/main/scenery-phet/js/TimeSpeedRadioButtonGroup.ts
--- a/main/scenery-phet/js/TimeSpeedRadioButtonGroup.ts	(revision c4e25406c328bdd0eba2df0e2abdf7b7e5018950)
+++ b/main/scenery-phet/js/TimeSpeedRadioButtonGroup.ts	(date 1660188365027)
@@ -22,9 +22,9 @@
 
 // maps TimeSpeed to its label and Tandem name
 const SPEED_LABEL_MAP = new Map();
-SPEED_LABEL_MAP.set( TimeSpeed.FAST, { labelString: sceneryPhetStrings.speed.fast, tandemName: 'fastRadioButton' } );
-SPEED_LABEL_MAP.set( TimeSpeed.NORMAL, { labelString: sceneryPhetStrings.speed.normal, tandemName: 'normalRadioButton' } );
-SPEED_LABEL_MAP.set( TimeSpeed.SLOW, { labelString: sceneryPhetStrings.speed.slow, tandemName: 'slowRadioButton' } );
+SPEED_LABEL_MAP.set( TimeSpeed.FAST, { textProperty: sceneryPhetStrings.speed.fastProperty, labelString: sceneryPhetStrings.speed.fast, tandemName: 'fastRadioButton' } );
+SPEED_LABEL_MAP.set( TimeSpeed.NORMAL, { textProperty: sceneryPhetStrings.speed.normalProperty, labelString: sceneryPhetStrings.speed.normal, tandemName: 'normalRadioButton' } );
+SPEED_LABEL_MAP.set( TimeSpeed.SLOW, { textProperty: sceneryPhetStrings.speed.slowProperty, labelString: sceneryPhetStrings.speed.slow, tandemName: 'slowRadioButton' } );
 
 type SelfOptions = {
   radius?: number;
@@ -69,8 +69,13 @@
 
     const items = timeSpeeds.map( speed => {
 
+      SPEED_LABEL_MAP.get( speed ).textProperty.link( speed );
+
       const speedLabel = SPEED_LABEL_MAP.get( speed ).labelString;
-      const labelNode = new Text( speedLabel, options.labelOptions );
+      const labelNode = new Text( speedLabel, {
+        ...options.labelOptions,
+        textProperty: SPEED_LABEL_MAP.get( speed ).textProperty;
+      } );
 
       return {
         value: speed,
Index: main/chipper/js/getStringModule.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/chipper/js/getStringModule.js b/main/chipper/js/getStringModule.js
--- a/main/chipper/js/getStringModule.js	(revision a63678534e0fa11d5154b3ffa8f8aa96e0e5fd09)
+++ b/main/chipper/js/getStringModule.js	(date 1660188479012)
@@ -16,7 +16,7 @@
 
 /**
  * @param {string} requirejsNamespace - E.g. 'JOIST', to pull string keys out from that namespace
- * @returns {Object} - Nested object to be accessed like joistStrings.ResetAllButton.name
+ * @returns {Object} - Nested object to be accessed like joistStrings.ResetAllButton.name.value
  */
 const getStringModule = requirejsNamespace => {
   // Our string information is pulled globally, e.g. phet.chipper.strings[ locale ][ stringKey ] = stringValue;
@@ -98,7 +98,51 @@
         // In case our assertions are not enabled, we'll need to proceed without failing out (so we allow for the
         // extended string keys in our actual code, even though assertions should prevent that).
         if ( typeof reference !== 'string' ) {
-          reference[ lastKeyPart ] = phet.chipper.mapString( partialStringMap[ stringKey ] );
+
+          class TinyProperty {
+
+            constructor( value ) {
+              this.value = value;
+              this.listeners = [];
+            }
+
+            link( listener ) {
+              this.listeners.push( listener );
+              listener( this.value );
+            }
+
+            isSettable(){
+              return true;
+            }
+
+            set(value){
+              this.value = value;
+            }
+
+            lazyLink( listener ) {
+              this.listeners.push( listener );
+            }
+          }
+
+          const tp = new TinyProperty( phet.chipper.mapString( partialStringMap[ stringKey ] ) );
+          reference[ lastKeyPart ] = tp.value;
+          reference[ lastKeyPart + 'Property' ] = tp;
+
+          const locales = [ 'en', 'es', 'ar' ];
+
+          let localeIndex = 0;
+
+          setInterval( () => {
+
+            localeIndex++;
+            const locale = locales[ localeIndex % locales.length ];
+            //
+            const partialStringMap = phet.chipper.strings[ locale ];
+            const value = phet.chipper.mapString( partialStringMap[ stringKey ] ) || '?';
+
+            tp.value = value;
+            tp.listeners.forEach( listener => listener( tp.value ) );
+          }, 1000 );
         }
       } );
 
Index: main/gravity-and-orbits/js/common/view/CheckboxPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/gravity-and-orbits/js/common/view/CheckboxPanel.ts b/main/gravity-and-orbits/js/common/view/CheckboxPanel.ts
--- a/main/gravity-and-orbits/js/common/view/CheckboxPanel.ts	(revision 33960849867f0ae2b9561565231dd1e825993a38)
+++ b/main/gravity-and-orbits/js/common/view/CheckboxPanel.ts	(date 1660188660811)
@@ -71,12 +71,12 @@
     const children = [];
     const options = combineOptions<CheckboxPanelOptions>( { tandem: Tandem.OPTIONAL }, providedOptions );
 
-    const gravityForceTextNode = new Text( gravityForceString, TEXT_OPTIONS );
-    const velocityTextNode = new Text( velocityString, TEXT_OPTIONS );
-    const massTextNode = new Text( massString, TEXT_OPTIONS );
-    const pathTextNode = new Text( pathString, TEXT_OPTIONS );
-    const gridTextNode = new Text( gridString, TEXT_OPTIONS );
-    const measuringTapeTextNode = new Text( measuringTapeString, TEXT_OPTIONS );
+    const gravityForceTextNode = new Text( gravityForceString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.gravityForceProperty } );
+    const velocityTextNode = new Text( velocityString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.velocityProperty } );
+    const massTextNode = new Text( massString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.massProperty } );
+    const pathTextNode = new Text( pathString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.pathProperty } );
+    const gridTextNode = new Text( gridString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.gridProperty } );
+    const measuringTapeTextNode = new Text( measuringTapeString, { ...TEXT_OPTIONS, textProperty: gravityAndOrbitsStrings.measuringTapeProperty } );
     const optionsWithTandem = ( tandemName: string ) => merge( { tandem: options.tandem!.createTandem( tandemName ) }, CHECKBOX_OPTIONS );
 
     // gravity force checkbox

Note there are several lint issues, type issues, etc in this experiment--just wanted to explore if this is where we want to be. Also, I created this issue here in chipper since that is where the strings are loaded and processed, even though this could have a Joist UI and overlap with a11y preferences and phet-io customizability.

Also heads up to @zepumph and @jessegreenberg and @pixelzoom about this idea.

UPDATE: Also heads up to @jbphet and @liam-mulhall since they work on rosetta.

@jonathanolson
Copy link
Contributor

Not done, but committed some progress (Molecule Shapes sim-specific is completely dynamic now, a lot of Gravity and Orbits is, but need to work on some details and some common interface).

phetioValidation=false is needed for now, due to overrides file errors like:

moleculeShapes.general.view.navigationBar.phetButton.phetMenu.optionsDialogCapsule.archetype.content.showOuterLonePairsCheckbox.labelText.textProperty:  3. Any schema entries in the overrides file must exist in the baseline file.

Example is http://localhost/studio/?sim=molecule-shapes&phetioWrapperDebug=true&phetioElementsDisplay=all&loadExtraLocales=es,ar_SA,de,cs&phetioValidation=false

I added the loadExtraLocales query parameter (for unbuilt sims, since they by default only load the NEEDED locales).

SIM.general.view.localeProperty controls the locale. Strings are in the studio interface, but it lags the studio interface a bit (even with only that many locales).

@jonathanolson
Copy link
Contributor

Added a VERY rough-draft locale switcher in the navbar for demo-ing, see https://bayes.colorado.edu/dev/olsonjb/locale-test.html

@samreid
Copy link
Member Author

samreid commented Aug 13, 2022

That draft looks really amazing, nice work!

@samreid samreid removed their assignment Aug 13, 2022
@jonathanolson
Copy link
Contributor

Includes some layout/helper improvements that I'll port to master in the commits above.

zepumph added a commit that referenced this issue Sep 6, 2022
zepumph added a commit that referenced this issue Sep 6, 2022
zepumph added a commit that referenced this issue Sep 6, 2022
zepumph added a commit that referenced this issue Sep 6, 2022
jbphet pushed a commit to phetsims/greenhouse-effect that referenced this issue Sep 10, 2022
jbphet pushed a commit to phetsims/greenhouse-effect that referenced this issue Sep 10, 2022
…zedString (with improved fallbacks), adding fallbackLocalesProperty/localeOrderProperty, see phetsims/chipper#1302
@jonathanolson
Copy link
Contributor

It looks like all commits from stringProperties branches have been merged to master. Can we please delete the branches and close the issues?

Branches deleted.

@jonathanolson
Copy link
Contributor

window.phet.chipper.mapString now seems to take a TReadOnlyProperty | string, can we update the documentation.

Looking at the implementation and usages, I don't see how that's true, thoughts?

@jonathanolson
Copy link
Contributor

There is a bug or at least weirdness in this code in localeOrderProperty. Given localeProperty: de and fallbackLocalesProperty set to [ar_MA, es], the localeOrder becomes ['de', 'ar_MA', 'es', 'ar', 'en']. Should we order the aribic locales together?

I have a preference for all the exact locales first, then fallbacks (since ar could have translations that might not be understood by someone who knwos ar_MA and es)

@jonathanolson
Copy link
Contributor

@zepumph can you review the above and my commits? I believe I've handled or responded to everything.

@zepumph
Copy link
Member

zepumph commented Sep 13, 2022

Can you please say a sentence about why we were using jsondiffpatch, even though we didn't end up going with that strategy?

I have a preference for all the exact locales first, then fallbacks (since ar could have translations that might not be understood by someone who knwos ar_MA and es)

Makes a lot of sense.

I like the documentation! Thanks again for all this awesome work. What progress for PhET's codebase! Fee free to close.

@zepumph zepumph assigned jonathanolson and unassigned zepumph Sep 13, 2022
@samreid
Copy link
Member Author

samreid commented Sep 14, 2022

I don't think the remaining questions are blocking RC's for Gravity and Orbits, so I'll update this on the project board.

@jonathanolson
Copy link
Contributor

Can you please say a sentence about why we were using jsondiffpatch, even though we didn't end up going with that strategy?

In a case where we had a large JSON structure of "original string state" and "string state after", it was useful to diff the entire structure. Later once we had each LocalizedString with its individual knowledge, that wasn't necessary.

samreid added a commit that referenced this issue Jan 18, 2023
zepumph added a commit that referenced this issue Jan 19, 2023
zepumph pushed a commit to phetsims/perennial that referenced this issue Oct 22, 2024
…ching to --locales=*, supporting string Properties in Text/RichText constructors, adding jsondiffpatch for saving string state deltas in phet-io state see phetsims/chipper#1302
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants