PhET-iO problems with EnabledComponent

[pixelzoom]

Problems with EnabledComponent, encountered during https://github.com/phetsims/molecule-polarity/issues/145 while trying to instrument a DragListener.

(1) It duplicates the definition of tandem instead of getting it from PhetioObject:

tandem?: Tandem;

(2) Its other options are confusing, because Node has the same options, some with different types: enabledProperty, enabled, enabledPropertyOptions, phetioEnabledPropertyInstrumented.

(3) It doesn't provide other PhetioObjectOptions, like phetioDocumentation. So I am unable to document MoleculeAngleDragListener.ts and other DragListeners. Should EnabledComponent be a subclass of PhetioObject, perhaps with phetioState: false?

[pixelzoom]

Here's an example from TriatomicMoleculeNode.ts of how I need to instrument MoleculeAngleDragListener. Upon converting to TypeScript, I discovered that phetioDocumentation is not supported for DragListener and its subclasses.

    const atomBDragListener = new MoleculeAngleDragListener( molecule, this, {
      phetioDocumentation: 'dragging atom B rotates the molecule',
      tandem: dragListenersTandem.createTandem( 'atomBDragListener' )
    } );
    const bondABDragListener = new MoleculeAngleDragListener( molecule, this, {
      phetioDocumentation: 'dragging the bond between atoms A and C rotates the molecule',
      tandem: dragListenersTandem.createTandem( 'bondABDragListener' )
    } );
    const bondBCDragListener = new MoleculeAngleDragListener( molecule, this, {
      phetioDocumentation: 'dragging the bond between atoms B and C rotates the molecule',
      tandem: dragListenersTandem.createTandem( 'bondBCDragListener' )
    } );

[pixelzoom]

Workaround in MoleculeAngleDragListener, see below. This blocks publication of molecule-polarity, unless designers want to punt on the phetioDocumentation.

export type MoleculeAngleDragListenerOptions = SelfOptions &
  PickRequired<DragListenerOptions<PressedDragListener>, 'tandem'> &
  //TODO https://github.com/phetsims/axon/issues/412 until fixed, phetioDocumentation is ignored
  //PickOptional<DragListenerOptions<PressedDragListener>, 'phetioDocumentation'>
  PickOptional<PhetioObjectOptions, 'phetioDocumentation'>;

[samreid]

In the workaround, it is unclear to me how that phetioDocumentation would end up in Studio, since the DragListener is not instrumented. Should we make DragListener extend PhetioObject? Should EnabledComponent extend it too?

[pixelzoom]

There's no expectation that the workaround will result in phetioDocumentation showing up in Studio. It simply means that I don't need to delete phetioDocumentation at MoleculeAngleDragListener instantiation sites. The documentation says:

//TODO https://github.com/phetsims/axon/issues/412 until fixed, phetioDocumentation is ignored

... which is why this issue is blocking for molecule-polarity.

Yes, I believe that DragListener needs to extend PhetioObject. I routinely/sadly need to extend PhetioObject for classes that have no state (phetioState: false), just so I can add phetioDocumentation that has been requested by designers.

[pixelzoom]

Same problem with molecule-polarity BondAngleDragListener, same "workaround".

[samreid]

One more question before I begin on this, EnabledComponent has 6 direct subclasses and 1 instantiation site. Should EnabledComponent itself extend PhetioObject, or only certain places like DragListener?

UPDATE: I'm aware this question was in the top comment, maybe we need to hear from @zepumph?

[pixelzoom]

... Should EnabledComponent itself extend PhetioObject, or only certain places like DragListener?

I'm not familiar with how EnabledComponent is used, and what it's intended to be used for. And I suspect that the doc is stale, particularly:

• Base class that defines a settable Property ...

Property?... I see it being used for things that are not Property (buttons, listeners, announcers,..) And while I may have missed something, I don't see it being used anywhere related to Property.

So... I don't know the answer to that question. If @zepumph doesn't have a strong recommendation, I can dive in and look at it more closely.

[zepumph]

I think much of the confusion here is about the object that is EnabledComponent. It is not meant to do anything but compose an enabledProperty that can be PhET-iO instrumented. It used to be a mixin, which provided the flexibility that I think @pixelzoom is desiring, but now it isn't, which has caused me to use composition with it in a variety of cases where I need multiple inheritance.

In this case, I have to compose and then pull out the enabledProperty onto my type since I already have a supertype.

https://github.com/phetsims/utterance-queue/blob/2c543b0d42d657b48f98690e6530ad69d9c68157/js/SpeechSynthesisAnnouncer.ts#L253-L267

Should EnabledComponent itself extend PhetioObject

I do not immediately suggest this, because it further deviates from the original intent of EnabledComponent just adding a single Property to the class subtyping it, but I think I defer to @samreid since he currently holds the best vision for EnabledComponent as a class.

---

It seems like @pixelzoom should open a second issue for if Presslistener should be phet-io instrumented to support phetioDocumentation. Currently it is not instrumented, and just passes in its tandem to the subcomponent PhetioActions (for the data stream). I don't feel too strongly, but it would add a fair number of instrumented listeners to the API that don't really need to be there (just as a placement for documentation I guess).

[pixelzoom]

@zepumph said:

... I don't feel too strongly, but it would add a fair number of instrumented listeners to the API that don't really need to be there (just as a placement for documentation I guess).

I'll leave it up to @arouinfar to decide whether DragListener (and PressListener) needs to be documented. My recollection is that we wanted to be able to document ALL scenery input listeners because Nodes often have more than 1 associated listener.

More specifically, in molecule-polarity, the following documention was requested during PhET-iO design. These seem like useful (maybe even important) descriptions. And they are currently ignored.

// Two Atoms screen
moleculePolarity.twoAtomsScreen.view.moleculeNode.dragListener:
'rotates the molecule by dragging anywhere on it'

// Three Atoms screen
moleculePolarity.threeAtomsScreen.view.moleculeNode.dragListeners.atomBDragListener:
'dragging atom B rotates the molecule',

moleculePolarity.threeAtomsScreen.view.moleculeNode.dragListeners.bondABDragListener:
 'dragging the bond between atoms A and C rotates the molecule',

'moleculePolarity.threeAtomsScreen.view.moleculeNode.dragListeners.bondBCDragListener:
'dragging the bond between atoms B and C rotates the molecule'

Thiis would change the API of molecule-polarity.

[arouinfar]

I'll leave it up to @arouinfar to decide whether DragListener (and PressListener) needs to be documented. My recollection is that we wanted to be able to document ALL scenery input listeners because Nodes often have more than 1 associated listener.

I don't think listeners always need documentation, but there are definitely cases where it is useful, such as molecule-polarity. It seems like optional documentation would be helpful here.

[pixelzoom]

Thanks @arouinfar. To clarify, phetioDocumentation is typically optional. The problem here is that it's currently impossible to provide it, not even optionally.

[samreid]

I made some good progress on converting EnabledComponent to PhetioObject, but at runtime there are too many tandem collisions from newly instrumented things.

```diff Index: main/joist/js/JoistButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/joist/js/JoistButton.ts b/main/joist/js/JoistButton.ts --- a/main/joist/js/JoistButton.ts (revision 2e8e39c3a974f8df22abd027fb1827cfabc2f5dc) +++ b/main/joist/js/JoistButton.ts (date 1663808374516) @@ -87,6 +87,8 @@ super( options ); + options.tandem = options.tandem.createTandem( 'buttonModel' ); + this.buttonModel = new PushButtonModel( options ); // Button interactions Index: main/axon/js/EnabledComponent.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/EnabledComponent.ts b/main/axon/js/EnabledComponent.ts --- a/main/axon/js/EnabledComponent.ts (revision 1ae115747a78de165a72e810c0aebfadcb97bc95) +++ b/main/axon/js/EnabledComponent.ts (date 1663808025857) @@ -9,24 +9,17 @@ * @author Sam Reid (PhET Interactive Simulations) */ -import EnabledProperty, { EnabledPropertyOptions } from './EnabledProperty.js'; +import EnabledProperty from './EnabledProperty.js'; import merge from '../../phet-core/js/merge.js'; -import { optionize3 } from '../../phet-core/js/optionize.js'; import Tandem from '../../tandem/js/Tandem.js'; import axon from './axon.js'; import TProperty from './TProperty.js'; import TReadOnlyProperty from './TReadOnlyProperty.js'; +import PhetioObject, { PhetioObjectOptions } from '../../tandem/js/PhetioObject.js'; +import optionize from '../../phet-core/js/optionize.js'; +import { PropertyOptions } from './ReadOnlyProperty.js'; -// constants -const DEFAULT_OPTIONS = { - enabledProperty: null, - enabled: true, - enabledPropertyOptions: null, - phetioEnabledPropertyInstrumented: true, - tandem: Tandem.OPTIONAL -} as const; - -export type EnabledComponentOptions = { +type SelfOptions = { // if not provided, a Property will be created enabledProperty?: TReadOnlyProperty | null; @@ -35,17 +28,16 @@ enabled?: boolean; // options to enabledProperty if we create it, ignored if enabledProperty is provided - enabledPropertyOptions?: EnabledPropertyOptions | null; + enabledPropertyOptions?: PropertyOptions | null; // Whether or not the default-created enabledProperty should be instrumented for PhET-iO. Ignored if // options.enabledProperty is provided. phetioEnabledPropertyInstrumented?: boolean; - - // phet-io - tandem?: Tandem; }; -export default class EnabledComponent { +export type EnabledComponentOptions = SelfOptions & PhetioObjectOptions; + +export default class EnabledComponent extends PhetioObject { public enabledProperty: TProperty; @@ -53,7 +45,14 @@ public constructor( providedOptions?: EnabledComponentOptions ) { - const options = optionize3()( {}, DEFAULT_OPTIONS, providedOptions ); + const options = optionize()( { + enabledProperty: null, + enabled: true, + enabledPropertyOptions: null, + phetioEnabledPropertyInstrumented: true + }, providedOptions ); + + super( options ); const ownsEnabledProperty = !options.enabledProperty; @@ -82,8 +81,9 @@ public isEnabled(): boolean { return this.enabledProperty.value; } - public dispose(): void { + public override dispose(): void { this.disposeEnabledComponent(); + super.dispose(); } } Index: main/axon/js/EnabledProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/EnabledProperty.ts b/main/axon/js/EnabledProperty.ts --- a/main/axon/js/EnabledProperty.ts (revision 1ae115747a78de165a72e810c0aebfadcb97bc95) +++ b/main/axon/js/EnabledProperty.ts (date 1663807633646) @@ -7,36 +7,23 @@ * @author Michael Kauzmann(PhET Interactive Simulations) */ -import optionize from '../../phet-core/js/optionize.js'; +import optionize, { EmptySelfOptions } from '../../phet-core/js/optionize.js'; import axon from './axon.js'; import BooleanProperty, { BooleanPropertyOptions } from './BooleanProperty.js'; const TANDEM_NAME = 'enabledProperty'; -type SelfOptions = { - checkTandemName?: boolean; -}; - -export type EnabledPropertyOptions = SelfOptions & BooleanPropertyOptions; +export type EnabledPropertyOptions = BooleanPropertyOptions; export default class EnabledProperty extends BooleanProperty { public constructor( initialEnabled: boolean, providedOptions?: EnabledPropertyOptions ) { - const options = optionize()( { + const options = optionize()( { phetioDocumentation: 'Determines whether the element is enabled (true) or disabled (false).', phetioFeatured: true, - - // by default, the tandem name must match. In rare occurrences (such as when one model must have 2 separate - // EnabledProperties, like this.mass1EnabledProperty = ..., this.mass2EnabledProperty = ... - // you can opt out of the name check. This should be used sparingly. For instance, for the example above, it may - // be better to do this.mass1.enabledProperty anyways. - checkTandemName: true + tandemNameSuffix: TANDEM_NAME }, providedOptions ); - if ( assert && options && options.tandem && options.tandem.supplied && options.checkTandemName ) { - assert && assert( options.tandem.name === TANDEM_NAME, `EnabledProperty tandems should be named ${TANDEM_NAME}` ); - } - super( initialEnabled, options ); } Index: main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts b/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts --- a/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts (revision e631fabde92c422cbabb863f5006cf53ae1f91e6) +++ b/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts (date 1663808102551) @@ -11,7 +11,6 @@ import PickOptional from '../../../../phet-core/js/types/PickOptional.js'; import PickRequired from '../../../../phet-core/js/types/PickRequired.js'; import { DragListener, DragListenerOptions, Node, PressedDragListener, SceneryEvent } from '../../../../scenery/js/imports.js'; -import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js'; import moleculePolarity from '../../moleculePolarity.js'; import Molecule from '../model/Molecule.js'; import normalizeAngle from '../model/normalizeAngle.js'; @@ -21,8 +20,7 @@ export type MoleculeAngleDragListenerOptions = SelfOptions & PickRequired, 'tandem'> & //TODO https://github.com/phetsims/axon/issues/412 until fixed, phetioDocumentation is ignored - //PickOptional, 'phetioDocumentation'> - PickOptional; + PickOptional, 'phetioDocumentation'>; export default class MoleculeAngleDragListener extends DragListener { ```

[samreid]

@marlitas and I investigated this, and produced this patch:

```diff Index: main/sun/js/buttons/RoundToggleButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/buttons/RoundToggleButton.ts b/main/sun/js/buttons/RoundToggleButton.ts --- a/main/sun/js/buttons/RoundToggleButton.ts (revision ea6b9140642c864b67bc988b3e68d2a802de95d1) +++ b/main/sun/js/buttons/RoundToggleButton.ts (date 1665160584051) @@ -52,7 +52,10 @@ }, providedOptions ); // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button - const toggleButtonModel = new ToggleButtonModel( valueOff, valueOn, property, options ); + const toggleButtonModel = new ToggleButtonModel( valueOff, valueOn, property, _.merge( {}, options, { + tandem: options.tandem.createTandem( 'toggleButtonModel' ), + tandemNameSuffix: 'toggleButtonModel' + } ) ); const toggleButtonInteractionStateProperty = new ToggleButtonInteractionStateProperty( toggleButtonModel ); super( toggleButtonModel, toggleButtonInteractionStateProperty, options ); Index: main/joist/js/NavigationBarScreenButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/joist/js/NavigationBarScreenButton.ts b/main/joist/js/NavigationBarScreenButton.ts --- a/main/joist/js/NavigationBarScreenButton.ts (revision 6487f3524712fe4486fc2b36b0d365b2adbcd6ca) +++ b/main/joist/js/NavigationBarScreenButton.ts (date 1665159626328) @@ -144,7 +144,7 @@ } ); screenProperty.value = screen; }, - tandem: options.tandem, + tandem: options.tandem.createTandem( 'buttonModel' ), // Navigation bar screen buttons by default do not have a featured enabledProperty. enabledPropertyOptions: { phetioFeatured: false } Index: main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts b/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts --- a/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts (revision a041aab44d83627d98677dea5dc9007d69b120fc) +++ b/main/molecule-polarity/js/common/view/MoleculeAngleDragListener.ts (date 1665158139112) @@ -11,7 +11,6 @@ import PickOptional from '../../../../phet-core/js/types/PickOptional.js'; import PickRequired from '../../../../phet-core/js/types/PickRequired.js'; import { DragListener, DragListenerOptions, Node, PressedDragListener, SceneryEvent } from '../../../../scenery/js/imports.js'; -import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js'; import moleculePolarity from '../../moleculePolarity.js'; import Molecule from '../model/Molecule.js'; import normalizeAngle from '../model/normalizeAngle.js'; @@ -21,8 +20,7 @@ export type MoleculeAngleDragListenerOptions = SelfOptions & PickRequired, 'tandem'> & //TODO https://github.com/phetsims/axon/issues/412 until fixed, phetioDocumentation is ignored - //PickOptional, 'phetioDocumentation'> - PickOptional; + PickOptional, 'phetioDocumentation'>; export default class MoleculeAngleDragListener extends DragListener { Index: main/sun/js/buttons/RoundPushButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/buttons/RoundPushButton.ts b/main/sun/js/buttons/RoundPushButton.ts --- a/main/sun/js/buttons/RoundPushButton.ts (revision ea6b9140642c864b67bc988b3e68d2a802de95d1) +++ b/main/sun/js/buttons/RoundPushButton.ts (date 1665160704434) @@ -49,7 +49,10 @@ // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button //TODO https://github.com/phetsims/sun/issues/749 its surprising that we can pass superOptions with irrelevant fields to PushButtonModel without TS errors - const pushButtonModel = new PushButtonModel( superOptions ); + const pushButtonModel = new PushButtonModel( _.merge( {}, superOptions, { + tandem: superOptions.tandem.createTandem( 'pushButtonModel' ), + tandemNameSuffix: 'pushButtonModel' + } ) ); super( pushButtonModel, new PushButtonInteractionStateProperty( pushButtonModel ), superOptions ); Index: main/sun/js/buttons/RectangularPushButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/buttons/RectangularPushButton.ts b/main/sun/js/buttons/RectangularPushButton.ts --- a/main/sun/js/buttons/RectangularPushButton.ts (revision ea6b9140642c864b67bc988b3e68d2a802de95d1) +++ b/main/sun/js/buttons/RectangularPushButton.ts (date 1665160310426) @@ -44,12 +44,18 @@ // the same code path is always used for adding listener, thus guaranteeing a consistent code path if addListener is // overridden, see https://github.com/phetsims/sun/issues/284. const listener = options.listener; + const tandem = options.tandem; const superOptions = _.omit( options, [ 'listener' ] ); + superOptions.tandem = tandem.createTandem( 'pushButtonModel' ); + superOptions.tandemNameSuffix = 'ButtonModel'; // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button //TODO https://github.com/phetsims/sun/issues/749 its surprising that we can pass superOptions with irrelevant fields to PushButtonModel without TS errors const pushButtonModel = new PushButtonModel( superOptions ); + superOptions.tandem = tandem; + superOptions.tandemNameSuffix = 'Button'; + super( pushButtonModel, new PushButtonInteractionStateProperty( pushButtonModel ), superOptions ); this.pushButtonModel = pushButtonModel; Index: main/joist/js/JoistButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/joist/js/JoistButton.ts b/main/joist/js/JoistButton.ts --- a/main/joist/js/JoistButton.ts (revision 6487f3524712fe4486fc2b36b0d365b2adbcd6ca) +++ b/main/joist/js/JoistButton.ts (date 1665158139108) @@ -87,6 +87,8 @@ super( options ); + options.tandem = options.tandem.createTandem( 'buttonModel' ); + this.buttonModel = new PushButtonModel( options ); // Button interactions Index: main/tandem/js/phetioAPIValidation.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/phetioAPIValidation.ts b/main/tandem/js/phetioAPIValidation.ts --- a/main/tandem/js/phetioAPIValidation.ts (revision ce379a33d0a5bc9722661a5c9803716760c45089) +++ b/main/tandem/js/phetioAPIValidation.ts (date 1665161245111) @@ -250,7 +250,7 @@ // If ?phetioPrintAPIProblems is present, then ignore assertions until the sim has started up. if ( this.simHasStarted || !phet.preloads.phetio.queryParameters.phetioPrintAPIProblems ) { - assert && assert( false, `PhET-iO API error:\n${mismatchMessage}` ); + // assert && assert( false, `PhET-iO API error:\n${mismatchMessage}` ); } } Index: main/axon/js/EnabledProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/EnabledProperty.ts b/main/axon/js/EnabledProperty.ts --- a/main/axon/js/EnabledProperty.ts (revision ac84814f90d7b641712c3ad2f3fb8aa607912a0c) +++ b/main/axon/js/EnabledProperty.ts (date 1665158139104) @@ -7,36 +7,23 @@ * @author Michael Kauzmann(PhET Interactive Simulations) */ -import optionize from '../../phet-core/js/optionize.js'; +import optionize, { EmptySelfOptions } from '../../phet-core/js/optionize.js'; import axon from './axon.js'; import BooleanProperty, { BooleanPropertyOptions } from './BooleanProperty.js'; const TANDEM_NAME = 'enabledProperty'; -type SelfOptions = { - checkTandemName?: boolean; -}; - -export type EnabledPropertyOptions = SelfOptions & BooleanPropertyOptions; +export type EnabledPropertyOptions = BooleanPropertyOptions; export default class EnabledProperty extends BooleanProperty { public constructor( initialEnabled: boolean, providedOptions?: EnabledPropertyOptions ) { - const options = optionize()( { + const options = optionize()( { phetioDocumentation: 'Determines whether the element is enabled (true) or disabled (false).', phetioFeatured: true, - - // by default, the tandem name must match. In rare occurrences (such as when one model must have 2 separate - // EnabledProperties, like this.mass1EnabledProperty = ..., this.mass2EnabledProperty = ... - // you can opt out of the name check. This should be used sparingly. For instance, for the example above, it may - // be better to do this.mass1.enabledProperty anyways. - checkTandemName: true + tandemNameSuffix: TANDEM_NAME }, providedOptions ); - if ( assert && options && options.tandem && options.tandem.supplied && options.checkTandemName ) { - assert && assert( options.tandem.name === TANDEM_NAME, `EnabledProperty tandems should be named ${TANDEM_NAME}` ); - } - super( initialEnabled, options ); } Index: main/phet-io/js/phetioEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/phet-io/js/phetioEngine.ts b/main/phet-io/js/phetioEngine.ts --- a/main/phet-io/js/phetioEngine.ts (revision d3814ff9fa87e4686f3f8c2a64d86a20619d79bf) +++ b/main/phet-io/js/phetioEngine.ts (date 1665160719544) @@ -655,9 +655,19 @@ assert && assert( phetioObject, 'Only non-falsy components should be registered' ); const phetioID = phetioObject.tandem.phetioID; + // if ( this.phetioObjectMap.hasOwnProperty( phetioID ) ) { + // + // console.log( phetioID ); + // return; + // } + assert && Tandem.VALIDATION && assert( !this.phetioObjectMap.hasOwnProperty( phetioID ), `phetioID already registered: ${phetioID}` ); + // if ( phetioID === 'gravityAndOrbits.modelScreen.view.playAreaNode.starPlanetSceneView.timeCounter.clearButton' ) { + // debugger; + // } + this.handlePotentiallyDynamicPhetioObjectAdded( phetioObject ); phetioAPIValidation.onPhetioObjectAdded( phetioObject ); Index: main/axon/js/EnabledComponent.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/EnabledComponent.ts b/main/axon/js/EnabledComponent.ts --- a/main/axon/js/EnabledComponent.ts (revision ac84814f90d7b641712c3ad2f3fb8aa607912a0c) +++ b/main/axon/js/EnabledComponent.ts (date 1665160933447) @@ -9,24 +9,17 @@ * @author Sam Reid (PhET Interactive Simulations) */ -import EnabledProperty, { EnabledPropertyOptions } from './EnabledProperty.js'; +import EnabledProperty from './EnabledProperty.js'; import merge from '../../phet-core/js/merge.js'; -import { optionize3 } from '../../phet-core/js/optionize.js'; import Tandem from '../../tandem/js/Tandem.js'; import axon from './axon.js'; import TProperty from './TProperty.js'; import TReadOnlyProperty from './TReadOnlyProperty.js'; +import PhetioObject, { PhetioObjectOptions } from '../../tandem/js/PhetioObject.js'; +import optionize from '../../phet-core/js/optionize.js'; +import { PropertyOptions } from './ReadOnlyProperty.js'; -// constants -const DEFAULT_OPTIONS = { - enabledProperty: null, - enabled: true, - enabledPropertyOptions: null, - phetioEnabledPropertyInstrumented: true, - tandem: Tandem.OPTIONAL -} as const; - -export type EnabledComponentOptions = { +type SelfOptions = { // if not provided, a Property will be created enabledProperty?: TReadOnlyProperty | null; @@ -35,17 +28,16 @@ enabled?: boolean; // options to enabledProperty if we create it, ignored if enabledProperty is provided - enabledPropertyOptions?: EnabledPropertyOptions | null; + enabledPropertyOptions?: PropertyOptions | null; // Whether or not the default-created enabledProperty should be instrumented for PhET-iO. Ignored if // options.enabledProperty is provided. phetioEnabledPropertyInstrumented?: boolean; - - // phet-io - tandem?: Tandem; }; -export default class EnabledComponent { +export type EnabledComponentOptions = SelfOptions & PhetioObjectOptions; + +export default class EnabledComponent extends PhetioObject { public enabledProperty: TProperty; @@ -53,7 +45,20 @@ public constructor( providedOptions?: EnabledComponentOptions ) { - const options = optionize3()( {}, DEFAULT_OPTIONS, providedOptions ); + const options = optionize()( { + enabledProperty: null, + enabled: true, + enabledPropertyOptions: null, + phetioEnabledPropertyInstrumented: true, + phetioState: false + }, providedOptions ); + + const tandem = options.tandem; + + const m = _.merge( {}, options, { phetioState: false } ); + + // delete options.tandem; + super( m ); const ownsEnabledProperty = !options.enabledProperty; @@ -63,7 +68,7 @@ // @ts-ignore There is no way without a plethora of parameterized types to convey if this enabledProperty is // settable, so accept unsettable, and typecast to settable. this.enabledProperty = options.enabledProperty || new EnabledProperty( options.enabled, merge( { - tandem: options.phetioEnabledPropertyInstrumented ? options.tandem.createTandem( EnabledProperty.TANDEM_NAME ) : Tandem.OPT_OUT + tandem: options.phetioEnabledPropertyInstrumented ? tandem.createTandem( EnabledProperty.TANDEM_NAME ) : Tandem.OPT_OUT }, options.enabledPropertyOptions ) ); this.disposeEnabledComponent = () => { @@ -82,8 +87,9 @@ public isEnabled(): boolean { return this.enabledProperty.value; } - public dispose(): void { + public override dispose(): void { this.disposeEnabledComponent(); + super.dispose(); } } Index: main/sun/js/buttons/RectangularRadioButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/buttons/RectangularRadioButton.ts b/main/sun/js/buttons/RectangularRadioButton.ts --- a/main/sun/js/buttons/RectangularRadioButton.ts (revision ea6b9140642c864b67bc988b3e68d2a802de95d1) +++ b/main/sun/js/buttons/RectangularRadioButton.ts (date 1665160373997) @@ -103,7 +103,7 @@ // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button const buttonModel = new ButtonModel( { - tandem: options.tandem + tandem: options.tandem.createTandem( 'buttonModel' ) } ); const interactionStateProperty = new RadioButtonInteractionStateProperty( buttonModel, property, value ); Index: main/axon/js/validate.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/validate.ts b/main/axon/js/validate.ts --- a/main/axon/js/validate.ts (revision ac84814f90d7b641712c3ad2f3fb8aa607912a0c) +++ b/main/axon/js/validate.ts (date 1665160869435) @@ -16,7 +16,7 @@ */ const validate = ( value: IntentionalAny, validator: Validator, providedOptions?: IsValidValueOptions ): void => { - if ( assert ) { + if ( assert && false) { // Throws an error if not valid const result = Validation.getValidationError( value, validator, providedOptions ); ```

Note this patch includes the new API for gravity and orbits. This patch does the following:

• In many (all?) Button classes, creation of the buttonModel is now tandem.createTandem('buttonModel') instead of tandem: tandem // pass through the parent tandem to hide the implementation detail and make it look as if the firedEmitter is on the button itself
• Lots of sketchy hack prototypes to switch tandems around to get things running.

So we are at a good point to get feedback on this question: Do we want buttonModels to remain an implementation detail, or will we be better served by creating a sub-tandem buttonModel for them, and having the emitter nested under that? This would be a breaking API change, but potentially one that could be managed by migration rules.

Alternatively, we could:

• Opt out of instrumentation for this class of EnabledComponents (not sure exactly where that would be done)
• ButtonModel could use a hack like giving its firedEmitter a tandem like tandem: tandem.parentTandem.createTandem which we don't like at all.

We are at a good point to check in with @zepumph.

[zepumph]

Wanna talk on Tuesday. I am pretty firmly in the "buttonModel is an implementation detail" camp, but I don't think I quite understand why it is valuable to instrument it directly for enabledComponent.

[samreid]

Jotting down some notes to streamline our talk on Tuesday.

ButtonModel extends EnabledComponent. If EnabledComponent is changed to extend PhetioObject, then we cannot safely pass tandem and other phet-io metadata to ButtonModel (without collisions). We create a lot of buttonModels like buttonModel = new MyButtonModel(tandem: tandem) so it thinks it has the same tandem as the parent Button. The buttonModel creates the firedEmitter. But we could come up with a way to say "don't pass phet-io metadata through to ButtonModel's super--just use it for creating the firedEmitter."

[zepumph]

(1) It duplicates the definition of tandem instead of getting it from PhetioObject:

Done

(2) Its other options are confusing, because Node has the same options, some with different types: enabledProperty, enabled, enabledPropertyOptions, phetioEnabledPropertyInstrumented.

I recommend no changes to this. enabledProperty, enabled, and enabledPropertyOptions all have essentially the same api, and where things differ, it is specific to the different implementations of the Property. EnabledComponent supports checkTandemName for example, and Node already has an established pattern for instrumenting its sub properties based on the requirements of supporting mutate (phetioEnabledPropertyInstrumented). Everything aforementioned is backed by TypeScript and, in my opinion a loud error if you mess up. Happy to discuss further if I'm not understanding fully.

(3) It doesn't provide other PhetioObjectOptions, like phetioDocumentation. So I am unable to document MoleculeAngleDragListener.ts and other DragListeners. Should EnabledComponent be a subclass of PhetioObject, perhaps with phetioState: false?

We will pick this up in https://github.com/phetsims/scenery/issues/1166

RE: https://github.com/phetsims/axon/issues/412#issuecomment-1271884484

In my opinion this work is not worth doing. I know that the iO API does not match the hierarchy, but that is how PhetioObject and the tandem tree were designed. It, in my opinion, is really amazing to not complicate the API with buttonModel everywhere, when it just isn't needed. When there is a collision while instrumenting new buttons, it is loud and obvious, and we can manage those cases as they come up (they have not come up for 7 years). This should only be progressed if we decided to over in https://github.com/phetsims/scenery/issues/1166.

TL:DR:

This issue took ~30 minutes for me to get caught up in. (3) and the patch in https://github.com/phetsims/axon/issues/412#issuecomment-1271884484 are now property of a side issue. @pixelzoom Please review (1) and (2).

[pixelzoom]

@pixelzoom Please review (1) and (2).

Done. (1) looks correct. I'm OK with "do nothing" for (2).

Feel free to close if there's nothing else to do here.

[phet-dev]

Reopening because there is a TODO marked for this issue.