Slider PhET-iO problems custom thumbNode and trackNode

[pixelzoom]

Broken by afbfeab.

molecule-polarity : phet-io-wrappers-tests : assert
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/phet-io-wrappers/phet-io-wrappers-tests.html?sim=molecule-polarity&phetioDebug
3 out of 4 tests passed. 1 failed.
SimTests: molecule-polarity: iframe api failed:
Uncaught Error: Uncaught Error: Assertion failed: Passed-in thumbNode must have the correct tandem. Expected: moleculePolarity.twoAtomsScreen.view.electronegativityPanels.atomAElectronegativityPanel.slider.thumb, actual: moleculePolarity.twoAtomsScreen.view.electronegativityPanels.atomAElectronegativityPanel.slider.thumbNode
Error: Assertion failed: Passed-in thumbNode must have the correct tandem. Expected: moleculePolarity.twoAtomsScreen.view.electronegativityPanels.atomAElectronegativityPanel.slider.thumb, actual: moleculePolarity.twoAtomsScreen.view.electronegativityPanels.atomAElectronegativityPanel.slider.thumbNode
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/assert/js/assert.js:25:13)
at new Slider (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/sun/js/Slider.js:200:17)
at new HSlider (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/sun/js/HSlider.js:27:5)
at new ElectronegativitySlider (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/molecule-polarity/js/common/view/ElectronegativitySlider.js:73:5)
at new ElectronegativityPanel (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/molecule-polarity/js/common/view/ElectronegativityPanel.js:62:20)
at new TwoAtomsScreenView (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/molecule-polarity/js/twoatoms/view/TwoAtomsScreenView.js:55:41)
at TwoAtomsScreen.createView (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/molecule-polarity/js/twoatoms/TwoAtomsScreen.js:42:16)
at TwoAtomsScreen.initializeView (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/joist/js/Screen.js:253:23)
at Array.<anonymous> (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/joist/js/Sim.js:924:16)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1618239924819/joist/js/Sim.js:932:25

If you pass in a custom trackNode or thumbNode via options, they must now have standard tandem names.

Problems:

• This apparently was not tested or fixed for sims that use trackNode or thumbNode.
• There's no documentation about this with the trackNode and thumbNode options.
• There are no constants for clients to use for the standard tandem names. Are you expecting clients to duplicate the 'track' and 'thumb' literals?

[pixelzoom]

Sam Reid 9:58 AM
Yes, as the assertion says, it expects the tandem to be “thumb”, not “thumbNode”. Sorry we neglected to check for inconsistencies before committing.

Chris Malley 9:58 AM
Since the options are trackNode and thumbNode, I think this is a very bad idea. That violates tandem naming conventions.

Sam Reid 9:59 AM
Are you recommending we change the tandem name to “trackNode” etc?

Chris Malley 9:59 AM
Yes

[pixelzoom]

Search for thumbNode:, trackNode:, options.thumbNode =, and options.trackNode =, to find usages that need to be inspected and possibly changed.

[zepumph]

This issue seems to be a great example of how poorly phet-io works with dependency injection. Above is what I was forced to do in order to fix the assertion error in a CL usage. This is a really really awkward example, perhaps some of the worst we may find for this case. A common code type (ISLCObjectControlPanel) composes a NumberControl, and then a subtype of that panel (ChargeControl) provides a custom thumb to the slider in said NumberControl. Since the parent defines the numberControl tandem, and the common code NumberControl defines the slider tandem, we have a lot of duplication of tandem names to support the same tandem.

Perhaps we have too strict a constraint? On the other hand, if the Slider tandem/default thumb tandem ever changes out from under this, that will fail the assertion too, so while it is more maintenance, the above commit is still always a loud error upon failure.

@samreid do you have any recommendations about how this could be better.

[pixelzoom]

Instead of options thumbNode and trackNode, you could have functions that create those subcomponents. Then make Slider responsible for passing the correct tandem to those functions.

  // {function(option:Object):Node
  createThumbNode: null,
  createTrackNode: null,

On second thought, I don't like that either. It really limits the constructor signture for these custom nodes. For example, in Fourier AmplitudeSlider.js, I have these nested classes for custom thumb and track:

class GrippyThumb extends Node {
  constructor( thumbSize, harmonic ) {

class BarTrack extends SliderTrack {
  constructor( trackSize, harmonic, amplitudeRange ) {

[samreid]

We discussed creating functions that take a tandem and return a component in https://github.com/phetsims/phet-io/issues/1617 and it was not among our favorites (for that problem).

[samreid]

In the commits, I changed the names, added documentation to indicate the required tandem names, and I visited the instrumented cases at thumbNode:, trackNode:, options.thumbNode =, and options.trackNode =. @zepumph can you please review?

[pixelzoom]

Two things:

• This was not tested thoroughly. FAMB fails to start in Studio, because of a bug in MotionControlPanel.js. It's also failing CT.

• There are no constants for these standard tandem names. I expected to see something like Slider.THUMB_NODE_TANDEM_NAME and Slider.TRACK_NODE_TANDEM_NAME. Instead, literals 'thumbNode' and 'trackNode' have been copied everywhere. Using constants at a call site is more verbose, but tells the reader that there’s a standard tandem name being used. 'thumbNode' at a call site tells me nothing.

I'll fix these things and assign back to @samreid for review.

[pixelzoom]

Fixed in the above commits.

Over to @samreid for review.

[zepumph]

https://github.com/phetsims/coulombs-law/blob/2518a5cd5fc2649df81ec9fc91cbf5214530cc23/js/common/view/ChargeControl.js#L83

This still seems unacceptable to me, @samreid by closing this issue are you recommending that we keep this line because even though there is duplication we still have an assertion letting us know when it needs to be updated? If you still feel good about it please re-close.

[pixelzoom]

Yeah, I agree with @zepumph. ChargeControl.js is awful. Again:

83   tandem: options.tandem.createTandem( 'numberControl' ).createTandem( 'slider' ).createTandem( Slider.THUMB_NODE_TANDEM_NAME )

Besides the verbose grossness... That 'slider' literal is copied from line 150 in NumberControl:

150     tandem: options.tandem.createTandem( 'slider' )

So the code in ChargeControl.js is relying on internal (private) details of NumberControl. If that ever changes in NumberControl, someone must know to change 'slider' in ChargeControl. That's just not going to happen, and is going to break the tandem hierarchy.

[samreid]

Is the proposal to factor out 'slider' (to sun) and 'numberControl' (to ISLC) string constants? Can those be done in a separate issue? Aside from those two steps, what do you recommend?

[pixelzoom]

No, not quite.

There's no need to factor out 'numberControl', that's a sim-specific choice in ChargeControl.js. It would have been just as correct to name it 'chargeNumberControl'.

Promoting 'slider' to a constant belongs in NumberControl, not Slider. It's a convention of NumberControl that its Slider subcomponent has tandem name 'slider'. So something like:

NumberControl.SLIDER_TANDEM_NAME = 'slider';

After that, we end up with this in ChargeControl.js:

83   tandem: options.tandem.createTandem( 'numberControl' ).createTandem( NumberControl.SLIDER_TANDEM_NAME ).createTandem( Slider.THUMB_NODE_TANDEM_NAME )

This addresses the possibility of breaking things because of relying on internal details of NumberControl.

It does not address the original concern, which @zepumph summarized nicely in #694 (comment):

This issue seems to be a great example of how poorly phet-io works with dependency injection.

I don't know how to address that problem.

[pixelzoom]

Yikes. Things are even worse than I thought in ChargeControl.js. Again:

83   tandem: options.tandem.createTandem( 'numberControl' ).createTandem( NumberControl.SLIDER_TANDEM_NAME ).createTandem( Slider.THUMB_NODE_TANDEM_NAME )

That 'numberControl' literal must match the literal over in ISLCObjectControlPanel.js:

111         tandem: tandem.createTandem( 'numberControl' )

Change one of these and the tandem hierachy is broken.

There are 3 problems here:

• inherently ugly method of injecting a tandem name into a subcomponent (PhET-iO)
• sloppy copying of tandem name literals (sim specific)
• some really odd code design (sim specific)

[zepumph]

Change one of these and the tandem hierarchy is broken.

Yes but it is a loud error, because we are testing for equality in places like

sun/js/Slider.js

Lines 201 to 205 in 1c65b74

	if ( Tandem.VALIDATION && options.thumbNode ) {
	assert && assert( options.thumbNode.tandem.equals( thumbTandem ),
	`Passed-in thumbNode must have the correct tandem. Expected: ${thumbTandem.phetioID}, actual: ${options.thumbNode.tandem.phetioID}`
	);
	}

, so presumably it would be simple enough to handle as we went. Even so I elected to create a couple more constants for clarity.

Assertion failed: Passed-in thumbNode must have the correct tandem. Expected: coulombsLaw.macroScreen.coulombsLawMacroScreen.view.charge1Control.numberControl.slider.thumbNode, actual: coulombsLaw.macroScreen.coulombsLawMacroScreen.view.charge1Control.numberControl.ohHi.thumbNode

Anything else here? Feel free to close @pixelzoom.

[pixelzoom]

Anything else here? Feel free to close @pixelzoom.

Yes. Please summarize what you did so that I don't have to deduce from comments and multiple commits.

[zepumph]

The commits above factored out duplicated tandem names into constants to fix the gross line in ChargeControl. It used the same pattern that you used for Slider tandem names (like the second checkbox in #694 (comment)).

[zepumph]

See https://github.com/phetsims/coulombs-law/blob/160d8ac87e30dc657e279d0f40edf464c7659399/js/common/view/ChargeControl.js#L84-L85 for what it looks like now

[pixelzoom]

Thanks for clarifying.

So we went from this:

tandem: options.tandem.createTandem( 'numberControl' ).createTandem( 'slider' ).createTandem( 'thumb' )

to this:

tandem: options.tandem.createTandem( ISLCObjectControlPanel.NUMBER_CONTROL_TANDEM_NAME )
          .createTandem( NumberControl.SLIDER_TANDEM_NAME ).createTandem( Slider.THUMB_NODE_TANDEM_NAME )

This likely does not address your comment in #694 (comment), but I don't have any other suggestions. It's still gross, but less likely to get broken now that you're using constants.

I don't see anything else that needs to be done here, so closing.