Instrument HSlider enabledProperty

[jessegreenberg]

From phetsims/forces-and-motion-basics#242. I can optionally pass in an instrumented enabledProperty, but should HSlider's default enabledProperty be instrumented?

[samreid]

Yes, the HSlider default enabledProperty should be instrumented, but I don't see a straightforward way to do so. If we add a tandem in the options declaration, it will register the property. Then if the enabledProperty is replaced by the options arguments, we would need to unregister it. Or we could wait until after the options extension to decide whether to create the HSlider enabledProperty, but that would be a bit unconventional and make the options harder to read.

@zepumph or @jessegreenberg can you make a recommendation?

[jessegreenberg]

Can tandems and associated properties be set dynamically or do they need to be set up during construction?

Would something like this work after the options extension?

    if ( !this.enabledProperty.tandem ) {
      this.enabledProperty.tandem = tandem.createTandem( 'enabledProperty' );
      this.enabledProperty.phetioValueType = TBoolean;
    }

[jessegreenberg]

Ah, it looks like Node might be the only thing with a getter for tandem.

[samreid]

As a convention, tandems are provided as constructor arguments and never changed. But we could delay creation of the Emitter until we are sure it is needed.

[jessegreenberg]

I see, so something like this before the options extension?

  // only create a tandemized Property if we know that it will be needed
  var enabledProperty = options.enabledProperty || new Property( true, { /*tandem stuff*/ } );

  options = _.extend( {
   
   enabledProperty: enabledProperty
    
  } );

[samreid]

Yes, that looks like it would work well, want to give it a try?

[jessegreenberg]

Sure! We will want to do this for enabledRangeProperty too.

[jessegreenberg]

Seems to be working but @samreid can you please take a look? The "tandem stuff" requires options.tandem to be defined, so they had to be wrapped in an extra conditional. What is the phetioValueType for the 'range' object passed to HSlider? Do we need something new for this like THSliderRange?

[samreid]

Yes, it seems we will need to create TRange. Also, the given implementation seems to be workable (after adding TRange), but these lines will need to be changed:

      enabledProperty: new Property( true ),
      enabledRangeProperty: new Property( range ), // controls the portion of the slider that is enabled

Also, it seems unfortunate that the values would have to be set outside of the options declaration, since that is the natural place for developers to look--but I couldn't find a palatable way to do so. Maybe I just need to look further...?

[samreid]

I created TRange and used it in HSlider, it seems to be appearing nicely in instance proxies.

[samreid]

Also, I'm wondering if should fill in those instrumented properties after the options in order to make sure the tandem gets filled in correctly (in the options extend call).

[samreid]

I'd love help finding a more elegant solution for this, if one exists, unassigning for now but would be good to collaborate with @jessegreenberg or @zepumph when our time matches up.

[zepumph]

This does seem messy! I committed above a small fix that is a bit more robust. Now I would like to talk through potential solutions.

Maybe we could have phet-io have a built in way of overriding something in the tandem registry. Currently we just have this line to protect against this:

      if ( validateTandems && wrapperMap.hasOwnProperty( phetioID ) ) {
        assert && assert( instance === wrapperMap[ phetioID ].instance, 'Cannot register two different instances to the same id: ' + phetioID );
      }

But if there was a way to opt out of that and say "Hey, I know that this tandem already exists. It was a default and I would quite like to overwrite it." Then we may be able to get away with business as usual. Maybe a flag like phetioDefaultTandemOverwritable (but event more verbose of course) that gets added to a type from the options.

I think it is worth mentioning that if we go down this road, it is yet one more phetio option that needs to be kept track of. Possible it would be good to try to organize all of those at some point to make development less confusing.

[samreid]

I tried to clean up the given implementation. We already know options exists after the extend call. We also know options.tandem exists because it is provided as a default or overriden. I also added a spot in the options for the two properties so clients will know to look for them below. I also switched to the || syntax in case it is clearer. Tested in instance proxies with concentration and it seems OK. Committing and reassigning to @zepumph for review.

[samreid]

It would be nice if we can avoid having to add something like overriding tandems or phetioDefaultTandemOverwritable. Having "assigned once and correct on instantiation" is a great simplifying factor.

[zepumph]

Looks very nice. I am much happier with this solution that where it was an hour ago. I don't think that there are many Types where this will take place, and so I think this is fine, as long as it is well documented.

I agree in favor of solutions without adding additional flags like phetioDefaultTandemOverwritable. Closing.