Keypad work still to do

[ariel-phet]

@pixelzoom and @amanda-phet have pointed out the need for a general NumberKeypad design meeting. We should look into addressing the following questions as well as fleshing out and prioritizing other anticipated needs

• need a control that combines keypad, value display and "enter" button #271
• NumberKeypad: add ability to constrain what's entered #272
• NumberKeypad: can't enter 0 before decimal place #275
• NumberKeypad: is armedForNewEntry feature needed? #280
• i18n requirements? (will be covered in NumberKeypad: decimal point is not localized #279 )
• Scientific notation? (there is no requirement for this yet, so we've decided not to have any requirements along these lines and we will cross this bridge when we come to it)
• layout options? (a vertically narrow, horizontally wide keypad might be advantageous in certain circumstances)
• Instrument Keypad.js for PhET-iO #540
• Create general Keypad modal layer type #663
• Create EditableNumberDisplay #678
• Problems converting scenery-phet/js/keypad/ to TypeScript. #758
• KeypadDialog standardization #823

[ariel-phet]

@amanda-phet do you mind getting a design doc started?

[pixelzoom]

Also labeling for developer meeting, so we can sort out what needs to be done on NumberKeypad sooner vs later, who is lead developer on the design, who is doing the work, etc.

[pixelzoom]

11/10/16 design meeting notes:

• We need 2 separate keypads: one for numbers, one for entering equations
• Focus first on numbers, then on equations
• Currently missing from numbers keypad is sign (+/-) button (see #274)

[amanda-phet]

We should edit the bullets above to use "expressions", "strings", "text", or something along those lines, rather than "equations" (which implies that there is an equal sign being typed).

[pixelzoom]

"strings" and "text" is way too general, we're not designing a text editor. How about "mathematical expressions"?

[pixelzoom]

Revised 11/10/16 design meeting notes: (replaces #283 (comment))

• We need 2 separate keypads: one for numbers, one for entering mathematical expressions
• Focus first on numbers, then on mathematical expressions
• Currently missing from numbers keypad is sign (+/-) button (see #274)

[jbphet]

@pixelzoom and @jonathanolson ,

@aadish and I (mostly Aadish) have done some work on the Keypad. I think the current form captures most of the architecture that we discussed. I’m mostly out for the next couple of weeks, but since this is going to be needed around the end of January, I thought it would be good for you to take a look and see if you think we’re generally on the right track. There are a number of things that can be improved – for instance, there is a ‘value’ field in AbstractKey that can probably be removed. Please take a look at it from a more of a high level and give us your comments, and we will address them in the new year.

[jbphet]

The bulk of the code can be found in scenery-phet/keypad, and there is demo code in scenery-phet/demo.

[pixelzoom]

Comments on scenery-phet demo (prior to looking at code):

• When nothing has been entered on the keypad, the default "arithmetic value" should not be 0. That makes it impossible to distinguish between no entry and an explicit 0 entry without consulting the string value. It would be preferable to use null to represent the absence of an arithmetic value.

• The demo itself is a little annoying, with stuff horizontally shifting around. This is probably just due to bad layout choices in the demo, but it's distracting, and potentially masks genuine problems.

• In the demo, "Clear on next keypress" should be a checkbox, not a button. It's a boolean state that's set, not an action that's performed. See the demo for NumberKeypad. (And if it doesn't already, the implementation must provide an API that allows the client to turn this on _and* off.)

• The demo doesn't demonstrate the decimal key. It should, because it's essential to sims in development (e.g. unit-rates). Again, see the demo of NumberKeypad.

[pixelzoom]

My first pass at the code was a general review, look mainly at conformance to PhET guidelines.

Issues:

• Keypad.createKeyNode - Private functions like this should be located after the constructor.

• Keypad.createKeyNode has an options parameter (documented as optional), but has no _.extend call to define defaults. So buttonFont and buttonColor are not really options, they are effectively required parameters. Either add an _.extend call or function parameters.

• Keypad and IntegerAccumulator constructor parameter options is not properly annotated as being optional. It should be {Object} [options].

• We generally put _.extend as the first line in the constructor, since options are an important part of the public API. This is currently not the case in Keypad and IntegerAccumulator, where _.extend follows the supertype constructor call.

• IntegerAccumulator is missing visibility annotation for validateAndProcessInput.

• AbstractKey has insufficient documentation for constructor parameter identifier.

• AbstractKey has no visibility annotations for properties defined in its constructor.

• AbstractKey handleKeyPressed is an abstract function. It should be annotated as @abstract and the default implementation should throw Error, not assert.

• AbstractKey handleKeyPressed is missing the keyAccumulator parameter, which I only discovered by looking at subtype implementations.

• AbstractKeyhandleKeyPressed is missing documentation of the keyAccumulator parameter.

• The documentation that is provided for AbstractKey handleKeyPressed is mostly redundant ("Function that is called by the key accumulator when this key is pressed, must be implemented in descendant classes."). It's obviously a function, a descendent class (sic) is a subtype (or subclass, if you must), and the @abstract annotation would indicate that it needs to be implemented by subtypes.

• PlusMinusKey is missing @public and @override annotations for handleKeyPressed.

• Redundant documentation like "PlusMinusKey Class derived from AbstractKey class" is not very useful. The reader needs a quick overview of this type's role and responsibilities, not the specifics of the class (sic) hierarchy.

• AbstractKeyAccumulator validateAndProcessInput is an abstract function. It should be annotated as @abstract and the default implementation should throw Error, not assert.

• Some functions are missing documentation, see TODO items that I've added in code.

[pixelzoom]

I corrected many of the issues related to code guidelines (check off in the above check list). I also added some TODO items in the code, where something still needs to be addressed.

On to review of the code architecture...

[pixelzoom]

First round of architectural review:

---

• Inappropriate property in AbstractKey base type.

The constructor signature of AbstractKey is:

function AbstractKey( displayNode, value, identifier )

... where value is {number}. Looking at non-numeric subtypes of AbstractKey (i.e. BackspaceKey, PlusMinusKey), I see that the value argument is null. This is a fundamental problem - a numeric value has no business being in the base type, it is specific to keys that have a numeric value (i.e. DigitKey).

---

• BackspaceKey has required constructor parameters that could (should?) be options.

The constructor signature is:

function BackspaceKey( width, height )

Why can't width and height be provided via options? There are surely reasonable defaults (as there apparently are for other keys). Also wondering... Why does BackspaceKey have these options when Keypad is responsible for button size (via buttonWidth and buttonHeight options)? Why is Backspace key instantiated as new BackspaceKey( DEFAULT_BUTTON_WIDTH, DEFAULT_BUTTON_HEIGHT ) in various Keypad layouts, when the button size might be changed via Keypad options?

---

• Responsibility for key dimensions.

... and if BackspaceKey is responsible for its dimensions, then why aren't the other subtypes of AbstractKey also responsible for their dimensions? Or the number of cells that they occupy in the grid?

---

• Validation and processing of input

This is apparently done in AbstractKeyAccumulator subtypes via:

validateAndProcessInput: function( accumulatedKeys )

I was surprised to see that this function has only one parameter, accumulatedKeys. How does it "validate and process input" if the input is not provided? Is this function mis-named? Is validation being done after the accumulator is modified?

NOTE: Validation was revised in b8d3bb9, see also #283 (comment).

---

• Why isn't clear on next key press the responsibility of the base type?

IntegerAccumulator defines _clearOnNextKeyPress and its associated setter/getter. Why aren't these defined in AbstractKeyAccumulator? I see nothing in the implementation to indicate that these are specific to IntegerAccumulator.

---

• Do you need a NumberAccumulator subtype?

There's a lot of stuff in IntegerAccumulator that would apply to (for example) DecimalAccumulator. Do you need to factor that out into a more general NumberAccumulator?

Highly recommended that you create DecimalAccumulator, and add it to scenery-phet demo. It will illuminate a lot of issues (like this one).

---

• Clients should not need to know about the accumulator.

This is another fundamental issue.

In the scenery-phet demo, I see:

555 var accumulator = new IntegerAccumulator( { maxLength: 5 } );
568  var keyPad = new Keypad( Keypad.PositiveAndNegativeIntegerLayout, accumulator
576 accumulator.clear();
583 accumulator.setClearOnNextKeyPress( true );

The accumulator should be an internal implementation detail, transparent to the client. Clients shouldn't need to instantiate it, and certainly shouldn't need to explicitly clear it, etc. Keypad (or its subtypes) should provide a complete API.

For example... Create NumberKeypad (Keypad subtype), which creates its own NumberAccumulator based on options, and (transparently) provides access to the string and numeric representations of the value. Could also possibly select a default layout based on options. Then scenery-phet demo might look like:

var keypad = new NumberKeypad( {
  plusMinus: true,
  maxDigits: 5
} );
keypad.clear();
keypad.setClearOnNextKeyPress( true );

---

• Absence of input should not be interpreted as 0.

IntegerAccumulator.updateNumericalValue:

72 return stringRepresentation.length > 0 ? parseInt( stringRepresentation, 10 ) : 0;

Absence of a value should most definitely not be mapped to a value of 0. This makes it impossible for the client to determine whether 0 was actually entered without consulting the string value. Recommended to change to:

72 return ( stringRepresentation.length > 0 ) ? parseInt( stringRepresentation, 10 ) : null;

[pixelzoom]

• Specification of layouts should mirror the layout structure.

The current specification of only 2 layouts requires 145 lines! (See Keypad PositiveIntegerLayout and PositiveAndNegativeIntegerLayout.) That specification is verbose, redundant, and makes it very difficult to see the structure of the keypad - especially when keys are specified out of order, as they currently are in both layouts.

Suggestions:
• Use a 2-dimensional array instead of row and column fields.
• Make horizontalSpan and verticalSpan properties of the keys, with default 1.

Then the specification of layouts would look like this:

    PositiveIntegerLayout: [
      [ new DigitKey( 7 ), new DigitKey( 8 ), new DigitKey( 9 ) ],
      [ new DigitKey( 4 ), new DigitKey( 5 ), new DigitKey( 6 ) ],
      [ new DigitKey( 1 ), new DigitKey( 2 ), new DigitKey( 3 ) ],
      [ new DigitKey( 0, { horizontalSpan: 2 } ), new BackspaceKey() ]
    ],

    PositiveAndNegativeIntegerLayout: [
      [ new DigitKey( 7 ), new DigitKey( 8 ), new DigitKey( 9 ) ],
      [ new DigitKey( 4 ), new DigitKey( 5 ), new DigitKey( 6 ) ],
      [ new DigitKey( 1 ), new DigitKey( 2 ), new DigitKey( 3 ) ],
      [ new PlusMinusKey(), new DigitKey( 0 ), new BackspaceKey() ]
    ],

This makes it very easy to see the structure of the keypad, and which keys occupy more than 1 grid cell. It also makes it very easy for clients to specify new (custom) layouts.

[pixelzoom]

• Use Unicode symbols for plus and minus.

In PlusMinusKey:

20 AbstractKey.call( this, '+/-', null, 'PlusMinus' );

... should be:

20 AbstractKey.call( this, '\u002b/\u2212', null, 'PlusMinus' );

[zepumph]

From today's dev meeting, I may be able bring this home while working on Projectile Motion for PhET-iO.

[zepumph]

Discussed with @jbphet today, and we checked off the top checkboxes that have been tackled.

Game plan for Keypad work this quarter:

• go through this issue and clean up anything still to do
• Close this issue (once making sure all work is done or captured in side issues)
• NumberKeypad: add ability to constrain what's entered #272
• need a control that combines keypad, value display and "enter" button #271
• Create general Keypad modal layer type #663
• some other issues I see around to work on during/after the above prioritized list:
  • Instrument Keypad.js for PhET-iO #540
  • Keypad should use nested options for its NumberAccumulator #541
  • Keypad: decimal point is not localized #333 this is not part of the scope of work I plan to tackle

Note I gathered this list from looking at https://github.com/issues?q=is%3Aopen+is%3Aissue+user%3Aphetsims+keypad as well as any linked issues around this issue. Anyone feel free to comment on loose ends or other thoughts I may not be thinking about.

[jbphet]

@zepumph is on the case, I'm going to unassign myself for now.

[zepumph]

From #283 (comment):

• additionalValidator still exists as an options, but is superfluous because of the validators parameter in AbstractAccumulator.
• I don't see support in NumberAccumulator for KeyID.X_SQUARED. I think that is alright, since @jonathanolson has it in his own sim. I don't see any more work here that isn't already in a separate issue. Closing

[markgammon]

Reopening and turning to an epic

[marlitas]

From a conversation with @kathy-phet this is not a priority for PhET-iO until a client asks for it. Completing keypad work is currently dependent on developer priority.

[jbphet]

The dev team has decided to make this somewhat of a priority, so I've used the "Monday request form" to request that this work be scheduled for an upcoming iteration. I'm going to defer the issue until the work is scheduled.