Skip to content

Commit

Permalink
Changing TODOs from #123 to #317
Browse files Browse the repository at this point in the history
  • Loading branch information
AgustinVallejo committed Aug 6, 2024
1 parent a06bf40 commit 96c540a
Show file tree
Hide file tree
Showing 35 changed files with 64 additions and 64 deletions.
2 changes: 1 addition & 1 deletion js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export default class BuoyancyBasicsExploreScreenView extends BuoyancyScreenView<
ManualConstraint.create( this, [ this.resetAllButton, blocksModeRadioButtonGroup ],
( resetAllButtonWrapper, blocksModeRadioButtonGroupWrapper ) => {

// TODO: What if the reset all button is not visible due to phet-io? See https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: What if the reset all button is not visible due to phet-io? See https://github.com/phetsims/density-buoyancy-common/issues/317
blocksModeRadioButtonGroupWrapper.right = resetAllButtonWrapper.left - DensityBuoyancyCommonConstants.MARGIN;
blocksModeRadioButtonGroupWrapper.bottom = resetAllButtonWrapper.bottom;
} );
Expand Down
2 changes: 1 addition & 1 deletion js/buoyancy/model/BuoyancyExploreModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export default class BuoyancyExploreModel extends DensityBuoyancyModel {
this.massB = Cube.createWithMass( this.engine, Material.ALUMINUM, new Vector2( 0.05, 0.35 ), 13.5, {
tag: MassTag.OBJECT_B,

// TODO: Tandem doesn't match variable name, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Tandem doesn't match variable name, see https://github.com/phetsims/density-buoyancy-common/issues/317
tandem: blocksTandem.createTandem( 'blockB' ),
availableMassMaterials: availableMassMaterials,
visible: false
Expand Down
4 changes: 2 additions & 2 deletions js/buoyancy/model/applications/ApplicationsMass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@ export default abstract class ApplicationsMass extends Mass {
*
* Assumes step information was updated.
*
* TODO: Why is this different than getDisplacedVolume? Should they share implementation? See https://github.com/phetsims/density-buoyancy-common/issues/123
* TODO: Why is this different than getDisplacedVolume? Should they share implementation? See https://github.com/phetsims/density-buoyancy-common/issues/317
*/
public getDisplacedArea( fluidLevel: number ): number {
const bottom = this.stepBottom;
const top = this.stepTop;

// TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 if the fluid level is beyond the top, it probably shouldn't be 0, right?
// TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 if the fluid level is beyond the top, it probably shouldn't be 0, right?
if ( fluidLevel < bottom || fluidLevel > top ) {
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion js/buoyancy/model/applications/Boat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ export default class Boat extends ApplicationsMass {
*/
private setSubmergedState( fluidLevel: number ): void {

// TODO: Should we set this value at the beginning of the post physics engine step, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Should we set this value at the beginning of the post physics engine step, see https://github.com/phetsims/density-buoyancy-common/issues/317
// It currently seems like it is updated partway through (after it is accessed)?
this.isFullySubmerged = this.stepTop < fluidLevel - DensityBuoyancyCommonConstants.TOLERANCE;
}
Expand Down
6 changes: 3 additions & 3 deletions js/buoyancy/model/applications/BoatDesign.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
* compute a shape (for a given block size) such that in 2D it "acts" as the proper 3d shape. This is mainly due to
* the block corners pressing against the inside of the boat's hull.
*
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 This file is > 800 lines of complicated code
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 This file is > 800 lines of complicated code
* with many magic numbers. We should make an effort to remove unused parts, and clean up where possible.
*
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 this file has 5 unused declarations.
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 this file has 5 unused declarations.
*
* @author Jonathan Olson (PhET Interactive Simulations)
*/
Expand Down Expand Up @@ -698,7 +698,7 @@ BoatDesign.ONE_LITER_HULL_VOLUME = BoatDesign.DESIGN_HULL_VOLUME * BoatDesign.ON

/**
* Replaces the main page with a debug view of the bottle, for debugging various curves and properties.
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 this is unused and also says "bottle"
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 this is unused and also says "bottle"
*
* phet.densityBuoyancyCommon.BoatDesign.getDebugCanvas()
*/
Expand Down
14 changes: 7 additions & 7 deletions js/buoyancy/model/applications/Bottle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@
* the tip to the saddle point (second curve).
* - Saddle point: The true mathematical saddle where the saddle/tip curves meet back up, at r=0
*
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 This file is > 1200 lines of complicated code
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 This file is > 1200 lines of complicated code
* with many magic numbers. We should make an effort to remove unused parts, and clean up where possible.
*
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 this file has 9 unused declarations.
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 this file has 9 unused declarations.
*
* Diagram in x,r coordinates, showing the "profile" (exactly half of the bottle, and the curves that the
* three-dimensional form will be made from):
Expand Down Expand Up @@ -315,7 +315,7 @@ export default class Bottle extends ApplicationsMass {
private static evaluateCubic( controlPoints: Vector2[], t: number ): Vector2 {

// Is this algorithm duplicated in dot? Are these variables using conventionally established names?
// TODO: See https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: See https://github.com/phetsims/density-buoyancy-common/issues/317
const mt = 1 - t;
const mmm = mt * mt * mt;
const mmt = 3 * mt * mt * t;
Expand All @@ -341,7 +341,7 @@ export default class Bottle extends ApplicationsMass {
private static evaluateCubicDerivative( controlPoints: Vector2[], t: number ): Vector2 {
const mt = 1 - t;

// TODO: Where are these algorithms documented? Maybe point to wikipedia? https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Where are these algorithms documented? Maybe point to wikipedia? https://github.com/phetsims/density-buoyancy-common/issues/317
return new Vector2(
controlPoints[ 0 ].x * ( -3 * mt * mt ) +
controlPoints[ 1 ].x * ( 3 * mt * mt - 6 * mt * t ) +
Expand Down Expand Up @@ -785,7 +785,7 @@ const FLAT_INTERSECTION_VERTICES = [ ${flatIntersectionVertices.map( v => `new V
];
}

// TODO: Remove unused code, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Remove unused code, see https://github.com/phetsims/density-buoyancy-common/issues/317
private static getMainFlatIntersectionProfile(): Vector2[] {
return [
...Bottle.getCapProfile(),
Expand Down Expand Up @@ -1157,7 +1157,7 @@ const FLAT_INTERSECTION_VERTICES = [ ${flatIntersectionVertices.map( v => `new V

/**
* Replaces the main page with a debug view of the bottle, for debugging various curves and properties.
* TODO: Is this unused? See https://github.com/phetsims/density-buoyancy-common/issues/123
* TODO: Is this unused? See https://github.com/phetsims/density-buoyancy-common/issues/317
*/
private static getDebugCanvas(): HTMLCanvasElement {
const canvas = document.createElement( 'canvas' );
Expand Down Expand Up @@ -1247,7 +1247,7 @@ const FLAT_INTERSECTION_VERTICES = [ ${flatIntersectionVertices.map( v => `new V
return canvas;
}

// TODO: Are many of these constants unused? https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Are many of these constants unused? https://github.com/phetsims/density-buoyancy-common/issues/317

// The number to scale the original values by to get a 10L-volume bottle
private static readonly TEN_LITER_SCALE_MULTIPLIER = TEN_LITER_SCALE_MULTIPLIER;
Expand Down
2 changes: 1 addition & 1 deletion js/buoyancy/model/applications/BottleOrBoat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* BottleOrBoat is a string literal union enumeration that describes whether the sim is showing the bottle or boat scene
* in the Buoyancy - Applications screen.
*
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 some string literal enumerations are uppercase, some are lowercase. Recommend to make consistent.
* TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 some string literal enumerations are uppercase, some are lowercase. Recommend to make consistent.
*
* @author Sam Reid (PhET Interactive Simulations)
*/
Expand Down
10 changes: 5 additions & 5 deletions js/buoyancy/model/applications/BuoyancyApplicationsModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,15 @@ export default class BuoyancyApplicationsModel extends DensityBuoyancyModel {
canMove: false,
inputEnabledPropertyOptions: {

// TODO: Why can the input enabled be turned on? Is this adding a phet-io feature that makes the scale movable? see , see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Why can the input enabled be turned on? Is this adding a phet-io feature that makes the scale movable? see , see https://github.com/phetsims/density-buoyancy-common/issues/317
// If so, is changing the inputEnabledProperty to true the only thing that has to happen to make it work?
phetioReadOnly: false
}
} );
this.availableMasses.push( this.scale );

// Adjust pool volume so that it's at the desired value WITH the pool scales inside.
// TODO: How does this relate to https://github.com/phetsims/density-buoyancy-common/blob/4038cb05c2b5c2b8b1f600bfbcf0a7eaac4617a2/js/common/model/DensityBuoyancyModel.ts#L435-L437, see , see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: How does this relate to https://github.com/phetsims/density-buoyancy-common/blob/4038cb05c2b5c2b8b1f600bfbcf0a7eaac4617a2/js/common/model/DensityBuoyancyModel.ts#L435-L437, see , see https://github.com/phetsims/density-buoyancy-common/issues/317
this.pool.fluidVolumeProperty.setInitialValue( this.pool.fluidVolumeProperty.value );

// This instance lives for the lifetime of the simulation, so we don't need to remove this listener
Expand All @@ -146,7 +146,7 @@ export default class BuoyancyApplicationsModel extends DensityBuoyancyModel {

public override step( dt: number ): void {

// TODO: This seems an odd spot to put the assertion, is it necessary? See https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: This seems an odd spot to put the assertion, is it necessary? See https://github.com/phetsims/density-buoyancy-common/issues/317
assert && assert( !this.boat.visibleProperty.value || !this.bottle.visibleProperty.value,
'Boat and bottle should not be visible at the same time' );

Expand All @@ -166,7 +166,7 @@ export default class BuoyancyApplicationsModel extends DensityBuoyancyModel {
this.boat.resetPosition();
this.block.resetPosition();

// TODO: Can we call boat.reset() here? See , see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Can we call boat.reset() here? See , see https://github.com/phetsims/density-buoyancy-common/issues/317
this.boat.verticalAcceleration = 0;
this.boat.verticalVelocity = 0;

Expand All @@ -178,7 +178,7 @@ export default class BuoyancyApplicationsModel extends DensityBuoyancyModel {
*/
public override reset(): void {

// TODO: Can we call resetBoatScene from reset? See , see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Can we call resetBoatScene from reset? See , see https://github.com/phetsims/density-buoyancy-common/issues/317

this.bottle.reset();
this.block.reset();
Expand Down
2 changes: 1 addition & 1 deletion js/buoyancy/model/shapes/BuoyancyShapesModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export default class BuoyancyShapesModel extends DensityBuoyancyModel {
} );


// TODO: This documentation on the next line doesn't seem fully correct: https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: This documentation on the next line doesn't seem fully correct: https://github.com/phetsims/density-buoyancy-common/issues/317
// When a new mass is created, set up its position to be that of the old mass
[ this.objectA, this.objectB ].forEach( shapeModel => {

Expand Down
2 changes: 1 addition & 1 deletion js/buoyancy/view/BuoyancyCompareScreenView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export default class BuoyancyCompareScreenView extends BuoyancyScreenView<Buoyan

public constructor( model: BuoyancyCompareModel, options: BuoyancyCompareScreenViewOptions ) {

// TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 is combineOptions preferable to optionize here?
// TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 is combineOptions preferable to optionize here?
super( model, combineOptions<DensityBuoyancyScreenViewOptions>( {
supportsDepthLines: true,
forcesInitiallyDisplayed: false,
Expand Down
4 changes: 2 additions & 2 deletions js/buoyancy/view/BuoyancyExploreScreenView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export default class BuoyancyExploreScreenView extends BuoyancyScreenView<Buoyan

const tandem = options.tandem;

// TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 why is combineOptions preferable to optionize in this case?
// TODO: https://github.com/phetsims/density-buoyancy-common/issues/317 why is combineOptions preferable to optionize in this case?
super( model, combineOptions<DensityBuoyancyScreenViewOptions>( {
supportsDepthLines: true,
forcesInitiallyDisplayed: false,
Expand Down Expand Up @@ -158,7 +158,7 @@ export default class BuoyancyExploreScreenView extends BuoyancyScreenView<Buoyan
ManualConstraint.create( this, [ this.resetAllButton, blocksModeRadioButtonGroup ],
( resetAllButtonWrapper, blocksModeRadioButtonGroupWrapper ) => {

// TODO: What if the reset all button is invisible due to phet-io customization? See https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: What if the reset all button is invisible due to phet-io customization? See https://github.com/phetsims/density-buoyancy-common/issues/317
blocksModeRadioButtonGroupWrapper.right = resetAllButtonWrapper.left - DensityBuoyancyCommonConstants.MARGIN;
blocksModeRadioButtonGroupWrapper.bottom = resetAllButtonWrapper.bottom;
} );
Expand Down
2 changes: 1 addition & 1 deletion js/buoyancy/view/DisplayProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export default class DisplayProperties {
this.forceValuesVisibleProperty.reset();
this.depthLinesVisibleProperty.reset();

// TODO: Reset vectorZoomLevelProperty? See https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Reset vectorZoomLevelProperty? See https://github.com/phetsims/density-buoyancy-common/issues/317
}

}
Expand Down
4 changes: 2 additions & 2 deletions js/buoyancy/view/FluidDensityPanel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ export default class FluidDensityPanel extends Panel {
popupLayer, {
invisibleMaterials: invisibleMaterials,

// TODO: Passing the same tandem 2 places seems suspicious. Check it and document if OK, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Passing the same tandem 2 places seems suspicious. Check it and document if OK, see https://github.com/phetsims/density-buoyancy-common/issues/317
tandem: tandem
} );
super( fluidDensityControlNode, combineOptions<PanelOptions>( {

// TODO: See note above, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: See note above, see https://github.com/phetsims/density-buoyancy-common/issues/317
tandem: tandem
}, DensityBuoyancyCommonConstants.PANEL_OPTIONS ) );
}
Expand Down
4 changes: 2 additions & 2 deletions js/buoyancy/view/FluidDisplacedAccordionBox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export default class FluidDisplacedAccordionBox extends AccordionBox {
forceReadout.centerX = beakerNode.centerX;
} );

// TODO: Does any of this layout need to be dynamic? What if things disappear or reshape? See https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Does any of this layout need to be dynamic? What if things disappear or reshape? See https://github.com/phetsims/density-buoyancy-common/issues/317
numberDisplay.bottom = beakerNode.bottom - beakerNode.height * 0.05;
numberDisplay.right = beakerNode.right;
scaleIcon.top = beakerNode.bottom - 13;
Expand All @@ -162,7 +162,7 @@ export default class FluidDisplacedAccordionBox extends AccordionBox {
);
super( panel, options );

// TODO: Rename displayedDisplacedVolumeProperty to displacedVolumeProperty to match the tandem, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Rename displayedDisplacedVolumeProperty to displacedVolumeProperty to match the tandem, see https://github.com/phetsims/density-buoyancy-common/issues/317
this.addLinkedElement( displayedDisplacedVolumeProperty, {
tandemName: 'displacedVolumeProperty'
} );
Expand Down
4 changes: 2 additions & 2 deletions js/buoyancy/view/ReadoutListAccordionBox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ export default abstract class ReadoutListAccordionBox<ReadoutType> extends Accor

this.readoutBox.children = readoutItems.map( readoutItem => {

// 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
// TODO: Create a new type and file for this data structure, see https://github.com/phetsims/density-buoyancy-common/issues/317
// 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/317
const readoutData = this.generateReadoutData( readoutItem.readoutItem );
const nameProperty = readoutItem.readoutNameProperty || readoutData.nameProperty;
const nameColonProperty = new PatternStringProperty(
Expand Down
4 changes: 2 additions & 2 deletions js/buoyancy/view/applications/ApplicationsDebugView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/**
* Extends DebugView, which shows a 2d version of the model. In this case, used for the boat's shape.
* TODO: Document how to show the debug view here. see https://github.com/phetsims/density-buoyancy-common/issues/257
* TODO: This file is overly-complicated and under-documented, see https://github.com/phetsims/density-buoyancy-common/issues/123
* TODO: This file is overly-complicated and under-documented, see https://github.com/phetsims/density-buoyancy-common/issues/317
*
* @author Jonathan Olson (PhET Interactive Simulations)
*/
Expand Down Expand Up @@ -52,7 +52,7 @@ export default class ApplicationsDebugView extends DebugView {
public override step( dt: number ): void {
super.step( dt );

// TODO: Change this to say this.applicationsModel.boat instead of searches and instanceof operations, see https://github.com/phetsims/density-buoyancy-common/issues/123
// TODO: Change this to say this.applicationsModel.boat instead of searches and instanceof operations, see https://github.com/phetsims/density-buoyancy-common/issues/317
const boat = this.model.masses.find( mass => mass instanceof Boat );
if ( boat instanceof Boat ) {
const boatYValues = _.range( boat.stepBottom, boat.stepTop, 0.002 );
Expand Down
Loading

0 comments on commit 96c540a

Please sign in to comment.