where should Ranges for NumberProperties live?

[pixelzoom]

Related to code review #259.

This started as several REVIEW comments, but it's gotten complicated, so creating this issue.

This issue applies to NumberProperty instances in Scene, WaterScene, and (possibly) DiffractionModel.

Ranges for many (all?) model NumberProperties are not not specified. Instead, they are associated with the UI components.

For example, in Scene:

      //REVIEW add value validation?
      //REVIEW* the model supports any value, though going outside the bounds of the lattice would probably be a logic
      //REVIEW* error.  The view slider has its own constraint.  Which value should I use here?
      // @public distance between the sources in the units of the scene, or 0 if there is only one
      // source initialized to match the initial slit separation,
      // see https://github.com/phetsims/wave-interference/issues/87
      this.sourceSeparationProperty = new NumberProperty(
        config.numberOfSources === 1 ? 0 : config.initialSlitSeparation, {
          units: this.positionUnits
        } );

A few issues with not providing Ranges with these NumberProperties, considering future PhET-iO requirements:

(1) How can these model Properties be instrumented (with a slider) in Studio if no Range is provided?

(2) When inspecting these model Properties in Studio, there will be no Range info found.

(3) The Ranges are currently spread out across view components, rather than being in the model with their associated Properties. For example, 6 of the Ranges are in SlitsControlPanel (search for "new Range"). These Ranges would presumably need to be instrumented explicitly to make them visible in Studio.

(4) One could argue that specifying Ranges in the UI components violates MVC. Info needed by controls should come from the model.

(5) The current pattern is contrary to what's done in other sims. This may be confusing to Studio users.

[pixelzoom]

Also worth noting that there is inconsistency here; some NumberProperty instances do have Range specified in the model. See for example frequencyProperty and amplitudeProperty in Scene. Why the different treatment?

[pixelzoom]

Here are the Ranges in question, currently in the view:

// InteferenceScreenView
const waterSceneRange = new Range( 1, 5 ); // cm
const soundSceneRange = new Range( 100, 400 ); // cm
const lightSceneRange = new Range( 500, 4000 ); // nm

// SlitsControlPanel
const waterRange = new Range( 0.5, 2.5 ); // cm
const soundRange = new Range( 20, 160 );
const lightRange = new Range( 200, 1600 );
const waterSeparationRange = new Range( 1, 5 );
const soundSeparationRange = new Range( 40, 320 ); // cm
const soundSeparationRange = new Range( 40, 320 ); // cm
const lightSeparationRange = new Range( 400, 3200 );

There are 6 more in DiffractionScreenView, but we're ignoring that screen for 1.0.

[samreid]

Good suggestion, I specified the Ranges in the model. Please review.

[pixelzoom]

👍 Closing.