Uninstrument Text and RichText

[zepumph]

In https://github.com/phetsims/phet-io/issues/1952#issuecomment-1664495275 we decided that we generally don't want to instrument Text nodes, and instead just want to focus on the stringProperties, especially if they are derived or pattern.

• [x] Let's double check that sim code isn't instrumenting Texts unless there is a reason (like hiding the Text).
• [x] Let's also look for common code UI that needs to be generally updated to support this new PhET-iO instrumented decision. (For example Natural Selection dealt with NumberDisplay and OopsDialog)

Most likely the important things are for general sim/screen names, and for HomeScreen.

[arouinfar]

I reviewed general and homeScreen for usages for Text and would recommend that we uninstrument these Text nodes:

• [ ] ~general.view.navigationBar.titleText~
• [x] general.view.navigationBar.*ScreenButton.text
• [x] homeScreen.view.buttonGroup.*ScreenButton.text
• [x] homeScreen.view.titleText

I also reviewed instances of StringProperty outside of general.model.strings and found these in the About dialog. These all link back to read-only strings, so I recommend uninstrumenting these too.

• [x] aboutDialogCapsule.archetype.termsPrivacyAndLicensingLinkTextStringProperty
• [x] aboutDialogCapsule.archetype.thirdPartyCreditsLinkTextStringProperty
• [x] aboutDialogCapsule.archetype.translationCreditsLinkTextStringProperty

Also, autoselect is working well on the navbar. The sim title routes users to general.model.displayedSimNameProperty which is desirable (and should remain instrumented). Autoselecting the screen buttons in the navbar takes me directly to the appropriate StringProperty and bypasses the view instrumentation.

I'm not sure who should be on the hook for these changes. Maybe @zepumph? Maybe @pixelzoom since Acid-Base Solutions is likely next up in the queue?

[pixelzoom]

If this ends up on my worklist, I won't be able to get to it until >= August 28. It's a little too cross-cutting for me to tackle while I'm on reduced hours.

[samreid]

Today @kathy-phet asked if I could take the lead, and I agreed. @zepumph can review. This will be part of the CaV and greenhouse SHAs.

[samreid]

Today we agreed that the navigation bar title text should be hide-able.

[samreid]

Fixed in the commits, @arouinfar can you please review/test? @zepumph would you like to code review? If we need migration rules (deletes), we can write them lazily.

[arouinfar]

Thanks @samreid! We reviewed with @kathy-phet @samreid @zepumph in Studio and the changes look good, closing.

[samreid]

In slack, @kathy-phet said:

One thing we forgot re joist issue is that we are supposed to make the migration rules at this time as well for each sim that is published. @samreid can you do that please.

[samreid]

This patch makes it possible for a Natural Selection 1.5 standard wrapper (published today) to load in local natural selection studio 1.6:

```diff Subject: [PATCH] Rename string keys, see https://github.com/phetsims/center-and-variability/issues/373 --- Index: repos/natural-selection/js/natural-selection-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/repos/natural-selection/js/natural-selection-migration-rules.ts b/repos/natural-selection/js/natural-selection-migration-rules.ts --- a/repos/natural-selection/js/natural-selection-migration-rules.ts (revision 80cdb36bf3cd8aa114bc6b2d2c4080da8f5a39d4) +++ b/repos/natural-selection/js/natural-selection-migration-rules.ts (date 1692302411240) @@ -270,5 +270,12 @@ // needs to be set by the state engine. This rule should be done last (after all renames) so that the phetioID // matches the new API. See further documentation in PhetioTypeDelete.ts. new PhetioTypeDelete( 'SimInfoIO' ) + ], + '1.6.0': [ + new TandemFragmentDelete( 'naturalSelection.homeScreen.view.titleText.stringProperty' ), + new TandemFragmentDelete( 'naturalSelection.general.view.navigationBar.introScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'naturalSelection.general.view.navigationBar.labScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'naturalSelection.homeScreen.view.buttonGroup.introScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'naturalSelection.homeScreen.view.buttonGroup.labScreenButton.text.stringProperty' ) ] }; \ No newline at end of file ```

@zepumph Is this the right approach?

[zepumph]

@zepumph Is this the right approach?

We haven't come to a consensus over in https://github.com/phetsims/phet-io-sim-specific/issues/27 about how to best handle migration rules for this. If we have decided to add migration rules to sims when the breaking change is introduced, then the above looks correct, but is incomplete. This is our current list of sims that support migration (PhET-iO Hydrogen), so they would each need to get rules. Right?

Gravity And Orbits
pH Scale
pH Scale: Basics
Friction
Beer's Law Lab
Concentration
Molecule Shapes
Molecule Shapes: Basics
Circuit Construction Kit: DC
Circuit Construction Kit: DC - Virtual Lab
Calculus Grapher
Geometric Optics
Geometric Optics: Basics
Density
Molecule Polarity
Graphing Quadratics
Natural Selection

[kathy-phet]

We had decided we would try writing migration rules for each sim that needs it at the time that change is made, and we would report back on how that went. This is the first such change - since we bailed on the other common code changes we had consider in the number display. So @samreid - yes, please put a migration rule in place for each of these sims, in "main".

[samreid]

I'm happy to do that, but would like to confirm with @zepumph that we don't need a LinkedElement related rule for these. Without the rules, trying to load a 1.5 standard wrapper into 1.6 gives this error:

assert.js:24 Assertion failed:  Impossible set state from iterate; unset state: {
  "naturalSelection.general.view.navigationBar.introScreenButton.text.stringProperty": {
    "elementID": "naturalSelection.general.model.strings.naturalSelection.screen.introStringProperty"
  },
  "naturalSelection.general.view.navigationBar.labScreenButton.text.stringProperty": {
    "elementID": "naturalSelection.general.model.strings.naturalSelection.screen.labStringProperty"
  },
  "naturalSelection.homeScreen.view.titleText.stringProperty": {
    "elementID": "naturalSelection.general.model.strings.naturalSelection.naturalSelection.titleStringProperty"
  },
  "naturalSelection.homeScreen.view.buttonGroup.introScreenButton.text.stringProperty": {
    "elementID": "naturalSelection.general.model.strings.naturalSelection.screen.introStringProperty"
  },
  "naturalSelection.homeScreen.view.buttonGroup.labScreenButton.text.stringProperty": {
    "elementID": "naturalSelection.general.model.strings.naturalSelection.screen.labStringProperty"
  }
} 

So they do look like linked elements because of the elementID. It it correct that LinkedElements used to be stateful, so I need to knock them out with a TandemFragmentDelete? I just want to confirm this for natural selection before applying this elsewhere. If we really do just need TandemFragmentDelete, should I abstract out regex or patterns for copy/pasting out to different sims? Are we at a point where we can experiment with putting

1.6.0: [
    ...UNINSTRUMENT_JOIST_TEXT // see https://github.com/phetsims/joist/issues/934
]

With an explanatory code comment there?

[pixelzoom]

Self assigning to look at the proposal in https://github.com/phetsims/joist/issues/934#issuecomment-1683305471, in hopes of unblocking https://github.com/phetsims/tandem/issues/303.

[pixelzoom]

As @zepumph noted in https://github.com/phetsims/joist/issues/934#issuecomment-1683073309, migration rules for this issue were only added to a subset of sims. That's causing problems with addressing other unrelated migration issues, like https://github.com/phetsims/tandem/issues/303 which is blocked. We need to avoid getting into situations like this where the main branch is essentially broken. @kathy-phet Please consider elevating the priority of migration testing in CT.

That said... I reviewed https://github.com/phetsims/joist/issues/934#issuecomment-1683305471 ...

Re:

... So they do look like linked elements because of the elementID. It it correct that LinkedElements used to be stateful, so I need to knock them out with a TandemFragmentDelete? I just want to confirm this for natural selection before applying this elsewhere. If we really do just need TandemFragmentDelete, should I abstract out regex or patterns for copy/pasting out to different sims?

I don't know. Consult with @zepumph. If we need them, then yes, add them to all migration-rules.ts files where they are relevant.

Re:

Are we at a point where we can experiment with putting

1.6.0: [ ...UNINSTRUMENT_JOIST_TEXT // see https://github.com/phetsims/joist/issues/934 ] With an explanatory code comment there?

We have previously decided that migration-rules.ts would only contain relevant rules (see ? strictMigrationRules) and they we'd update all migration-rules.ts for all affected sims when a PhET-iO change is made. Yes that's painful, but we also decided that a goal of this issue is to evaluate how painful. So let's please stick with that plan here, get this unblocked ASAP, and save the discussion of other approaches for another time, in another issue.

[samreid]

Here's a patch that seems to work OK for calculus grapher:

```diff Subject: [PATCH] Simplify NumberTone.play API, fix a bug in mean sound effect, see https://github.com/phetsims/center-and-variability/issues/491 --- Index: repos/calculus-grapher/js/calculus-grapher-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/repos/calculus-grapher/js/calculus-grapher-migration-rules.ts b/repos/calculus-grapher/js/calculus-grapher-migration-rules.ts new file mode 100644 --- /dev/null (date 1692709198252) +++ b/repos/calculus-grapher/js/calculus-grapher-migration-rules.ts (date 1692709198252) @@ -0,0 +1,29 @@ +// Copyright 2022, University of Colorado Boulder +// This PhET-iO file requires a license +// USE WITHOUT A LICENSE AGREEMENT IS STRICTLY PROHIBITED. +// For licensing, please contact phethelp@colorado.edu + +import TandemFragmentDelete from '../../../../phet-io-wrappers/common/js/migration/TandemFragmentDelete.js'; + +window.phetio = window.phetio || {}; + +// Use ${simName} in rules that are general and might be usable for other sims +const simName = 'calculusGrapher'; + +window.phetio.migrationRules = { + '1.2.0': [ + new TandemFragmentDelete( 'calculusGrapher.homeScreen.view.titleText.stringProperty' ), + + new TandemFragmentDelete( 'calculusGrapher.general.view.navigationBar.derivativeScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'calculusGrapher.homeScreen.view.buttonGroup.derivativeScreenButton.text.stringProperty' ), + + new TandemFragmentDelete( 'calculusGrapher.general.view.navigationBar.integralScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'calculusGrapher.homeScreen.view.buttonGroup.integralScreenButton.text.stringProperty' ), + + new TandemFragmentDelete( 'calculusGrapher.general.view.navigationBar.advancedScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'calculusGrapher.homeScreen.view.buttonGroup.advancedScreenButton.text.stringProperty' ), + + new TandemFragmentDelete( 'calculusGrapher.general.view.navigationBar.labScreenButton.text.stringProperty' ), + new TandemFragmentDelete( 'calculusGrapher.homeScreen.view.buttonGroup.labScreenButton.text.stringProperty' ) + ] +}; ```

After that change, the migration wrapper still reports:

assert.js:24 Assertion failed:  These phetioIDs are not found in the new API. Either they were renamed incorrectly or were not migrated at all:
calculusGrapher.advancedScreen.view.controlPanel.pushButtonGroup.smoothButton.text.stringProperty
calculusGrapher.labScreen.view.controlPanel.pushButtonGroup.smoothButton.text.stringProperty

[samreid]

For Calculus Grapher, I committed rules for the joist changes and for a missing smoothButton text. Now it's working in the migration wrapper.

[pixelzoom]

Thanks. But the problem with smoothButton text is a general problem with TextPushButton that should have been addressed in https://github.com/phetsims/sun/issues/854.

[zepumph]

Subset of https://github.com/phetsims/phet-io/issues/1960

[zepumph]

Alright. We can confirm that migration rules are now complete and working for this issue. Closing

[pixelzoom]

Reopening. These TODOs appear in beers-law-lab-migration-rules.ts and concentration-migration-rules.ts. This was discovered while working on https://github.com/phetsims/beers-law-lab/issues/332, and complicated that issue.

    // Delete items that no longer exist.
    // TODO: https://github.com/phetsims/joist/issues/934 Some of these should be renames or other rules

    // Uninstrument Text and RichText.  These were copied from beers-law-lab-migration-rules.ts
    // Delete items that no longer exist.
    // TODO: https://github.com/phetsims/joist/issues/934 Some of these should be renames or other rules. Note there are also unused rules since
    // this was copied from beers-law-lab

[pixelzoom]

Also wondering why this was not reopened automatically. Is the automated task still running that looks for TODOs?

[zepumph]

Great question. It was working in https://github.com/phetsims/greenhouse-effect/issues/361#issuecomment-1712467793. I will take a look.

[samreid]

I was going to work on this next, but the comment in https://github.com/phetsims/phet-io/issues/1963#issuecomment-1714788272 indicates that maybe @pixelzoom is working on it. So I'll self unassign and check in at standup tomorrow.

[pixelzoom]

I do not plan to do any further work on this. Or anything else related to migration until this is addressed.

[zepumph]

Also wondering why this was not reopened automatically. Is the automated task still running that looks for TODOs?

Ahh, because the feature relies on the lint rule called todo-should-have-issue, and whereever that rule is turned off, we don't have this feature.

https://github.com/phetsims/phet-io-sim-specific/blob/b5d1d086b5a06ad1199acce78056ceb6eb60689b/package.json#L19

[pixelzoom]

Why is "todo-should-have-issue" turned off in phet-io-sim-specific? Shall we turn it on?

[zepumph]

https://github.com/phetsims/tasks/issues/1128

[zepumph]

@samreid will take the lead on the TODO that is left.

[samreid]

It was addressed in https://github.com/phetsims/phet-io-sim-specific/commit/43e849416cec874431b41893f7bdb88f741ef0ff, closing.