Add a StepBackwardButton to TimeControlNode
Closed this issue · 32 comments
@veillette mentioned that the time control nodes in collision-lab include a StepBackwardButton phetsims/collision-lab#43.
It would be useful for TimeControlNode to also include this, this group looks like it could be used more than once.
https://user-images.githubusercontent.com/7633344/70754080-47ce5280-1d04-11ea-8b49-cc9157b66a6e.png
I asked slack:
Jesse Greenberg 5:47 PM
Any objections to adding a StepBackwardButton to scenery-phet/TimeControlNode?
Chris Malley 5:48 PM
As an option? What is the layout?
Jesse Greenberg 5:48 PM
Yes, as an option. To the left of the PlayPauseButton like this:
(image above)
Chris Malley 5:50 PM
Instantiated and added to the scenegraph conditionally?
Jesse Greenberg 5:50 PM
Correct, as is curently done for the normal/slow radio buttons
Chris Malley 5:50 PM
Sounds great.
StepBackwardButton added in f0e2586. stepOptions renamed to stepForwardOptions to go with stepBackwardOptions in b7bdb96. Rest of commits are for the option rename at usages in sims.
@ariel-phet can you please assign a developer to review this change?
@pixelzoom could you review?
Changes look good overall, but 2 problems that need to be addressed:
(1) If a StepBackwardButton is created, it is not cleaned up in this.disposeTimeControlNode. That will leave the button registered with PhET-iO, creating a memory leak.
You'll need to keep a reference to your StepBackwardButton instance, then call dispose in this.disposeTimeControlNode:
let stepBackwardButton = null;
if ( options.includeStepBackwardButton ) {
stepBackwardButton = new StepBackwardButton( merge( {
tandem: options.tandem.createTandem( 'stepBackwardButton' )
}, stepButtonOptions, options.stepBackwardOptions ) );
buttons.unshift( stepBackwardButton );
}
// in this.disposeTimeControlNode:
stepBackwardButton && stepBackwardButton.dispose();(2) These merge calls both have the same potential problems:
const stepForwardButton = new StepForwardButton( merge( {
tandem: options.tandem.createTandem( 'stepForwardButton' )
}, stepButtonOptions, options.stepForwardOptions ) );
const stepBackwardButton = new StepBackwardButton( merge( {
tandem: options.tandem.createTandem( 'stepBackwardButton' )
}, stepButtonOptions, options.stepBackwardOptions ) );It's possible for the client to override tandem and isPlayingProperty, e.g.:
const isPlayingProperty1 = new BooleanProperty( true );
const isPlayingProperty2 = new BooleanProperty( true );
const isPlayingProperty3 = new BooleanProperty( true );
new TimeControlNode( isPlayingProperty1, {
includeStepBackwardButton: true,
stepBackwardOptions: {
tandem: someTandem.createTandem( 'mySpecialStepBackwardButton' ),
isPlayingProperty: isPlayingProperty2
},
stepForwardOptions: {
tandem: someTandem.createTandem( 'mySpecialStepForwardButton' ),
isPlayingProperty: isPlayingProperty3
}
} );I can imagine maybe wanting to change the tandem name. But it's a stretch, doesn't match with PhET's policy of standardizing tandem names where possible for common-code components, and it would be difficult to make the buttons looks like they are nested under the TimeControlNode -- since the TimeControlNode's tandem needs to create the tandems for its subcomponents. So I don't recommend allowing tandem override.
I cannot imagine allowing the client to override isPlayingProperty, That would make it possible for each button to be interacting with a different instance of isPlayingProperty. The results would certainly be interesting but undesirable.
The easiest way to address this is to add assertions:
assert && assert( !options.stepForwardOptions.tandem, '...' );
assert && assert( !options.stepForwardOptions.isPlayingProperty, '...' );
assert && assert( !options. stepBackwardOptions.tandem, '...' );
assert && assert( !options. stepBackwardOptions.isPlayingProperty, '...' );Back to @jessegreenberg.
Thanks for review!
You'll need to keep a reference to your StepBackwardButton instance, then call dispose in this.disposeTimeControlNode:
Done in ae9907f
These merge calls both have the same potential problems...I cannot imagine allowing the client to override isPlayingProperty
Assertions added in d723fbb
Back to @pixelzoom.
👍 Good catch on options.playPauseOptions. I didn't notice that it had the same problem with overriding options. Closing.
Reopening, in phetsims/neuron#142 @SaurabhTotey pointed out a case where we need to pass a different isPlayingProperty to the StepButtons. The StepButton enabled state is controlled by the isPlayingProperty. The only way to have custom conditions for when the StepButton is enabled is to provide a custom isPlayingProperty through options.
I removed the assertions. @pixelzoom does this seem reasonable?
I am considering renaming isPlayingProperty option of StepButton to something like isPlayingAndEnabledProperty to indicate the intent of this option to also control enabled state of the button. Do you think that is worthwhile?
@jessegreenberg said:
... The only way to have custom conditions for when the
StepButtonis enabled is to provide a customisPlayingPropertythrough options.
Could you please explain more about why Neuron has (as noted in phetsims/neuron#142) "special conditions for when it is disabled". This feels to me like either Neuron is doing something odd, or Neuron shouldn't be using TimeControlNode.
Here is what the comment in NeuronScreenView says:
// Allow step back only if the mode is not playing and if recorded state data exists in the data buffer. Data
// recording is only initiated after the neuron has been stimulated, so if the sim is paused before the neuron
// was ever stimulated, backwards stepping will not be possible.
@SaurabhTotey thanks for clarifying the Neuron requirements. I can see why it's useful for the client to have control over the enabled state of the StepForwardButton and StepBackwardButton. That said, I do not think that the solution is to provide different isPlayingProperty instances to each button. The solution is to optionally provided enabledProperty for each button.
So to answer @jessegreenberg's "does this seem reasonable?" question in #563 (comment)... No, I don't think that allowing multiple isPlayingProperty instances is the right direction. And I also don't think that renaming isPlayingProperty to isPlayingAndEnabledProperty is the right direction.
I'm out until 1/2/2020, so I don't have time right now to provide specifics on an alternative solution. I'll leave this self-assigned so that I can look at this further when I return. And if that holds you up, feel free to proceed and I'll catch up later.
Thanks for reviewing, definitely no rush on this and it can wait till you return.
The solution is to optionally provided enabledProperty for each button.
That makes sense too. Currently, the only purpose of isPlayingProperty for StepButton is to control enabled state. So for those buttons a straight rename of isPlayingProperty to enabledProperty may be sufficient.
I don't think it's as easy as renaming isPlayingProperty to enabledProperty. I think we'll want something like this in StepButton:
Property.multilink(
[ isPlayingProperty, enabledProperty ],
( isPlaying, enabled ) => {
this.enabled = ( !isPlaying && enabled );
}
);I've taken a closer look at this. Ideally, we would pass an optional enabledProperty to StepButton. But unfortunately, sun buttons don't follow the "enabled" pattern used by other PhET UI components. In sun buttons, the enabledProperty is at (protected) this.buttonModel.enabledProperty, and the API only supports setEnabled and getEnabled; there is no API for observing enabled, or passing in your own enabledProperty.
This is not the first time that we've bumped into this, and it's described in more detail in phetsims/sun#515. I'm going to increase the priority of addressing that issue and see what happens.
If we don't change anything about sun buttons, then we can consider passing in our own enabledProperty to StepButton.
phetsims/sun#515 and this issue were discussed at 1/2/2020 dev meeting. The consensus was that the correct way to address this is by addressing phetsims/sun#515, then do something like #563 (comment) in StepButton. @jbphet is going to look at phetsims/sun#515, but with medium priority.
@jessegreenberg if you have a more urgent need, then coordinate with @jbphet and @ariel-phet.
I'm going to self-unassign. If you need any more input, let me know.
Thanks for going over during dev meeting. This was motivated by neuron (phetsims/neuron#142) which currently has a workaround. I think this can wait for phetsims/sun#515.
Just wanted to add that Collision Lab is now the second sim that needs to pass a custom enabledProperty to a step button. Like neuron, I will be using the isPlayingProperty workaround until this gets addressed (as discussed in #606).
The most recent decision for this issues is in #563 (comment):
@jbphet is going to look at phetsims/sun#515, but with medium priority.
sun#515 has been addressed and is closed, so this issue can presumably move forward, and is no long "on hold".
I see StepForwardButton in TimeControlNode, and @jessegreenberg made some changes in the above commit for greehouse-effect.
@jessegreenberg what do we need to do to wrap this up?
The remaining work is to add something like #563 (comment) to StepButton and then add these assertions back 6d3ee93.
I gave it a shot, but I didn't see a way to do this without adding another Property to StepButton to control whether its enabled. The result of phetsims/sun#515 was to move enabledProperty into Node.js. First, logic like #563 (comment) won't work because setting this.enabled will set this.enabledProperty of the StepButton. Second, Node.js requires the enabledProperty to be settable (cannot be a DerivedProperty), and both cases in nueron and collision-lab identified in this issue would want to pass DerivedProperties to the StepButton.
I tried adding a second optional buttonEnabledProperty and supporting the combinations of using it with isPlayingProperty
Index: js/buttons/StepButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/buttons/StepButton.js (revision 7be5f40001a7c2e019a0b9e68c23917898c94adf)
+++ js/buttons/StepButton.js (date 1631561542965)
@@ -8,6 +8,7 @@
* @author Chris Malley (PixelZoom, Inc.)
*/
+import Property from '../../../axon/js/Property.js';
import Shape from '../../../kite/js/Shape.js';
import InstanceRegistry from '../../../phet-core/js/documentation/InstanceRegistry.js';
import merge from '../../../phet-core/js/merge.js';
@@ -47,6 +48,11 @@
// and you should avoid using the button's native 'enabled' property.
isPlayingProperty: null,
+ // {Property.<boolean>|null} A Property that determines whether the button should be enabled. If
+ // buttonEnabledProperty AND isPlayingProperty are both provided, the button will be enabled when
+ // isPlayingProperty is false and buttonEnabledProperty is true.
+ buttonEnabledProperty: null,
+
// use the step-forward sound by default
soundPlayer: stepForwardSoundPlayer,
@@ -88,18 +94,38 @@
super( options );
- // Disable the button when the sim is playing
- let playingObserver = null;
- if ( options.isPlayingProperty ) {
- playingObserver = playing => {
- this.enabled = !playing;
+ let multilink = null;
+ let propertyToUnlink;
+ let listenerToUnlink;
+ if ( options.isPlayingProperty && options.buttonEnabledProperty ) {
+ multilink = Property.multilink( [ options.isPlayingProperty, options.buttonEnabledProperty ],
+ ( isPlaying, enabled ) => {
+ this.enabled = !isPlaying && enabled;
+ } );
+ }
+ else if ( options.isPlayingProperty ) {
+ propertyToUnlink = options.isPlayingProperty;
+ listenerToUnlink = isPlaying => {
+ this.enabled = !isPlaying;
};
- options.isPlayingProperty.link( playingObserver );
+ options.isPlayingProperty.link( listenerToUnlink );
+ }
+ else if ( options.buttonEnabledProperty ) {
+ propertyToUnlink = options.buttonEnabledProperty;
+ listenerToUnlink = enabled => {
+ this.enabled = enabled;
+ };
+ options.buttonEnabledProperty.link( listenerToUnlink );
}
// @private
this.disposeStepButton = () => {
- options.isPlayingProperty && options.isPlayingProperty.unlink( playingObserver );
+ if ( multilink ) {
+ Property.unmultilink( multilink );
+ }
+ if ( propertyToUnlink ) {
+ propertyToUnlink.unlink( listenerToUnlink );
+ }
};
// support for binder documentation, stripped out in builds and only runs when ?binder is specified
Alternatively...isPlayingProperty is a feature of TimeControlNode so what if we added support for this there so we don't need these options in the StepButton? Like this, with example usage in Neuron
Index: neuron/js/neuron/view/NeuronScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- neuron/js/neuron/view/NeuronScreenView.js (revision 669f37c9e1e8cd5d943be9ed28395400bd428bbe)
+++ neuron/js/neuron/view/NeuronScreenView.js (date 1631565817630)
@@ -210,15 +210,13 @@
const playingProperty = neuronClockModelAdapter.playingProperty; // convenience variable
- // property that determines whether the StepBackwardButton should be disabled
- // is passed in as an isPlayingProperty because isPlayingProperty is used by StepButtons to figure out whether
- // they should be enabled or not
- // the StepBackwardButton doesn't use the normal playingProperty for reasons stated below
- const stepBackDisabledProperty = new DerivedProperty( [
- playingProperty,
+ // Property that determines whether the StepBackwardButton should be enabled. In addition to the default
+ // behavior for of TimeControlNode (disabled while playing), it should remain disabled until there is some
+ // data to step backwards through.
+ const stepBackButtonEnabledProperty = new DerivedProperty( [
this.neuronModel.timeProperty
],
- ( playing, time ) => playing || time <= this.neuronModel.getMinRecordedTime() || this.neuronModel.getRecordedTimeRange() <= 0
+ time => time > this.neuronModel.getMinRecordedTime() && this.neuronModel.getRecordedTimeRange() > 0
);
// space between layout edge and controls like reset, zoom control, legend, speed panel, etc.
@@ -235,7 +233,7 @@
},
stepBackwardButtonOptions: {
listener: () => { neuronClockModelAdapter.stepClockBackWhilePaused(); },
- isPlayingProperty: stepBackDisabledProperty
+ buttonEnabledProperty: stepBackButtonEnabledProperty
},
playPauseStepXSpacing: 5
},
Index: scenery-phet/js/TimeControlNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/TimeControlNode.js (revision 7be5f40001a7c2e019a0b9e68c23917898c94adf)
+++ scenery-phet/js/TimeControlNode.js (date 1631565877575)
@@ -10,6 +10,9 @@
* @author Jesse Greenberg (PhET Interactive Simulations)
*/
+import BooleanProperty from '../../axon/js/BooleanProperty.js';
+import DerivedProperty from '../../axon/js/DerivedProperty.js';
+import Property from '../../axon/js/Property.js';
import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
import merge from '../../phet-core/js/merge.js';
import HBox from '../../scenery/js/nodes/HBox.js';
@@ -277,8 +280,12 @@
}
}
+ const defaultButtonEnabledProperty = new DerivedProperty( [ isPlayingProperty ], isPlaying => {
+ return !isPlaying;
+ } );
+
const defaultStepButtonOptions = {
- isPlayingProperty: isPlayingProperty,
+ buttonEnabledProperty: defaultButtonEnabledProperty,
radius: 15,
touchAreaDilation: 5
};
@@ -304,12 +311,22 @@
// Options for the StepBackwardButton
stepBackwardButtonOptions: merge( {
+
+ // {BooleanProperty} - Controls the enabled state of the StepButton, it will only be enabled if this Property
+ // and isPlayingProperty are both true.
+ buttonEnabledProperty: new BooleanProperty( true ),
+
tandem: tandem.createTandem( 'stepBackwardButton' ),
phetioDocumentation: 'Progress the simulation a single model step backwards.'
}, defaultStepButtonOptions ),
// Options for the StepForwardButton
stepForwardButtonOptions: merge( {
+
+ // {BooleanProperty} - Controls the enabled state of the StepButton, it will only be enabled if this Property
+ // and isPlayingProperty are both true.
+ buttonEnabledProperty: new BooleanProperty( true ),
+
phetioDocumentation: 'Progress the simulation a single model step forwards.',
tandem: tandem.createTandem( 'stepForwardButton' )
}, defaultStepButtonOptions )
@@ -321,15 +338,25 @@
buttons.push( playPauseButton );
let stepForwardButton = null;
+ let stepForwardButtonEnabledMultilink = null;
if ( options.includeStepForwardButton ) {
stepForwardButton = new StepForwardButton( options.stepForwardButtonOptions );
buttons.push( stepForwardButton );
+
+ stepForwardButtonEnabledMultilink = Property.multilink( [ isPlayingProperty, options.stepForwardButtonOptions.buttonEnabledProperty ], ( isPlaying, enabled ) => {
+ stepForwardButton.enabled = !isPlaying && enabled;
+ } );
}
let stepBackwardButton = null;
+ let stepBackwardButtonEnabledMultilink = null;
if ( options.includeStepBackwardButton ) {
stepBackwardButton = new StepBackwardButton( options.stepBackwardButtonOptions );
buttons.unshift( stepBackwardButton );
+
+ stepBackwardButtonEnabledMultilink = Property.multilink( [ isPlayingProperty, options.stepBackwardButtonOptions.buttonEnabledProperty ], ( isPlaying, enabled ) => {
+ stepBackwardButton.enabled = !isPlaying && enabled;
+ } );
}
super( {
@@ -349,6 +376,10 @@
this.disposePlayPauseStepButtons = () => {
playPauseButton.dispose();
+
+ stepForwardButtonEnabledMultilink && Property.unmultilink( stepForwardButtonEnabledMultilink );
+ stepBackwardButtonEnabledMultilink && Property.unmultilink( stepBackwardButtonEnabledMultilink );
+
stepForwardButton && stepForwardButton.dispose();
stepBackwardButton && stepBackwardButton.dispose();
};
Index: scenery-phet/js/buttons/StepButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/buttons/StepButton.js (revision 7be5f40001a7c2e019a0b9e68c23917898c94adf)
+++ scenery-phet/js/buttons/StepButton.js (date 1631565293077)
@@ -42,11 +42,6 @@
// shift the content to center align, assumes 3D appearance and specific content
xContentOffset: ( options.direction === 'forward' ) ? ( 0.075 * options.radius ) : ( -0.15 * options.radius ),
- // {Property.<boolean>|null} is the sim playing? This is a convenience option.
- // If this Property is provided, it will disable the button while the sim is playing,
- // and you should avoid using the button's native 'enabled' property.
- isPlayingProperty: null,
-
// use the step-forward sound by default
soundPlayer: stepForwardSoundPlayer,
@@ -88,20 +83,6 @@
super( options );
- // Disable the button when the sim is playing
- let playingObserver = null;
- if ( options.isPlayingProperty ) {
- playingObserver = playing => {
- this.enabled = !playing;
- };
- options.isPlayingProperty.link( playingObserver );
- }
-
- // @private
- this.disposeStepButton = () => {
- options.isPlayingProperty && options.isPlayingProperty.unlink( playingObserver );
- };
-
// support for binder documentation, stripped out in builds and only runs when ?binder is specified
assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'scenery-phet', 'StepButton', this );
}
@pixelzoom what would you recommend or prefer?
@zepumph helped think through another possibility where we remove option isPlayingProperty from StepButton and just pass an enabledProperty to it instead. The default enabledProperty is a DerivedProperty whose value is !isPlayingProperty.value. If clients want to use their own enabledProperty they will be responsible for including the state of isPlayingProperty in the derivation (if they want to). A DerivedProperty should work for Node's enabledProperty, but if you use a DerivedProperty then the Node.enabled setter cannot be used. I didn't realize that and was incorrectly using it when I initially tried in #563 (comment). Here is the patch and I think I am going to commit to it after updating usages of isPlayingProperty option and testing.
Index: neuron/js/neuron/view/NeuronScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- neuron/js/neuron/view/NeuronScreenView.js (revision 289fb835434aacf43e0408ef6d46d32826b9c92a)
+++ neuron/js/neuron/view/NeuronScreenView.js (date 1632430563615)
@@ -214,7 +214,7 @@
// is passed in as an isPlayingProperty because isPlayingProperty is used by StepButtons to figure out whether
// they should be enabled or not
// the StepBackwardButton doesn't use the normal playingProperty for reasons stated below
- const stepBackDisabledProperty = new DerivedProperty( [
+ const stepBackEnabledProperty = new DerivedProperty( [
playingProperty,
this.neuronModel.timeProperty
],
@@ -235,7 +235,7 @@
},
stepBackwardButtonOptions: {
listener: () => { neuronClockModelAdapter.stepClockBackWhilePaused(); },
- isPlayingProperty: stepBackDisabledProperty
+ isPlayingProperty: stepBackEnabledProperty
},
playPauseStepXSpacing: 5
},
Index: scenery-phet/js/TimeControlNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/TimeControlNode.js (revision 1e4ac1ae8d25ea47d40361f817ed400de53e0c23)
+++ scenery-phet/js/TimeControlNode.js (date 1632428802679)
@@ -10,6 +10,7 @@
* @author Jesse Greenberg (PhET Interactive Simulations)
*/
+import DerivedProperty from '../../axon/js/DerivedProperty.js';
import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
import merge from '../../phet-core/js/merge.js';
import HBox from '../../scenery/js/nodes/HBox.js';
@@ -75,7 +76,7 @@
// is included
buttonGroupXSpacing: 40,
- // {Object|null} - options passed along to the PlayPauseStepButtons
+ // {Object|null} - options passed along to the PlayPauseStepButtons, see the inner class for defaults
playPauseStepButtonOptions: null,
// {Object|null} - options passed along to the SpeedRadioButtonGroup, if included
@@ -277,8 +278,18 @@
}
}
+ // by default, the button is enabled when isPlayingProperty is false, but only create a PhET-iO instrumented
+ // Property if it is going to be used
+ let defaultEnabledProperty = null;
+ if ( ( !options.stepForwardButtonOptions || !options.stepForwardButtonOptions.enabledProperty ) ||
+ ( !options.stepBackwardButtonOptions || !options.stepBackwardButtonOptions.enabledProperty ) ) {
+ defaultEnabledProperty = DerivedProperty.not( isPlayingProperty, {
+ tandem: tandem.createTandem( 'enabledProperty' )
+ } );
+ }
+
const defaultStepButtonOptions = {
- isPlayingProperty: isPlayingProperty,
+ enabledProperty: defaultEnabledProperty,
radius: 15,
touchAreaDilation: 5
};
@@ -315,6 +326,21 @@
}, defaultStepButtonOptions )
}, options );
+ // by default, the step buttons are enabled when isPlayingProperty is false, but only create a PhET-iO instrumented
+ // Property if it is going to be used
+ if ( ( !options.stepForwardButtonOptions.enabledProperty ) || ( !options.stepBackwardButtonOptions.enabledProperty ) ) {
+ const defaultEnabledProperty = DerivedProperty.not( isPlayingProperty, {
+ tandem: tandem.createTandem( 'enabledProperty' )
+ } );
+
+ if ( !options.stepForwardButtonOptions.enabledProperty ) {
+ options.stepForwardButtonOptions.enabledProperty = defaultEnabledProperty;
+ }
+ if ( !options.stepBackwardButtonOptions.enabledProperty ) {
+ options.stepBackwardButtonOptions.enabledProperty = defaultEnabledProperty;
+ }
+ }
+
const buttons = [];
const playPauseButton = new PlayPauseButton( isPlayingProperty, options.playPauseButtonOptions );
Index: scenery-phet/js/buttons/StepButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/buttons/StepButton.js (revision 1e4ac1ae8d25ea47d40361f817ed400de53e0c23)
+++ scenery-phet/js/buttons/StepButton.js (date 1632430657978)
@@ -42,11 +42,6 @@
// shift the content to center align, assumes 3D appearance and specific content
xContentOffset: ( options.direction === 'forward' ) ? ( 0.075 * options.radius ) : ( -0.15 * options.radius ),
- // {Property.<boolean>|null} is the sim playing? This is a convenience option.
- // If this Property is provided, it will disable the button while the sim is playing,
- // and you should avoid using the button's native 'enabled' property.
- isPlayingProperty: null,
-
// use the step-forward sound by default
soundPlayer: stepForwardSoundPlayer,
@@ -58,7 +53,6 @@
assert && assert( options.xMargin === undefined && options.yMargin === undefined, 'stepButton sets margins' );
options.xMargin = options.yMargin = options.radius * marginCoefficient;
-
assert && assert( options.direction === 'forward' || options.direction === 'backward',
`unsupported direction: ${options.direction}` );
@@ -88,32 +82,9 @@
super( options );
- // Disable the button when the sim is playing
- let playingObserver = null;
- if ( options.isPlayingProperty ) {
- playingObserver = playing => {
- this.enabled = !playing;
- };
- options.isPlayingProperty.link( playingObserver );
- }
-
- // @private
- this.disposeStepButton = () => {
- options.isPlayingProperty && options.isPlayingProperty.unlink( playingObserver );
- };
-
// support for binder documentation, stripped out in builds and only runs when ?binder is specified
assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'scenery-phet', 'StepButton', this );
}
-
- /**
- * @public
- * @override
- */
- dispose() {
- this.disposeStepButton();
- super.dispose();
- }
}
sceneryPhet.register( 'StepButton', StepButton );
This change would mean that the enabled state of the TimeControlNode step buttons cannot be set by a client with PhET-iO because the enabledProperty is using a DerivedProperty. @arouinfar is that an OK limitation? If not @zepumph said that we could add support for that by adding more to the above patch.
EDIT: updated patch to remove isPlayingProperty from StepButton.
Changes made above. @pixelzoom would you mind reviewing this change to StepButton and TimeControlNode?
Update PhET-iO API above.
The approach described in #563 (comment) sounds good. I reviewed 73a9542, spot-checked the other commits, and didn't see any obvious problems. So I think this is ready to close.
Oops, I see that @arouinfar was also assigned here. So I'm not sure if I should have closed this. Is there still some design sign-off required? Back to @jessegreenberg in case there's more to do here.
In #563 (comment), here's the feedback that @jessegreenberg wanted from @arouinfar:
This change would mean that the enabled state of the TimeControlNode step buttons cannot be set by a client with PhET-iO because the
enabledPropertyis using a DerivedProperty. @arouinfar is that an OK limitation? If not @zepumph said that we could add support for that by adding more to the above patch.
This change would mean that the enabled state of the TimeControlNode step buttons cannot be set by a client with PhET-iO because the
enabledPropertyis using a DerivedProperty. @arouinfar is that an OK limitation? If not @zepumph said that we could add support for that by adding more to the above patch.
Yes, that makes sense to me. The state of the step buttons should be determined by the state of the PlayPauseButton. It seems messy to let clients override the enabledProperty.
Here's a possible use case:
- Client wants students to make a prediction or take some measurements with a sim in a particular paused state.
- Using the step buttons could reveal the answer or lead to students examining different states.
- Client should use
timeControlNode.enabledPropertyortimeControlNode.visiblePropertyin this case. There is no need to directly controlstep*Button.enabledProperty.
It looks like this changed the API of phet-io sims. I'll regenerate the gravity-and-orbits one, which is showing on CT.
I ran grunt generate-phet-io-api in gravity-and-orbits and the changes seemed reasonable. Reassigning to @jessegreenberg to spot check and see if other phet-io APIs need to be regenerated.
Thanks @samreid, sorry I am not sure how gravity-and-orbits got missed. APIs were updated generally in https://github.com/phetsims/phet-io/commit/1f9193d540aa313d4f197848838c6694dfd14cac.
I pulled all and ran perennial/bin/for-each.sh phet-io-api-stable grunt generate-phet-io-api. I saw changes for states-of-matter but it didn't seem related to TimeControlNode.
It may be a new complication from TypeScript. TypeScript sims need to be compiled before running grunt generate-phet-io-api. Sorry for the hassle! Can this issue be closed?
Ah, interesting. OK thanks, closing.
@jessegreenberg in phetsims/chipper#1118 we changed grunt generate-phet-io-api so it auto-compiles now.