phetsims / number-line-distance

"Number Line: Distance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 3 forks source link

NLDScene enumeration is unnecessary. #55

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

In NLDExploreModel, you have:

    // @public {Property.<NLDScene>} the currently selected scene for the explore screen
    this.selectedSceneProperty = new EnumerationProperty( NLDScene, NLDScene.DISTANCE );

    // @public the instance for the model of the 'Distance' scene
    this.distanceSceneModel = new DistanceSceneModel( tandem );

    // @public the instance for the model of the 'Temperature' scene
    this.temperatureSceneModel = new TemperatureSceneModel( tandem );

    // @public the instance for the model of the 'Elevation' scene
    this.elevationSceneModel = new ElevationSceneModel( tandem );

And then in NLDExploreScreenView, you do this:

    // Link each specific scene view's visibility with whether it is selected in the model.
    model.selectedSceneProperty.link( selectedScene => {
      distanceSceneView.visible = selectedScene === NLDScene.DISTANCE;
      temperatureSceneView.visible = selectedScene === NLDScene.TEMPERATURE;
      elevationSceneView.visible = selectedScene === NLDScene.ELEVATION;
    } );

But there's really no need for a separate enumeration, you could just use the scene instances as the values for this.selectedSceneProperty. In NLDExploreModel:

    // @public the instance for the model of the 'Distance' scene
    this.distanceSceneModel = new DistanceSceneModel( tandem );

    // @public the instance for the model of the 'Temperature' scene
    this.temperatureSceneModel = new TemperatureSceneModel( tandem );

    // @public the instance for the model of the 'Elevation' scene
    this.elevationSceneModel = new ElevationSceneModel( tandem );

    // @public {Property.<AbstractNLDBaseModel>} the currently selected scene for the explore screen
    this.selectedSceneProperty = new Property( this.distanceSceneModel, {
      validValues: [ this.distanceSceneModel, this.temperatureSceneModel, this.elevationSceneModel ]
    } );

Then either add a this.model field to each *SceneView subclass and do this in NLDExploreScreenView:

    // Link each specific scene view's visibility with whether it is selected in the model.
    model.selectedSceneProperty.link( selectedScene => {
      distanceSceneView.visible = ( selectedScene === model.distanceSceneModel );
      temperatureSceneView.visible = ( selectedScene === model.temperatureSceneModel );
      elevationSceneView.visible = ( selectedScene === model.elevationSceneModel );
    } );

Or pass model.selectedSceneProperty to the SceneView classes, and make each SceneView responsible for setting its own visibility. For example, in TemperatureSceneView:

constructor( model, selectedSceneProperty ) {
...
   selectedSceneProperty.link( selectedScene => {
      this.visible = ( selectedScene === model );
   } );
}
SaurabhTotey commented 3 years ago

I have reworked the property to now store the scene model, and the view now sets the visible scene view to the one that has the selected model (as suggested in the first solution). I'm closing this issue, but feel free to reopen if there's more to do.