phetsims / scenery-phet

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

PhET-iO design for Stopwatch and StopwatchNode #827

Closed pixelzoom closed 9 months ago

pixelzoom commented 9 months ago

In https://github.com/phetsims/keplers-laws/issues/220, changes that @arouinfar requested for Kepler's Laws:

  • [ ] stopwatchNode
  • Seems buggy to allow clients to change the visibility of the buttons. Do their visibleProperties need to be instrumented?
  • resetButton.enabledProperty should be read-only

stopwatchNode is common-code component StopWatchNode. We can't make these changes without changing the API of other PhET-iO sims. And it looks like StopWatchNode has not had a proper PhET-iO design.

pixelzoom commented 9 months ago

As a workaround for the request in https://github.com/phetsims/keplers-laws/issues/220, I added playPauseButtonOptions and resetButtonOptions to StopwatchNode. Then I used those options to customize as requested for Kepler's Laws, without changing the PhET-iO API of other sims. Those options are useful to have going forward. But StopwatchNode still needs a PhET-iO design.

pixelzoom commented 9 months ago

This API design should also include Stopwatch, the associated model element. I've updated the issue title accordingly.

pixelzoom commented 9 months ago

In https://github.com/phetsims/keplers-laws/issues/217, @arouinfar requested that all of Properties in Stopwatch be featured. @AgustinVallejo did that in https://github.com/phetsims/scenery-phet/commit/ea2803ca07028d90d35a61de18f4b50f5d7f3b56, and it (surprisingly) resulted in no changes to PhET-iO "stable" APIs.

pixelzoom commented 9 months ago

Discussed with @arouinfar. She would like to move these overrides that we added in KeplersLawsScreenView to StopwatchNode. We suspect that there are no stable PhET-iO sims that currently have a StopwatchNode, and that Kepler's Laws is the first. I'll do this.

    const stopwatchNode = new StopwatchNode(
...
        // See https://github.com/phetsims/keplers-laws/issues/220
        playPauseButtonOptions: {
          phetioVisiblePropertyInstrumented: false,
          phetioEnabledPropertyInstrumented: false
        },

        // See https://github.com/phetsims/keplers-laws/issues/220
        resetButtonOptions: {
          phetioVisiblePropertyInstrumented: false,
          phetioEnabledPropertyInstrumented: false
        },
pixelzoom commented 9 months ago

In the above commits, I moved the defaults from KeplersLawsScreenView to StopwatchNode. grunt generate-phet-io-api --stable confirmed that no API files were changed, so I will go ahead and close this issue.