search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
http://scenerystack.org/
MIT License
5 stars 12 forks source link

AquaRadioButton and RadioButtonGroup are instrumented differently #550

Open pixelzoom opened 4 years ago

pixelzoom commented 4 years ago

Noted while working on https://github.com/phetsims/sun/issues/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 https://github.com/phetsims/sun/issues/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 https://github.com/phetsims/sun/issues/549.

AquaRadioButton example from NATURAL_SELECTION/GraphsRadioButtonGroup:

screenshot_30

RadioButtonGroup button example from NATURAL_SELECTION/EnvironmentRadioButtonGroup:

screenshot_31
chrisklus commented 4 years ago

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 commented 4 years ago

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:

screenshot_40

RadioButtonGroup button example from NATURAL_SELECTION/EnvironmentRadioButtonGroup:

screenshot_39
zepumph commented 4 years ago

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 https://github.com/phetsims/sun/issues/550#issue-539375861 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 commented 4 years ago

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 https://github.com/phetsims/sun/issues/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 https://github.com/phetsims/sun/issues/523#issuecomment-545994022 (unify radio button group implementations). In the meantime, I'll unassign this issue.

zepumph commented 4 years ago

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 commented 4 years ago
  1. 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 commented 4 years ago

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 https://github.com/phetsims/sun/issues/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.