Skip to content

Commit

Permalink
More review notes, see #253
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanolson committed Mar 13, 2018
1 parent 7eb4b9f commit 59d31bc
Show file tree
Hide file tree
Showing 11 changed files with 62 additions and 12 deletions.
1 change: 1 addition & 0 deletions js/common/view/OneSpringScreenView.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ define( function( require ) {
model.naturalLengthVisibleProperty,
tandem,
{ modelViewTransform: this.modelViewTransform } );
//REVIEW: Just pass these options into the DisplacementArrowNode options? ( e.g. left: ... )
displacementArrowNode.left = this.springNodes[ 0 ].right + 12;
displacementArrowNode.centerY = this.modelViewTransform.modelToViewY( this.springNodes[ 0 ].spring.bottomProperty.value );

Expand Down
1 change: 1 addition & 0 deletions js/common/view/TimeControlNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ define( function( require ) {
]
} );

//REVIEW: Do these need precise vertical alignment, or can we wrap these in an HBox (TimeControlNode could inherit HBox?)
timeSpeedRadioNode.left = timeControlHBox.right + 40;
this.addChild( timeControlHBox );
this.addChild( timeSpeedRadioNode );
Expand Down
16 changes: 16 additions & 0 deletions js/common/view/ToolboxPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ define( function( require ) {
cornerRadius: MassesAndSpringsConstants.PANEL_CORNER_RADIUS
}, options );

//REVIEW: JSDoc
//REVIEW: The only usage I see of this is it being set to a {number} (constant 3) in TwoSpringScreenView. Can this be removed?
this.dragBounds = options.dragBounds;

var self = this;
Expand Down Expand Up @@ -82,10 +84,17 @@ define( function( require ) {
{ tandem: tandem.createTandem( 'ruler' ) } );
ruler.rotate( 40, false );

//REVIEW: Since toImage is asynchronous, it looks like the ruler/timer callbacks could finish in any order. This could
//REVIEW: mean that sometimes when the sim loads, the timer will be on the other side of the ruler.
//REVIEW: Node.toDataURLNodeSynchronous could be used instead (potentially), so things can be removed from the callbacks.

// Create timer icon
ruler.toImage( function( image ) {

// @private - visible option is used only for reset() in ToolboxPanel.js
//REVIEW: Not great form to declare properties inside callbacks. Hopefully when this is de-callbacked (see
//REVIEW above notes), this will be fine. Also JSDoc the type.
//REVIEW: Wait, why is this set as a property? Should be able to use a local variable instead?
self.rulerIcon = new Image( image, {
// Instead of changing the rendering, we'll dynamically generate a mipmap so that the ruler icon appearance looks better.
// See https://github.com/phetsims/masses-and-springs/issues/199.
Expand All @@ -111,7 +120,9 @@ define( function( require ) {

startDrag: function( event ) {
// Toggle visibility
//REVIEW: Why would this be set to true here?
rulerVisibleProperty.set( true );
//REVIEW: This looks like a potential memory leak, since every startDrag will add a listener that never goes away
rulerVisibleProperty.link( function( visible ) {
self.rulerIcon.visible = !visible;
} );
Expand All @@ -134,6 +145,9 @@ define( function( require ) {
timer.toImage( function( image ) {

// @private - visible option is used only for reset() in ToolboxPanel.js
//REVIEW: Not great form to declare properties inside callbacks. Hopefully when this is de-callbacked (see
//REVIEW above notes), this will be fine. Also JSDoc the type.
//REVIEW: Wait, why is this set as a property? Should be able to use a local variable instead?
self.timerIcon = new Image( image, {
cursor: 'pointer',
pickable: true,
Expand All @@ -153,7 +167,9 @@ define( function( require ) {

startDrag: function( event ) {
// Toggle visibility
//REVIEW: Why would this be set to true here?
timerVisibleProperty.set( true );
//REVIEW: This looks like a potential memory leak, since every startDrag will add a listener that never goes away
timerVisibleProperty.link( function( visible ) {
self.timerIcon.visible = !visible;
} );
Expand Down
16 changes: 14 additions & 2 deletions js/common/view/TwoSpringScreenView.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2016-2018, University of Colorado Boulder

/**
* Common ScreenView for using two masses.
* Common ScreenView for using two masses.
*
* @author Matt Pennington (PhET Interactive Simulations)
* @author Denzell Barnett (PhET Interactive Simulations)
Expand Down Expand Up @@ -45,6 +45,7 @@ define( function( require ) {
model.springs[ 1 ].options.modelViewTransform2 = this.modelViewTransform;

// Spring Hanger Node
//REVIEW: JSDoc
this.springHangerNode = new SpringHangerNode(
model.springs,
this.modelViewTransform,
Expand All @@ -55,13 +56,17 @@ define( function( require ) {


var leftSpring = this.model.springs[ 0 ];
//REVIEW: JSDoc
this.firstSpringStopperButtonNode = this.createStopperButton( leftSpring, tandem );
this.firstSpringStopperButtonNode.right = this.springHangerNode.left - this.spacing;

var rightSpring = this.model.springs[ 1 ];
//REVIEW: JSDoc
this.secondSpringStopperButtonNode = this.createStopperButton( rightSpring, tandem );
this.secondSpringStopperButtonNode.left = this.springHangerNode.right + this.spacing;

//REVIEW: Not sure why these two properties are linked together. This looks like it could be handled with two
//REVIEW: separate link statements?
Property.multilink( [ leftSpring.buttonEnabledProperty, rightSpring.buttonEnabledProperty ],
function( leftButtonEnabled, rightButtonEnabled ) {
self.firstSpringStopperButtonNode.enabled = leftButtonEnabled;
Expand All @@ -73,15 +78,18 @@ define( function( require ) {
new Text( smallString, { font: MassesAndSpringsConstants.LABEL_FONT } ),
new Text( largeString, { font: MassesAndSpringsConstants.LABEL_FONT } )
];
//REVIEW: JSDoc
this.firstSpringConstantControlPanel = this.createSpringConstantPanel( 0, minMaxLabels, tandem );
this.firstSpringConstantControlPanel.right = this.firstSpringStopperButtonNode.left - this.spacing;

//REVIEW: JSDoc
this.secondSpringConstantControlPanel = this.createSpringConstantPanel( 1, minMaxLabels, tandem );
this.secondSpringConstantControlPanel.left = this.secondSpringStopperButtonNode.right + this.spacing;

// Initializes red movable reference line
var xBoundsLimit = this.springHangerNode.centerX + 5;
var movableLineNode = new MovableLineNode(
//REVIEW: If it's easier, you can use .center instead of .getCenter(), as it's slightly shorter
this.visibleBoundsProperty.get().getCenter().minus( new Vector2( 45, 0 ) ),
210,
model.movableLineVisibleProperty,
Expand All @@ -90,6 +98,8 @@ define( function( require ) {
);

// @public Initializes natural line for first spring
//REVIEW: Don't need visibility doc for local variable
//REVIEW: Lots of duplication with construction of these. Can we simplify?
var firstNaturalLengthLineNode = new ReferenceLineNode(
this.modelViewTransform,
model.springs[ 0 ],
Expand All @@ -101,6 +111,7 @@ define( function( require ) {
);

// @public Initializes natural line for second spring
//REVIEW: Don't need visibility doc for local variable
var secondNaturalLengthLineNode = new ReferenceLineNode(
this.modelViewTransform,
model.springs[ 1 ],
Expand All @@ -112,6 +123,7 @@ define( function( require ) {
);

// Gravity Control Panel
//REVIEW: JSDoc
this.gravityAndDampingControlNode = new GravityAndDampingControlNode(
model, this, tandem.createTandem( 'gravityAndDampingControlNode' ),
{
Expand Down Expand Up @@ -158,7 +170,7 @@ define( function( require ) {
self.toolboxPanel.right = self.panelRightSpacing;
self.resetAllButton.right = self.panelRightSpacing;
self.timeControlPanel.right = self.resetAllButton.left - self.spacing * 6;
self.toolboxPanel.dragBounds = 3;
self.toolboxPanel.dragBounds = 3; //REVIEW: Is this setting a {number} to a {Bounds2}?
self.timerNode.updateBounds( visibleBounds.withOffsets(
self.timerNode.width / 2, self.timerNode.height / 2, -self.timerNode.width / 2, -self.timerNode.height / 2
) );
Expand Down
2 changes: 1 addition & 1 deletion js/intro/view/ConstantsControlPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ define( function( require ) {
* @param {Property.<string>} selectedConstantProperty determines which value to hold constant, values are 'spring-constant' and 'spring-thickness'
* @param {string} title: string used to title the panel
* @param {tandem} tandem
* @param {Object} options
* @param {Object} [options]
* @constructor
*/
function ConstantsControlPanel( selectedConstantProperty, title, tandem, options ) {
Expand Down
3 changes: 3 additions & 0 deletions js/intro/view/IntroScreenView.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ define( function( require ) {
new Text( shortString, { font: MassesAndSpringsConstants.LABEL_FONT } ),
new Text( longString, { font: MassesAndSpringsConstants.LABEL_FONT } )
];

//REVIEW: JSDoc
this.springLengthControlPanel = new SpringControlPanel(
model.spring1.naturalRestingLengthProperty,
//REVIEW: Only uses {Range} parts, so why specify a RangeWithValue?
Expand All @@ -79,6 +81,7 @@ define( function( require ) {
);

// @private panel that keeps thickness/spring constant at constant value
//REVIEW: JSDoc type
this.constantsControlPanel = new ConstantsControlPanel(
model.constantParameterProperty,
constantParameterString,
Expand Down
10 changes: 6 additions & 4 deletions js/intro/view/SceneSelectionButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ define( function( require ) {
new Vector2( 0.65, 2.1 ),
MassesAndSpringsConstants.DEFAULT_SPRING_LENGTH,
0,
new Property(0),
new Property(0), //REVIEW: NumberProperty? (Really don't care here, but it is the code style)
tandem.createTandem( 'firstIconSpring' )
),
new Spring(
new Vector2( 0.85, 2.1 ),
MassesAndSpringsConstants.DEFAULT_SPRING_LENGTH,
0,
new Property(0),
new Property(0), //REVIEW: NumberProperty? (Really don't care here, but it is the code style)
tandem.createTandem( 'secondIconSpring' )
)
];
Expand All @@ -69,6 +69,7 @@ define( function( require ) {
mvt,
tandem.createTandem( 'firstSpringIcon' ),
springNodeOptions );
//REVIEW: Can this be in the options?
firstSpringIcon.loopsProperty.set( 6 );
firstSpringIcon.lineWidthProperty.set( 3 );

Expand All @@ -80,6 +81,7 @@ define( function( require ) {
tandem.createTandem( 'secondSpringIcon' ),
springNodeOptions
);
//REVIEW: Can this be in the options?
secondSpringIcon.loopsProperty.set( 6 );
secondSpringIcon.lineWidthProperty.set( 3 );

Expand All @@ -100,15 +102,15 @@ define( function( require ) {
lineWidth: 4,
centerX: firstSpringIcon.centerX,
top: firstSpringIcon.top,
opacity: this.opacity
opacity: this.opacity // REVIEW: Nested opacity, so the true opacity here is 0.9 * 0.9? Is that desired?
} );

var secondVerticalLineNode = new Line( 60, 0, 0, 0, {
stroke: 'black',
lineWidth: 4,
centerX: secondSpringIcon.centerX,
top: secondSpringIcon.top,
opacity: this.opacity
opacity: this.opacity // REVIEW: Nested opacity, so the true opacity here is 0.9 * 0.9? Is that desired?
} );

//REVIEW: Local variables shouldn't have visibility docs, and don't need read-only docs.
Expand Down
12 changes: 8 additions & 4 deletions js/vectors/view/IndicatorVisibilityControlNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ define( function( require ) {
* @param {Tandem} tandem
* @returns {Line} line object with passed in color.
*/
//REVIEW: Looks suspiciously duplicated from LineVisibilityNode. Can/should some code be factored out?
var createLine = function( color, tandem ) {
return new Line( 0, 0, LINE_LENGTH, 0, {
stroke: color,
Expand All @@ -66,19 +67,22 @@ define( function( require ) {
};

// Lines added for reference in panel
//REVIEW: Looks suspiciously duplicated from LineVisibilityNode. Can/should some code be factored out?
var blackLine = createLine( 'black', tandem.createTandem( 'blackLine' ) );
var blueLine = createLine( 'rgb( 65, 66, 232 )', tandem.createTandem( 'blueLine' ) );
var redLine = createLine( 'red', tandem.createTandem( 'redLine' ) );
var grayLine = createLine( '#e6e6e6' ); // TODO: we shouldn't need this for spacing on lab screen.
var displacementSymbol = new DisplacementArrowNode(
new Property( 10 ),
new Property( true ),
new Property( 10 ), //REVIEW: NumberProperty?
new Property( true ), //REVIEW: BooleanProperty?
//REVIEW: Or... can we maybe make a dev meeting note to see if we can just leave "never-changing" Properties like this as Property instead of using subtypes?
tandem,
{
modelViewTransform: this.modelViewTransform,
symbolRepresentation: true,
}
);
//REVIEW: Just pass these options into the DisplacementArrowNode options? ( e.g. scale: ... )
displacementSymbol.scale( .65 );

// Labels for the displacement arrow and natural length line
Expand Down Expand Up @@ -158,7 +162,7 @@ define( function( require ) {
tandem: tandem.createTandem( 'indicatorVisibilityCheckboxGroup' )
} );
}
var titleToControlsVerticalSpace = 2;
var titleToControlsVerticalSpace = 2; //REVIEW: Can this be inlined?
var indicatorVisibilityControlsVBox = new VBox( {
children: [
new VStrut( titleToControlsVerticalSpace ),
Expand Down Expand Up @@ -197,7 +201,7 @@ define( function( require ) {
var controlBox = new HBox( {
children: [
indicatorVisibilityControlsVBox,
new HStrut( 25 ),
new HStrut( 25 ), //REVIEW: Use spacing instead?
lineVBox,
]
} );
Expand Down
3 changes: 3 additions & 0 deletions js/vectors/view/LineArrowNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ define( function( require ) {
*/
function LineArrowNode( tailX, tailY, tipX, tipY, options ) {

//REVIEW: This looks like a duplicated form of LineArrowNode from hookes-law, and it should be moved to common code.
//REVIEW: See TODO in the header above.

// default options
options = _.extend( {
headHeight: 10,
Expand Down
4 changes: 3 additions & 1 deletion js/vectors/view/VectorVisibilityControlNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ define( function( require ) {

Node.call( this, options );

// REVIEW: This has some duplication with MassNode. Is it something that can be factored out?
var velocityArrow = new VectorArrow( MassesAndSpringsConstants.VELOCITY_ARROW_COLOR );
var accelerationArrow = new VectorArrow( MassesAndSpringsConstants.ACCELERATION_ARROW_COLOR );
var gravityArrow = new ForceVectorArrow( MassesAndSpringsConstants.GRAVITY_ARROW_COLOR );
Expand Down Expand Up @@ -181,6 +182,7 @@ define( function( require ) {
else {
vectorVisibilityControlsVBox = new VBox( {
children: [
//REVIEW: This is put into an HBox with centers vertically. Why would VStruts of equal size be needed?
new VStrut( 2 ),
vectorVisibilityCheckboxGroup,
new VStrut( 2 )
Expand All @@ -201,7 +203,7 @@ define( function( require ) {
var controlsHBox = new HBox( {
children: [
vectorVisibilityControlsVBox,
new HStrut( 40 ),
new HStrut( 40 ), //REVIEW: Use spacing?
vectorVBox,
]
}
Expand Down
6 changes: 6 additions & 0 deletions js/vectors/view/VectorsScreenView.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ define( function( require ) {
model.naturalLengthVisibleProperty,
tandem,
{ modelViewTransform: this.modelViewTransform } );
//REVIEW: Just pass these options into the DisplacementArrowNode options? ( e.g. left: ... )
firstDisplacementArrowNode.left = this.springNodes[ 0 ].right + 8;
firstDisplacementArrowNode.centerY = this.modelViewTransform.modelToViewY( this.springNodes[ 0 ].spring.bottomProperty.value );
this.addChild( firstDisplacementArrowNode );
Expand All @@ -53,13 +54,15 @@ define( function( require ) {
model.naturalLengthVisibleProperty,
tandem,
{ modelViewTransform: this.modelViewTransform } );
//REVIEW: Just pass these options into the DisplacementArrowNode options? ( e.g. left: ... )
secondDisplacementArrowNode.right = this.springNodes[ 1 ].left + 14;
secondDisplacementArrowNode.centerY = this.modelViewTransform.modelToViewY( this.springNodes[ 1 ].spring.bottomProperty.value );
this.addChild( secondDisplacementArrowNode );

// Equilibrium of mass is dependent on the mass being attached and the visibility of the equilibrium line.
var firstMassEquilibriumVisibilityProperty = new DerivedProperty( [ model.equilibriumPositionVisibleProperty, model.springs[ 0 ].massAttachedProperty ],
function( equilibriumPositionVisible, massAttached ) {
//REVIEW: return massAttached && equilibriumPositionVisible;
if ( massAttached ) {
return equilibriumPositionVisible;
}
Expand All @@ -69,13 +72,15 @@ define( function( require ) {
} );
var secondMassEquilibriumVisibilityProperty = new DerivedProperty( [ model.equilibriumPositionVisibleProperty, model.springs[ 1 ].massAttachedProperty ],
function( equilibriumPositionVisible, massAttached ) {
//REVIEW: return massAttached && equilibriumPositionVisible;
if ( massAttached ) {
return equilibriumPositionVisible;
}
else {
return false;
}
} );
//REVIEW: Some duplication for things for each spring. Is there an easy/moderate way to remove this? If not no worries

// Initializes equilibrium line for first spring
var firstSpringEquilibriumLineNode = new ReferenceLineNode(
Expand Down Expand Up @@ -172,6 +177,7 @@ define( function( require ) {
} );

// Determines where we want the force vectors of the attached mass to be placed.
//REVIEW: Shouldn't directly change spring options. Also, can we do this on spring construction instead?
model.springs[ 0 ].options.forcesOrientation = -1;
model.springs[ 1 ].options.forcesOrientation = 1;
}
Expand Down

0 comments on commit 59d31bc

Please sign in to comment.