CT tried to removeListener on something that wasn't a listener #67
Assignees
Comments
|
Seeing this again |
|
May be related: |
|
@jonathanolson @zepumph and I reproduced this problem by: Buoyancy => Application screen => Cement => set volume and mass to 0 => select "custom" |
|
@jonathanolson and @zepumph and I looked into this, and it seems the problem is:
The Material derives a name Property in this code: @jonathanolson hypothesizes that when a re-entrant Property changes from A=>B then B=>C we do not have the correct "oldValue" or miss it somehow. @jonathanolson was reminded of a scenery case where there was something similar in rendering SVG gradients, where we had to track the oldValue ourselves because it was showing incorrectly in the axon notifications. @jonathanolson and @zepumph agree we need a unit test to exercise this case and see if there is an incorrect or missing |
|
Patch from discoveries with @jbphet and @marlitas : Subject: [PATCH] Restore density testing, see https://github.com/phetsims/phet-io-wrappers/issues/551
---
Index: axon/js/DynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts
--- a/axon/js/DynamicProperty.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/DynamicProperty.ts (date 1695749935550)
@@ -241,13 +241,17 @@
* undefined.
*/
private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void {
+ // console.log( 'new = ', newPropertyValue, 'old', oldPropertyValue );
if ( oldPropertyValue ) {
+ console.log( this.id, 'unlinking from ', oldPropertyValue );
this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener );
}
if ( newPropertyValue ) {
+ console.log( this.id, 'linking to ', newPropertyValue );
this.derive( newPropertyValue ).link( this.propertyPropertyListener );
}
else {
+ console.log( 'null???' );
// Switch to null when our Property's value is null.
this.onPropertyPropertyChange( this.defaultValue, null, null );
}
Index: axon/js/PropertyTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/PropertyTests.ts b/axon/js/PropertyTests.ts
--- a/axon/js/PropertyTests.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/PropertyTests.ts (date 1695751222840)
@@ -311,4 +311,30 @@
thirdProperty.dispose();
} );
-}
\ No newline at end of file
+}
+
+QUnit.test( 'TEST HELLO', assert => {
+ const myProperty = new Property( 'a',{
+ reentrant: true
+ } );
+ myProperty.link( value => {
+ if ( value === 'b' ) {
+ myProperty.value = 'c';
+ }
+ } );
+
+ myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
+ myProperty.value = 'b';
+
+ assert.ok( true, 'first test' );
+
+ // We hope:
+ // null => a
+ // a => b
+ // b => c
+
+ // We think since it is buggy we will instead get:
+ // null=>a
+ // b=>c
+ // a=>b
+} );
\ No newline at end of file
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 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/TinyEmitter.ts (date 1695750540343)
@@ -171,7 +171,10 @@
// Throw an error when removing a non-listener (except when the Emitter has already been disposed, see
// https://github.com/phetsims/sun/issues/394#issuecomment-419998231
if ( assert && !this.isDisposed ) {
- assert( this.listeners.has( listener ), 'tried to removeListener on something that wasn\'t a listener' );
+ if ( !this.listeners.has( listener ) ) {
+ console.log( this.listeners.has( listener ), 'tried to removeListener on something that wasn\'t a listener' );
+ return;
+ }
}
this.guardListeners();
this.listeners.delete( listener );
Index: density-buoyancy-common/js/buoyancy/view/BoatView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/BoatView.ts b/density-buoyancy-common/js/buoyancy/view/BoatView.ts
--- a/density-buoyancy-common/js/buoyancy/view/BoatView.ts (revision e3e1c677c4b60789c7a497f772567a3de905ca67)
+++ b/density-buoyancy-common/js/buoyancy/view/BoatView.ts (date 1695746335194)
@@ -142,6 +142,17 @@
boat.displacementVolumeProperty,
boat.basin.liquidVolumeProperty
], ( boatLiquidY, boatDisplacement, boatLiquidVolume ) => {
+
+ setTimeout( () => updateItAll(), 0 );
+
+ } );
+
+ const updateItAll = () => {
+
+ const boatLiquidY = boat.basin.liquidYInterpolatedProperty.value;
+ const boatDisplacement = boat.displacementVolumeProperty.value;
+ const boatLiquidVolume = boat.basin.liquidVolumeProperty.value;
+
const poolLiquidY = liquidYInterpolatedProperty.value;
const liters = boatDisplacement / 0.001;
@@ -169,7 +180,7 @@
}
bottomPoolClipPlane.constant = poolLiquidY;
topPoolClipPlane.constant = -poolLiquidY;
- } );
+ };
Material.linkLiquidColor( boat.liquidMaterialProperty, topLiquidMaterial );
Material.linkLiquidColor( boat.liquidMaterialProperty, backMiddleMaterial );
Index: density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts b/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts
--- a/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts (revision e3e1c677c4b60789c7a497f772567a3de905ca67)
+++ b/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts (date 1695750122003)
@@ -26,8 +26,11 @@
align: 'center'
} );
- this.children = materialProperties.map( materialProperty => {
+ this.children = [ materialProperties[ 0 ] ].map( materialProperty => {
// Exists for the lifetime of a sim, so disposal patterns not needed.
+ const dynamicProperty = new DynamicProperty<string, string, Material>( materialProperty, { derive: material => material.nameProperty } );
+
+ console.log( 'making dynamic property material=>nameProperty', dynamicProperty.id );
return new RichText( new PatternStringProperty( new DerivedProperty( [
DensityBuoyancyCommonPreferences.volumeUnitsProperty,
DensityBuoyancyCommonStrings.densityReadoutPatternStringProperty,
@@ -35,7 +38,7 @@
], ( units, litersString, decimetersCubedString ) => {
return units === 'liters' ? litersString : decimetersCubedString;
} ), {
- material: new DynamicProperty<string, string, Material>( materialProperty, { derive: material => material.nameProperty } ),
+ material: dynamicProperty,
density: new DerivedProperty( [ materialProperty ], material => material.density / 1000 )
}, {
tandem: Tandem.OPT_OUT,
Indeed we saw the incorrect order in a listener test: QUnit.test( 'TEST HELLO', assert => {
const myProperty = new Property( 'a',{
reentrant: true
} );
myProperty.link( value => {
if ( value === 'b' ) {
myProperty.value = 'c';
}
} );
myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
myProperty.value = 'b';
assert.ok( true, 'first test' );
// We hope:
// null => a
// a => b
// b => c
// We think since it is buggy we will instead get:
// null=>a
// b=>c
// a=>b
} ); |
|
We proposed a fix for that problem above that has the correct behavior and doesn't use setTimeout. That is working well. However, when we tried to generalize to N levels of re-entrancy, it had very broken behavior: Subject: [PATCH] Restore density testing, see https://github.com/phetsims/phet-io-wrappers/issues/551
---
Index: axon/js/DynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts
--- a/axon/js/DynamicProperty.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/DynamicProperty.ts (date 1695749935550)
@@ -241,13 +241,17 @@
* undefined.
*/
private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void {
+ // console.log( 'new = ', newPropertyValue, 'old', oldPropertyValue );
if ( oldPropertyValue ) {
+ console.log( this.id, 'unlinking from ', oldPropertyValue );
this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener );
}
if ( newPropertyValue ) {
+ console.log( this.id, 'linking to ', newPropertyValue );
this.derive( newPropertyValue ).link( this.propertyPropertyListener );
}
else {
+ console.log( 'null???' );
// Switch to null when our Property's value is null.
this.onPropertyPropertyChange( this.defaultValue, null, null );
}
Index: axon/js/PropertyTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/PropertyTests.ts b/axon/js/PropertyTests.ts
--- a/axon/js/PropertyTests.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/PropertyTests.ts (date 1695752519473)
@@ -311,4 +311,62 @@
thirdProperty.dispose();
} );
-}
\ No newline at end of file
+}
+
+QUnit.test( 'TEST HELLO', assert => {
+ const myProperty = new Property( 'a', {
+ reentrant: true
+ } );
+ myProperty.link( value => {
+ if ( value === 'b' ) {
+ myProperty.value = 'c';
+ }
+ } );
+
+ myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
+ myProperty.value = 'b';
+
+ assert.ok( true, 'first test' );
+
+ // We hope:
+ // null => a
+ // a => b
+ // b => c
+
+ // We think since it is buggy we will instead get:
+ // null=>a
+ // b=>c
+ // a=>b
+} );
+
+QUnit.test( 'TEST Property reentrant multi levels ', assert => {
+ const myProperty = new Property( 'a', {
+ reentrant: true
+ } );
+ myProperty.link( value => {
+ if ( value === 'b' ) {
+ myProperty.value = 'c';
+ }
+ else if ( value === 'c' ) {
+ myProperty.value = 'd';
+ }
+ else if ( value === 'd' ) {
+ myProperty.value = 'e';
+ }
+ } );
+
+ myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
+ myProperty.value = 'b';
+
+ assert.ok( true, 'first test' );
+
+ // We hope:
+ // null => a
+ // a => b
+ // b => c
+
+ // We think since it is buggy we will instead get:
+ // null=>a
+ // b=>c
+ // a=>b
+} );
\ No newline at end of file
Index: density-buoyancy-common/js/buoyancy/view/BoatView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/BoatView.ts b/density-buoyancy-common/js/buoyancy/view/BoatView.ts
--- a/density-buoyancy-common/js/buoyancy/view/BoatView.ts (revision e3e1c677c4b60789c7a497f772567a3de905ca67)
+++ b/density-buoyancy-common/js/buoyancy/view/BoatView.ts (date 1695746335194)
@@ -142,6 +142,17 @@
boat.displacementVolumeProperty,
boat.basin.liquidVolumeProperty
], ( boatLiquidY, boatDisplacement, boatLiquidVolume ) => {
+
+ setTimeout( () => updateItAll(), 0 );
+
+ } );
+
+ const updateItAll = () => {
+
+ const boatLiquidY = boat.basin.liquidYInterpolatedProperty.value;
+ const boatDisplacement = boat.displacementVolumeProperty.value;
+ const boatLiquidVolume = boat.basin.liquidVolumeProperty.value;
+
const poolLiquidY = liquidYInterpolatedProperty.value;
const liters = boatDisplacement / 0.001;
@@ -169,7 +180,7 @@
}
bottomPoolClipPlane.constant = poolLiquidY;
topPoolClipPlane.constant = -poolLiquidY;
- } );
+ };
Material.linkLiquidColor( boat.liquidMaterialProperty, topLiquidMaterial );
Material.linkLiquidColor( boat.liquidMaterialProperty, backMiddleMaterial );
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 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/ReadOnlyProperty.ts (date 1695753423326)
@@ -94,7 +94,7 @@
private tinyProperty: TinyProperty<T>;
// whether we are in the process of notifying listeners; changed in some Property test files with @ts-expect-error
- private notifying: boolean;
+ private notifying: number;
// whether to allow reentry of calls to set
private readonly reentrant: boolean;
@@ -113,6 +113,8 @@
protected readonly valueValidator: Validator<T>;
public static readonly TANDEM_NAME_SUFFIX: string = 'Property';
+ private thingsToDoLater: Array<() => void> = [];
+
/**
* This is protected to indicate to clients that subclasses should be used instead.
* @param value - the initial value of the property
@@ -176,7 +178,7 @@
// Since we are already in the heavyweight Property, we always assign TinyProperty.useDeepEquality for clarity.
// @ts-expect-error
this.tinyProperty.useDeepEquality = options.valueComparisonStrategy && options.valueComparisonStrategy === 'equalsFunction';
- this.notifying = false;
+ this.notifying = 0;
this.reentrant = options.reentrant;
this.isDeferred = false;
this.deferredValue = null;
@@ -259,7 +261,7 @@
else if ( !this.equalsValue( value ) ) {
const oldValue = this.get();
this.setPropertyValue( value );
- this._notifyListeners( oldValue );
+ this._notifyListeners( oldValue, value );
}
}
}
@@ -289,8 +291,8 @@
/**
* NOTE: a few sims are calling this even though they shouldn't
*/
- private _notifyListeners( oldValue: T | null ): void {
- const newValue = this.get();
+ private _notifyListeners( oldValue: T | null, newValue: T ): void {
+ // const newValue = this.get();
// validate the before notifying listeners
assert && validate( newValue, this.valueValidator, VALIDATE_OPTIONS_FALSE );
@@ -308,12 +310,37 @@
} );
// notify listeners, optionally detect loops where this Property is set again before this completes.
- assert && assert( !this.notifying || this.reentrant,
+ assert && assert( this.notifying === 0 || this.reentrant,
`reentry detected, value=${newValue}, oldValue=${oldValue}` );
- this.notifying = true;
+
+ const later = this.notifying > 0;
- this.tinyProperty.emit( newValue, oldValue, this ); // cannot use tinyProperty.notifyListeners because it uses the wrong this
- this.notifying = false;
+ console.log( 'incrementing notifying = ' + this.notifying, 'oldvalue = ' + oldValue, 'newValue = ', newValue );
+ if ( oldValue === 'e' ) {
+ debugger;
+ }
+
+ this.notifying++;
+
+ if ( later ) {
+
+ console.log( 'pushing one for later that has oldValue = ' + oldValue, 'newValue', newValue );
+
+ debugger;
+ this.thingsToDoLater.unshift( () => {
+ this.tinyProperty.emit( newValue, oldValue, this ); // cannot use tinyProperty.notifyListeners because it uses the wrong this
+ } );
+ }
+ else {
+ this.tinyProperty.emit( newValue, oldValue, this ); // cannot use tinyProperty.notifyListeners because it uses the wrong this
+ }
+
+ this.notifying--;
+
+ if ( this.notifying === 0 ) {
+ this.thingsToDoLater.forEach( action => action() );
+ this.thingsToDoLater.length = 0;
+ }
Tandem.PHET_IO_ENABLED && this.isPhetioInstrumented() && this.phetioEndEvent();
}
Index: density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts b/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts
--- a/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts (revision e3e1c677c4b60789c7a497f772567a3de905ca67)
+++ b/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts (date 1695750122003)
@@ -26,8 +26,11 @@
align: 'center'
} );
- this.children = materialProperties.map( materialProperty => {
+ this.children = [ materialProperties[ 0 ] ].map( materialProperty => {
// Exists for the lifetime of a sim, so disposal patterns not needed.
+ const dynamicProperty = new DynamicProperty<string, string, Material>( materialProperty, { derive: material => material.nameProperty } );
+
+ console.log( 'making dynamic property material=>nameProperty', dynamicProperty.id );
return new RichText( new PatternStringProperty( new DerivedProperty( [
DensityBuoyancyCommonPreferences.volumeUnitsProperty,
DensityBuoyancyCommonStrings.densityReadoutPatternStringProperty,
@@ -35,7 +38,7 @@
], ( units, litersString, decimetersCubedString ) => {
return units === 'liters' ? litersString : decimetersCubedString;
} ), {
- material: new DynamicProperty<string, string, Material>( materialProperty, { derive: material => material.nameProperty } ),
+ material: dynamicProperty,
density: new DerivedProperty( [ materialProperty ], material => material.density / 1000 )
}, {
tandem: Tandem.OPT_OUT,
We feel it overlaps with the intermediate states listed in phetsims/axon#303 We saw that queueMicrotask for all callbacks did have the correct listener order. But that is a scary change. Here is the one that uses queueMicrotask at every level: Subject: [PATCH] Restore density testing, see https://github.com/phetsims/phet-io-wrappers/issues/551
---
Index: axon/js/DynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts
--- a/axon/js/DynamicProperty.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/DynamicProperty.ts (date 1695749935550)
@@ -241,13 +241,17 @@
* undefined.
*/
private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void {
+ // console.log( 'new = ', newPropertyValue, 'old', oldPropertyValue );
if ( oldPropertyValue ) {
+ console.log( this.id, 'unlinking from ', oldPropertyValue );
this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener );
}
if ( newPropertyValue ) {
+ console.log( this.id, 'linking to ', newPropertyValue );
this.derive( newPropertyValue ).link( this.propertyPropertyListener );
}
else {
+ console.log( 'null???' );
// Switch to null when our Property's value is null.
this.onPropertyPropertyChange( this.defaultValue, null, null );
}
Index: axon/js/PropertyTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/PropertyTests.ts b/axon/js/PropertyTests.ts
--- a/axon/js/PropertyTests.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/PropertyTests.ts (date 1695752519473)
@@ -311,4 +311,62 @@
thirdProperty.dispose();
} );
-}
\ No newline at end of file
+}
+
+QUnit.test( 'TEST HELLO', assert => {
+ const myProperty = new Property( 'a', {
+ reentrant: true
+ } );
+ myProperty.link( value => {
+ if ( value === 'b' ) {
+ myProperty.value = 'c';
+ }
+ } );
+
+ myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
+ myProperty.value = 'b';
+
+ assert.ok( true, 'first test' );
+
+ // We hope:
+ // null => a
+ // a => b
+ // b => c
+
+ // We think since it is buggy we will instead get:
+ // null=>a
+ // b=>c
+ // a=>b
+} );
+
+QUnit.test( 'TEST Property reentrant multi levels ', assert => {
+ const myProperty = new Property( 'a', {
+ reentrant: true
+ } );
+ myProperty.link( value => {
+ if ( value === 'b' ) {
+ myProperty.value = 'c';
+ }
+ else if ( value === 'c' ) {
+ myProperty.value = 'd';
+ }
+ else if ( value === 'd' ) {
+ myProperty.value = 'e';
+ }
+ } );
+
+ myProperty.link( ( value, oldValue ) => console.log( `link result: ${oldValue} => ${value}` ) );
+ myProperty.value = 'b';
+
+ assert.ok( true, 'first test' );
+
+ // We hope:
+ // null => a
+ // a => b
+ // b => c
+
+ // We think since it is buggy we will instead get:
+ // null=>a
+ // b=>c
+ // a=>b
+} );
\ No newline at end of file
Index: density-buoyancy-common/js/buoyancy/view/BoatView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/BoatView.ts b/density-buoyancy-common/js/buoyancy/view/BoatView.ts
--- a/density-buoyancy-common/js/buoyancy/view/BoatView.ts (revision e3e1c677c4b60789c7a497f772567a3de905ca67)
+++ b/density-buoyancy-common/js/buoyancy/view/BoatView.ts (date 1695746335194)
@@ -142,6 +142,17 @@
boat.displacementVolumeProperty,
boat.basin.liquidVolumeProperty
], ( boatLiquidY, boatDisplacement, boatLiquidVolume ) => {
+
+ setTimeout( () => updateItAll(), 0 );
+
+ } );
+
+ const updateItAll = () => {
+
+ const boatLiquidY = boat.basin.liquidYInterpolatedProperty.value;
+ const boatDisplacement = boat.displacementVolumeProperty.value;
+ const boatLiquidVolume = boat.basin.liquidVolumeProperty.value;
+
const poolLiquidY = liquidYInterpolatedProperty.value;
const liters = boatDisplacement / 0.001;
@@ -169,7 +180,7 @@
}
bottomPoolClipPlane.constant = poolLiquidY;
topPoolClipPlane.constant = -poolLiquidY;
- } );
+ };
Material.linkLiquidColor( boat.liquidMaterialProperty, topLiquidMaterial );
Material.linkLiquidColor( boat.liquidMaterialProperty, backMiddleMaterial );
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 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/ReadOnlyProperty.ts (date 1695753747505)
@@ -94,7 +94,7 @@
private tinyProperty: TinyProperty<T>;
// whether we are in the process of notifying listeners; changed in some Property test files with @ts-expect-error
- private notifying: boolean;
+ private notifying: number;
// whether to allow reentry of calls to set
private readonly reentrant: boolean;
@@ -113,6 +113,8 @@
protected readonly valueValidator: Validator<T>;
public static readonly TANDEM_NAME_SUFFIX: string = 'Property';
+ // private thingsToDoLater: Array<() => void> = [];
+
/**
* This is protected to indicate to clients that subclasses should be used instead.
* @param value - the initial value of the property
@@ -176,7 +178,7 @@
// Since we are already in the heavyweight Property, we always assign TinyProperty.useDeepEquality for clarity.
// @ts-expect-error
this.tinyProperty.useDeepEquality = options.valueComparisonStrategy && options.valueComparisonStrategy === 'equalsFunction';
- this.notifying = false;
+ this.notifying = 0;
this.reentrant = options.reentrant;
this.isDeferred = false;
this.deferredValue = null;
@@ -259,7 +261,7 @@
else if ( !this.equalsValue( value ) ) {
const oldValue = this.get();
this.setPropertyValue( value );
- this._notifyListeners( oldValue );
+ this._notifyListeners( oldValue, value );
}
}
}
@@ -289,7 +291,7 @@
/**
* NOTE: a few sims are calling this even though they shouldn't
*/
- private _notifyListeners( oldValue: T | null ): void {
+ private _notifyListeners( oldValue: T | null): void {
const newValue = this.get();
// validate the before notifying listeners
@@ -308,12 +310,37 @@
} );
// notify listeners, optionally detect loops where this Property is set again before this completes.
- assert && assert( !this.notifying || this.reentrant,
+ assert && assert( this.notifying === 0 || this.reentrant,
`reentry detected, value=${newValue}, oldValue=${oldValue}` );
- this.notifying = true;
+
+ const later = this.notifying > 0;
- this.tinyProperty.emit( newValue, oldValue, this ); // cannot use tinyProperty.notifyListeners because it uses the wrong this
- this.notifying = false;
+ console.log( 'incrementing notifying = ' + this.notifying, 'oldvalue = ' + oldValue, 'newValue = ', newValue );
+ // if ( oldValue === 'e' ) {
+ // debugger;
+ // }
+
+ this.notifying++;
+
+ // if ( later ) {
+
+ console.log( 'pushing one for later that has oldValue = ' + oldValue, 'newValue', newValue );
+
+ // debugger;
+ window.queueMicrotask( () => {
+ this.tinyProperty.emit( newValue, oldValue, this ); // cannot use tinyProperty.notifyListeners because it uses the wrong this
+ } );
+ // }
+ // else {
+ // this.tinyProperty.emit( newValue, oldValue, this ); // cannot use tinyProperty.notifyListeners because it uses the wrong this
+ // }
+
+ this.notifying--;
+
+ // if ( this.notifying === 0 ) {
+ // this.thingsToDoLater.forEach( action => action() );
+ // this.thingsToDoLater.length = 0;
+ // }
Tandem.PHET_IO_ENABLED && this.isPhetioInstrumented() && this.phetioEndEvent();
}
Index: density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts b/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts
--- a/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts (revision e3e1c677c4b60789c7a497f772567a3de905ca67)
+++ b/density-buoyancy-common/js/buoyancy/view/DensityReadoutListNode.ts (date 1695750122003)
@@ -26,8 +26,11 @@
align: 'center'
} );
- this.children = materialProperties.map( materialProperty => {
+ this.children = [ materialProperties[ 0 ] ].map( materialProperty => {
// Exists for the lifetime of a sim, so disposal patterns not needed.
+ const dynamicProperty = new DynamicProperty<string, string, Material>( materialProperty, { derive: material => material.nameProperty } );
+
+ console.log( 'making dynamic property material=>nameProperty', dynamicProperty.id );
return new RichText( new PatternStringProperty( new DerivedProperty( [
DensityBuoyancyCommonPreferences.volumeUnitsProperty,
DensityBuoyancyCommonStrings.densityReadoutPatternStringProperty,
@@ -35,7 +38,7 @@
], ( units, litersString, decimetersCubedString ) => {
return units === 'liters' ? litersString : decimetersCubedString;
} ), {
- material: new DynamicProperty<string, string, Material>( materialProperty, { derive: material => material.nameProperty } ),
+ material: dynamicProperty,
density: new DerivedProperty( [ materialProperty ], material => material.density / 1000 )
}, {
tandem: Tandem.OPT_OUT,
|
|
@zepumph and I worked out a pattern in TinyEmitter so that re-entrant |
|
I'd like to add that I also added unit tests to Property to show how that fixed things up. Subject: [PATCH] update dependencies for chains test, https://github.com/phetsims/perennial/issues/334
---
Index: js/TinyEmitterTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/TinyEmitterTests.ts b/js/TinyEmitterTests.ts
--- a/js/TinyEmitterTests.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/js/TinyEmitterTests.ts (date 1695760327497)
@@ -204,4 +204,63 @@
// Check these values when running with ?listenerOrder=reverse or ?listenerOrder=random or ?listenerOrder=random(123)
console.log( values.join( '' ) );
+} );
+
+
+class FakeEmitter {
+ listeners = [];
+ notifyingCount = 0;
+ notifyingCountListeners = [];
+ emitContexts = [];
+
+ constructor() {}
+
+ addListener( listener ) {
+ this.listeners.push( listener );
+ }
+
+ emit( arg: number ) {
+ this.emitContexts.push( {
+ listeners: this.listeners,
+ arg: arg
+ } );
+ if ( this.emitContexts.length === 1 ) {
+ while ( this.emitContexts.length > 0 ) {
+ const emitContext = this.emitContexts[ 0 ];
+ for ( let i = 0; i < emitContext.listeners.length; i++ ) {
+ const listener = emitContext.listeners[ i ];
+ listener( emitContext.arg );
+ }
+ this.emitContexts.shift(); }
+ }
+ }
+
+ handleSelf() {
+
+ }
+}
+
+
+QUnit.test( 'TinyEmitter listener order bugz', assert => {
+ assert.ok( true );
+ const emitter = new TinyEmitter();
+ emitter.addListener( number => {
+ if ( number < 10 ) {
+ emitter.emit( number + 1 );
+ }
+ } );
+ emitter.addListener( number => console.log( number ) );
+ emitter.emit( 1 );
+
+ /*
+ expect:
+ 2
+ 1
+ 3
+
+ desire:
+ 1
+ 2
+ 3
+ */
} );
\ No newline at end of file
Index: js/TinyEmitter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/TinyEmitter.ts b/js/TinyEmitter.ts
--- a/js/TinyEmitter.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/js/TinyEmitter.ts (date 1695762396344)
@@ -31,6 +31,7 @@
type EmitContext<T extends IntentionalAny[]> = {
index: number;
listenerArray?: TEmitterListener<T>[];
+ args: T;
};
// Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime.
@@ -114,30 +115,37 @@
if ( this.listeners.size > 0 ) {
const emitContext: EmitContext<T> = {
- index: 0
+ index: 0,
+ args: args
// listenerArray: [] // {Array.<function>|undefined} assigned if a mutation is made during emit
};
this.emitContexts.push( emitContext );
+ // assert && assert( this.emitContexts.length <= 1000, 'reentrant depth of 1000 seems like a bug to me!' );
- for ( const listener of this.listeners ) {
- listener( ...args );
- emitContext.index++;
-
- // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to
- // iterate over the guarded array
- if ( emitContext.listenerArray ) {
- break;
- }
- }
+ if ( this.emitContexts.length === 1 ) {
+ while ( this.emitContexts.length > 0 ) {
+ const emitContext = this.emitContexts[ 0 ];
+ const listeners = emitContext.listenerArray || Array.from( this.listeners );
+ const hadListenerArray = !!emitContext.listenerArray;
+ for ( let i = 0; i < listeners.length; i++ ) {
+ const listener = listeners[ i ];
+ listener( ...emitContext.args );
+ emitContext.index++;
+ }
- // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original
- // listeners in order from where we left off.
- if ( emitContext.listenerArray ) {
- for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) {
- emitContext.listenerArray[ i ]( ...args );
+ // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original
+ // listeners in order from where we left off.
+ if ( emitContext.listenerArray ) {
+ for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) {
+ emitContext.listenerArray[ i ]( ...args );
+ }
+ }
+ this.emitContexts.shift();
}
}
- this.emitContexts.pop();
+ else {
+ console.log( this.emitContexts.length );
+ }
}
}
|
|
We met with @jonathanolson today and things seemed really promising. We found an infinite loop in Buoyancy with these changes when switching the "Volume units" simulation preference, this patch comments out the maxWidth setter in NumberDisplay to show that that is the bug. @jonathanolson wanted to take a look at that. There is also still that infinite loop to investigate with dynamic stringTest. Subject: [PATCH] so hard, so bad
---
Index: axon/js/TinyEmitterTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyEmitterTests.ts b/axon/js/TinyEmitterTests.ts
--- a/axon/js/TinyEmitterTests.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/TinyEmitterTests.ts (date 1695850084702)
@@ -204,4 +204,19 @@
// Check these values when running with ?listenerOrder=reverse or ?listenerOrder=random or ?listenerOrder=random(123)
console.log( values.join( '' ) );
+} );
+
+QUnit.test( 'TinyEmitter listener order bugz', assert => {
+ const emitter = new TinyEmitter<[ number ]>();
+ let count = 1;
+ emitter.addListener( number => {
+ if ( number < 10 ) {
+ emitter.emit( number + 1 );
+ }
+ } );
+ emitter.addListener( number => {
+ console.log( number );
+ assert.ok( number === count++, `should go in order of emitting: ${number}` );
+ } );
+ emitter.emit( count );
} );
\ No newline at end of file
Index: axon/js/TinyPropertyTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/TinyPropertyTests.ts b/axon/js/TinyPropertyTests.ts
--- a/axon/js/TinyPropertyTests.ts (revision 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/TinyPropertyTests.ts (date 1695849980490)
@@ -21,6 +21,35 @@
assert.ok( true, 'one test' );
} );
+QUnit.test( 'TinyProperty notify in value-change order', assert => {
+ let count = 2; // starts as a value of 1, so 2 is the first value we change to.
+
+ const myProperty = new TinyProperty<number>( 1 );
+
+ myProperty.lazyLink( value => {
+ if ( value < 10 ) {
+ myProperty.value = value + 1;
+ }
+ } );
+
+ myProperty.lazyLink( ( value, oldValue ) => {
+ console.log( `asserts ${oldValue} => ${value}` );
+ assert.ok( value === oldValue + 1, `increment each time: ${oldValue} -> ${value}` );
+ assert.ok( value === count++, `increment in order expected: ${count - 2}->${count - 1}, received: ${oldValue} -> ${value}` );
+ } );
+ myProperty.value = count;
+
+ // We hope:
+ // null => a
+ // a => b
+ // b => c
+
+ // We think since it is buggy we will instead get:
+ // null=>a
+ // b=>c
+ // a=>b
+} );
+
QUnit.test( 'TinyProperty onBeforeNotify', assert => {
class MyObservedObject {
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 2c5d217ca16faaa5be669b3a1b804027ce0bafc6)
+++ b/axon/js/TinyEmitter.ts (date 1695850084680)
@@ -31,6 +31,7 @@
type EmitContext<T extends IntentionalAny[]> = {
index: number;
listenerArray?: TEmitterListener<T>[];
+ args: T;
};
// Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime.
@@ -114,30 +115,36 @@
if ( this.listeners.size > 0 ) {
const emitContext: EmitContext<T> = {
- index: 0
+ index: 0,
+ args: args //.slice() as T // TODO: do we need to slice? https://github.com/phetsims/buoyancy/issues/67
// listenerArray: [] // {Array.<function>|undefined} assigned if a mutation is made during emit
};
this.emitContexts.push( emitContext );
+ // assert && assert( this.emitContexts.length <= 1000, 'reentrant depth of 1000 seems like a bug to me!' );
- for ( const listener of this.listeners ) {
- listener( ...args );
- emitContext.index++;
-
- // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to
- // iterate over the guarded array
- if ( emitContext.listenerArray ) {
- break;
- }
- }
+ if ( this.emitContexts.length === 1 ) {
+ while ( this.emitContexts.length > 0 ) {
+ const emitContext = this.emitContexts[ 0 ];
+ const listeners = emitContext.listenerArray || Array.from( this.listeners );
+ for ( let i = 0; i < listeners.length; i++ ) {
+ listeners[ i ]( ...emitContext.args );
+ emitContext.index++;
+ }
- // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original
- // listeners in order from where we left off.
- if ( emitContext.listenerArray ) {
- for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) {
- emitContext.listenerArray[ i ]( ...args );
+ // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original
+ // listeners in order from where we left off.
+ if ( emitContext.listenerArray ) {
+ for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) {
+ emitContext.listenerArray[ i ]( ...args );
+ }
+ }
+ this.emitContexts.shift();
}
}
- this.emitContexts.pop();
+ else {
+ // TODO: delete or assert out? "too many levels deep, it must be an infinite loop" 1000 worked well in testing, https://github.com/phetsims/buoyancy/issues/67
+ console.log( 'this.emitContexts.length', this.emitContexts.length );
+ }
}
}
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 93246e46f9a0072ae8a8e05c04ddfbc110af7b5d)
+++ b/scenery/js/nodes/Node.ts (date 1695850084698)
@@ -3072,6 +3072,9 @@
public setCenterY( y: number ): this {
const currentCenterY = this.getCenterY();
if ( isFinite( currentCenterY ) ) {
+ if ( this.constructor.name === 'RichText' ) {
+ console.log( 'y', y, currentCenterY );
+ }
this.translate( 0, y - currentCenterY, true );
}
Index: scenery-phet/js/NumberDisplay.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts
--- a/scenery-phet/js/NumberDisplay.ts (revision 9d90b2325a590021ba74b585d1c110288a38292d)
+++ b/scenery-phet/js/NumberDisplay.ts (date 1695850084707)
@@ -244,8 +244,9 @@
longestStringProperty.link( longestString => {
const demoText = new Constructor( longestString, _.omit( valueTextOptions, 'tandem' ) );
- valueText.maxWidth = ( options.textOptions.maxWidth !== null ) ? options.textOptions.maxWidth! :
- ( demoText.width !== 0 ) ? demoText.width : null;
+ // TODO: This maxWidth causes a bug with "breadth first" listener order https://github.com/phetsims/buoyancy/issues/67
+ // valueText.maxWidth = ( options.textOptions.maxWidth !== null ) ? options.textOptions.maxWidth! :
+ // ( demoText.width !== 0 ) ? demoText.width : null;
demoText.maxWidth = valueText.maxWidth;
backgroundNode.rectWidth = Math.max( options.minBackgroundWidth, demoText.width + 2 * options.xMargin );
@@ -260,6 +261,11 @@
this.valueText = valueText;
this.backgroundNode = backgroundNode;
+ this.valueText.transformEmitter.listeners = new Set( [ () => {
+ debugger;
+ console.log( new Error().stack );
+ }, ...Array.from( this.valueText.transformEmitter.listeners ) ] );
+
// Align the value in the background.
ManualConstraint.create( this, [ valueText, backgroundNode ], ( valueTextProxy, backgroundNodeProxy ) => {
|
|
Since fixing #73, this issue is breaking CT just about every snapshot. |
|
Just catching up on this issue right now, it seems like there are two problems, and both feel like they are worth @jonathanolson's time in investigating. Co-assigning. |
jonathanolson
added a commit
to phetsims/axon
that referenced
this issue
Feb 7, 2024
…ing Property change notifications out of order. See phetsims/buoyancy#67, #447
|
Lots of great investigation here today. @jonathanolson and @AgustinVallejo and I were able to reproduce with steps in #67 (comment). This is far from the first time that reentrancy has meant that we can't trust the "oldValue" of the Property. For example, it was worked around in phetsims/scenery@3f1cd688. So above @jonathanolson added another workaround that doesn't solve the underlying problem, but does solve the assertion we were getting by keeping track of the actually last set Property (old value) instead of trusting the value that comes from the Property listener callback parameter. @jonathanolson and I were still worried about how this is quite buggy for DynamicProperty still in cases where the reentrant cases need to take precedent as the listened-to Property, since it will be swapped out for a previous BUT, it doesn't effect this case in buoyancy. This is because all the reentrant cases just HAPPEN to be mapping to the same value (all new instance of a custom Material have the same value for the nameProperty and the density as the combobox settles on the new value). This is totally happen stance, but at least we can close this issue. CT is clear, and we know how to proceed generally. Closing |
|
CT still shows cases of this problem, and @AgustinVallejo and I can reproduce it via this link: |
samreid
added a commit
to phetsims/axon
that referenced
this issue
Aug 5, 2024
|
Patch with refactoring and debug information, still broken: Subject: [PATCH] Make methods static, see https://github.com/phetsims/density-buoyancy-common/issues/314
---
Index: axon/js/DynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts
--- a/axon/js/DynamicProperty.ts (revision 8188a120188df2107121e308e109420718c67e20)
+++ b/axon/js/DynamicProperty.ts (date 1722896731122)
@@ -243,13 +243,26 @@
* undefined.
*/
private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void {
+ if ( this.hello ) {
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'onPropertyChange' );
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'thisdisposed', this.isDisposed );
+ }
if ( oldPropertyValue ) {
+ if ( this.hello ) {
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'remove old one' )
+ }
this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener );
}
if ( newPropertyValue ) {
+ if ( this.hello ) {
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'add new one' )
+ }
this.derive( newPropertyValue ).link( this.propertyPropertyListener );
}
else {
+ if ( this.hello ) {
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'its the else' )
+ }
// Switch to null when our Property's value is null.
this.onPropertyPropertyChange( this.defaultValue, null, null );
}
@@ -280,6 +293,9 @@
* Disposes this Property
*/
public override dispose(): void {
+ if ( this.hello ) {
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'disposing' )
+ }
this.valuePropertyProperty.unlink( this.propertyListener );
if ( this.valuePropertyProperty.value !== null ) {
Index: density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts b/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts
--- a/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts (revision c176d2fcf605eb025443a6747dfe797af3767744)
+++ b/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts (date 1722896896216)
@@ -16,11 +16,14 @@
import DensityBuoyancyCommonConstants from '../../common/DensityBuoyancyCommonConstants.js';
import { combineOptions, optionize4 } from '../../../../phet-core/js/optionize.js';
import AccordionBox, { AccordionBoxOptions } from '../../../../sun/js/AccordionBox.js';
-import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js';
-import PatternStringProperty from '../../../../axon/js/PatternStringProperty.js';
import TinyProperty from '../../../../axon/js/TinyProperty.js';
import WithRequired from '../../../../phet-core/js/types/WithRequired.js';
+import PatternStringProperty from '../../../../axon/js/PatternStringProperty.js';
+import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js';
import DerivedStringProperty from '../../../../axon/js/DerivedStringProperty.js';
+import MaterialProperty from '../../common/model/MaterialProperty.js';
+import Mass from '../../common/model/Mass.js';
+import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
const DEFAULT_FONT = new PhetFont( 14 );
const HBOX_SPACING = 5;
@@ -30,15 +33,18 @@
font: DEFAULT_FONT
};
-type SelfOptions<ReadoutType> = {
+type SelfOptions<ReadoutType = MaterialProperty | Mass> = {
// Provide the ideal max content width for the accordion box content. This is used to apply maxWidths to the Texts of the readout.
contentWidthMax?: number | TReadOnlyProperty<number>;
readoutItems?: ReadoutItemOptions<ReadoutType>[];
+
+ generateNameItem: ( readoutItem: ReadoutType ) => TReadOnlyProperty<string>;
+ generateValueItem: ( readoutItem: ReadoutType ) => TReadOnlyProperty<string>;
};
-export type ReadoutItemOptions<ReadoutType> = {
+export type ReadoutItemOptions<ReadoutType = MaterialProperty | Mass> = {
readoutItem: ReadoutType; // Provided for use by generateReadoutData() to create the name/value Properties
// By default, the implementation of generateReadoutData() will create a default nameProperty, but you can supply your
@@ -47,20 +53,18 @@
readoutFormat?: RichTextOptions; // Any extra formatting options to be passed ONLY to the value text.
};
-export type ReadoutData = {
- nameProperty: TReadOnlyProperty<string>;
- valueProperty: TReadOnlyProperty<string>;
-};
+export type ReadoutListAccordionBoxOptions<ReadoutType = MaterialProperty | Mass> = SelfOptions<ReadoutType> & WithRequired<AccordionBoxOptions, 'tandem'>;
-export type ReadoutListAccordionBoxOptions<ReadoutType> = SelfOptions<ReadoutType> & WithRequired<AccordionBoxOptions, 'tandem'>;
-
-export default abstract class ReadoutListAccordionBox<ReadoutType> extends AccordionBox {
+export default abstract class ReadoutListAccordionBox<ReadoutType = MaterialProperty | Mass> extends AccordionBox {
protected readonly cleanupEmitter = new TinyEmitter();
private readonly readoutBox: VBox;
private readonly contentWidthMaxProperty: TReadOnlyProperty<number>;
+ private readonly generateValueItem: ( readoutItem: ReadoutType ) => TReadOnlyProperty<string>;
+ private readonly generateNameItem: ( readoutItem: ReadoutType ) => TReadOnlyProperty<string>;
+
protected constructor(
titleStringProperty: TReadOnlyProperty<string>,
providedOptions?: ReadoutListAccordionBoxOptions<ReadoutType>
@@ -96,71 +100,80 @@
this.contentWidthMaxProperty.link( maxWidthListener );
this.disposeEmitter.addListener( () => this.contentWidthMaxProperty.unlink( maxWidthListener ) );
+ this.generateValueItem = options.generateValueItem;
+ this.generateNameItem = options.generateNameItem;
+
options.readoutItems && this.setReadoutItems( options.readoutItems );
}
public setReadoutItems( readoutItems: ReadoutItemOptions<ReadoutType>[] ): void {
+ this.readoutBox.children.forEach( child => child.dispose() );
+ this.readoutBox.children = readoutItems.map( readoutItem => new ReadoutBoxLine(
+ readoutItem,
+ this.contentWidthMaxProperty,
+ item => this.generateValueItem( item ),
+ item => this.generateNameItem( item )
+ ) );
+ }
- // Clear the previous materials that may have been created.
- this.cleanupEmitter.emit();
- this.cleanupEmitter.removeAllListeners();
+ public override dispose(): void {
+ assert && assert( false, 'Not disposable' );
+ this.readoutBox.children.forEach( child => child.dispose() );
+ super.dispose();
+ }
+}
- this.readoutBox.children = readoutItems.map( readoutItem => {
+class ReadoutBoxLine<T> extends HBox {
+ public constructor( readoutItem: ReadoutItemOptions<T>, contentWidthMaxProperty: TReadOnlyProperty<number>, generateValueProperty: ( t: T ) => TReadOnlyProperty<string>, generateNameProperty: ( t: T ) => TReadOnlyProperty<string> ) {
- // TODO: Create a new type and file for this data structure, see https://github.com/phetsims/density-buoyancy-common/issues/123
- // TODO: So that the map will read readoutItems.map( item => new ReadoutItemNode(item)), and dispose has a method, see https://github.com/phetsims/density-buoyancy-common/issues/123
- const readoutData = this.generateReadoutData( readoutItem.readoutItem );
- const nameProperty = readoutItem.readoutNameProperty || readoutData.nameProperty;
- const nameColonProperty = new PatternStringProperty(
- DensityBuoyancyCommonStrings.nameColonPatternStringProperty, {
- name: nameProperty
- } );
+ const valueProperty = generateValueProperty( readoutItem.readoutItem );
+ const nameProperty = readoutItem.readoutNameProperty ? new DerivedProperty( [ readoutItem.readoutNameProperty ], name => name ) : generateNameProperty( readoutItem.readoutItem );
+ const nameColonProperty = new PatternStringProperty(
+ DensityBuoyancyCommonStrings.nameColonPatternStringProperty, {
+ name: nameProperty
+ } );
- const labelText = new RichText( nameColonProperty, TEXT_OPTIONS );
- const readoutFormat = readoutItem.readoutFormat ? readoutItem.readoutFormat : {};
- const valueText = new RichText( readoutData.valueProperty,
- combineOptions<RichTextOptions>( {
+ const labelText = new RichText( nameColonProperty, TEXT_OPTIONS );
+ const readoutFormat = readoutItem.readoutFormat ? readoutItem.readoutFormat : {};
+ const derivedStringProperty = new DerivedStringProperty( [ nameColonProperty, valueProperty ], ( name, value ) => {
+ return `${name} ${value}`;
+ } );
+ const valueText = new RichText( valueProperty,
+ combineOptions<RichTextOptions>( {
- // A11y content for the PDOM
- tagName: 'p',
- innerContent: new DerivedStringProperty( [ nameColonProperty, readoutData.valueProperty ], ( name, value ) => {
- return `${name} ${value}`;
- } )
- }, TEXT_OPTIONS, readoutFormat ) );
+ // A11y content for the PDOM
+ tagName: 'p',
+ innerContent: derivedStringProperty
+ }, TEXT_OPTIONS, readoutFormat ) );
- const maxWidthListener = ( contentWidthMax: number ) => {
- const maxWidth = ( contentWidthMax - HBOX_SPACING ) / 2;
- labelText.maxWidth = maxWidth;
- valueText.maxWidth = maxWidth;
- };
- this.contentWidthMaxProperty.link( maxWidthListener );
-
- this.cleanupEmitter.addListener( () => {
- this.contentWidthMaxProperty.unlink( maxWidthListener );
- valueText.dispose();
- labelText.dispose();
- nameColonProperty.dispose();
- } );
+ const maxWidthListener = ( contentWidthMax: number ) => {
+ const maxWidth = ( contentWidthMax - HBOX_SPACING ) / 2;
+ labelText.maxWidth = maxWidth;
+ valueText.maxWidth = maxWidth;
+ };
+ contentWidthMaxProperty.link( maxWidthListener );
- const alignGroup = new AlignGroup();
- return new HBox( {
- children: [
- alignGroup.createBox( labelText, { xAlign: 'right' } ),
- alignGroup.createBox( valueText, { xAlign: 'left' } )
- ],
- align: 'origin',
- justify: 'center',
- spacing: DensityBuoyancyCommonConstants.SPACING_SMALL
- } );
+ const alignGroup = new AlignGroup();
+ super( {
+ children: [
+ alignGroup.createBox( labelText, { xAlign: 'right' } ),
+ alignGroup.createBox( valueText, { xAlign: 'left' } )
+ ],
+ align: 'origin',
+ justify: 'center',
+ spacing: DensityBuoyancyCommonConstants.SPACING_SMALL
} );
- }
- protected abstract generateReadoutData( readoutType: ReadoutType ): ReadoutData;
-
- public override dispose(): void {
- assert && assert( false, 'Not disposable' );
- this.cleanupEmitter.emit();
- super.dispose();
+ this.disposeEmitter.addListener( () => {
+ contentWidthMaxProperty.unlink( maxWidthListener );
+ valueText.dispose();
+ labelText.dispose();
+ nameColonProperty.dispose();
+ valueProperty.dispose();
+ // nameProperty.dispose();
+ alignGroup.dispose();
+ derivedStringProperty.dispose();
+ } );
}
}
Index: density-buoyancy-common/js/buoyancy/view/DensityAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/DensityAccordionBox.ts b/density-buoyancy-common/js/buoyancy/view/DensityAccordionBox.ts
--- a/density-buoyancy-common/js/buoyancy/view/DensityAccordionBox.ts (revision c176d2fcf605eb025443a6747dfe797af3767744)
+++ b/density-buoyancy-common/js/buoyancy/view/DensityAccordionBox.ts (date 1722896868767)
@@ -14,14 +14,16 @@
import DensityBuoyancyCommonConstants from '../../common/DensityBuoyancyCommonConstants.js';
import StringUtils from '../../../../phetcommon/js/util/StringUtils.js';
import Utils from '../../../../dot/js/Utils.js';
-import ReadoutListAccordionBox, { ReadoutData, ReadoutListAccordionBoxOptions } from './ReadoutListAccordionBox.js';
+import ReadoutListAccordionBox, { ReadoutListAccordionBoxOptions } from './ReadoutListAccordionBox.js';
import DerivedStringProperty from '../../../../axon/js/DerivedStringProperty.js';
import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
import MaterialProperty from '../../common/model/MaterialProperty.js';
+import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
+import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
type ParentOptions = ReadoutListAccordionBoxOptions<MaterialProperty>;
type SelfOptions = EmptySelfOptions;
-type DensityAccordionBoxOptions = SelfOptions & ParentOptions;
+type DensityAccordionBoxOptions = SelfOptions & StrictOmit<ParentOptions, 'generateNameItem' | 'generateValueItem'>;
export default class DensityAccordionBox extends ReadoutListAccordionBox<MaterialProperty> {
@@ -29,28 +31,21 @@
const options = optionize<DensityAccordionBoxOptions, SelfOptions, ParentOptions>()( {
expandedDefaultValue: false,
- accessibleName: DensityBuoyancyCommonStrings.densityStringProperty
- }, providedOptions );
-
- super( titleStringProperty, options );
- }
+ accessibleName: DensityBuoyancyCommonStrings.densityStringProperty,
+ generateNameItem: materialProperty => {
- protected override generateReadoutData( materialProperty: MaterialProperty ): ReadoutData {
-
- // Use DynamicProperty so that this name is updated based on the material AND material's name changing.
- const nameProperty = new DynamicProperty<string, string, Material>( materialProperty, {
- derive: material => material.nameProperty
- } );
-
- // Returns the filled in string for the material readout or '?' if the material is hidden
- const valueProperty = new DerivedStringProperty(
- [
+ const dynaprop = new DynamicProperty<string, string, Material>( materialProperty, {
+ derive: material => new DerivedProperty( [ material.nameProperty ], name => name )
+ } );
+ dynaprop.hello = true;
+ return dynaprop;
+ },
+ generateValueItem: materialProperty => new DerivedStringProperty( [
materialProperty,
materialProperty.densityProperty,
DensityBuoyancyCommonConstants.KILOGRAMS_PER_VOLUME_PATTERN_STRING_PROPERTY,
DensityBuoyancyCommonStrings.questionMarkStringProperty
- ],
- ( material, density, patternStringProperty, questionMarkString ) => {
+ ], ( material, density, patternStringProperty, questionMarkString ) => {
return material.hidden ?
questionMarkString :
StringUtils.fillIn( patternStringProperty, {
@@ -61,17 +56,10 @@
value: Utils.toFixed( density / DensityBuoyancyCommonConstants.LITERS_IN_CUBIC_METER, 2 ),
decimalPlaces: 2
} );
- } );
+ } )
+ }, providedOptions );
- this.cleanupEmitter.addListener( () => {
- nameProperty.dispose();
- valueProperty.dispose();
- } );
-
- return {
- nameProperty: nameProperty,
- valueProperty: valueProperty
- };
+ super( titleStringProperty, options );
}
}
Index: density-buoyancy-common/js/buoyancy/view/SubmergedAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/SubmergedAccordionBox.ts b/density-buoyancy-common/js/buoyancy/view/SubmergedAccordionBox.ts
--- a/density-buoyancy-common/js/buoyancy/view/SubmergedAccordionBox.ts (revision c176d2fcf605eb025443a6747dfe797af3767744)
+++ b/density-buoyancy-common/js/buoyancy/view/SubmergedAccordionBox.ts (date 1722895931915)
@@ -8,45 +8,38 @@
import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
import { combineOptions } from '../../../../phet-core/js/optionize.js';
-import Utils from '../../../../dot/js/Utils.js';
-import ReadoutListAccordionBox, { ReadoutData, ReadoutListAccordionBoxOptions } from './ReadoutListAccordionBox.js';
+import ReadoutListAccordionBox, { ReadoutListAccordionBoxOptions } from './ReadoutListAccordionBox.js';
import DensityBuoyancyCommonStrings from '../../DensityBuoyancyCommonStrings.js';
import DensityBuoyancyCommonPreferences from '../../common/model/DensityBuoyancyCommonPreferences.js';
import Mass from '../../common/model/Mass.js';
import PatternStringProperty from '../../../../axon/js/PatternStringProperty.js';
+import Utils from '../../../../dot/js/Utils.js';
+import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
+import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
export default class SubmergedAccordionBox extends ReadoutListAccordionBox<Mass> {
- public constructor( providedOptions?: ReadoutListAccordionBoxOptions<Mass> ) {
+ public constructor( providedOptions?: StrictOmit<ReadoutListAccordionBoxOptions<Mass>, 'generateNameItem' | 'generateValueItem'> ) {
const options = combineOptions<ReadoutListAccordionBoxOptions<Mass>>( {
visibleProperty: DensityBuoyancyCommonPreferences.percentSubmergedVisibleProperty,
readoutItems: [],
expandedDefaultValue: false,
- accessibleName: DensityBuoyancyCommonStrings.percentSubmergedStringProperty
- }, providedOptions );
-
- super( DensityBuoyancyCommonStrings.percentSubmergedStringProperty, options );
- }
+ accessibleName: DensityBuoyancyCommonStrings.percentSubmergedStringProperty,
- protected override generateReadoutData( mass: Mass ): ReadoutData {
-
- const patternStringProperty = new PatternStringProperty( DensityBuoyancyCommonStrings.valuePercentStringProperty, {
- value: mass.percentSubmergedProperty
- }, {
- maps: {
- value: percentSubmerged => Utils.toFixed( percentSubmerged, 1 )
- }
- } );
- this.cleanupEmitter.addListener( () => {
- patternStringProperty.dispose();
- } );
+ // Thin wrapper that is safe to dispose
+ generateNameItem: mass => new DerivedProperty( [ mass.nameProperty ], name => name ),
+ generateValueItem: mass => new PatternStringProperty( DensityBuoyancyCommonStrings.valuePercentStringProperty, {
+ value: mass.percentSubmergedProperty
+ }, {
+ maps: {
+ value: percentSubmerged => Utils.toFixed( percentSubmerged, 1 )
+ }
+ } )
+ }, providedOptions );
- return {
- nameProperty: mass.nameProperty,
- valueProperty: patternStringProperty
- };
+ super( DensityBuoyancyCommonStrings.percentSubmergedStringProperty, options );
}
}
|
|
Steps to reproduce the problem:
|
samreid
added
dev:help-wanted
This was referenced Aug 6, 2024
samreid
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Aug 8, 2024
|
Even removing all of the cleanupEmitter work in the accordion boxes: Subject: [PATCH] Eliminate availableMasses.remove, see https://github.com/phetsims/density-buoyancy-common/issues/331
---
Index: js/buoyancy/view/ReadoutListAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/ReadoutListAccordionBox.ts b/js/buoyancy/view/ReadoutListAccordionBox.ts
--- a/js/buoyancy/view/ReadoutListAccordionBox.ts (revision 9038a37285a27b952d9ccee43a93c60d70890788)
+++ b/js/buoyancy/view/ReadoutListAccordionBox.ts (date 1723581970099)
@@ -12,7 +12,6 @@
import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
import { AlignGroup, HBox, RichText, RichTextOptions, Text, VBox } from '../../../../scenery/js/imports.js';
import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
-import TinyEmitter from '../../../../axon/js/TinyEmitter.js';
import DensityBuoyancyCommonConstants from '../../common/DensityBuoyancyCommonConstants.js';
import { combineOptions, optionize4 } from '../../../../phet-core/js/optionize.js';
import AccordionBox, { AccordionBoxOptions } from '../../../../sun/js/AccordionBox.js';
@@ -57,7 +56,7 @@
export default abstract class ReadoutListAccordionBox<ReadoutType> extends AccordionBox {
- protected readonly cleanupEmitter = new TinyEmitter();
+ // protected readonly cleanupEmitter = new TinyEmitter();
private readonly readoutBox: VBox;
private readonly contentWidthMaxProperty: TReadOnlyProperty<number>;
@@ -103,8 +102,8 @@
public setReadoutItems( readoutItems: ReadoutItemOptions<ReadoutType>[] ): void {
// Clear the previous materials that may have been created.
- this.cleanupEmitter.emit();
- this.cleanupEmitter.removeAllListeners();
+ // this.cleanupEmitter.emit();
+ // this.cleanupEmitter.removeAllListeners();
this.readoutBox.children = readoutItems.map( readoutItem => {
@@ -136,12 +135,12 @@
};
this.contentWidthMaxProperty.link( maxWidthListener );
- this.cleanupEmitter.addListener( () => {
- this.contentWidthMaxProperty.unlink( maxWidthListener );
- valueText.dispose();
- labelText.dispose();
- nameColonProperty.dispose();
- } );
+ // this.cleanupEmitter.addListener( () => {
+ // this.contentWidthMaxProperty.unlink( maxWidthListener );
+ // valueText.dispose();
+ // labelText.dispose();
+ // nameColonProperty.dispose();
+ // } );
const alignGroup = new AlignGroup();
return new HBox( {
Index: js/buoyancy/view/DensityAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/DensityAccordionBox.ts b/js/buoyancy/view/DensityAccordionBox.ts
--- a/js/buoyancy/view/DensityAccordionBox.ts (revision 9038a37285a27b952d9ccee43a93c60d70890788)
+++ b/js/buoyancy/view/DensityAccordionBox.ts (date 1723582005437)
@@ -63,10 +63,10 @@
} );
} );
- this.cleanupEmitter.addListener( () => {
- nameProperty.dispose();
- valueProperty.dispose();
- } );
+ // this.cleanupEmitter.addListener( () => {
+ // nameProperty.dispose();
+ // valueProperty.dispose();
+ // } );
return {
nameProperty: nameProperty,
Index: js/buoyancy/view/SubmergedAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buoyancy/view/SubmergedAccordionBox.ts b/js/buoyancy/view/SubmergedAccordionBox.ts
--- a/js/buoyancy/view/SubmergedAccordionBox.ts (revision 9038a37285a27b952d9ccee43a93c60d70890788)
+++ b/js/buoyancy/view/SubmergedAccordionBox.ts (date 1723581988414)
@@ -42,9 +42,9 @@
value: percentSubmerged => Utils.toFixed( percentSubmerged, 1 )
}
} );
- this.cleanupEmitter.addListener( () => {
- patternStringProperty.dispose();
- } );
+ // this.cleanupEmitter.addListener( () => {
+ // patternStringProperty.dispose();
+ // } );
return {
nameProperty: mass.nameProperty,
There is still a |
|
This patch demonstrates that we attempt to dispose the listener before it is linked: Subject: [PATCH] Eliminate availableMasses.remove, see https://github.com/phetsims/density-buoyancy-common/issues/331
---
Index: axon/js/DynamicProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts
--- a/axon/js/DynamicProperty.ts (revision 7fc653baeca10f122b62e2ad8dcf803cc226decd)
+++ b/axon/js/DynamicProperty.ts (date 1723595144158)
@@ -205,6 +205,12 @@
// No unlink needed, since our own disposal will remove this listener.
this.lazyLink( this.onSelfChange.bind( this ) );
}
+
+ setInterval( () => {
+ if ( this.id === 4888 ) {
+ console.log( phet.preloads.phetio.queryParameters.frameTitle + ': 4888.isdisposed = ', this.isDisposed )
+ }
+ }, 1000 );
}
/**
@@ -244,10 +250,18 @@
*/
private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void {
if ( oldPropertyValue ) {
- this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener );
+ const derive = this.derive( oldPropertyValue );
+ if (derive.id===508){
+ console.log( phet.preloads.phetio.queryParameters.frameTitle + ': 508.unlink old');
+ }
+ derive.unlink( this.propertyPropertyListener );
}
if ( newPropertyValue ) {
- this.derive( newPropertyValue ).link( this.propertyPropertyListener );
+ const derive1 = this.derive( newPropertyValue );
+ if (derive1.id===508){
+ console.log( phet.preloads.phetio.queryParameters.frameTitle + ': 508.link new');
+ }
+ derive1.link( this.propertyPropertyListener );
}
else {
// Switch to null when our Property's value is null.
@@ -280,10 +294,25 @@
* Disposes this Property
*/
public override dispose(): void {
+
+ assert && assert( !this.isDisposed, 'Disposable can only be disposed once' );
+
+ console.log( phet.preloads.phetio.queryParameters.frameTitle, 'id', this.id, 'dispose, ', this.valuePropertyProperty.phetioID );
+ if ( this.valuePropertyProperty.phetioID === 'buoyancy.applicationsScreen.model.objects.bottle.materialInside.materialProperty' ) {
+ debugger;
+ }
this.valuePropertyProperty.unlink( this.propertyListener );
if ( this.valuePropertyProperty.value !== null ) {
- this.derive( this.valuePropertyProperty.value ).unlink( this.propertyPropertyListener );
+
+
+ const derive = this.derive( this.valuePropertyProperty.value );
+
+ if (derive.id===508){
+ console.log( phet.preloads.phetio.queryParameters.frameTitle + ': 508.dispose');
+ }
+
+ derive.hasListener(this.propertyPropertyListener) && derive.unlink( this.propertyPropertyListener );
}
super.dispose();
Index: density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts b/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts
--- a/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts (revision 9038a37285a27b952d9ccee43a93c60d70890788)
+++ b/density-buoyancy-common/js/buoyancy/view/ReadoutListAccordionBox.ts (date 1723593170022)
@@ -140,7 +140,7 @@
this.contentWidthMaxProperty.unlink( maxWidthListener );
valueText.dispose();
labelText.dispose();
- nameColonProperty.dispose();
+ // nameColonProperty.dispose();
} );
const alignGroup = new AlignGroup();
Index: density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts b/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts
--- a/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts (revision 9038a37285a27b952d9ccee43a93c60d70890788)
+++ b/density-buoyancy-common/js/buoyancy/view/applications/BuoyancyApplicationsScreenView.ts (date 1723595144113)
@@ -38,6 +38,8 @@
import getBottleIcon from './getBottleIcon.js';
import BoatPanel from './BoatPanel.js';
import BottlePanel from './BottlePanel.js';
+import propertyStateHandlerSingleton from '../../../../../axon/js/propertyStateHandlerSingleton.js';
+import PropertyStatePhase from '../../../../../axon/js/PropertyStatePhase.js';
type BuoyancyApplicationsScreenViewOptions = StrictOmit<BuoyancyScreenViewOptions, 'supportsDepthLines' | 'forcesInitiallyDisplayed' | 'massValuesInitiallyDisplayed' | 'initialForceScale'>;
@@ -145,6 +147,10 @@
const displayedMysteryMaterials = [ Material.FLUID_E, Material.FLUID_F ];
const invisibleMaterials = [ ...Material.BUOYANCY_FLUID_MYSTERY_MATERIALS ].filter( material => !displayedMysteryMaterials.includes( material ) );
+ // propertyStateHandlerSingleton.registerPhetioOrderDependency( model.applicationModeProperty, PropertyStatePhase.UNDEFER, model.bottle.materialInsideProperty, PropertyStatePhase.UNDEFER );
+ // propertyStateHandlerSingleton.registerPhetioOrderDependency( model.applicationModeProperty, PropertyStatePhase.UNDEFER, DensityBuoyancyCommonStrings.material.oilStringProperty, PropertyStatePhase.UNDEFER );
+ // propertyStateHandlerSingleton.registerPhetioOrderDependency( model.applicationModeProperty, PropertyStatePhase.UNDEFER, DensityBuoyancyCommonColors.materialOilColorProperty, PropertyStatePhase.UNDEFER );
+
model.applicationModeProperty.link( scene => {
const materials = scene === 'bottle' ? [ model.bottle.materialInsideProperty, model.bottle.materialProperty ] :
scene === 'boat' ? [ model.block.materialProperty, model.boat.materialProperty ] : [];
Index: axon/js/Multilink.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/axon/js/Multilink.ts b/axon/js/Multilink.ts
--- a/axon/js/Multilink.ts (revision 7fc653baeca10f122b62e2ad8dcf803cc226decd)
+++ b/axon/js/Multilink.ts (date 1723593479702)
@@ -127,8 +127,6 @@
const values = dependencies.map( dependency => dependency.get() ) as [ T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15 ];
callback( ...values );
}
-
- this.isDisposed = false;
}
/**
Index: bamboo/js/CanvasPainter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bamboo/js/CanvasPainter.ts b/bamboo/js/CanvasPainter.ts
--- a/bamboo/js/CanvasPainter.ts (revision 2c5740011601cf178bd7a262af2e377eaaf32d32)
+++ b/bamboo/js/CanvasPainter.ts (date 1723593479772)
@@ -30,8 +30,7 @@
}, providedOptions );
this.visible = options.visible;
- this.isDisposed = false;
- }
+ };
// override to paint or change the canvas context state. Only called if this.visible is true
public abstract paintCanvas( context: CanvasRenderingContext2D ): void;
|
samreid
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Aug 14, 2024
samreid
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Aug 14, 2024
samreid
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Aug 14, 2024
samreid
added
status:ready-for-review
and removed
dev:help-wanted
zepumph
added a commit
to phetsims/utterance-queue
that referenced
this issue
Aug 14, 2024
…hetsims/buoyancy#67 Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Aug 14, 2024
…cy#67 Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
to phetsims/axon
that referenced
this issue
Aug 14, 2024
…cy#67 Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
to phetsims/axon
that referenced
this issue
Aug 14, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph
added a commit
to phetsims/axon
that referenced
this issue
Aug 14, 2024
…hetsims/buoyancy#67 Signed-off-by: Michael Kauzmann <[email protected]>
|
@zepumph and I fixed this and it has been clear for 10+ columns on CT. Thanks everyone, closing. |
|
Reopening because there is a TODO marked for this issue. |
samreid
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Aug 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The text was updated successfully, but these errors were encountered: