Closed pixelzoom closed 9 months ago
Resolving this problem with the current NumberDisplay API doesn't seem reasonable or desirable. It will result in more workarounds on top of an API that is long overdue for replacement. So I'll repeat what I said in https://github.com/phetsims/scenery-phet/issues/812#issue-1827117013:
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.
@samreid Let's discuss in the context of https://github.com/phetsims/axon/issues/441.
In https://github.com/phetsims/scenery-phet/commit/60364e4574b0e3af71abf12815bf80685a5d47fc, @samreid temporarily silenced these problems by adding accessNonDependencies: true
.
I'm planning to investigate this. Solo-assigning for now.
Issue https://github.com/phetsims/scenery-phet/issues/814 is likely a symptom of this.
I leveraged the patch in #814 and used the new tooling in https://github.com/phetsims/axon/issues/441 to make sure coverage was complete. This is working well in Kepler's Laws.
However, I feel we should check in with @pixelzoom before committing. Also, do not commit the full patch above because it is optimized for Kepler's Laws and a general commit probably would to specify accessNonDependencies: true
until other cases are addressed.
I’m using the “dev:help-wanted” label to indicate a synchronous conversation would be best. Maybe we can schedule a short time after a coming standup. Or we could request help from another dev after check-in on Thursday's dev meeting.
@samreid and I discussed the patch in https://github.com/phetsims/scenery-phet/issues/824#issuecomment-1813580399. While it does the job, we both feel like it's a relatively ugly workaround that neither of us really wants to apply to NumberDisplay - see https://github.com/phetsims/scenery-phet/issues/828. What is really needed is a next-generation NumberDisplay, with an API that is better suited to displaying a string that is derived from a TReadOnlyProperty<number | null>
and other dependencies (like translated strings, preferences, etc.)
So we decided to:
accessNonDependencies: true
in NumberDisplayAt today's dev meeting, we agreed to go ahead with the patch. I'll update it and commit it, and @pixelzoom agreed to review.
1/4/24 dev meeting summary:
additionalPropertiesForUpdate
to additionalDependencies
, or something like what @zepumph has used for description optionsLooks like I called it a11yDependencies
in AccessibleValueHandler.
Then additionalDescriptionDependencies
for ISLC:
So I think just anything with dependencies
sounds nice.
@pixelzoom and I reviewed my proposed changes and agreed it is ready to push.
We observed that all calls to setNumberFormatter
look like a workaround trying to solve this same problem. @pixelzoom will look at BLL and Fourier to see if the new option is preferable. It could clean things up to get rid of setNumberFormatter.
OK the work is pushed. Over to @pixelzoom for a closer look.
In the above commits, I replaced workarounds (typically link or multilink) with use of numberFormatterDependencies
. This cleaned things up nicely.
It also made it possible to delete these methods:
setNumberFormatter
and recomputeText
setNumberFormatter
and redrawNumberDisplay
setNumberFormatter
and redrawNumberDisplay
Assigning back to @samreid for a final look. Feel free to close if OK.
Note the changes to wave-interference in https://github.com/phetsims/wave-interference/commit/a7fc9a3e6e9d41ab889f4456bedf02fa983aa40d and https://github.com/phetsims/wave-interference/commit/6dd8dc0a386646736b4645b4a75d6f52fb8582d2. The stopwatch does update when switching scenes. But I found it odd that switching scenes puts the stopwatch back in the toolbox.
I skimmed through the commits and they all look good. I think this issue can be closed.
Discovered in https://github.com/phetsims/axon/issues/441, in the context of gas-properties (and gases-intro, diffusion) and keplers-laws ...
NumberDisplay has a problem with missing dependencies for these DerivedProperties:
In all 3 of these Derivations,
valueToString
uses anumberFormatter
that may rely on other Properties.In the case of gas-properties, gases-intro, and diffusion, this problem manifests through StopwatchNode. Here's the relevant code for the
numberFormatter
, and the missing dependencies:In keplers-laws, the problem manifests in EllipticalOrbitNode, where each
numberFormatter
has dependencies on StringProperties. Here's the relevant code: