search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Keypad work still to do #283

Open ariel-phet opened 8 years ago

ariel-phet commented 8 years ago

@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

ariel-phet commented 8 years ago

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

pixelzoom commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

pixelzoom commented 7 years ago

Revised 11/10/16 design meeting notes: (replaces https://github.com/phetsims/scenery-phet/issues/283#issuecomment-260809625)

• 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 commented 7 years ago

@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 commented 7 years ago

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

pixelzoom commented 7 years ago

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

pixelzoom commented 7 years ago

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

Issues:

pixelzoom commented 7 years ago

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 commented 7 years ago

First round of architectural review:

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).

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?

... 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?

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 https://github.com/phetsims/scenery-phet/commit/b8d3bb96fc725cdfae1572554347283e88e392c4, see also https://github.com/phetsims/scenery-phet/issues/283#issuecomment-268633910.

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.

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).

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 );

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 commented 7 years ago

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 commented 7 years ago

In PlusMinusKey:

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

... should be:

20 AbstractKey.call( this, '\u002b/\u2212', null, 'PlusMinus' );
pixelzoom commented 7 years ago

After addressing the issue of the accumulator being transparent to the client... You need to address the issue of disposing of a Keypad. In scenery-phet demo (ComponentsView), you have the client linking to Properties and there is no dispose anywhere in the keypad implementation.

pixelzoom commented 7 years ago

In AbstractAccumulator:

23    // @public - array property that tracks the accumulated key presses
24    this.accumulatedKeysProperty = new Property( [] );

I see no calls to accumulatedKeysProperty.link, so apparently nothing is observing it. So why does it need to be a Property? And if it does need to be observable, why is it not an ObservableArray?

pixelzoom commented 7 years ago

These functions don't actually update anything. They compute values, which are used elsewhere to update Properties. More appropriate names would be computeStringValue and computeNumericalValue.

[NOTE: Renamed to keysToString and stringToInteger.

pixelzoom commented 7 years ago

The first if statement in the current implementation is redundant, and can be deleted:

    updateNumericalValue: function( accumulatedKeys, index ) {
      if ( accumulatedKeys.length === 0 ) {
        return 0;
      }

      var stringRepresentation = this.updateStringValue( accumulatedKeys, index );
      return stringRepresentation.length > 0 ? parseInt( stringRepresentation, 10 ) : 0;
    },
pixelzoom commented 7 years ago 
      var length = accumulatedKeys.length;
      var multiplier = 1;
      var maxLength = this.options.maxLength;
      var startIndex = 0;
      var startString = '';
      if ( length > 0 && accumulatedKeys[ 0 ] instanceof PlusMinusKey ) {
        multiplier = -1;
        maxLength += 1;
        startIndex = 1;
        startString = '-';
      }

All of these local vars are unnecessary. The minus character should be Unicode, but requires special care when converting from string to number.

I'm going to go ahead and change this. Ask me if you have questions.

pixelzoom commented 7 years ago

This was a problem before I made the above changes (git checkout 0c4958c to confirm). If you enter a '0', it should be overwritten by subsequent digits. The displayed string should not look like this:

screenshot_101

pixelzoom commented 7 years ago

OK, I'm done with this review. There are a lot of open issues, represented by check-box items in the above comments.

The big issues are:

(1) The client shouldn't need to know about accumulator; Keypad (and its subtypes) should provide the complete API. (2) AbstractKey should not have an associated {number} value, which is relevant only for DigitKey. (3) Specification of layouts should be simplified/clarified. (4) DecimalAccumulator would help to flush out type hierarchy issues. (5) Leading zeros are not handled properly.

pixelzoom commented 7 years ago

In retrospect... A separate issue (something like "initial review") should have been created for this review, rather than putting this in the "design requirements" issue. Let's keep that in mind for the next review.

pixelzoom commented 7 years ago

A couple of other things that occurred to me, after ruminating on the current implementation. These are (imo) both big architectural flaws.

In https://github.com/phetsims/scenery-phet/issues/272 we discussed the importance on being able to customize the constraints on what can be entered using the keypad. And that issue resulted in the addition of options.validateKey in NumberKeypad. I see no evidence of any similar API in this new implementation. In fact, the only validation is a maximum number of digits that is hardwired into IntegerAccumulator.validateKeys:

65 if ( this.getNumberOfDigits( proposedKeys ) <= this.options.maxLength ) {

Validation should be encapsulated in one place, and it should be customizable.

My first red flag related to this was in AbstractKey.handleKeyPress, whose signature is:

39 handleKeyPressed: function( keyAccumulator )

If feels wrong that keys must know about the accumulator. handleKeyPressed creates a "proposed" set of keys, based on the current accumulated keys. And then that proposed set is what is validated. Keys are also responsible for handling the "clear on next pressed feature" (which results in duplicated code, btw.) And keys know the structure of how the accumulator stores keys - e.g., the keys are manipulating arrays. This all seems very wrong to me. Keys should know nothing about the accumulator. The accumulator should make decisions based on which key is pressed, and how keys are accumulated should be private to the accumulator.

pixelzoom commented 7 years ago

Another note on the validation issue mentioned in https://github.com/phetsims/scenery-phet/issues/283#issuecomment-268678103... The current implementation of IntegerAccumulator only allows you to constrain the number of digits entered. As an example of why this isn't sufficient... Unit Rates needs to constrain the number of digits and requires that the entry is a positive integer (i.e., 0 is not valid.) Future sims may have other constraints, and trying to foresee all constraints is impossible, so Keypad must allow clients to specify their own constraint algorithm.

pixelzoom commented 7 years ago

In https://github.com/phetsims/scenery-phet/issues/283#issuecomment-268389241, @jbphet asked for feedback from me and @jonathanolson. I don't see any feedback from @jonathanolson. So I'm wondering if we should defer the design phone call scheduled for this morning?

pixelzoom commented 7 years ago

@jbphet and I discussed status via Skype, and we decided to defer our design meeting until @jonathanolson has provided feedback.

Here's the feedback I'd like to hear from @jonathanolson:

jonathanolson commented 7 years ago

I reviewed independently first, and came to the same high-level conclusions as @pixelzoom. Review here starts with higher-level points and moves towards low-level things I noticed. Feel free to convert to a checklist if it helps.

I was anticipating a structure where there was an enumeration of values:

KeypadValue = {
  0: ...
  1: ...
  // ...
  9: ...
  MINUS: ...
  DECIMAL: ...
};

and the accumulator stored the current values (similar to what is current):

// @public {Property.<Array.<KeypadValue>>}
this.valuesProperty = new Property( [] );

where each key contained logic just based on the keypad values:

getValuesAfterPress: function( values: {Array.<KeypadValue>} ) : {Array.<KeypadValue}

The current structure is similar, but keys themselves are used in place of the values requiring extra documentation like:

// PlusMinusKey (if present) will be first key, and indicates that the number is negative
if ( keys.length > 0 && keys[ i ] instanceof PlusMinusKey ) {
  returnValue = PlusMinusKey.MINUS_CHAR;
  i++;
}

and each key is instead given the full accumulator (and the responsibility to have to handle accumulator responsibilites, like clearing on next keypress):

if ( keyAccumulator.getClearOnNextKeyPress() ) {
  newArray = [];
  keyAccumulator.setClearOnNextKeyPress( false );
}
else {
  // actually do things
}

This would prevent bugs like the BackspaceKey not fully clearing everything on the next keypress with clearOnNextKeyPress is set (it only removes one character and removes the flag).

Ideally, this would be handled in the accumulator:

onPress: function( key ) {
  // Handle clearing on key presses in one place
  if ( this.clearOnNextKeyPress ) {
    this.valuesProperty.value = [];
    this.clearOnNextKeyPress = false;
  }
  else {
    var proposedValues = key.getValuesAfterPress( this.valuesProperty.value );
    if ( this.validate( proposedValues ) ) {
      this.valuesProperty.value = proposedValues;
    }
  }
}

So that keys could have a slightly simpler implementation, e.g. for DigitKey:

getValuesAfterPress: function( currentValues ) {
  return Keypad.removeLeadingZero( currentValues ).concat( [ this.value ] );
}

instead of the current:

handleKeyPressed: function( keyAccumulator ) {
  var newArray;
  if ( keyAccumulator.getClearOnNextKeyPress() ) {
    newArray = [];
    keyAccumulator.setClearOnNextKeyPress( false );
  }
  else{
    newArray = _.clone( keyAccumulator.accumulatedKeysProperty.get() );
    if ( keyAccumulator.valueProperty.get() === 0 ){
      newArray.pop();
    }
  }
  newArray.push( this );
  return newArray;
}

or for PlusMinusKey:

getValuesAfterPress: function( currentValues ) {
  var isNegative = currentValues.length > 0 && currentValues[ 0 ] === KeypadValue.MINUS;

  // Remove minus sign if the value is already negative
  if ( isNegative ) {
    return currentValues.slice( 1 );
  }
  // Otherwise add a minus sign on the front
  else {
    return [ KeypadValue.MINUS ].concat( currentValues );
  }
}

Additionally, it would be ideal for the accumulator to have a custom validate() function given in the options (which may include maxDigits handling). I agree with @pixelzoom on the need to have custom validation functions. Having a library (like Keypad.validateMaxDigits, Keypad.validateNonzero, etc.) that can be composed would be great, so you could:

new Keypad( ..., function validate( values ) {
  // Allow up to 4 digits, don't allow 0 on its own, and don't allow the digit 5
  return Keypad.validateMaxDigits( values, 4 ) &&
         Keypad.validateNonzero( values ) &&
         !_.includes( values, KeypadValue[ 5 ] );
} );

I didn't see anything about clearOnNextKeyPress in the AbstractKeyAccumulator, but it is assumed by all keys (which only require an AbstractKeyAccumulator). Can this move to the AbstractKeyAccumulator?

I like having the currentValues be a property (or ObservableArray as @pixelzoom notes), since the stringValue and numberValue can be DerivedProperties. However, I'd have the numberValue created directly from the values, so we don't have to handle stripping out Unicode minus signs or other display complexities. Thus I'd like to see valuesToString( values ) and valuesToNumber( values ), and:

// in Keypad
this.stringProperty = new DerivedProperty( [ accumulator.valuesProperty ], accumulator.valuesToString.bind( accumulator ) );
this.numberProperty = new DerivedProperty( [ accumulator.valuesProperty ], accumulator.valuesToNumber.bind( accumulator ) );

Documentation of layout (and possibly simplification of that as noted by @pixelzoom) would help a lot. Took a bit to figure out it was {Array.<Array.<AbstractKey|null>>}. Also, it should be further simplified, as I think rows like [ new DigitKey( 5, { horizontalSpan: 5 } ), null, new DigitKey( 3 ) ] will improperly position the 3 key. There seem to be a lot of edge cases, so accumulating row/column offsets (instead of computing for each value) in layout would be more error-proof.

For loops for rows/columns (or multiple dimensions handled in general) are more readable with names:

for ( var row = 0, ... ) {
  for ( var column = 0, ... ) {
    doSomething( values[ row ][ column ], values[ row ][ column - 1 ] );
  }
}

There's a lot of inconsistent formatting, particularly with array brackets, e.g. layout[ numRows - 1][i] should be layout[ numRows - 1 ][ i ].

createKeyNode types should have {AbstractKey} for keys and {AbstractKeyAccumulator} for the accumulator.

Default buttonColor and buttonFont are specified in two places.

It's weird that displayNode can be a {string} or {Node}, although AbstractKey only allows Node. Can we remove the ability to have strings, or fully support it? If we keep the string support, I'm worried about the asymmetric auto-scaling to a fixed dimension.

Noting that stringToInteger can return null is quite important.

jonathanolson commented 7 years ago

Also I want to note that having an enumeration for values isn't a strict requirement, but the abstraction sounds helpful in general. It may be overgeneralizing, and keys as values may be fine.

In the future, what happens if you have a key that conditionally wants to store one of a few values? It probably won't come up, but having a concrete list of what values can be in the array may just help comprehension.

Either way, thinks like isDigit() is probably a helpful utility function (currently done with instanceof DigitKey).

pixelzoom commented 7 years ago

Self-assigning to review for discussion on 2/20/17 @ 10am.

jbphet commented 7 years ago

Over the last few days I've discussed the design of the keypad with @pixelzoom, @jonathanolson, and @aadish . Below is a summary of the decisions made based on those discussions.

On the topic of the Keypad API, we agreed that the client shouldn't need to know about accumulator. The Keypad (and its subtypes) should provide a complete API. The fields should be accessible like this:

...versus something like this:

Since we may sometimes have a need for custom accumulators, the keypads should have default accumulators with the ability to provide a different one.

On the topic of custom validation, we need to support this, so we decided to use a combination of general options so that clients don't have to create a validator any time they want any sort of variation, but also should allow the client to supply a completely different validator. We are thinking that the API will offer options like this:

...but also these:

additionalValidator will be a function that was just in addition to the default validation. alternativeValidator will be a different function and, if provided, would override all default and additional validation.

To make it easier to create new keypads, we could have a library of parameterized validators, creating them something like this:

   options.additionalValidation = Keypad.currencyValidator( { maxDigitsLeftOfMantissa: 4 } )

We should be sure to note in the documentation that the validator can have side effects. Write in use case where decimal point is added.

On the topic of whether the keys should encapsulate the knowledge of how they are accumulated, we decided that the accumulator should have that knowledge. This would allow keys to have different behaviors in different keypads. For example, we may want the +/- key to work differently in number accumulator versus an equation accumulator. The tradeoff is that we will need to change more files to add a new key.

We discussed using an observable array for the accumulated keys instead of a Propery<Array>, but decided that the latter will work better.

@pixelzoom suggested that we implement a floating point accumulator, determine what is common between this and the integer accumulator and consider putting it into a common base class. @aadish will do this.

@pixelzoom and @jonathanolson - please comment if you think I've left anything out or misrepresented any of the things that we discussed.

@aadish is going to move forward with these changes and we will re-review once the main elements are working.

pixelzoom commented 7 years ago

@jbphet What's the status of the new keypad?

@ariel-phet FYI, the lack of closure on this issue is resulting in work-around solutions that are being copy-pasted between sims. See https://github.com/phetsims/projectile-motion/issues/118

jonathanolson commented 7 years ago

The fields should be accessible like this:

keypad.stringProperty.link keypad.valueProperty.link keypad.accumulatedKeysProperty.link

AbstractKeyAccumulator doesn't have stringProperty/valueProperty, that seems like an implementation detail in NumberAccumulator? (It's also not marked @override in NumberAccumulator).

TermAccumulator (in Area Model) has richStringProperty and termProperty (which somewhat map over), but initially there was no stringProperty equivalent (where I used termProperty.value.toRichString()) before I needed to handle the display slightly differently than the "value". I don't think assuming there's always a stringProperty equivalent is a safe assumption.

Are we sure we want to put restrictions on accumulators by pushing the client to forward things through Keypad itself?

We should be sure to note in the documentation that the validator can have side effects.

That seems like an anti-pattern. Presumably validating would have no side effects?

As you refactor, please keep Area Model in mind (3 custom keypad layouts, custom accumulator).

jonathanolson commented 7 years ago

Also, it looks like these are already being forwarded in Keypad (but some are undefined for Area Model's accumulator):

    this.stringProperty = this.keyAccumulator.stringProperty; // @public (read-only)
    this.valueProperty = this.keyAccumulator.valueProperty; // @public (read-only)
    this.accumulatedKeysProperty = this.keyAccumulator.accumulatedKeysProperty; // @public (read-only)

Documentation for what the accumulator API is should probably be in the abstract accumulator, as if these were part of the API, it was undocumented.

jbphet commented 7 years ago

@jonathanolson said:

AbstractKeyAccumulator doesn't have stringProperty/valueProperty, that seems like an implementation detail in NumberAccumulator?

and

Are we sure we want to put restrictions on accumulators by pushing the client to forward things through Keypad itself?

This is a good point, and in fact, that's why I designed it this way in the first place - there will be different accumulators needed in different circumstances. This makes it difficult to make these properties available directly from the keypad. In a previous comment about design discussions with @pixelzoom and @jonathanolson I said, "we agreed that the client shouldn't need to know about accumulator". I'd like to reverse this decision - it's a lot easier to implement and use this if the client knows about and hooks up to the accumulator. It also makes sense to me - you have a keypad, and the keypad has a place where the keys get collected. I think there will be enough examples available in the code so that new users will be able to make sense of this pretty quickly.

On the topic of validators, @pixelzoom said:

We should be sure to note in the documentation that the validator can have side effects.

..and @jonathanolson said:

That seems like an anti-pattern. Presumably validating would have no side effects?

I just spent some time looking at this, and I think it is no longer the case. It may have been at one time - I honestly don't remember - but AbstractKeyAccumulator.validateAndUpdate is just expecting a boolean value from the validators. I suppose there isn't anything that actively prevents the validators from modifying the proposed keys, but the default validator in NumberAccumulator doesn't do it, and there aren't any examples in the demo that do it either. So, in short, I think this is no longer an issue. Feel free to let me know if I've missed anything.

jbphet commented 7 years ago

I think the issues raised above have been addressed at this point, but there were a lot of them, so I have only a moderate degree of confidence that this is true. I'm going to give @andrea-phet the go-ahead to use this in Projectile Motion, see https://github.com/phetsims/projectile-motion/issues/127. I think the API is unlikely to change much at this point, so it should be safe. I'm going to assign to @jonathanolson to comment in case he thinks it's not ready for this, since he has used it in Area Model. If he's okay with it and doesn't veto, it should be assigned to @pixelzoom for another look when he returns. If all looks good to him, it can be migrated into unit rates. If not, I'll work with him to iron out any issues.

jbphet commented 7 years ago

Scanning back through the list of comments, I realized that I missed an item: implement dispose. I'll do this shortly, but I think any review can occur in parallel.

jonathanolson commented 7 years ago

I'm going to assign to @jonathanolson to comment in case he thinks it's not ready for this, since he has used it in Area Model. If he's okay with it and doesn't veto, it should be assigned to @pixelzoom for another look when he returns.

Sounds ready for projectile-motion usage to me. Assigning as described.

pixelzoom commented 7 years ago

@jbphet In addition to the lack of dispose... NumberAccumulator does not propagate options to AbstractKeyAccumulator, looks like a bug. I'm also guessing that the alternativeValidator and additionalValidator features of AbstractKeyAccumulator are therefore untested.

pixelzoom commented 7 years ago

@jbphet said:

Scanning back through the list of comments, I realized that I missed an item: implement dispose. I'll do this shortly, but I think any review can occur in parallel.

Integration of Keypad into projectile-motion is completed. Publication of that sim is blocked until dispose is implemented and options propagation is fixed (see https://github.com/phetsims/scenery-phet/issues/283#issuecomment-321404395). High priority.

ariel-phet commented 7 years ago

@jonathanolson said he might be able to take care of this quickly. Since it is a blocking issue for projectile motion I ask him to move forward.

jonathanolson commented 7 years ago

As a side note, things would be more convenient if we had an array of validators, instead of having to branch to check for default/alternative/additional. It would also allow greater flexibility in the future.

jonathanolson commented 7 years ago

Added the options pass-through.

It seems like the only thing that needs disposal are the key nodes (as if the key labels are Nodes themselves, like in Area Model's case, they need the parent removed).

Am I missing something? (I double-checked all files in the keypad directory and really didn't see any complicated disposal that would be required).

jbphet commented 7 years ago

I added disposal of the properties that are created by the keypad. I also added a test harness to the scenery-phet demo that removes one of the demo keypads and invokes the dispose function.

jbphet commented 7 years ago

@jonathanolson added the options pass through, and he and I added the dispose function. However, I also added a test case to the scenery-phet demo that tests the disposal of the keypad, but that test case isn't passing yet. The keypad is being successfully removed from the scene graph, and the dispose function is being called, and as far as I can tell all references from the demo are being cleaned up, yet the keypad is not being garbage collected. I'll leave this open and investigate early next week.

jbphet commented 7 years ago

The issue that was causing the keypad to not be garbage collected has been resolved, and was unrelated to the dispose function. It was an odd and subtle case, and a separate issue will be logged on it since @jonathanolson wanted to investigate it more after he and I looked at it. I'm going to leave the test harness in the scenery-phet demo so that we can easily regression test the dispose function. The test harness consists of a button that, when pressed, removes one of the keypads from the scene graph and calls its dispose function. When using this, the idea is to load the keypad components, take a memory snapshot, then press the removal button and take another snapshot. There should be three keypads in the first memory snapshot and two in the second.

This is what the keypad test/demo page looks like as of this writing, and the green button at the lower right is the one that tests the disposal: scenery-phet-test

At this point the options pass-through and dispose function should be taken care of. Back to @pixelzoom for another check.

pixelzoom commented 7 years ago

Several things still to be addressed.

Little things:

Big things:

[EDIT: I numbered the items above, to make it easier to refer to them in subsequent comments.]

jonathanolson commented 7 years ago

KeyID.X, KeyID.X_SQUARED, … Have these features been tested? If not, why not? And should ‘untested’ be noted in the internal documentation?

These are used in Area Model (see TermAccumulator). It was noted that any sim-specific Key IDs should be placed in KeyID, should that change?

I’d really like to see another subtype, if only to flush out issues of where responsibilities should reside.

TermAccumulator (not common code) is another subtype.

jbphet commented 7 years ago

Several of the "little things" have been addressed.

In the "big things" subcategory...

@pixelzoom said:

A string representation seems useful to (required by?) all accumulator subtypes.

Maybe. It just seemed to me that making any assumptions about what representations are needed makes for a brittle design, and I wanted to keep the abstract accumulator as simple and general as possible, so it only accumulates keys. I could envision a case where only a numeric value is needed, for instance. So, I'd prefer to keep it this way. In the grand scheme of things, if there is a string representation in the base type, it probably won't ever mess anything up, so if others feel strongly I'll capitulate and move it there.

Also, @jonathanolson pointed out that there is a second accumulator sub-type - TermAccumulator. @pixelzoom - do you feel that this sufficiently addresses the 2nd "Big Thing"?

I'll work on Little Thing 4 and Big Things 3 and 4 at a later time. I'm not going to assign back @pixelzoom until I've done that, though he's welcome to comment when he sees this update if so inclined.

pixelzoom commented 7 years ago

Re questions in the above comment...

Perhaps @jonathanolson can chime in on whether he ran into hierarchy issues when creating TermAccumulator, and whether having stringProperty available in AbstractKeyAccumulator would have been useful.

jonathanolson commented 7 years ago

Perhaps @jonathanolson can chime in on whether he ran into hierarchy issues when creating TermAccumulator, and whether having stringProperty available in AbstractKeyAccumulator would have been useful.

As noted in https://github.com/phetsims/scenery-phet/issues/283#issuecomment-318182334 above:

TermAccumulator (in Area Model) has richStringProperty and termProperty (which somewhat map over), but initially there was no stringProperty equivalent (where I used termProperty.value.toRichString()) before I needed to handle the display slightly differently than the "value". I don't think assuming there's always a stringProperty equivalent is a safe assumption.

So having to require a stringProperty implementation for all accumulator subtypes doesn't "feel" correct to me.

The general accumulation and validation seemed quite general, like it should work for any subtype.