Checkbox expects its associated Property to be instrumented

[pixelzoom]

Noted in phetsims/joist#569 (comment)

Checkbox currently expects its associated Property to be instrumented, so that it can create a linked element. That expectation is not checked, and I don't agree with it. It should be possible to pass an uninstrumented Property to a Checkbox, in which case it should not attempt to create a linked element.

[zepumph]

That expectation is not checked, and I don't agree with it.

When creating the linked elements in each individual component, we decided to not implement enabled Property checks totally and completely, since they should all be factored out into a mixin in progress.
The proof-of-concept full checks are only implemented in Slider:

    if ( assert && Tandem.PHET_IO_ENABLED && !ownsEnabledProperty ) {
      options.tandem.supplied && assert( options.enabledProperty.isPhetioInstrumented(),
        'enabledProperty must be instrumented if slider is' );

      // This may be too strict long term, but for now, each enabledProperty should be phetioFeatured
      assert( options.enabledProperty.phetioFeatured,
        'provided enabledProperty must be phetioFeatured' );
    }

Note these checks were created with the assumption that Tandem.required cannot be overridden. So we would probably want to add a tandem.supplied check to the surrounding if statement too. We only want to do these checks if the component is instrumented.

How do you feel about that? What is the use case for having an "either or" instrumentation where one of either Property or component is instrumented, but not both?

[pixelzoom]

I don't agree with the "proof-of-concept full checks" currently in Slider. Instrumenting a UI component should not require that its associated Properties are instrumented.

Whether this needs to be fixed in Checkbox depends on your timeframe for the "mixin in progress", which I have not previous heard of.

[zepumph]

the "mixin in progress", which I have not previous heard of.

I am speaking about the enabledProperty mixin in #257.

@pixelzoom could you illustrate your qualms with linking the instrumentation levels between component and the Property it runs on. For clarity please note that I am only talking about doing this for phet-io brand, when the component is instrumented.

[pixelzoom]

Have I no qualms with linked Properties -- if a Property is instrumented, by all means create a linked element. But I don't think it should be required. That's inflexible, and creates the type of problem described in phetsims/joist#569 (comment), where we want to feature ProjectorModeCheckbox, but not its internal Property.

[pixelzoom]

And why would the "enabledProperty" mixin be responsible for determining whether to create a linked element? This is related to (for example) Checkbox's property constructor parameter, and has nothing to do with enabledProperty.

[samreid]

That's inflexible, and creates the type of problem described in phetsims/joist#569 (comment), where we want to feature ProjectorModeCheckbox, but not its internal Property.

The comment referenced says:

I instrumented internal Property projectorModeEnabledProperty because Checkbox expects its associated Property to be instrumented.

This is a tradeoff between flexibility and uniformity. We discussed the advantage of every instrumented Checkbox having an instrumented Property. I haven't read the full thread of phetsims/joist#569 and I don't understand if it's essential to have the underlying Property be uninstrumented. If it is uninstrumented, how will phet-io read or change its value?

[pixelzoom]

phet-io does not need to read or change its value. That can be done via globals.colorProfile.profileNameProperty.

This came up in the context of phetsims/joist#572, whether to instrument the internal projectorModeEnabledProperty of ProjectorModeCheckbox. I had no choice but to instrument it in that case, and @jessegreenberg (rightfully imo) questioned that.

And regardless of the decision for ProjectorModeCheckbox, this is a general issue. Designers should be consulted on the "tradeoff between flexibility and uniformity".

[samreid]

Thanks @pixelzoom, that makes sense. @chrisklus and @zepumph and I will schedule this for discussion.

[pixelzoom]

@zepumph you assigned this to me again. What additional feedback do you need?

[zepumph]

Sorry I though I commented the below 1 hour ago, but it didn't go through:

I think I would understand this better, and contribute to further conversation more effectively, if I understood a case or two where the passed in Property shouldn't be instrumented for an instrumented Slider/Combobox/RadioButton etc.

[pixelzoom]

Have you looked at ProjectorModeCheckbox? I've been referencing it repeatedly as a concrete example.

[zepumph]

Yes I have indeed. I understand the intermediate nature of the Property, but from the Checkbox perspective. I see lots of added value in having some way of linking back to the Property that controls it. Here it seems like really what we wish for is a way to link back to the Property in ColorProfile. Without that linkage, it seems like the instrumentation of the Checkbox is incomplete. From the checkbox you wouldn't know how to change the value.

[jessegreenberg]

I ran into a case like this while using ComboBox. Like #518 (comment), I needed an internal adapter Property to drive the ComboBox. Since ComboBox required the adapter Property to be instrumented, there will be two Properties available that will control the same thing. It doesn't seem necessary to have both and it is possible that the instrumented adapter Property could add confusion.

[zepumph]

What about an option to ComboBox like phetioLinkedProperty that can be supplied as a override. If not provided, then it will use the passed in parameter. @jessegreenberg in your case would it be possible to get the actual parameter from the ComboBox call site?

[pixelzoom]

Why not just create the linkedElement only if the Property associated with the ComboBox is instrumented? If the Property is not instrumented, then no link.

[samreid]

In order to streamline the user experience in studio and make the API more understandable, it would be nice to generally map from instrumented UI component => instrumented Model Property. I'm not understanding the use case of an UI component without a corresponding instrumented Model Property (unless something is just drawn as an icon, but that sounds like an unusual case--like a button that has a checkbox icon). And I understand that sometimes the passed-in property is not the semantically meaningful one from a PhET-iO client perspective.

[pixelzoom]

In order to streamline the user experience in studio and make the API more understandable, would be nice to generally map from instrumented UI component => instrumented Model Property.

Exposing implementation details for the sake of consistency is not equivalent to making something more streamlined and understandable. The requirement to instrument a UI component's associated Property will (in some cases) expose an implementation detail, creating a PhET-iO interface that is not the recommended way of interfacing with the UI component, and creating a dependency that will result in a breaking change if the internals of the UI component are changed.

I'm not understanding the use case of an UI component without a corresponding instrumented Model Property ...

The issue is not that it doesn't have a corresponding model Property. The issue is that it's not desirable to expose the model Property because it's an internal implementation detail that is inappropriate to expose.

And I understand that sometimes the passed-in property is not the semantically meaningful one from a PhET-iO client perspective.

Please explain how exposing something that is "not semantically meaningful" will "streamline the user experience" and "make the API more understandable". These are contradictory statements.

When constructing a UI component based on one type of abstraction, it's useful to use more primitive UI components that are based on other abstractions. And in that case, an internal mapping may be needed between the abstractions. And it may be undesirable (or even confusing) to expose that internal mapping.

An example is ProjectorModeCheckbox. The abstraction there is "switch between 2 color profiles". It is constructed using Checkbox, whose abstraction is "switch between booleans true and false". There is no reason to expose the fact that there is an internal mapping between color profile and boolean.

I'm trying to understand why it's a big deal to have the option to not instrument the Property associated with a UI component, and therefore not create a linked element. In the case of Checkbox, it seems like a simple test of property before calling addLinkedElement. Or is there something deeper that I'm not aware of?

[samreid]

Sorry for the the confusion, in retrospect I can see how my comment is ambiguous. The proposal was to expose and link the semantically-relevant model property, not the implementation-based model property. I believe this is what @zepumph proposed in #518 (comment). In the case of ProjectorModeCheckbox, the exposed and linked model property would be the profileNameProperty. The projectorModeEnabledProperty is an implementation detail which would not be exposed or linked.

I'm trying to understand why it's a big deal to have the option to not instrument the Property associated with a UI component,

If neither the profileNameProperty nor the projectorModeEnabledProperty are instrumented, then the PhET-iO client would have no way to toggle the checkbox programmatically, or get its value, or save its value in the state, or see the value in the data stream. Instrumenting profileNameProperty solves all of these problems. Instrumenting projectorModeEnabledProperty solves all of these problems too, and leads to a uniform API across simulations (all checkboxes have a BooleanProperty). I'm not sure whether the uniformity is worth the cost of exposing an implementation detail, and I'd like to discuss further.

[pixelzoom]

Thanks for clarifying. Agreed, linking the semantically-relevant Property (profileNameProperty in the case of ProjectorModeCheckbox) is the way to go.

As for how this is done... I'm wonder if we really need/want yet-another option like phetioLinkedProperty. An alternative would be to make Checkbox call addLinkedElement only if its Property is instrumented. And make ProjectorModeCheckbox responsible for calling addLinkedElement for profileNameProperty.

projectorModeEnabledProperty solves all of these problems too, and leads to a uniform API across simulations (all checkboxes have a BooleanProperty).

And because of this uniformity, clients are liable to use the internal Property, rather than the semantically-relevant Property. So yes, it provides a way to set/get value and save/restore state. But the "solution" relies on a detail that shouldn't be exposed, which creates other potential problems.

I'm not sure whether the uniformity is worth the cost of exposing an implementation detail, and I'd like to discuss further.

If this is something that designers can live with in the specific case of ProjectorModeCheckbox (see design issue phetsims/joist#572) and in general, then I see how it (slightly) simplifies PhET-iO implementation. But I think there are more cons than pros for how this affects the client API.

[samreid]

And because of this uniformity, clients are liable to use the internal Property, rather than the semantically-relevant Property. So yes, it provides a way to set/get value and save/restore state. But the "solution" relies on a detail that shouldn't be exposed, which creates other potential problems.

Somewhat devil's advocating here, but to a Client, wouldn't they want to be able to set the ProjectorModeCheckbox to true (checked) or false (unchecked) without having to understand the underlying details of profileNameProperty? To rephrase: we have been saying that the projectorModeEnabledProperty is an implementation detail. But wouldn't clients see the profileNameProperty as the implementation detail?

[pixelzoom]

Ask the designers.

[zepumph]

Its hard for me to know exactly where this conversation should go. I will note that now addLinkedElement is graceful, and doesn't need to be called on an instrumented Property.

I will also note that over in phetsims/joist#654 I added back in phetioLinkProperty to achieve a better phet-io structure for ProjectorModeCheckbox.

I didn't really follow either argument above about "implementation details" If they are instrumented Properties, then they aren't implementation details.

Note that currently there are no linked elements for ProjectorModeCheckbox.

I don't see any issues with master currently. @pixelzoom as the opener of this issue, can you please review and see if anything else needs to be done here?

[pixelzoom]

The phetioLinkProperty option is nice. And are you saying that addLinkedElement is OK to call with an element that isn't instrumented? If so, then the currently implementation in Checkbox (below) seems sufficient.

  options.phetioLinkProperty && this.addLinkedElement( property, {
    tandem: options.tandem.createTandem( 'property' )
  } );

[zepumph]

And are you saying that addLinkedElement is OK to call with an element that isn't instrumented?

Yes I am saying that is the case since work around phetsims/tandem@85f4515.

It seems like we should only implement phetioLinkProperty when really needed in common code elements.

I'm going to close this issue.