Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NumberDisplay: uninstrument Text, fix stringProperty dependencies #812

Open
pixelzoom opened this issue Jul 28, 2023 · 6 comments
Open

NumberDisplay: uninstrument Text, fix stringProperty dependencies #812

pixelzoom opened this issue Jul 28, 2023 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 28, 2023

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

Over in phetsims/natural-selection#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
Copy link
Contributor Author

This has become an issue for instrumentation of Acid-Base Solutions, see phetsims/acid-base-solutions#197.

@zepumph
Copy link
Member

zepumph commented Aug 3, 2023

From design meeting today, we decided to:

  • Uninstrument the text
  • Uninstrument the stringProperty associated with the text
  • Add a linked element for the underlying Number Property, run off of an option like Checkbox.phetioLinkProperty
  • In cases like NumberSpinner, where there is nothing but the linked Property in numberDisplay, uninstrument it in NumberSpinner.
  • In cases like NumberControl, when where we could have a visibleProperty for the numberDisplay, then opt out of the linkage, since it is redundant to the NumberControl linked element.

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
Copy link
Member

zepumph commented Aug 3, 2023

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

@zepumph zepumph assigned zepumph and unassigned kathy-phet Aug 3, 2023
@pixelzoom
Copy link
Contributor Author

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 phetsims/acid-base-solutions#197 (comment).

@arouinfar
Copy link

arouinfar commented Aug 7, 2023

This work will block the publication of Greenhouse Effect, see phetsims/greenhouse-effect#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
Copy link
Member

zepumph commented Dec 22, 2023

I wonder if #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.

@zepumph zepumph removed their assignment Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants