phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

NumberDisplay: uninstrument Text, fix stringProperty dependencies #812

Open pixelzoom opened 1 year ago

pixelzoom commented 1 year ago

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

Over in https://github.com/phetsims/natural-selection/issues/339, I attempted to uninstrument the valueText and valueText.stringProperty PhET-iO elements in NumberDisplay, and add valueStringProperty. This quickly turned into a big effort, and I punted. In addition to having to update API and migration rules for many (most?) PhET-iO sims, valueText.stringProperty is missing an important dependency - the string pattern that gets filled in.

I'll also note that NumberDisplay has become quite the mess. I was the original author, and I almost didn't recognize it. So much chaos has been introduced that it might be better to rewrite, rather than revise.

Assigning to @kathy-phet for prioritization, assign, and decide whether this blocks anything.

@samreid @zepumph @brent-phet @arouinfar FYI.

pixelzoom commented 1 year ago

This has become an issue for instrumentation of Acid-Base Solutions, see https://github.com/phetsims/acid-base-solutions/issues/197.

zepumph commented 1 year ago

From design meeting today, we decided to:

We recognized that there could be a desire to see what the display is showing, but due to the complexity, we don't feel like it is worth keeping around in the current code base.

For migration rules, the current stringProperty is a DerivedProperty, so it will just need to be a TandemFragmentDelete.

This is NOT blocking Natural Selection because they uninstrumented the few NumberDisplays in that sim.

zepumph commented 1 year ago

This is likely blocking Acid Base Solutions. Scheduling a few weeks out (MK out next week)

pixelzoom commented 1 year ago

This is likely blocking Acid Base Solutions. ...

It's also possible (and my recommendation, but up to @arouinfar) to uninstrument the one NumberDisplay that appears in Acid-Base Solutions. See https://github.com/phetsims/acid-base-solutions/issues/197#issuecomment-1664545901.

arouinfar commented 1 year ago

This work will block the publication of Greenhouse Effect, see https://github.com/phetsims/greenhouse-effect/issues/341.

Greenhouse Effect will likely be be ready for dev testing this week. I don't think this should block dev testing, but it should be addressed before taking SHAs for RC testing.

Edit: we may be able to uninstrument the NumberDisplays in Greenhouse Effect instead. I'll update my comment once we decide.

Edit 2: we decided to uninstrument the NumberDisplays, so this no longer blocks Greenhouse Effect.

zepumph commented 9 months ago

I wonder if https://github.com/phetsims/scenery-phet/issues/828 is going to make this issue obsolete. Either way. I never got to this because it never became blocking for an instrumentation of a sim. It will be best to work on this when in the context of a sim that I can test and commit changes against.