apply phetioFeatured

[samreid]

In #14 (comment) we identified the PhET-iO features we wanted to support for this sim. In https://github.com/phetsims/phet-io-wrappers/issues/143 we introduced a new PhetioObject option to indicate featured instances. We should identify them as such:

• toggle visibility of the equation that appears above sliders
• disable sliders individually
• disable pickers individually (implemented via pickable:false, should be redone with setEnabled())
• hide checkboxes individually
• hide point tools individually
• hide save and erase button as a group, but not individually
• hide complete accordion box
• hide complete control panel
• get full state of Quadratic model element
• instrument all view properties (GQViewProperties and subtypes)
• instrument drag handlers for manipulators
• instrument slider drag handlers
• do not instrument graph

This is not essential for 1.0. Testing this requires the studioc branch of phet-io-wrappers. Tagging @pixelzoom so he is aware I'm looking into this.

[pixelzoom]

Is this a prerequisite for 1.0 release?

[pixelzoom]

@samreid said:

In phetsims/phet-io-wrappers#143 we introduced a new PhetioObject option to indicate featured instances.

What are " featured instances"? I see no description of the term here or in phetsims/phet-io-wrappers#143.

In PhetioObject, I see:

phetioFeatured: false // True if this is an important instance to be "featured" in the PhET-iO API

That helps a little, but how and where is an instance "featured" in the API?

[pixelzoom]

Looking through the above commits, phetsims/sun@44e0a69 is a bit of a red flag. It adds Slider option enabledPropertyOptions, which is ignored if the client sets option enabledProperty. We have lots of places in common code where a component instantiates a default Property if the client does not provide one. Will we need to use this pattern in all of those cases?

Side note... Slider's default for options.enabledProperty should be changed from Property to BooleanProperty.

[samreid]

Is this a prerequisite for 1.0 release?

This was addressed in the initial comment where it says "This is not essential for 1.0."

That helps a little, but how and where is it featured in the API?

The new version of studio (in the branch referred to in https://github.com/phetsims/phet-io-wrappers/issues/143 has an option to only show "Featured" items.

Looking through the above commits, phetsims/sun@44e0a69 is a bit of a red flag.

We should add checks that enabledPropertyOptions and enabledProperty options are mutually exclusive.

Will we need to use this pattern in all of those cases?

Perhaps we would need to use this pattern for instrumented subcomponents that need to be customizable, which can be created or passed in.

Side note... Slider's default for options.enabledProperty should be changed from Property to BooleanProperty.

Sounds good to me!

[pixelzoom]

I've made related Slider improvements in phetsims/sun#418.

[pixelzoom]

After today's PhET-iO dev call with @kathy-phet, the decision was made to rollout Graphing Quadratics 1.0 with the redesigned Studio that has the "Featured" feature. So this issue needs to be completed. Priority raised to high.

[samreid]

instrument drag handlers for manipulators

What are the "manipulators"?

UPDATE: I discovered VertexManipulator, FocusManipulator and PointOnParabolaManipulator. I passed phetioFeatured: true to their input listeners, but it is not cascading to the nested emitters. I may need to resort to phetioComponentOptions again...

[pixelzoom]

... I may need to resort to phetioComponentOptions again...

Gross.

[samreid]

Oh wait, there is a more palatable established pattern for this case... Hold for commits.

[samreid]

instrument slider drag handlers

This is instrumented but not "featured". But the associated model properties are already featured. It is really important to feature the slider input listener? If we do make the slider thumb drag handler featured, do we also want to make other slider input listeners featured, such as clicking in the track?

[kathy-phet]

@samreid and @pixelzoom - I'm feeling like this list is what we wanted to instrument for this sim, but not necessarily what we might want to feature? Feature seems like it would be the most basic set of things we would imagine someone wanting to do. Some of these were for "advanced use" cases. I'm starting to feel like a PhET-iO design includes both the full brainstorm and then the "what should be featured" list out of that list.

[pixelzoom]

@kathy-phet said:

... I'm starting to feel like a PhET-iO design includes both the full brainstorm and then the "what should be featured" list out of that list.

Agreed. But the concept of "featured" didn't exist when the PhET-iO design was done. Suggestions on how to proceed? Can what is "featured" be reviewed/revised as part of the general PhET-iO review to be done in #115?

[kathy-phet]

Is there space in Design Meeting this week to refine the list for "featured"?

[pixelzoom]

The first thing would be to make sure that @amanda-phet is aware that the "Featured" feature exists, so she has some time to think about it. As far as I know, she has been out of the loop.

[amanda-phet]

Thanks for looping me in, @pixelzoom ! I'm happy to have a design meeting about this. Is this the first sim that has a featured list? If so, we might want to invite Amy as well so we can both learn more about PhET-iO design (since we are responsible for contributing to these kinds of decisions).

[kathy-phet]

Yes, first sim with a featured list. Kathy

…

Sent from my iPhone On Nov 27, 2018, at 8:14 PM, Amanda McGarry <[email protected]<mailto:[email protected]>> wrote: Thanks for looping me in, @pixelzoom<https://github.com/pixelzoom> ! I'm happy to have a design meeting about this. Is this the first sim that has a featured list? If so, we might want to invite Amy as well so we can both learn more about PhET-iO design (since we are responsible for contributing to these kinds of decisions). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#112 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AE3FZDPWQmSqrxYB7fIcu4BF0caDKZR6ks5uzf90gaJpZM4YngeI>.

[pixelzoom]

PhET-iO version has been deferred and will not be published with 1.0, see #101 (comment). So this issue can be deferred.

[pixelzoom]

What is the next step for this issue?

Seems like we should ask @kathy-phet whether she wants to review.

[zepumph]

Yes that sounds good. I'll mark for phet-io meeting to make sure this doesn't fall off our list. Removing assignment.

[arnabp]

@kathy-phet will be taking a look at this tomorrow (6/21)

[samreid]

@kathy-phet provided notes on this via slack.

[pixelzoom]

@samreid Can you please add @kathy-phet's slack notes here, for posterity?

[samreid]

@kathy-phet said:

Hi All, I discussed the changes in the PhET-iO Feature defaults that happened last week around GQ in the GQIO work. At the last meeting I attended around the common code feature discussion, I recall discussing featuring visible and enabled for as many controls as possible as a rule - for consistency of what is available and flexibility for the instructional designer. And only un-featuring in very special cases. The current approach of just highlighting one for each control doesn't reach one of the goals for the PhET-iO feature set - specifically the use case for Smart Sparrows integration into their platform that I think is a powerful use case.

Apologies for having to reopen this discussion. Can we meet on Wednesday [..] to revisit these changes?

[samreid]

@chrisklus recalls a meeting (where @kathy-phet was not present) where @amanda-phet advocated for keeping "featured" as sparse as possible, and we wanted to make a distinction between (a) controls that are solely used for action and (b) controls that also serve as readouts. For controls that serve as readouts, such as sliders, spinners, radio buttons, they would default to featuring "enabledProperty". For controls that serve only as inputs, such as buttons, they would default to featuring "visibleProperty". We cannot find a paper trail on this, but that is our recollection.

[kathy-phet]

Yes, this is what was discussed when I was out of town. There are competing goals here - and Amanda and I discussed these tensions. This is why we are planning on having a meeting on Wednesday - to discuss with the PhET-iO team. My preference is to trust the clients to be smart about their configurations (or at least take responsibility for their own choices), rather than trying to limit them, - which prioritizes consistency and flexibility of these featured items.

[pixelzoom]

Just to add to the collective recollection...

We concluded (on 1/6, I believe) that featuring visibleProperty and not featuring enabledProperty in general was a better strategy because:

(1) visibleProperty is typically more useful than enabledProperty when considering UI components in general.

(2) It reduces noise in the "Featured" view of tree, which makes that view easier to navigate.

(3) We wouldn't be limiting the client's choices or capabilities by doing this, because enabledProperty is still available in the "All" tree view. So this isn't about trusting clients or limiting what they can do, it's about how to prune a huge navigation space.

I still feel prefer this conclusion, but feel free to discuss/change without me.

[pixelzoom]

Another reason for not featuring enabledProperty right now is the fact that PhET does not have a consistent API for enabledProperty. Every UI component (including buttons) is a little different, and we've identified that this needs to be revised and unified. See for example phetsims/sun#257 (comment) and phetsims/sun#515. So consistently featuring enabledProperty is currently tricky, it's in code that will be revised, and it should be easier to feature after revising.

[samreid]

This is why we are planning on having a meeting on Wednesday - to discuss with the PhET-iO team.

Has this meeting been scheduled yet? I'm not seeing it on the calendar.

[zepumph]

Has this meeting been scheduled yet? I'm not seeing it on the calendar.

We had the meeting this morning. During the meeting we decided that all ui components would have phetioFeatured enabled and visibleProperties until we got feedback from clients informing us that things should be done otherwise. In https://github.com/phetsims/phet-io/issues/1517 I implemented that. @kathy-phet and @amanda-phet it is now over to use to make the necessary overrides updates to GQ.

[zepumph]

@kathy-phet, @chrisklus, and I discussed the next steps here since https://github.com/phetsims/phet-io/issues/1517 was implemented. We were able to build a version before those commits, and then compare it to master with the diff wrapper. Here was the output: GQ diff.pdf

From here @kathy-phet will review the changes, and then work in studio to update anything that is needed. She will then reach out to @amanda-phet, @zepumph, or @chrisklus for help as needed in committing the changes.

[zepumph]

@kathy-phet made changes to the overrides in response to the common code updates. Committed above. I think that this is the final phetioFeatured update. I'm not sure what is left for this issue. Perhaps it is ready for a close. @pixelzoom want to give things a once over?

[kathy-phet]

@zepumph - with those commited is phettest now up to date so I could look at studio once more and review all?

[zepumph]

I'm not sure if it was, but I just pulled phettest and the answer is NOW yes.

[pixelzoom]

I've been out of the loop for design decisions, so I will assume that others have verified that metadata matches what is desired for GQ. I ran graphing-quadratics in phetmarks with Mode=studio, and I don't see any obvious problems.

In #112 (comment), it sounds like @kathy-phet would like to review one more time. So I will assign to @kathy-phet and @zepumph to move this issue to closure.

[zepumph]

Thanks for the once over. @kathy-phet and I have reviewed this in the past week. Closing.