phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

AquaRadioButtonGroupItem results in incorrect tandem #793

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

@veillette and I ran across this problem today in Calculus Grapher. Using this form of AquaRadioButtonGroupItem:

{ 
  value: T,
  node: Node,
  tandem: Tandem
}

... we were unable to get proper tandems for the item, and kept getting "placeholder" prefixes:

screenshot_1893

Here's a patch that will demonstrate the problem:

patch ```diff Index: js/common/view/CurveManipulationModeRadioButtonGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CurveManipulationModeRadioButtonGroup.ts b/js/common/view/CurveManipulationModeRadioButtonGroup.ts --- a/js/common/view/CurveManipulationModeRadioButtonGroup.ts (revision a35393f5262ec058905532546334f58746a99619) +++ b/js/common/view/CurveManipulationModeRadioButtonGroup.ts (date 1664308445975) @@ -31,8 +31,8 @@ mode => { return { value: mode, - createNode: () => new Text( mode.toString() ), - tandemName: `${mode.tandemPrefix}${AquaRadioButton.TANDEM_NAME_SUFFIX}` + node: new Text( mode.toString() ), + tandem: options.tandem.createTandem( `${mode.tandemPrefix}${AquaRadioButton.TANDEM_NAME_SUFFIX}` ) }; } ); ```

This form of AquaRadioButtonGroupItem seems to be working fine:

{ 
  value: T,
  createNode: ( tandem: Tandem ) => Node,
  tandemName: string
}
pixelzoom commented 1 year ago

The bug is in AquaRadioButtonGroup, line 118:

      const radioButton = new AquaRadioButton( property, item.value, content,
...
          // Instead of using Tandem.REQUIRED, use the same tandem that is passed into the group, helping to support Tandem.OPT_OUT
118       tandem: options.tandem.createTandem( item.tandemName || `placeHolder${dotRandom.nextInt( 1000000 )}RadioButton` )
        }, options.radioButtonOptions! ) );

So item.tandem is ignored, never used. This is also likely to be the source of the problem reported in https://github.com/phetsims/sun/issues/746#issuecomment-1245598878.

pixelzoom commented 1 year ago

This is also a problem:

placeHolder${dotRandom.nextInt( 1000000 )}RadioButton

It should be:

placeHolder${dotRandom.nextInt( 1000000 )}${AquaRadioButton.TANDEM_NAME_SUFFIX}

pixelzoom commented 1 year ago

Yikes, there's an even bigger problem here:

placeHolder${dotRandom.nextInt( 1000000 )}RadioButton

(1) The tandem names will change on each run, breaking the API. (2) The tandem names are not guaranteed to be unique.

samreid commented 1 year ago

@pixelzoom and I collaborated on fixing the problem. We are considering next whether we can just abandon the undesirable node/tandem API.

pixelzoom commented 1 year ago

Simplifying GroupItemOptions will be tracked in https://github.com/phetsims/sun/issues/794.

Closing.