Move instrumented textProperty from IO Type to core type

[zepumph]

From #1046 and largely related to #1047, we want to remove the phet-io specific Property that exists in TextIO and RichTextIO. It would be best if Text.js and RichText could be refactored to run on TinyProperty. Likely this would be a similar pattern as implemented for visibleProperty over in #1046.

I want to put this on hold until #1046 is more thoroughly vetted. If we need to use this pattern 5 times (Text.text, RichText.text, Node.visible, Node.opacity, Node.pickable), the likely we may want to change how much logic is needed to accomplish it.

[zepumph]

This is blocking This is blocking phetsims/tandem#210

[zepumph]

would be best if Text.js and RichText could be refactored to run on TinyProperty.

Oops, this work happened as part of #490. This issue is really focused on getting rid of PhET-iO instrumented text Properties implemented out of the IO type and into the core type.

[samreid]

@zepumph and I discussed how to apply the pattern for visibleProperty to Text.textProperty and RichText.textProperty. Some topics:

• Will textProperty need to be instrumented if we are using https://github.com/phetsims/studio/issues/192 for text customization? Or maybe text should default to phetioReadOnly: true
• How can we share code between Text/RichText? We cannot use the modern pattern for mixins until Text and RichText use class

[samreid]

I applied the pattern from visibleProperty to Text.textProperty. I tested in Natural Selection like so:

Index: js/common/view/EnvironmentalFactorsPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/EnvironmentalFactorsPanel.js	(revision bd63acdecbd514f293e742612a4854f7fd89e3df)
+++ js/common/view/EnvironmentalFactorsPanel.js	(date 1602899306510)
@@ -8,6 +8,7 @@
  */
 
 import Property from '../../../../axon/js/Property.js';
+import StringProperty from '../../../../axon/js/StringProperty.js';
 import merge from '../../../../phet-core/js/merge.js';
 import AssertUtils from '../../../../phetcommon/js/AssertUtils.js';
 import AlignGroup from '../../../../scenery/js/nodes/AlignGroup.js';
@@ -43,10 +44,15 @@
       tandem: Tandem.REQUIRED
     }, NaturalSelectionConstants.PANEL_OPTIONS, options );
 
+    const myTextProperty = new StringProperty('salut',{
+      tandem: options.tandem.createTandem('bonjourProperty')
+    });
+
     const titleNode = new Text( naturalSelectionStrings.environmentalFactors, {
       font: NaturalSelectionConstants.TITLE_FONT,
       maxWidth: 175, // determined empirically,
-      tandem: options.tandem.createTandem( 'titleNode' )
+      tandem: options.tandem.createTandem( 'titleNode' ),
+      textProperty: myTextProperty
     } );
 
     // To make all checkbox labels have the same effective size

It created the correct linkedProperty. I also generated generate-phet-io-api before and after, and they came out the same.

[samreid]

I applied the same pattern to RichText. Now there are 100 lines that are identical between RichText.js and Text.js, and they are highly similar with visibleProperty in Node.js. I considered:

• Using the old pattern for mixins, but I am not sure of costs or risks involved in that pattern. It would at least make navigation/autocomplete readability worse.
• Using the new pattern for mixins, which would be possible after these use ES6, this would also make navigation/autocomplete/readability worse.
• Marking with line comments on every line that those lines should be maintained together
• Factoring out method bodies or parts of method bodies to shared common code

I'll move the "factor out shared code" to a side issue and request review of what's been done so far.

[zepumph]

It does seem like this could largely benefit from some factored out logic. Thanks for making #1097. I have no other thoughts for this issue. Thanks for implementing!

I think this issue further adds priority to https://github.com/phetsims/studio/issues/192, but that is more of a medium-term goal that we should do after more clean-up.

Anything else here?

[zepumph]

Oh! I feel like we should create factored out unit tests for these Texts. Perhaps we could reuse blocks of code from NodeTests.js. What do you think?

[zepumph]

• @samreid, over in Move enabledProperty into Node #1100 I factored out a way to test an added Property to Node. It looks like

  scenery/js/nodes/NodeTests.js

  Line 266 in daa3529

  const testInstrumentedNodeProperty = ( assert, nodeField, nodeProperty, nodePropertySetter, ownedPropertyPhetioInstrumented ) => {

  , feel free to change/refactor further to get it to work in Text if you'd like.

[samreid]

I don't think we should add Text and RichText sub-Properties tests like we have done for the NodeTests. I think this issue can be closed, but feel free to reopen/disagree.