Improvements to phet-io general tandem structure

[chrisklus]

From 04/09/20 design meeting:

• Uninstrument nameProperty for the HomeScreen and single screen sims' screen
• Unify title tandem naming as titleText. This includes homeScreen.view.title and general.view.navigationBar.titleTextNode.
• Refactor Sim.name as Sim.simNameProperty and instrument it. Also use to update text of titles mention above, and mark those title's textPropertys as phetioReadOnly: true
• Sim title and screen name should update when the nav bar is in the format of ?screens=1, and not include the dash if the screen name is changed to be an empty string.

Since this is about to introduce several changes to common phet-brand code and general phet-io tandems (and publications are on the near horizon), marking as blocks-publication.

[chrisklus]

Here's a working patch for item 3:

Index: js/simInfo.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/simInfo.js	(revision e06929f6471105741603f67a80e1f840646d25dc)
+++ js/simInfo.js	(date 1587046615000)
@@ -67,7 +67,7 @@
 
     // no need to add this again if the method has already been called
     if ( !info.simName ) {
-      putInfo( 'simName', sim.name );
+      putInfo( 'simName', sim.simTitleProperty.value );
       putInfo( 'simVersion', sim.version );
       putInfo( 'repoName', packageJSON.name );
       putInfo( 'screens', sim.screens.map( screen => {
Index: js/HomeScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/HomeScreenView.js	(revision e06929f6471105741603f67a80e1f840646d25dc)
+++ js/HomeScreenView.js	(date 1587049771000)
@@ -6,12 +6,12 @@
  * @author Sam Reid (PhET Interactive Simulations)
  */
 
-import PDOMPeer from '../../scenery/js/accessibility/pdom/PDOMPeer.js';
 import Bounds2 from '../../dot/js/Bounds2.js';
 import inherit from '../../phet-core/js/inherit.js';
 import merge from '../../phet-core/js/merge.js';
 import StringUtils from '../../phetcommon/js/util/StringUtils.js';
 import PhetFont from '../../scenery-phet/js/PhetFont.js';
+import PDOMPeer from '../../scenery/js/accessibility/pdom/PDOMPeer.js';
 import HBox from '../../scenery/js/nodes/HBox.js';
 import Node from '../../scenery/js/nodes/Node.js';
 import Text from '../../scenery/js/nodes/Text.js';
@@ -30,14 +30,14 @@
 const TITLE_FONT_FAMILY = 'Century Gothic, Futura';
 
 /**
- * @param {string} simName - the internationalized text for the sim name
+ * @param {Property.<string>} simTitleProperty - the internationalized text for the sim title
  * @param {HomeScreenModel} model
  * @param {Tandem} tandem
  * @param {Object} [options]
  * @constructor
  */
-function HomeScreenView( simName, model, tandem, options ) {
-  assert && assert( simName, 'simName is required: ' + simName );
+function HomeScreenView( simTitleProperty, model, tandem, options ) {
+  assert && assert( simTitleProperty.value, 'simTitleProperty is required: ' + simTitleProperty.value );
   const self = this;
 
   options = merge( {
@@ -53,14 +53,14 @@
     includePDOMNodes: false,
 
     // pdom
-    labelContent: simName,
+    labelContent: simTitleProperty.value,
     descriptionContent: StringUtils.fillIn( homeScreenDescriptionPatternString, {
-      name: simName,
+      name: simTitleProperty.value,
       screens: model.simScreens.length
     } )
   } );
 
-  const titleText = new Text( simName, {
+  const titleText = new Text( simTitleProperty.value, {
     font: new PhetFont( {
       size: 52,
       family: TITLE_FONT_FAMILY
@@ -68,7 +68,15 @@
     fill: 'white',
     y: 130,
     maxWidth: this.layoutBounds.width - 10, // To support PhET-iO Clients setting this
-    tandem: tandem.createTandem( 'titleText' )
+    tandem: tandem.createTandem( 'titleText' ),
+    phetioComponentOptions: {
+      textProperty: { phetioReadOnly: true }
+    }
+  } );
+
+  // update the titleText when the sim title changes
+  simTitleProperty.link( simTitle => {
+    titleText.setText( simTitle );
   } );
 
   // Have this before adding the child to support the startup layout.
Index: js/PhetMenu.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetMenu.js	(revision e06929f6471105741603f67a80e1f840646d25dc)
+++ js/PhetMenu.js	(date 1587047619000)
@@ -117,7 +117,7 @@
   Node.call( this );
 
   const aboutDialogCapsule = new PhetioCapsule( tandem => {
-    return new AboutDialog( sim.name, sim.version, sim.credits, sim.locale, phetButton, tandem );
+    return new AboutDialog( sim.simTitleProperty.value, sim.version, sim.credits, sim.locale, phetButton, tandem );
   }, [], {
     tandem: tandem.createTandem( 'aboutDialogCapsule' ),
     phetioType: PhetioCapsuleIO( DialogIO )
@@ -198,7 +198,7 @@
       present: isPhETBrand && !isApp,
       callback: function() {
         const url = 'http://phet.colorado.edu/files/troubleshooting/' +
-                    '?sim=' + encodeURIComponent( sim.name ) +
+                    '?sim=' + encodeURIComponent( sim.simTitleProperty.value ) +
                     '&version=' + encodeURIComponent( sim.version + ' ' +
                     ( phet.chipper.buildTimestamp ? phet.chipper.buildTimestamp : '(require.js)' ) ) +
                     '&url=' + encodeURIComponent( window.location.href ) +
Index: js/NavigationBar.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/NavigationBar.js	(revision e06929f6471105741603f67a80e1f840646d25dc)
+++ js/NavigationBar.js	(date 1587049771000)
@@ -24,6 +24,7 @@
  */
 
 import DerivedProperty from '../../axon/js/DerivedProperty.js';
+import Property from '../../axon/js/Property.js';
 import Dimension2 from '../../dot/js/Dimension2.js';
 import inherit from '../../phet-core/js/inherit.js';
 import StringUtils from '../../phetcommon/js/util/StringUtils.js';
@@ -36,8 +37,8 @@
 import HomeButton from './HomeButton.js';
 import HomeScreen from './HomeScreen.js';
 import HomeScreenView from './HomeScreenView.js';
-import joistStrings from './joistStrings.js';
 import joist from './joist.js';
+import joistStrings from './joistStrings.js';
 import NavigationBarScreenButton from './NavigationBarScreenButton.js';
 import PhetButton from './PhetButton.js';
 
@@ -115,15 +116,7 @@
   this.barContents = new Node();
   this.addChild( this.barContents );
 
-  let title = sim.name;
-
-  // If the 'screens' query parameter only selects 1 screen, than update the nav bar title to include that screen name.
-  if ( isMultiScreenSimDisplayingSingleScreen && this.simScreens[ 0 ].nameProperty.value ) {
-    title = StringUtils.fillIn( simTitleWithScreenNamePatternString, {
-      simName: sim.name,
-      screenName: this.simScreens[ 0 ].nameProperty.value
-    } );
-  }
+  let title = sim.simTitleProperty.value;
 
   // Sim title
   this.titleText = new Text( title, {
@@ -133,12 +126,33 @@
     phetioDocumentation: 'Displays the title of the simulation in the navigation bar (bottom left)',
     phetioComponentOptions: {
       visibleProperty: { phetioFeatured: true },
-      textProperty: { phetioFeatured: true }
+      textProperty: { phetioReadOnly: true }
     }
   } );
   this.titleText.setVisible( false );
   this.barContents.addChild( this.titleText );
 
+  // update the titleText based on values of the sim title and screen name
+  Property.multilink( [ sim.simTitleProperty, this.simScreens[ 0 ].nameProperty ],
+    ( simTitle, screenName ) => {
+
+      if ( isMultiScreenSimDisplayingSingleScreen && simTitle && screenName ) {
+        // If the 'screens' query parameter selects only 1 screen, than update the nav bar title to include that screen name.
+        title = StringUtils.fillIn( simTitleWithScreenNamePatternString, {
+          simName: simTitle,
+          screenName: screenName
+        } );
+      }
+      else if ( isMultiScreenSimDisplayingSingleScreen && screenName ) {
+        title = screenName;
+      }
+      else {
+        title = simTitle;
+      }
+
+      this.titleText.setText( title );
+    } );
+
   // @private - PhET button, fill determined by state of navigationBarFillProperty
   this.phetButton = new PhetButton(
     sim,
@@ -167,8 +181,8 @@
 
     // title can occupy all space to the left of the PhET button
     this.titleText.maxWidth = HomeScreenView.LAYOUT_BOUNDS.width - TITLE_LEFT_MARGIN - TITLE_RIGHT_MARGIN -
-                                  PHET_BUTTON_LEFT_MARGIN - this.a11yButtonsHBox.width - PHET_BUTTON_LEFT_MARGIN -
-                                  this.phetButton.width - PHET_BUTTON_RIGHT_MARGIN;
+                              PHET_BUTTON_LEFT_MARGIN - this.a11yButtonsHBox.width - PHET_BUTTON_LEFT_MARGIN -
+                              this.phetButton.width - PHET_BUTTON_RIGHT_MARGIN;
   }
   else {
     /* multi-screen sim */
@@ -262,7 +276,7 @@
 
     // Now determine the actual width constraint for the sim title.
     this.titleText.maxWidth = this.screenButtonsContainer.left - TITLE_LEFT_MARGIN - TITLE_RIGHT_MARGIN -
-                                  HOME_BUTTON_RIGHT_MARGIN - this.homeButton.width - HOME_BUTTON_LEFT_MARGIN;
+                              HOME_BUTTON_RIGHT_MARGIN - this.homeButton.width - HOME_BUTTON_LEFT_MARGIN;
   }
 
   // initial layout (that doesn't need to change when we are re-laid out)
Index: js/HomeScreen.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/HomeScreen.js	(revision e06929f6471105741603f67a80e1f840646d25dc)
+++ js/HomeScreen.js	(date 1587048598000)
@@ -21,7 +21,7 @@
 class HomeScreen extends Screen {
 
   /**
-   * @param {string} simName
+   * @param {Property.<string>} simTitleProperty
    * @param {function():Property.<Screen>} getScreenProperty - at the time of construction, the Sim.screenProperty is
    *                                                         - not yet assigned (because it may itself include the
    *                                                         - HomeScreen), so we must use a function to lazily get it
@@ -31,7 +31,7 @@
    * @param {Object} [options]
    * @constructor
    */
-  constructor( simName, getScreenProperty, simScreens, tandem, options ) {
+  constructor( simTitleProperty, getScreenProperty, simScreens, tandem, options ) {
 
     options = merge( {
 
@@ -47,7 +47,7 @@
 
     super(
       () => new HomeScreenModel( getScreenProperty(), simScreens, tandem.createTandem( 'model' ) ),
-      model => new HomeScreenView( simName, model, tandem.createTandem( 'view' ), _.pick( options, [
+      model => new HomeScreenView( simTitleProperty, model, tandem.createTandem( 'view' ), _.pick( options, [
         'warningNode'
       ] ) ),
       options
Index: js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Sim.js	(revision e06929f6471105741603f67a80e1f840646d25dc)
+++ js/Sim.js	(date 1587049045000)
@@ -18,6 +18,7 @@
 import ObservableArray from '../../axon/js/ObservableArray.js';
 import Property from '../../axon/js/Property.js';
 import PropertyIO from '../../axon/js/PropertyIO.js';
+import StringProperty from '../../axon/js/StringProperty.js';
 import timer from '../../axon/js/timer.js';
 import Bounds2 from '../../dot/js/Bounds2.js';
 import Dimension2 from '../../dot/js/Dimension2.js';
@@ -125,7 +126,12 @@
   options.rootRenderer = phet.chipper.queryParameters.rootRenderer || options.rootRenderer;
 
   // @public (joist-internal)
-  this.name = name;
+  this.simTitleProperty = new StringProperty( name, {
+    tandem: Tandem.GENERAL_VIEW.createTandem( 'simTitleProperty' ),
+    phetioFeatured: true,
+    phetioDocumentation: 'The title of the sim. Changing this value will update the title on the navigation bar and ' +
+                         'the home screen, if it exists.'
+  } );
 
   // 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
@@ -324,7 +330,7 @@
     screensQueryParameter,
     QueryStringMachine.containsKey( 'screens' ),
     selectedSimScreens => {
-      return new HomeScreen( this.name, () => this.screenProperty, selectedSimScreens, Tandem.ROOT.createTandem( 'homeScreen' ), {
+      return new HomeScreen( this.simTitleProperty, () => this.screenProperty, selectedSimScreens, Tandem.ROOT.createTandem( 'homeScreen' ), {
         warningNode: options.homeScreenWarningNode
       } );
     }
@@ -791,7 +797,7 @@
         screen.initializeModel();
       } );
       workItems.push( function() {
-        screen.initializeView( self.name, self.screens.length );
+        screen.initializeView( self.simTitleProperty.value, self.screens.length );
       } );
     } );
 

Before committing, I wanted to see if designers could consider any alternative names for simTitleProperty. While nameProperty would be the most idiomatic with how the code is structured for the previous usage of name, I think simNameProperty would still be a large improvement over simTitleProperty (it was mentioned that including sim was important because the Property was off on its own under general.view).

Throughout our project, sims are considered to have a name, not a title. I realize the motivation for using title was to match titleText under general.navigationBar.titleText and homeScreen.view.titleText, but having a discrepancy there seems okay to me because "titleness" feels like a characteristic that relates to how a string is presented (font size, location, etc).

Some examples of phet-brand and a11y discrepancies with simTitleProperty:

phetInfo.js

putInfo( 'simName', sim.simTitleProperty.value ); // previously sim.name
putInfo( 'simVersion', sim.version );
putInfo( 'repoName', packageJSON.name );
putInfo( 'screens', sim.screens.map( screen => {

HomeScreenView.js

// pdom
labelContent: simTitleProperty.value, // previously sim.name
descriptionContent: StringUtils.fillIn( homeScreenDescriptionPatternString, {
  name: simTitleProperty.value, // previously sim.name
  screens: model.simScreens.length
} )

Basically, my argument is that the desire for using simTitleProperty should be strong enough that we'd be willing to also change all usages of name to title in the context of a sim. Marking for discussion at phet-io meeting.

[chrisklus]

We discussed #627 (comment) in 04/16/20 phet-io meeting and concluded that nameProperty was indeed not specific enough, but simNameProperty sounds good.

[zepumph]

Uninstrument nameProperty for the HomeScreen and single screen sims' screen

• @chrisklus this means that the name Property for each Screen will never have a value of null, is that correct? If so perhaps it would worth investigating making its phetioType PropertyIO<StringIO>. I'm unsure how the validation will be in that case though. It may not be worth the effort. If that is changed, we can likely remove the creator in StudioInstanceCreator.

[chrisklus]

@zepumph and I just chatted on slack - since the Property needs null as a possible type too (in phet brand), but the phetioType applies validation regardless of if the Property is instrumented, the type will have to remain as PropertyIO( NullableIO( StringIO ) ).

[chrisklus]

All set - @arouinfar please review appearance and behavior in Studio for the checklist items in #627 (comment).

[arouinfar]

Looks good @chrisklus. I wonder if the documentation for general.view.navigationBar.titleText.textProperty and homeScreen.view.titleText.textProperty should mention that the sim name is changed via general.view.simNameProperty. I don't know if it's critical since the textProperties aren't featured, but the simNameProperty is.

[chrisklus]

Thanks @arouinfar!

I wonder if the documentation for general.view.navigationBar.titleText.textProperty and homeScreen.view.titleText.textProperty should mention that the sim name is changed via general.view.simNameProperty. I don't know if it's critical since the textProperties aren't featured, but the simNameProperty is.

Soon those textProperties will be gone and general.view.simNameProperty will be there instead as a linked element. It's possible that we'll still have one separate textProperty though, depending on how we handle the screens=1 case, so I'll keep that in mind.

Assigning to @zepumph or @samreid to review all four commits from this issue as they pertain to the checklist in #627 (comment)

[samreid]

simNameProperty is described as:

// @public {Property.<string>} (joist-internal)
  this.simNameProperty = new StringProperty( name, {

However, other code checks for its existence like so:

if ( isMultiScreenSimDisplayingSingleScreen && simName && screenName ) {

Can it really be falsy? If not, this logic should be simplified. If so, that should be documented and the type should probably be changed.

For the documentation of Screen.nameProperty, add documentation about when it is null.

  // @public (read-only) {Property<String|null>}
  this.nameProperty = new Property( options.name, {
    phetioType: PropertyIO( NullableIO( StringIO ) ),
    tandem: instrumentNameProperty ? options.tandem.createTandem( 'nameProperty' ) : Tandem.OPT_OUT,
    phetioFeatured: true,
    phetioDocumentation: 'The name of the screen. Changing this value will update the screen name for the screen\'s ' +
                         'corresponding button on the navigation bar and home screen, if they exist.'
  } );

Also, the sim "name" string seems like a model thing instead of a view thing. Did I miss a conversation about that? Should that choice be revisited? The titleText which is a NodeIO is a view thing, but the string text that is displayed there is more like a model thing.

Everything else looks good to me, but would be good to have @zepumph double check if he has time.

[chrisklus]

Can it really be falsy? If not, this logic should be simplified. If so, that should be documented and the type should probably be changed.

That code is inside a multilink, so its value will be falsy if phet-io was used to change the string to ''. The empty string is a valid value for Property.<string>, so the type doc seems reasonable to me. I updated the comment in that usage to clarify, let me know if you think more is needed here. In Studio you can play around with the values of the sim name and screen name for a multiscreen sim when screens=1 and observe the behavior on the nav bar.

For the documentation of Screen.nameProperty, add documentation about when it is null.

Committed above, thanks-I forgot to do that when I removed it from the phetioDocumentation.

Also, the sim "name" string seems like a model thing instead of a view thing. Did I miss a conversation about that? Should that choice be revisited? The titleText which is a NodeIO is a view thing, but the string text that is displayed there is more like a model thing.

I think designers proposed it during phet-io meeting and developers signed off. Model sounds good to me too, what do you think @arouinfar?

[arouinfar]

Also, the sim "name" string seems like a model thing instead of a view thing. Did I miss a conversation about that? Should that choice be revisited? The titleText which is a NodeIO is a view thing, but the string text that is displayed there is more like a model thing.

I think designers proposed it during phet-io meeting and developers signed off. Model sounds good to me too, what do you think @arouinfar?

I'm not sure I completely follow. Is the proposal that the simNameProperty should be moved to the model? This seems a bit incongruous with the various titleText elements in the view.

[samreid]

From a programming perspective, the simName is a data-oriented model value, and the titleText is a view of that model value. We are free to put that data where we wish, but I thought it would match our existing patterns and conventions to put simNameProperty in the model, unless we decided this is an exception to our rule.

@chrisklus the changes look good, thanks!

[chrisklus]

Thanks @samreid!

I'll add that when the textProperty of homeScreen.view.titleText is a linked element, it also would match our conventions for that to link back to the model. It doesn't make any difference on the code end though, let me know what your preference is @arouinfar.

[arouinfar]

Thank you for the explanation @samreid. I think it makes sense for the sim name to be in the model; no need to go against convention.

[chrisklus]

Sounds good, thanks @arouinfar. I pushed a simple change above and CT looks good, closing.