duplication of view Properties

[chrisklus]

@samreid and I found a tandem for coordinatesVisibleProperty in the explore screen in studio but coordinates only seem to be relevant to screens 2, 3, and 4. For context:
graphingQuadratics.exploreScreen.view.viewProperties.coordinatesVisibleProperty

At a minimum, we should add documentation that explains this is not on the first screen.

Found when working on #14.

[pixelzoom]

So it's OK to provide access to every Node property when we only want to make one or two visible. But it's not OK to provide sim-specific features in a base class that aren't used in every screen? That seems like a double standard to me. That said...

Alternatively, I could remove coordinatesVisibleProperty from the GQViewProperties base class and duplicate it in the subclasses for the 3 screens that have this Property. But I don't like the prescendent that sets. I also don't like having to put something like "not used in the Explore" screen in the phetioDocumentation for coordinatesVisibleProperty. There will be features in reused code that are not used in every screen. Imo, clients will figure it out.

[samreid]

Have you considered the following?

ExploreViewProperties extends GQViewProperties
{{GQViewProperties2}} extends GQViewProperties // adds coordinatesVisibleProperty 
FocusAndDirectrixViewProperties extends {{GQViewProperties2}}
StandardFormViewProperties extends {{GQViewProperties2}}
VertexFormViewProperties extends {{GQViewProperties2}}

Also, the code already has this.vertexVisibleProperty = ... duplicated in 3 files. That should be factored out to {{GQViewProperties2}}. I'm putting that in template curly braces because I'm not familiar enough with the sim to know the best name for it.

[pixelzoom]

Have you considered the following? ....

Yes. And in this case that makes for a very (unnecessarily) complicated class hierarchy.

At one time, GQViewProperties was the union of all view Properties for the 4 screens with the documented caveat that "screens do not make all of these Properties visible". This was because the model supports all of the view Properties, and the screens could (and at some point did) use most or all of the Properties.

Once the set of Properties for each screen "settled down" in the design process, I considered a class hierarchy for view Properties. coordinatesVisibleProperty is by no means that only Property that appears in <4 screens; see for example vertexVisibleProperty. While it's probably possible to come up with a hierarchy where no Property is duplicated, and no Property is unused by a screen's view, the end result makes it unnecessarily difficult to see what Properties each screen has. So my preference for Properties that are used in <4 screens is to either (a) duplicate their definition in the subclass, or (b) include their definition in the base class with a note that they are not used in all screens. I'm going to go with (a).

[pixelzoom]

Also note that duplicating coordinatesVisibleProperty allows the phetioDocumentation to be screen specific, i.e.:

Standard Form: 'whether coordinates are visible on the vertex and roots'
Vertex Form: 'whether coordinates are visible on the vertex'
Focus & Directrix: 'whether coordinates are visible on manipulators (vertex, focus, point on parabola)'

[pixelzoom]

coordinatesVisibleProperty has been removed from GQViewProperties base class and is now duplicated in the screen-specific base classes where it's relevant, with phetioDocumentation that is specific to each screen. @chrisklus or @samreid please review.

[pixelzoom]

Above I said:

And in this case that makes for a very (unnecessarily) complicated class hierarchy.

To demonstrate... Here's the current class hierarchy, 1 base class and a subclass for each screen, pretty straightforward to compare Properties to checkboxes:

class GQViewProperties {
  this.graphContentsVisibleProperty
  this.equationAccordionBoxExpandedProperty
  this.equationsVisibleProperty 
}

class ExploreViewProperties extends GQViewProperties {
  this.quadraticTermsAccordionBoxExpandedProperty
  this.quadraticTermVisibleProperty 
  this.linearTermVisibleProperty
  this.constantTermVisibleProperty
}

class StandardFormViewProperties extends GQViewProperties {
  this.vertexVisibleProperty
  this.axisOfSymmetryVisibleProperty
  this.rootsVisibleProperty 
  this.coordinatesVisibleProperty
}

class VertexFormViewProperties extends GQViewProperties {
  this.vertexVisibleProperty
  this.axisOfSymmetryVisibleProperty
  this.coordinatesVisibleProperty
}

class FocusAndDirectrixViewProperties extends GQViewProperties {
  this.vertexVisibleProperty
  this.focusVisibleProperty
  this.directrixVisibleProperty
  this.pointOnParabolaVisibleProperty
  this.coordinatesVisibleProperty
}

Here's the hierarchy required to eliminate duplication. It took me several tries to come up with this, and I'm still not certain that it's correct. It has 3 abstract base classes. It's difficult to see how this maps to each screen, I'd hate to have to maintain this. And there's no way to tailor the phetioDocumentation to specific screens (e.g. for vertexProperty).

class GQViewProperties {
  this.graphContentsVisibleProperty
  this.equationAccordionBoxExpandedProperty
  this.equationsVisibleProperty 
}

class GQViewProperties2 extends GQViewProperties {
  this.coordinatesVisibleProperty
}

class GQViewProperties3 extends GQViewProperties2 {
  this.vertexVisibleProperty
}

class ExploreViewProperties extends GQViewProperties {
  this.quadraticTermsAccordionBoxExpandedProperty
  this.quadraticTermVisibleProperty 
  this.linearTermVisibleProperty
  this.constantTermVisibleProperty
}

class StandardFormViewProperties extends VertexFormViewProperties {
  this.rootsVisibleProperty 
} 

class VertexFormViewProperties extends GQViewProperties3 {
  this.axisOfSymmetryVisibleProperty
}

class FocusAndDirectrixViewProperties extends GQViewProperties2 {
  this.focusVisibleProperty
  this.directrixVisibleProperty
  this.pointOnParabolaVisibleProperty
}

(EDIT: coordinatesVisibleProperties -> coordinatesVisibleProperty)

[samreid]

On initial inspection, the proposed hierarchy at the bottom of #55 (comment) seem like overkill, but in fact it perfectly captures the relevant properties for each screen at the cost of introducing only 2 trivial subclasses. Therefore, this seems promising. @chrisklus and I recommend to name GQViewProperties2 => GQViewPropertiesWithCoordinates and GQViewProperties3 => GQViewPropertiesWithCoordinatesAndVertex

Also, @chrisklus and I noted that VertexFormViewProperties.js has:

// @public
this.vertexVisibleProperty = new BooleanProperty( GQQueryParameters.checkAll, {
  tandem: options.tandem.createTandem( 'vertexVisibleProperty' ),
  phetioDocumentation: 'whether the vertex is visible'
} );

However, this seems like a "vertex manipulator" (borrowing terminology from another screen). Should the documentation be changed?

If it is important to that the the phetioDocumentation differs for some Properties (like vertexVisibleProperty), then that can be passed through as an option. However, it is unclear whether different documentation is necessary. For instance, they currently read:

whether the vertex is visible
vs
whether the vertex manipulator is visible

It seems using the same text "Whether the vertex is visible" would be sufficient for both models, and it is the responsibility of the view side (vertexNode vs vertexManipulatorNode) to indicate whether it's draggable.

The same logic may apply to coordinatesVisibleProperty as well.

[pixelzoom]

The current phetioDocumentation for this.vertexVisibleProperty in VertexFormViewProperties.js is correct. In the Vertex Form screen, the vertex is not a manipulator, it's a non-interactive point displayed on the parabola.

Re changing the class hierarchy... Sorry, but this is a change that I am not willing to make. Duplicating 2 Properties is (in this case) preferable to the compromise to complexity, readability and maintainability that comes with the added subclasses. I also think that having screen-specific phetioDocumentation is useful, and adding options to inject sim-specific phetioDocumentation would complicate things even further.

[samreid]

Here is a table that indicates the Properties for each screen as indicated in the top half of #55 (comment)

Property	Explore	Standard	Vertex	Directrix
graphContentsVisibleProperty	x	x	x	x
equationAccordionBoxExpandedProperty	x	x	x	x
equationsVisibleProperty	x	x	x	x
quadraticTermsAccordionBoxExpandedProperty	x
quadraticTermVisibleProperty	x
linearTermVisibleProperty	x
constantTermVisibleProperty	x
vertexVisibleProperty		x	x	x
axisOfSymmetryVisibleProperty		x	x
rootsVisibleProperty		x
coordinatesVisibleProperties (sic)		x	x	x
focusVisibleProperty				x
directrixVisibleProperty				x
pointOnParabolaVisibleProperty				x

This can be implemented as:

class GQViewProperties {
  this.graphContentsVisibleProperty
  this.equationAccordionBoxExpandedProperty
  this.equationsVisibleProperty 
}

class ExploreViewProperties extends GQViewProperties {
  this.quadraticTermsAccordionBoxExpandedProperty
  this.quadraticTermVisibleProperty 
  this.linearTermVisibleProperty
  this.constantTermVisibleProperty
}

class VertexFormViewProperties extends GQViewProperties {
  this.vertexVisibleProperty
  this.axisOfSymmetryVisibleProperty
  this.coordinatesVisibleProperties
}

class StandardFormViewProperties extends VertexFormViewProperties {
  this.rootsVisibleProperty
}

class FocusAndDirectrixViewProperties extends VertexFormViewProperties {
  this.focusVisibleProperty
  this.directrixVisibleProperty
  this.pointOnParabolaVisibleProperty
}

This hierarchy (a) introduces 0 new classes (b) duplicates no code and (c) only has one Property appear in a place it is unused (the Directrix screen has an unused axisOfSymmetryVisibleProperty). Having one unused Property seems preferable to having a duplicated declaration.
phetioDocumentation can be passed as options (not great, but superior to duplication). It's nice to eliminate duplication so that Properties will automatically be synchronized across screens. For instance, if one day we renamed vertexVisibleProperty to isVertexVisibleProperty, it would be done in exactly one place instead of scattered across multiple files.

[pixelzoom]

Your implementation above is incorrect. It results in FocusAndDirectrixViewProperties having Property axisOfSymmetryVisibleProperty, which is not part of the Focus & Directrix screen. Like I said... the additional hierarchy required to factor out 2 Properties is overly complicated, and difficult to implement correctly.

[pixelzoom]

... has one Property appear in a place it is unused (the Directrix screen has an unused axisOfSymmetryVisibleProperty). Having one unused Property seems preferable to having a duplicated declaration. ...

Reminder that this issue was created because I had one Property in the base class (coordinatesVisibleProperty) that was not used in one screen (Explore). Why was that not OK, but it's now OK to include a different unused Property in a different screen's superclass?

[pixelzoom]

Did you read the review comment?

Yes, I read the review comment. I stated that the implementation is incorrect because you're comparing apples to oranges. The requirements were that we have no duplicated or unused Properties. You changed the requirements.

[pixelzoom]

Are you suggesting that duplicated code is preferable to unused Properties?

I have no problem with unused Properties. That is apparently a PhET-iO problem/requirement. The union of all view Properties was in one class for a long time, with the documented caveat "Some screens expose only a subset of these."

Duplicating code may indeed be a better solution in some situations. And imo this is such a situation. I've lost count of how many times Properties been added to and removed from screens, and I've been able to manage that easily (and without error) by keeping things simple.

[pixelzoom]

If unused Properties are OK, then here's an approach that goes (probably too far) in the opposite direction. The base class contains Properties that are used in >1 screen. Screen-specific subclasses add Properties that are unique to a screen.

But my understanding is that this is problematic for PhET-iO, and (in the best case) we'd have to indicate which screen(s) a Property is relevant to in the phetioDocumentation.

// Properties used in > 1 screen
class GQViewProperties {
  this.graphContentsVisibleProperty
  this.equationAccordionBoxExpandedProperty
  this.equationsVisibleProperty 
  this.coordinatesVisibileProperty
  this.vertexVisibileProperty
  this.axisOfSymmetryVisibileProperty
}

// Extends the base class with Properties that are specific to the Explore screen
class ExploreViewProperties extends GQViewProperties {
  this.quadraticTermsAccordionBoxExpandedProperty
  this.quadraticTermVisibleProperty 
  this.linearTermVisibleProperty
  this.constantTermVisibleProperty
}

// Extends the base class with Properties that are specific to the Standard Form screen
class StandardFormViewProperties extends GQViewProperties {
  this.rootsVisibleProperty
}

// Extends the base class with Properties that are specific to the Vertex Form screen
class VertexFormViewProperties extends GQViewProperties { 
   // none
} 

// Extends the base class with Properties that are specific to the Focus & Directrix screen
class FocusAndDirectrixViewProperties extends GQViewProperties {
  this.focusVisibleProperty
  this.directrixVisibleProperty
  this.pointOnParabolaVisibleProperty
}

[samreid]

This has been a productive conversation that I think will apply many places outside of this context. I do not believe that avoiding code duplication or preventing leaked attributes are essential at all costs, but in the absence of overriding concerns are good to strive for.

For this case, a strategy like the one in #55 (comment) could be combined with the following pattern:

class VertexFormViewProperties extends GQViewProperties {

  /**
   * @param {Object} [options]
   */
  constructor( options ) {

    options = _.extend( {
      equationForm: 'vertex',
      tandem: Tandem.required,
      hasAxisOfSymmetryVisibleProperty: false
    }, options );

    super( options );

    if ( options.hasAxisOfSymmetryVisibleProperty ) {

      // @public
      this.axisOfSymmetryVisibleProperty = new BooleanProperty( GQQueryParameters.checkAll, {
        tandem: options.tandem.createTandem( 'axisOfSymmetryVisibleProperty' ),
        phetioDocumentation: 'whether the axis of symmetry is visible'
      } );
    }

This would lead to 0 code duplication and 0 leaked attributes, without introducing other classes or a complicated hierarchy.

Whatever solution is selected for this issue, it would be good to raise this for discussion in an upcoming PhET-iO developer meeting to gauge the "cost" of leaked attributes or duplicated code, with respect to PhET-iO and non-PhET-iO maintainability.

[pixelzoom]

At the risk of starting another similar discussion... I also wanted to point out that checkboxes are duplicated that correspond to view Properties. See *GraphControls classes. The contents and order of these control panels has changed continuously during development, and trying to factor out base classes to reuse checkboxes would have been more complicated than simply duplicating them. So I'm leaning toward using the same approach (duplication) in both the model and view.

[pixelzoom]

Would the approach in #55 (comment) (GQViewProperties contains view Properties used in >1 screen) be acceptable if I added phetioDocumentation to instantiation of the GQViewProperties subclasses? E.g.:

const viewProperties = new ExploreViewProperties( {
  tandem: tandem.createTandem( 'viewProperties' ),
  phetioDocumentation: 'Properties that are specific to the view, some of which may be unused by this screen'
} );

EDIT: Or I suppose I could add this phetioDocumentation in the GQViewProperties base class, rather than duplicating it for instantiation of the 4 different subclasses?

[samreid]

The contents and order of these control panels has changed continuously during development, and trying to factor out base classes to reuse checkboxes would have been more complicated than simply duplicating them

During initial development it is sometimes efficient to copy/paste code to remain agile and quickly respond to design changes, but as the design of the simulation settles down and the simulation moves into a long term-maintainability (and now stable API) phase, it is often good to deduplicate code.

[samreid]

Would the approach in #55 (comment) (GQViewProperties contains view Properties used in >1 screen) be acceptable if I added phetioDocumentation to instantiation of the GQViewProperties subclasses?

There has been no discussion of the proposal in #55 (comment) which claims to have "0 code duplication and 0 leaked attributes, without introducing other classes or a complicated hierarchy."

[pixelzoom]

Indeed. But in this case, I feel there's little to be gained from factoring out a few duplicated checkboxes, due to the contents and order of the controls. Same argument as for the view Properties. So I'm going to stick with the current implementation.

[pixelzoom]

Reopening. During code review #43, @chrisklus added this REVIEW comment to VertexFormViewProperties:

     //REVIEW: Since this Property is exactly duplicated in FocusAndDirectrixViewProperties.js, should there be comments
      //that note that the documentation should stay in sync? Along with the other duplicated view Properties, like
      //axisOfSymmetryVisibleProperty and coordinatesVisibleProperty.
      // @public
      this.vertexVisibleProperty = new BooleanProperty( true, {
        tandem: options.tandem.createTandem( 'vertexVisibleProperty' ),
        phetioDocumentation: 'whether the vertex manipulator is visible'
      } );

Since this issue apparently won't die, I'm going to cave in and make what I consider to be a bad tradeoff. Standby.

[pixelzoom]

In the above commit, I changed the semantics of GQViewProperties. Instead of containing Properties that are common to all screens, it now contains Properties that are common to more than one screen. Any Property that is optional (e.g. vertexVisibleProperty) is instantiated only if an initial value is provided via options (e.g. vertexVisible: true or vertexVisible: false). This implementation removes duplication without resorting to a convoluted/unnatural type hierarchy.

@chrisklus please review.

[pixelzoom]

I should also mention that a casualty of this change was the checkAll query parameter, which I found to be very useful during development. It automatically sets all checkboxes to checked on startup. It wasn't worth the effort to make it survive this refactor, so I removed it.

[samreid]

@chrisklus, @zepumph and I touched base on this issue during PhET-iO meeting. We concluded that this is more of a proper Code Review issue and no longer something solely for PhET-iO, so I'll remove that label and let @chrisklus and @pixelzoom proceed at their convenience.

[chrisklus]

Looks good @pixelzoom! Ready to close if you are.