Uninstrument Node.pickableProperty in exchange for Node.inputEnabledProperty

[zepumph]

This is a sub/side issue from #1116.

@arouinfar noted the need to change client guides and overrides as part of this process. What fun!

On hold until #1116 is read to work for PhET-iO.

[zepumph]

Node.inputEnabledProperty is not optionally instrumentable. So this issue is no longer on hold for #1116, but I ran into #1164, and I don't want to continue on this until that is solved.

[zepumph]

Some points for this issue as a list:

• Uninstrument pickableProperty from Node
• Convert all usages of pickablePropertyPhetioInstrumented to -> inputEnabledPropertyPhetioInstrumented
• Part of this work should be to look through instrumented usages of pickableProperty and make sure that we are now setting inputEnabled instead. This is from Add PhET-iO support for Node.inputEnabledProperty #1116 (comment)
• Update all the Client guides that contain pickableProperty stuff
• Update all the overrides files that contain pickableProperty stuff
• Confirm in some studio instances that there are no more pickableProperties

[zepumph]

We decided we didn't want to publish another PhET-iO simulation with pickableProperty, and instead want to use inputEnabled now. Marking as blocking.

[zepumph]

At PhET-iO meeting today, we determined that Gravity and Orbits will not need to have pickableProperty removed. So we will just do this on master.

[zepumph]

Now that #1116 and #1175 have been committed to, we can mark this off hold.

[zepumph]

Questions for the future:

• Should we be able to pass in a pickableProperty, even though it will never be PhET-iO instrumented? That isn't currently supported in Node by anything (in other words: the only TinyForwardingProperties are those which support PhET-iO instumentation). For example, you can't pass in a Property to be a Node's opacityProperty. So should pickableProperty be converted back (downgraded) to a TinyProperty, which can't be overridden (like opacity), or should we still allow setPickableProperty( myProperty ).
  • TODO: If we keep as, we should write tests, for phet brand, that work for a TinyStaticProperty for setting it/getting it, regardless of phetioInstrumented status.
• I noticed that there aren't getters for *PhetioInstrumented boolean options. Should there be? I think "no", but maybe?

[zepumph]

Initial work has been done. Co-assigning @arouinfar to review https://github.com/phetsims/phet-io-client-guides/commit/c96920e0c93a7bbbe5730ea5998f8245f6bb4146

From here I will watch CT and report issues. @samreid can you review, and discuss the questions above?

[zepumph]

There are cases where instrumentation was relying on pickableProperty, like in

https://github.com/phetsims/ph-scale/blob/c7bfbd28343a7766dbab588074902178d65ddc09/js/common/view/graph/GraphIndicatorNode.js#L179-L184

@pixelzoom will want to review his sims to make sure these get changed also.

[arouinfar]

@zepumph it looks like find/replace all worked for client guides. I read through https://github.com/phetsims/phet-io-client-guides/commit/c96920e0c93a7bbbe5730ea5998f8245f6bb4146 and everything looks really good. Since this isn't something we'll be cherry-picking into release branches, I don't think we'll need to make any further changes to client guides. Thanks for taking care of the edits! Not sure if there is anything else left for this issue, so back to you @zepumph.

[pixelzoom]

@pixelzoom will want to review his sims to make sure these get changed also.

Tracking in sim-specific issues, as linked above.

[zepumph]

Over to @samreid for review.

[samreid]

I skimmed through the commits and did not see any trouble.

Should we be able to pass in a pickableProperty, even though it will never be PhET-iO instrumented?

Here are some Properties that look like they can be passed in (I'm searching for setTargetProperty)

• visibleProperty
• pickableProperty
• enabledProperty

PhET-iO instrumentation seems independent from whether you should be able to pass in a Property. It seems like it would round out the API to be able to pass in a pickableProperty, even if we use that rarely. It looks like pickableProperty is listed in the mutation keys, and the setter seems wired up properly, so I don't understand why it isn't already possible to pass in a pickable property. If it really is not currently supported, what are the drawbacks of adding support for that?

I noticed that there aren't getters for *PhetioInstrumented boolean options. Should there be? I think "no", but maybe?

I think if someone called myNode.enabledPropertyPhetioInstrumented and got back a falsy value of undefined, it could be confusing and difficult to debug, so it would be nice to add the getters.

[zepumph]

Getters added above. I am going to create a new issue for being able to pass in Properties for Node Properties that don't currently support PhET-iO.

[zepumph]

I created #1194, closing.