change behavior of 'Curve' check box

[pixelzoom]

Toggling the 'Curve' check box from on to off currently resets the values of the sliders associated with 'Adjustable fit'. Is that really what you want?

[amanda-phet]

Probably not. It doesn't currently reset the curve type (linear, etc.) so it shouldn't reset the sliders either. So let's specify the behavior a bit more clearly:

When unchecked, the state of the residuals checkbox (checked or unchecked), the curve type (radio button selection), and the adjustable fit sliders (locations), are all "remembered". When checked again, these items return to their previous selections.

Is that do-able, or is that too much to "remember"?

[pixelzoom]

That sounds perfectly reasonable, and what I would expect. I.e. the "Curve" check box only controls visibility, it doesn't reset the curve and settings related to the curve.

[memo330179]

The sim no longer resets when the curve view is toggled. It does still update curve fit when the curve is made visible again. It looks like the curve itself is not updated by new points unless it is visible.

[pixelzoom]

@memo330179 Is this ready for me to review, or is there more to be done?

[memo330179]

I don't think this one is ready yet. I removed the functionality of resetting the curve fit when the curve view is toggled. I need some more time to completely separate the view and the model. I opened issue #103, and @veillette responded to it. That will need to be implemented so that the simulation doesn't slow down.

[memo330179]

I think this is ready for review. I thought I had to make the fit update even when curve visibility was off, but now I realize that doesn't make sense.

[pixelzoom]

The requirements in https://github.com/phetsims/curve-fitting/issues/89#issuecomment-235999000 were:

When unchecked, the state of the residuals checkbox (checked or unchecked), the curve type (radio button selection), and the adjustable fit sliders (locations), are all "remembered". When checked again, these items return to their previous selections.

The state of the "Residuals" check box is not being restored.

[memo330179]

I added a line on GraphAreaNode.js

curve.isVisibleProperty.linkAttribute( residualsPath, 'visible' );

that makes the residuals invisible when the curve is invisible. I also removed the link that sets the residualsVisibleProperty to false when the curve is made invisible.

assigning @pixelzoom for review

[pixelzoom]

Still not quite right, two problems:

(1) The 'Residuals' check box should be enabled only when 'Curve' is checked. 'Residuals' is currently enabled all of the time, allowing you to interact with it when 'Curve' is not checked.

(2) When 'Curve' is unchecked, 'Residuals' should be unchecked. When 'Curve' is checked, 'Residuals' should be restored to the state it had the last time that 'Curve' was checked.

Here's the test sequence to verify that the 2 problems above are fixed:

• check 'Curve' • check 'Residuals' • uncheck 'Curve' -- 'Residuals' becomes disabled and unchecked • check 'Curve' -- 'Residuals' becomes disabled and checked • uncheck 'Residuals • uncheck 'Curve' -- 'Residuals' becomes disabled and remains unchecked • check 'Curve' -- 'Residuals' become enabled and remains unchecked

[memo330179]

I see. I accidentally removed the line that enables and disables the checkbox. I was trying to do it without creating a storage variable that 'remembers' the state of the checkbox. Do you recommend adding that variable in? The sim currently, disregarding point 1, does the behavior except visibly checking and unchecking the residuals checkbox.

[pixelzoom]

The sim currently, disregarding point 1, does the behavior except visibly checking and unchecking the residuals checkbox.

Running the test sequence described in https://github.com/phetsims/curve-fitting/issues/89#issuecomment-261582940, I don't see it saving and restoring the state of the 'Residuals' check box. Where is that handled?

[memo330179]

I restored the line that enables and disables the residuals checkbox This is the way it is currently working:

• check 'Curve'
• check 'Residuals'
• add three or more points to see residuals
• uncheck 'Curve' -- 'Residuals' remain checked. 'Residuals' become disabled. Residuals path become invisible.
• check 'Curve' -- 'Residuals' remain checked. 'Residuals' become enabled. Residuals path become visible
• uncheck 'Residuals'
• uncheck 'Curve' -- 'Residuals' becomes disabled and remains unchecked
• check 'Curve' -- 'Residuals' become enabled and remains unchecked

So the things that needs to be done is visibly checking and unchecking 'Residuals' on 'Curve' change. This behavior will, I think, require a storage variable to hold residualsVisibleProperty's value when 'Curve' is unchecked.

EDIT: I supposed it is not still the desired behavior as the color changes from blue to gray. I will have to look into that if this change is sufficient.

[pixelzoom]

Yes, a local state variable is needed - see wereResidualsVisible in the above commit.

Note that this also makes this line unnecessary in GraphNode, so I've removed it:

curve.isVisibleProperty.linkAttribute( residualsPath, 'visible' );

[memo330179]

Thank you that makes sense to me.

[pixelzoom]

It was difficult to describe how to do this without actually doing it, so I hope you don't mind that I made the change.

I think we can close this issue, do you agree?

[memo330179]

That's alright. You explained it well. I was just trying to get as much clarification as possible. I do think the issue can be closed.