Make some LocalizedStrings phetioReadOnly: true

[samreid]

From https://github.com/phetsims/axon/issues/408

[samreid]

Patch from today's meeting:

Index: js/LocalizedString.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/LocalizedString.ts b/js/LocalizedString.ts
--- a/js/LocalizedString.ts (revision d2b106e08c42c805e082570609352aab5f39aefc)
+++ b/js/LocalizedString.ts (date 1661465048890)
@@ -53,7 +53,11 @@
     this.property = new DynamicProperty( localeProperty, {
       derive: ( locale: string ) => this.getLocaleSpecificProperty( locale ),
       bidirectional: true,
-      phetioReadOnly: false,
+
+      // Don't allow editing the joist credits
+      phetioReadOnly: tandem.phetioID.includes( '.joist.credits.' ) ||
+                      tandem.phetioID.includes( 'joist.versionPatternStringProperty' ) ||
+                      tandem.phetioID.includes( 'joist.termsPrivacyAndLicensingStringProperty' ),
       phetioValueType: StringIO,
       phetioState: false,
       tandem: tandem,

[zepumph]

How about this crazy idea!!?!?!?

```diff Index: chipper/js/LocalizedString.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/LocalizedString.ts b/chipper/js/LocalizedString.ts --- a/chipper/js/LocalizedString.ts (revision 90024daf1d69ecd536cea33b9ee1f90bfd38e02f) +++ b/chipper/js/LocalizedString.ts (date 1661467052999) @@ -17,6 +17,7 @@ import TProperty from '../../axon/js/TProperty.js'; import { localizedStrings } from './getStringModule.js'; import arrayRemove from '../../phet-core/js/arrayRemove.js'; +import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js'; // constants const FALLBACK_LOCALE = 'en'; @@ -42,7 +43,7 @@ // Store initial values, so we can handle state deltas private readonly initialValues: Record = {}; - public constructor( englishValue: TranslationString, tandem: Tandem ) { + public constructor( englishValue: TranslationString, tandem: Tandem, options: IntentionalAny ) { this.englishProperty = new TinyProperty( englishValue ); this.initialValues[ FALLBACK_LOCALE ] = englishValue; @@ -53,10 +54,10 @@ this.property = new DynamicProperty( localeProperty, { derive: ( locale: string ) => this.getLocaleSpecificProperty( locale ), bidirectional: true, - phetioReadOnly: false, + phetioReadOnly: options.phetioReadOnlyPredicate( tandem.phetioID ), phetioValueType: StringIO, phetioState: false, - tandem: tandem + tandem: options.phetioFeaturedPredicate( tandem.phetioID ) } ); localizedStrings.push( this ); Index: joist/js/joistStrings.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/joistStrings.ts b/joist/js/joistStrings.ts --- a/joist/js/joistStrings.ts (revision a5064217d68bd28986e9b6db82ea9d7df53cc6dc) +++ b/joist/js/joistStrings.ts (date 1661467053013) @@ -387,7 +387,16 @@ } }; -const joistStrings = getStringModule( 'JOIST' ) as StringsType; +const joistStrings = getStringModule( 'JOIST', { + phetioReadOnlyPredicate: ( stringKey: string ) => { + return stringKey.includes( '.joist.credits.' ) || + stringKey.includes( 'joist.versionPatternStringProperty' ) || + stringKey.includes( 'joist.termsPrivacyAndLicensingStringProperty' ); + }, + phetioFeaturedPredicate: ( stringKey: string ) => { + stringKey.includes( 'voicing.titleProperty' ) + } +} ) as StringsType; joist.register( 'joistStrings', joistStrings ); Index: chipper/js/getStringModule.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/getStringModule.ts b/chipper/js/getStringModule.ts --- a/chipper/js/getStringModule.ts (revision 90024daf1d69ecd536cea33b9ee1f90bfd38e02f) +++ b/chipper/js/getStringModule.ts (date 1661467110463) @@ -15,6 +15,7 @@ import Tandem from '../../tandem/js/Tandem.js'; import CouldNotYetDeserializeError from '../../tandem/js/CouldNotYetDeserializeError.js'; import IOType from '../../tandem/js/types/IOType.js'; +import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js'; import ObjectLiteralIO from '../../tandem/js/types/ObjectLiteralIO.js'; import LocalizedString, { LocalizedStringStateDelta } from './LocalizedString.js'; import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; @@ -86,7 +87,7 @@ * @param requirejsNamespace - E.g. 'JOIST', to pull string keys out from that namespace * @returns Nested object to be accessed like joistStrings.ResetAllButton.name */ -const getStringModule = ( requirejsNamespace: string ): object => { +const getStringModule = ( requirejsNamespace: string, localizationPropertyOptions: IntentionalAny ): object => { // Our string information is pulled globally, e.g. phet.chipper.strings[ locale ][ stringKey ] = stringValue; // Our locale information is from phet.chipper.locale @@ -178,7 +179,7 @@ tandem = tandem.createTandem( tandemName ); } - const localizedString = new LocalizedString( phet.chipper.mapString( phet.chipper.strings[ FALLBACK_LOCALE ][ stringKey ] ), tandem ); + const localizedString = new LocalizedString( phet.chipper.mapString( phet.chipper.strings[ FALLBACK_LOCALE ][ stringKey ] ), tandem, localizationPropertyOptions ); localizedStringMap[ stringKey ] = localizedString; // Push up the translated values ```

[samreid]

That is much better than the previous iteration, thanks! Considering taking it one step further, what about this?

Index: joist-strings_en.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist-strings_en.json b/joist-strings_en.json
--- a/joist-strings_en.json (revision a5064217d68bd28986e9b6db82ea9d7df53cc6dc)
+++ b/joist-strings_en.json (date 1661477546163)
@@ -51,7 +51,8 @@
     "value": "Software Development: {0}"
   },
   "credits.team": {
-    "value": "Team: {0}"
+    "value": "Team: {0}",
+    "phetioReadOnly": true
   },
   "credits.contributors": {
     "value": "Contributors: {0}"

[arouinfar]

We'd like to prevent clients from editing the sim credits and other contents of the About dialog. Let's make these strings phetioReadOnly: true.

• [x] strings.joist.termsPrivacyAndLicensingStringProperty
• [x] strings.joist.versionPatternStringProperty
• [x] strings.joist.credits.*
• [x] strings.joist.thirdParty.credits.linkStringProperty
• [x] strings.joist.translation.credits.linkStringProperty

[zepumph]

I don't yet feel like I have a good solution for how to do this. It makes me incredibly worried to put this data into the string files, but perhaps that is an acceptable solution. I won't be able to investigate this soon. @samreid, would you mind trying out putting the data in string files, and seeing what you need to do to load-unbuilt-strings and getStringModule to have this work in both (un)built modes?

[zepumph]

Before committing, we would want to double check with rosetta team that they are depending on a specific structure of phet.chipper.strings or somethign.

[samreid]

@jbphet or @liammulh does the above proposal sound reasonable to you?

[liammulh]

I could be wrong, but I don't think this (the most recent patch) will negatively affect rosetta since it doesn't look for anything other than stringKey.value. @jbphet, what say you?

[zepumph]

@samreid, just to be clear here, the object returned by getStringModule most likely shouldn't change at all. We only need to store this data in phet.chipper.strings, which will be used to create the Properties in that object, but we don't need to put phetioReadOnly into that object used by sims.

Some ideas:

  "menuItem.phetWebsite": {
    "value": "PhET Website\u2026",
    "phet-ioMetadata": {},
    "phetioMetadata": {},
    "_metadata": {},
    "metadata": {
      "phetioReadOnly": true
    },
    "phetioReadOnly": true
  },

[zepumph]

We looked at this today at PhET-iO design meeting, @jbphet can you please comment here as to if adding this metadata to the en json string files would be alright from a rosetta/translation perspective?

[zepumph]

@jonathanolson recommends keeping it out of phet.chipper.strings, because that is more of a per-locale data structure, so we will just want to create our own metadata object that can be used in getStringModule.

We need to support this in getStringMap and load-unbuilt-strings.

[jbphet]

Above, @zepumph asked:

@jbphet can you please comment here as to if adding this metadata to the en json string files would be alright from a rosetta/translation perspective?

In an earlier comment, he said:

It makes me incredibly worried to put this data into the string files...

In the comment immediately above this one, he said:

@jonathanolson recommends keeping it out of phet.chipper.strings, because that is more of a per-locale data structure, so we will just want to create our own metadata object that can be used in getStringModule.

So, at this point, I'm a little confused. Are we still talking about putting the metadata to mark a string as phetioReadOnly in the English version of the strings file? If so, would it only be in the English file and not in the others? If we specifically know the strings that we want to be phetioReadOnly and don't expect to create more of them during the development process, perhaps we should just make them a special case and handle it in the code, and not require additional information in the strings file(s).

I obviously have some concerns about this approach, but to specifically answer your question, I don't think adding this metadata to the English version of the strings file will negatively impact Rosetta. Rosetta will just ignore the information.

[jonathanolson]

There's certain metadata for strings that apply to ALL locales, this seems to be an example (confirmed during design meeting). I propose that we keep that metadata in the file that is checked into the sim (which happens to be the English string file).

For the runtime of sims (built and unbuilt), that metadata doesn't fit well with what phet.chipper.strings currently is. I recommend placing the metadata elsewhere (phet.chipper.stringData?) that can be used by getStringModule.

[jonathanolson]

@zepumph thoughts on:

  "menuItem.phetWebsite": {
    "value": "PhET Website\u2026",
    "metadata": {
      "phetioReadOnly": true
    }
  },

and phet.chipper.stringMetadata[ stringKey ] is the content of everything under metadata?

So in getStringModule, near

const localizedString = new LocalizedString( phet.chipper.mapString( phet.chipper.strings[ FALLBACK_LOCALE ][ stringKey ] ), tandem );

we can just grab phet.chipper.stringMetadata[ stringKey ]?.phetioReadOnly. We'll need to create options for LocalizedString.

load-unbuilt-strings will populate phet.chipper.stringMetadata as it loads the English file.

We'll create getStringMetadata (similar to getStringMap), to be used in buildRunnable, we'll include it in commonInitializationOptions, getInitializationScript() will be modified to include stringMetadata, and chipper/templates/chipper-initialization.js will be modified to add a lint for the stringMetadata.

Thoughts? Should we collaborate on this, or schedule a time for it?

[zepumph]

I'd love to take a first pass on this if you don't mind. I'll let you know how it goes!

[zepumph]

Unfortunately I will not have time for this like I thought I would. @jonathanolson, can you please take the lead on it and work with @samreid for any PhET-iO questions?

[jonathanolson]

Implemented, and added read-only to the strings in https://github.com/phetsims/joist/issues/845#issuecomment-1228885371.

@zepumph can you verify?

[samreid]

I'll review this issue, thanks @jonathanolson

[samreid]

I reviewed the places phetioReadOnly was marked true, and checked that they appeared correctly in studio. I tested built and unbuilt mode. I considered that we may one day want to put metadata in a parent node, like:

credits:{
   phetioReadOnly: true,
   author: {
      // etc
   }

But I believe that would be overcomplicated for now.

I think this issue can be closed, @kathy-phet do you recommend any further design or code review?

[zepumph]

Review:

• I factored out a default metadata value.
• In my opinion, LocalizedString should be moved to joist, it is the only module in chipper that depends on joist. It also seems best in i18n. I would also recommend that moving getStringModule to joist would also be an improvement. Then all sim-loaded i18n code would be together.

@arouinfar Here is the list that we marked as read only:

"termsPrivacyAndLicensing"
"credits.title"
"credits.leadDesign"
"credits.softwareDevelopment"
"credits.team"
"credits.contributors"
"credits.qualityAssurance"
"credits.graphicArts"
"credits.soundDesign"
"credits.translation"
"credits.thanks"
"options.title"
"versionPattern"
"translation.credits.link"
"thirdParty.credits.link"

@arouinfar does this look complete and correct?

Everything else looks great! Thanks all!

[arouinfar]

Yes, that all looks good @zepumph.

[jonathanolson]

It sounds fine to move LocalizedString/getStringModule to joist, although that will make maintenance releases more difficult for getStringModule. @kathy-phet do you know if we would maintenance release anything like that?

[zepumph]

No strong preference. Let's just keep getStringModel in chipper.

[samreid]

@zepumph can this issue be closed?

[samreid]

@zepumph and I reviewed the issue and agreed it is ready to close. Thanks!