Instrument Keypad.js for PhET-iO

[zepumph]

This is needed for phetsims/projectile-motion#177. I'll take a first pass at this, but I think it will want some PhET-iO design time too.

Design Questions:

• Does AbstractKeyAccumulator.accumulatedKeysProperty need to be instrumented?

[zepumph]

From first design meeting, the notes were to have the most minimal instrumentation to support record/payback and data stream from user inputs. It also seemed valuable to be able to get the value out of the keypad. @samreid will you please review the current instrumentation. A great way to do that is in Projectile Motion studio. The lab screen -> custom options in the combobox -> press a yellow "edit" button to pop up the keybad.

[samreid]

the notes were to have the most minimal instrumentation to support record/payback

Scenery input event recording does not require UI components to be instrumented, right?

[zepumph]

Sorry, I edited that above.

[samreid]

Thanks! What's the timeline for this sim? Should this review be bundled with other review?

[zepumph]

Same timeline for Gravity and Orbits I believe, asap for the phet-io developer as it can be balanced with dynamic state.

[samreid]

Things look pretty good. A few questions. I was surprised to see the "enterButton" was a sibling of the "keypad"--should they be more closely related, sharing a nearer parent or something? Also, do you like dynamically creating the tandem names like keyPadTandem.createTandem( `${_.camelCase( keyObject.identifier )}Button` )? I have experimented with creating "whole" tandem names earlier, such as keyPadTandem.createTandem( keyObject.buttonTandemName ). Also, (could be a separate issue) are you surprised that Key is all based on strings and not numbers?

[zepumph]

I was surprised to see the "enterButton" was a sibling of the "keypad"--should they be more closely related, sharing a nearer parent or something?

I think this is fine to me, as enter is part of the current keypad implementation. I can only assume that there was a reason that that was not implemented as part of the keypad (likely for flexibility if you don't want to have a submit style entry?). Perhaps someone like @jbphet would know why the enter button didn't make it into Keypad.js as I think he worked on it with @aadish a while back.

Also, do you like dynamically creating the tandem names like keyPadTandem.createTandem( ${_.camelCase( keyObject.identifier )}Button )? I have experimented with creating "whole" tandem names earlier, such as keyPadTandem.createTandem( keyObject.buttonTandemName ).

So would you rather have a field in Key.js that looks like this?

    this.buttonTandemName = _.camelCase( keyObject.identifier )}Button

That feels like an improvement to me. I'll implement.

Also, (could be a separate issue) are you surprised that Key is all based on strings and not numbers?

Strings feels right to me here. It makes it more general for things like the backspace or . button. Does that seem reasonable to you? If not please make an issue with your thoughts.

[samreid]

Not feeling 100% great about any of the proposals (including the one below), but the one I had in mind was new Key( '7', KeyID.SEVEN, '7Button' ). The philosophy is to make all tandems 100% searchable, and not using string concatenation for any tandem term construction.

[zepumph]

From phetsims/projectile-motion#177 design meeting, here are two more items:

• instrument the KeypadLayer
• knowing when the keypad is shown.

@samreid's item:

• look into how the tandemName is set, try not to use string concatentation for it. (see Should we use string concatenation or whole terms for tandem components? tandem#128)

[zepumph]

@samreid, I understand your concern, and know that up until this point, we have refactored to make this pattern consistent. I think the main reason I am uninterested in it here is for two reasons.

• The word "Button" doesn't feel like it belongs as a definitive param to Key. In my commit, note that I called it the "buttonTandemName".
• (subjective) It doesn't seem more valuable to have it that way, just more busy work. If in the future we instrumented Key.js, then needing to pass a tandem to all Key instances may be required. But why do that before it is necessary?

[arouinfar]

@kathy-phet and I reviewed this as a part of phetsims/projectile-motion#244.

We don't think clients need to customize the keypad, and things could get pretty weird if they start removing individual buttons or messing with enabledProperties. The model should be the only thing controlling whether or not the keypad is visible/enabled, so everything should be read only. Similarly, nothing needs to be phetioFeatured. There are lots of buttons which by default have their enabledProperty and visibleProperty featured, but ideally these would be unfeatured within keypad (rather than doing it in the overrides for every sim).

State is not currently being saved correctly. In Projectile Motion > Lab Screen > Custom, open a keypad and type in a value. Launch the sim, and the range string (e.g. 1 - 5000 kg) and the value in the numberAccumulator will be cleared.

[zepumph]

This will most likely block Projectile Motion publication, which we may get to in a couple of months.

[marlitas]

This seems like a good contender for @matthew-blackman and I this iteration since it relates to Projectile Motion. Tagging it as such.

[zepumph]

104bad5 broke collision-lab by call createTandem on undefined. Fixed above.