Updates to instrumentation of scenery listeners #1676

zepumph · 2024-12-13T22:55:00Z

FireListener should uninstrument its press/releaseAction
All PhetioActions in these scenery listeners should be phetioReadOnly:true because they require a DOM event to fire
UI components like Checkbox shouldn't nest a tandem with fireListener, because we want 'firedEmitter' to be top level.

The text was updated successfully, but these errors were encountered:

zepumph · 2024-12-13T23:18:56Z

These were some of the items discussed by the phet-io team, by @samreid and I independently, and by me while implementing. I think it would be good to review this together.

zepumph · 2024-12-16T17:10:53Z

Why is there no studio button for naturalSelection.labScreen.view.resetAllButton.firedEmitter?
Why is naturalSelection.labScreen.view.environmentPanel.playButtonGroup.addAMateButton.firedEmitter phetioReadOnly:true?

samreid · 2024-12-16T22:07:10Z

This patch restores the emit button for resetAllButton but I was unable to pinpoint the time we changed EmitterIO to EmitterIO<> so I can't tell all what releases are affected.

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: js/PhetioElementView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/PhetioElementView.ts b/js/PhetioElementView.ts
--- a/js/PhetioElementView.ts	(revision fa325b5800ad17739855f7f90912970587683ca8)
+++ b/js/PhetioElementView.ts	(date 1734386778920)
@@ -673,7 +673,7 @@
     return div;
   },
 
-  EmitterIO( phetioElementView: PhetioElementView ) {
+  'EmitterIO<>'( phetioElementView: PhetioElementView ) {
     const phetioElement = phetioElementView.phetioElement;
     const div = creators.NodeIO( phetioElementView );

samreid · 2024-12-16T22:16:21Z

This patch is likely what was intended for the addAMateButton

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: js/common/view/PlayButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/PlayButtonGroup.ts b/js/common/view/PlayButtonGroup.ts
--- a/js/common/view/PlayButtonGroup.ts	(revision ff948a96c9fe819eb78591281b69de3729110577)
+++ b/js/common/view/PlayButtonGroup.ts	(date 1734387307472)
@@ -152,9 +152,9 @@
       cornerRadius: 5,
       xMargin: 12,
       yMargin: 8,
-      phetioReadOnly: true, // because sim state controls when this button is visible
       visiblePropertyOptions: {
-        phetioFeatured: false
+        phetioFeatured: false,
+        phetioReadOnly: true // because sim state controls when this button is visible
       },
       enabledPropertyOptions: {
         phetioFeatured: false

samreid · 2024-12-16T22:20:16Z

I opened phetsims/natural-selection#366 and assigned to @pixelzoom

samreid · 2025-01-07T23:39:35Z

potential goal

Review instrumentation of 'pressListener' everywhere
opt out of pressListener instrumentation where covered by firedEmitter or equivalent
wild! change PressListener's tandem to OPTIONAL by default. DragListener can leave it as REQUIRED.

TO DO:

Apply the patch (moved to Uninstrument pressListener from UI Components that have a better alternative view-side emitter. #1680)
Regenerate APIs (moved to Uninstrument pressListener from UI Components that have a better alternative view-side emitter. #1680)
Remove pressListener from radioButtons. Review the radio button firedEmitter, note that it is readOnly: false with no args. Can it be safely called by phet-io clients? But clients should trigger the underlying Property instead. (moved to Uninstrument pressListener from UI Components that have a better alternative view-side emitter. #1680)
Discuss with @arouinfar (not done, but she thumbs upped Uninstrument pressListener from UI Components that have a better alternative view-side emitter. #1680 (comment))
Note that items like buoyancy.exploreScreen.view.displayOptionsPanel.forcesSubpanel.gravityForceCheckbox.firedEmitter with NullableIO are not escaped properly, and hence show as event: NullableIO instead of NullableIO. That is a bug. UPDATE: Looks like this is coming directly from the phetioDocumentation string in Emitter. We want to escape that, but I thought there were some cases where we actually do render that as HTML, so perhaps we should escape when the doc is created.
Should Checkbox.firedEmitter be readOnly: true? Would we like it to be read-only for data stream purposes, and phet-io clients that want to change the value should operate on the BooleanProperty itself? it seems so

Subject: [PATCH] Add validateLinkedElementInstrumentation to RoundToggleButton linked property, see https://github.com/phetsims/scenery-phet/issues/890
---
Index: sun/js/buttons/RectangularPushButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/RectangularPushButton.ts b/sun/js/buttons/RectangularPushButton.ts
--- a/sun/js/buttons/RectangularPushButton.ts	(revision 445b94cec3812a6d1c4130cb8ba9958fc01f45e2)
+++ b/sun/js/buttons/RectangularPushButton.ts	(date 1736292120164)
@@ -36,7 +36,10 @@
 
     const options = optionize<RectangularPushButtonOptions, SelfOptions, SuperOptions>()( {
       soundPlayer: sharedSoundPlayers.get( 'pushButton' ),
-      tandem: Tandem.REQUIRED
+      tandem: Tandem.REQUIRED,
+      listenerOptions: {
+        tandem: Tandem.OPT_OUT // PushButtonModel provides a firedEmitter which is sufficient
+      }
     }, providedOptions );
 
     // Save the listener and add it after creating the button model.  This is done so that
Index: sun/js/buttons/RoundToggleButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/RoundToggleButton.ts b/sun/js/buttons/RoundToggleButton.ts
--- a/sun/js/buttons/RoundToggleButton.ts	(revision 445b94cec3812a6d1c4130cb8ba9958fc01f45e2)
+++ b/sun/js/buttons/RoundToggleButton.ts	(date 1736291479097)
@@ -50,7 +50,11 @@
 
       // phet-io support
       tandem: Tandem.REQUIRED,
-      phetioFeatured: true
+      phetioFeatured: true,
+
+      listenerOptions: {
+        tandem: Tandem.OPT_OUT
+      }
     }, providedOptions );
 
     // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button
Index: joist/js/NavigationBarScreenButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/NavigationBarScreenButton.ts b/joist/js/NavigationBarScreenButton.ts
--- a/joist/js/NavigationBarScreenButton.ts	(revision 4f77003d219b705c52c9220f209b90c81b41e319)
+++ b/joist/js/NavigationBarScreenButton.ts	(date 1736292067234)
@@ -20,6 +20,7 @@
 import PhetFont from '../../scenery-phet/js/PhetFont.js';
 import { Color, Node, NodeOptions, Rectangle, Text, VBox, Voicing, VoicingOptions } from '../../scenery/js/imports.js';
 import PushButtonModel from '../../sun/js/buttons/PushButtonModel.js';
+import Tandem from '../../tandem/js/Tandem.js';
 import HighlightNode from './HighlightNode.js';
 import joist from './joist.js';
 import { AnyScreen } from './Screen.js';
@@ -145,7 +146,7 @@
 
     // Hook up the input listener
     const pressListener = this.buttonModel.createPressListener( {
-      tandem: options.tandem.createTandem( 'pressListener' )
+      tandem: Tandem.OPT_OUT // PushButtonModel provides a firedEmitter which is sufficient
     } );
     this.addInputListener( pressListener );
 
Index: sun/js/buttons/RectangularToggleButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/RectangularToggleButton.ts b/sun/js/buttons/RectangularToggleButton.ts
--- a/sun/js/buttons/RectangularToggleButton.ts	(revision 445b94cec3812a6d1c4130cb8ba9958fc01f45e2)
+++ b/sun/js/buttons/RectangularToggleButton.ts	(date 1736292120088)
@@ -49,7 +49,11 @@
 
       // phet-io support
       tandem: Tandem.REQUIRED,
-      phetioFeatured: true
+      phetioFeatured: true,
+
+      listenerOptions: {
+        tandem: Tandem.OPT_OUT // PushButtonModel provides a firedEmitter which is sufficient
+      }
     }, providedOptions );
 
     // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button
Index: sun/js/buttons/RoundPushButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/buttons/RoundPushButton.ts b/sun/js/buttons/RoundPushButton.ts
--- a/sun/js/buttons/RoundPushButton.ts	(revision 445b94cec3812a6d1c4130cb8ba9958fc01f45e2)
+++ b/sun/js/buttons/RoundPushButton.ts	(date 1736292120106)
@@ -37,7 +37,10 @@
 
     const options = optionize<RoundPushButtonOptions, SelfOptions, SuperOptions>()( {
       soundPlayer: sharedSoundPlayers.get( 'pushButton' ),
-      tandem: Tandem.REQUIRED
+      tandem: Tandem.REQUIRED,
+      listenerOptions: {
+        tandem: Tandem.OPT_OUT // PushButtonModel provides a firedEmitter which is sufficient
+      }
     }, providedOptions );
 
     // Save the listener and add it after creating the button model. This is done so that
Index: joist/js/JoistButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/JoistButton.ts b/joist/js/JoistButton.ts
--- a/joist/js/JoistButton.ts	(revision 4f77003d219b705c52c9220f209b90c81b41e319)
+++ b/joist/js/JoistButton.ts	(date 1736292120199)
@@ -16,6 +16,7 @@
 import ButtonInteractionState from '../../sun/js/buttons/ButtonInteractionState.js';
 import PushButtonInteractionStateProperty from '../../sun/js/buttons/PushButtonInteractionStateProperty.js';
 import PushButtonModel from '../../sun/js/buttons/PushButtonModel.js';
+import Tandem from '../../tandem/js/Tandem.js';
 import HighlightNode from './HighlightNode.js';
 import joist from './joist.js';
 
@@ -115,7 +116,7 @@
 
     // Hook up the input listener
     this._pressListener = this.buttonModel.createPressListener( {
-      tandem: options.tandem.createTandem( 'pressListener' )
+      tandem: Tandem.OPT_OUT // PushButtonModel provides a firedEmitter which is sufficient
     } );
     this.addInputListener( this._pressListener );

zepumph · 2025-01-08T22:35:22Z

(done in Uninstrument pressListener from UI Components that have a better alternative view-side emitter. #1680) Just noting here that this issue has taken on the responsibility of pruning pressListener instances from multiple button types (see patch above). Let's make sure that get's committed, as issues like ComboBox PhET-iO API revision sun#922 are now relying on that cleanup.

zepumph · 2025-01-10T21:41:16Z

I made #1680 for specific pressListener questions. It seems big enough for its own issue.

zepumph · 2025-01-10T22:52:36Z

With this patch, I can go to Buoyancy and fire a radio button like so: phetioClient.invoke( 'buoyancy.compareScreen.view.blocksPanel.blockSetRadioButtonGroup.sameMassRadioButton.firedEmitter', 'emit', [null]).

Subject: [PATCH] fdsa
---
Index: js/listeners/FireListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/listeners/FireListener.ts b/js/listeners/FireListener.ts
--- a/js/listeners/FireListener.ts	(revision 3bcd0d9fae967d36abf81f1a9cf51af0bfb7f139)
+++ b/js/listeners/FireListener.ts	(date 1736549438921)
@@ -59,7 +59,7 @@
       phetioReleaseActionInstrumented: false,
 
       // Though FireListener is not instrumented, declare these here to support properly passing this to children, see https://github.com/phetsims/tandem/issues/60.
-      phetioReadOnly: true
+      phetioReadOnly: false
     }, providedOptions );
 
     assert && assert( typeof options.fire === 'function', 'The fire callback should be a function' );

That said, I don't recommend this as a change. I believe that we are done with the list of items found from @samreid and I partial co-review. Let's meet back up and discuss what is left for this issue.

zepumph · 2025-01-13T23:38:24Z

I went through this issue with @samreid, we are in agreement about the core state of things:

PressListener remains Tandem.REQUIRED, and usages of it in the current codebase don't have a better view-side element that can replace it. That's ok! If they ever get re-designed, then they can have this work done to them (like with a firedEmitter).
FireListener.firedEmitter should still remain phetioReadOnly:true. It isn't reasonable to assume all usages of it work well when fired from PhET-iO. Instead we think of PushButtonModel.firedEmitter (and radiobutton) as the exception that we can assume we can trigger from PhET-iO. Probably this should be renamed to fireEmitter, that would make more sense, but we are never going incur that cost. There just isn't enough value there.

Everything else is in side issues, and we are ready to close

zepumph added a commit to phetsims/sun that referenced this issue Dec 13, 2024

updates to FireListener instrumentation, phetsims/scenery#1676

f03d623

zepumph added a commit that referenced this issue Dec 13, 2024

updates to FireListener instrumentation, #1676

deaf7b9

zepumph self-assigned this Dec 13, 2024

zepumph added a commit that referenced this issue Dec 13, 2024

add TODO issue, #1676

ba50b0f

zepumph mentioned this issue Dec 13, 2024

Checkbox mini-sprint phetsims/sun#914

Closed

zepumph added the status:ready-for-review label Dec 13, 2024

zepumph assigned samreid and unassigned zepumph Dec 13, 2024

zepumph mentioned this issue Dec 13, 2024

RadioButtonGroup PhET-iO API revision phetsims/sun#920

Closed

8 tasks

samreid added the dev:help-wanted label Dec 16, 2024

samreid removed the dev:help-wanted label Dec 16, 2024

samreid mentioned this issue Dec 16, 2024

addAMateButton is incorrectly phetioReadOnly: true phetsims/natural-selection#366

Closed

samreid added the dev:help-wanted label Dec 30, 2024

samreid assigned zepumph Dec 30, 2024

samreid mentioned this issue Dec 31, 2024

ComboBox PhET-iO API revision phetsims/sun#922

Closed

20 tasks

zepumph removed the status:ready-for-review label Jan 7, 2025

samreid removed their assignment Jan 8, 2025

samreid removed the dev:help-wanted label Jan 8, 2025

zepumph mentioned this issue Jan 10, 2025

Uninstrument pressListener from UI Components that have a better alternative view-side emitter. #1680

Closed

This was referenced Jan 10, 2025

Escape phetioDocumentation for Emitters phetsims/tandem#318

Closed

FireListener's options.fire must support null #1681

Closed

zepumph added the dev:help-wanted label Jan 10, 2025

zepumph added a commit that referenced this issue Jan 10, 2025

better typescript, #1676

695e30e

zepumph added the status:ready-for-review label Jan 10, 2025

zepumph added a commit that referenced this issue Jan 10, 2025

remove TODO, #1676

3bcd0d9

zepumph assigned samreid Jan 10, 2025

zepumph closed this as completed Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to instrumentation of scenery listeners #1676

Updates to instrumentation of scenery listeners #1676

zepumph commented Dec 13, 2024 •

edited

Loading

zepumph commented Dec 13, 2024

zepumph commented Dec 16, 2024 •

edited

Loading

samreid commented Dec 16, 2024

samreid commented Dec 16, 2024

samreid commented Dec 16, 2024

samreid commented Jan 7, 2025 •

edited by zepumph

Loading

zepumph commented Jan 8, 2025 •

edited

Loading

zepumph commented Jan 10, 2025

zepumph commented Jan 10, 2025

zepumph commented Jan 13, 2025

Updates to instrumentation of scenery listeners #1676

Updates to instrumentation of scenery listeners #1676

Comments

zepumph commented Dec 13, 2024 • edited Loading

zepumph commented Dec 13, 2024

zepumph commented Dec 16, 2024 • edited Loading

samreid commented Dec 16, 2024

samreid commented Dec 16, 2024

samreid commented Dec 16, 2024

samreid commented Jan 7, 2025 • edited by zepumph Loading

zepumph commented Jan 8, 2025 • edited Loading

zepumph commented Jan 10, 2025

zepumph commented Jan 10, 2025

zepumph commented Jan 13, 2025

zepumph commented Dec 13, 2024 •

edited

Loading

zepumph commented Dec 16, 2024 •

edited

Loading

samreid commented Jan 7, 2025 •

edited by zepumph

Loading

zepumph commented Jan 8, 2025 •

edited

Loading