RadioButtonGroup PhET-iO API revision

[arouinfar]

Related issues

• https://github.com/phetsims/phet-io/issues/1989
• AquaRadioButton and RadioButtonGroup are instrumented differently  #550
• unify radio button group implementations #523 (Out of scope for current work.)
• RectangularRadioButtons should only phet-io instrument their enabledProperty if there are >2 buttons in the group #826

RadioButtonGroup has two styles: rectangular and aqua. From the PhET-iO design perspective, the needs are the same. @samreid @zepumph and I have noted that totally unifying these components as recommended in #523 is out of scope, but the PhET-iO API should be the same (or as similar in possible) so we would like to address #550.

In general

• Address AquaRadioButton and RadioButtonGroup are instrumented differently  #550
• Review the listeners and emitters to determine if there are redundancies.
• Determine what migration rules will be necessary.

RadioButtonGroup

• Add assertions to remind devs that there should be a LinkedElement to the associated Property and that the Property itself should be featured.
• Verify that the enabledProperty and visibleProperty are instrumented in common code and phetioFeatured: true by default.

Individual Radio Buttons

• Allow for the flexibility to opt-out of instrumentation of individual radio buttons. In many cases where there are only 2 radio buttons in the group, there is no need to instrument them. (This flexibility may already exist, but documenting it anyway.) After discussion, we talked about keeping things simpler and more consistent, so having every radio button instrumented equally seems reasonable.
• Verify that visibleProperty is phetioFeatured: true. If not, feature it.
• Uninstrument enabledProperty.
  • This contradicts decisions made starting in RectangularRadioButtons should only phet-io instrument their enabledProperty if there are >2 buttons in the group #826 (comment) and may need some time at a PhET-iO meeting.
  • In my opinion, disabling individual radio buttons creates a poor UX. If clients only want students to explore a single option, they should disable the entire group or hide the unwanted options.
  • UPDATE: @samreid @arouinfar @zepumph decided that we don't need individual radio button enabledProperty elements.

displayOnlyProperty

I do not think RadioButtonGroup is a good candidate for displayOnlyProperty. My reasons are explained below.

Expand for more info.

There are a few limited cases where a display only mode for AquaRadioButtonGroup would be useful. For example:

• beersLawLab.beersLawScreen.view.detectorNode.bodyNode.radioButtonGroup
• circuitConstructionKitDc.introScreen.view.displayOptionsPanel.currentTypeRadioButtonGroup
• geometricOptics.lensScreen.view.controls.controlPanel.raysSubpanel.raysRadioButtonGroup

In Beer’s Law Lab, there is a detector that can collect either absorbance or transmittance data. Clients may only want students to collect one type of data and ignore the other. Because the radio button group serves as the detector label, it’s not possible to hide the group.

In CCK, the current can be displayed as the flow of electrons or the conventional current. Displaying only the selected radio button would clarify the chosen convention and potentially introduce important terminology.

In Geometric Optics, principal rays follow a specific convention and are of pedagogical importance. Displaying “Principal” emphasizes the chosen convention and introduces terminology, similar to the CCK example above.

If we were to implement a displayOnlyProperty the ideal behavior would be:

• inputEnabledProperty: false (no hand cursor on the Node left behind)
• Only the selected radio button is visible – individual radioButton.visibleProperty automatically controlled based on the Property.
  • This could prevent clients from hiding individual buttons, or make it way more complicated.
  • Alternatively, we could require clients to be reasonable and make sure that the only visible radio button left behind is the selected one. (This is no different from what we already expect when hiding one or more buttons in a group.)
• The aqua radio button is hidden, but its label is left behind (like Checkbox)
• For dynamic layout, the label would scoot to the left to fill in the empty space left by the radio button (unlike Checkbox)

In the above examples, the desired goals can be met by hiding all unwanted radio buttons. It does not look as clean as a true displayOnlyProperty would, but I think our time is better spent elsewhere. RadioButtonGroup does not suffer the same issues as ComboBox or Checkbox when it comes to creating a display only mode using the current functionality of PhET-iO.

Over to @samreid and @zepumph.

[zepumph]

Over in phetsims/scenery#1676 I renamed aquaRadioButton.fireListener.firedEmitter to aquaRadioButton.firedEmitter.

[samreid]

Test harness I was using for experimentation:

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: js/common/view/DensityBuoyancyScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/DensityBuoyancyScreenView.ts b/js/common/view/DensityBuoyancyScreenView.ts
--- a/js/common/view/DensityBuoyancyScreenView.ts	(revision cab58b384733ba7ab738eb93265b2710d2432700)
+++ b/js/common/view/DensityBuoyancyScreenView.ts	(date 1734366444076)
@@ -20,6 +20,7 @@
 import DynamicProperty from '../../../../axon/js/DynamicProperty.js';
 import Emitter from '../../../../axon/js/Emitter.js';
 import Property from '../../../../axon/js/Property.js';
+import StringProperty from '../../../../axon/js/StringProperty.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 import Bounds2 from '../../../../dot/js/Bounds2.js';
 import Plane3 from '../../../../dot/js/Plane3.js';
@@ -36,6 +37,8 @@
 import ResetAllButton from '../../../../scenery-phet/js/buttons/ResetAllButton.js';
 import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
 import { AlignBox, AlignBoxXAlign, AlignBoxYAlign, animatedPanZoomSingleton, Color, ColorProperty, Image, ImageableImage, LinearGradient, ManualConstraint, Mouse, Node, Pointer, Rectangle, Text } from '../../../../scenery/js/imports.js';
+import AquaRadioButtonGroup from '../../../../sun/js/AquaRadioButtonGroup.js';
+import RectangularRadioButtonGroup from '../../../../sun/js/buttons/RectangularRadioButtonGroup.js';
 import Checkbox from '../../../../sun/js/Checkbox.js';
 import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
@@ -326,6 +329,38 @@
     // This instance lives for the lifetime of the simulation, so we don't need to remove these listeners
     new DynamicProperty( this.leftBarrierViewPointPropertyProperty ).lazyLink( () => this.resizeBarrier() );
     new DynamicProperty( this.rightBarrierViewPointPropertyProperty ).lazyLink( () => this.resizeBarrier() );
+
+    const timeRadioButtonGroup = new AquaRadioButtonGroup( new StringProperty( 'ancient',{
+      tandem: options.tandem.createTandem( 'timeProperty' ),
+      phetioFeatured: true
+    } ), [ {
+      tandemName: 'ancientRadioButton',
+      createNode: () => new Text( 'Ancient', { font: new PhetFont( 12 ) } ),
+      value: 'ancient'
+    }, {
+      value: 'modern',
+      tandemName: 'modernRadioButton',
+      createNode: () => new Text( 'Modern', { font: new PhetFont( 12 ) } )
+    } ], {
+      tandem: options.tandem.createTandem( 'timeRadioButtonGroup' )
+    } );
+    this.addChild( timeRadioButtonGroup );
+
+    const spaceRadioButtonGroup = new RectangularRadioButtonGroup( new StringProperty( 'earth',{
+      tandem: options.tandem.createTandem( 'spaceProperty' ),
+      phetioFeatured: true
+    } ), [ {
+      tandemName: 'earthRadioButton',
+      createNode: () => new Text( 'Earth', { font: new PhetFont( 12 ) } ),
+      value: 'earth'
+    }, {
+      value: 'moon',
+      tandemName: 'moonRadioButton',
+      createNode: () => new Text( 'Moon', { font: new PhetFont( 12 ) } )
+    } ], {
+      tandem: options.tandem.createTandem( 'spaceRadioButtonGroup' )
+    } );
+    this.addChild( spaceRadioButtonGroup );
   }
 
   protected setRightBarrierViewPoint( rightBoxBoundsProperty: TReadOnlyProperty<Bounds2> ): void {

[samreid]

Review the listeners and emitters to determine if there are redundancies.

AquaRadioButton looks good, but we will probably want to remove pressListener from the RectangularRadioButtonGroup, which it picks up in

sun/js/buttons/ButtonNode.ts

Lines 145 to 147 in 70b6edd

	options.listenerOptions = combineOptions<PressListenerOptions>( {
	tandem: options.tandem?.createTandem( 'pressListener' )
	}, options.listenerOptions );

. Also the pressAction and releaseAction in PressListener seem like overinstrumentation, and we could eliminate them and favor firedEmitter instead. @zepumph is that accurate? Is it covered elsewhere, or should we open a side issue?

[samreid]

To uninstrument radio button enabledProperties re migration, I set out to use fine grained DeleteUninstrumentedPhetioElement like so:

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: sun/js/AquaRadioButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/AquaRadioButton.ts b/sun/js/AquaRadioButton.ts
--- a/sun/js/AquaRadioButton.ts	(revision 46abda161799f96f2d7c6c0c25daab94b138f973)
+++ b/sun/js/AquaRadioButton.ts	(date 1735528155053)
@@ -111,7 +111,7 @@
       tandem: Tandem.REQUIRED,
       tandemNameSuffix: 'RadioButton',
       visiblePropertyOptions: { phetioFeatured: true },
-      phetioEnabledPropertyInstrumented: true, // opt into default PhET-iO instrumented enabledProperty
+      phetioEnabledPropertyInstrumented: false,
 
       // pdom
       tagName: 'input',
Index: phet-io-sim-specific/repos/beers-law-lab/js/beers-law-lab-migration-processors.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phet-io-sim-specific/repos/beers-law-lab/js/beers-law-lab-migration-processors.ts b/phet-io-sim-specific/repos/beers-law-lab/js/beers-law-lab-migration-processors.ts
--- a/phet-io-sim-specific/repos/beers-law-lab/js/beers-law-lab-migration-processors.ts	(revision 4883a213e99ae87ee1a3fb218a7c6e3b064e7ffe)
+++ b/phet-io-sim-specific/repos/beers-law-lab/js/beers-law-lab-migration-processors.ts	(date 1735528732795)
@@ -123,7 +123,15 @@
       `${simName}.general.model.strings.sceneryPhet.wavelengthNMValuePatternStringProperty`
     ),
 
-    new DeleteUninstrumentedPhetioElement( `${simName}.general.model.fallbackLocalesProperty` )
+    new DeleteUninstrumentedPhetioElement( `${simName}.general.model.fallbackLocalesProperty` ),
+
+    // Individual radio button enabled properties uninstrumented in https://github.com/phetsims/sun/issues/920
+    new DeleteUninstrumentedPhetioElement( 'beersLawLab.concentrationScreen.view.solutePanel.soluteFormRadioButtonGroup.solidRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'beersLawLab.concentrationScreen.view.solutePanel.soluteFormRadioButtonGroup.solutionRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'beersLawLab.beersLawScreen.view.detectorNode.bodyNode.radioButtonGroup.transmittanceRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'beersLawLab.beersLawScreen.view.detectorNode.bodyNode.radioButtonGroup.absorbanceRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'beersLawLab.beersLawScreen.view.wavelengthPanel.radioButtonGroup.presetWavelengthRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'beersLawLab.beersLawScreen.view.wavelengthPanel.radioButtonGroup.variableWavelengthRadioButton.enabledProperty' )
   ]
 };
 
Index: sun/js/buttons/RectangularRadioButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/RectangularRadioButton.ts b/sun/js/buttons/RectangularRadioButton.ts
--- a/sun/js/buttons/RectangularRadioButton.ts	(revision 46abda161799f96f2d7c6c0c25daab94b138f973)
+++ b/sun/js/buttons/RectangularRadioButton.ts	(date 1735528247675)
@@ -109,7 +109,7 @@
     const buttonModel = new ButtonModel( {
       enabledPropertyOptions: options.enabledPropertyOptions,
       tandem: options.tandem,
-      phetioEnabledPropertyInstrumented: options.phetioEnabledPropertyInstrumented
+      phetioEnabledPropertyInstrumented: false
     } );
 
     const interactionStateProperty = new RadioButtonInteractionStateProperty( buttonModel, property, value );
Index: phet-io-sim-specific/repos/acid-base-solutions/js/acid-base-solutions-migration-processors.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phet-io-sim-specific/repos/acid-base-solutions/js/acid-base-solutions-migration-processors.ts b/phet-io-sim-specific/repos/acid-base-solutions/js/acid-base-solutions-migration-processors.ts
--- a/phet-io-sim-specific/repos/acid-base-solutions/js/acid-base-solutions-migration-processors.ts	(revision 4883a213e99ae87ee1a3fb218a7c6e3b064e7ffe)
+++ b/phet-io-sim-specific/repos/acid-base-solutions/js/acid-base-solutions-migration-processors.ts	(date 1735528732786)
@@ -16,7 +16,26 @@
 const acidBaseSolutionsMigrationProcessors: MigrationProcessors = {
   '1.4.0': [
     new DeleteUninstrumentedPhetioElement( `${simName}.general.model.fallbackLocalesProperty` ),
-    ...uninstrumentAboutDialogMenuItemVisible( simName )
+    ...uninstrumentAboutDialogMenuItemVisible( simName ),
+
+    // Individual radio button enabled properties uninstrumented in https://github.com/phetsims/sun/issues/920
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.solutionPanel.radioButtonGroup.waterRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.solutionPanel.radioButtonGroup.strongAcidRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.solutionPanel.radioButtonGroup.weakAcidRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.solutionPanel.radioButtonGroup.strongBaseRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.solutionPanel.radioButtonGroup.weakBaseRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.viewsPanel.radioButtonGroup.particlesRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.viewsPanel.radioButtonGroup.graphRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.viewsPanel.radioButtonGroup.hideViewsRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.toolsRadioButtonGroup.phMeterRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.toolsRadioButtonGroup.phPaperRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.introScreen.view.toolsRadioButtonGroup.conductivityTesterRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.mySolutionScreen.view.viewsPanel.radioButtonGroup.particlesRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.mySolutionScreen.view.viewsPanel.radioButtonGroup.graphRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.mySolutionScreen.view.viewsPanel.radioButtonGroup.hideViewsRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.mySolutionScreen.view.toolsRadioButtonGroup.phMeterRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.mySolutionScreen.view.toolsRadioButtonGroup.phPaperRadioButton.enabledProperty' ),
+    new DeleteUninstrumentedPhetioElement( 'acidBaseSolutions.mySolutionScreen.view.toolsRadioButtonGroup.conductivityTesterRadioButton.enabledProperty' )
   ]
 };
 

However, this is taking a long time and I'm facing a list of 34 simulations that need migration processors. I wonder if it would be clearer and better to factor out:

new DeleteUninstrumentedPhetioElement( 'RadioButton.enabledProperty' )

So perhaps I will begin with that, but happy to switch back if @zepumph recommends otherwise. I also note that this strategy would not work if there is nested content afterwards, like RadioButton.enabledProperty.myValueProperty since the migration processor matches substrings only.

Footnote aside:
This also reminds me this is a downside of favoring composition over types in the studio tree. For instance if RadioButtonIO had a setEnabled method, this would be clearer for migration and API maintenance.

[samreid]

I addressed the recommendations for the issue. CTQ is complaining about something I cannot reproduce, and it may be a partial pull or push. Otherwise, this issue is ready for next steps with @zepumph. Please see the two preceding comments, and spot-check review the changes.

[zepumph]

Looks really good. I went through the commits, comments, and linked issues. The "pressListener" note will be handled inhttps://github.com/phetsims/scenery/issues/1680. Ready to close.

[phet-dev]

Reopening because there is a TODO marked for this issue.