search

phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Duplicate code for creating launcherNode and adding it to launchLayer #217

Closed pixelzoom closed 5 months ago

pixelzoom commented 5 months ago

For code review #32...

This code is duplicated in every ScreenView subclass:

  protected readonly launcherNode: LauncherNode;
...
    this.launcherNode = new LauncherNode(
      ...
    );

    this.launcherLayer.addChild( this.launcherNode );

The duplication could be consolidated in PDLScreenView by adding this to SelfOptions:

  createLauncherNode: ( modelViewTransform: ModelViewTransform2 ) => LauncherNode;

Here's a complete working patch:

patch ```diff Subject: [PATCH] delete redundant WithRequired, https://github.com/phetsims/projectile-data-lab/issues/216 --- Index: js/variability/view/VariabilityScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/variability/view/VariabilityScreenView.ts b/js/variability/view/VariabilityScreenView.ts --- a/js/variability/view/VariabilityScreenView.ts (revision c95317ead272a8faf79edd74e874aebcdc9d6328) +++ b/js/variability/view/VariabilityScreenView.ts (date 1709502339101) @@ -21,17 +21,25 @@ import PDLColors from '../../common/PDLColors.js'; import { histogramAccordionBoxTandemName } from '../../common/view/HistogramAccordionBox.js'; import VSMHistogramNode from '../../common-vsm/view/VSMHistogramNode.js'; +import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; type SelfOptions = EmptySelfOptions; -type VariabilityScreenViewOptions = SelfOptions & VSMScreenViewOptions; +type VariabilityScreenViewOptions = SelfOptions & StrictOmit; export default class VariabilityScreenView extends VSMScreenView { - protected readonly launcherNode: LauncherNode; - public constructor( model: VariabilityModel, providedOptions: VariabilityScreenViewOptions ) { - const options = optionize()( {}, providedOptions ); + const options = optionize()( { + + createLauncherNode: modelViewTransform => new LauncherNode( + modelViewTransform, + model.meanLaunchAngleProperty, + model.launcherHeightProperty, + model.launcherProperty, + model.fieldProperty + ) + }, providedOptions ); const launchPanel = new VariabilityLaunchPanel( model.launcherConfigurationProperty, model.projectileTypeProperty, model.launcherProperty, { tandem: options.tandem.createTandem( 'launchPanel' ) @@ -60,16 +68,6 @@ } ); super( model, launchPanel, staticToolPanel, interactiveToolPanel, createHistogramNode, options ); - - this.launcherNode = new LauncherNode( - this.modelViewTransform, - model.meanLaunchAngleProperty, - model.launcherHeightProperty, - model.launcherProperty, - model.fieldProperty - ); - - this.launcherLayer.addChild( this.launcherNode ); } } projectileDataLab.register( 'VariabilityScreenView', VariabilityScreenView ); \ No newline at end of file Index: js/common/view/PDLScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/PDLScreenView.ts b/js/common/view/PDLScreenView.ts --- a/js/common/view/PDLScreenView.ts (revision c95317ead272a8faf79edd74e874aebcdc9d6328) +++ b/js/common/view/PDLScreenView.ts (date 1709502339087) @@ -10,7 +10,7 @@ import ScreenView, { ScreenViewOptions } from '../../../../joist/js/ScreenView.js'; import projectileDataLab from '../../projectileDataLab.js'; import ResetAllButton from '../../../../scenery-phet/js/buttons/ResetAllButton.js'; -import { HBox, ManualConstraint, Node, createGatedVisibleProperty } from '../../../../scenery/js/imports.js'; +import { createGatedVisibleProperty, HBox, ManualConstraint, Node } from '../../../../scenery/js/imports.js'; import PDLConstants from '../PDLConstants.js'; import ProjectileDataLabStrings from '../../ProjectileDataLabStrings.js'; import PDLColors from '../PDLColors.js'; @@ -34,11 +34,12 @@ import LaunchButton from './LaunchButton.js'; import FieldSignNode from './FieldSignNode.js'; import EraserButton from '../../../../scenery-phet/js/buttons/EraserButton.js'; -import { EmptySelfOptions } from '../../../../phet-core/js/optionize.js'; import { DerivedProperty } from '../../../../axon/js/imports.js'; import PDLPreferences from '../PDLPreferences.js'; -type SelfOptions = EmptySelfOptions; +type SelfOptions = { + createLauncherNode: ( modelViewTransform: ModelViewTransform2 ) => LauncherNode; +}; export type PDLScreenViewOptions = SelfOptions & WithRequired; export default abstract class PDLScreenView extends ScreenView { @@ -50,7 +51,7 @@ protected readonly behindProjectilesLayer = new Node(); protected readonly projectileLayer = new Node(); - protected abstract readonly launcherNode: LauncherNode; + protected readonly launcherNode: LauncherNode; protected abstract readonly launchPanel: PDLLaunchPanel; protected abstract readonly accordionBox: HistogramAccordionBox; @@ -230,6 +231,9 @@ this.addChild( this.singleOrContinuousRadioButtonGroup ); this.addChild( this.noAirResistanceText ); this.addChild( this.eraseResetContainer ); + + this.launcherNode = options.createLauncherNode( this.modelViewTransform ); + this.launcherLayer.addChild( this.launcherNode ); } /** Index: js/measures/view/MeasuresScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/view/MeasuresScreenView.ts b/js/measures/view/MeasuresScreenView.ts --- a/js/measures/view/MeasuresScreenView.ts (revision c95317ead272a8faf79edd74e874aebcdc9d6328) +++ b/js/measures/view/MeasuresScreenView.ts (date 1709502339109) @@ -21,17 +21,30 @@ import IntervalToolNode from './IntervalToolNode.js'; import { Node } from '../../../../scenery/js/imports.js'; import { histogramAccordionBoxTandemName } from '../../common/view/HistogramAccordionBox.js'; +import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; type SelfOptions = EmptySelfOptions; -type MeasuresScreenViewOptions = SelfOptions & VSMScreenViewOptions; +type MeasuresScreenViewOptions = SelfOptions & StrictOmit; export default class MeasuresScreenView extends VSMScreenView { - protected readonly launcherNode: CustomLauncherNode; - public constructor( model: MeasuresModel, providedOptions: MeasuresScreenViewOptions ) { - const options = optionize()( {}, providedOptions ); + const options = optionize()( { + createLauncherNode: modelViewTransform => new CustomLauncherNode( + modelViewTransform, + model.launcherConfigurationProperty, + model.meanLaunchAngleProperty, + model.launcherHeightProperty, + model.mysteryOrCustomProperty, + model.mysteryLauncherProperty, + model.customLauncherMechanismProperty, + model.standardDeviationAngleProperty, + model.latestLaunchSpeedProperty, + model.fieldProperty, + {} + ) + }, providedOptions ); const launchPanel = new MeasuresLaunchPanel( model.launcherConfigurationProperty, model.projectileTypeProperty, model.mysteryOrCustomProperty, model.mysteryLauncherProperty, model.customLauncherMechanismProperty, model.angleStabilizerProperty, { @@ -75,22 +88,6 @@ super( model, launchPanel, staticToolPanel, interactiveToolPanel, createHistogramNode, options ); - this.launcherNode = new CustomLauncherNode( - this.modelViewTransform, - model.launcherConfigurationProperty, - model.meanLaunchAngleProperty, - model.launcherHeightProperty, - model.mysteryOrCustomProperty, - model.mysteryLauncherProperty, - model.customLauncherMechanismProperty, - model.standardDeviationAngleProperty, - model.latestLaunchSpeedProperty, - model.fieldProperty, - {} - ); - - this.launcherLayer.addChild( this.launcherNode ); - const intervalToolNode = new IntervalToolNode( model.intervalTool, this.modelViewTransform, { isIcon: false, visibleProperty: model.isIntervalToolVisibleProperty, Index: js/sampling/view/SamplingScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/sampling/view/SamplingScreenView.ts b/js/sampling/view/SamplingScreenView.ts --- a/js/sampling/view/SamplingScreenView.ts (revision c95317ead272a8faf79edd74e874aebcdc9d6328) +++ b/js/sampling/view/SamplingScreenView.ts (date 1709502339095) @@ -16,7 +16,6 @@ import SamplingAccordionBox from './SamplingAccordionBox.js'; import SamplingField from '../model/SamplingField.js'; import SampleSelectorNode from './SampleSelectorNode.js'; -import LauncherNode from '../../common/view/LauncherNode.js'; import SamplingCanvasNode from './SamplingCanvasNode.js'; import ProjectileDataLabStrings from '../../ProjectileDataLabStrings.js'; import MeanIndicatorNode from '../../common/view/MeanIndicatorNode.js'; @@ -29,20 +28,30 @@ import { histogramAccordionBoxTandemName } from '../../common/view/HistogramAccordionBox.js'; import FieldSignNode from '../../common/view/FieldSignNode.js'; import PDLPreferences from '../../common/PDLPreferences.js'; +import LauncherNode from '../../common/view/LauncherNode.js'; +import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; type SelfOptions = EmptySelfOptions; -type SamplingScreenViewOptions = SelfOptions & PDLScreenViewOptions; +type SamplingScreenViewOptions = SelfOptions & StrictOmit; export default class SamplingScreenView extends PDLScreenView { protected readonly fieldSignNode: FieldSignNode; - protected readonly launcherNode: LauncherNode; protected readonly launchPanel: SamplingLaunchPanel; protected readonly accordionBox: SamplingAccordionBox; public constructor( model: SamplingModel, providedOptions: SamplingScreenViewOptions ) { - const options = optionize()( {}, providedOptions ); + const options = optionize()( { + + createLauncherNode: modelViewTransform => new LauncherNode( + modelViewTransform, + model.meanLaunchAngleProperty, + model.launcherHeightProperty, + model.launcherProperty, + model.fieldProperty + ) + }, providedOptions ); const launchButtonEnabledProperty = new DerivedProperty( [ model.phaseProperty, model.numberOfStartedSamplesProperty, model.singleOrContinuousProperty ], ( phase, startedSamples, singleOrContinuous ) => { @@ -103,16 +112,6 @@ } } ); - this.launcherNode = new LauncherNode( - this.modelViewTransform, - model.meanLaunchAngleProperty, - model.launcherHeightProperty, - model.launcherProperty, - model.fieldProperty - ); - - this.launcherLayer.addChild( this.launcherNode ); - this.launchPanel = new SamplingLaunchPanel( model.launcherProperty, model.sampleSizeProperty, { tandem: options.tandem.createTandem( 'launchPanel' ) } ); Index: js/sources/view/SourcesScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/sources/view/SourcesScreenView.ts b/js/sources/view/SourcesScreenView.ts --- a/js/sources/view/SourcesScreenView.ts (revision c95317ead272a8faf79edd74e874aebcdc9d6328) +++ b/js/sources/view/SourcesScreenView.ts (date 1709502395928) @@ -23,16 +23,34 @@ import { histogramAccordionBoxTandemName } from '../../common/view/HistogramAccordionBox.js'; import SMField from '../../common-sm/model/SMField.js'; import VSMHistogramNode from '../../common-vsm/view/VSMHistogramNode.js'; +import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; type SelfOptions = EmptySelfOptions; -type SourcesScreenViewOptions = SelfOptions & VSMScreenViewOptions; +type SourcesScreenViewOptions = SelfOptions & StrictOmit; export default class SourcesScreenView extends VSMScreenView { - protected readonly launcherNode: CustomLauncherNode; - public constructor( model: SourcesModel, providedOptions: SourcesScreenViewOptions ) { - const options = optionize()( {}, providedOptions ); + + const launcher = model.launcherProperty.value; + assert && assert( launcher.mysteryOrCustom === 'custom', 'The launcher should be custom' ); + + const options = optionize()( { + + createLauncherNode: modelViewTransform => new CustomLauncherNode( + modelViewTransform, + model.launcherConfigurationProperty, + model.meanLaunchAngleProperty, + model.launcherHeightProperty, + new Property( 'custom' ), + new Property( launcher ), + model.customLauncherMechanismProperty, + model.standardDeviationAngleProperty, + model.latestLaunchSpeedProperty, + model.fieldProperty, + {} + ) + }, providedOptions ); const launchPanel = new SourcesLaunchPanel( model.launcherConfigurationProperty, model.projectileTypeProperty, model.customLauncherMechanismProperty, model.angleStabilizerProperty, { @@ -62,25 +80,6 @@ } ); super( model, launchPanel, staticToolPanel, interactiveToolPanel, createHistogramNode, options ); - - const launcher = model.launcherProperty.value; - assert && assert( launcher.mysteryOrCustom === 'custom', 'The launcher should be custom' ); - - this.launcherNode = new CustomLauncherNode( - this.modelViewTransform, - model.launcherConfigurationProperty, - model.meanLaunchAngleProperty, - model.launcherHeightProperty, - new Property( 'custom' ), - new Property( launcher ), - model.customLauncherMechanismProperty, - model.standardDeviationAngleProperty, - model.latestLaunchSpeedProperty, - model.fieldProperty, - {} - ); - - this.launcherLayer.addChild( this.launcherNode ); } } ```
matthew-blackman commented 5 months ago

Reviewed and committed. Thanks for the recommendation and patch! Closing.