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

More locale work: localeProperty should be graceful and more central #970

Closed
zepumph opened this issue Jun 10, 2024 · 14 comments
Closed

More locale work: localeProperty should be graceful and more central #970

zepumph opened this issue Jun 10, 2024 · 14 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jun 10, 2024

From #963 and phetsims/chipper#1441, lots of changes occurred to the locale system (like support for localeData in main/babel, locale3, hyphens and case-insensitivity), but we didn't get things working for Standard PhET-iO Wrappers to pass through the locale query parameter. This was reported in https://github.com/phetsims/phet-io/issues/1881#issuecomment-2142674672.

From this @jonathanolson @samreid and I have a new plan for how to support this. We want localeProperty to support setting values that are identical to any value you can provide via the query parameter. This means using the same logic as is in initialize-globals' checkAndRemapLocale().

Beginning work can be found here: https://github.com/phetsims/phet-io/issues/1881#issuecomment-2155281836 and below.

@zepumph
Copy link
Member Author

zepumph commented Jun 10, 2024

This patch is much more put together, and I think we are close to a commit point. This is the change set meant for main, and not to be MR'd to all hydrogen sims.

Subject: [PATCH] better debug-info support for strictAxonDependencies, https://github.com/phetsims/aqua/issues/212
---
Index: chipper/js/initialize-globals.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/initialize-globals.js b/chipper/js/initialize-globals.js
--- a/chipper/js/initialize-globals.js	(revision 739e543ddef46bcda67e058ed0e0e2a422142fbd)
+++ b/chipper/js/initialize-globals.js	(date 1718060867163)
@@ -34,22 +34,28 @@
 
   assert && assert( window.QueryStringMachine, 'QueryStringMachine is used, and should be loaded before this code runs' );
 
+  // Create the attachment point for all PhET globals
+  window.phet = window.phet ?? {};
+  window.phet.chipper = window.phet.chipper ?? {};
+
   // packageObject may not always be available if initialize-globals used without chipper-initialization.js
-  const packageObject = _.hasIn( window, 'phet.chipper.packageObject' ) ? phet.chipper.packageObject : {};
-  const packagePhet = packageObject.phet || {};
+  const packageObject = phet.chipper.packageObject ?? {};
+  const packagePhet = packageObject.phet ?? {};
 
   // Not all runtimes will have this flag, so be graceful
-  const allowLocaleSwitching = _.hasIn( window, 'phet.chipper.allowLocaleSwitching' ) ? phet.chipper.allowLocaleSwitching : true;
+  const allowLocaleSwitching = phet.chipper.allowLocaleSwitching ?? true;
 
   // duck type defaults so that not all package.json files need to have a phet.simFeatures section.
-  const packageSimFeatures = packagePhet.simFeatures || {};
+  const packageSimFeatures = packagePhet.simFeatures ?? {};
 
   // The color profile used by default, if no colorProfiles are specified in package.json.
   // NOTE: Duplicated in SceneryConstants.js since scenery does not include initialize-globals.js
   const DEFAULT_COLOR_PROFILE = 'default';
 
+  const FALLBACK_LOCALE = 'en';
+
   // The possible color profiles for the current simulation.
-  const colorProfiles = packageSimFeatures.colorProfiles || [ DEFAULT_COLOR_PROFILE ];
+  const colorProfiles = packageSimFeatures.colorProfiles ?? [ DEFAULT_COLOR_PROFILE ];
 
   // Private Doc: Note: the following jsdoc is for the public facing PhET-iO API. In addition, all query parameters in the schema
   // that are a "memberOf" the "PhetQueryParameters" namespace are used in the jsdoc that is public (client facing)
@@ -323,7 +329,7 @@
      */
     locale: {
       type: 'string',
-      defaultValue: 'en'
+      defaultValue: window.phet.chipper.locale ?? FALLBACK_LOCALE
       // Do NOT add the `public` key here. We want invalid values to fall back to en.
     },
 
@@ -956,12 +962,8 @@
     }
   };
 
-  // Initialize query parameters, see docs above
-  ( function() {
-
-    // Create the attachment point for all PhET globals
-    window.phet = window.phet || {};
-    window.phet.chipper = window.phet.chipper || {};
+  // Initialize query parameters in a new scope, see docs above
+  {
 
     // Read query parameters
     window.phet.chipper.queryParameters = QueryStringMachine.getAll( QUERY_PARAMETERS_SCHEMA );
@@ -1028,70 +1030,119 @@
              stringTest;
     };
 
-    // We will need to check for locale validity (once we have localeData loaded, if running unbuilt), and potentially
-    // either fall back to `en`, or remap from 3-character locales to our locale keys.
-    phet.chipper.checkAndRemapLocale = () => {
-      // We need both to proceed. Provided as a global, so we can call it from load-unbuilt-strings
-      // (IF initialize-globals loads first)
-      if ( !phet.chipper.localeData || !phet.chipper.locale ) {
-        return;
-      }
+    phet.chipper.remapLocale = ( locale, assertInsteadOfWarn = false ) => {
+      assert && assert( locale );
+      assert && assert( phet.chipper.localeData );
 
-      let locale = phet.chipper.locale;
+      const inputValueLocale = locale;
 
-      if ( locale ) {
-        if ( locale.length < 5 ) {
-          locale = locale.toLowerCase();
-        }
-        else {
-          locale = locale.replace( /-/, '_' );
+      if ( locale.length < 5 ) {
+        locale = locale.toLowerCase();
+      }
+      else {
+        locale = locale.replace( /-/, '_' );
 
-          const parts = locale.split( '_' );
-          if ( parts.length === 2 ) {
-            locale = parts[ 0 ].toLowerCase() + '_' + parts[ 1 ].toUpperCase();
-          }
-        }
+        const parts = locale.split( '_' );
+        if ( parts.length === 2 ) {
+          locale = parts[ 0 ].toLowerCase() + '_' + parts[ 1 ].toUpperCase();
+        }
+      }
 
-        if ( locale.length === 3 ) {
-          for ( const candidateLocale of Object.keys( phet.chipper.localeData ) ) {
-            if ( phet.chipper.localeData[ candidateLocale ].locale3 === locale ) {
-              locale = candidateLocale;
-              break;
+      if ( locale.length === 3 ) {
+        for ( const candidateLocale of Object.keys( phet.chipper.localeData ) ) {
+          if ( phet.chipper.localeData[ candidateLocale ].locale3 === locale ) {
+            locale = candidateLocale;
+            break;
+          }
+        }
+      }
+
+      // Permissive patterns for locale query parameter patterns.
+      // We don't want to show a query parameter warning if it matches these patterns, EVEN if it is not a valid locale
+      // in localeData, see https://github.com/phetsims/qa/issues/1085#issuecomment-2111105235.
+      const pairRegex = /^[a-zA-Z]{2}$/;
+      const tripleRegex = /^[a-zA-Z]{3}$/;
+      const doublePairRegex = /^[a-zA-Z]{2}[_-][a-zA-Z]{2}$/;
+
+      // Sanity checks for verifying localeData (so hopefully we don't commit bad data to localeData).
+      if ( assert ) {
+        for ( const locale of Object.keys( phet.chipper.localeData ) ) {
+          // Check the locale itself
+          assert( pairRegex.test( locale ) || doublePairRegex.test( locale ), `Invalid locale format: ${locale}` );
+
+          // Check locale3 (if it exists)
+          if ( phet.chipper.localeData[ locale ].locale3 ) {
+            assert( tripleRegex.test( phet.chipper.localeData[ locale ].locale3 ), `Invalid locale3 format: ${phet.chipper.localeData[ locale ].locale3}` );
+          }
+
+          // Check fallbackLocales (if it exists)
+          if ( phet.chipper.localeData[ locale ].fallbackLocales ) {
+            for ( const fallbackLocale of phet.chipper.localeData[ locale ].fallbackLocales ) {
+              assert( phet.chipper.localeData[ fallbackLocale ] );
             }
           }
         }
       }
 
       if ( !phet.chipper.localeData[ locale ] ) {
-        const badLocale = phet.chipper.queryParameters.locale;
+        const badLocale = inputValueLocale;
 
-        // Be permissive with case for the query parameter warning, see https://github.com/phetsims/qa/issues/1085#issuecomment-2111105235
-        const isPair = /^[a-zA-Z]{2}$/.test( badLocale );
-        const isTriple = /^[a-zA-Z]{3}$/.test( badLocale );
-        const isPair_PAIR = /^[a-zA-Z]{2}[_-][a-zA-Z]{2}$/.test( badLocale );
+        if ( !pairRegex.test( badLocale ) && !tripleRegex.test( badLocale ) && !doublePairRegex.test( badLocale ) ) {
+          if ( assertInsteadOfWarn ) {
+            assert && assert( false, 'invalid locale:', inputValueLocale );
+          }
+          else {
+            QueryStringMachine.addWarning( 'locale', inputValueLocale, `Invalid locale format received: ${badLocale}. ?locale query parameter accepts the following formats: "xx" for ISO-639-1, "xx_XX" for ISO-639-1 and a 2-letter country code, "xxx" for ISO-639-2` );
+          }
+        }
 
-        if ( !isPair && !isTriple && !isPair_PAIR ) {
-          QueryStringMachine.addWarning( 'locale', phet.chipper.queryParameters.locale, `Invalid locale format received: ${badLocale}. ?locale query parameter accepts the following formats: "xx" for ISO-639-1, "xx_XX" for ISO-639-1 and a 2-letter country code, "xxx" for ISO-639-2` );
-        }
+        locale = FALLBACK_LOCALE;
+      }
 
-        locale = 'en';
+      return locale;
+    };
+
+    // Get the "most" valid locale, see https://github.com/phetsims/phet-io/issues/1882
+    // As part of https://github.com/phetsims/joist/issues/963, this as changed. We check a specific fallback order based
+    // on the locale. In general, it will usually try a prefix for xx_XX style locales, e.g. 'ar_SA' would try 'ar_SA', 'ar', 'en'
+    // NOTE: If the locale doesn't actually have any strings: THAT IS OK! Our string system will use the appropriate
+    // fallback strings.
+    phet.chipper.getValidRuntimeLocale = locale => {
+      const possibleLocales = [
+        locale,
+        ...( phet.chipper.localeData[ locale ]?.fallbackLocales ?? [] ),
+        FALLBACK_LOCALE
+      ];
+
+      return possibleLocales.find( possibleLocale => !!phet.chipper.strings[ possibleLocale ] );
+    };
+
+    // We will need to check for locale validity (once we have localeData loaded, if running unbuilt), and potentially
+    // either fall back to `en`, or remap from 3-character locales to our locale keys. This overwrites phet.chipper.locale.
+    // Used when setting locale through JOIST/localeProperty also.
+    phet.chipper.checkAndRemapLocale = ( locale = phet.chipper.locale, assertInsteadOfWarn = false ) => {
+      // We need both to proceed. Provided as a global, so we can call it from load-unbuilt-strings
+      // (IF initialize-globals loads first)
+      if ( !phet.chipper.localeData || !phet.chipper.strings || !locale ) {
+        return locale;
       }
 
-      phet.chipper.locale = locale;
+      const remappedLocale = phet.chipper.remapLocale( locale, assertInsteadOfWarn );
+      const finalLocale = phet.chipper.getValidRuntimeLocale( remappedLocale );
+
+      // Export this for analytics, see gogole-analytics.js
+      // (Yotta and GA will want the non-fallback locale for now, for consistency)
+      phet.chipper.remappedLocale = remappedLocale;
+      phet.chipper.locale = finalLocale; // NOTE: this will change with every setting of JOIST/localeProperty
+      return finalLocale;
     };
 
-    // If locale was provided as a query parameter, then change the locale used by Google Analytics.
-    if ( QueryStringMachine.containsKey( 'locale' ) ) {
-      phet.chipper.locale = phet.chipper.queryParameters.locale;
+    // Query parameter default will pick up the phet.chipper.locale default from the built sim, if it exists.
+    phet.chipper.locale = phet.chipper.queryParameters.locale;
 
-      // NOTE: If we are loading in unbuilt mode, this may execute BEFORE we have loaded localeData. We have a similar
-      // remapping in load-unbuilt-strings when this happens.
-      phet.chipper.checkAndRemapLocale();
-    }
-    else if ( !window.phet.chipper.locale ) {
-      // Fill in a default
-      window.phet.chipper.locale = 'en';
-    }
+    // NOTE: If we are loading in unbuilt mode, this may execute BEFORE we have loaded localeData. We have a similar
+    // remapping in load-unbuilt-strings when this happens.
+    phet.chipper.checkAndRemapLocale();
 
     const stringOverrides = JSON.parse( phet.chipper.queryParameters.strings || '{}' );
 
@@ -1115,7 +1166,7 @@
       const fallbackLocales = [
         phet.chipper.locale,
         ...( phet.chipper.localeData[ phet.chipper.locale ]?.fallbackLocales || [] ),
-        ( phet.chipper.locale !== 'en' ? [ 'en' ] : [] )
+        ( phet.chipper.locale !== FALLBACK_LOCALE ? [ FALLBACK_LOCALE ] : [] )
       ];
 
       let stringMap = null;
@@ -1129,7 +1180,7 @@
 
       return phet.chipper.mapString( stringMap[ key ] );
     };
-  }() );
+  }
 
   /**
    * Utility function to pause synchronously for the given number of milliseconds.
Index: number-suite-common/js/common/view/LanguageAndVoiceControl.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/number-suite-common/js/common/view/LanguageAndVoiceControl.ts b/number-suite-common/js/common/view/LanguageAndVoiceControl.ts
--- a/number-suite-common/js/common/view/LanguageAndVoiceControl.ts	(revision 2c829f16f30d1d4869786781bc5bde00159d39d0)
+++ b/number-suite-common/js/common/view/LanguageAndVoiceControl.ts	(date 1718045176740)
@@ -11,8 +11,7 @@
 import numberSuiteCommon from '../../numberSuiteCommon.js';
 import { Color, HBox, HBoxOptions, ManualConstraint, Node, RichText, RichTextOptions, Text, TextOptions, VBox } from '../../../../scenery/js/imports.js';
 import optionize, { combineOptions, EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
-import { Locale } from '../../../../joist/js/i18n/localeProperty.js';
-import Property from '../../../../axon/js/Property.js';
+import { Locale, LocaleProperty } from '../../../../joist/js/i18n/localeProperty.js';
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
 import Carousel, { CarouselItem, CarouselOptions } from '../../../../sun/js/Carousel.js';
 import TProperty from '../../../../axon/js/TProperty.js';
@@ -45,7 +44,7 @@
 
 export default class LanguageAndVoiceControl extends HBox {
 
-  public constructor( localeProperty: Property<Locale>,
+  public constructor( localeProperty: LocaleProperty,
                       voiceProperty: TProperty<SpeechSynthesisVoice | null>,
                       utteranceQueue: NumberSuiteCommonUtteranceQueue,
                       providedOptions?: LanguageAndVoiceControlOptions ) {
@@ -60,7 +59,7 @@
     }, providedOptions );
 
     // Carousel for choosing a language.
-    const languageCarouselItems: LanguageCarouselItem[] = localeProperty.validValues!.map(
+    const languageCarouselItems: LanguageCarouselItem[] = localeProperty.availableRuntimeLocales.map(
       locale => {
         return {
           locale: locale,
Index: chipper/js/LocalizedStringProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/LocalizedStringProperty.ts b/chipper/js/LocalizedStringProperty.ts
--- a/chipper/js/LocalizedStringProperty.ts	(revision 739e543ddef46bcda67e058ed0e0e2a422142fbd)
+++ b/chipper/js/LocalizedStringProperty.ts	(date 1718060867146)
@@ -31,7 +31,7 @@
 
     super( localeProperty, {
       derive: ( locale: Locale ) => localizedString.getLocaleSpecificProperty( locale ),
-      bidirectional: true,
+      bidirectional: true, // TODO: Why is this allowed to change the localeProperty, https://github.com/phetsims/joist/issues/970
       phetioValueType: StringIO,
       phetioState: false,
       tandem: tandem,
Index: chipper/js/load-unbuilt-strings.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/load-unbuilt-strings.js b/chipper/js/load-unbuilt-strings.js
--- a/chipper/js/load-unbuilt-strings.js	(revision 739e543ddef46bcda67e058ed0e0e2a422142fbd)
+++ b/chipper/js/load-unbuilt-strings.js	(date 1718060867151)
@@ -177,6 +177,10 @@
   // The callback to execute when all string files are processed.
   const finishProcessing = () => {
 
+    // Because load-unbuilt-strings' "loading" of the locale data and strings might not have happened BEFORE initialize-globals
+    // runs (and sets phet.chipper.locale), we'll attempt to handle the case where it hasn't been set yet. You'll see the same call over in initialize-globals
+    phet.chipper.checkAndRemapLocale && phet.chipper.checkAndRemapLocale();
+
     // Progress with loading modules
     window.phet.chipper.loadModules();
   };
@@ -207,10 +211,6 @@
   requestJSONFile( '../babel/localeData.json', json => {
     phet.chipper.localeData = json;
 
-    // Because load-unbuilt-strings' "loading" of the locale data might not have happened BEFORE initialize-globals
-    // runs (and sets phet.chipper.locale), we'll attempt to handle the case where it hasn't been set yet.
-    phet.chipper.checkAndRemapLocale && phet.chipper.checkAndRemapLocale();
-
     rtlLocales = Object.keys( phet.chipper.localeData ).filter( locale => {
       return phet.chipper.localeData[ locale ].direction === 'rtl';
     } );
Index: phetcommon/js/analytics/google-analytics.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phetcommon/js/analytics/google-analytics.js b/phetcommon/js/analytics/google-analytics.js
--- a/phetcommon/js/analytics/google-analytics.js	(revision 3d8102ba5a66bbe637793cb79ca1a0b52d7f497f)
+++ b/phetcommon/js/analytics/google-analytics.js	(date 1718045176744)
@@ -15,13 +15,15 @@
     return;
   }
 
+  const locale = phet.chipper.remappedLocale;
+
   assert && assert( window.phet && phet.chipper, 'We will require multiple things from the chipper preload namespace' );
   assert && assert( !!phet.chipper.brand, 'A brand is required, since some messages depend on the brand' );
   assert && assert( !!phet.chipper.queryParameters, 'We will need query parameters to be parsed for multiple purposes' );
   assert && assert( !!phet.chipper.buildTimestamp, 'buildTimestamp is required for GA messages' );
   assert && assert( !!phet.chipper.project, 'project is required for GA messages' );
   assert && assert( !!phet.chipper.version, 'version is required for GA messages' );
-  assert && assert( !!phet.chipper.locale, 'locale is required for GA messages' );
+  assert && assert( !!locale, 'locale is required for GA messages' );
 
   const ua = navigator.userAgent;
   const hasIESecurityRestrictions = !!( ua.match( /MSIE/ ) || ua.match( /Trident\// ) || ua.match( /Edge\// ) );
@@ -73,7 +75,7 @@
       'project='}${encodeURIComponent( phet.chipper.project )}&` +
       `brand=${encodeURIComponent( phet.chipper.brand )}&` +
       `version=${encodeURIComponent( phet.chipper.version )}&` +
-      `locale=${encodeURIComponent( phet.chipper.locale )}&` +
+      `locale=${encodeURIComponent( locale )}&` +
       `buildTimestamp=${encodeURIComponent( phet.chipper.buildTimestamp )}&` +
       `domain=${encodeURIComponent( document.domain )}&` +
       `href=${encodeURIComponent( window.location.href )}&` +
@@ -154,7 +156,7 @@
       simBrand: phet.chipper.brand,
       simName: phet.chipper.project,
       simVersion: phet.chipper.version,
-      simLocale: phet.chipper.locale,
+      simLocale: locale,
       simBuildTimestamp: phet.chipper.buildTimestamp,
       simLoadType: loadType,
       documentReferrer: document.referrer
Index: joist/js/preferences/LocalePanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/LocalePanel.ts b/joist/js/preferences/LocalePanel.ts
--- a/joist/js/preferences/LocalePanel.ts	(revision ff8657b06963957a9cd45db83fcb83690268c19e)
+++ b/joist/js/preferences/LocalePanel.ts	(date 1718045176725)
@@ -13,16 +13,15 @@
 import joist from '../joist.js';
 import Panel from '../../../sun/js/Panel.js';
 import { GridBox } from '../../../scenery/js/imports.js';
-import Property from '../../../axon/js/Property.js';
 import LanguageSelectionNode from './LanguageSelectionNode.js';
-import { Locale } from '../i18n/localeProperty.js';
 import JoistStrings from '../JoistStrings.js';
 import StringUtils from '../../../phetcommon/js/util/StringUtils.js';
+import { LocaleProperty } from '../i18n/localeProperty.js';
 
 class LocalePanel extends Panel {
-  public constructor( localeProperty: Property<Locale> ) {
+  public constructor( localeProperty: LocaleProperty ) {
 
-    const locales = localeProperty.validValues!;
+    const locales = localeProperty.availableRuntimeLocales;
 
     // Sort these properly by their localized name (without using _.sortBy, since string comparison does not provide
     // a good sorting experience). See https://github.com/phetsims/joist/issues/965
Index: joist/js/i18n/localeProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/i18n/localeProperty.ts b/joist/js/i18n/localeProperty.ts
--- a/joist/js/i18n/localeProperty.ts	(revision ff8657b06963957a9cd45db83fcb83690268c19e)
+++ b/joist/js/i18n/localeProperty.ts	(date 1718060867168)
@@ -13,53 +13,45 @@
 import Tandem from '../../../tandem/js/Tandem.js';
 import StringIO from '../../../tandem/js/types/StringIO.js';
 import joist from '../joist.js';
-
-const FALLBACK_LOCALE = 'en';
+import { ReadOnlyPropertyState } from '../../../axon/js/ReadOnlyProperty.js';
 
 export type Locale = keyof typeof localeInfoModule;
 
+assert && assert( phet.chipper.locale );
+assert && assert( phet.chipper.localeData );
+assert && assert( phet.chipper.strings );
+
 // All available locales for the runtime
-export const availableRuntimeLocales = _.sortBy( Object.keys( phet.chipper.strings ), locale => {
+const availableRuntimeLocales = _.sortBy( Object.keys( phet.chipper.strings ), locale => {
   return StringUtils.localeToLocalizedName( locale ).toLowerCase();
 } ) as Locale[];
 
-// Start only with a valid locale, see https://github.com/phetsims/phet-io/issues/1882
-const isLocaleValid = ( locale?: Locale ): boolean => {
-  return !!( locale && availableRuntimeLocales.includes( locale ) );
-};
-
-// Get the "most" valid locale, see https://github.com/phetsims/phet-io/issues/1882
-// As part of https://github.com/phetsims/joist/issues/963, this as changed. We check a specific fallback order based
-// on the locale. In general, it will usually try a prefix for xx_XX style locales, e.g. 'ar_SA' would try 'ar_SA', 'ar', 'en'
-// NOTE: If the locale doesn't actually have any strings: THAT IS OK! Our string system will use the appropriate
-// fallback strings.
-const validInitialLocale = [
-  phet.chipper.locale,
-  ...( phet.chipper.localeData[ phet.chipper.locale ]?.fallbackLocales ?? [] ),
-  FALLBACK_LOCALE
-].find( isLocaleValid );
+export class LocaleProperty extends Property<Locale> {
+  public readonly availableRuntimeLocales: Locale[] = availableRuntimeLocales;
 
-// Just in case we had an invalid locale, remap phet.chipper.locale to the "corrected" value
-phet.chipper.locale = validInitialLocale;
-
-class LocaleProperty extends Property<Locale> {
   protected override unguardedSet( value: Locale ): void {
-    if ( availableRuntimeLocales.includes( value ) ) {
-      super.unguardedSet( value );
-    }
-    else {
-      assert && assert( false, 'Unsupported locale: ' + value );
+    // NOTE: updates phet.chipper.locale as a side-effect
+    super.unguardedSet( phet.chipper.checkAndRemapLocale( value, true ) );
+  }
+
+  protected override toStateObject<StateType>(): ReadOnlyPropertyState<StateType> {
+    const parentObject = super.toStateObject<StateType>();
 
-      // Do not try to set if the value was invalid
-    }
+    // Provide via validValues without forcing validation assertions if a different value is set.
+    parentObject.validValues = this.availableRuntimeLocales as StateType[];
+    return parentObject;
+  }
+
+  protected override applyState<StateType>( stateObject: ReadOnlyPropertyState<StateType> ): void {
+    stateObject.validValues = null;
+    super.applyState( stateObject );
   }
 }
 
-const localeProperty = new LocaleProperty( validInitialLocale, {
+const localeProperty = new LocaleProperty( phet.chipper.locale, {
   tandem: Tandem.GENERAL_MODEL.createTandem( 'localeProperty' ),
   phetioFeatured: true,
   phetioValueType: StringIO,
-  validValues: availableRuntimeLocales,
   phetioDocumentation: 'Specifies language currently displayed in the simulation'
 } );
 
Index: number-suite-common/js/common/model/NumberSuiteCommonPreferences.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/number-suite-common/js/common/model/NumberSuiteCommonPreferences.ts b/number-suite-common/js/common/model/NumberSuiteCommonPreferences.ts
--- a/number-suite-common/js/common/model/NumberSuiteCommonPreferences.ts	(revision 2c829f16f30d1d4869786781bc5bde00159d39d0)
+++ b/number-suite-common/js/common/model/NumberSuiteCommonPreferences.ts	(date 1718060867154)
@@ -14,7 +14,7 @@
 import Property from '../../../../axon/js/Property.js';
 import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
-import localeProperty, { availableRuntimeLocales, Locale } from '../../../../joist/js/i18n/localeProperty.js';
+import localeProperty, { Locale, LocaleProperty } from '../../../../joist/js/i18n/localeProperty.js';
 
 //TODO https://github.com/phetsims/number-suite-common/issues/18 type string map, perhaps getStringModule.TStringModule?
 //TODO https://github.com/phetsims/number-suite-common/issues/18 replace any
@@ -29,7 +29,7 @@
   public readonly showSecondLocaleProperty: Property<boolean>;
 
   // the second locale
-  public readonly secondLocaleProperty: Property<Locale>;
+  public readonly secondLocaleProperty: LocaleProperty;
 
   // whether the Ones are included on the 'Lab' Screen
   public readonly showLabOnesProperty: Property<boolean>;
@@ -60,9 +60,7 @@
     this.showSecondLocaleProperty = new BooleanProperty( !!NumberSuiteCommonQueryParameters.secondLocale );
 
     // if a secondLocale was provided via a query parameter, use that, otherwise default to the primaryLocale
-    this.secondLocaleProperty = new Property<Locale>( NumberSuiteCommonQueryParameters.secondLocale as Locale || localeProperty.value, {
-      validValues: availableRuntimeLocales
-    } );
+    this.secondLocaleProperty = new LocaleProperty( NumberSuiteCommonQueryParameters.secondLocale as Locale || localeProperty.value ); // TODO: review this please, it was using validValues, https://github.com/phetsims/joist/issues/970
 
     this.showLabOnesProperty = new BooleanProperty( NumberSuiteCommonQueryParameters.showLabOnes );
 
Index: joist/js/preferences/PreferencesModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/PreferencesModel.ts b/joist/js/preferences/PreferencesModel.ts
--- a/joist/js/preferences/PreferencesModel.ts	(revision ff8657b06963957a9cd45db83fcb83690268c19e)
+++ b/joist/js/preferences/PreferencesModel.ts	(date 1718045176731)
@@ -19,7 +19,7 @@
 import optionize, { EmptySelfOptions } from '../../../phet-core/js/optionize.js';
 import SpeechSynthesisAnnouncer from '../../../utterance-queue/js/SpeechSynthesisAnnouncer.js';
 import Tandem from '../../../tandem/js/Tandem.js';
-import localeProperty, { Locale } from '../i18n/localeProperty.js';
+import localeProperty, { LocaleProperty } from '../i18n/localeProperty.js';
 import merge from '../../../phet-core/js/merge.js';
 import TReadOnlyProperty from '../../../axon/js/TReadOnlyProperty.js';
 import IOType from '../../../tandem/js/types/IOType.js';
@@ -170,7 +170,7 @@
 } & Required<InputPreferencesOptions>;
 
 export type LocalizationModel = BaseModelType & {
-  localeProperty: Property<Locale>;
+  localeProperty: LocaleProperty;
 } & Required<LocalizationPreferencesOptions>;
 
 type FeatureModel = SimulationModel | AudioModel | VisualModel | InputModel | LocalizationModel;
@@ -228,7 +228,7 @@
       }, providedOptions.inputOptions ),
       localizationOptions: optionize<LocalizationPreferencesOptions, LocalizationPreferencesOptions, BaseModelType>()( {
         tandemName: 'localizationModel',
-        supportsDynamicLocale: !!localeProperty.validValues && localeProperty.validValues.length > 1 && phet.chipper.queryParameters.supportsDynamicLocale,
+        supportsDynamicLocale: !!localeProperty.availableRuntimeLocales && localeProperty.availableRuntimeLocales.length > 1 && phet.chipper.queryParameters.supportsDynamicLocale,
         customPreferences: [],
         includeLocalePanel: true
       }, providedOptions.localizationOptions )
@@ -256,7 +256,7 @@
                               // Running with english locale OR an environment where locale switching is supported and
                               // english is one of the available languages.
                               phet.chipper.locale.startsWith( 'en' ) ||
-                              ( phet.chipper.queryParameters.supportsDynamicLocale && _.some( localeProperty.validValues, value => value.startsWith( 'en' ) ) )
+                              ( phet.chipper.queryParameters.supportsDynamicLocale && _.some( localeProperty.availableRuntimeLocales, value => value.startsWith( 'en' ) ) )
                             );
 
     // Audio can be disabled explicitly via query parameter
Index: number-suite-common/js/common/view/SecondLanguageControl.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/number-suite-common/js/common/view/SecondLanguageControl.ts b/number-suite-common/js/common/view/SecondLanguageControl.ts
--- a/number-suite-common/js/common/view/SecondLanguageControl.ts	(revision 2c829f16f30d1d4869786781bc5bde00159d39d0)
+++ b/number-suite-common/js/common/view/SecondLanguageControl.ts	(date 1718052644033)
@@ -17,7 +17,7 @@
 import ToggleSwitch from '../../../../sun/js/ToggleSwitch.js';
 import PreferencesDialogConstants from '../../../../joist/js/preferences/PreferencesDialogConstants.js';
 import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
-import { availableRuntimeLocales } from '../../../../joist/js/i18n/localeProperty.js';
+import localeProperty from '../../../../joist/js/i18n/localeProperty.js';
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
 import NumberSuiteCommonConstants from '../NumberSuiteCommonConstants.js';
 import LanguageAndVoiceControl from './LanguageAndVoiceControl.js';
@@ -56,7 +56,7 @@
       labelNode: labelText,
       descriptionNode: descriptionText,
       controlNode: toggleSwitch,
-      enabled: ( availableRuntimeLocales.length > 1 ), // disabled if we do not have multiple locales available
+      enabled: ( localeProperty.availableRuntimeLocales.length > 1 ), // disabled if we do not have multiple locales available
       ySpacing: NumberSuiteCommonConstants.PREFERENCES_DESCRIPTION_Y_SPACING
     } );
 
Index: joist/js/preferences/VoicingPanelSection.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/preferences/VoicingPanelSection.ts b/joist/js/preferences/VoicingPanelSection.ts
--- a/joist/js/preferences/VoicingPanelSection.ts	(revision ff8657b06963957a9cd45db83fcb83690268c19e)
+++ b/joist/js/preferences/VoicingPanelSection.ts	(date 1718045176736)
@@ -111,7 +111,7 @@
 
     // Voicing feature only works when running in English. If running in a version where you can change locale,
     // indicate through the title that the feature will only work in English.
-    const titleStringProperty = ( localeProperty.validValues && localeProperty.validValues.length > 1 ) ?
+    const titleStringProperty = ( localeProperty.availableRuntimeLocales && localeProperty.availableRuntimeLocales.length > 1 ) ?
                                 voicingEnglishOnlyLabelStringProperty : voicingLabelStringProperty;
 
     // the checkbox is the title for the section and totally enables/disables the feature
Index: number-suite-common/js/common/NumberSuiteCommonQueryParameters.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/number-suite-common/js/common/NumberSuiteCommonQueryParameters.ts b/number-suite-common/js/common/NumberSuiteCommonQueryParameters.ts
--- a/number-suite-common/js/common/NumberSuiteCommonQueryParameters.ts	(revision 2c829f16f30d1d4869786781bc5bde00159d39d0)
+++ b/number-suite-common/js/common/NumberSuiteCommonQueryParameters.ts	(date 1718060867157)
@@ -19,6 +19,7 @@
 
   // specifies a second locale to make available on the 'Ten', 'Twenty', and 'Compare' screens. Values are specified
   // with a locale code, e.g. "en" or "zh_CN".
+  // TODO: Use checkAndRemapLocales to support this one too, https://github.com/phetsims/joist/issues/970
   secondLocale: {
     public: true,
     type: 'string',

This is a nice change because it doesn't involve any PhET-iO API changes to accomplish.

For MR:

  • The PhET-iO changes may likely instead want to create a LocalePropertyIO the hacks the availableRuntimeLocales into the validValues in toStateObject.
  • For the PhET branded changes, we will still want to be able to call checkAndRemapLocale from localeProperty, but I'm not sure how much of this patch needs to come along with that.

Next steps:

  • Work with @jonathanolson to commit this patch to main.
  • Work with @jonathanolson to create an MR patch for Hydrogen sims.
  • add supportsDynamicLocale flag on localeProperty that just checks on availableLocales length> 1
  • aboutDialog links should use current localeProperty, instead of hard coded to the set locale when the dialog was created.
  • sort the validValues in toStateObject for an alphabetical validValues in studio and the API file (maybe?), note Locales are not presented in alphabetical order. #911 and sorting logic change.
  • secondLocale in number suite should use this new logic.
  • Yotta does not correctly track the locale for standard wrapper use. This is because the locale is not provided through a startup query parameter, and phet-io only changes the localeProperty, much later in the process than when GA and yotta process. We can likely fix this by providing the locale to the standard wrapper, knowing that it will flip flop later, but allowing the startup state to be correct for preloads.
  • I noticed two entries in the QSM warning for the same query parameter, is this just because of unbuilt race condition? http://localhost:8080/center-and-variability/center-and-variability_en.html?brand=phet&ea&debugger&locale=es_Pfjdskal;fjkdsla;fj

zepumph added a commit to phetsims/number-suite-common that referenced this issue Jun 11, 2024
…operty should support it with grace, phetsims/joist#970

Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit that referenced this issue Jun 11, 2024
…operty should support it with grace, #970

Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit to phetsims/axon that referenced this issue Jun 11, 2024
…operty should support it with grace, phetsims/joist#970

Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit to phetsims/chipper that referenced this issue Jun 11, 2024
…operty should support it with grace, phetsims/joist#970

Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit to phetsims/phetcommon that referenced this issue Jun 11, 2024
…operty should support it with grace, phetsims/joist#970

Signed-off-by: Michael Kauzmann <[email protected]>
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
jonathanolson added a commit that referenced this issue Jun 12, 2024
…ding duplicated initialize-globals logic, see #970
jonathanolson added a commit to phetsims/acid-base-solutions that referenced this issue Jun 12, 2024
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
jonathanolson added a commit that referenced this issue Jun 12, 2024
…ding duplicated initialize-globals logic, see #970
jonathanolson added a commit to phetsims/beers-law-lab that referenced this issue Jun 12, 2024
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
jonathanolson added a commit that referenced this issue Jun 12, 2024
…ding duplicated initialize-globals logic, see #970
jonathanolson added a commit to phetsims/calculus-grapher that referenced this issue Jun 12, 2024
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
jonathanolson added a commit that referenced this issue Jun 12, 2024
…ding duplicated initialize-globals logic, see #970
jonathanolson added a commit to phetsims/center-and-variability that referenced this issue Jun 12, 2024
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
jonathanolson added a commit that referenced this issue Jun 12, 2024
…ding duplicated initialize-globals logic, see #970
jonathanolson added a commit to phetsims/circuit-construction-kit-dc that referenced this issue Jun 12, 2024
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
jonathanolson added a commit that referenced this issue Jun 12, 2024
…ding duplicated initialize-globals logic, see #970
jonathanolson added a commit to phetsims/circuit-construction-kit-dc-virtual-lab that referenced this issue Jun 12, 2024
jonathanolson added a commit to phetsims/axon that referenced this issue Jun 12, 2024
zepumph added a commit to phetsims/circuit-construction-kit-dc that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/circuit-construction-kit-dc-virtual-lab that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/concentration that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/density that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/diffusion that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/friction that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/gas-properties that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/gases-intro that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/geometric-optics that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/geometric-optics-basics that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/graphing-quadratics that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/gravity-and-orbits that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/greenhouse-effect that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/greenhouse-effect that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/keplers-laws that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/molecule-polarity that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/molecule-shapes that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/molecule-shapes-basics that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/my-solar-system that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/natural-selection that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/ph-scale that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/ph-scale-basics that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/projectile-data-lab that referenced this issue Jul 16, 2024
zepumph added a commit to phetsims/projectile-sampling-distributions that referenced this issue Jul 16, 2024
@zepumph
Copy link
Member Author

zepumph commented Jul 16, 2024

MR for doc update was completed above.

@zepumph
Copy link
Member Author

zepumph commented Jul 16, 2024

Yotta does not correctly track the locale for standard wrapper use. This is because the locale is not provided through a startup query parameter, and phet-io only changes the localeProperty, much later in the process than when GA and yotta process. We can likely fix this by providing the locale to the standard wrapper, knowing that it will flip flop later, but allowing the startup state to be correct for preloads.

Brainstorming solutions:

  1. Delay google-analytics.js logic from running until the simulation construction has completed, so we can use the most up to date value for phet.chipper.locale.
  2. Create the notion of "default locale", and have studio set the current value of localeProperty to that default when creating the standard PhET-iO wrapper. Then the default locale needs to be set to phet.chipper.locale as part of initialize-globals (before analytics fires).

More notes about fixing this:

  1. We already noted that it wasn't worth creating a notion of a default locale to fix the fallback behavior for the standard wrapper.
  2. To maintenance release either of these solutions would be serious work. Would we ever want to just fix on main and call it a day?
  3. I personally do not recommend any changes for this, but it isn't my call.

@zepumph
Copy link
Member Author

zepumph commented Jul 16, 2024

Added to PhET-iO meeting agenda before closing.

@zepumph
Copy link
Member Author

zepumph commented Jul 25, 2024

Discussed and noted by the PhET-iO team. No changes requested.

@zepumph zepumph closed this as completed Jul 25, 2024
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