Customized strings in old studio version applies to active language

[Nancy-Salpepi]

Test device MacBook Air (m1 chip)

Operating System macOS 13.1

Browser safari

Problem description For https://github.com/phetsims/qa/issues/872, if I load a customized wrapper from studio version 1.5 into studio version 1.6, where 1.6 is in a locale other than English (ex. ar), the result is a blend of languages.

Steps to reproduce

1. In Studio version 1.5, open the Macro Screen.
2. Press Save
3. In Studio version 1.6, add ?locale=ar to the url OR change the locale in Preferences Menu
4. Press Load File button and select the customized wrapper you just saved.

Visuals Here is the Macro Screen in arabic. You can see it is translated.

Here is the Macro Screen once the customized wrapper has been loaded. It is a mix of ar and en.

[zepumph]

@samreid made an important note about this, where this really is only going to apply to the 5 sims that we are updating to use the stringsState for the next version. After these, we will go from a previous version that uses stringsState to a new one that uses the same.

A migration rule will likely be simple here. We discovered this as part of https://github.com/phetsims/phet-io/issues/1903, and this will likely need commits for the ph-scale rcs.

[zepumph]

I still think it is worth making this right. I'll take a look this afternoon.

[zepumph]

Here is an exciting new migration rule for converting into stringsState instead of individual Properties. I'll come back to clean up later.

```diff Subject: [PATCH] new stringsState migration rule --- Index: phet-io-wrappers/common/js/migration/MigrationEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/MigrationEngine.ts b/phet-io-wrappers/common/js/migration/MigrationEngine.ts --- a/phet-io-wrappers/common/js/migration/MigrationEngine.ts (revision 708869e7fef07c6840e68db46317404ae5728129) +++ b/phet-io-wrappers/common/js/migration/MigrationEngine.ts (date 1674063385523) @@ -55,6 +55,7 @@ this.rules.forEach( migrationRule => { const result = migrationRule.processCommand( command, this.oldAPIFlat, this.newAPIFlat ); command = result.command; + console.log( command.args[ 0 ][ 'phScale.general.model.stringsState' ] ); messageBatches.push( result.messages ); } ); Index: phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.ts b/phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.ts new file mode 100644 --- /dev/null (date 1674063702169) +++ b/phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.ts (date 1674063702169) @@ -0,0 +1,82 @@ +// Copyright 2022, University of Colorado Boulder +// This PhET-iO file requires a license +// USE WITHOUT A LICENSE AGREEMENT IS STRICTLY PROHIBITED. +// For licensing, please contact phethelp@colorado.edu + +/** + * A migration rule that renames any phetioID that matches the "fragment" provided, replacing it with the new "fragment" + * Each fragment can be the entire phetioID if desired. + * + * @author Michael Kauzmann (PhET Interactive Simulations) + * @author Sam Reid (PhET Interactive Simulations) + */ + +import Command from './Command.js'; +import { MigrationResult } from './MigrationRule.js'; +import TandemFragmentRenamed from './TandemFragmentRenamed.js'; + +export default class ConvertToStringsStateForLocale extends TandemFragmentRenamed { + + public constructor( oldFragment: string, + newFragment: string, + public readonly stringsStatePhetioID: string, // TODO: compute this? Probably not worth it, but maybe + public readonly locale = 'en' ) { + super( oldFragment, newFragment ); + } + + public override toString(): string { // TODO + return 'TandemFragmentRenamed: Renamed phetioIDs containing ' + this.oldFragment + ' -> ' + this.newFragment + '. The phetioID was renamed between versions, ' + + 'and the PhET-iO Element\'s state was forwarded to the new name. Most likely nothing else is needed for this ' + + 'migration rule, but if customizing in the previous version, it would be good to double check that it is ' + + 'still customized in the new version.'; + } + + private getBlankStringsState(): object { + return { data: { [ this.locale ]: {} } }; + } + + public override processCommand( command: Command ): MigrationResult { + + const messages: string[] = []; + if ( command.phetioID === 'phetioEngine' && command.method === 'setState' ) { + + const oldState = command.args[ 0 ] as object; + + // @ts-expect-error + const stringsState = oldState.hasOwnProperty( this.stringsStatePhetioID ) ? oldState[ this.stringsStatePhetioID ] : + this.getBlankStringsState(); + + + const newState = { [ this.stringsStatePhetioID ]: stringsState }; + + const oldStatePhetioIDs = Object.keys( oldState ); + + for ( let i = 0; i < oldStatePhetioIDs.length; i++ ) { + const oldPhetioID = oldStatePhetioIDs[ i ]; + + if ( oldPhetioID !== this.stringsStatePhetioID ) { + + + let newPhetioID = null; + if ( this.matchesTandem( oldPhetioID ) ) { + newPhetioID = this.promote( oldPhetioID ); + + // @ts-expect-error + const oldStringState = oldState[ oldPhetioID ]; + assert && assert( oldStringState.hasOwnProperty( 'value' ), 'dealing with Properties here' ); + stringsState.data[ this.locale ][ newPhetioID ] = oldStringState.value; + messages.push( `ConvertToStringsStateForLocale (into ${this.locale} stringsState): ${oldPhetioID} ==> ${newPhetioID}` ); + } + else { + + // @ts-expect-error https://github.com/phetsims/phet-io-wrappers/issues/470 + newState[ oldPhetioID ] = oldState[ oldPhetioID ]; + } + } + } + + command = command.withArgs( [ newState ] ); + } + return { command: command, messages: messages }; + } +} Index: phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts b/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts --- a/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts (revision 4d7ad23ebab0e38bca4e2e926880d25c90856078) +++ b/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts (date 1674063468765) @@ -4,6 +4,7 @@ // For licensing, please contact phethelp@colorado.edu import TandemFragmentRenamed from '../../../../phet-io-wrappers/common/js/migration/TandemFragmentRenamed.js'; +import ConvertToStringsStateForLocale from '../../../../phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.js'; import TandemRegExpReplacement from '../../../../phet-io-wrappers/common/js/migration/TandemRegExpReplacement.js'; import LinkedElementHasElementID from '../../../../phet-io-wrappers/common/js/migration/LinkedElementHasElementID.js'; import StatefulLinkedElementDelete from '../../../../phet-io-wrappers/common/js/migration/StatefulLinkedElementDelete.js'; @@ -28,8 +29,9 @@ // NOTE! These should go before the solutes conversion new TandemFragmentRenamed( 'phScale.macroScreen.view.waterFaucetNode.waterLabelNode.textProperty', 'phScale.general.model.strings.phScale.choice.waterStringProperty' ), - new TandemFragmentRenamed( 'phScale.microScreen.view.waterFaucetNode.waterLabelNode.textProperty', - 'phScale.general.model.strings.phScale.choice.waterStringProperty' ), + new ConvertToStringsStateForLocale( 'phScale.microScreen.view.waterFaucetNode.waterLabelNode.textProperty', + 'phScale.general.model.strings.phScale.choice.waterStringProperty', + 'phScale.general.model.stringsState', 'en' ), // Solutions - prioritize version of water over the faucet node text new TandemRegExpReplacement( /phScale\.global\.model\.solutes\.(.*)\.nameProperty/, 'phScale.general.model.strings.phScale.choice.$1StringProperty' ), Index: phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts b/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts --- a/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts (revision 708869e7fef07c6840e68db46317404ae5728129) +++ b/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts (date 1674063385533) @@ -21,6 +21,8 @@ private readonly oldInterTermFragment: string; private readonly newInterTermFragment: string; + protected static readonly SEPARATOR = SEPARATOR; + public constructor( public readonly oldFragment: string, public readonly newFragment: string ) { super( 'TandemFragmentRenamed' ); assert && assert( oldFragment !== 'phetioEngine', 'please do not rename phetioEngine' );

[zepumph]

I got to a commit point! I'm a bit nervous about this, and I think a timely co-review with @samreid will be very beneficial. Right now I updated ph-scale, but it should be propagated into GAO and others as part of the review. Marking blocking publication until we can talk it through.

[zepumph]

Noting also https://github.com/phetsims/phet-io/issues/1903 since that issue would have tagged these are wrong anyways.

[samreid]

We additionally need commits from:

• [x] https://github.com/phetsims/phet-io-wrappers/issues/474

[samreid]

This is ready for cherry-picking, all referenced commits except the GAO one. ALSO, the commit https://github.com/phetsims/phet-io/commit/9bb5bd062a5793b417a7fe6b11dbffc5b6220174 should not be cherry picked for this issue, it is more related to https://github.com/phetsims/phet-io/issues/1903.

[samreid]

Here come some more!!

[zepumph]

Will be cherry picked in https://github.com/phetsims/ph-scale/issues/269.