options are passed to both super and model of Button classes

[pixelzoom]

Noted while converting RoundToggleButton to ES6 class for https://github.com/phetsims/tasks/issues/1044.

In 71f10eb7a400db49dd28bf2f5c254bcc00a3eab1, @zepumph passed the complete set of options to the button's model:

function RoundToggleButton( valueOff, valueOn, property, options ) {
...
  // @public (phet-io)
  // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button
  this.toggleButtonModel = new ToggleButtonModel( valueOff, valueOn, property, options );
...

Besides the fact that this feels like a hack for making the phetioID tree hide the button model... Passing the complete set of options to a subcomponent is an anti-pattern. Only the options intended for that subcomponent should be passed.

I see the anti-pattern widely used: RectangularMomentaryButton, RectangularPushButton, RectangularStickyToggleButton, RectangularToggleButton, RoundMomentaryButton, RoundPushButton, and RoundStickyToggleButton.

This needs to be corrected.

[pixelzoom]

There's also a memory leak that this introduced in some of the above classes. Because the button model is now instrumented, it needs to be disposed. And some of these classes (RectangularMomentaryButton, ... ) are not overriding dispose, and not handling disposal of buttonModel.

[pixelzoom]

Raising priority since this is an obvious memory leak.

[pixelzoom]

@zepumph can you please explain the // @public (phet-io) annotation above this.toggleButtonModel (and other button models).

[pixelzoom]

To clarify... I'm wondering why this.toggleButtonModel is needed when the superclass already defines this.buttonModel. This is causing dissonance in cases like RoundToggleButton dispose:

    this.disposeRoundToggleButton = () => {
      this.toggleButtonModel.dispose();
      this.buttonModel.produceSoundEmitter.removeListener( playSounds );
      toggleButtonInteractionStateProperty.dispose();
    };

this.toggleButtonModel and this.buttonModel are references to the same object here. Very confusing. And in other classes, this.toggleButtonModel was named this.buttonModel, overwriting the superclass definition. So unless something in PhET-iO is reaching in and accessing this.toggleButtonModel, it would be better if it were const toggleButtonModel.

[zepumph]

I think that it is acceptable to give the buttonModel the same tandem as the view, but that is no excuse to do that for all options. I will work to see what usages use the ButtonModel options and likely refactor them to be nested under buttonModelOptions

RE: https://github.com/phetsims/sun/issues/631#issuecomment-701114577:

Agreed! this.toggleButtonModel should be a local var, only needed for dispose. I also see that this line is redundant, but it isn't obvious since it is opaque that this.buttonModel === toggleButtonModel.

[zepumph]

In the above commit, I got rid of the obvious error where this.toggleButtonModel and this.buttonModel both existed.

As for passing options through to the button model, I think I can see it both ways after poking around more. It seems a little bit like an implementation detail that the view and model of sun buttons are separate. They aren't separate for any other common code UI components. Separating the options out into buttonModelOptions may be the way to go, but I think I'd like a quick discussion at developer meeting to see what people think.

Only the options intended for that subcomponent should be passed.

I'm not sure I think of a button's model as its subcomponent, but rather just an implementation detail as a result of how sun button hierarchy is tied to the view of the buttons.

[zepumph]

Reassigning to implement dispose in button models that don't currently.

[zepumph]

Changes above:

• All buttons should implement a dispose to take care of the button model they create
• interaction state Properties are DerivedProperties based entirely on the button model, so disposing the button model is enough; there is no need to dispose the interaction state Property.
• subtypes don't need to declare this.buttonModel

Still marking for dev meeting to discuss the main part of this issue: if we should be passing options directly to the button models instead of splitting it up into buttonModelOptions.

[zepumph]

Raising priority since this is an obvious memory leak.

Dispose methods have been implemented. Reducing priority.

[samreid]

buttonModelOptions sounds most robust and consistent with the rest of our patterns. Is the main disadvantage of that approach the initial cost to split up the options?

[jbphet]

This was discussed in the 10/29/2020 dev meeting and we decided to pursue the nested buttonModelOptions approach.

[pixelzoom]

Also noted in 10/29/2020 dev meeting: This is not the only case where sun buttons should be using nested options. https://github.com/phetsims/sun/issues/653 identifies the need for nested options related to appearance strategies.

[zepumph]

I hit this today over in https://github.com/phetsims/phet-io/issues/1746. Raising priority, as currently there is some confusing buggy behavior in how enabledPropertyOptions are being passed around. In the Button heirarchy they are for ButtonModel, but Node also accepts enabledPropertyOptions. These need to be split out!

[zepumph]

Options I'm really not excited about doing this for:

• listener: To me this refactor would be exposing an implementation detail in order to make the call site (at least one per screen with a ResetAllButton) less suave. UPDATE: likely I just feel this way because of the number of usages of this.
• enabledProperty. Right now it is a total implementation detail that the ButtonNode passes its enabledProperty option to the ButtonModel before taking that and passing it up to the Node (so they have the same Property controlling enabled). If this pattern changes in the future, it would be again refactoring this when in reality the client just wants to pass an enabledProperty to the Button and move on with life.
• enabledPropertyOptions. I feel weird about this refactor, since it creates asymmetry like so in buttons. I'm still going for it though:

    visiblePropertyOptions: { phetioReadOnly: true },
    inputEnabledPropertyPhetioInstrumented: true,
    buttonModelOptions: {
      enabledPropertyOptions: { phetioReadOnly: true }
    }

---

I guess as I go here, it would make sense to just assert in ButtonNode that there are none of the following, making sure that they are passed through buttonModelOptions instead in the button hierarchy. That could definitely work to avoid gotchas, and it is basically what we already have, just more confusing.

• enabled
• enabledProperty
• enabledPropertyOptions
• enabledPropertyPhetioInstrumented (not passed to ButtonModel, but it would signify that the client is trying to pass options to Node instead of to ButtonModel to get enabledProperty).

[zepumph]

Snapshot comparison was not clean, so I can't commit right now. Here is the patch that covers everything except for listener and enabledProperty.

```diff Index: states-of-matter/js/atomic-interactions/view/AtomicInteractionsScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/states-of-matter/js/atomic-interactions/view/AtomicInteractionsScreenView.js b/states-of-matter/js/atomic-interactions/view/AtomicInteractionsScreenView.js --- a/states-of-matter/js/atomic-interactions/view/AtomicInteractionsScreenView.js +++ b/states-of-matter/js/atomic-interactions/view/AtomicInteractionsScreenView.js @@ -116,7 +116,10 @@ phetioReadOnly: true, visiblePropertyOptions: { phetioReadOnly: true }, inputEnabledPropertyPhetioInstrumented: true, - enabledPropertyOptions: { phetioReadOnly: true } + buttonModelOptions: { + enabledPropertyOptions: { phetioReadOnly: true }, + phetioReadOnly: true + } } ); this.addChild( this.returnAtomButton ); Index: natural-selection/js/common/view/AddAMateButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/natural-selection/js/common/view/AddAMateButton.js b/natural-selection/js/common/view/AddAMateButton.js --- a/natural-selection/js/common/view/AddAMateButton.js +++ b/natural-selection/js/common/view/AddAMateButton.js @@ -29,7 +29,10 @@ // phet-io tandem: Tandem.REQUIRED, - phetioReadOnly: true // because sim state controls when this button is visible + phetioReadOnly: true, // because sim state controls when this button is visible + buttonModelOptions: { + phetioReadOnly: true // because sim state controls when this button is enabled + } }, options ); assert && assert( !options.content, 'AddAMateButton sets content' ); Index: sun/js/demo/ButtonsScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/demo/ButtonsScreenView.js b/sun/js/demo/ButtonsScreenView.js --- a/sun/js/demo/ButtonsScreenView.js +++ b/sun/js/demo/ButtonsScreenView.js @@ -370,9 +370,11 @@ listener: () => console.log( 'fireQuicklyWhenHeldButton fired' ), baseColor: new Color( 114, 132, 62 ), enabledProperty: buttonsEnabledProperty, - fireOnHold: true, - fireOnHoldDelay: 100, - fireOnHoldInterval: 50 + buttonModelOptions: { + fireOnHold: true, + fireOnHoldDelay: 100, + fireOnHoldInterval: 50 + } } ); const fireSlowlyWhenHeldButton = new RectangularPushButton( { @@ -380,9 +382,13 @@ listener: () => console.log( 'fireSlowlyWhenHeldButton fired' ), baseColor: new Color( 147, 92, 120 ), enabledProperty: buttonsEnabledProperty, - fireOnHold: true, - fireOnHoldDelay: 600, - fireOnHoldInterval: 300, + + buttonModelOptions: { + fireOnHold: true, + fireOnHoldDelay: 600, + fireOnHoldInterval: 300, + }, + top: fireQuicklyWhenHeldButton.bottom + 10 } ); @@ -430,9 +436,13 @@ listener: () => console.log( 'fireOnDownButton fired' ), baseColor: new Color( 255, 255, 61 ), enabledProperty: buttonsEnabledProperty, - fireOnDown: true, stroke: 'black', - lineWidth: 1 + lineWidth: 1, + + buttonModelOptions: { + fireOnDown: true, + } + } ); // transparent button with something behind it Index: gravity-and-orbits/js/common/view/GravityAndOrbitsTimeControlNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/gravity-and-orbits/js/common/view/GravityAndOrbitsTimeControlNode.js b/gravity-and-orbits/js/common/view/GravityAndOrbitsTimeControlNode.js --- a/gravity-and-orbits/js/common/view/GravityAndOrbitsTimeControlNode.js +++ b/gravity-and-orbits/js/common/view/GravityAndOrbitsTimeControlNode.js @@ -65,7 +65,9 @@ } ); const restartButton = new RestartButton( { - enabled: false, + buttonModelOptions: { + enabled: false, + }, radius: STEP_BUTTON_RADIUS, xMargin: 9.5, yMargin: 9.5, Index: fractions-common/js/building/view/ShapeGroupNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/fractions-common/js/building/view/ShapeGroupNode.js b/fractions-common/js/building/view/ShapeGroupNode.js --- a/fractions-common/js/building/view/ShapeGroupNode.js +++ b/fractions-common/js/building/view/ShapeGroupNode.js @@ -84,7 +84,9 @@ xMargin: FractionsCommonConstants.ROUND_BUTTON_MARGIN, yMargin: FractionsCommonConstants.ROUND_BUTTON_MARGIN, listener: shapeGroup.increaseContainerCount.bind( shapeGroup ), - enabled: !this.isIcon, + buttonModelOptions: { + enabled: !this.isIcon + }, baseColor: FractionsCommonColorProfile.greenRoundArrowButtonProperty } ); @@ -99,7 +101,9 @@ xMargin: FractionsCommonConstants.ROUND_BUTTON_MARGIN, yMargin: FractionsCommonConstants.ROUND_BUTTON_MARGIN, listener: shapeGroup.decreaseContainerCount.bind( shapeGroup ), - enabled: !this.isIcon, + buttonModelOptions: { + enabled: !this.isIcon, + }, baseColor: FractionsCommonColorProfile.redRoundArrowButtonProperty } ); Index: sun/js/buttons/ArrowButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/ArrowButton.js b/sun/js/buttons/ArrowButton.js --- a/sun/js/buttons/ArrowButton.js +++ b/sun/js/buttons/ArrowButton.js @@ -48,14 +48,17 @@ numberOfArrows: 1, // each arrow will have the same shape and styling arrowSpacing: -DEFAULT_ARROW_HEIGHT * ( 1 / 2 ), // spacing for each arrow such that they overlap slightly - // options related to fire-on-hold feature - fireOnHold: true, - fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds) - fireOnHoldInterval: 100, // fire continuously at this interval (milliseconds) + buttonModelOptions: { + + // options related to fire-on-hold feature + fireOnHold: true, + fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds) + fireOnHoldInterval: 100, // fire continuously at this interval (milliseconds) - // callbacks - startCallback: _.noop, // {function()} called when the pointer is pressed - endCallback: _.noop // {function(over:boolean)} called when the pointer is released, {boolean} over indicates whether the pointer was over when released + // callbacks + startCallback: _.noop, // {function()} called when the pointer is pressed + endCallback: _.noop // {function(over:boolean)} called when the pointer is released, {boolean} over indicates whether the pointer was over when released + } }, options ); Index: sun/js/buttons/RoundPushButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/RoundPushButton.js b/sun/js/buttons/RoundPushButton.js --- a/sun/js/buttons/RoundPushButton.js +++ b/sun/js/buttons/RoundPushButton.js @@ -41,9 +41,15 @@ const listener = options.listener; options = _.omit( options, [ 'listener' ] ); + // TODO: this is incomplete, and should be converted to use buttonModelOptions, https://github.com/phetsims/sun/issues/631 + // TODO: but likely tandem still needs to do this transfer, without a deletion, https://github.com/phetsims/sun/issues/631 + options.buttonModelOptions = merge( {}, _.pick( options, [ 'enabledProperty', 'listener', 'tandem' ] ), options.buttonModelOptions ); + delete options.enabledProperty; + delete options.listener; + // @public - listening only // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button - const buttonModel = new PushButtonModel( options ); + const buttonModel = new PushButtonModel( options.buttonModelOptions ); super( buttonModel, new PushButtonInteractionStateProperty( buttonModel ), options ); Index: sun/js/buttons/RectangularPushButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/RectangularPushButton.js b/sun/js/buttons/RectangularPushButton.js --- a/sun/js/buttons/RectangularPushButton.js +++ b/sun/js/buttons/RectangularPushButton.js @@ -30,6 +30,8 @@ // {function} listener called when button is pushed. listener: _.noop, + buttonModelOptions: {}, + // tandem support tandem: Tandem.REQUIRED @@ -41,9 +43,15 @@ const listener = options.listener; options = _.omit( options, [ 'listener' ] ); + // TODO: this is incomplete, and should be converted to use buttonModelOptions, https://github.com/phetsims/sun/issues/631 + // TODO: but likely tandem still needs to do this transfer, without a deletion, https://github.com/phetsims/sun/issues/631 + options.buttonModelOptions = merge( {}, _.pick( options, [ 'enabledProperty', 'listener', 'tandem' ] ), options.buttonModelOptions ); + delete options.enabledProperty; + delete options.listener; + // Safe to pass through options to the PushButtonModel like "fireOnDown". Other scenery options will be safely ignored. // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button - const buttonModel = new PushButtonModel( options ); // @public, listen only + const buttonModel = new PushButtonModel( options.buttonModelOptions ); // @public, listen only super( buttonModel, new PushButtonInteractionStateProperty( buttonModel ), options ); Index: number-line-integers/js/explore/view/AccountBalanceControllerNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-line-integers/js/explore/view/AccountBalanceControllerNode.js b/number-line-integers/js/explore/view/AccountBalanceControllerNode.js --- a/number-line-integers/js/explore/view/AccountBalanceControllerNode.js +++ b/number-line-integers/js/explore/view/AccountBalanceControllerNode.js @@ -19,17 +19,19 @@ import RoundPushButton from '../../../../sun/js/buttons/RoundPushButton.js'; import withdrawingCoinsImage from '../../../images/coin-in-hand_png.js'; import depositingCoinsImage from '../../../images/coin-in-slot_png.js'; -import numberLineIntegersStrings from '../../numberLineIntegersStrings.js'; import numberLineIntegers from '../../numberLineIntegers.js'; +import numberLineIntegersStrings from '../../numberLineIntegersStrings.js'; // constants const MARGIN = 10; const BUTTON_OPTIONS = { xMargin: MARGIN, yMargin: MARGIN, - fireOnHold: true, - fireOnHoldDelay: 400, - fireOnHoldInterval: 30 + buttonModelOptions: { + fireOnHold: true, + fireOnHoldDelay: 400, + fireOnHoldInterval: 30 + } }; const CURRENCY_SYMBOL_FONT = new PhetFont( 12 ); const CURRENCY_SYMBOL_MAX_WIDTH = 10; Index: energy-forms-and-changes/js/systems/view/BikerNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/energy-forms-and-changes/js/systems/view/BikerNode.js b/energy-forms-and-changes/js/systems/view/BikerNode.js --- a/energy-forms-and-changes/js/systems/view/BikerNode.js +++ b/energy-forms-and-changes/js/systems/view/BikerNode.js @@ -62,8 +62,8 @@ import cyclistTorsoTired3 from '../../../images/cyclist_torso_tired_3_png.js'; import EFACConstants from '../../common/EFACConstants.js'; import EnergyChunkLayer from '../../common/view/EnergyChunkLayer.js'; -import energyFormsAndChangesStrings from '../../energyFormsAndChangesStrings.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; +import energyFormsAndChangesStrings from '../../energyFormsAndChangesStrings.js'; import Biker from '../model/Biker.js'; import MoveFadeModelElementNode from './MoveFadeModelElementNode.js'; @@ -238,7 +238,11 @@ centerY: cyclistTorsoNodes[ 0 ].centerTop.y - 15, minHeight: 30, tandem: tandem.createTandem( 'feedMeButton' ), + phetioReadOnly: true, + buttonModelOptions: { + phetioReadOnly: true + }, phetioDocumentation: 'button that replenish\'s the biker\'s energy. only visible when the biker is out of energy' } ); this.addChild( feedMeButton ); Index: scenery-phet/js/ZoomButtonGroup.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/ZoomButtonGroup.js b/scenery-phet/js/ZoomButtonGroup.js --- a/scenery-phet/js/ZoomButtonGroup.js +++ b/scenery-phet/js/ZoomButtonGroup.js @@ -43,9 +43,11 @@ // RectangularPushButton options buttonOptions: { - fireOnHold: true, - fireOnHoldDelay: 600, // ms - fireOnHoldInterval: 250 // ms + buttonModelOptions: { + fireOnHold: true, + fireOnHoldDelay: 600, // ms + fireOnHoldInterval: 250 // ms + } }, // LayoutBox options Index: states-of-matter/js/phase-changes/view/PhaseChangesScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/states-of-matter/js/phase-changes/view/PhaseChangesScreenView.js b/states-of-matter/js/phase-changes/view/PhaseChangesScreenView.js --- a/states-of-matter/js/phase-changes/view/PhaseChangesScreenView.js +++ b/states-of-matter/js/phase-changes/view/PhaseChangesScreenView.js @@ -229,7 +229,10 @@ tandem: tandem.createTandem( 'returnLidButton' ), phetioReadOnly: true, visiblePropertyOptions: { phetioReadOnly: true }, - enabledPropertyOptions: { phetioReadOnly: true } + buttonModelOptions: { + phetioReadOnly: true, + enabledPropertyOptions: { phetioReadOnly: true } + } } ); this.addChild( this.returnLidButton ); model.isExplodedProperty.linkAttribute( this.returnLidButton, 'visible' ); Index: plinko-probability/js/intro/view/IntroPlayPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/plinko-probability/js/intro/view/IntroPlayPanel.js b/plinko-probability/js/intro/view/IntroPlayPanel.js --- a/plinko-probability/js/intro/view/IntroPlayPanel.js +++ b/plinko-probability/js/intro/view/IntroPlayPanel.js @@ -67,10 +67,12 @@ touchAreaXDilation: 5 } ); - //Creation of play button + // Creation of play button const playButton = new PlayButton( { listener: model.updateBallsToCreateNumber.bind( model ), - enabled: true + buttonModelOptions: { + enabled: true + } } ); // Disables play button if maximum amount of balls are dropped Index: scenery-phet/js/FineCoarseSpinner.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/FineCoarseSpinner.js b/scenery-phet/js/FineCoarseSpinner.js --- a/scenery-phet/js/FineCoarseSpinner.js +++ b/scenery-phet/js/FineCoarseSpinner.js @@ -73,10 +73,13 @@ mouseAreaXDilation: 0, mouseAreaYDilation: 0, - // phet-io, as requested in https://github.com/phetsims/sun/issues/575 - enabledPropertyOptions: { - phetioReadOnly: true, - phetioFeatured: false + buttonModelOptions: { + + // phet-io, as requested in https://github.com/phetsims/sun/issues/575 + enabledPropertyOptions: { + phetioReadOnly: true, + phetioFeatured: false + } } }, options.arrowButtonOptions ); @@ -85,10 +88,13 @@ numberOfArrows: 2, arrowSpacing: -0.5 * fineButtonOptions.arrowHeight, // arrows overlap - // phet-io, as requested in https://github.com/phetsims/sun/issues/575 - enabledPropertyOptions: { - phetioReadOnly: true, - phetioFeatured: false + buttonModelOptions: { + + // phet-io, as requested in https://github.com/phetsims/sun/issues/575 + enabledPropertyOptions: { + phetioReadOnly: true, + phetioFeatured: false + } } } ); Index: joist/js/JoistButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/JoistButton.js b/joist/js/JoistButton.js --- a/joist/js/JoistButton.js +++ b/joist/js/JoistButton.js @@ -27,6 +27,20 @@ */ constructor( content, navigationBarFillProperty, tandem, options ) { + [ + 'startCallback', // + 'endCallback', // + 'fireOnDown', // + 'fireOnHold', // + 'fireOnHoldDelay', // + 'fireOnHoldInterval', + 'enabledPropertyOptions', + // 'enabledProperty', + 'enabled' + ].forEach( key => { + assert && assert( !options.hasOwnProperty( key ), `${key} in ButtonNode, move to model!!` ); + } ); + options = merge( { cursor: 'pointer', // {string} listener: null, // {function} @@ -35,14 +49,22 @@ highlightExtensionHeight: 0, highlightCenterOffsetX: 0, highlightCenterOffsetY: 0, + buttonModelOptions: { - // JoistButtons by default do not have a featured enabledProperty - enabledPropertyOptions: { phetioFeatured: false } + // JoistButtons by default do not have a featured enabledProperty + enabledPropertyOptions: { phetioFeatured: false } + } }, options ); assert && assert( options.tandem === undefined, 'JoistButton sets tandem' ); options.tandem = tandem; + // TODO: this is incomplete, and should be converted to use buttonModelOptions, https://github.com/phetsims/sun/issues/631 + // TODO: but likely tandem still needs to do this transfer, without a deletion, https://github.com/phetsims/sun/issues/631 + options.buttonModelOptions = merge( {}, _.pick( options, [ 'enabledProperty', 'listener', 'tandem' ] ), options.buttonModelOptions ); + delete options.enabledProperty; + delete options.listener; + // Creates the highlights for the button. const createHighlight = function( fill ) { return new HighlightNode( content.width + options.highlightExtensionWidth, content.height + options.highlightExtensionHeight, { @@ -66,7 +88,7 @@ // @public (phet-io|a11y) - Button model // Note it shares a tandem with "this", so the emitter will be instrumented as a child of the button - this.buttonModel = new PushButtonModel( options ); + this.buttonModel = new PushButtonModel( options.buttonModelOptions ); // Button interactions const interactionStateProperty = new PushButtonInteractionStateProperty( this.buttonModel ); Index: build-an-atom/js/game/view/NumberEntryNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/build-an-atom/js/game/view/NumberEntryNode.js b/build-an-atom/js/game/view/NumberEntryNode.js --- a/build-an-atom/js/game/view/NumberEntryNode.js +++ b/build-an-atom/js/game/view/NumberEntryNode.js @@ -39,7 +39,13 @@ }, options ); // Node creation - const arrowButtonOptions = { arrowHeight: 12, arrowWidth: 15, fireOnHoldDelay: 200 }; + const arrowButtonOptions = { + arrowHeight: 12, + arrowWidth: 15, + buttonModelOptions: { + fireOnHoldDelay: 200 + } + }; const upArrowButton = new ArrowButton( 'up', () => { numberProperty.value = numberProperty.value + 1; }, merge( { Index: sun/js/buttons/ButtonNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/ButtonNode.js b/sun/js/buttons/ButtonNode.js --- a/sun/js/buttons/ButtonNode.js +++ b/sun/js/buttons/ButtonNode.js @@ -102,9 +102,28 @@ // phet-io tandem: Tandem.OPTIONAL, visiblePropertyOptions: { phetioFeatured: true }, - enabledPropertyPhetioInstrumented: true // opt into default PhET-iO instrumented enabledProperty + enabledPropertyPhetioInstrumented: true // opt into default PhET-iO instrumented enabledProperty // TODO: likely don't want this https://github.com/phetsims/sun/issues/631 }, options ); + [ + // 'enabledPropertyPhetioInstrumented', + 'enabledPropertyOptions', + // 'enabledProperty', + 'enabled' + ].forEach( key => { + assert && assert( !options.hasOwnProperty( key ), `enabled-related key "${key}" should be passed through buttonModelOptions` ); + } ); + +// TODO: handle Tandem passing https://github.com/phetsims/sun/issues/631 + + // TODO: options.listener will be a large refactor into the buttonModelOptions, see https://github.com/phetsims/sun/issues/631 + if ( options.listener ) { + options.buttonModelOptions = merge( { + listener: options.listener + }, options.buttonModelOptions ); + } + + options.listenerOptions = merge( { tandem: options.tandem.createTandem( 'pressListener' ) }, options.listenerOptions ); Index: sun/js/buttons/RoundButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/RoundButton.js b/sun/js/buttons/RoundButton.js --- a/sun/js/buttons/RoundButton.js +++ b/sun/js/buttons/RoundButton.js @@ -42,7 +42,11 @@ xMargin: 5, // Minimum margin in x direction, i.e. on left and right yMargin: 5, // Minimum margin in y direction, i.e. on top and bottom - fireOnDown: false, + buttonModelOptions: { + + // TODO: why is this here, not all RoundButtons have pushButtonModels, and furthermore, isn't the model already created and passed in above? https://github.com/phetsims/sun/issues/631 + fireOnDown: false + }, // pointer area dilation touchAreaDilation: 0, // radius dilation for touch area Index: joist/js/PhetButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/PhetButton.js b/joist/js/PhetButton.js --- a/joist/js/PhetButton.js +++ b/joist/js/PhetButton.js @@ -94,11 +94,13 @@ phetioType: PhetButton.PhetButtonIO, phetioDocumentation: 'The button that appears at the right side of the navigation bar, which shows a menu when pressed', - // This is the primary way to disable learners from accessing the phet menu in PhET-iO, so feature it. - enabledPropertyOptions: { - phetioFeatured: true + buttonModelOptions: { + + // This is the primary way to disable learners from accessing the phet menu in PhET-iO, so feature it. + enabledPropertyOptions: { + phetioFeatured: true + } }, - visiblePropertyOptions: { phetioReadOnly: true }, Index: natural-selection/js/common/view/PlayButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/natural-selection/js/common/view/PlayButton.js b/natural-selection/js/common/view/PlayButton.js --- a/natural-selection/js/common/view/PlayButton.js +++ b/natural-selection/js/common/view/PlayButton.js @@ -29,7 +29,10 @@ // phet-io tandem: Tandem.REQUIRED, - phetioReadOnly: true // because sim state controls when this button is visible + phetioReadOnly: true, // because sim state controls when this button is visible + buttonModelOptions: { + phetioReadOnly: true // because sim state controls when this button is enabled + } }, options ); assert && assert( !options.content, 'PlayButton sets content' ); Index: function-builder/js/common/view/table/XYTableNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/function-builder/js/common/view/table/XYTableNode.js b/function-builder/js/common/view/table/XYTableNode.js --- a/function-builder/js/common/view/table/XYTableNode.js +++ b/function-builder/js/common/view/table/XYTableNode.js @@ -67,7 +67,9 @@ // options for scroll buttons const BUTTON_OPTIONS = { - fireOnHold: false, // because scrolling is animated + buttonModelOptions: { + fireOnHold: false // because scrolling is animated + }, minWidth: options.size.width }; Index: natural-selection/js/common/view/StartOverButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/natural-selection/js/common/view/StartOverButton.js b/natural-selection/js/common/view/StartOverButton.js --- a/natural-selection/js/common/view/StartOverButton.js +++ b/natural-selection/js/common/view/StartOverButton.js @@ -29,7 +29,10 @@ // phet-io tandem: Tandem.REQUIRED, - phetioReadOnly: true // because sim state controls when this button is visible + phetioReadOnly: true, // because sim state controls when this button is visible + buttonModelOptions: { + phetioReadOnly: true // because sim state controls when this button is enabled + } }, options ); assert && assert( !options.content, 'StartOverButton sets content' ); Index: fractions-common/js/common/view/RoundArrowButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/fractions-common/js/common/view/RoundArrowButton.js b/fractions-common/js/common/view/RoundArrowButton.js --- a/fractions-common/js/common/view/RoundArrowButton.js +++ b/fractions-common/js/common/view/RoundArrowButton.js @@ -24,7 +24,10 @@ radius: FractionsCommonConstants.ROUND_BUTTON_RADIUS, xMargin: FractionsCommonConstants.ROUND_BUTTON_MARGIN, yMargin: FractionsCommonConstants.ROUND_BUTTON_MARGIN, - fireOnHold: true, + + buttonModelOptions: { + fireOnHold: true + }, arrowRotation: 0, baseColor: FractionsCommonColorProfile.greenRoundArrowButtonProperty, enabledProperty: new BooleanProperty( true ) Index: scenery-phet/js/NumberControl.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/NumberControl.js b/scenery-phet/js/NumberControl.js --- a/scenery-phet/js/NumberControl.js +++ b/scenery-phet/js/NumberControl.js @@ -268,8 +268,10 @@ value = Math.max( value, getCurrentRange().min ); // constrain to range numberProperty.set( value ); }, merge( { - startCallback: options.arrowButtonOptions.leftStart, - endCallback: options.arrowButtonOptions.leftEnd, + buttonModelOptions: { + startCallback: options.arrowButtonOptions.leftStart, + endCallback: options.arrowButtonOptions.leftEnd + }, tandem: options.tandem.createTandem( 'leftArrowButton' ) }, options.arrowButtonOptions ) ); @@ -279,8 +281,10 @@ value = Math.min( value, getCurrentRange().max ); // constrain to range numberProperty.set( value ); }, merge( { - startCallback: options.arrowButtonOptions.rightStart, - endCallback: options.arrowButtonOptions.rightEnd, + buttonModelOptions: { + startCallback: options.arrowButtonOptions.rightStart, + endCallback: options.arrowButtonOptions.rightEnd + }, tandem: options.tandem.createTandem( 'rightArrowButton' ) }, options.arrowButtonOptions ) ); Index: gravity-and-orbits/js/common/view/GravityAndOrbitsSceneView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/gravity-and-orbits/js/common/view/GravityAndOrbitsSceneView.js b/gravity-and-orbits/js/common/view/GravityAndOrbitsSceneView.js --- a/gravity-and-orbits/js/common/view/GravityAndOrbitsSceneView.js +++ b/gravity-and-orbits/js/common/view/GravityAndOrbitsSceneView.js @@ -185,7 +185,9 @@ x: 100, y: 100, visiblePropertyOptions: { phetioReadOnly: true }, - enabledPropertyOptions: { phetioReadOnly: true }, + buttonModelOptions: { + enabledPropertyOptions: { phetioReadOnly: true } + }, listener: () => { // the return button should behave exactly like the rewind button Index: sun/js/NumberSpinner.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/NumberSpinner.js b/sun/js/NumberSpinner.js --- a/sun/js/NumberSpinner.js +++ b/sun/js/NumberSpinner.js @@ -106,10 +106,13 @@ lineWidth: options.arrowButtonLineWidth, focusable: false, - // as requested in https://github.com/phetsims/sun/issues/575 - enabledPropertyOptions: { - phetioReadOnly: true, - phetioFeatured: false + buttonModelOptions: { + + // as requested in https://github.com/phetsims/sun/issues/575 + enabledPropertyOptions: { + phetioReadOnly: true, + phetioFeatured: false + } } }; Index: sun/js/ComboBoxButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/ComboBoxButton.js b/sun/js/ComboBoxButton.js --- a/sun/js/ComboBoxButton.js +++ b/sun/js/ComboBoxButton.js @@ -59,8 +59,10 @@ soundPlayer: Playable.NO_SOUND, // disable default sound generation // PushButtonModel options - enabledPropertyOptions: { - phetioFeatured: false + buttonModelOptions: { + enabledPropertyOptions: { + phetioFeatured: false + } }, visiblePropertyOptions: { phetioFeatured: false }, Index: scenery-phet/js/buttons/StepButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/buttons/StepButton.js b/scenery-phet/js/buttons/StepButton.js --- a/scenery-phet/js/buttons/StepButton.js +++ b/scenery-phet/js/buttons/StepButton.js @@ -36,7 +36,9 @@ }, options ); options = merge( { - fireOnHold: true, + buttonModelOptions: { + fireOnHold: true + }, iconFill: 'black', // shift the content to center align, assumes 3D appearance and specific content Index: sun/js/Dialog.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/Dialog.js b/sun/js/Dialog.js --- a/sun/js/Dialog.js +++ b/sun/js/Dialog.js @@ -207,7 +207,6 @@ // phet-io tandem: options.tandem.createTandem( 'closeButton' ), phetioReadOnly: options.phetioReadOnly, // match the readOnly of the Dialog - phetioState: false, // close button should not be in state // dialog close buttons by default do not have a featured visibleProperty @@ -216,8 +215,15 @@ phetioState: false }, - // dialog close buttons by default do not have a featured enabledProperty - enabledPropertyOptions: { phetioFeatured: false }, + buttonModelOptions: { + phetioReadOnly: options.phetioReadOnly, // match the readOnly of the Dialog + phetioState: false, // close button should not be in state + + // dialog close buttons by default do not have a featured enabledProperty + enabledPropertyOptions: { + phetioFeatured: false + } + }, // turn off default sound generation, Dialog will create its own sounds soundPlayer: Playable.NO_SOUND, @@ -227,6 +233,8 @@ innerContent: sunStrings.a11y.close } ); + debugger; + // touch/mouse areas for the close button closeButton.touchArea = closeButton.bounds.dilatedXY( options.closeButtonTouchAreaXDilation, ```