Convert components to use EnabledComponent mixin

[zepumph]

From #257, once that issue is complete, and EnabledComponent is "production ready", we should begin swapping out custom enabledProperty instances for this mixin. Hopefully each of these will be minimally invasive, because the api should likely not change (in most cases at least).

Here is a list of components to update. Most probably deserve dedicated issues, but this issue can serve as a central location for the status of the conversion as a whole.

sun:

• AquaRadioButtonGroup.js (addition not conversion, see add enabledProperty to AquaRadioButtonGroup #582)
• AquaRadioButton.js
• Checkbox.js
• ComboBox.js
• NumberSpinner.js
• Slider.js
• RadioButtonGroup.js
• ButtonModel.js (this one may be a bit tricky, waiting on the outcome of the creation issue to see if this is supported)
• RoundButtonView.js and RectangularButtonView. These likely will use the solution for ButtonModel, but maybe they will want their own?

scenery-phet:

• EyeDropperNode
• FineCoarseSpinner
• GaugeNode
• LaserPointerNode
• NumberControl
• NumberPicker
• TimeControlNode

tambo:

• AmplitudeModulator

I will mark this for dev meeting once this is ready.

[zepumph]

One thing that we need to make sure to do as part of regression testing is to look at interrupting input when toggling enabled. This has been added to EnabledComponent, but isn't is most custom usages across the project.

over in #257 (comment) @jbphet said:

There are TODO items about whether to call interruptSubtree and whether to set the cursor in the handler for changes to the enabled state. Both seem like good ideas to me. I'd suggest doing them, and setting up a QA task to test this - with an emphasis on multi-touch - to see whether there are any subtle implications that aren't immediately obvious to us.

I agree, but think it will be best to do this as a batch here (or in component specific sub issues) when components are converted as opposed to in #257.

• Test interruptSubtree regression with QA

[zepumph]

From #257 (comment):

• Note that when converting AquaRadioButton.js, the api for it has changed, as we don't support a default value passed in via options.enabled. If that proves too challenging, we can add it, but it is nice to try to keep a simpler api to begin with.

UPDATE: this support was added in d640f5c

[pixelzoom]

@ariel-phet asked me to take this on. The goal is to have enabledProperty supported uniformly across PhET UI components, so that opacityProperty and pickableProperty can be (by default) uninstrumented for PhET-iO.

I will be looking through other repos for similar/related issues, and will self-assign as I find them.

[zepumph]

For clarity, those phet-io Properties are scheduled for uninstrumentation (by default) over in phetsims/scenery#1047, and I do not think of this issue's work as a precursor to that. Most cases already have an enabledProperty specified that is PhET-iO instrumented, and so the conversion over here will not actually change the API significantly (or at all).

For PhET-iO, I am not aware of cases where we have elected to tell the client to use opacity/pickable instead of manually adding an instrumented enabledProperty to come UI component.

This comment shouldn't change your workflow, I say this only to increase understanding of this work being done and the dependencies (or lack there of) that it has on each other.

[pixelzoom]

While working on another issue, I discovered that in addition to EnabledComponent, there is EnabledNode and EnabledPhetioObject. They are all documented the same, all authored by @zepumph, and all updated recently. @zepumph can you please clarify (or better yet document) the relationship between these 3 classes, and when each one should be used?

[zepumph]

EnabledComponent and its hierarchy is out for review over in #257 and I responded to your concerns there, see #257 (comment). This issue should be on hold until that issue has been reviewed. I did work here for a specific PhET-iO requirement. Marking back on hold until that issue is reviewed.

[zepumph]

I have no problem with c3f449b and others like it, but was wondering if you could explain your rationale.

[zepumph]

I looked through every commit, I also checked out the new features in the scenery-phet demo. I then ran a couple of sims that used these components and played around with setting their enabledProperties, and the same when passing in an enabledProperty.

In general I am totally ecstatic about these changes. The above commits removed so much duplication! Thank you so much @pixelzoom for taking this on. This may be the final manifestation of how we support enabled on components at PhET, or things may develop further in phetsims/scenery#1100. Either way this issue was a huge step towards our end goal. Thanks!

Back to you.

[zepumph]

• Test interruptSubtree regression with QA

@pixelzoom can you also please comment on this, do you think this is necessary? If so how should we set that up? What tests may be good to do?

[pixelzoom]

I have no problem with c3f449b and others like it, but was wondering if you could explain your rationale.

AccessibleSlider uses the enabledProperty created by EnabledNode. So initializeEnabledNode must be called before initializeAccessibleSlider. Therefore it seems like good hygiene to call mixInto in the same order.

[pixelzoom]

• Test interruptSubtree regression with QA

@pixelzoom can you also please comment on this, do you think this is necessary? If so how should we set that up? What tests may be good to do?

In general, developers and QA should be verifying that "in progress" interactions are cancelled when the target of the interaction becomes disabled. I doubt that's happening, and I don't know how to make that happen other than a PSA/reminder.

If there are any regressions introduced, they might be caught by CT, assuming that fuzzing uses multitouch.

[zepumph]

If there are any regressions introduced, they might be caught by CT, assuming that fuzzing uses multitouch.

That will be added as part of phetsims/aqua#106.

In general, developers and QA should be verifying that "in progress" interactions are cancelled when the target of the interaction becomes disabled. I doubt that's happening, and I don't know how to make that happen other than a PSA/reminder.

I agree, but also know that multi-touch testing is often not done (for cost saving). Perhaps a discussion point on the dev meeting agenda? I would also be fine with catching things in CT and calling it good. Up to you!

[pixelzoom]

I agree, but also know that multi-touch testing is often not done (for cost saving).

Not done by whom? Developers, QA, both? Imo there's not much cost if the developer checks this "as you go" -- it's just a good habit. And it shouldn't be QA's responsibility to do all of the multi-touch testing.

[pixelzoom]

Assigning to @ariel-phet to decide if he wants to do something about (a) testing for interruptSubtreeInput regressions, and (b) clarifying (in general) what multi-touch testing should be done and by whom.

[zepumph]

I agree, but also know that multi-touch testing is often not done (for cost saving).

Not done by whom? Developers, QA, both? Imo there's not much cost if the developer checks this "as you go" -- it's just a good habit. And it shouldn't be QA's responsibility to do all of the multi-touch testing.

In that message I was speaking to QA personally. I know that a lot of devs don't have access to multi-touch devices, and it likely isn't in many people's workflow (it definitely isn't in mine). Perhaps a psa at dev meeting would be nice.

[ariel-phet]

@pixelzoom I think doing a regression test would be good. Can you make a QA issue with some suggestions of what to test/how (not immediately clearly to me) and place it in the pipeline project (I will triage, but I think there is room to test immediately)

Could you also place a PSA in this week's dev meeting notes and we can discuss. I can make a note or two in the dev meeting doc on your PSA.

[pixelzoom]

Discussed on Zoom with @ariel-phet. I really don't know how to specify this as a task for QA. So what we decided is: I'll identify which classes need to be tested, create an issue in QA repo, and assign to @ariel-phet. That issue is phetsims/qa#572.

Closing.