Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
chrisklus opened this issue Mar 29, 2022 · 43 comments
Closed

Comments

@chrisklus
Copy link
Contributor

@samreid and I noticed while adding tandem to checkboxes for phetsims/center-and-variability#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
Copy link
Contributor Author

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:

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
Copy link
Member

zepumph commented Mar 29, 2022

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

@zepumph zepumph removed their assignment Mar 29, 2022
@samreid
Copy link
Member

samreid commented Aug 13, 2022

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

@samreid
Copy link
Member

samreid commented Aug 13, 2022

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?

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<boolean>; // 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<VerticalCheckboxGroupItem, 'tandem'> & PickOptional<VerticalCheckboxGroupItem, 'options'>;
+} & PickRequired<VerticalCheckboxGroupItem, 'tandemName'> & PickOptional<VerticalCheckboxGroupItem, 'options'>;
 
 function createItem( string: string, property: Property<boolean>, 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.

@samreid samreid assigned pixelzoom and unassigned samreid Aug 13, 2022
@pixelzoom
Copy link
Contributor

... 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?

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Aug 22, 2022
@samreid
Copy link
Member

samreid commented Aug 27, 2022

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 phetsims/joist#783 (comment)

@samreid samreid assigned samreid and unassigned samreid Aug 27, 2022
@samreid
Copy link
Member

samreid commented Aug 27, 2022

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.

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<boolean>; // 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<boolean>, 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<VerticalCheckboxGroupItem, 'tandem'> & PickOptional<VerticalCheckboxGroupItem, 'options'>;
+} & PickRequired<VerticalCheckboxGroupItem, 'tandemName'> & PickOptional<VerticalCheckboxGroupItem, 'options'>;
 
 function createItem( string: string, property: Property<boolean>, 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
Copy link
Member

samreid commented Aug 29, 2022

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

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 29, 2022

@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 pixelzoom removed their assignment Aug 29, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 29, 2022

I just ran into this same problem in natural-selection for phetsims/natural-selection#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 added a commit to phetsims/natural-selection that referenced this issue Aug 30, 2022
pixelzoom added a commit to phetsims/natural-selection that referenced this issue Aug 30, 2022
pixelzoom added a commit to phetsims/natural-selection that referenced this issue Aug 30, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 30, 2022

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.

screenshot_1838

screenshot_1836

@samreid
Copy link
Member

samreid commented Aug 30, 2022

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

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

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 29, 2022

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 #746 (comment), 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
Copy link
Member

samreid commented Oct 7, 2022

Thanks, I addressed the rename, and disposal in the commits. I also updated some documentation. I'll continue in #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
Copy link
Contributor

pixelzoom commented Oct 16, 2022

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 pixelzoom assigned samreid and unassigned pixelzoom Oct 16, 2022
pixelzoom added a commit that referenced this issue Oct 16, 2022
pixelzoom added a commit that referenced this issue Oct 16, 2022
@pixelzoom
Copy link
Contributor

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 #746 (comment).

@samreid please review.

@samreid
Copy link
Member

samreid commented Oct 18, 2022

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

@samreid samreid assigned pixelzoom and unassigned samreid Oct 18, 2022
@pixelzoom
Copy link
Contributor

👍🏻 closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants