Simplify disposal code #455
Labels
Comments
|
Here is a more elaborate example applying the pattern to Bending Light usages of Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: bending-light/js/prisms/model/Prism.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/model/Prism.ts b/bending-light/js/prisms/model/Prism.ts
--- a/bending-light/js/prisms/model/Prism.ts (revision b2cffd015c0ebc7c9a35d265d4c5470541a8bf5c)
+++ b/bending-light/js/prisms/model/Prism.ts (date 1733477434502)
@@ -37,10 +37,7 @@
this.typeName = typeName;
- this.disposeEmitter.addListener( () => {
- this.shapeProperty.dispose();
- this.positionProperty.dispose();
- } );
+ this.addDisposables( this.shapeProperty, this.positionProperty );
}
/**
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision b2cffd015c0ebc7c9a35d265d4c5470541a8bf5c)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1733477953604)
@@ -54,9 +54,7 @@
this.addChild( this.knobNode );
}
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposables( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -88,10 +86,8 @@
this.knobNode.addInputListener( knobDragListener );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addInputListenerNode( this.knobNode, knobDragListener );
+ this.addDisposables( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -108,15 +104,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposables( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ this.lazyLinkProperty( dragBoundsProperty, keepInBounds );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -135,27 +126,14 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.addInputListenerNode( this.prismPathNode, this.dragListener );
+ this.addDisposables( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
-
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ this.linkProperty( prism.positionProperty, prismShapeListener );
+ this.linkProperty( prism.shapeProperty, prismShapeListener );
+ this.linkProperty( prismsModel.mediumColorFactory.lightTypeProperty, prismColorListener );
+ this.linkProperty( prismsModel.prismMediumProperty, prismColorListener );
this.updatePrismShape();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/Disposable.ts (date 1733477892792)
@@ -13,7 +13,9 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import { Node, TInputListener } from '../../scenery/js/imports.js';
import axon from './axon.js';
+import Property from './Property.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -87,6 +89,25 @@
this._isDisposed = true;
}
+ public linkProperty<T>( property: Property<T>, listener: ( playing: T ) => void ): void {
+ property.link( listener );
+ this.disposeEmitter.addListener( () => property.unlink( listener ) );
+ }
+
+ public lazyLinkProperty<T>( property: Property<T>, listener: ( playing: T ) => void ): void {
+ property.lazyLink( listener );
+ this.disposeEmitter.addListener( () => property.unlink( listener ) );
+ }
+
+ public addDisposables( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ public addInputListenerNode( node: Node, inputListener: TInputListener ): void {
+ node.addInputListener( inputListener );
+ this.disposeEmitter.addListener( () => node.removeInputListener( inputListener ) );
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Here is a picture of how the code becomes easier to understand on the right (a diff of PrismNode): 
Concerns with the patch:
|
|
Patch with names unified to begin with Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: bending-light/js/prisms/model/Prism.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/model/Prism.ts b/bending-light/js/prisms/model/Prism.ts
--- a/bending-light/js/prisms/model/Prism.ts (revision b2cffd015c0ebc7c9a35d265d4c5470541a8bf5c)
+++ b/bending-light/js/prisms/model/Prism.ts (date 1733477434502)
@@ -37,10 +37,7 @@
this.typeName = typeName;
- this.disposeEmitter.addListener( () => {
- this.shapeProperty.dispose();
- this.positionProperty.dispose();
- } );
+ this.addDisposables( this.shapeProperty, this.positionProperty );
}
/**
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision b2cffd015c0ebc7c9a35d265d4c5470541a8bf5c)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1733479534938)
@@ -54,9 +54,7 @@
this.addChild( this.knobNode );
}
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposables( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -88,10 +86,8 @@
this.knobNode.addInputListener( knobDragListener );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposableInputListener( this.knobNode, knobDragListener );
+ this.addDisposables( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -108,15 +104,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposables( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ this.addDisposableLazyLink( dragBoundsProperty, keepInBounds );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -135,27 +126,14 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.addDisposableInputListener( this.prismPathNode, this.dragListener );
+ this.addDisposables( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
-
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ this.addDisposableLink( prism.positionProperty, prismShapeListener );
+ this.addDisposableLink( prism.shapeProperty, prismShapeListener );
+ this.addDisposableLink( prismsModel.mediumColorFactory.lightTypeProperty, prismColorListener );
+ this.addDisposableLink( prismsModel.prismMediumProperty, prismColorListener );
this.updatePrismShape();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/Disposable.ts (date 1733479723713)
@@ -13,8 +13,10 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import { Node, TInputListener } from '../../scenery/js/imports.js';
import axon from './axon.js';
-import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
+import Property from './Property.js';
+import TEmitter, { TEmitterListener, TEmitterParameter, TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
// Used in subclasses to support mutate.
@@ -87,6 +89,42 @@
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposables( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * For Emitter.ts, add a listener that is removed on dispose
+ */
+ public addDisposableListener<T extends TEmitterParameter[]>( emitter: TReadOnlyEmitter<T>, listener: TEmitterListener<T> ): void {
+ emitter.addListener( listener );
+ this.disposeEmitter.addListener( () => emitter.removeListener( listener ) );
+ }
+
+ /**
+ * For Property.ts, add a listener that is removed on dispose
+ */
+ public addDisposableLink<T>( property: Property<T>, listener: ( playing: T ) => void ): void {
+ property.link( listener );
+ this.disposeEmitter.addListener( () => property.unlink( listener ) );
+ }
+
+ public addDisposableLazyLink<T>( property: Property<T>, listener: ( playing: T ) => void ): void {
+ property.lazyLink( listener );
+ this.disposeEmitter.addListener( () => property.unlink( listener ) );
+ }
+
+ /**
+ * For Node.ts, add an input listener that is removed on dispose
+ */
+ public addDisposableInputListener( node: Node, inputListener: TInputListener ): void {
+ node.addInputListener( inputListener );
+ this.disposeEmitter.addListener( () => node.removeInputListener( inputListener ) );
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text |
|
After discussion with @zepumph, some great ideas! Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: bending-light/js/prisms/model/Prism.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/model/Prism.ts b/bending-light/js/prisms/model/Prism.ts
--- a/bending-light/js/prisms/model/Prism.ts (revision 194957d8eda874998274cac81be5137d89340e08)
+++ b/bending-light/js/prisms/model/Prism.ts (date 1733504023685)
@@ -37,10 +37,7 @@
this.typeName = typeName;
- this.disposeEmitter.addListener( () => {
- this.shapeProperty.dispose();
- this.positionProperty.dispose();
- } );
+ this.addDisposables( this.shapeProperty, this.positionProperty );
}
/**
Index: scenery-phet/js/buttons/PlayPauseStepButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/buttons/PlayPauseStepButtonGroup.ts b/scenery-phet/js/buttons/PlayPauseStepButtonGroup.ts
--- a/scenery-phet/js/buttons/PlayPauseStepButtonGroup.ts (revision 22a84d402b1da679dfbc1cf5a04e29e450900988)
+++ b/scenery-phet/js/buttons/PlayPauseStepButtonGroup.ts (date 1733505218619)
@@ -141,6 +141,7 @@
phetioDocumentation: 'Progress the simulation a single model step forwards.'
}, options.stepForwardButtonOptions ) );
children.push( stepForwardButton );
+
}
let stepBackwardButton: StepBackwardButton | null = null;
@@ -179,10 +180,12 @@
this.disposePlayPauseStepButtonGroup = () => {
isPlayingProperty.unlink( playingListener );
- playPauseButton.dispose();
stepForwardButton && stepForwardButton.dispose();
stepBackwardButton && stepBackwardButton.dispose();
};
+
+ this.addDisposables( playPauseButton, stepForwardButton, stepBackwardButton );
+ stepForwardButton && this.addDisposables( stepForwardButton );
}
/**
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision 194957d8eda874998274cac81be5137d89340e08)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1733504818358)
@@ -54,9 +54,7 @@
this.addChild( this.knobNode );
}
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposables( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -88,10 +86,8 @@
this.knobNode.addInputListener( knobDragListener );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposableInputListener( this.knobNode, knobDragListener );
+ this.addDisposables( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -108,15 +104,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposables( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ this.addDisposableLazyLink( dragBoundsProperty, keepInBounds );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -135,27 +126,14 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.addDisposableInputListener( this.prismPathNode, this.dragListener );
+ this.addDisposables( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
-
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ this.addDisposableLink( prism.positionProperty, prismShapeListener );
+ this.addDisposableLink( prism.shapeProperty, prismShapeListener );
+ this.addDisposableLink( prismsModel.mediumColorFactory.lightTypeProperty, prismColorListener );
+ this.addDisposableLink( prismsModel.prismMediumProperty, prismColorListener );
this.updatePrismShape();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/Disposable.ts (date 1733505218611)
@@ -13,7 +13,9 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import { TInputListener } from '../../scenery/js/imports.js';
import axon from './axon.js';
+import Property from './Property.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -82,11 +84,53 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup.
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code)
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter.
+
this._disposeEmitter.dispose();
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposables( ...disposables: ( Disposable | undefined )[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * For Emitter.ts, add a listener that is removed on dispose
+ */
+ public addDisposableListener<Listener>( emitter: { addListener: ( listener: Listener ) => void, removeListener: ( listener: Listener ) => void }, listener: Listener ): void {
+ emitter.addListener( listener );
+ this.disposeEmitter.addListener( () => emitter.removeListener( listener ) );
+ }
+
+ /**
+ * For Property.ts, add a listener that is removed on dispose
+ */
+ public addDisposableLink<T>( property: { link: ( listener: T ) => void }, listener: ( playing: T ) => void ): void {
+ property.link( listener );
+ this.disposeEmitter.addListener( () => property.unlink( listener ) );
+ }
+
+ public addDisposableLazyLink<T>( property: Property<T>, listener: ( playing: T ) => void ): void {
+ property.lazyLink( listener );
+ this.disposeEmitter.addListener( () => property.unlink( listener ) );
+ }
+
+ /**
+ * For Node.ts, add an input listener that is removed on dispose
+ */
+ public addDisposableInputListener( node: { addInputListener: ( t: TInputListener ) => void; removeInputListener: ( t: TInputListener ) }, inputListener: TInputListener ): void {
+ node.addInputListener( inputListener );
+ this.disposeEmitter.addListener( () => node.removeInputListener( inputListener ) );
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text |
|
We like this pattern better: this.prismPathNode.addInputListener( this.dragListener, { disposable: this } );
prism.shapeProperty.link( prismShapeListener, { disposable: this } );type TReadOnlyEmitterOptions = {
disposable?: Disposable;
};
// A "read only" version of an emitter. You can listen to it, but cannot control it
export type TReadOnlyEmitter<T extends TEmitterParameter[] = []> = {
addListener: ( listener: TEmitterListener<T>, options?: TReadOnlyEmitterOptions ) => void; |
|
Current proposal based on discussion with @zepumph Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: axon/js/TEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TEmitter.ts b/axon/js/TEmitter.ts
--- a/axon/js/TEmitter.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/TEmitter.ts (date 1733514588989)
@@ -9,6 +9,7 @@
*/
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
+import { HasDisposableOptions } from './Disposable.js';
// undefined and never are not allowed as parameters to Emitter
export type TEmitterParameter = Exclude<IntentionalAny, undefined | never>;
@@ -18,7 +19,7 @@
// A "read only" version of an emitter. You can listen to it, but cannot control it
export type TReadOnlyEmitter<T extends TEmitterParameter[] = []> = {
- addListener: ( listener: TEmitterListener<T> ) => void;
+ addListener: ( listener: TEmitterListener<T>, options?: HasDisposableOptions ) => void;
hasListener: ( listener: TEmitterListener<T> ) => boolean;
removeListener: ( listener: TEmitterListener<T> ) => void;
Index: bending-light/js/prisms/model/Prism.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/model/Prism.ts b/bending-light/js/prisms/model/Prism.ts
--- a/bending-light/js/prisms/model/Prism.ts (revision 194957d8eda874998274cac81be5137d89340e08)
+++ b/bending-light/js/prisms/model/Prism.ts (date 1733512761617)
@@ -37,10 +37,7 @@
this.typeName = typeName;
- this.disposeEmitter.addListener( () => {
- this.shapeProperty.dispose();
- this.positionProperty.dispose();
- } );
+ this.addDisposable( this.shapeProperty, this.positionProperty );
}
/**
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision 194957d8eda874998274cac81be5137d89340e08)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1733514915118)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposable: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -108,15 +102,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposable: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -135,27 +124,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposable: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposable: this } );
+ prism.positionProperty.link( prismShapeListener, { disposable: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposable: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposable: this } );
this.updatePrismShape();
}
Index: axon/js/TinyEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts
--- a/axon/js/TinyEmitter.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/TinyEmitter.ts (date 1733514685964)
@@ -12,6 +12,7 @@
import Pool, { TPoolable } from '../../phet-core/js/Pool.js';
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
+import { HasDisposableOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
// constants
@@ -233,7 +234,7 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: HasDisposableOptions ): void {
assert && assert( !this.isDisposed, 'Cannot add a listener to a disposed TinyEmitter' );
assert && assert( !this.hasListener( listener ), 'Cannot add the same listener twice' );
@@ -250,6 +251,8 @@
console.log( `Max TinyEmitter listeners: ${maxListenerCount}` );
assert( maxListenerCount <= listenerLimit, `listener count of ${maxListenerCount} above ?listenerLimit=${listenerLimit}` );
}
+
+ options?.disposable?.disposeEmitter.addListener( () => this.removeListener( listener ) );
}
/**
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/Disposable.ts (date 1733514588977)
@@ -22,7 +22,15 @@
isDisposable?: boolean;
};
-class Disposable {
+type HasDisposeEmitter = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type HasDisposableOptions = {
+ disposable?: HasDisposeEmitter;
+};
+
+class Disposable implements HasDisposeEmitter {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -82,11 +90,24 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision f744a1526185eeb12aed37875f3ee194e8b3181e)
+++ b/scenery/js/nodes/Node.ts (date 1733514589009)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { HasDisposableOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2334,7 +2335,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: HasDisposableOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2350,7 +2351,10 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ options?.disposable?.disposeEmitter.addListener( () => this.removeInputListener( listener ) );
}
+
return this;
}
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 46d1abcde81e4450fb8663e2a5ebf383fc01c7bf)
+++ b/axon/js/ReadOnlyProperty.ts (date 1733514588985)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { HasDisposableOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
@@ -85,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & HasDisposableOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -439,6 +440,9 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ options?.disposable?.disposeEmitter?.addListener( () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,6 +455,8 @@
this.addPhetioStateDependencies( options.phetioDependencies );
}
this.tinyProperty.lazyLink( listener );
+
+ options?.disposable?.disposeEmitter?.addListener( () => this.unlink( listener ) );
}
/**
For review/discussion:
|
|
I would like to discuss further this issue. In general I think it is ready for main. I'm worried about the notion of undoing these (unlink/removeListener/etc support before dispose). It seems like the API should support it, but in practice it isn't too valuable or clear. Also instead of "HasDisposable", how about "Disposer". We are talking about the thing that is responsible for disposing, so it would make sense to have |
|
The main point from above is to make sure that clients that register like so: this.knobNode.addInputListener( knobDragListener, { disposable: this } );Will get an error if they try to remove the listener like so: this.knobNode.removeInputListener( knobDragListener );Since we do not want a memory leak on the disposeEmitter listeners. |
|
This patch throws an assertion error if you try to removeInputListener on something that was managed by a disposer: Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1734474535393)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposer: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +107,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposer: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +129,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposer: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposer: this } );
+ prism.positionProperty.link( prismShapeListener, { disposer: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposer: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposer: this } );
this.updatePrismShape();
}
Index: axon/js/TinyEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts
--- a/axon/js/TinyEmitter.ts (revision 1e4b7e8543cc0b83508fb21843122cf27372d94b)
+++ b/axon/js/TinyEmitter.ts (date 1734474681035)
@@ -12,6 +12,7 @@
import Pool, { TPoolable } from '../../phet-core/js/Pool.js';
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
// constants
@@ -233,7 +234,7 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
assert && assert( !this.isDisposed, 'Cannot add a listener to a disposed TinyEmitter' );
assert && assert( !this.hasListener( listener ), 'Cannot add the same listener twice' );
@@ -250,6 +251,8 @@
console.log( `Max TinyEmitter listeners: ${maxListenerCount}` );
assert( maxListenerCount <= listenerLimit, `listener count of ${maxListenerCount} above ?listenerLimit=${listenerLimit}` );
}
+
+ options?.disposer?.disposeEmitter.addListener( () => this.removeListener( listener ) );
}
/**
Index: axon/js/TEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TEmitter.ts b/axon/js/TEmitter.ts
--- a/axon/js/TEmitter.ts (revision 1e4b7e8543cc0b83508fb21843122cf27372d94b)
+++ b/axon/js/TEmitter.ts (date 1734474681031)
@@ -9,6 +9,7 @@
*/
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
+import { DisposerOptions } from './Disposable.js';
// undefined and never are not allowed as parameters to Emitter
export type TEmitterParameter = Exclude<IntentionalAny, undefined | never>;
@@ -18,7 +19,7 @@
// A "read only" version of an emitter. You can listen to it, but cannot control it
export type TReadOnlyEmitter<T extends TEmitterParameter[] = []> = {
- addListener: ( listener: TEmitterListener<T> ) => void;
+ addListener: ( listener: TEmitterListener<T>, options?: DisposerOptions ) => void;
hasListener: ( listener: TEmitterListener<T> ) => boolean;
removeListener: ( listener: TEmitterListener<T> ) => void;
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 1e4b7e8543cc0b83508fb21843122cf27372d94b)
+++ b/axon/js/Disposable.ts (date 1734475294928)
@@ -13,6 +13,7 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -22,7 +23,15 @@
isDisposable?: boolean;
};
-class Disposable {
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer;
+};
+
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -82,11 +91,32 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ public registerDisposer( listener: IntentionalAny, callback: () => void ): void {
+
+ }
+
+ public unregisterDisposer( listener: IntentionalAny ): void {
+
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 24e22cc5f1a7406f090aa41efc65649368d4c647)
+++ b/scenery/js/nodes/Node.ts (date 1734482399428)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2337,7 +2338,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2354,25 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ if ( options?.disposer ) {
+ options?.disposer?.disposeEmitter.addListener( () => {
+
+ if ( assert ) {
+ delete listener.__unsafe_managed_by_disposer;
+ }
+
+ this.removeInputListener( listener );
+ } );
+ if ( assert ) {
+
+ assert && assert( !listener.__unsafe_managed_by_disposer, 'Cannot addInputListener on a listener managed by disposer' );
+
+ listener.__unsafe_managed_by_disposer = true;
+ }
+ }
}
+
return this;
}
@@ -2375,6 +2394,10 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ if ( assert && listener.__unsafe_managed_by_disposer ) {
+ assert && assert( false, 'Cannot removeInputListener on a listener managed by disposer' );
+ }
}
return this;
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 1e4b7e8543cc0b83508fb21843122cf27372d94b)
+++ b/axon/js/ReadOnlyProperty.ts (date 1734474681049)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
@@ -85,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -439,6 +440,9 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ options?.disposer?.disposeEmitter?.addListener( () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,6 +455,8 @@
this.addPhetioStateDependencies( options.phetioDependencies );
}
this.tinyProperty.lazyLink( listener );
+
+ options?.disposer?.disposeEmitter?.addListener( () => this.unlink( listener ) );
}
/** |
|
Auto registration and removal: Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1734534344261)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposer: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +107,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposer: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +129,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposer: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposer: this } );
+ prism.positionProperty.link( prismShapeListener, { disposer: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposer: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposer: this } );
this.updatePrismShape();
}
Index: axon/js/TinyEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts
--- a/axon/js/TinyEmitter.ts (revision 0740e0b0ea3dc1bc3f8223a189cb0b526115ca7a)
+++ b/axon/js/TinyEmitter.ts (date 1734534344252)
@@ -12,6 +12,7 @@
import Pool, { TPoolable } from '../../phet-core/js/Pool.js';
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
// constants
@@ -233,7 +234,7 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
assert && assert( !this.isDisposed, 'Cannot add a listener to a disposed TinyEmitter' );
assert && assert( !this.hasListener( listener ), 'Cannot add the same listener twice' );
@@ -250,6 +251,8 @@
console.log( `Max TinyEmitter listeners: ${maxListenerCount}` );
assert( maxListenerCount <= listenerLimit, `listener count of ${maxListenerCount} above ?listenerLimit=${listenerLimit}` );
}
+
+ options?.disposer?.disposeEmitter.addListener( () => this.removeListener( listener ) );
}
/**
Index: axon/js/TEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TEmitter.ts b/axon/js/TEmitter.ts
--- a/axon/js/TEmitter.ts (revision 0740e0b0ea3dc1bc3f8223a189cb0b526115ca7a)
+++ b/axon/js/TEmitter.ts (date 1734534344242)
@@ -9,6 +9,7 @@
*/
import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
+import { DisposerOptions } from './Disposable.js';
// undefined and never are not allowed as parameters to Emitter
export type TEmitterParameter = Exclude<IntentionalAny, undefined | never>;
@@ -18,7 +19,7 @@
// A "read only" version of an emitter. You can listen to it, but cannot control it
export type TReadOnlyEmitter<T extends TEmitterParameter[] = []> = {
- addListener: ( listener: TEmitterListener<T> ) => void;
+ addListener: ( listener: TEmitterListener<T>, options?: DisposerOptions ) => void;
hasListener: ( listener: TEmitterListener<T> ) => boolean;
removeListener: ( listener: TEmitterListener<T> ) => void;
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 0740e0b0ea3dc1bc3f8223a189cb0b526115ca7a)
+++ b/axon/js/Disposable.ts (date 1734534344214)
@@ -13,6 +13,7 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -22,7 +23,15 @@
isDisposable?: boolean;
};
-class Disposable {
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer;
+};
+
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -82,11 +91,32 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ public registerDisposer( listener: IntentionalAny, callback: () => void ): void {
+
+ }
+
+ public unregisterDisposer( listener: IntentionalAny ): void {
+
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1734535083586)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -168,6 +169,7 @@
import Vector2 from '../../../dot/js/Vector2.js';
import { Shape } from '../../../kite/js/imports.js';
import arrayDifference from '../../../phet-core/js/arrayDifference.js';
+import arrayRemove from '../../../phet-core/js/arrayRemove.js';
import deprecationWarning from '../../../phet-core/js/deprecationWarning.js';
import optionize, { combineOptions, EmptySelfOptions, optionize3 } from '../../../phet-core/js/optionize.js';
import IntentionalAny from '../../../phet-core/js/types/IntentionalAny.js';
@@ -2337,7 +2339,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2355,21 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ if ( options?.disposer ) {
+
+ this.managedDisposals = this.managedDisposals || [];
+
+ const term = () => {
+ arrayRemove( this.managedDisposals, entry );
+ this.removeInputListener( listener );
+ };
+ const entry = { disposer: options.disposer, listener: listener, term: term };
+ this.managedDisposals.push( entry );
+ options.disposer.disposeEmitter.addListener( term );
+ }
}
+
return this;
}
@@ -2375,6 +2391,14 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ if ( this.managedDisposals ) {
+ const found = this.managedDisposals.filter( ( disposal ) => disposal.listener === listener );
+ found.forEach( element => {
+ element.disposer.disposeEmitter.removeListener( element.term );
+ } );
+
+ }
}
return this;
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 0740e0b0ea3dc1bc3f8223a189cb0b526115ca7a)
+++ b/axon/js/ReadOnlyProperty.ts (date 1734534344236)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
@@ -86,7 +87,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +441,9 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ options?.disposer?.disposeEmitter?.addListener( () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -452,6 +456,8 @@
this.addPhetioStateDependencies( options.phetioDependencies );
}
this.tinyProperty.lazyLink( listener );
+
+ options?.disposer?.disposeEmitter?.addListener( () => this.unlink( listener ) );
}
/**
After experimentation with a few strategies, I'm inclined to recommend that we document calling with |
|
Let's try adding that part in Disposable, keeping it sparse and deal with it automatically in |
|
Just noticed that this and a bit below in the constructor seems like a really good candidate for this new feature. |
|
This version is more general, has a clean client-side usage, should not measurably impact CPU or memory where the feature is unused, and I think could be ready for main. I added unit tests which exhibit reasonable behavior. Bending Light remains leak-free with this patch. I believe the TODOs could be moved to a side issue or worked on after committing to main (I'm concerned it will be destabilizing to reverse all disposeEmitters, and would not be necessary to bringing this to main). Still, this is a significant change and I would like to request review support from @zepumph. Feel free to review asynchronously or we can review together over zoom if you prefer. Any other unit tests or order testing we should add? Thanks! Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: axon/js/Emitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Emitter.ts b/axon/js/Emitter.ts
--- a/axon/js/Emitter.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Emitter.ts (date 1734896744832)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
import TinyEmitter, { TinyEmitterOptions } from './TinyEmitter.js';
@@ -77,8 +78,11 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
this.tinyEmitter.addListener( listener );
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'listener', listener, options.disposer, () => this.removeListener( listener ) );
}
/**
@@ -86,6 +90,9 @@
*/
public removeListener( listener: TEmitterListener<T> ): void {
this.tinyEmitter.removeListener( listener );
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( 'listener' ) && this.removeDisposer( 'listener', listener );
}
/**
@@ -93,6 +100,9 @@
*/
public removeAllListeners(): void {
this.tinyEmitter.removeAllListeners();
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( 'listener' ) && this.removeAllDisposers( 'listener' );
}
/**
Index: axon/js/DisposableTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DisposableTests.ts b/axon/js/DisposableTests.ts
--- a/axon/js/DisposableTests.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/DisposableTests.ts (date 1734904720338)
@@ -6,6 +6,7 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import BooleanProperty from './BooleanProperty.js';
import Disposable, { DisposableOptions } from './Disposable.js';
QUnit.module( 'Disposable' );
@@ -59,4 +60,47 @@
assert.throws( () => object3.dispose(), 'should throw if isDisposable is false1' );
assert.throws( () => object4.dispose(), 'should throw if isDisposable is false2' );
}
+} );
+
+QUnit.test( 'Disposable.addDisposer', assert => {
+ assert.ok( true, 'when assertions are not enabled' );
+
+ class Model extends Disposable {
+ public readonly myProperty = new BooleanProperty( false );
+ private readonly myListener: () => void;
+
+ public constructor() {
+ super();
+ this.myListener = () => console.log( 'link called back' );
+ this.myProperty.link( this.myListener, { disposer: this } );
+
+ const b = () => console.log( 'second link' );
+ this.myProperty.link( b );
+ this.myProperty.unlink( b );
+ }
+
+ public detachListener(): void {
+ this.myProperty.unlink( this.myListener );
+ }
+ }
+
+ {
+ const model1 = new Model();
+ assert.ok( model1.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+ model1.detachListener();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ model1.dispose();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+
+ {
+ const model2 = new Model();
+ assert.ok( model2.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+
+ model2.detachListener();
+ assert.ok( !model2.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+
+ model2.dispose();
+ assert.ok( !model2.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
} );
\ No newline at end of file
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1734887889522)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposer: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +107,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposer: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +129,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposer: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposer: this } );
+ prism.positionProperty.link( prismShapeListener, { disposer: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposer: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposer: this } );
this.updatePrismShape();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Disposable.ts (date 1734904884488)
@@ -13,6 +13,8 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import affirm from '../../perennial-alias/js/browser-and-node/affirm.js';
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -22,7 +24,18 @@
isDisposable?: boolean;
};
-class Disposable {
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer;
+};
+
+// Specify possible listener types to avoid typos. Please add more types here as needed.
+type DisposerKey = 'link' | 'listener' | 'inputListener';
+
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -35,6 +48,13 @@
// Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
private _isDisposed = false;
+ // Keep track of disposers that are added to this instance, so that they can be removed when:
+ // (a) the listener is removed via another mechanism
+ // (b) the disposer is disposed
+ // Keep track of each type of listener separately, in case we ever have one container that has multiple types of
+ // listeners, like a Model that has both link() and addListener()
+ private disposerMap?: Map<DisposerKey, Map<IntentionalAny, { disposer: Disposer; disposeCallback: () => void }>>;
+
// Most time, Disposable should only be used by subtypes, only instantiate it if you run into multiple inheritance issues.
public constructor( providedOptions?: DisposableOptions ) {
@@ -82,11 +102,87 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
+
+ this.disposerMap?.forEach( ( _, key ) => this.removeAllDisposers( key ) );
+
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * Adds a listener to the given disposeEmitter, so that when it is called it also calls the callback.
+ * For instance, the callback would be the corresponding removal/unlink/removeInputListener.
+ */
+ public addDisposer( key: DisposerKey, listener: IntentionalAny, disposer: Disposer, removeListener: () => void ): void {
+
+ this.disposerMap = this.disposerMap || new Map();
+
+ if ( !this.disposerMap.has( key ) ) {
+ this.disposerMap.set( key, new Map() );
+ }
+ const entries = this.disposerMap.get( key )!;
+ affirm( !entries.has( listener ), 'Listener already added' );
+
+ const disposeCallback = () => {
+ entries.delete( listener );
+ removeListener();
+ };
+ entries.set( listener, { disposer: disposer, disposeCallback: disposeCallback } );
+ disposer.disposeEmitter.addListener( disposeCallback );
+ }
+
+ /**
+ * Should be guarded with a call to hasDisposers at the call site.
+ * @param listener
+ */
+ public removeDisposer( key: DisposerKey, listener: IntentionalAny ): void {
+ affirm( this.disposerMap, 'removeDisposer should only be called if there are disposers' );
+
+ const disposers = this.disposerMap.get( key );
+ affirm( disposers, 'removeDisposer should only be called if there are disposers' );
+
+ // If there is no entry, that means either:
+ // a) this particular listener was never registered with a disposer, which is fine.
+ // b) or it has already been removed via the disposeEmitter.
+ const entry = disposers.get( listener );
+ if ( entry ) {
+ entry.disposer.disposeEmitter.removeListener( entry.disposeCallback );
+ disposers.delete( listener );
+ }
+
+ // NOTE: Do not delete any of the Maps, as they may be used again in the future.
+ }
+
+ public removeAllDisposers( key: DisposerKey ): void {
+ affirm( this.disposerMap, 'removeAllDisposers should only be called if there are disposers' );
+
+ const disposers = this.disposerMap.get( key );
+ affirm( disposers, 'removeAllDisposers should only be called if there are disposers for this type' );
+
+ disposers.forEach( ( _, listener ) => this.removeDisposer( key, listener ) );
+ }
+
+ /**
+ * To be used as a guard, as a performance optimization during remove listeners.
+ */
+ protected hasDisposers( key: DisposerKey ): boolean {
+ return !!this.disposerMap && !!this.disposerMap.get( key ) && this.disposerMap.get( key )!.size > 0;
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1734896880597)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2337,7 +2338,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2354,11 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'inputListener', listener, options.disposer, () => this.removeInputListener( listener ) );
}
+
return this;
}
@@ -2375,6 +2380,9 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( 'inputListener' ) && this.removeDisposer( 'inputListener', listener );
}
return this;
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/ReadOnlyProperty.ts (date 1734896630637)
@@ -21,12 +21,12 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
import TinyProperty from './TinyProperty.js';
-import type TReadOnlyProperty from './TReadOnlyProperty.js';
-import type { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
+import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
import units, { Units } from './units.js';
import validate from './validate.js';
import Validation, { Validator, ValueComparisonStrategy } from './Validation.js';
@@ -86,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +440,10 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'link', listener, options.disposer, () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,7 +455,10 @@
if ( options && options.phetioDependencies ) {
this.addPhetioStateDependencies( options.phetioDependencies );
}
- this.tinyProperty.lazyLink( listener );
+ this.tinyProperty.lazyLink( listener ); // Note: do not pass through the disposer options
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'link', listener, options.disposer, () => this.unlink( listener ) );
}
/**
@@ -459,12 +466,19 @@
*/
public unlink( listener: PropertyListener<T> ): void {
this.tinyProperty.unlink( listener );
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( 'link' ) && this.removeDisposer( 'link', listener );
}
/**
* Removes all listeners. If no listeners are registered, this is a no-op.
*/
public unlinkAll(): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( 'link' ) && this.removeAllDisposers( 'link' );
+
this.tinyProperty.unlinkAll();
}
|
|
Review:
This isn't the only concern to me. Adding this level of complexity to production code makes this algorithm look quite differently in my brain. It makes changes to disposal code in the future more challenging to keep straight, and the possibility of memory leaks more likely as we maintain Disposable. An assertion maintains a more modest scope for the feature, focusing on how we are actually planning on using the disposer-pattern. I recommend:
I'm also happy to proven wrong, and perhaps sync conversation is what is best to proceed if you have reservations about my comment here. Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1734987724323)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposer: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +107,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposer: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +129,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposer: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposer: this } );
+ prism.positionProperty.link( prismShapeListener, { disposer: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposer: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposer: this } );
this.updatePrismShape();
}
Index: axon/js/DisposableTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DisposableTests.ts b/axon/js/DisposableTests.ts
--- a/axon/js/DisposableTests.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/DisposableTests.ts (date 1734987724295)
@@ -6,6 +6,7 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import BooleanProperty from './BooleanProperty.js';
import Disposable, { DisposableOptions } from './Disposable.js';
QUnit.module( 'Disposable' );
@@ -59,4 +60,47 @@
assert.throws( () => object3.dispose(), 'should throw if isDisposable is false1' );
assert.throws( () => object4.dispose(), 'should throw if isDisposable is false2' );
}
+} );
+
+QUnit.test( 'Disposable.addDisposer', assert => {
+ assert.ok( true, 'when assertions are not enabled' );
+
+ class Model extends Disposable {
+ public readonly myProperty = new BooleanProperty( false );
+ private readonly myListener: () => void;
+
+ public constructor() {
+ super();
+ this.myListener = () => console.log( 'link called back' );
+ this.myProperty.link( this.myListener, { disposer: this } );
+
+ const b = () => console.log( 'second link' );
+ this.myProperty.link( b );
+ this.myProperty.unlink( b );
+ }
+
+ public detachListener(): void {
+ this.myProperty.unlink( this.myListener );
+ }
+ }
+
+ {
+ const model1 = new Model();
+ assert.ok( model1.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+ model1.detachListener();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ model1.dispose();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+
+ {
+ const model2 = new Model();
+ assert.ok( model2.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+
+ model2.detachListener();
+ assert.ok( !model2.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+
+ model2.dispose();
+ assert.ok( !model2.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
} );
\ No newline at end of file
Index: axon/js/Emitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Emitter.ts b/axon/js/Emitter.ts
--- a/axon/js/Emitter.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Emitter.ts (date 1734988397548)
@@ -21,12 +21,15 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
import TinyEmitter, { TinyEmitterOptions } from './TinyEmitter.js';
// By default, Emitters are not stateful
const PHET_IO_STATE_DEFAULT = false;
+const ADD_LISTENER_DISPOSER_KEY = 'listenerDisposer';
+
type SelfOptions = Pick<TinyEmitterOptions, 'reentrantNotificationStrategy'>;
export type EmitterOptions = SelfOptions & StrictOmit<PhetioDataHandlerOptions, 'phetioOuterType'>;
@@ -77,8 +80,11 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
this.tinyEmitter.addListener( listener );
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( ADD_LISTENER_DISPOSER_KEY, listener, options.disposer, () => this.removeListener( listener ) );
}
/**
@@ -86,6 +92,9 @@
*/
public removeListener( listener: TEmitterListener<T> ): void {
this.tinyEmitter.removeListener( listener );
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( ADD_LISTENER_DISPOSER_KEY ) && this.removeDisposer( ADD_LISTENER_DISPOSER_KEY, listener );
}
/**
@@ -93,6 +102,9 @@
*/
public removeAllListeners(): void {
this.tinyEmitter.removeAllListeners();
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( ADD_LISTENER_DISPOSER_KEY ) && this.removeAllDisposers( ADD_LISTENER_DISPOSER_KEY );
}
/**
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Disposable.ts (date 1734988799219)
@@ -13,6 +13,8 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import affirm from '../../perennial-alias/js/browser-and-node/affirm.js';
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -22,7 +24,18 @@
isDisposable?: boolean;
};
-class Disposable {
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer;
+};
+
+// Specify possible listener types to avoid typos. Please add more types here as needed.
+type DisposerKey = 'linkDisposer' | 'listenerDisposer' | 'inputListenerDisposer';
+
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -35,6 +48,14 @@
// Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
private _isDisposed = false;
+ // Keep track of disposers that are added to this instance, so that they can be removed when:
+ // (a) the listener is removed via another mechanism
+ // (b) the disposer is disposed
+ // Keep track of each type of listener separately, in case we ever have one container that has multiple types of
+ // listeners, like a Model that has both link() and addListener().
+ // This is only created lazyily upon first registration.
+ private disposerMap?: Map<DisposerKey, Map<IntentionalAny, { disposer: Disposer; disposeCallback: () => void }>>;
+
// Most time, Disposable should only be used by subtypes, only instantiate it if you run into multiple inheritance issues.
public constructor( providedOptions?: DisposableOptions ) {
@@ -82,11 +103,94 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
+
+ this.disposerMap?.forEach( ( _, key ) => this.removeAllDisposers( key ) );
+
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * Adds a listener to the given disposeEmitter, so that when it is called it also calls the callback.
+ * For instance, the callback would be the corresponding removal/unlink/removeInputListener.
+ */
+ public addDisposer( key: DisposerKey, listener: IntentionalAny, disposer: Disposer, removeListener: () => void ): void {
+ // TODO: I thought we only want to add the entry to the map (and create the map) when assertions are enabled?
+ this.disposerMap = this.disposerMap || new Map();
+
+ if ( !this.disposerMap.has( key ) ) {
+ this.disposerMap.set( key, new Map() );
+ }
+ const entries = this.disposerMap.get( key )!;
+ affirm( !entries.has( listener ), 'Listener already added' );
+
+ const disposeCallback = () => {
+ entries.delete( listener );
+ removeListener();
+ };
+
+ // Keep track of this callback in case this instance of Disposable is disposed before the disposer is (so the
+ // listener can be cleaned up)
+ entries.set( listener, { disposer: disposer, disposeCallback: disposeCallback } );
+
+ disposer.disposeEmitter.addListener( disposeCallback );
+ }
+
+ /**
+ * Should be guarded with a call to hasDisposers at the call site.
+ */
+ public removeDisposer( key: DisposerKey, listener: IntentionalAny ): void {
+
+ affirm( this.disposerMap, 'removeDisposer should only be called if there are disposers' );
+
+ const disposers = this.disposerMap.get( key );
+ affirm( disposers, 'removeDisposer should only be called if there are disposers' );
+
+ // If there is no entry, that means either:
+ // a) this particular listener was never registered with a disposer, which is fine.
+ // b) or it has already been removed via the disposeEmitter.
+ const entry = disposers.get( listener );
+ if ( entry ) {
+ // affirm( entry.disposer.disposeEmitter.hasListener( entry.disposeCallback ) ); ??????
+ entry.disposer.disposeEmitter.removeListener( entry.disposeCallback );
+
+ // TODO: I do not know if this level of cleanup is required. I would prefer an assertion to this much complexity. Let's discuss.
+ disposers.delete( listener );
+ }
+
+ // NOTE: Do not delete any of the Maps, as they may be used again in the future.
+ }
+
+ public removeAllDisposers( key: DisposerKey ): void {
+ affirm( this.disposerMap, 'removeAllDisposers should only be called if there are disposers' );
+
+ const disposers = this.disposerMap.get( key );
+ affirm( disposers, 'removeAllDisposers should only be called if there are disposers for this type' );
+
+ disposers.forEach( ( _, listener ) => this.removeDisposer( key, listener ) );
+ }
+
+ /**
+ * To be used as a guard, as a performance optimization during remove listeners.
+ */
+ protected hasDisposers( key: DisposerKey ): boolean {
+ return !!this.disposerMap && !!this.disposerMap.get( key ) && this.disposerMap.get( key )!.size > 0;
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/ReadOnlyProperty.ts (date 1734988397517)
@@ -21,12 +21,12 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
import TinyProperty from './TinyProperty.js';
-import type TReadOnlyProperty from './TReadOnlyProperty.js';
-import type { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
+import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
import units, { Units } from './units.js';
import validate from './validate.js';
import Validation, { Validator, ValueComparisonStrategy } from './Validation.js';
@@ -49,6 +49,8 @@
units: string | null;
};
+const LINK_DISPOSER_KEY = 'linkDisposer';
+
// Options defined by Property
type SelfOptions = {
@@ -86,7 +88,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +442,10 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( LINK_DISPOSER_KEY, listener, options.disposer, () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,7 +457,10 @@
if ( options && options.phetioDependencies ) {
this.addPhetioStateDependencies( options.phetioDependencies );
}
- this.tinyProperty.lazyLink( listener );
+ this.tinyProperty.lazyLink( listener ); // Note: do not pass through the disposer options
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( LINK_DISPOSER_KEY, listener, options.disposer, () => this.unlink( listener ) );
}
/**
@@ -459,12 +468,19 @@
*/
public unlink( listener: PropertyListener<T> ): void {
this.tinyProperty.unlink( listener );
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( LINK_DISPOSER_KEY ) && this.removeDisposer( LINK_DISPOSER_KEY, listener );
}
/**
* Removes all listeners. If no listeners are registered, this is a no-op.
*/
public unlinkAll(): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( LINK_DISPOSER_KEY ) && this.removeAllDisposers( LINK_DISPOSER_KEY );
+
this.tinyProperty.unlinkAll();
}
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1734988397510)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -196,6 +197,8 @@
// Store the number of children from the single Node instance that has the most children in the whole runtime.
let maxChildCount = 0;
+const INPUT_LISTENER_DISPOSER_KEY = 'inputListenerDisposer';
+
export const REQUIRES_BOUNDS_OPTION_KEYS = [
'leftTop', // {Vector2} - The upper-left corner of this Node's bounds, see setLeftTop() for more documentation
'centerTop', // {Vector2} - The top-center of this Node's bounds, see setCenterTop() for more documentation
@@ -2337,7 +2340,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2356,11 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( INPUT_LISTENER_DISPOSER_KEY, listener, options.disposer, () => this.removeInputListener( listener ) );
}
+
return this;
}
@@ -2375,6 +2382,9 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ this.hasDisposers( INPUT_LISTENER_DISPOSER_KEY ) && this.removeDisposer( INPUT_LISTENER_DISPOSER_KEY, listener );
}
return this; |
|
I believe Patch that:
UPDATE: I fuzzed bending light "prisms" screen and confirmed it is leak-free with assertions enabled. Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: axon/js/Emitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Emitter.ts b/axon/js/Emitter.ts
--- a/axon/js/Emitter.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Emitter.ts (date 1734992230539)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
import TinyEmitter, { TinyEmitterOptions } from './TinyEmitter.js';
@@ -77,14 +78,21 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
this.tinyEmitter.addListener( listener );
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.removeListener( listener ) );
}
/**
* Removes a listener
*/
public removeListener( listener: TEmitterListener<T> ): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ assert && this.assertNotManagedByDisposer( listener );
+
this.tinyEmitter.removeListener( listener );
}
@@ -92,6 +100,10 @@
* Removes all the listeners
*/
public removeAllListeners(): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ assert && this.tinyEmitter.forEachListener( listener => this.assertNotManagedByDisposer( listener ) );
+
this.tinyEmitter.removeAllListeners();
}
Index: axon/js/DisposableTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DisposableTests.ts b/axon/js/DisposableTests.ts
--- a/axon/js/DisposableTests.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/DisposableTests.ts (date 1734989099101)
@@ -6,6 +6,7 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import BooleanProperty from './BooleanProperty.js';
import Disposable, { DisposableOptions } from './Disposable.js';
QUnit.module( 'Disposable' );
@@ -59,4 +60,47 @@
assert.throws( () => object3.dispose(), 'should throw if isDisposable is false1' );
assert.throws( () => object4.dispose(), 'should throw if isDisposable is false2' );
}
+} );
+
+QUnit.test( 'Disposable.addDisposer', assert => {
+ assert.ok( true, 'when assertions are not enabled' );
+
+ class Model extends Disposable {
+ public readonly myProperty = new BooleanProperty( false );
+ private readonly myListener: () => void;
+
+ public constructor() {
+ super();
+ this.myListener = () => console.log( 'link called back' );
+ this.myProperty.link( this.myListener, { disposer: this } );
+
+ const b = () => console.log( 'second link' );
+ this.myProperty.link( b );
+ this.myProperty.unlink( b );
+ }
+
+ public detachListener(): void {
+ this.myProperty.unlink( this.myListener );
+ }
+ }
+
+ {
+ const model1 = new Model();
+ assert.ok( model1.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+ model1.detachListener();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ model1.dispose();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+
+ {
+ const model2 = new Model();
+ assert.ok( model2.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+
+ model2.detachListener();
+ assert.ok( !model2.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+
+ model2.dispose();
+ assert.ok( !model2.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
} );
\ No newline at end of file
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1734989099136)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposer: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +107,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposer: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +129,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposer: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposer: this } );
+ prism.positionProperty.link( prismShapeListener, { disposer: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposer: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposer: this } );
this.updatePrismShape();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Disposable.ts (date 1734992230545)
@@ -13,6 +13,8 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import affirm from '../../perennial-alias/js/browser-and-node/affirm.js';
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -22,7 +24,15 @@
isDisposable?: boolean;
};
-class Disposable {
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer;
+};
+
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -35,6 +45,14 @@
// Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
private _isDisposed = false;
+ // For assertions only, keep track of disposers that are added to this instance, so we can identify a potential error
+ // case where a listener was registered for a disposer, but removed by another means.
+ // Even for assertions, this is only created lazily upon first registration.
+ // Note: We keep this simple and do not differentiate between the different types of listeners. This assumes clients
+ // will not pass the same listener to two different types like: model.link(myListener,{disposer}), model.addListener(myListener,{disposer}) // will throw assertion error.
+ // However, registering one listener like so model.link(myListener,{disposer}) then removing like so model.removeListener(myListener) will not be handled properly.
+ private managedListeners?: Set<IntentionalAny>;
+
// Most time, Disposable should only be used by subtypes, only instantiate it if you run into multiple inheritance issues.
public constructor( providedOptions?: DisposableOptions ) {
@@ -82,11 +100,60 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
+
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * Adds a listener to the given disposeEmitter, so that when it is called it also calls the callback.
+ * For instance, the callback would be the corresponding removal/unlink/removeInputListener.
+ */
+ public addDisposer( listener: IntentionalAny, disposer: Disposer, removeListener: () => void ): void {
+ if ( assert ) {
+ this.managedListeners = this.managedListeners || new Set();
+ affirm( !this.managedListeners.has( listener ), 'Listener already added' );
+ this.managedListeners.add( listener );
+
+ // Bundle together so we can be certain the order is correct. NOTE: we are hoping to reverse the dispatch order for teardown.
+ const myRemoveListener = () => {
+
+ // Remove from the managed list so that removal doesn't indicate an error.
+ this.managedListeners!.delete( listener );
+ removeListener();
+ };
+ disposer.disposeEmitter.addListener( myRemoveListener );
+ }
+ else {
+ disposer.disposeEmitter.addListener( removeListener );
+ }
+ }
+
+ /**
+ * A listener registered for removal via a disposer should not be removed by another mechanism, which could indicate
+ * a programming error.
+ */
+ public assertNotManagedByDisposer( listener: IntentionalAny ): void {
+
+ if ( assert && this.managedListeners ) {
+ assert( !this.managedListeners.has( listener ), 'The listener was registered for removal by a disposer, but was removed by some other mechanism. This may indicate a bug in the code.' );
+ }
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1734992308037)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2337,7 +2338,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2354,11 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.removeInputListener( listener ) );
}
+
return this;
}
@@ -2366,6 +2371,8 @@
// ensure the listener is in our list (ignore assertion for disposal, see https://github.com/phetsims/sun/issues/394)
assert && assert( this.isDisposed || index >= 0, 'Could not find input listener to remove' );
if ( index >= 0 ) {
+ assert && this.assertNotManagedByDisposer( listener );
+
this._inputListeners.splice( index, 1 );
this._picker.onRemoveInputListener();
if ( assertSlow ) { this._picker.audit(); }
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/ReadOnlyProperty.ts (date 1734992308000)
@@ -21,12 +21,12 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
import TinyProperty from './TinyProperty.js';
-import type TReadOnlyProperty from './TReadOnlyProperty.js';
-import type { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
+import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
import units, { Units } from './units.js';
import validate from './validate.js';
import Validation, { Validator, ValueComparisonStrategy } from './Validation.js';
@@ -86,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +440,10 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,13 +455,18 @@
if ( options && options.phetioDependencies ) {
this.addPhetioStateDependencies( options.phetioDependencies );
}
- this.tinyProperty.lazyLink( listener );
+ this.tinyProperty.lazyLink( listener ); // Note: do not pass through the disposer options
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.unlink( listener ) );
}
/**
* Removes a listener. If listener is not registered, this is a no-op.
*/
public unlink( listener: PropertyListener<T> ): void {
+ assert && this.assertNotManagedByDisposer( listener );
+
this.tinyProperty.unlink( listener );
}
@@ -465,6 +474,8 @@
* Removes all listeners. If no listeners are registered, this is a no-op.
*/
public unlinkAll(): void {
+ assert && this.tinyProperty.forEachListener( listener => this.assertNotManagedByDisposer( listener ) );
+
this.tinyProperty.unlinkAll();
}
|
|
Back to me to double check on the unit tests before requesting review. |
|
This patch additionally corrects the unit tests to check if a listener registered with @zepumph can you please review and advise? Subject: [PATCH] Add canvasBounds, see https://github.com/phetsims/bending-light/issues/410
---
Index: axon/js/Emitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Emitter.ts b/axon/js/Emitter.ts
--- a/axon/js/Emitter.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Emitter.ts (date 1735526141758)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
import TinyEmitter, { TinyEmitterOptions } from './TinyEmitter.js';
@@ -77,14 +78,21 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
this.tinyEmitter.addListener( listener );
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.removeListener( listener ) );
}
/**
* Removes a listener
*/
public removeListener( listener: TEmitterListener<T> ): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ assert && this.assertNotManagedByDisposer( listener );
+
this.tinyEmitter.removeListener( listener );
}
@@ -92,6 +100,10 @@
* Removes all the listeners
*/
public removeAllListeners(): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ assert && this.tinyEmitter.forEachListener( listener => this.assertNotManagedByDisposer( listener ) );
+
this.tinyEmitter.removeAllListeners();
}
Index: axon/js/DisposableTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DisposableTests.ts b/axon/js/DisposableTests.ts
--- a/axon/js/DisposableTests.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/DisposableTests.ts (date 1735526452704)
@@ -6,6 +6,8 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import { isAffirmEnabled } from '../../perennial-alias/js/browser-and-node/affirm.js';
+import BooleanProperty from './BooleanProperty.js';
import Disposable, { DisposableOptions } from './Disposable.js';
QUnit.module( 'Disposable' );
@@ -59,4 +61,43 @@
assert.throws( () => object3.dispose(), 'should throw if isDisposable is false1' );
assert.throws( () => object4.dispose(), 'should throw if isDisposable is false2' );
}
+} );
+
+QUnit.test( 'Disposable.addDisposer', assert => {
+ assert.ok( true, 'when assertions are not enabled' );
+
+ class Model extends Disposable {
+ public readonly myProperty = new BooleanProperty( false );
+ private readonly myListener: () => void;
+
+ public constructor() {
+ super();
+ this.myListener = () => console.log( 'link called back' );
+ this.myProperty.link( this.myListener, { disposer: this } );
+
+ const b = () => console.log( 'second link' );
+ this.myProperty.link( b );
+ this.myProperty.unlink( b );
+ }
+
+ public detachListener(): void {
+ this.myProperty.unlink( this.myListener );
+ }
+ }
+
+ {
+ const model1 = new Model();
+ assert.ok( model1.myProperty.hasListeners(), 'after creating the model, it should have a listener' );
+ model1.dispose();
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+
+ {
+ const model2 = new Model();
+ assert.ok( model2.myProperty.hasListeners(), 'after creating the model, it should have a listener' );
+
+ if ( isAffirmEnabled() ) {
+ assert.throws( () => model2.detachListener(), 'detect that the listener has been removed another way' );
+ }
+ }
} );
\ No newline at end of file
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1735526141770)
@@ -53,10 +53,7 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
-
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +82,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, { disposer: this } );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +107,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, { disposer: this } );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +129,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, { disposer: this } );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, { disposer: this } );
+ prism.positionProperty.link( prismShapeListener, { disposer: this } );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, { disposer: this } );
+ prismsModel.prismMediumProperty.link( prismColorListener, { disposer: this } );
this.updatePrismShape();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/Disposable.ts (date 1735526141745)
@@ -13,6 +13,8 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import affirm from '../../perennial-alias/js/browser-and-node/affirm.js';
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
@@ -22,7 +24,15 @@
isDisposable?: boolean;
};
-class Disposable {
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer;
+};
+
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -35,6 +45,14 @@
// Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
private _isDisposed = false;
+ // For assertions only, keep track of disposers that are added to this instance, so we can identify a potential error
+ // case where a listener was registered for a disposer, but removed by another means.
+ // Even for assertions, this is only created lazily upon first registration.
+ // Note: We keep this simple and do not differentiate between the different types of listeners. This assumes clients
+ // will not pass the same listener to two different types like: model.link(myListener,{disposer}), model.addListener(myListener,{disposer}) // will throw assertion error.
+ // However, registering one listener like so model.link(myListener,{disposer}) then removing like so model.removeListener(myListener) will not be handled properly.
+ private managedListeners?: Set<IntentionalAny>;
+
// Most time, Disposable should only be used by subtypes, only instantiate it if you run into multiple inheritance issues.
public constructor( providedOptions?: DisposableOptions ) {
@@ -82,11 +100,60 @@
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that teardowns happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
+
+ // TODO: So be mindful of teardown order particularly in common code and subtyping (and sim code extending common code), see https://github.com/phetsims/axon/issues/455
+ // TODO: In particular when mixing different patterns for dispose, like this.disposeMyClass(), override dispose(), and disposeEmitter., see https://github.com/phetsims/axon/issues/455
+
this._disposeEmitter.dispose();
+
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * Adds a listener to the given disposeEmitter, so that when it is called it also calls the callback.
+ * For instance, the callback would be the corresponding removal/unlink/removeInputListener.
+ */
+ public addDisposer( listener: IntentionalAny, disposer: Disposer, removeListener: () => void ): void {
+ if ( assert ) {
+ this.managedListeners = this.managedListeners || new Set();
+ affirm( !this.managedListeners.has( listener ), 'Listener already added' );
+ this.managedListeners.add( listener );
+
+ // Bundle together so we can be certain the order is correct. NOTE: we are hoping to reverse the dispatch order for teardown.
+ const myRemoveListener = () => {
+
+ // Remove from the managed list so that removal doesn't indicate an error.
+ this.managedListeners!.delete( listener );
+ removeListener();
+ };
+ disposer.disposeEmitter.addListener( myRemoveListener );
+ }
+ else {
+ disposer.disposeEmitter.addListener( removeListener );
+ }
+ }
+
+ /**
+ * A listener registered for removal via a disposer should not be removed by another mechanism, which could indicate
+ * a programming error.
+ */
+ public assertNotManagedByDisposer( listener: IntentionalAny ): void {
+
+ if ( assert && this.managedListeners ) {
+ assert( !this.managedListeners.has( listener ), 'The listener was registered for removal by a disposer, but was removed by some other mechanism. This may indicate a bug in the code.' );
+ }
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1735526141789)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2337,7 +2338,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2354,11 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.removeInputListener( listener ) );
}
+
return this;
}
@@ -2366,6 +2371,8 @@
// ensure the listener is in our list (ignore assertion for disposal, see https://github.com/phetsims/sun/issues/394)
assert && assert( this.isDisposed || index >= 0, 'Could not find input listener to remove' );
if ( index >= 0 ) {
+ assert && this.assertNotManagedByDisposer( listener );
+
this._inputListeners.splice( index, 1 );
this._picker.onRemoveInputListener();
if ( assertSlow ) { this._picker.audit(); }
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision 9b3f7a0ac0118de1ddefba659698b83e274f09c8)
+++ b/axon/js/ReadOnlyProperty.ts (date 1735526141765)
@@ -21,12 +21,12 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
import TinyProperty from './TinyProperty.js';
-import type TReadOnlyProperty from './TReadOnlyProperty.js';
-import type { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
+import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
import units, { Units } from './units.js';
import validate from './validate.js';
import Validation, { Validator, ValueComparisonStrategy } from './Validation.js';
@@ -86,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +440,10 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,13 +455,18 @@
if ( options && options.phetioDependencies ) {
this.addPhetioStateDependencies( options.phetioDependencies );
}
- this.tinyProperty.lazyLink( listener );
+ this.tinyProperty.lazyLink( listener ); // Note: do not pass through the disposer options
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.unlink( listener ) );
}
/**
* Removes a listener. If listener is not registered, this is a no-op.
*/
public unlink( listener: PropertyListener<T> ): void {
+ assert && this.assertNotManagedByDisposer( listener );
+
this.tinyProperty.unlink( listener );
}
@@ -465,6 +474,8 @@
* Removes all listeners. If no listeners are registered, this is a no-op.
*/
public unlinkAll(): void {
+ assert && this.tinyProperty.forEachListener( listener => this.assertNotManagedByDisposer( listener ) );
+
this.tinyProperty.unlinkAll();
}
|
|
Subject: [PATCH] assertions, including if shapeProperty.dispose is called before the node, https://github.com/phetsims/axon/issues/455
---
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1735935612447)
@@ -53,10 +53,9 @@
if ( prism.shapeProperty.get().getReferencePoint() ) {
this.addChild( this.knobNode );
}
+ this.addDisposable( this.knobNode );
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ const disposerOptions = { disposer: this };
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +84,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, disposerOptions );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +109,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
// When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ dragBoundsProperty.lazyLink( keepInBounds, disposerOptions );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +131,21 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, disposerOptions );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, disposerOptions );
+ prism.positionProperty.link( prismShapeListener, disposerOptions );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
+ /////////////////////////////////////
+ // should error because PrismNode has not disposed yet.
+ prism.shapeProperty.dispose();
+ /////////////////////////////////////
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, disposerOptions );
+ prismsModel.prismMediumProperty.link( prismColorListener, disposerOptions );
this.updatePrismShape();
}
Index: axon/js/DisposableTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DisposableTests.ts b/axon/js/DisposableTests.ts
--- a/axon/js/DisposableTests.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/DisposableTests.ts (date 1735935612456)
@@ -6,6 +6,8 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import { isAffirmEnabled } from '../../perennial-alias/js/browser-and-node/affirm.js';
+import BooleanProperty from './BooleanProperty.js';
import Disposable, { DisposableOptions } from './Disposable.js';
QUnit.module( 'Disposable' );
@@ -59,4 +61,58 @@
assert.throws( () => object3.dispose(), 'should throw if isDisposable is false1' );
assert.throws( () => object4.dispose(), 'should throw if isDisposable is false2' );
}
+} );
+
+QUnit.test( 'Disposable.addDisposer', assert => {
+ assert.ok( true, 'when assertions are not enabled' );
+
+ class Model extends Disposable {
+ public readonly myProperty = new BooleanProperty( false );
+ private readonly myListener: () => void;
+
+ public constructor() {
+ super();
+ this.myListener = () => console.log( 'link called back' );
+ this.myProperty.link( this.myListener, { disposer: this } );
+
+ const b = () => console.log( 'second link' );
+ this.myProperty.link( b );
+ this.myProperty.unlink( b );
+ }
+
+ public detachListener(): void {
+ this.myProperty.unlink( this.myListener );
+ }
+ }
+
+ {
+ const model1 = new Model();
+ assert.ok( model1.myProperty.hasListeners(), 'after creating the model, it should have a listener' );
+ // add another listener
+ model1.dispose();
+ // assert there is one listener.
+ // assert myProperty is not disposed.
+ assert.ok( !model1.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+
+ {
+ const model2 = new Model();
+ assert.ok( model2.myProperty.hasListeners(), 'after creating the model, it should have a listener2' );
+
+ if ( isAffirmEnabled() ) {
+ assert.throws( () => model2.detachListener(), 'detect that the listener has been removed another way' );
+ }
+ }
+
+ {
+ const model3 = new Model();
+ assert.ok( model3.myProperty.hasListeners(), 'after creating the model, it should have a listener3' );
+
+ if ( isAffirmEnabled() ) {
+ assert.throws( () => model3.myProperty.dispose(),
+ 'The model is in charge of unlinking, but it is done by Property dispose instead' );
+
+ //
+ }
+ }
} );
\ No newline at end of file
Index: axon/js/Emitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Emitter.ts b/axon/js/Emitter.ts
--- a/axon/js/Emitter.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/Emitter.ts (date 1735927513407)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
import TinyEmitter, { TinyEmitterOptions } from './TinyEmitter.js';
@@ -77,14 +78,21 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
this.tinyEmitter.addListener( listener );
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.removeListener( listener ) );
}
/**
* Removes a listener
*/
public removeListener( listener: TEmitterListener<T> ): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ assert && this.assertNotManagedByDisposer( listener );
+
this.tinyEmitter.removeListener( listener );
}
@@ -92,6 +100,10 @@
* Removes all the listeners
*/
public removeAllListeners(): void {
+
+ // undo addDisposer (see above). Guard on hasDisposers to avoid unnecessary work
+ assert && this.tinyEmitter.forEachListener( listener => this.assertNotManagedByDisposer( listener ) );
+
this.tinyEmitter.removeAllListeners();
}
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/Disposable.ts (date 1735934477370)
@@ -13,16 +13,31 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import affirm, { isAffirmEnabled } from '../../perennial-alias/js/browser-and-node/affirm.js';
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
+// A "disposer" is a Type for another entity that can be responsible for disposing this instance. This type is much better
+// than directly linking this to Disposable the class.
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+
+ // When provided, a disposer is solely responsible for the disposal of something, and an assertion will fire if
+ // disposed in another way.
+ disposer?: Disposer;
+};
+
// Used in subclasses to support mutate.
export type DisposableOptions = {
isDisposable?: boolean;
};
-class Disposable {
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -35,6 +50,21 @@
// Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
private _isDisposed = false;
+ // For assertions only, keep track of disposers we wire up "unregister" actions to when the disposer disposes.
+ // This is so we can identify a potential error case where a disposer target was wired up to a disposer, but fired
+ // by another means. See addDisposer().
+ // Even for assertions, this is only created lazily upon first registration.
+ // Note: We keep this simple and do not differentiate between the different types of targets. This assumes clients
+ // will not pass the same listener (target) to two different types like:
+ // `model.link(myListener,{disposer}), model.addListener(myListener,{disposer})`
+ // as this will throw assertion error.
+ //
+ // However, registering one listener and then "removing" it like:
+ // `model.link(myListener,{disposer})`
+ // `model.removeListener(myListener)`
+ // will not be handled properly, and will result in an assertion that is not necessarily a logical error. Be Careful!
+ private managedDisposerUnregistrations?: Set<IntentionalAny>;
+
// Most time, Disposable should only be used by subtypes, only instantiate it if you run into multiple inheritance issues.
public constructor( providedOptions?: DisposableOptions ) {
@@ -79,14 +109,64 @@
}
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed, via the disposeEmitter.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
public dispose(): void {
assert && !this._isDisposable && Disposable.assertNotDisposable();
assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // If you hit this assertion because of disposal order or other unvaluable reasons, then please talk to SR
+ // and MK about removing it. See https://github.com/phetsims/axon/issues/455
+ assert && this.managedDisposerUnregistrations && assert( this.managedDisposerUnregistrations.size === 0,
+ 'Disposing an instance with registered disposer unregistrations is not allowed. Another class was expecting to handle disposal steps' );
+
this._disposeEmitter.emit();
this._disposeEmitter.dispose();
this._isDisposed = true;
}
+ /**
+ * Adds a target unregistration to the given disposeEmitter, so that when it is called it also calls the callback.
+ * For instance, the callback would be the corresponding removal/unlink/removeInputListener. Subtypes of Disposable
+ * can use this function with DisposerOptions as a convenience to more easily handle disposal/teardown.
+ */
+ protected addDisposer( target: IntentionalAny, disposer: Disposer, unregister: () => void ): void {
+ if ( assert ) {
+ this.managedDisposerUnregistrations = this.managedDisposerUnregistrations || new Set();
+ assert && assert( !this.managedDisposerUnregistrations.has( target ), 'Target already registered by this Disposer.' );
+ this.managedDisposerUnregistrations.add( target );
+ }
+
+ disposer.disposeEmitter.addListener( () => {
+
+ // Remove from the managed list first so that unregistration below doesn't indicate an assertion.
+ if ( isAffirmEnabled() ) {
+ affirm( this.managedDisposerUnregistrations );
+ this.managedDisposerUnregistrations.delete( target );
+ }
+
+ unregister();
+ } );
+ }
+
+ /**
+ * A target registered for removal via a disposer should not be removed by another mechanism, which could indicate
+ * a programming error. Subtypes that decide to call addDisposer should also use this assertion in their own
+ * implementation of unregistration methods.
+ */
+ protected assertNotManagedByDisposer( target: IntentionalAny ): void {
+
+ // If you hit this assertion because of disposal order or other unvaluable reasons, then please talk to SR
+ // and MK about removing it. See https://github.com/phetsims/axon/issues/455
+ assert && this.managedDisposerUnregistrations && assert( !this.managedDisposerUnregistrations.has( target ),
+ 'The target was registered for removal by a disposer, but was removed by some other mechanism. This may indicate a bug in the code.' );
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/ReadOnlyProperty.ts (date 1735930234053)
@@ -21,12 +21,12 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
import TinyProperty from './TinyProperty.js';
-import type TReadOnlyProperty from './TReadOnlyProperty.js';
-import type { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
+import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
import units, { Units } from './units.js';
import validate from './validate.js';
import Validation, { Validator, ValueComparisonStrategy } from './Validation.js';
@@ -86,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +440,10 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,13 +455,18 @@
if ( options && options.phetioDependencies ) {
this.addPhetioStateDependencies( options.phetioDependencies );
}
- this.tinyProperty.lazyLink( listener );
+ this.tinyProperty.lazyLink( listener ); // Note: do not pass through the disposer options
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.unlink( listener ) );
}
/**
* Removes a listener. If listener is not registered, this is a no-op.
*/
public unlink( listener: PropertyListener<T> ): void {
+ assert && this.assertNotManagedByDisposer( listener );
+
this.tinyProperty.unlink( listener );
}
@@ -465,6 +474,8 @@
* Removes all listeners. If no listeners are registered, this is a no-op.
*/
public unlinkAll(): void {
+ assert && this.tinyProperty.forEachListener( listener => this.assertNotManagedByDisposer( listener ) );
+
this.tinyProperty.unlinkAll();
}
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1735927513448)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2337,7 +2338,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2354,11 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( listener, options.disposer, () => this.removeInputListener( listener ) );
}
+
return this;
}
@@ -2366,6 +2371,8 @@
// ensure the listener is in our list (ignore assertion for disposal, see https://github.com/phetsims/sun/issues/394)
assert && assert( this.isDisposed || index >= 0, 'Could not find input listener to remove' );
if ( index >= 0 ) {
+ assert && this.assertNotManagedByDisposer( listener );
+
this._inputListeners.splice( index, 1 );
this._picker.onRemoveInputListener();
if ( assertSlow ) { this._picker.audit(); } |
|
@samreid and I agreed we want to instead bring the more thorough one to production from
Subject: [PATCH] fdsaf
---
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts (revision d34c4ff71c449c754799fe93ef1bdea3fb8485c3)
+++ b/bending-light/js/prisms/view/PrismNode.ts (date 1735941955805)
@@ -54,9 +54,9 @@
this.addChild( this.knobNode );
}
- this.disposeEmitter.addListener( () => {
- this.knobNode.dispose();
- } );
+ this.addDisposable( this.knobNode );
+
+ const disposerOptions = { disposer: this };
// Prism rotation with knob
if ( !isIcon ) {
@@ -85,13 +85,10 @@
// A Prism cannot be put back into the toolbox by rotating it.
end: _.noop
} );
- this.knobNode.addInputListener( knobDragListener );
+ this.knobNode.addInputListener( knobDragListener, disposerOptions );
this.knobNode.touchArea = Shape.circle( 0, 10, 40 );
- this.disposeEmitter.addListener( () => {
- this.knobNode.removeInputListener( knobDragListener );
- knobDragListener.dispose();
- } );
+ this.addDisposable( knobDragListener );
}
this.prismPathNode = new Path( modelViewTransform.modelToViewShape( prism.getTranslatedShape().shape ), {
@@ -113,15 +110,10 @@
const prismShapeListener = () => this.updatePrismShape();
const prismColorListener = () => this.updatePrismColor();
- prism.disposeEmitter.addListener( () => {
- this.dispose();
- } );
+ prism.addDisposable( this );
- // When the window reshapes, make sure no prism is left outside of the play area
- dragBoundsProperty.lazyLink( keepInBounds );
- this.disposeEmitter.addListener( () => {
- dragBoundsProperty.unlink( keepInBounds );
- } );
+ // When the window reshapes, make sure no prism is left outside the play area
+ dragBoundsProperty.lazyLink( keepInBounds, disposerOptions );
if ( !isIcon ) {
this.dragListener = new DragListener( {
@@ -140,27 +132,15 @@
}
} );
- this.prismPathNode.addInputListener( this.dragListener );
- this.disposeEmitter.addListener( () => {
- this.prismPathNode.removeInputListener( this.dragListener! );
- this.dragListener!.dispose();
- } );
+ this.prismPathNode.addInputListener( this.dragListener, disposerOptions );
+ this.addDisposable( this.dragListener );
}
- prism.shapeProperty.link( prismShapeListener );
- prism.positionProperty.link( prismShapeListener );
+ prism.shapeProperty.link( prismShapeListener, disposerOptions );
+ prism.positionProperty.link( prismShapeListener, disposerOptions );
- this.disposeEmitter.addListener( () => {
- this.prism.shapeProperty.unlink( prismShapeListener );
- this.prism.positionProperty.unlink( prismShapeListener );
- } );
-
- prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener );
- prismsModel.prismMediumProperty.link( prismColorListener );
- this.disposeEmitter.addListener( () => {
- prismsModel.mediumColorFactory.lightTypeProperty.unlink( prismColorListener );
- prismsModel.prismMediumProperty.unlink( prismColorListener );
- } );
+ prismsModel.mediumColorFactory.lightTypeProperty.link( prismColorListener, disposerOptions );
+ prismsModel.prismMediumProperty.link( prismColorListener, disposerOptions );
this.updatePrismShape();
}
Index: axon/js/DisposableTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DisposableTests.ts b/axon/js/DisposableTests.ts
--- a/axon/js/DisposableTests.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/DisposableTests.ts (date 1735942764215)
@@ -6,6 +6,7 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import BooleanProperty from './BooleanProperty.js';
import Disposable, { DisposableOptions } from './Disposable.js';
QUnit.module( 'Disposable' );
@@ -59,4 +60,58 @@
assert.throws( () => object3.dispose(), 'should throw if isDisposable is false1' );
assert.throws( () => object4.dispose(), 'should throw if isDisposable is false2' );
}
+} );
+
+QUnit.test( 'Disposable.addDisposer', assert => {
+ assert.ok( true, 'when assertions are not enabled' );
+
+ class Model extends Disposable {
+ public readonly myProperty = new BooleanProperty( false );
+ private readonly myListener: () => void;
+
+ public constructor() {
+ super();
+ this.myListener = () => console.log( 'link called back' );
+ this.myProperty.link( this.myListener, { disposer: this } );
+
+ const b = () => console.log( 'second link' );
+ this.myProperty.link( b );
+ this.myProperty.unlink( b );
+ }
+
+ public detachListener(): void {
+ this.myProperty.unlink( this.myListener );
+ }
+ }
+
+ {
+ const model = new Model();
+
+ // @ts-expect-error disposeEmitter is a TEmitter.
+ const listenerCount = model.disposeEmitter[ 'listeners' ];
+ assert.ok( listenerCount.size === 1, 'disposer wired up on construction' );
+ assert.ok( model.myProperty.hasListeners(), 'after creating the model, it should have a listerer' );
+ model.detachListener();
+ assert.ok( !model.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+ {
+ const model = new Model();
+ model.dispose();
+ assert.ok( !model.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ }
+ {
+ const model = new Model();
+
+ // @ts-expect-error disposeEmitter is a TEmitter.
+ const listenersSet = model.disposeEmitter[ 'listeners' ];
+ const listenerCount = listenersSet.size;
+ model.myProperty.dispose();
+ assert.ok( !model.myProperty.hasListeners(), 'after disposing the model, it should not have a listener' );
+ if ( window.assert ) {
+ for ( const [ key, value ] of model.myProperty[ 'disposerMap' ]!.entries() ) {
+ assert.ok( value.size === 0, `after disposing the Property, it should not have any lingering disposers: ${key}` );
+ }
+ }
+ assert.ok( listenersSet.size === listenerCount - 1, 'disposing Property should remove the listener on model.disposeEmitter' );
+ }
} );
\ No newline at end of file
Index: axon/js/Emitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Emitter.ts b/axon/js/Emitter.ts
--- a/axon/js/Emitter.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/Emitter.ts (date 1735940459290)
@@ -21,6 +21,7 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js';
import TinyEmitter, { TinyEmitterOptions } from './TinyEmitter.js';
@@ -77,8 +78,11 @@
/**
* Adds a listener which will be called during emit.
*/
- public addListener( listener: TEmitterListener<T> ): void {
+ public addListener( listener: TEmitterListener<T>, options?: DisposerOptions ): void {
this.tinyEmitter.addListener( listener );
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'listener', listener, options.disposer, () => this.removeListener( listener ) );
}
/**
@@ -86,6 +90,9 @@
*/
public removeListener( listener: TEmitterListener<T> ): void {
this.tinyEmitter.removeListener( listener );
+
+ // undo addDisposer (see above)
+ this.removeDisposerRegistration( 'listener', listener );
}
/**
@@ -93,6 +100,9 @@
*/
public removeAllListeners(): void {
this.tinyEmitter.removeAllListeners();
+
+ // undo addDisposer (see above)
+ this.removeAllDisposers( 'listener' );
}
/**
Index: axon/js/Disposable.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Disposable.ts b/axon/js/Disposable.ts
--- a/axon/js/Disposable.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/Disposable.ts (date 1735941847270)
@@ -13,16 +13,31 @@
* @author Michael Kauzmann (PhET Interactive Simulations)
*/
+import affirm, { isAffirmEnabled } from '../../perennial-alias/js/browser-and-node/affirm.js';
+import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js';
import axon from './axon.js';
import TEmitter, { TReadOnlyEmitter } from './TEmitter.js';
import TinyEmitter from './TinyEmitter.js';
+// A "disposer" is a Type for another entity that can be responsible for disposing this instance. This type is much better
+// than directly linking this to the Disposable class.
+type Disposer = {
+ disposeEmitter: TReadOnlyEmitter;
+};
+
+export type DisposerOptions = {
+ disposer?: Disposer; // if provided, an "unregistration" will occur for this Disposable when the disposer is disposed.
+};
+
+// Specify possible target types to avoid typos. Please add more types here as needed.
+type DisposerType = 'link' | 'listener' | 'inputListener';
+
// Used in subclasses to support mutate.
export type DisposableOptions = {
isDisposable?: boolean;
};
-class Disposable {
+class Disposable implements Disposer {
// Called after all code that is directly in `dispose()` methods. Be careful with mixing this pattern and the
// `this.disposeMyClass()` pattern.
@@ -35,20 +50,27 @@
// Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
private _isDisposed = false;
+ // Keep track of disposers that are added to this instance, so that they can be removed when:
+ // (a) the target is removed via another mechanism (like unlink(), or that Property being disposed)
+ // (b) the disposer is disposed
+ // Keep track of each type of target type separately, in case we ever have one container that has multiple types of
+ // targets, like a Model that has both link() and addListener()
+ private disposerMap?: Map<DisposerType, Map<IntentionalAny, { disposer: Disposer; unregisterCallback: () => void }>>;
+
// Most time, Disposable should only be used by subtypes, only instantiate it if you run into multiple inheritance issues.
public constructor( providedOptions?: DisposableOptions ) {
providedOptions && this.initializeDisposable( providedOptions );
- if ( assert ) {
+ if ( isAffirmEnabled() ) {
// Wrap the prototype dispose method with a check. NOTE: We will not catch devious cases where the dispose() is
// overridden after the Node constructor (which may happen).
const protoDispose = this.dispose;
this.dispose = () => {
- assert && assert( !this._isDisposed, 'This Disposable has already been disposed, and cannot be disposed again' );
+ affirm( !this._isDisposed, 'This Disposable has already been disposed, and cannot be disposed again' );
protoDispose.call( this );
- assert && assert( this._isDisposed, 'Disposable.dispose() call is missing from an overridden dispose method' );
+ affirm( this._isDisposed, 'Disposable.dispose() call is missing from an overridden dispose method' );
};
}
}
@@ -80,17 +102,87 @@
}
public dispose(): void {
- assert && !this._isDisposable && Disposable.assertNotDisposable();
- assert && assert( !this._isDisposed, 'Disposable can only be disposed once' );
+ isAffirmEnabled() && !this._isDisposable && Disposable.assertNotDisposable();
+ affirm( !this._isDisposed, 'Disposable can only be disposed once' );
+
+ // TODO: Emit in reverse order so that tear-downs happen in the opposite order of setup, see https://github.com/phetsims/axon/issues/455
this._disposeEmitter.emit();
this._disposeEmitter.dispose();
+
+ // Unregister any disposers that were responsible for a portion of ths instance's cleanup, since it is getting disposed.
+ if ( this.disposerMap ) {
+ for ( const [ key ] of this.disposerMap ) {
+ this.removeAllDisposers( key );
+ }
+ }
+
this._isDisposed = true;
}
+ /**
+ * Add disposables that will be disposed when this instance is disposed.
+ */
+ public addDisposable( ...disposables: Disposable[] ): void {
+ this.disposeEmitter.addListener( () => disposables.forEach( disposable => disposable.dispose() ) );
+ }
+
+ /**
+ * Adds a target to the given disposeEmitter, so that when it is called it also calls the callback.
+ * For instance, the callback would be the corresponding removal/unlink/removeInputListener.
+ */
+ public addDisposer( disposerType: DisposerType, target: IntentionalAny, disposer: Disposer, unregister: () => void ): void {
+
+ this.disposerMap = this.disposerMap || new Map();
+ if ( !this.disposerMap.has( disposerType ) ) {
+ this.disposerMap.set( disposerType, new Map() );
+ }
+
+ const entries = this.disposerMap.get( disposerType )!;
+ affirm( !entries.has( target ), 'Target already registered for disposal.' );
+
+ const unregisterCallback = () => {
+ entries.delete( target );
+ unregister();
+ };
+ entries.set( target, { disposer: disposer, unregisterCallback: unregisterCallback } );
+ disposer.disposeEmitter.addListener( unregisterCallback );
+ }
+
+ /**
+ * Remove the target on the disposer's disposeEmitter. This will not call the unregistration.
+ */
+ public removeDisposerRegistration( key: DisposerType, target: IntentionalAny ): void {
+ if ( this.disposerMap ) {
+
+ const disposers = this.disposerMap.get( key );
+ if ( disposers ) {
+ const entry = disposers.get( target );
+
+ // If there is no entry, that means either:
+ // a) this particular target was never registered with a disposer, which is fine.
+ // b) or it has already been removed via the disposeEmitter or elsewhere.
+ // Graceful is best here to avoid disposal order dependencies.
+ if ( entry ) {
+ entry.disposer.disposeEmitter.removeListener( entry.unregisterCallback );
+ disposers.delete( target );
+ }
+
+ // NOTE: Do not delete any of the Maps, as they may be used again in the future.
+ }
+ }
+ }
+
+ public removeAllDisposers( key: DisposerType ): void {
+ if ( this.disposerMap ) {
+ const disposers = this.disposerMap.get( key );
+ disposers && disposers.forEach( ( _, unregister ) => this.removeDisposerRegistration( key, unregister ) );
+ }
+ }
+
public static assertNotDisposable(): void {
// eslint-disable-next-line phet/bad-sim-text
- assert && assert( false, 'dispose is not supported, exists for the lifetime of the sim' );
+ affirm( false, 'dispose is not supported, exists for the lifetime of the sim' );
}
}
Index: axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/ReadOnlyProperty.ts b/axon/js/ReadOnlyProperty.ts
--- a/axon/js/ReadOnlyProperty.ts (revision d72042fb29d9aba1ca059055279a617786c393d5)
+++ b/axon/js/ReadOnlyProperty.ts (date 1735942171054)
@@ -21,12 +21,12 @@
import StringIO from '../../tandem/js/types/StringIO.js';
import VoidIO from '../../tandem/js/types/VoidIO.js';
import axon from './axon.js';
+import { DisposerOptions } from './Disposable.js';
import { propertyStateHandlerSingleton } from './PropertyStateHandler.js';
import PropertyStatePhase from './PropertyStatePhase.js';
import { TinyEmitterOptions } from './TinyEmitter.js';
import TinyProperty from './TinyProperty.js';
-import type TReadOnlyProperty from './TReadOnlyProperty.js';
-import type { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
+import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js';
import units, { Units } from './units.js';
import validate from './validate.js';
import Validation, { Validator, ValueComparisonStrategy } from './Validation.js';
@@ -86,7 +86,7 @@
export type LinkOptions = {
phetioDependencies?: Array<TReadOnlyProperty<unknown>>;
-};
+} & DisposerOptions; // If provided, will be unlinked when the disposable is disposed
/**
* Base class for Property, DerivedProperty, DynamicProperty. Set methods are protected/not part of the public
@@ -440,6 +440,10 @@
}
this.tinyProperty.addListener( listener ); // cannot use tinyProperty.link() because of wrong this
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'link', listener, options.disposer, () => this.unlink( listener ) );
+
listener( this.get(), null, this ); // null should be used when an object is expected but unavailable
}
@@ -451,7 +455,10 @@
if ( options && options.phetioDependencies ) {
this.addPhetioStateDependencies( options.phetioDependencies );
}
- this.tinyProperty.lazyLink( listener );
+ this.tinyProperty.lazyLink( listener ); // Note: do not pass through the disposer options
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'link', listener, options.disposer, () => this.unlink( listener ) );
}
/**
@@ -459,6 +466,9 @@
*/
public unlink( listener: PropertyListener<T> ): void {
this.tinyProperty.unlink( listener );
+
+ // undo addDisposer (see above)
+ this.removeDisposerRegistration( 'link', listener );
}
/**
@@ -466,6 +476,9 @@
*/
public unlinkAll(): void {
this.tinyProperty.unlinkAll();
+
+ // undo addDisposer (see above)
+ this.removeAllDisposers( 'link' );
}
/**
Index: scenery/js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts
--- a/scenery/js/nodes/Node.ts (revision 91dc78be838e44076b68148237515c5fb69c96a2)
+++ b/scenery/js/nodes/Node.ts (date 1735940459344)
@@ -152,6 +152,7 @@
*/
import BooleanProperty, { BooleanPropertyOptions } from '../../../axon/js/BooleanProperty.js';
+import { DisposerOptions } from '../../../axon/js/Disposable.js';
import EnabledProperty, { EnabledPropertyOptions } from '../../../axon/js/EnabledProperty.js';
import Property, { PropertyOptions } from '../../../axon/js/Property.js';
import TEmitter from '../../../axon/js/TEmitter.js';
@@ -2337,7 +2338,7 @@
* - cursor {string|null}: If node.cursor is null, any non-null cursor of an input listener will effectively
* "override" it. NOTE: this can be implemented as an es5 getter, if the cursor can change
*/
- public addInputListener( listener: TInputListener ): this {
+ public addInputListener( listener: TInputListener, options?: DisposerOptions ): this {
assert && assert( !_.includes( this._inputListeners, listener ), 'Input listener already registered on this Node' );
assert && assert( listener !== null, 'Input listener cannot be null' );
assert && assert( listener !== undefined, 'Input listener cannot be undefined' );
@@ -2353,7 +2354,11 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // If a disposer is specified, then automatically remove this input listener when the disposer is disposed.
+ options?.disposer && this.addDisposer( 'inputListener', listener, options.disposer, () => this.removeInputListener( listener ) );
}
+
return this;
}
@@ -2375,6 +2380,9 @@
if ( listener.hotkeys ) {
hotkeyManager.updateHotkeysFromInputListenerChange( this );
}
+
+ // undo addDisposer (see above)
+ this.removeDisposerRegistration( 'inputListener', listener );
}
return this; |
zepumph
added a commit
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/sun
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/bending-light
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/sun
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/bending-light
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/scenery-phet
that referenced
this issue
Jan 6, 2025
zepumph
added a commit
to phetsims/scenery
that referenced
this issue
Jan 6, 2025
|
This is working well, and CT has no complaints. Usages are in Bending light and Buoyancy suite, so we will be able to see any future problems that we missed. @samreid and I discussed before committing about how this is only available to the ReadOnlyProperty class, and not in TProperty. This is unfortunate for many spots in common code, like here, https://github.com/phetsims/scenery-phet/blob/35066b4b45399af358d37b0f6b9e52dd8b7633d3/js/BackgroundNode.ts#L60. That said, we are ok keeping this in the Disposable utility, since our primary hierarchy is there. We treat TinyProperty as a special case, which purposefully holds a smaller featureset. If we want to add this over there, or continue to support TProperty in common code, it is explicit that we aren't going to have the same level of API support that ReadOnlyProperty does. |
|
@samreid, from here, how would you like to proceed? I image that your new sim will want to use this convenience. Do you want to PSA to devs? Or let us add a few more usages and test things out before letting people know. Want to close this issue? |
|
I added as a PSA for Jan 16 dev meeting, self-unassigning until then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Now that we have established usage of Disposable.ts, I would like to reconsider an API for automatically managing unlinking Properties, removing emitter listeners, removing Node InputListeners, etc. This was originally proposed in phetsims/scenery#1494 (comment) while many things were in flux. Please be aware of concerns mentioned in the following comment phetsims/scenery#1494 (comment)
For example, here is a starter patch with the basic proposal and one sample usage:
This work was previously discussed in the following issues:
#84
#93
#214
phetsims/scenery#1494
I'm ready to re-evaluate since we have support from TypeScript, an established pattern for
Disposable, an establisheddisposeEmitter. Note that concerns in the linked issues still apply, such as teardown order.The text was updated successfully, but these errors were encountered: