circuitChangedEmitter doesn't always trigger when expected

[samreid]

Notes from discussion with @arouinfar:

circuitChangedEmitter

• Is this overeager? Cutting a vertex with N connected elements fires this emitter N+1 times, regardless of element type.
  • Not overeager, just an implementation detail. Clients can filter as they like.
• Fires the emitter once:
  • Connecting a vertex to another vertex (regardless of how many elements are connected to these vertices)
  • Updating R, V, etc.
• Does not fire the emitter, should it?
  • Opening/closing switch
  • Reversing battery
  • Blowing fuse
  • Seems like a bug, let’s open an issue.

We decided it is OK for the circuitChangedEmitter to trigger many times. But we were concerned it did not trigger for cases like:

• Opening/closing switch
• Reversing battery
• Blowing fuse

[samreid]

Patch from failed attempt:

```diff Subject: [PATCH] Add phetioDocumentation, see https://github.com/phetsims/chipper/issues/1302 --- Index: js/model/CircuitConstructionKitModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/CircuitConstructionKitModel.ts b/js/model/CircuitConstructionKitModel.ts --- a/js/model/CircuitConstructionKitModel.ts (revision 05818b7d39ad432d34623d6151eba6663f72101f) +++ b/js/model/CircuitConstructionKitModel.ts (date 1674081834763) @@ -226,19 +226,6 @@ this.circuit.circuitElements.lengthProperty.link( pause ); } - // Broad channel for PhET-iO that signifies a change in the circuit. Wrapper listeners can call get state after circuit - // changes to obtain the new circuit. - const circuitChangedEmitter = new Emitter( { - tandem: circuitTandem.createTandem( 'circuitChangedEmitter' ), - phetioDocumentation: 'Emits when any circuit model parameter or topology has changed', - phetioReadOnly: true - } ); - - const emitCircuitChanged = () => circuitChangedEmitter.emit(); - this.circuit.vertexGroup.elementCreatedEmitter.addListener( emitCircuitChanged ); - this.circuit.vertexGroup.elementDisposedEmitter.addListener( emitCircuitChanged ); - this.circuit.componentEditedEmitter.addListener( emitCircuitChanged ); - // When the simulation pauses and resumes, clear the time scaling factor (so it doesn't show a stale value) this.isValueDepictionEnabledProperty.link( () => this.circuit.chargeAnimator.timeScaleRunningAverage.clear() ); Index: js/model/Circuit.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/Circuit.ts b/js/model/Circuit.ts --- a/js/model/Circuit.ts (revision 05818b7d39ad432d34623d6151eba6663f72101f) +++ b/js/model/Circuit.ts (date 1674081834771) @@ -237,20 +237,28 @@ // When a Charge is removed from the list, dispose it this.charges.addItemRemovedListener( charge => charge.dispose() ); - this.circuitChangedEmitter = new Emitter(); + // Broad channel for PhET-iO that signifies a change in the circuit. Wrapper listeners can call get state after circuit + // changes to obtain the new circuit. + this.circuitChangedEmitter = new Emitter( { + tandem: tandem.createTandem( 'circuitChangedEmitter' ), + phetioDocumentation: 'Emits when any circuit model parameter or topology has changed', + phetioReadOnly: true + } ); + + const emitCircuitChanged = () => { + this.dirty = true; + this.circuitChangedEmitter.emit(); + }; + this.vertexDroppedEmitter = new Emitter( { parameters: [ { valueType: Vertex } ] } ); this.componentEditedEmitter = new Emitter(); + this.componentEditedEmitter.addListener( emitCircuitChanged ); this.selectionProperty = new Property( null, { tandem: tandem.createTandem( 'selectionProperty' ), phetioValueType: NullableIO( ReferenceIO( OrIO( [ CircuitElement.CircuitElementIO, Vertex.VertexIO ] ) ) ) } ); - const emitCircuitChanged = () => { - this.dirty = true; - this.circuitChangedEmitter.emit(); - }; - this.vertexGroup = new PhetioGroup( ( tandem, position ) => { return new Vertex( position, this.selectionProperty, { tandem: tandem, @@ -268,6 +276,8 @@ const filtered = this.vertexGroup.filter( candidateVertex => vertex === candidateVertex ); assert && assert( filtered.length === 1, 'should only have one copy of each vertex' ); + + emitCircuitChanged(); } ); // Stop watching the vertex positions for updating the voltmeter and ammeter @@ -283,6 +293,8 @@ // More sanity checks for the listeners assert && assert( !vertex.positionProperty.hasListener( emitCircuitChanged ), 'Listener should be removed' ); + + emitCircuitChanged(); } ); this.stepActions = []; ```

[arouinfar]

Related to #917

[samreid]

I added circuitChangedEmitter emits for:

• switch toggled
• battery reversed
• fuse tripped or reset

@arouinfar can you please review/test?

[arouinfar]

Looks good, thanks @samreid!