Sim screen name property and title property should link back to LocalizedStringProperties

[samreid]

Discovered in phetsims/axon#408, you cannot localize Sim title and sim screen titles. Sim screen name property and title property should link back to LocalizedStringProperties

[samreid]

I'm seeing Sim.ts:

    this.simNameProperty = new StringProperty( typeof name === 'string' ? name : name.value, {
      tandem: Tandem.GENERAL_MODEL.createTandem( 'simNameProperty' ),
      phetioFeatured: true,
      phetioDocumentation: 'The name of the sim. Changing this value will update the title text on the navigation bar ' +
                           'and the title text on the home screen, if it exists.'
    } );

Do we want to:
(a) eliminate this and just use the localized string
(b) create a DerivedProperty here? (so we can keep the phetioDocumentation?)

I would recommend (a) and just working on making it discoverable via autoselect.

@zepumph what do you recommend?

[zepumph]

Creating a DerivedProperty sounds nice, when you create a Sim, I don't really think we should lock in needing to pass a Property for the name. Shouldn't we support the case of new Sim( 'mySim' . . .)? If feels like something that should live in Sim instead of off in the strings files. Note 39 usages of .simNameProperty in the project (mostly passed around joist). I don't see it as a gain to strip that out.

Note that no matter what we will need to instrument all the derivedProperties that map from the Title text in the nav bar back to the model. This means instrumenting the displayedSimNameProperty,

[samreid]

I committed this and it seems to be working well. The main UX issue is that it takes a lot of clicks to get from, say, the nav bar to the sim title:

• gravityAndOrbits.general.view.navigationBar.titleText (via autoselect)
• gravityAndOrbits.general.view.navigationBar.titleText.textProperty
• gravityAndOrbits.general.model.displayedSimNameProperty
• gravityAndOrbits.general.model.displayedSimNameProperty.dependencies.gravityAndOrbits-general-model-simNameProperty
• gravityAndOrbits.general.model.simNameProperty
• gravityAndOrbits.general.model.simNameProperty.dependencies.gravityAndOrbits-general-model-strings-gravityAndOrbits-gravityAndOrbits-titleStringProperty
• gravityAndOrbits.general.model.strings.gravityAndOrbits.gravityAndOrbits.titleStringProperty

Wow, that's a lot of clicks to get to the title! Maybe we should unravel some of those layers, or maybe we should add nested cutouts (which also follow dependencies???), so you can get there by clicking once and scrolling? I'm not confident about that at all.

[zepumph]

Tagging for design review tomorrow.

[zepumph]

We want to have the sim constructor only take a StringProperty as an arg, not a string.

Sim.simNameProperty will just be a pointer for that.

Index: js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Sim.ts b/js/Sim.ts
--- a/js/Sim.ts	(revision 9f3c65ed39604d7c6a1c4899c4397af2c1ad7825)
+++ b/js/Sim.ts	(date 1662059596837)
@@ -257,7 +257,7 @@
    * @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 | TReadOnlyProperty<string>, allSimScreens: Screen[], providedOptions?: SimOptions ) {
+  public constructor( name: TReadOnlyProperty<string>, allSimScreens: Screen[], providedOptions?: SimOptions ) {
 
     window.phetSplashScreenDownloadComplete();
 
@@ -308,15 +308,7 @@
 
     this.credits = options.credits;
 
-    const stringTitleDependency = typeof name === 'string' ? new StringProperty( name ) : name;
-
-    this.simNameProperty = new DerivedProperty( [ stringTitleDependency ], dep => dep, {
-      tandem: Tandem.GENERAL_MODEL.createTandem( 'simNameProperty' ),
-      phetioValueType: StringIO,
-      phetioFeatured: true,
-      phetioDocumentation: 'The name of the sim, shown in the navigation bar ' +
-                           'and the title text on the home screen, if it exists.'
-    } );
+    this.simNameProperty = 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

[zepumph]

• Make sure after this change to post on the breaking changes thread of the google group. (https://groups.google.com/g/developing-interactive-simulations-in-html5/c/1Zm4l3sye20/m/n1CSL6JwAwAJ)

[samreid]

Changes in Screen.ts look great, I would like to promote this to a new issue:

I see 71 usages of String = .*StringProperty;, should these variables be renamed? I kinda don't think it matters.

Anything else to do here?

[kathy-phet]

@samreid @zepumph - I was reviewing GAO as part of the Guide review, and the work here doesn't look like its done yet, or at least it is not fully working in what I am seeing.

The spots I still see issues:

• For the screen names, everything should link back to these two strings:
  gravityAndOrbits.general.model.strings.gravityAndOrbits.modelStringProperty
  gravityAndOrbits.general.model.strings.gravityAndOrbits.toScaleStringProperty

• However, the screen names still have an intermediary that is separately editable ...
  gravityAndOrbits.modelScreen.nameProperty
  gravityAndOrbits.toScaleScreen.nameProperty

• If I use "Alt" mouse over the sim name in the joist bar, it takes me here:
  gravityAndOrbits.general.model.displayedSimNameProperty
  But one of the two dependencies is incorrect. This one needs changing:
  gravityAndOrbits.general.model.displayedSimNameProperty.dependencies.gravityAndOrbits-modelScreen-nameProperty
  It shows an editable intermediary that we want to remove in the cutout instead of the base String property.
  gravityAndOrbits.general.model.strings.gravityAndOrbits.modelStringProperty

• These are all linking to the incorrect element... they should show the editable stringProperty in the cut out. Those intermediaries should be gone.
  gravityAndOrbits.general.view.navigationBar.modelScreenButton.text.textProperty
  gravityAndOrbits.general.view.navigationBar.toScaleScreenButton.text.textProperty
  gravityAndOrbits.general.view.navigationBar.titleText.textProperty
  gravityAndOrbits.homeScreen.view.buttonGroup.modelScreenButton.text.textProperty
  gravityAndOrbits.homeScreen.view.buttonGroup.toScaleScreenButton.text.textProperty

This one is good!
gravityAndOrbits.homeScreen.view.titleText.textProperty

[kathy-phet]

Unless I'm being fooled by phettest ... it doesn't seem to be pulling correctly.

[kathy-phet]

I was being fooled by phettest. Sad face.

One question though:
I understand that "gravityAndOrbits.general.model.displayedSimNameProperty" is used to create names like
"Gravity and Orbits - To Scale"
This seems to only happen when you actually use ?screens=2 or another screen parameter.

When you do "availableScreensProperty" and choose [ 2 ]. It remains "Gravity and Orbits".

I want to make sure this is by-design. I think so, because this leaves ultimate flexibility to change what the title node says. Otherwise you could not get rid of the "-" within Studio. Is this why its done this way? It's just a difference between ?screens and the behavior with the options in studio so I want to make sure everyone knows its different.

[samreid]

Thanks, I believe that was an oversight and that it should update accordingly. I revised that logic like so:

Index: js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Sim.ts b/js/Sim.ts
--- a/js/Sim.ts	(revision d6008a3977b907f1c238147959e7115baed43e73)
+++ b/js/Sim.ts	(date 1663259046872)
@@ -539,9 +539,14 @@
       }
     } );
 
-    this.displayedSimNameProperty = new DerivedProperty( [ this.simNameProperty, this.simScreens[ 0 ].nameProperty ],
-      ( simName, screenName ) => {
-        const isMultiScreenSimDisplayingSingleScreen = this.simScreens.length === 1 && allSimScreens.length !== this.simScreens.length;
+    this.displayedSimNameProperty = DerivedProperty.deriveAny( [ this.simNameProperty, this.availableScreensProperty, ...this.simScreens.map( screen => screen.nameProperty ) ],
+      () => {
+
+        const availableScreens = this.availableScreensProperty.value;
+        const simName = this.simNameProperty.value;
+        const screenName = this.selectedScreenProperty.value.nameProperty.value;
+
+        const isMultiScreenSimDisplayingSingleScreen = availableScreens.length === 1 && allSimScreens.length > 1;
 
         // update the titleText based on values of the sim name and screen name
         if ( isMultiScreenSimDisplayingSingleScreen && simName && screenName ) {

@zepumph can you please review and commit (or give me the go-ahead) if everything seems good?

[kathy-phet]

@samreid - OK - but one more fix, it then needs one more fix if you haven't done it. Please add the 3rd dependency to the dependencies list. It should be the title and both screen names, not just the title and first screen.

[samreid]

Just checking on understanding of the patch syntax--the red code with "-" is the deleted code, the green code with "+" is the added code. The deleted code has this.simScreens[ 0 ].nameProperty. The proposed code has ...this.simScreens.map( screen => screen.nameProperty ) which takes all of them.

[zepumph]

Index: js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Sim.ts b/js/Sim.ts
--- a/js/Sim.ts	(revision 3594a6bd1680b9867075fb7b31044b3205390c05)
+++ b/js/Sim.ts	(date 1663269726662)
@@ -539,30 +539,39 @@
       }
     } );
 
-    this.displayedSimNameProperty = new DerivedProperty( [ this.simNameProperty, this.simScreens[ 0 ].nameProperty ],
-      ( simName, screenName ) => {
-        const isMultiScreenSimDisplayingSingleScreen = this.simScreens.length === 1 && allSimScreens.length !== this.simScreens.length;
+    this.displayedSimNameProperty = new DerivedProperty( [
+      this.availableScreensProperty,
+      this.simNameProperty,
+      this.selectedScreenProperty,
+      JoistStrings.simTitleWithScreenNamePatternStringProperty,
+
+      // We just need notifications on any of these changing, return args as a unique value to make sure listeners fire.
+      DerivedProperty.deriveAny( this.simScreens.map( screen => screen.nameProperty ), ( ...args ) => [ ...args ] )
+    ], ( availableScreens, simName, selectedScreen, titleWithScreenPattern ) => {
+      const screenName = selectedScreen.nameProperty.value;
+
+      const isMultiScreenSimDisplayingSingleScreen = availableScreens.length === 1 && allSimScreens.length > 1;
 
-        // update the titleText based on values of the sim name and screen name
-        if ( isMultiScreenSimDisplayingSingleScreen && simName && screenName ) {
+      // update the titleText based on values of the sim name and screen name
+      if ( isMultiScreenSimDisplayingSingleScreen && simName && screenName ) {
 
-          // If the 'screens' query parameter selects only 1 screen and both the sim and screen name are not the empty
-          // string, then update the nav bar title to include a hyphen and the screen name after the sim name.
-          return StringUtils.fillIn( JoistStrings.simTitleWithScreenNamePattern, {
-            simName: simName,
-            screenName: screenName
-          } );
-        }
-        else if ( isMultiScreenSimDisplayingSingleScreen && screenName ) {
-          return screenName;
-        }
-        else {
-          return simName;
-        }
-      }, {
-        tandem: Tandem.GENERAL_MODEL.createTandem( 'displayedSimNameProperty' ),
-        phetioValueType: StringIO
-      } );
+        // If the 'screens' query parameter selects only 1 screen and both the sim and screen name are not the empty
+        // string, then update the nav bar title to include a hyphen and the screen name after the sim name.
+        return StringUtils.fillIn( titleWithScreenPattern, {
+          simName: simName,
+          screenName: screenName
+        } );
+      }
+      else if ( isMultiScreenSimDisplayingSingleScreen && screenName ) {
+        return screenName;
+      }
+      else {
+        return simName;
+      }
+    }, {
+      tandem: Tandem.GENERAL_MODEL.createTandem( 'displayedSimNameProperty' ),
+      phetioValueType: StringIO
+    } );
 
     // Local variable is settable...
     const browserTabVisibleProperty = new BooleanProperty( true, {

[samreid]

@zepumph and I collaborated on the patch above. I re-reviewed it and tested it, and it seems complete and correct. I tested changing the availableScreens in studio and saw that this value updated correctly. I also confirmed that I could trace the dependencies and change the name of the sim entirely (whether it had one availableScreen or more).

I think this issue can be closed. @kathy-phet do you want further design review?

[zepumph]

Can you also do a quick test with ?screens=2 to make sure that looks the same and is editable in studio?

[samreid]

Testing in studio with ?screens=2, the title initially appears correctly, and is editable by tracing back to gravityAndOrbits.general.model.displayedSimNameProperty.dependencies.gravityAndOrbits-general-model-strings-joist-simTitleWithScreenNamePatternStringProperty

However, launching with ?screens=2 then changing availableScreensProperty at runtime gives this assertion error:

Do we need to change Joist so that in brand=phet-io, all screens are validValues and the ?screens query parameter just sets availableScreens on startup?

[zepumph]

I think, "no, please no." We only make the screens provided in the query string, I just wanted to confirm the editability you reported in the first paragraph. All good on my side!

[samreid]

@zepumph agreed the behavior is as expected and the issue can be closed, thanks!