From 5afbfce895a49dfb93d1dfb094328d6563ea9a6a Mon Sep 17 00:00:00 2001 From: tmildemberger Date: Sun, 16 Feb 2020 23:01:32 -0300 Subject: [PATCH] Document the need of unlink or dispose for #51 --- js/common/model/Spring.js | 1 + js/common/view/MassNode.js | 2 ++ js/common/view/SpringNode.js | 2 ++ js/one-dimension/model/OneDimensionModel.js | 12 +++++++++--- js/one-dimension/view/MassNode1D.js | 2 ++ .../view/NormalModeSpectrumAccordionBox.js | 2 ++ js/one-dimension/view/NormalModesAccordionBox.js | 2 ++ js/one-dimension/view/WallNode.js | 1 + js/two-dimensions/model/TwoDimensionsModel.js | 8 ++++++-- js/two-dimensions/view/AmplitudeSelectorRectangle.js | 4 ++++ js/two-dimensions/view/MassNode2D.js | 2 ++ .../view/NormalModeAmplitudesAccordionBox.js | 3 +++ 12 files changed, 36 insertions(+), 5 deletions(-) diff --git a/js/common/model/Spring.js b/js/common/model/Spring.js index 07f2204..37f7729 100644 --- a/js/common/model/Spring.js +++ b/js/common/model/Spring.js @@ -25,6 +25,7 @@ define( require => { this.rightMass = rightMass; // @public {Property.} determines the visibility of the spring + // dispose is unnecessary because all masses and springs exist for the lifetime of the sim this.visibilityProperty = new DerivedProperty( [ this.leftMass.visibilityProperty, this.rightMass.visibilityProperty ], function( leftVisible, rightVisible ) { diff --git a/js/common/view/MassNode.js b/js/common/view/MassNode.js index 629a874..a6cb764 100644 --- a/js/common/view/MassNode.js +++ b/js/common/view/MassNode.js @@ -37,10 +37,12 @@ define( require => { this.size = 20; // @public {Property.} determines the visibility of the MassNode + // TODO - this property is unnecessary (see https://github.com/phetsims/normal-modes/issues/45) this.visibilityProperty = new DerivedProperty( [ this.mass.visibilityProperty ], function( massVisible ) { return massVisible; } ); + // dispose is unnecessary, the MassNode and the dependencies exist for the lifetime of the sim Property.multilink( [ this.mass.equilibriumPositionProperty, this.mass.displacementProperty ], ( massPosition, massDisplacement ) => { this.translation = this.modelViewTransform.modelToViewPosition( massPosition.plus( massDisplacement ) ); diff --git a/js/common/view/SpringNode.js b/js/common/view/SpringNode.js index 495b67d..786e3d0 100644 --- a/js/common/view/SpringNode.js +++ b/js/common/view/SpringNode.js @@ -39,6 +39,7 @@ define( require => { this.model = model; // @public {Property.} determines the visibility of the SpringNode + // dispose is unnecessary because the SpringNode and the dependencies exist for the lifetime of the sim this.visibilityProperty = new DerivedProperty( [ this.spring.visibilityProperty, this.model.springsVisibilityProperty ], function( mySpringVisible, springsVisible ) { @@ -61,6 +62,7 @@ define( require => { let currentXScaling = 1; + // dispose is unnecessary because the SpringNode and the dependencies exist for the lifetime of the sim Property.multilink( [ this.spring.leftMass.equilibriumPositionProperty, this.spring.leftMass.displacementProperty, diff --git a/js/one-dimension/model/OneDimensionModel.js b/js/one-dimension/model/OneDimensionModel.js index 36ec096..64391e7 100644 --- a/js/one-dimension/model/OneDimensionModel.js +++ b/js/one-dimension/model/OneDimensionModel.js @@ -91,6 +91,7 @@ define( require => { range: new Range( OneDimensionConstants.MIN_MODE_PHASE, OneDimensionConstants.MAX_MODE_PHASE ) } ); + // dispose is unnecessary, since this class owns the dependency this.modeFrequencyProperty[ i ] = new DerivedProperty( [ this.numVisibleMassesProperty ], function( numMasses ) { const k = OneDimensionConstants.SPRING_CONSTANT_VALUE; const m = OneDimensionConstants.MASSES_MASS_VALUE; @@ -112,9 +113,6 @@ define( require => { this.springs = new Array( MAX_SPRINGS ); this.createDefaultSprings(); - this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) ); - this.amplitudeDirectionProperty.link( this.changedAmplitudeDirection.bind( this ) ); - // @public {Property.} the index of the mass being dragged this.draggingMassIndexProperty = new NumberProperty( 0, { tandem: tandem.createTandem( 'draggingMassIndexProperty' ) @@ -124,6 +122,12 @@ define( require => { this.arrowsVisibilityProperty = new BooleanProperty( true, { tandem: tandem.createTandem( 'arrowsVisibilityProperty' ) } ); + + // unlink is unnecessary, exists for the lifetime of the sim + this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) ); + + // unlink is unnecessary, exists for the lifetime of the sim + this.amplitudeDirectionProperty.link( this.changedAmplitudeDirection.bind( this ) ); } /** @@ -183,6 +187,7 @@ define( require => { for ( let i = 0; i < MAX_MASSES; i++ ) { const visible = ( i <= defaultMassesNum ); + // All the masses needed are created at once, and exist for the lifetime of the sim this.masses[ i ] = new Mass( new Vector2( x, 0 ), visible, tandem.createTandem( 'mass' + i ) ); if ( x < xFinal ) { @@ -197,6 +202,7 @@ define( require => { */ createDefaultSprings() { for ( let i = 0; i < MAX_SPRINGS; i++ ) { + // All the springs needed are created at once, and exist for the lifetime of the sim this.springs[ i ] = new Spring( this.masses[ i ], this.masses[ i + 1 ] ); } } diff --git a/js/one-dimension/view/MassNode1D.js b/js/one-dimension/view/MassNode1D.js index f2fd501..d05f58f 100644 --- a/js/one-dimension/view/MassNode1D.js +++ b/js/one-dimension/view/MassNode1D.js @@ -88,9 +88,11 @@ define( require => { } ); this.addInputListener( this.dragListener ); + // unlink is unnecessary, the MassNode1D and the depencency exist for the lifetime of the sim this.model.arrowsVisibilityProperty.link( arrowsVisible => { const callback = this.overUpCallback.bind( this ); if ( arrowsVisible ) { + // unlink is needed when the arrows become invisible this.dragListener.isOverProperty.link( callback ); } else { diff --git a/js/one-dimension/view/NormalModeSpectrumAccordionBox.js b/js/one-dimension/view/NormalModeSpectrumAccordionBox.js index 3963a78..c9421a4 100644 --- a/js/one-dimension/view/NormalModeSpectrumAccordionBox.js +++ b/js/one-dimension/view/NormalModeSpectrumAccordionBox.js @@ -210,6 +210,7 @@ define( require => { super( contentNode, options ); + // unlink is unnecessary, exists for the lifetime of the sim model.phasesVisibilityProperty.link( phasesVisibility => { for ( let i = 1; i < panelColumns.length; ++i ) { const j = i - 1; @@ -240,6 +241,7 @@ define( require => { this.bottom = options.bottom; } ); + // unlink is unnecessary, exists for the lifetime of the sim model.numVisibleMassesProperty.link( numMasses => { for ( let i = 0; i < numMasses; i++ ) { const k = OneDimensionConstants.SPRING_CONSTANT_VALUE; diff --git a/js/one-dimension/view/NormalModesAccordionBox.js b/js/one-dimension/view/NormalModesAccordionBox.js index e9b262c..4feede9 100644 --- a/js/one-dimension/view/NormalModesAccordionBox.js +++ b/js/one-dimension/view/NormalModesAccordionBox.js @@ -76,6 +76,7 @@ define( require => { for ( let i = 0; i < normalModeGraphs.length; i++ ) { normalModeGraphs[ i ] = new ModeGraphCanvasNode( model, i ); + // dispose is unnecessary, exists for the lifetime of the sim Property.multilink( [ model.timeProperty, model.modeAmplitudeProperty[ i ], model.modePhaseProperty[ i ] ], function( time, amp, phase ) { normalModeGraphs[ i ].update(); } ); @@ -89,6 +90,7 @@ define( require => { super( graphContainer, options ); + // dispose is unnecessary, exists for the lifetime of the sim Property.multilink( [ model.numVisibleMassesProperty, this.expandedProperty ], ( numMasses, isExpanded ) => { graphContainer.children = normalModeGraphs.slice( 0, numMasses ); graphContainer.children.forEach( graph => graph.update() ); diff --git a/js/one-dimension/view/WallNode.js b/js/one-dimension/view/WallNode.js index ccc5c53..892abec 100644 --- a/js/one-dimension/view/WallNode.js +++ b/js/one-dimension/view/WallNode.js @@ -44,6 +44,7 @@ define( require => { } ); this.addChild( this.rect ); + // dispose is unnecessary, the WallNode and the dependencies exist for the lifetime of the sim Property.multilink( [ this.mass.equilibriumPositionProperty, this.mass.displacementProperty ], ( massPosition, massDisplacement ) => { this.translation = this.modelViewTransform.modelToViewPosition( massPosition.plus( massDisplacement ) ).subtract( new Vector2( this.rect.rectWidth / 2, this.rect.rectHeight / 2 ) ); } ); diff --git a/js/two-dimensions/model/TwoDimensionsModel.js b/js/two-dimensions/model/TwoDimensionsModel.js index d3fbdfb..c443a47 100644 --- a/js/two-dimensions/model/TwoDimensionsModel.js +++ b/js/two-dimensions/model/TwoDimensionsModel.js @@ -104,6 +104,7 @@ define( require => { range: new Range( TwoDimensionsConstants.MIN_MODE_PHASE, TwoDimensionsConstants.MAX_MODE_PHASE ) } ); + // dispose is unnecessary, since this class owns the dependency this.modeFrequencyProperty[ i ][ j ] = new DerivedProperty( [ this.numVisibleMassesProperty ], function( numMasses ) { const k = TwoDimensionsConstants.SPRING_CONSTANT_VALUE; const m = TwoDimensionsConstants.MASSES_MASS_VALUE; @@ -135,8 +136,6 @@ define( require => { } this.createDefaultSprings(); - this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) ); - // @public {Property.} the indexes of the mass being dragged (an object with and 'i' and a 'j') this.draggingMassIndexesProperty = new Property( null, { tandem: tandem.createTandem( 'draggingMassIndexesProperty' ) @@ -151,6 +150,9 @@ define( require => { this.amplitudeDirectionProperty = new EnumerationProperty( AmplitudeDirection, AmplitudeDirection.VERTICAL, { tandem: tandem.createTandem( 'amplitudeDirectionProperty' ) } ); + + // unlink is unnecessary, exists for the lifetime of the sim + this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) ); } /** @@ -238,6 +240,7 @@ define( require => { //TODO see https://github.com/phetsims/normal-modes/issues/17 // const massTandem = tandem.createTandem( 'mass' + i + '_' + j ); const massTandem = Tandem.OPTIONAL; + // All the masses needed are created at once, and exist for the lifetime of the sim this.masses[ i ][ j ] = new Mass( new Vector2( x, y ), visible, massTandem ); if ( x < xFinal - xStep / 2 ) { @@ -260,6 +263,7 @@ define( require => { for ( let i = 0; i < MAX_SPRINGS; i++ ) { for ( let j = 0; j < MAX_SPRINGS; ++j ) { + // All the springs needed are created at once, and exist for the lifetime of the sim if ( i !== MAX_SPRINGS - 1 ) { this.springsX[ i ][ j ] = new Spring( this.masses[ i + 1 ][ j ], this.masses[ i + 1 ][ j + 1 ] ); } diff --git a/js/two-dimensions/view/AmplitudeSelectorRectangle.js b/js/two-dimensions/view/AmplitudeSelectorRectangle.js index 144e205..95f0d92 100644 --- a/js/two-dimensions/view/AmplitudeSelectorRectangle.js +++ b/js/two-dimensions/view/AmplitudeSelectorRectangle.js @@ -103,15 +103,19 @@ define( require => { this.amplitudeChanged( axisAmplitudesProperty.get()[ row ][ col ].get(), amplitudeDirection ); }; + // unlink is unnecessary, exists for the lifetime of the sim model.modeXAmplitudeProperty[ row ][ col ].link( amplitude => { this.amplitudeChanged( amplitude, AmplitudeDirection.HORIZONTAL ); } ); + // unlink is unnecessary, exists for the lifetime of the sim model.modeYAmplitudeProperty[ row ][ col ].link( amplitude => { this.amplitudeChanged( amplitude, AmplitudeDirection.VERTICAL ); } ); + // unlink is unnecessary, exists for the lifetime of the sim model.numVisibleMassesProperty.link( this.numMassesChanged ); + // unlink is unnecessary, exists for the lifetime of the sim model.amplitudeDirectionProperty.link( this.amplitudeDirectionChanged ); const isNear = function( n1, n2 ) { diff --git a/js/two-dimensions/view/MassNode2D.js b/js/two-dimensions/view/MassNode2D.js index c4a2c40..9b3526a 100644 --- a/js/two-dimensions/view/MassNode2D.js +++ b/js/two-dimensions/view/MassNode2D.js @@ -88,9 +88,11 @@ define( require => { } ); this.addInputListener( this.dragListener ); + // unlink is unnecessary, the MassNode2D and the depencency exist for the lifetime of the sim this.model.arrowsVisibilityProperty.link( arrowsVisible => { const callback = this.overUpCallback.bind( this ); if ( arrowsVisible ) { + // unlink is needed when the arrows become invisible this.dragListener.isOverProperty.link( callback ); } else { diff --git a/js/two-dimensions/view/NormalModeAmplitudesAccordionBox.js b/js/two-dimensions/view/NormalModeAmplitudesAccordionBox.js index 48c0b31..a4a3ae7 100644 --- a/js/two-dimensions/view/NormalModeAmplitudesAccordionBox.js +++ b/js/two-dimensions/view/NormalModeAmplitudesAccordionBox.js @@ -79,14 +79,17 @@ define( require => { const amplitudeDirectionRadioButtonGroup = new AmplitudeDirectionRadioButtonGroup( model.amplitudeDirectionProperty ); + // dispose is unnecessary, exists for the lifetime of the sim const axisAmplitudesProperty = new DerivedProperty( [ model.amplitudeDirectionProperty ], amplitudeDirection => { return ( amplitudeDirection === AmplitudeDirection.VERTICAL ) ? model.modeYAmplitudeProperty : model.modeXAmplitudeProperty; } ); + // dispose is unnecessary, exists for the lifetime of the sim const maxAmpProperty = new DerivedProperty( [ model.numVisibleMassesProperty ], numMasses => { return TwoDimensionsConstants.MAX_MODE_AMPLITUDE[ numMasses - 1 ]; } ); + // dispose is unnecessary, exists for the lifetime of the sim const gridSizeProperty = new DerivedProperty( [ model.numVisibleMassesProperty ], numMasses => { return PANEL_SIZE / ( 1 + ( RECT_GRID_UNITS + PADDING_GRID_UNITS ) * numMasses ); } );