Group "items" should be specified using tandemName instead of tandem

[chrisklus]

@samreid and I noticed while adding tandem to checkboxes for https://github.com/phetsims/center-and-variability/issues/105 that both VerticalCheckboxGroup and its items use tandem - but we think instead it should match the pattern of AquaRadioButtonGroup, where the group gets a tandem and the items get a tandemName. This way, the type is responsible for nesting the checkbox tandems correctly. With the current pattern, it is easy to create an incorrect tandem structure.

[chrisklus]

As part of this work, VerticalCheckboxGroup should also be Tandem.REQUIRED, as @samreid and I did not instrument any checkboxes in CAV and we did not get any missing required tandem errors. Here is a starting patch for that:

```diff Index: js/VerticalCheckboxGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/VerticalCheckboxGroup.ts b/js/VerticalCheckboxGroup.ts --- a/js/VerticalCheckboxGroup.ts (revision 080863198c4d242d910f0c62b733690a355e2532) +++ b/js/VerticalCheckboxGroup.ts (date 1648567233932) @@ -48,7 +48,7 @@ // supertype options spacing: 10, // vertical spacing align: 'left', - tandem: Tandem.OPTIONAL + tandem: Tandem.REQUIRED }, providedOptions ); // Determine the max item width @@ -69,7 +69,7 @@ } ); const checkbox = new Checkbox( content, item.property, merge( {}, options.checkboxOptions, item.options, { - tandem: item.tandem || Tandem.OPTIONAL + tandem: item.tandem || Tandem.REQUIRED } ) ); // set pointer areas, y dimensions are computed ```

[zepumph]

I was hoping to make this change, and that it would clean up a lot of code, but unfortunately there is still the same issue when instrumenting the Node passed to the checkbox. This shouldn't stop us from making this change, but it makes me sad, for example we will still need to create the checkbox group tandem and potentially individual checkbox tandems to then properly nest the text here:

https://github.com/phetsims/build-an-atom/blob/3425685d2fde1e392a02270fae425218b0996d5e/js/common/view/BAAScreenView.js#L238-L270

[samreid]

It reminds me of the patterns discussed in https://github.com/phetsims/phet-io/issues/1617 "tension between dependency injection and tandem structure".

[samreid]

I made good progress in this patch (in TypeScript usage sites), but was uncertain how @pixelzoom would want to deal with VisibilityCheckboxGroup's function createItem( string: string, property: Property<boolean>, providedOptions: ItemOptions ): VerticalCheckboxGroupItem {

@pixelzoom can you please advise?

```diff Index: main/sun/js/VerticalCheckboxGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/VerticalCheckboxGroup.ts b/main/sun/js/VerticalCheckboxGroup.ts --- a/main/sun/js/VerticalCheckboxGroup.ts (revision e977eeee6db245999e6e16da492f58590ef76390) +++ b/main/sun/js/VerticalCheckboxGroup.ts (date 1660434628406) @@ -20,7 +20,7 @@ node: Node; // Label for the button property: Property; // Property associated with the checkbox options?: CheckboxOptions; // Item-specific options to be passed to the checkbox - tandem?: Tandem; // optional tandem for PhET-iO + tandemName?: string; // optional tandem name for PhET-iO }; type SelfOptions = { @@ -47,7 +47,7 @@ // supertype options spacing: 10, // vertical spacing align: 'left', - tandem: Tandem.OPTIONAL + tandem: Tandem.REQUIRED }, providedOptions ); // Determine the max item width @@ -72,7 +72,7 @@ } ); const checkbox = new Checkbox( item.property, content, merge( {}, options.checkboxOptions, item.options, { - tandem: item.tandem || Tandem.OPTIONAL + tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.OPTIONAL } ) ); // set pointer areas, y dimensions are computed Index: main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts b/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts --- a/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts (revision 5f81d5f0e9abdc6f3bfb4a3b104f5b9492c2f85d) +++ b/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts (date 1660434541693) @@ -61,7 +61,7 @@ // Focal Points (F) createItem( geometricOpticsStrings.checkbox.focalPoints, visibleProperties.focalPointsVisibleProperty, { iconNode: FocalPointNode.createIcon(), - tandem: options.tandem.createTandem( 'focalPointsCheckbox' ) + tandemName: 'focalPointsCheckbox' } ), // 2F Points @@ -70,7 +70,7 @@ options: { visibleProperty: GOOptions.add2FPointsCheckboxProperty }, - tandem: options.tandem.createTandem( 'twoFPointsCheckbox' ) + tandemName: 'twoFPointsCheckbox' } ) ]; @@ -82,12 +82,12 @@ options: { enabledProperty: virtualImageCheckboxEnabledProperty }, - tandem: options.tandem.createTandem( 'virtualImageCheckbox' ) + tandemName: 'virtualImageCheckbox' } ), // Labels createItem( geometricOpticsStrings.checkbox.labels, visibleProperties.labelsVisibleProperty, { - tandem: options.tandem.createTandem( 'labelsCheckbox' ) + tandemName: 'labelsCheckbox' } ), // Second Point @@ -96,7 +96,7 @@ options: { visible: !options.isBasicsVersion }, - tandem: options.tandem.createTandem( 'secondPointCheckbox' ) + tandemName: 'secondPointCheckbox' } ) ]; @@ -107,7 +107,7 @@ options: { visible: GOQueryParameters.addGuidesCheckbox }, - tandem: options.tandem.createTandem( 'guidesCheckbox' ) + tandemName: 'guidesCheckbox' } ) ); } @@ -117,7 +117,7 @@ type ItemOptions = { iconNode?: Node; -} & PickRequired & PickOptional; +} & PickRequired & PickOptional; function createItem( string: string, property: Property, providedOptions: ItemOptions ): VerticalCheckboxGroupItem { Index: main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts b/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts --- a/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts (revision 8e475f172c92b15bf64975df68a8a7e6463da44e) +++ b/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts (date 1660434461958) @@ -26,24 +26,23 @@ const tickMarksText = new Text( meanShareAndBalanceStrings.tickMarks, { fontSize: 15, maxWidth: MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH } ); // Checkbox Group - const introOptionsCheckboxGroupTandem = tandem.createTandem( 'introOptionsCheckboxGroup' ); const introOptionsCheckboxGroup = new VerticalCheckboxGroup( [ { node: predictMeanText, property: predictMeanVisibleProperty, - tandem: introOptionsCheckboxGroupTandem.createTandem( 'predictMeanCheckbox' ), + tandemName: 'predictMeanCheckbox', options: { accessibleName: meanShareAndBalanceStrings.predictMean } }, { node: meanText, property: meanVisibleProperty, - tandem: introOptionsCheckboxGroupTandem.createTandem( 'showMeanCheckbox' ), + tandemName: 'showMeanCheckbox', options: { accessibleName: meanShareAndBalanceStrings.mean } }, { node: tickMarksText, property: tickMarksVisibleProperty, - tandem: introOptionsCheckboxGroupTandem.createTandem( 'tickMarksCheckbox' ), + tandemName: 'tickMarksCheckbox', options: { accessibleName: meanShareAndBalanceStrings.tickMarks } } ], { - + tandem: tandem.createTandem( 'introOptionsCheckboxGroup' ), checkboxOptions: { boxWidth: 16 } ```

Also, as a note to myself for later, instead of:

export type VerticalCheckboxGroupItem = {
  node: Node; // Label for the button

Maybe now that we have TypeScript, we would be more comfortable with a pattern like https://github.com/phetsims/phet-io/issues/1617#issuecomment-589167877 which was omitted from the list of (1)->(6) in https://github.com/phetsims/phet-io/issues/1617#issuecomment-600806640

node: Node | ( ( t: Tandem ) => Node ); // Label for the button

Or we could use a mutually exclusive pattern to have it be node:Node | createNode: ( t: Tandem ) => Node if we want to have separate option key names for those.

[pixelzoom]

... but was uncertain how @pixelzoom would want to deal with VisibilityCheckboxGroup's function createItem ...

I'm not sure how to deal with that.

I'll also point out that in other places that use VisibilityCheckboxGroup, the checkbox labels are not being instrumented. For example, in balancing-act BasicBalaneScreenView.js:

 const indicatorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ {
      node: new Text( massLabelsString, PANEL_OPTION_FONT ),
      property: this.viewProperties.massLabelsVisibleProperty,
      label: massLabelsString,
      tandem: showPanelTandem.createTandem( 'massLabelsCheckbox' )
    }, {
...

That's a problem. And when someone notices it, then they'll have the same problem as geometric-options.

So... Maybe it's better not to make this change, and leave the API as is?

[samreid]

I added a note to my calendar to reach out to @pixelzoom in the coming week. Self-unassigning until then.

UPDATE: Oops, this comment was meant for https://github.com/phetsims/joist/issues/783#issuecomment-1229109687

[samreid]

Now that we have TypeScript, we can specify the options like so:

// Label for the button. Provide either a Node or a function that creates a Node from a Tandem (but not both)
type LabelOptions = {
  node: Node;
  createNode?: never;
} | {
  node?: never;
  createNode: ( tandem: Tandem ) => Node;
};

export type VerticalCheckboxGroupItem = {
  property: Property<boolean>; // Property associated with the checkbox
  options?: CheckboxOptions; // Item-specific options to be passed to the checkbox
  tandemName?: string; // optional tandem name for PhET-iO
} & LabelOptions;

This makes cases like VisibilityCheckboxGroup very nice:

function createItem( string: string, property: Property<boolean>, providedOptions: ItemOptions ): VerticalCheckboxGroupItem {

  return {
    createNode: tandem => {
      const labelText = new Text( string, {
        font: GOConstants.CONTROL_FONT,
        maxWidth: 90,
        tandem: tandem.createTandem( 'labelText' ),
        phetioVisiblePropertyInstrumented: false
      } );

      // Create HBox if icon is present, otherwise the label is just text.
      return providedOptions.iconNode ?
             new HBox( { children: [ labelText, providedOptions.iconNode ], spacing: 8 } ) :
             labelText;
    },
    property: property,
    options: providedOptions.options,
    tandemName: providedOptions.tandemName
  };
}

Full patch for discussion. I'll try to schedule a discussion with @pixelzoom this week.

```diff Index: main/sun/js/VerticalCheckboxGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/VerticalCheckboxGroup.ts b/main/sun/js/VerticalCheckboxGroup.ts --- a/main/sun/js/VerticalCheckboxGroup.ts (revision cf27582f16055f343f4828e87572ca2c6ba813ac) +++ b/main/sun/js/VerticalCheckboxGroup.ts (date 1661615215181) @@ -16,12 +16,20 @@ import sun from './sun.js'; import Property from '../../axon/js/Property.js'; +// Label for the button. Provide either a Node or a function that creates a Node from a Tandem (but not both) +type LabelOptions = { + node: Node; + createNode?: never; +} | { + node?: never; + createNode: ( tandem: Tandem ) => Node; +}; + export type VerticalCheckboxGroupItem = { - node: Node; // Label for the button property: Property; // Property associated with the checkbox options?: CheckboxOptions; // Item-specific options to be passed to the checkbox - tandem?: Tandem; // optional tandem for PhET-iO -}; + tandemName?: string; // optional tandem name for PhET-iO +} & LabelOptions; type SelfOptions = { checkboxOptions?: CheckboxOptions; @@ -47,13 +55,22 @@ // supertype options spacing: 10, // vertical spacing align: 'left', - tandem: Tandem.OPTIONAL + tandem: Tandem.REQUIRED }, providedOptions ); + const nodes = items.map( ( item, index ) => { + if ( item.node ) { + return item.node; + } + else { + return item.createNode( options.tandem.createTandem( item.tandemName || `optionalTandem${index}` ) ); + } + } ); + // Determine the max item width let maxItemWidth = 0; - for ( let i = 0; i < items.length; i++ ) { - maxItemWidth = Math.max( maxItemWidth, items[ i ].node.width ); + for ( let i = 0; i < nodes.length; i++ ) { + maxItemWidth = Math.max( maxItemWidth, nodes[ i ].width ); } // Create a checkbox for each item @@ -61,18 +78,19 @@ for ( let i = 0; i < items.length; i++ ) { const item = items[ i ]; + const node = nodes[ i ]; - assert && assert( !item.node.hasPDOMContent, + assert && assert( !node.hasPDOMContent, 'Accessibility is provided by Checkbox and VerticalCheckboxGroupItem.options. ' + 'Additional PDOM content in the provided Node could break accessibility.' ); // Content for the checkbox. Add an invisible strut, so that checkboxes have uniform width. const content = new Node( { - children: [ new HStrut( maxItemWidth ), item.node ] + children: [ new HStrut( maxItemWidth ), node ] } ); const checkbox = new Checkbox( item.property, content, merge( {}, options.checkboxOptions, item.options, { - tandem: item.tandem || Tandem.OPTIONAL + tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.OPTIONAL } ) ); // set pointer areas, y dimensions are computed Index: main/center-and-variability/js/common/view/BottomRepresentationCheckboxGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/center-and-variability/js/common/view/BottomRepresentationCheckboxGroup.ts b/main/center-and-variability/js/common/view/BottomRepresentationCheckboxGroup.ts --- a/main/center-and-variability/js/common/view/BottomRepresentationCheckboxGroup.ts (revision e1347139feb73c938a8906c4f9571e615cc02ae4) +++ b/main/center-and-variability/js/common/view/BottomRepresentationCheckboxGroup.ts (date 1661614359582) @@ -14,7 +14,7 @@ import Tandem from '../../../../tandem/js/Tandem.js'; import ArrowNode from '../../../../scenery-phet/js/ArrowNode.js'; import centerAndVariability from '../../centerAndVariability.js'; -import VerticalCheckboxGroup, { VerticalCheckboxGroupOptions } from '../../../../sun/js/VerticalCheckboxGroup.js'; +import VerticalCheckboxGroup, { VerticalCheckboxGroupItem, VerticalCheckboxGroupOptions } from '../../../../sun/js/VerticalCheckboxGroup.js'; import { HBox, TColor, Text } from '../../../../scenery/js/imports.js'; import CAVModel from '../model/CAVModel.js'; import CAVConstants from '../CAVConstants.js'; @@ -50,10 +50,10 @@ includePredictMedian: true }, providedOptions ); - const items = []; + const items: VerticalCheckboxGroupItem[] = []; const createPredictionItem = ( property: Property, string: string, color: TColor, spacing: number, - tandemName: string ) => { + tandemName: string ): VerticalCheckboxGroupItem => { return { node: new HBox( { spacing: spacing, @@ -71,7 +71,7 @@ ] } ), property: property, - tandem: options.tandem.createTandem( tandemName ) + tandemName: tandemName }; }; @@ -93,7 +93,7 @@ ] } ), property: model.isShowingPlayAreaMeanProperty, - tandem: options.tandem.createTandem( 'meanCheckbox' ) + tandemName: 'meanCheckbox' } ); options.includeMedian && items.push( { @@ -115,7 +115,7 @@ ] } ), property: model.isShowingPlayAreaMedianProperty, - tandem: options.tandem.createTandem( 'medianCheckbox' ) + tandemName: 'medianCheckbox' } ); super( items, options ); Index: main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts b/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts --- a/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts (revision 6c76da8168be1fc3c2ebd510b0accef66c1cdf9a) +++ b/main/geometric-optics/js/common/view/VisibilityCheckboxGroup.ts (date 1661608879002) @@ -61,7 +61,7 @@ // Focal Points (F) createItem( geometricOpticsStrings.checkbox.focalPoints, visibleProperties.focalPointsVisibleProperty, { iconNode: FocalPointNode.createIcon(), - tandem: options.tandem.createTandem( 'focalPointsCheckbox' ) + tandemName: 'focalPointsCheckbox' } ), // 2F Points @@ -70,7 +70,7 @@ options: { visibleProperty: GOPreferences.add2FPointsCheckboxProperty }, - tandem: options.tandem.createTandem( 'twoFPointsCheckbox' ) + tandemName: 'twoFPointsCheckbox' } ) ]; @@ -82,12 +82,12 @@ options: { enabledProperty: virtualImageCheckboxEnabledProperty }, - tandem: options.tandem.createTandem( 'virtualImageCheckbox' ) + tandemName: 'virtualImageCheckbox' } ), // Labels createItem( geometricOpticsStrings.checkbox.labels, visibleProperties.labelsVisibleProperty, { - tandem: options.tandem.createTandem( 'labelsCheckbox' ) + tandemName: 'labelsCheckbox' } ), // Second Point @@ -96,7 +96,7 @@ options: { visible: !options.isBasicsVersion }, - tandem: options.tandem.createTandem( 'secondPointCheckbox' ) + tandemName: 'secondPointCheckbox' } ) ]; @@ -107,7 +107,7 @@ options: { visible: GOQueryParameters.addGuidesCheckbox }, - tandem: options.tandem.createTandem( 'guidesCheckbox' ) + tandemName: 'guidesCheckbox' } ) ); } @@ -117,27 +117,27 @@ type ItemOptions = { iconNode?: Node; -} & PickRequired & PickOptional; +} & PickRequired & PickOptional; function createItem( string: string, property: Property, providedOptions: ItemOptions ): VerticalCheckboxGroupItem { - const labelText = new Text( string, { - font: GOConstants.CONTROL_FONT, - maxWidth: 90, - tandem: providedOptions.tandem.createTandem( 'labelText' ), - phetioVisiblePropertyInstrumented: false - } ); + return { + createNode: tandem => { + const labelText = new Text( string, { + font: GOConstants.CONTROL_FONT, + maxWidth: 90, + tandem: tandem.createTandem( 'labelText' ), + phetioVisiblePropertyInstrumented: false + } ); - // Create HBox if icon is present, otherwise the label is just text. - const labelNode = providedOptions.iconNode ? - new HBox( { children: [ labelText, providedOptions.iconNode ], spacing: 8 } ) : - labelText; - - return { - node: labelNode, + // Create HBox if icon is present, otherwise the label is just text. + return providedOptions.iconNode ? + new HBox( { children: [ labelText, providedOptions.iconNode ], spacing: 8 } ) : + labelText; + }, property: property, options: providedOptions.options, - tandem: providedOptions.tandem + tandemName: providedOptions.tandemName }; } Index: main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts b/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts --- a/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts (revision 19921d0de898ad03e23fb2fb3917e06e8c1269bf) +++ b/main/mean-share-and-balance/js/intro/view/IntroControlPanel.ts (date 1661608167525) @@ -45,28 +45,27 @@ maxWidth: MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH } ); - const introOptionsCheckboxGroupTandem = options.tandem.createTandem( 'introOptionsCheckboxGroup' ); const introOptionsCheckboxGroup = new VerticalCheckboxGroup( [ { node: predictMeanText, property: predictMeanVisibleProperty, options: { accessibleName: meanShareAndBalanceStrings.predictMeanProperty }, // phet-io - tandem: introOptionsCheckboxGroupTandem.createTandem( 'predictMeanCheckbox' ) + tandemName: 'predictMeanCheckbox' }, { node: meanText, property: meanVisibleProperty, options: { accessibleName: meanShareAndBalanceStrings.meanProperty }, // phet-io - tandem: introOptionsCheckboxGroupTandem.createTandem( 'meanCheckbox' ) + tandemName: 'meanCheckbox' }, { node: tickMarksText, property: tickMarksVisibleProperty, options: { accessibleName: meanShareAndBalanceStrings.tickMarksProperty }, // phet-io - tandem: introOptionsCheckboxGroupTandem.createTandem( 'tickMarksCheckbox' ) + tandemName: 'tickMarksCheckbox' }, { node: cupWaterLevel, property: cupWaterLevelVisibleProperty, ```

[samreid]

In review with @pixelzoom, we discovered IntroControlPanel.ts in MSB could use createNode. Same with CAV.

[pixelzoom]

@samreid and I reviewed today via Zoom. This looks like a great approach. If applied here, it should also be applied (eagerly? as needed?) to other "groups" that have "items".

[pixelzoom]

I just ran into this same problem in natural-selection for https://github.com/phetsims/natural-selection/issues/319. GraphChoiceRadioButtonGroup extends VerticalAquaRadioButtonGroup. I need to instrument the Text labels on the radio buttons, but I can't get the tandem hierachy correct -- to make each Text look like a child of each radio button. The createNode approach proposed above seems like it would solve that problem.

[pixelzoom]

In the above commits, I hacked together the desired tandem hierarchy (shown below) for natural-selection GraphChoiceRadioButtonGroup. It's super ugly, and should be replaced when AquaRadioButtonGroup has a better API for "items" (like the API discussed here for VerticalCheckboxGroup). There's a TODO in the code referencing this issue.

[samreid]

We'll need to assert that all cases are using tandemName like so:

assert && assert( !item.tandem, 'Use tandemName on items.  ' + item.tandem.phetioID );

[samreid]

I added tandemName and createNode as new options on VerticalCheckboxGroupItem (as an alternative to tandem + node.). I exercised it in Geometric Optics in https://github.com/phetsims/geometric-optics/commit/8f2aa3ade847840777edeb26519dd4f017ed8f44. @pixelzoom can you please review that change?

I'd like to try VerticalCheckboxGroup in gravity and orbits for https://github.com/phetsims/scenery-phet/issues/766#issuecomment-1232208143 next.

[samreid]

I have progress on AquaRadioButtonGroup (which also led to refactoring in VerticalCheckboxGroup), but won't be able to commit for a bit. @pixelzoom may want to postpone review until I have more progress on this.

[pixelzoom]

Changes to geometric-optics in https://github.com/phetsims/geometric-optics/commit/8f2aa3ade847840777edeb26519dd4f017ed8f44 look good. I'll defer further review until you're done with AquaRadioButtonGroup (and other groups?)

[samreid]

I factored out ChildComponentOptions and added it to AquaRadioButtonGroup. It seems to be working well. @pixelzoom can you please review, and comment on the 2 TODOS listed referencing this issue?

[zepumph]

I'm sorta out of this issue here, but I don't really understand how this code works:

https://github.com/phetsims/sun/blob/5bdfecf478b6355c22d9a127416efa48382918bc/js/VerticalCheckboxGroup.ts#L20-L24

ChildComponentOptions requires a tandem and not a tandemName. But the opposite is true for VerticalCheckboxGroup?

No matter, whatever you want to do here, I would recommend an assertion to help catch cases missed, like in capacitor-lab-basics:

https://github.com/phetsims/capacitor-lab-basics/blob/4ec8fc1a09f2d1e507f42a4b5bd314c297277e73/js/common/view/control/CLBViewControlPanel.js#L72

I'm also seeing this CT error in FAMB but didn't look into it.

[pixelzoom]

Problem in https://github.com/phetsims/geometric-optics/commit/fcdc2a0cb04fa002e0d1bd2132ed0826c2954d8c. You're never using the tandem argument to createNode. RaysRadioButtonGroup.ts:

    createNode: tandem => new Text( labelStringProperty, {
      font: GOConstants.CONTROL_FONT,
      maxWidth: 65,
      tandem: groupTandem.createTandem( itemTandemName ).createTandem( 'labelText' ),
      phetioVisiblePropertyInstrumented: false
    } ),

I can fix this later, unles you want to have a go. But has this been done elsewhere?

[samreid]

I can fix this later, unles you want to have a go. But has this been done elsewhere?

I addressed several cases like this, must have missed some. I'll take a look.

[samreid]

That looked like the only missed case, nice catch! I fixed it in the commit.

[samreid]

ChildComponentOptions requires a tandem and not a tandemName.

Thanks, I corrected that bug in https://github.com/phetsims/sun/commit/4ec0a6c615d317ee2242198ccb6f62e04c244056. ChildComponentOptions says: "If you pass in a Node, you can also pass in a Tandem. Or if you use createNode then you can specify tandemName".

[zepumph]

I still see the error in FAMB:

forces-and-motion-basics : phet-io-wrappers-tests : assert
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/phet-io-wrappers/phet-io-wrappers-tests.html?sim=forces-and-motion-basics&phetioDebug=true&phetioWrapperDebug=true
32 out of 33 tests passed. 1 failed.
SimTests: forces-and-motion-basics: event indexing failed:
Uncaught Error: Uncaught Error: Assertion failed: required tandems must be supplied
Error: Assertion failed: required tandems must be supplied
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/assert/js/assert.js:28:13)
at VerticalCheckboxGroup.initializePhetioObject (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/tandem/js/PhetioObject.js:144:17)
at VerticalCheckboxGroup.initializePhetioObject (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/nodes/Node.js:6368:11)
at VerticalCheckboxGroup.mutate (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/nodes/Node.js:6358:10)
at VerticalCheckboxGroup.mutate (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/util/DelayedMutate.js:70:20)
at VerticalCheckboxGroup.mutate (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/util/DelayedMutate.js:70:20)
at VerticalCheckboxGroup.mutate (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/layout/Sizable.js:110:20)
at VerticalCheckboxGroup.mutate (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/util/DelayedMutate.js:70:20)
at new FlowBox (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/layout/nodes/FlowBox.js:90:10)
at new VBox (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/chipper/dist/js/scenery/js/layout/nodes/VBox.js:13:5)

Uncaught Error: Uncaught Error: Assertion failed: required tandems must be suppliedError: Assertion failed: required tandems must be supplied
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1662033971020/assert/js/assert.js:28:13)
at assert (PhetioObject.ts:212:16)
at initializePhetioObject (Node.ts:6321:10)
at initializePhetioObject (Node.ts:6311:9)
at mutate (DelayedMutate.ts:82:19)
at mutate (DelayedMutate.ts:82:19)
at mutate (Sizable.ts:172:19)
at mutate (DelayedMutate.ts:82:19)
at mutate (FlowBox.ts:118:9)
at (VBox.ts:19:4)
id: Bayes Puppeteer
Snapshot from 9/1/2022, 6:06:11 AM

[pixelzoom]

API-wise, this is looking nice. But AquaRadioButtonGroupItem and RectangularRadioButtonItem now have very different APIs. Can/should they be more similar?

[samreid]

Thanks, I'll do that in https://github.com/phetsims/sun/issues/787

[samreid]

I would recommend an assertion to help catch cases missed, like in capacitor-lab-basics:

Passing in node and tandem is still supported. The new API if createNode + tandemName is opt-in and type checked. Can be used for new development, so I haven't added an assertion due to our precedent that we can relax assertions for new code in TypeScript.

This issue is ready for review. RectangularRadioButtonAPI will be done in https://github.com/phetsims/sun/issues/787. @pixelzoom would you like to review?

[pixelzoom]

Looking good.

One naming consideration... ChildComponentOptions seems perhaps a little too general. How about GroupItemOptions, since (so far) it's added to types that have a "GroupItem" suffix, like VerticalCheckboxGroupItem, AquaRadioButtonGroupItem, etc. Or do you intend it to be used with things that are not a "group"?

[samreid]

I renamed it in the commits.

Or do you intend it to be used with things that are not a "group"?

It was unclear how generally useful it will be. The pattern of passing tandem => Thing instead of Thing seems more broadly usable. But this pattern didn't work out of the box for https://github.com/phetsims/sun/issues/787, so GroupItemOptions sounds ok for now? Ready for re-review.

[pixelzoom]

👍🏻 closing

[samreid]

As discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs in the code referring to this issue. Reopening.

[pixelzoom]

There's also a problem with AquaRadioButtonGroupItem that I've discovered while converting BLL to TypeScript.

The node options was supposed to be retained for backward compatiblity. This TS conversion in SoluteFormRadioButtonGroup.ts should work:

    const items: AquaRadioButtonGroupItem<SoluteForm>[] = [
      {
        value: SoluteForm.SOLID,
        node: createRadioButtonLabel( BeersLawLabStrings.solid, shakerIcon_png, RADIO_BUTTON_TEXT_OPTIONS ),
        tandemName: 'solidRadioButton'
      },
      {
        value: SoluteForm.SOLUTION,
        node: createRadioButtonLabel( BeersLawLabStrings.solution, dropperIcon_png, RADIO_BUTTON_TEXT_OPTIONS ),
        tandemName: 'solutionRadioButton'
      }
    ];

Instead, I'm getting error:

TS2322: Type '{ value: SoluteForm; node: Node; tandemName: string; }' is not assignable to type 'AquaRadioButtonGroupItem'.   Type '{ value: SoluteForm; node: Node; tandemName: string; }' is not assignable to type '{ value: SoluteForm; labelContent?: string | undefined; } & { node?: undefined; tandem?: undefined; createNode: (tandem: Tandem) => Node; tandemName?: string | undefined; }'.     Property 'createNode' is missing in type '{ value: SoluteForm; node: Node; tandemName: string; }' but required in type '{ node?: undefined; tandem?: undefined; createNode: (tandem: Tandem) => Node; tandemName?: string | undefined; }'.

If I replace the node options with createNode, the error goes away:

createNode: tandem => createRadioButtonLabel( BeersLawLabStrings.solid, shakerIcon_png, RADIO_BUTTON_TEXT_OPTIONS ),

[samreid]

If specifying a node, then you must specify tandem instead of tandemName, right?

As discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs in the code referring to this issue. Reopening.

I meant to ask for feedback on those parts from the reviewer. @pixelzoom the TODO questions are:

/**
 * Get the nodes for the GroupItemOptions
 * TODO: https://github.com/phetsims/sun/issues/746 should this be renamed? Should it just be a top level `export function`,
 *       or does it belong elsewhere?
 */
export function getNodes( array: GroupItemOptions[], tandem: Tandem ): Node[] {

and

    this.disposeAquaRadioButtonGroup = () => {
      this.removeInputListener( intentListener );

      for ( let i = 0; i < radioButtons.length; i++ ) {
        radioButtons[ i ].dispose();

        // TODO: https://github.com/phetsims/sun/issues/746 what if these nodes are reused elsewhere?
        // We may not have ownership, and hence maybe shouldn't dispose?
        // We could (a) detect if the nodes[i] appears in multiple places in the scene graph (do this before dispose above)
        // Or maybe (b) only dispose ones created with createNode() since we do have ownership of those
        nodes[ i ].dispose();
      }
    };

Can you please recommend how to proceed?

[pixelzoom]

If specifying a node, then you must specify tandem instead of tandemName, right?

That's not the case. Before you started work on AquaRadioButtonGroupItem, it was defined like this:

export type AquaRadioButtonGroupItem<T> = {
  value: T; // value associated with the button
  node: Node; // label for the button
  tandemName?: string; // name of the tandem for PhET-iO
  labelContent?: string; // label for a11y
};

So two usages were broken in BLL. There may be others.

I meant to ask for feedback on those parts from the reviewer. @pixelzoom the TODO questions are: ...

For getNodes in GroupItemOptions.ts... Since it's exported, I'd probably rename it to getGroupItemNodes, to avoid name collisions. It seems fine to have it live in GroupItemOptions.ts.

For disposal of "item" Nodes... That TODO should be addressed. If you didn't create it, then you shouldn't dispose it. This can be addressed by keeping an array of the Nodes that are created via createNode.

[pixelzoom]

As noted in https://github.com/phetsims/sun/issues/746#issuecomment-1245598878, there's something very wrong with AquaRadioButtonGroupItem. And https://github.com/phetsims/sun/issues/793 was discovered today.

Raising priority and labeling as blocking.

[pixelzoom]

Re https://github.com/phetsims/sun/issues/746#issuecomment-1245598878 ... There are more sims that are using the { node: Node, tandemName: string } form of AquaRadioButtonGroupItem, which is now verbotten.

(not necessarily a complete list)

• gas-properties HoldConstantControl
• hookes-law EnergyVisibilityPanel
• hookes-law SpringForceRadioButtonGroup
• molecule-shape RealMoleculesScreenView
• states-of-matter AtomicInteractionsControlPanel
• states-of-matter ForcesAccordionBox

So you've change the AquaRadioButtonGroupItem API here without updating these usages.

[samreid]

OK if I work toward the createNode/tandemName API, and eliminate the node/tandem API for cases like these?

[pixelzoom]

That sounds nice, and is the subject of https://github.com/phetsims/sun/issues/794.

[samreid]

In https://github.com/phetsims/sun/issues/794, I converted all legacy usages to the new pattern and added assertions to guard against the legacy pattern. Can this issue be closed?

[pixelzoom]

Can this issue be closed?

No, there are 2 TODOs for this issue that have not been addressed. And the feeddback that you asked for related to getNodes has not been addressed. My feedback was in https://github.com/phetsims/sun/issues/746#issuecomment-1245598878, reproduced here:

I meant to ask for feedback on those parts from the reviewer. @pixelzoom the TODO questions are: ...

For getNodes in GroupItemOptions.ts... Since it's exported, I'd probably rename it to getGroupItemNodes, to avoid name collisions. It seems fine to have it live in GroupItemOptions.ts.

For disposal of "item" Nodes... That TODO should be addressed. If you didn't create it, then you shouldn't dispose it. This can be addressed by keeping an array of the Nodes that are created via createNode.

Note that addressing disposal of the Node return by getNodes should be easier now (it may be "do nothing"), since createNode is the only way to specify the Node for an item. Some doc that mentions that would be nice.

[samreid]

Thanks, I addressed the rename, and disposal in the commits. I also updated some documentation. I'll continue in https://github.com/phetsims/sun/issues/787. But this issue itself seems ready for review.

UPDATE: By the way, I was surprised to see that VerticalCheckboxGroup was missing a dispose method altogether (befroe we got here). That commit could use a closer review.

[pixelzoom]

Looks good, including VerticalCheckboxGroup dispose.

One nit about how Nodes that were created with getGroupItemNodesare disposed... For example in VerticalCheckboxGroup:

    this.disposeVerticalCheckboxGroup = () => {

      for ( let i = 0; i < checkboxes.length; i++ ) {
        checkboxes[ i ].dispose();

        // We own the nodes since they were created with createNode, so we can dispose them here
        nodes[ i ].dispose();
      }
    };

That implementation iterates over parallel arrays, and assumes that checkbox.length === node.length. That will probably never be a problem. But this would be more robust, and less verbose:

    this.disposeVerticalCheckboxGroup = () => {
      checkboxes.forEach( checkbox => checkbox.dispose );
      nodes.forEach( node => node.dispose );
    };

Back to @samreid to decide if he wants to do anything about this nit. Feel free to close either way.

[pixelzoom]

Inspecting places where getGroupItemNodes is called, I found 2 more memory leaks, in ToggleNode and RectangularRadioButtonGroup. Fixed in the above commits.

I also noticed that ToggleNode and RectangularRadioButtonGroup used forEach in dispose, so I change VerticalCheckboxGroup and AquaRadioButtonGroup to do the same, as described in https://github.com/phetsims/sun/issues/746#issuecomment-1279966238.

@samreid please review.

[samreid]

The changes look correct to me, thanks! Anything else to do here?

[pixelzoom]

👍🏻 closing.