Instrument FineCoarseSpinner with PhET-iO

[jessegreenberg]

Instrumentation was requested in phetsims/forces-and-motion-basics#274, but I have less familiarity with PhET-iO so I would like this to have its own issue for changes and review. Over slack I asked for assistance and got a nice list of steps for this process from @zepumph and @samreid, so I will start there.

[jessegreenberg]

Here is the list provided:
Note that ArrowButton is already instrumented because it is a RectangularPushButton. The same for NumberDisplay

1. Use the "studio" link in phetmarks to test as you go.
2. Make FineCoarseSpinner take an optional tandem and pass a tandem into it in FAMB.
3. Pass that tandem into all ArrowButtons and NumberDisplay.
4. See how far that takes you in terms of customization capabilities.
5. You will also want to look at the data stream in the console: colorized link in phetmarks to see if events are being generated for each function you would expect for that component (like buttons firing, the value changing, etc).
6. Much of this is scattered around https://github.com/phetsims/phet-io/blob/master/doc/how-to-instrument-a-phet-simulation-for-phet-io.md but is a bit more than your case warrants. The doc is mainly desired as a start-to-finish guide for an entire sim.
7. Request a code review when you are complete
8. Feel free to request to work with someone else as you are starting out

[jessegreenberg]

I think this is done with the above commits. To summarize, I

• Passed tandems to ArrowButtons and NumberDisplay of FineCoarseSpinner.
• Passed tandem to the enabledProperty, if it wasn't provided in options to FineCoarseSpinner (like Slider.js)
• Dispose ArrowButtons NumberDisplay and enabledProperty to unregister tandems.
• Pass tandem to FineCoarseSpinner in FAMB and scenery-phet demo

I tested with studio and event console and wrapper unit tests and built phet-io branded FAMB, everything seems to be working.

@ariel-phet can you please assign a reviewer? Probably either @zepumph or @samreid, otherwise @pixelzoom because he is the author of FineCoarseSpinner?

[pixelzoom]

I'll have a look.

[pixelzoom]

Looks great, but one problem. If a client does this, either by misplacing the tandem for FineCoarseSpinner or attempting to set the tandem for ArrowButtons ....

const spinner = new FineCoarseSpinner( ..., {
  arrowButtonOptions: {
    tandem: tandem.createTandem( ... )
  }
} );

.... then options.arrowButtons.tandem will be assigned to each of the ArrowButton instances.

Recommended to:

(1) assert && assert( option.arrowButtonOptions.tandem === undefined, ... )

(2) When assembling options for each ArrowButton instance, use this form:

      const leftFineButton = new ArrowButton( 'left', function() {
        valueProperty.value = valueProperty.value - options.deltaFine;
      }, _.extend( {}, fineButtonOptions,  { tandem: options.tandem.createTandem( 'leftFineButton' ) } ) );

[pixelzoom]

@jessegreenberg said:

• Pass tandem to FineCoarseSpinner in FAMB and scenery-phet demo

How did you test this? scenery-phet demo is currently not marked as supporting phet-io in package.json. And when I attempt to run the demo with .../phet-io-wrappers/studio/?sim=scenery-phet&phetioValidateTandems&phetioDebug it fails with "Uninstrumented code detected" in some non-FineCoarseSpinner code.

[pixelzoom]

I was able to test in FAMB because it's instrumented. So I'm guessing that this was tested in FAMB, but not in scenery-phet demo (which is probably OK).

[jessegreenberg]

If a client does this, either by misplacing the tandem for FineCoarseSpinner or attempting to set the tandem for ArrowButtons ....

Sounds good, and similarly for NumberDisplay, right?

How did you test this?

Sorry, not tested in scenery-phet demo. I searched for usages of FineCoarseSpinner to see where tandems were needed (since tandem is required) and noticed some instrumentation in the scenery-phet demo.

[pixelzoom]

Sounds good, and similarly for NumberDisplay, right?

Yes.

assert && assert( options.numberDisplayOptions.tandem === undefined, 
  'FineCoarseSpinners sets numberDisplayOptions.tandem' );
options.numberDisplayOptions = _.extend( {
  tandem: options.tandem.createTandem( 'numberDisplay' )
}, options.numberDisplayOptions );

[pixelzoom]

Or if you prefer:

options.numberDisplayOptions = options.numberDisplayOptions || {};
assert && assert( options.numberDisplayOptions.tandem === undefined, 
  'FineCoarseSpinners sets numberDisplayOptions.tandem' );
options.numberDisplayOptions.tandem = options.tandem.createTandem( 'numberDisplay' );

[jessegreenberg]

OK, thanks @pixelzoom, back to you.

[pixelzoom]

👍 Closing.

[pixelzoom]

Reopening for phetsims/gas-properties#191.

Based on how we're instrumenting sims these days, FineCoarseSpinner seems over-instrumented. For example, all 4 buttons and the NumberDisplay can be hidden independently, resulting in some really unusable configurations, like:

Here's an example of the current level of instrumentation, from gas-properties:

FineCoarseSpinner is currently used only by gas-properties and forces-and-motion-basics, neither of which has been published as a PhET-iO version. So now would be the time to revise this.

@arouinfar Would you like to revisit the instrumentation of FineCoarseSpinner?

[pixelzoom]

I should also mention that NumberSpinner has a similar over-instrumentation style. Both buttons and the NumberDisplay are fully instrumented, can be hidden separately etc. This is probably a symptom of the "instrument everything" approach that PhET followed for awhile. Or it may be a "feature" for being able to convert a NumberSpinner to a "display" by hiding the buttons (which does not explain why hiding the NumberDisplay is useful) -- not a good way to implement such a feature.

NumberPicker seems to have a much more reasonable PhET-iO instrumentation.

And in any case, NumberSpinner, NumberPicker, and FineCoarseSpinner are all "spinners" and should have similar PhET-iO instrumentations. See #460.

[pixelzoom]

@arouinfar and I discussed this in the context of phetsims/gas-properties#30.

A better PhET-iO API for spinners would be:

• No instrumentation of the NumberDisplay
• No instrumentation of the individual buttons
• A Property that allows you to show/hide all buttons, to turn the spinner into a display.