tandem problems in WavelengthSlider

[pixelzoom]

Noted while working on #258... WavelengthSlider creates tandems for its ArrowButton instances:

     plusButton = new ArrowButton( 'right', function() {
        wavelength.set( wavelength.get() + 1 );
      }, {
        tandem: options.tandem && options.tandem.createTandem( 'plusButton' )
      } );

      minusButton = new ArrowButton( 'left', function() {
        wavelength.set( wavelength.get() - 1 );
      }, {
        tandem: options.tandem && options.tandem.createTandem( 'minusButton' )
      } );

Two problems:

(1) ArrowButton doesn't do anything with options.tandem, it has not been instrumented for PhET-iO. It will need to call tandem.addInstance and implement dispose.

(2) ArrowButton instances (plusButton, minusButton) are not cleaned up by WavelengthSlider.dispose.

@samreid Do you want to take care of this, or do you want me to handle it?

[samreid]

This would be good for @andrewadare to work on when he has time.

[pixelzoom]

I may take care of it, since I'm doing a lot of other work on WavelengthSlider (#211, #260, #261, ...) @andrewadare, if you start work on this, please let me know so that we don't collide too much. If I get to it before you, I'll let you know.

[samreid]

@pixelzoom I'm happy for you to work on this if you get to it before @andrewadare. To test addInstance(this,TNode) calls, you can launch instance-proxies.html for a sim that uses the node, and test toggling the "visible" and "pickable" checkboxes. Here's a link to the test pages: http://localhost/phet-io/html/dev-wrappers.html

[pixelzoom]

NumberControl has this identical problem -- it creates tandems for instances of ArrowButton (which doesn't support tandem), and then doesn't clean those buttons up in dispose.

Awhile back I proposed adding this assertion to all UI components that aren't instrumented:

assert && assert( !options.tandem, 'tandem is not supported' );

In light of the obvious error that I'm seeing here with ArrowButton, perhaps we should revisit that proposal.

[pixelzoom]

Ah... No tandem appears in the ArrowButton source code, but it is passed through to its supertype (RectangularPushButton), where it appears to be handled properly.

So the tandem is added, and there is a dispose call that will remove it. Need to fix WavelengthSlider and NumberControl to call dispose for its ArrowButton instances.

[pixelzoom]

I added dispose calls for the ArrowButton instances in WavelengthSlider and NumberControl, see above commit.

[pixelzoom]

Above I said:

No tandem appears in the ArrowButton source code, but it is passed through to its supertype (RectangularPushButton), where it appears to be handled properly.

Looking at ResetAllButton (a subtype of RoundPushButton), I see:

tandem: null // Marker entry to indicate that tandem is supported (in the parent)

@samreid Should ArrowButton have a similar "marker entry" in its options? Is this a convention that we're going to use?

[samreid]

I have mixed feelings about the marker and would like to discuss it before we decide. I suppose at the moment I'm leaning toward "delete them because they are duplicated information and it would be crazy to have marker entries for everything" though I do see the advantage of being able to easily see if something is instrumented in a parent class. Perhaps there is a better way to proceed.

[pixelzoom]

Discussion of "marker entry" for tandem moved to #262.

[pixelzoom]

@samreid Could you please review the changes mentioned in #259 (comment)? If all looks well, close this issue.

[samreid]

Looks good, there is just one issue to discuss. There are now 2 different ways of dealing with the presence of the arrow buttons.

// method 1, using tweakersVisible
if ( options.tweakersVisible ) {
      thisNode.addChild( plusButton );
      thisNode.addChild( minusButton );
    }

// method 2, using truthiness of plusButton and minusButton
      plusButton && plusButton.dispose();
      minusButton && minusButton.dispose();

It seems somewhat better to be consistent about these. I'm leaning toward method 2, perhaps we should update the method 1 sites to read more like so:

plusButton && thisNode.addChild(plusButton);
// etc.

Let me know your thoughts on the matter.

[pixelzoom]

There are multiple sites (3) that we'd need to change from

if ( options.tweakersVisible ) { ...}

to

plusButton && ...;
minusButton && ...;

I think these 2 sites would be less clear if changed:

138 if ( options.tweakersVisible ) {
      plusButton.left = track.right + 8;
      plusButton.centerY = track.centerY;
      minusButton.right = track.left - 8;
      minusButton.centerY = track.centerY;
    }

192 if ( options.tweakersVisible ) {
        plusButton.enabled = ( wavelength < options.maxWavelength );
        minusButton.enabled = ( wavelength > options.minWavelength );
      }

So I'm inclined to leave things "as is".

I'm also inclined to leave things "as is" because I suspect that WavelengthSlider is going to require a major rewrite to address #211 (support for non-visible wavelengths).

[samreid]

OK agreed, anything else to do here?

[pixelzoom]

All done, closing.