Should Text/RichText visibleProperty be instrumented by default?

[pixelzoom]

Node visibleProperty is currently instrumented by default. Text/RichText is often used to label things (titles, checkboxes, radio buttons...) where hiding the label doesn't make sense. So for Text/RichText, I'm having to opt-out for almost every instrumented instance, using phetioVisiblePropertyInstrumented: false.

Here's some data from the 3 sims that I've just revised. As you can see, there was only 1 case where I did not opt out (and that 1 case was questionable).

sim	Text instances	Text opt-out	RichText instances	RichText opt-out
natural-selection	30	29	0	0
geometric-optics	7	7	1	1
molecule-polarity	26	26	0	0

And while I was compiling these numbers, I found (and fixed) a couple of errors where I should have specified phetioVisiblePropertyInstrumented: false. So instrumenting Text/RichText visibleProperty by default is probably resulting in making it possible to hide some text that shouldn't be hidden.

So I'm questioning whether instrumenting opt-out is what PhET really wants for Text/RichText visibleProperty.

[samreid]

I agree it would not be a good use of time to visit each site and have to specify phetioVisiblePropertyInstrumented: false or even phetioReadOnly:false. It sounds like the main argument from this issue is that texts are rarely "standalone" features that would make sense to hide without hiding their context. That makes sense to me. I'm interested to hear from @zepumph too.

[kathy-phet]

1. I agree, if its possible to just not instrument visible property on these String Properties by default, that is good. Is it relatively easy to bake change the default?

2. If someone really wants to "hide" the string itself. They can just put in a blank string or space - but that seems like they wouldn't do that. They don't need visible property to do that.

[kathy-phet]

Oh - And I don't think we thought about this aspect - so you won't find this set to false the the two sims that were first converted to dynamic string.

[zepumph]

Yes I would love to simplify the API like this patch. I think this issue is also very similar to #1443 where we are realizing that most instances of Text should not create an instrumented textProperty because that should just link back to i18n model strings through Derived/PatternStringProperties.

Index: js/nodes/Text.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/Text.ts b/js/nodes/Text.ts
--- a/js/nodes/Text.ts	(revision edb9a2bf1639e8b83bca4ea09a97ca32ce20daa4)
+++ b/js/nodes/Text.ts	(date 1662485551645)
@@ -104,7 +104,8 @@
     const definedOptions = extendDefined( {
       fill: '#000000', // Default to black filled text
       tandem: Tandem.OPTIONAL,
-      phetioType: Text.TextIO
+      phetioType: Text.TextIO,
+      phetioVisiblePropertyInstrumented: false
     }, options );
 
     if ( typeof text === 'string' || typeof text === 'number' ) {
Index: js/nodes/RichText.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/RichText.ts b/js/nodes/RichText.ts
--- a/js/nodes/RichText.ts	(revision edb9a2bf1639e8b83bca4ea09a97ca32ce20daa4)
+++ b/js/nodes/RichText.ts	(date 1662485551625)
@@ -334,7 +334,8 @@
     const options = optionize<RichTextOptions, Pick<SelfOptions, 'fill'>, NodeOptions>()( {
       fill: '#000000',
       tandem: Tandem.OPTIONAL,
-      phetioType: RichText.RichTextIO
+      phetioType: RichText.RichTextIO,
+      phetioVisiblePropertyInstrumented: false
     }, providedOptions );
 
     if ( typeof text === 'string' || typeof text === 'number' ) {

I'm just worried about the fallout in 2 ways:

1. redundant usages of opting out ( phetioVisiblePropertyInstrumented: false) that can be removed
2. Any spot where we in fact do want this behavior, but don't realize it at this time, and will silently remove that instrumentation, updating the API files, and moving on.

I made this chunk of code that can help us catch case one, but not sure about how acceptable it is to just do nothing about number 2.

Index: js/nodes/Text.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/Text.ts b/js/nodes/Text.ts
--- a/js/nodes/Text.ts	(revision edb9a2bf1639e8b83bca4ea09a97ca32ce20daa4)
+++ b/js/nodes/Text.ts	(date 1662485926385)
@@ -101,6 +101,10 @@
     this._isHTML = false; // TODO: clean this up
     this._cachedRenderedText = null;
 
+    if ( options && options.hasOwnProperty( 'phetioVisiblePropertyInstrumented' ) ) {
+      console.log( new Error().stack );
+    }
+
     const definedOptions = extendDefined( {
       fill: '#000000', // Default to black filled text
       tandem: Tandem.OPTIONAL,

[samreid]

In the patch, did you intend to say phetioTextPropertyInstrumented?

[zepumph]

Nope, this issue is about visibleProperty of texts, I believe you are thinking of #1443.

[samreid]

redundant usages of opting out ( phetioVisiblePropertyInstrumented: false) that can be removed

Here is a patch that detects when an option matches a default, it could be adapted to detect cases that can be removed here

Index: js/optionize.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/optionize.ts b/js/optionize.ts
--- a/js/optionize.ts	(revision b430b4d2fbebe0ee7bbc4667404431f17ba495dd)
+++ b/js/optionize.ts	(date 1662585326137)
@@ -60,7 +60,24 @@
 // TODO: "Limitation (I)" How can we indicate that a required parameter (for ParentOptions) will come in through defaults and/or providedOptions? Note: required parameters for S will not come from defaults.  See https://github.com/phetsims/chipper/issues/1128
 
 // Factor out the merge arrow closure to avoid heap/cpu at runtime
-const merge4 = ( a: IntentionalAny, b?: IntentionalAny, c?: IntentionalAny, d?: IntentionalAny ) => merge( a, b, c, d );
+const merge4 = ( a: IntentionalAny, b?: IntentionalAny, c?: IntentionalAny, d?: IntentionalAny ) => {
+
+  const defaults = a;
+
+  assert && Object.keys( defaults ).forEach( key => {
+    if ( b && defaults[ key ] === b[ key ] ) {
+
+      console.log( 'option matched the default in 2nd arg: ' + key + ': ' + defaults[ key ] );
+    }
+    if ( c && defaults[ key ] === c[ key ] ) {
+      console.log( 'option matched the default in 3rd arg: ' + key + ': ' + defaults[ key ] );
+    }
+    if ( d && defaults[ key ] === d[ key ] ) {
+      console.log( 'option matched the default in 4th arg: ' + key + ': ' + defaults[ key ] );
+    }
+  } );
+  return merge( a, b, c, d );
+};
 
 // ProvidedOptions = The type of this class's public API (type of the providedOptions parameter in the constructor)
 // SelfOptions = Options that are defined by "this" class. Anything optional in this block must have a default provided in "defaults"

Any spot where we in fact do want this behavior, but don't realize it at this time, and will silently remove that instrumentation,

That is a question for phet-io designers. Should we just implement the change, make a PSA and make sure they double check as each sim is released? This seems it would match our convention of erring on the side of under-instrumentation, so seems safe to me.

I'm happy to (a) commit the patch from #1447 (comment), (b) eliminate redundant options for this key and schedule a PSA, sound OK?

[zepumph]

At design meeting today, we agreed with @pixelzoom that a default uninstrumented visibleProperty for Text/RichText is excellent default behavior!

[samreid]

I would like to write a first draft for this issue, removing other assignees for now.

[samreid]

Thanks for catching this. I think you are right, and I should probably restore all of those and add phetioVisiblePropertyInstrumented: true in the corresponding places. Still, I'd like to confirm this is the desired behavior with @arouinfar before spending time on it. @arouinfar please skim through the commits labeled "Prune overrides file" and confirm whether we want to reinstrument + re-feature some or all of those.

[arouinfar]

@samreid @zepumph I reviewed the "prune overrides" commits, and it's a bit of a mixed bag. I question whether some of these overrides needed to be there in the first place, and in other cases, using an empty string would be functionally the same. In general, though, I think most of those commits can be reverted.

There are a few situations where we would want phetioVisiblePropertyInstrumented: true rather than using an empty string:

• The StringProperty is connected to several labels in the sim, and you want to hide a specific instance, such as the "Water" label on the FaucetNode in pH Scale.
• A label has several dependencies and it would be inconvenient to edit them individually. It's also likely that setting individual dependencies to an empty string would result in unwanted changes elsewhere. An example of this case is the "Force on m1 by m2" text in Gravity Force Lab.
• If an empty string will leave unwanted empty space, it may be preferable to use a visibleProperty instead. For example, the "Environmental Factors" panel title in Natural Selection.
  

@samreid please revert these commits:

• phetsims/natural-selection@02fe05d
• phetsims/gravity-force-lab@1c7e2a2
• phetsims/ph-scale@bc022eb
• phetsims/ph-scale-basics@855e6f5
• phetsims/energy-forms-and-changes@4cb9143

The changes made to states-of-matter, states-of-matter-basics, and friction can stay. The SOM(B) cases seem like odd choices, and I don't think they need to be there. Friction can be handled with empty strings, so the book titles don't need to have their own visibleProperty.

[samreid]

I restored the pruned parts, and added phetioVisiblePropertyInstrumented: true where appropriate. It would be good for @arouinfar to double check the metadata in studio when you have time.

[arouinfar]

Thanks @samreid. I reviewed in master, and the requested visibleProperty have all been restored.