problems with API for StepBackButton and StepForwardButton

[pixelzoom]

Noticed while adding buttons to the scenery-phet demo.

Multiple problems with the API for StepBackButton and StepForwardButton, which have these constructors:

  /**
   * @param {function} stepFunction
   * @param {Property.<boolean>} enabledProperty
   * @param {Object} [options]
   * @constructor
   */
  function StepBackButton( stepFunction, enabledProperty, options );

  /**
   * @param {function} stepFunction
   * @param {Property.<boolean>} playingProperty - button is disabled when this is true
   * @param {Object} [options]
   * @constructor
   */
  function StepForwardButton( stepFunction, playingProperty, options );

(1) Parameters that do the same thing have different names: enabledProperty vs playingProperty.

(2) StepBackButton and StepForwardButton are subtypes of PushButton, which has its own enabledProperty. Why do these buttons need an extra enabledProperty? All other buttons in scenery-phet require the client to use the enabledProperty provided by the button.

(3) Why is stepFunction a required parameter? It's likely that there function() {} is a reasonable default. If that's the case, then it should be an option.

(4) Due to 1-3, the API for these buttons looks different than other scenery-phet buttons.

Assigning to @samreid for comment, since I believe he was the original author. (I factored out StepButton recently.)

[pixelzoom]

Looking more closely at (1) above... playingProperty actually has inverted semantics. The button is disabled when when true.

[pixelzoom]

(5) I also see this in both buttons:

    assert && assert( !options.listener, 'stepFunction replaces options.listener' );
    options.listener = stepFunction;

Why is this difference necessary? Why can't we just use options.listener, as for other buttons?

[samreid]

(1) Yes those parameters should be unified. I'm not sure whether we should go with isPlayingProperty, or enabledProperty. I don't know why these diverged.
(2) I think this was just a way to disable the buttons when the sim is in "play" (not pause) mode. It was assuming the sim already had a property for that. As described in (1) we should make this consistent.
(3) It seems incorrect for button listeners to be optional. Why would a simulation have a button that does nothing? However, there is precedent for the listener being passed as an option (perhaps for uniformity with other options and so it is a named parameter), so I'm ok to stick with that precedent.
(4) I agree
(5) Sure, sounds like a good plan to use options.listener as for other buttons.

[pixelzoom]

I'll assign this to me to clean up, low priority.

[pixelzoom]

Check list, for what I need to change and test...

Sims that use StepBackButton:

• neuron

Sims that use StepForwardButton:

• bending-light
• color-vision
• energy-forms-and-changes
• energy-skate-park-basics
• fluid-pressure-and-flow
• forces-and-motion-basics
• gravity-and-orbits
• molecules-and-light
• neuron
• pendulum-lab
• rutherford-scattering
• states-of-matter
• wave-on-a-string

[pixelzoom]

In the above commits, I removed the stepFunction constructor parameter for StepForwardButton and StepButton, and used options.listener instead. I adjusted all call sites, and tested all sims listed in #243 (comment). API is now:

 /**
   * @param {Property.<boolean>} enabledProperty
   * @param {Object} [options]
   * @constructor
   */
  function StepBackButton( enabledProperty, options );

  /**
   * @param {Property.<boolean>} playingProperty - button is disabled when this is true
   * @param {Object} [options]
   * @constructor
   */
  function StepForwardButton( playingProperty, options );

[pixelzoom]

(6) The only reason why we currently need separate "forward" and "back" buttons is because of the phetsims/sun#236 (a problem with buttons in general). Working around this problem requires different values for the xContent option.

[pixelzoom]

If we fixed phetsims/sun#236, then we really don't need StepForwardButton and StepBackButton. We add a playProperty option to supertype StepButton, for clients that want the convenience of disabling the button when playProperty.value === true. Other clients could use button.enabled direction (neuron is an example of where this would be more appropriate.)

Here's what a typical call site would look like:

var playingProperty = new Property( false );

var stepForwardButton = new StepButton( {
  direction: 'forward',
  listener: function() { .... },
  playingProperty: playingProperty
} );


var stepBackButton = new StepButton( {
  direction: 'back',
  listener: function() { .... },
  playingProperty: playingProperty
} );

[samreid]

If we fixed phetsims/sun#236, then we really don't need StepForwardButton and StepBackButton.

In fact, we could move the content offset into StepButton (checks the direction to determine the offset) even if we don't fix phetsims/sun#236 and then we wouldn't really need StepForwardButton and StepBackButton. But on the other hand, I'm wondering if it is simpler for developer to look for StepForwardButton, or StepButton and know they must supply a direction parameter.

There is some precedent for this pattern in HBox, which is a trivial subclass of LayoutBox whose sole purpose is to provide { orientation: 'horizontal' }, but new HBox reads a bit more elegantly than new LayoutBox({orientation: 'horizontal'}).

[pixelzoom]

And here's an example (similar to neuron) where enabling/disabling the buttons involves more than whether the sim is playing.

var playingProperty = new Property( false );
var timeProperty = new Property( 0 );
var recordedTimeRange = new Range( 0, 0 );

var stepBackButton = new StepButton( {
  direction: 'forward',
  listener: function() { ... }
} );

var stepForwardButton = new StepButton( {
  direction: 'back',
  listener: function() { ... }
} );

var multilink = new Multilink( [ playingProperty, timeProperty ],
  function( playing, time ) {
    stepBackButton.enabled = ( playing && time > recordedTimeRange.min );
    stepForwardButton.enabled = ( playing &&  time < recordedTimeRange.max );
  } );

[pixelzoom]

Add options playingProperty to StepButton, fixed up and tested all call sites.

[pixelzoom]

StepBackButton renamed to StepBackwardButton.

StepForwardButton and StepBackwardButton added to scenery-phet demo.

All work completed here. @samreid would you mind reviewing?

[samreid]

• scenery-phet demo looks good
• this assertion should have a comment (in both occurrences): assert && assert( !options.direction );
• Why not simplify this extend call? Currently written as:

StepButton.call( this, _.extend( {}, options, { direction: 'backward' } ) );

Why not write as the following, since we already asserted that direction doesn't appear in the options?

StepButton.call( this, _.extend( { direction: 'backward' }, options ) );

• These lines read kind of confusingly to me:

var BUTTON_RADIUS = ( options && options.radius ) ? options.radius : 20;

Would it be better to use this?

var BUTTON_RADIUS = options && options.radius || 20;

Also in both of these logics, it is impossible to provide a radius of 0 because it is falsy. Perhaps that is a good thing in this case though.

• I skimmed through usage sites and didn't see any trouble.
• I tested Wave on a String and all seems well.

[samreid]

I made a few of the recommendations above, will wait to discuss the "These lines read kind of confusingly to me:" part with @pixelzoom

[samreid]

Here's an implementation that allows 0 radius values:

options && typeof options.radius==='number' ? options.radius : 20

I didn't grok the original proposal right away, but really I'm fine with any of the proposed solutions for this (including the original).

[pixelzoom]

Re the computation of BUTTON_RADIUS in #243 (comment)...

This one doesn't do what you think it does. It returns true or false, not a radius value.

var BUTTON_RADIUS = options && options.radius || 20;

I'm fine with either of these, but prefer the first (the current implementation) since a zero radius is almost certain to cause RoundPushButton to fail.

var BUTTON_RADIUS = ( options && options.radius ) ? options.radius : 20;
var BUTTON_RADIUS = ( options && typeof options.radius==='number' ) ? options.radius : 20;

[samreid]

Let's stick with the current implementation then.

But just to clarify in case there is a misunderstanding, the JavaScript || operator does not always return a boolean. Here is a transcript from my console:

If this makes sense to you, then please close the issue.

[pixelzoom]

Got it, thanks for the clarification. Closing.