AquaRadioButton and RadioButtonGroup are instrumented differently

[pixelzoom]

Noted while working on #549, and Natural Selection, which has both types of radio buttons.

AquaRadioButton and the buttons created by RadioButtonGroup are instrumented differently. This is another impediment to unifying the implementations of radio buttons and their groups, see #523.

The differences are:

(1) AquaRadioButton input listener has tandem name 'inputListener', RadioButtonGroup is 'pressListener'.
(2) RadioButtonGroup has 'firedEmitter', which is absent from AquaRadioButton.
(3) RadioButtonGroup lacks '*property', the link to its associated Property, which is being addressed in #549.

AquaRadioButton example from NATURAL_SELECTION/GraphsRadioButtonGroup:

RadioButtonGroup button example from NATURAL_SELECTION/EnvironmentRadioButtonGroup:

[chrisklus]

Discussed with @samreid, @zepumph, and @jonathanolson.

For (1), we renamed inputListener to fireListener in AquaRadioButton.js.

Regarding (2), a firedEmitter exists for AquaRadioButton, it's just nested under its fireListener.

[pixelzoom]

I'm trying to figure out what was actually accomplished here. It looks like all you did is rename the differences so that the APIs are still different. The example I used above is now:

AquaRadioButton example from NATURAL_SELECTION/GraphsRadioButtonGroup:

RadioButtonGroup button example from NATURAL_SELECTION/EnvironmentRadioButtonGroup:

[zepumph]

AquaRadioButton and RadioButtonGroup use completely different guts. In an effort to try to align them without completely refactoring them, we decided that this rename was an improvement.

The two items we were trying to solve were (1) and (2) from #550 (comment) since (3) will be covered in the other issue.

Since RadioButtonGroupMember is a sun button, it uses a PressListener, and is not able to use a FireListener, as a result we implemented out own fireEmitter (hence the different nesting of the fireEmitter). Though we know this isn't ideal, we could not come up with a way to have RadioButtonGroupMember use FireListener (the PressListener subtype). The rename recognizes that they use two different listeners, but also now since "fire" is in the listener name, it seems more clear that both have a fireEmitter, just in a different hierarchy.

Perhaps one day these two types will be rewritten to be unified. Then we will have the change to also unify the PhET-iO interfaces. After attempting to hammer these two very different implementation into the same PhET-iO api, we felt like this, though not ideal, was the best cost/benefit solution.

[pixelzoom]

Thanks for the clarification.

I don't think this issue should be closed -- these implementation do need to be unified, which is the subject of #523. And unfortunately that unification likely requires breaking every PhET-iO API that uses radio buttons.

I've added this issue to the list of issues to be addressed in #523 (comment) (unify radio button group implementations). In the meantime, I'll unassign this issue.

[zepumph]

Final thoughts on this issue before it is kicked off to be done with general radio button consolidation.

1. I really like the instrumentation of AquaRadioButton and its members. In the future I hope that RadioButtonGroup can be converted over to this pattern.
2. Because of this, I do not think this issue should block publication of Gravity Force Lab, which has a VerticalAquaRadioButtonGroup and no RadioButtonGroup.
3. Leaving unassigned.

[pixelzoom]

2. Because of this, I do not think this issue should block publication of Gravity Force Lab, which has a VerticalAquaRadioButtonGroup and no RadioButtonGroup.

You're assuming that the linked Property is going to continue to live at the group level. Imo that's not a done deal and needs to be revisited in #549. If it changes, that will break GFL API.

[zepumph]

Noting a conversation that @samreid and I had about the imminent publication of gravity force lab with phet-io. We feel like the current instrumentation of AquaRadioButton makes sense, and since consolidation over in #523 will take longer than the time line of GFL, we think that this should not block its publication. The future implementation of radio buttons should embody similar features of this implementation, and so even if the API changes, it will likely not be a 180 flip from the current instrumentation strategy.

[samreid]

Standardization of the linked element for AquaRadioButtonGroup and RectangularRadioButtonGroup was addressed in #920. @zepumph has been working on pruning the listeners/emitters and I asked him about it in #920 (comment)

So I think this issue can be reviewed with #920. @zepumph anything else to do here?

[arouinfar]

Not sure why the Monday bot assigned this to me, so unassigning myself.

[zepumph]

Things are looking really good here. They have now aligned, and are quite simple, each are composed of just a visibleProperty and a firedEmitter. Closing