From 7eb4b9f25f403b8f8fa993775228a18fac53328c Mon Sep 17 00:00:00 2001 From: Jonathan Olson Date: Tue, 13 Mar 2018 14:38:12 -0600 Subject: [PATCH] More review notes, see https://github.com/phetsims/masses-and-springs/issues/253 --- js/common/model/MassesAndSpringsModel.js | 1 + js/common/view/OneSpringScreenView.js | 2 +- js/common/view/SpringScreenView.js | 24 ++++++++++++++++++++++-- js/common/view/ToolboxPanel.js | 4 ++++ js/common/view/TwoSpringScreenView.js | 3 ++- js/lab/view/PeriodTraceNode.js | 8 +++++--- 6 files changed, 35 insertions(+), 7 deletions(-) diff --git a/js/common/model/MassesAndSpringsModel.js b/js/common/model/MassesAndSpringsModel.js index 7ee9108a..e58bd9e3 100644 --- a/js/common/model/MassesAndSpringsModel.js +++ b/js/common/model/MassesAndSpringsModel.js @@ -50,6 +50,7 @@ define( function( require ) { } ); //@public {Property.} determines whether the sim is playing sound + //REVIEW: Looks like nothing is listening to this? Can it be removed? this.isSoundEnabledProperty = new BooleanProperty( true, { tandem: tandem.createTandem( 'isSoundEnabledProperty' ) } ); diff --git a/js/common/view/OneSpringScreenView.js b/js/common/view/OneSpringScreenView.js index d034bfba..79c755a1 100644 --- a/js/common/view/OneSpringScreenView.js +++ b/js/common/view/OneSpringScreenView.js @@ -43,7 +43,7 @@ define( function( require ) { */ function OneSpringScreenView( model, tandem ) { //REVIEW: JSDOc - this.model = model; // Make model available + this.model = model; // Make model available REVIEW: This is already done in the supertype, not necessary here SpringScreenView.call( this, model, tandem ); var self = this; diff --git a/js/common/view/SpringScreenView.js b/js/common/view/SpringScreenView.js index 610fd972..30ef5086 100644 --- a/js/common/view/SpringScreenView.js +++ b/js/common/view/SpringScreenView.js @@ -70,13 +70,16 @@ define( function( require ) { // @protected {number} - Spacing used for the margin of layout bounds this.spacing = 10; - // @protected {number} - Alignment for panels on most right side of sim view + // @protected {number} - Alignment for panels on most right side of sim view REVIEW: 'alignment' is usually like right/left/center. This is a padding between the right side of the screen and the panels, correct? this.rightPanelAlignment = this.visibleBoundsProperty.get().right - this.spacing; // Add masses + //REVIEW: JSDoc this.massLayer = new Node( { tandem: tandem.createTandem( 'massLayer' ), preventFit: true } ); + //REVIEW: JSDoc this.massNodes = []; + //REVIEW: Could use `this.massNodes = model.masses.map( ... )`, returning the mass node at the end of the callback? model.masses.forEach( function( mass ) { var massNode = new MassNode( mass, @@ -91,6 +94,7 @@ define( function( require ) { } ); // Add shelf for to house massNodes + //REVIEW: JSDoc this.shelf = new Shelf( tandem, { rectHeight: 7 } ); @@ -99,6 +103,7 @@ define( function( require ) { this.addChild( this.shelf ); // Timer and Ruler + //REVIEW: JSDoc this.timerNode = new DraggableTimerNode( this.visibleBoundsProperty.get(), Vector2.ZERO, @@ -107,6 +112,7 @@ define( function( require ) { model.timerVisibleProperty, tandem.createTandem( 'timerNode' ) ); + //REVIEW: JSDoc this.rulerNode = new DraggableRulerNode( this.modelViewTransform, this.visibleBoundsProperty.get(), @@ -116,6 +122,7 @@ define( function( require ) { ); // Create specific layer for tools so they don't overlap the reset all button. + //REVIEW: JSDoc this.toolsLayer = new Node( { children: [ this.timerNode, this.rulerNode ], tandem: tandem.createTandem( 'massLayer' ), @@ -123,6 +130,7 @@ define( function( require ) { } ); // Reset All button + //REVIEW: JSDoc this.resetAllButton = new ResetAllButton( { listener: function() { model.reset(); @@ -136,15 +144,18 @@ define( function( require ) { } ); // Sim speed controls + //REVIEW: JSDoc this.timeControlPanel = new TimeControlNode( model, this.visibleBoundsProperty.get(), tandem.createTandem( 'timeControlPanel' ), { + //REVIEW: Another 0.02 constant to factor out bottom: this.modelViewTransform.modelToViewY( MassesAndSpringsConstants.FLOOR_Y + 0.02 ) } ); // sound toggle button at bottom right + //REVIEW: This is never added. Can it just be removed? Why do we have an isSoundEnabledProperty? var soundToggleButton = new SoundToggleButton( model.isSoundEnabledProperty, { centerY: this.resetAllButton.centerY - 55, scale: 0.9 @@ -152,8 +163,11 @@ define( function( require ) { // this.addChild( soundToggleButton ); // Toolbox Panel + //REVIEW: JSDoc this.toolboxPanel = new ToolboxPanel( this.visibleBoundsProperty.get(), + //REVIEW: Usually we wouldn't need to pass in the rulerNode/TimerNode (and we wouldn't need to set the toolbox property on the nodes below). + //REVIEW: We can collaborate to remove this dependency? this.rulerNode, this.timerNode, model.rulerVisibleProperty, model.timerVisibleProperty, @@ -165,9 +179,13 @@ define( function( require ) { // Adding tools in toolbox this.addChild( this.toolboxPanel ); + //REVIEW: This can be removed presumably if the above is done? this.timerNode.toolbox = this.toolboxPanel; this.rulerNode.toolbox = this.toolboxPanel; + //REVIEW: It seems like there are listeners similar to this on subtypes. It's better to create a layout() method + //REVIEW: on the screenview and override/extend as necessary, as you are unintentionally depending on this listener + //REVIEW: being added before the other visibleBoundsProperty listeners are added (for layout) this.visibleBoundsProperty.link( function( visibleBounds ) { //Update the bounds of view elements @@ -189,6 +207,8 @@ define( function( require ) { massNode.moveToFront(); } ); }, + + //REVIEW: This can be removed? // // /** // * Responsible for making sure the resetAllButton is in front of the scene graph at all times. @@ -218,7 +238,7 @@ define( function( require ) { * Creates a panel that controls the designated spring's spring constant value. * * @param {number} springIndex - * @param {array.} labels + * @param {Array.} labels * @param {Tandem} tandem * @returns {SpringControlPanel} */ diff --git a/js/common/view/ToolboxPanel.js b/js/common/view/ToolboxPanel.js index b84c6b51..bb139639 100644 --- a/js/common/view/ToolboxPanel.js +++ b/js/common/view/ToolboxPanel.js @@ -100,6 +100,8 @@ define( function( require ) { var rulerUnboundedPosition = new Vector2(); // Drag listener for event forwarding: rulerIcon ---> rulerNode + //REVIEW: Forwarding should generally use SimpleDragHandler.createForwardingListener. I'll be available to help with it. + //REVIEW: Definitely don't need a full listener here, particularly a drag listener. self.rulerIcon.addInputListener( new MovableDragHandler( rulerNode.positionProperty, { // allow moving a finger (on a touchscreen) dragged across this node to interact with it @@ -140,6 +142,8 @@ define( function( require ) { } ); // Drag listener for event forwarding: timerIcon ---> timerNode + //REVIEW: Forwarding should generally use SimpleDragHandler.createForwardingListener. I'll be available to help with it. + //REVIEW: Definitely don't need a full listener here, particularly a drag listener. self.timerIcon.addInputListener( new MovableDragHandler( timerNode.positionProperty, { // allow moving a finger (on a touchscreen) dragged across this node to interact with it diff --git a/js/common/view/TwoSpringScreenView.js b/js/common/view/TwoSpringScreenView.js index f2b2bb67..fc6a5e6b 100644 --- a/js/common/view/TwoSpringScreenView.js +++ b/js/common/view/TwoSpringScreenView.js @@ -36,7 +36,8 @@ define( function( require ) { * @constructor */ function TwoSpringScreenView( model, tandem ) { - this.model = model; // Make model available + //REVIEW: JSDoc + this.model = model; // Make model available REVIEW: This is already done in the supertype, not necessary here SpringScreenView.call( this, model, tandem ); var self = this; diff --git a/js/lab/view/PeriodTraceNode.js b/js/lab/view/PeriodTraceNode.js index 98f3ee56..4d0177b7 100644 --- a/js/lab/view/PeriodTraceNode.js +++ b/js/lab/view/PeriodTraceNode.js @@ -22,22 +22,23 @@ define( function( require ) { /** * @constructor + * REVIEW: params */ function PeriodTraceNode( periodTrace, modelViewTransform ) { Node.call( this ); var self = this; - // @protected + // @protected PREVIEW: protected? what inherits from this? this.periodTrace = periodTrace; // @private {number} - The opacity of the trace (not using Node opacity for performance reasons) this.colorAlpha = 1; - // @protected + // @protected PREVIEW: protected? what inherits from this? this.traceColor = new Color( 'black' ); - // @protected + // @protected PREVIEW: protected? what inherits from this? this.modelViewTransform = modelViewTransform; this.originalX = this.modelViewTransform.modelToViewX( this.periodTrace.springProperty.value.positionProperty.value.x - 0.2 ); @@ -72,6 +73,7 @@ define( function( require ) { }, // TODO:documentation + //REVIEW: Looks private. Also since it's just called from step, it COULD be inlined. At least remove the passing in of the MVT and use this.modelViewTransform? (Same for the spring?) updateTrace: function( spring, modelViewTransform ) { var mass = spring.massAttachedProperty.value;