Uninstrument Text and RichText

[arouinfar]

For #323

Issues identified in https://github.com/phetsims/studio/issues/303 lead to a new way to autoselect strings in Studio. As a result, the recommendation in https://github.com/phetsims/phet-io/issues/1941 is that most instances of text do not need to be instrumented.

@pixelzoom subsequently uninstrumented text in other sims like phetsims/molecule-polarity#160 and phetsims/graphing-quadratics#187. Seems like we should do the same in Natural Selection. Do you agree @pixelzoom?

[pixelzoom]

Yes agreed.

[pixelzoom]

In the above commits, I uninstrumented Text (there are no RichText) and updated the API file. The next step will be to add migration rules. Before I do that, I'd like @arouinfar to review in Studio. Please assign back to me after review.

[pixelzoom]

All of these Text elements were apparently instrumented since the most recent (1.4) release, so they were never part of a published API. Therefore no migration rules are needed, as confirmed via the Migration wrapper. Great call on uninstrumenting these @arouinfar. Feel free to close this issue if everything looks good in Studio.

[arouinfar]

Thanks @pixelzoom. I searched for "text" and found a few remaining instances:

• view.dialogs.memoryLimitDialog.text
• view.graphs.proportionsNode.proportionsGraphNode.generationSpinner.numberDisplay.valueText

Back to you @pixelzoom.

[pixelzoom]

@arouinfar Both of the "text" elements that you've identified above are in common code. view.dialogs.memoryLimitDialog.text is in OopsDialog. generationSpinner.numberDisplay.valueText is in NumberDisplay. If I remove those, it will change the API of all sims, and require migration rules for all sims.

I recommend that we leave this "as is" for Natural Selection. And optionally open issue(s) to uninstrument Text/RichText in common code. @arouinfar thoughts?

[pixelzoom]

... And optionally open issue(s) to uninstrument Text/RichText in common code. ...

See https://github.com/phetsims/phet-io/issues/1952

[arouinfar]

I recommend that we leave this "as is" for Natural Selection. And optionally open issue(s) to uninstrument Text/RichText in common code. @arouinfar thoughts?

Agreed @pixelzoom. Thanks for opening https://github.com/phetsims/phet-io/issues/1952! Closing.

[pixelzoom]

Reopening. Over in https://github.com/phetsims/phet-io/issues/1952, we decided to address the common-code elements that appear in for Natural Selection 1.5:

• NumberDisplay
• OopsDialog

[pixelzoom]

In the above commits, I uninstrumented RichText in OopsDialog. This was relatively easy because natural-selection-phet-api.json is the only API that contained this element, and it was added since the 1.4 release, so could be removed without a migration rule.

[pixelzoom]

NumberDisplay quickly turned into a big effort. numberDisplay.valueText.stringProperty is missing the string pattern dependency, so there's much more work to be done in NumberDisplay than just uninstrument numberDisplay.valueText -- see phetsims/scenery-phet#812.

So for this sim, I simply uninstrumented generationSpinner.numberDisplay. The Studio tree used to look like this:

... and now it looks like this:

This seems OK to me for now. There's really no reason to make the numberDisplay invisible (its visibleProperty probably should have been readonly). And all of the info is available elsewhere. And we have not instrumented the other NumberDisplay instance in this sim, whcih would have been populationGraphNode.dataProbeNode.numberDisplay.

@arouinfar Please review. To summarize:

• memoryLimitDialog.text was uninstrumented
• memoryLimitDialog.memoryLimitMessageStringProperty has been added
• generationSpinner.numberDisplay was uninstrumented

[arouinfar]

Thanks @pixelzoom. The updates and tree structure look great.

I noticed that toggling generationSpinner.visibleProperty can create layout issues.

I don't remember seeing this before, but I might of missed it. Not sure if this is related to the changes made to generationSpinner here or if this is entirely unrelated issue.

[pixelzoom]

Unrelated to this issue (I reverted the above changes to confirm) and generationSpinner.visibleProperty has always been writeable. So I created #347.