search

phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

Link `coordinatesNode.visibleProperty` to `viewProperties.coordinatesVisibleProperty`, delete section from Client Requests guide. #171

Closed KatieWoe closed 3 years ago

KatieWoe commented 3 years ago

For https://github.com/phetsims/qa/issues/700. I noticed that for the "Hide Coordinates of the Vertex" section, the Standard Form screen did not have instructions, even though it has a vertex and coordinates. I also noticed that vertex form screen did not have the checkbox enabled option but the focus and directrix screen did.

moreinfo standardform
amanda-phet commented 3 years ago

Thanks @KatieWoe . I just reviewed some other recent client request files and I see that Amy has been outlining the properties by screen, so I can do that. I used those two screens as examples because the instructional designer might achieve the goal in two different way, and Standard Form screen seemed repetitive, but I can add that now since we'll be doing another RC anyway.

pixelzoom commented 3 years ago

@amanda-phet I'm assuming that you're going to address this for the 1.2 release, so I'm labeling as "blocks-sim-publication". When you've made the change, please assign to me for cherry-picking.

amanda-phet commented 3 years ago

OK I made this as comprehensive as possible and added all of the relevant properties. @pixelzoom the commit should be right above this comment.

pixelzoom commented 3 years ago

Patched in the 1.2 branch, ready for testing in the next RC. To test:

  1. Go to the Wrapper index.
  2. Click on the "Client Requests" link.
  3. The last section in the document should look like this:
screenshot_1227
KatieWoe commented 3 years ago

The coordinatesNode does not seem to be featured. It also does not change the state of the checkbox. Is this alright?

KatieWoe commented 3 years ago

Video of https://github.com/phetsims/graphing-quadratics/issues/171#issuecomment-909566884 checkboxuncheck For https://github.com/phetsims/qa/issues/704

KatieWoe commented 3 years ago

Otherwise this looks ok

pixelzoom commented 3 years ago

The coordinatesNode does not seem to be featured.

It does seem like if it's important enough to be described in Client Requests guide, then it should be featured. But that's up to @amanda-phet. If you want to feature it, please edit overrides.js, and let me know that I need to cherry-pick.

It also does not change the state of the checkbox. Is this alright?

No, this is a problem. All elements named coordinatesNode.visibleProperty currently have their own visibleProperty, which is not connected to viewProperties.coordinatesVisibleProperty. You can change coordinatesNode.visibleProperty for one manipulator, and then overwrite it by toggling viewProperties.coordinatesVisibleProperty.

Options for addressing this, from least to most cost:

(1) Do nothing. (2) Set all coordinatesNode.visibleProperty to phetioReadOnly: true. This would work fine, will take ~15 minutes. (3) Pass viewProperties.coordinatesVisibleProperty through to each coordinatesNode. Each coordinatesNode will then have a link back to viewProperties.coordinatesVisibleProperty in Studio. This is the most current pattern, more programming involved, ~1 hour.

I recommend (3). @amanda-phet is that OK with you?

pixelzoom commented 3 years ago

(3) actually turned out to be pretty easy, ~15 minutes of work. But...

@amanda-phet if we do (2) or (3) in the comment above, then you'll need to rewrite this section of the Client Guide again. The client will not (and should not!) be able to set coordinatesNode.visibleProperty for manipulators and points, like the vertex. The visibility of all coordinates will be linked to viewProperties.coordinatesVisibleProperty, which is the Property controlled by the "Coordinates" checkbox.

Note to self, here's the patch to implement this:

Patch ```diff Index: phet-io/api/graphing-quadratics.json IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io/api/graphing-quadratics.json b/phet-io/api/graphing-quadratics.json --- a/phet-io/api/graphing-quadratics.json (revision e8149147a9827580de81a9339ff3d556e0de36c0) +++ b/phet-io/api/graphing-quadratics.json (date 1630545613803) @@ -3268,14 +3268,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.focusAndDirectrixScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, @@ -4008,14 +4006,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.focusAndDirectrixScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, @@ -4222,14 +4218,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.focusAndDirectrixScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, @@ -7955,14 +7949,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.standardFormScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, @@ -8011,14 +8003,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.standardFormScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, @@ -8064,14 +8054,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.standardFormScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, @@ -9705,14 +9693,12 @@ "visibleProperty": { "_data": { "initialState": { - "units": null, - "validValues": null, - "value": true + "elementID": "graphingQuadratics.vertexFormScreen.view.viewProperties.coordinatesVisibleProperty" } }, "_metadata": { - "phetioDocumentation": "Controls whether the Node will be visible (and interactive).", - "phetioTypeName": "PropertyIO" + "phetioReadOnly": true, + "phetioTypeName": "LinkedElementIO" } } }, Index: graphing-quadratics/js/standardform/view/PointNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/graphing-quadratics/js/standardform/view/PointNode.js b/graphing-quadratics/js/standardform/view/PointNode.js --- a/graphing-quadratics/js/standardform/view/PointNode.js (revision 5daae58dfc24d71ecca15a39096f90e14f0da8cd) +++ b/graphing-quadratics/js/standardform/view/PointNode.js (date 1630545456270) @@ -60,6 +60,7 @@ backgroundColor: options.coordinatesBackgroundColor, foregroundColor: options.coordinatesForegroundColor, decimals: options.coordinatesDecimals, + visibleProperty: coordinatesVisibleProperty, tandem: options.tandem.createTandem( 'coordinatesNode' ), phetioDocumentation: 'coordinates displayed on this point' } ); @@ -76,7 +77,6 @@ // visibility coordinatesVisibleProperty.link( visible => { - coordinatesNode.visible = visible; if ( visible ) { options.layoutCoordinates( coordinatesProperty.value, coordinatesNode, pointNode ); } Index: graphing-quadratics/js/common/view/GQManipulator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/graphing-quadratics/js/common/view/GQManipulator.js b/graphing-quadratics/js/common/view/GQManipulator.js --- a/graphing-quadratics/js/common/view/GQManipulator.js (revision 5daae58dfc24d71ecca15a39096f90e14f0da8cd) +++ b/graphing-quadratics/js/common/view/GQManipulator.js (date 1630545215952) @@ -63,6 +63,7 @@ backgroundColor: options.coordinatesBackgroundColor, foregroundColor: options.coordinatesForegroundColor, decimals: options.coordinatesDecimals, + visibleProperty: coordinatesVisibleProperty, pickable: false, tandem: options.tandem.createTandem( 'coordinatesNode' ), phetioDocumentation: 'coordinates displayed on this manipulator' @@ -76,7 +77,6 @@ // visibility coordinatesVisibleProperty.link( visible => { - coordinatesNode.visible = visible; if ( visible ) { options.layoutCoordinates( coordinatesProperty.value, coordinatesNode, actualRadius ); } ```
pixelzoom commented 3 years ago

@amanda-phet and I discussed on Zoom.

I should definitely link each coordinatesNode.visibleProperty to viewProperties.coordinatesVisibleProperty, since that's the new PhET-iO pattern. To not do so here would be wrong.

It's unclear to me what the goal of "Hide coordinates of the vertex" is. Does it mean to control the visiblity of vertex coordinates independently from the visibility of all other coordinates? Does it apply only to the vertex (why?) or to potentially any point's coordinates? If independent control is indeed needed, then there is more work to be done here -- any coordinate whose visibility needs to be controlled independently will need to be wrapped in a new Node, whose visibleProperty can be used to independently hide those coordinates.

Give the amount of work involved... If "Hide coordinates of the vertex" was not actually requested by a client, and was something that PhET thought would be nice to have, then I recommend that we delete this section from the Client Requests guide, and wait until a client asks for something like this.

amanda-phet commented 3 years ago

I discussed this with @kathy-phet and @arouinfar and it seems like it makes the most sense to follow current patterns and go with option (3).

We confirmed that there is no actual client request for this feature, so I will remove it from the client guide.

If a client does want this functionality in the future (to be able to hide specific coordinate and not all coordinates), it sounds like we would need up to up to 3 new properties per screen (3 for the Focus & Directrix screen, but fewer for previous screens) that combine with viewProperties.coordinatesVisibleProperty to control visibility. This means the individual coordinates would need to be true AND viewProperties.coordinatesVisibleProperty would need to be true to display the specific coordinates. We can return to this if a client requests this in the future.

I think this is in line with what you said here:

If independent control is indeed needed, then there is more work to be done here -- any coordinate whose visibility needs to be controlled independently will need to be wrapped in a new Node, whose visibleProperty can be used to independently hide those coordinates.

pixelzoom commented 3 years ago

Yes, your understanding of the future plan is correct.

I'll cherry-pick the Client Guide change, then proceed with applying the patch that I created above.

pixelzoom commented 3 years ago

Done in the above commits, for master and 1.2 branches.

@amanda-phet please verify in master. Run the sim in Studio, inspect the 7 instances of coordinatesNode.visibleProperty. You should see that they are linked to viewProperty.coordinatesVisibleProperty. And if you toggle viewProperty.coordinatesVisibleProperty, you should see the visibility of all coordinates change, for static points, and interactive manipulators.

amanda-phet commented 3 years ago

That all looks correct to me.

pixelzoom commented 3 years ago

Ready for testing in the next RC.

To verify:

KatieWoe commented 3 years ago

It seems to be viewProperties.coordinatesVisibleProperty instead. Is that alright?

KatieWoe commented 3 years ago

Otherwise this looks ok. If https://github.com/phetsims/graphing-quadratics/issues/171#issuecomment-915406793 is alright, feel free to close.

amanda-phet commented 3 years ago

It seems to be viewProperties.coordinatesVisibleProperty instead. Is that alright?

Yes, that's right @KatieWoe . I think it was just a typo.