Improvement on Enumeration subtyping pattern

[zepumph]

Tagging https://github.com/phetsims/chipper/issues/1106. Over in https://github.com/phetsims/phet-core/issues/101, @chrisklus reported a strange bug that has to do with mutating EnumerationValue instances depending on what Enumeration they are part of. We recommend a couple of changes to the pattern.

Basically we need to make sure that MyEnumeration.SOME_VALUE.enumeration is always of type MyEnumeration, even if MyEnumeration is a subtype or supertype of other EnumerationValues. This means guarding this line https://github.com/phetsims/phet-core/blob/c2d477a21fcc5259360d0fc820f5e6fab9b708da/js/Enumeration.ts#L74.

We also want to make the pattern clear in EnumerationProperty, about passing in the appropriate validValues (which could be a subset of the available Enumeration. I'll potentially make a new issue to support that.

[zepumph]

• [x] I feel like we could go one step further with this commit and add a setter for enumeration that assert if trying to be set when already non null.

[zepumph]

Should be complete by the end of next week for Number Play.

[zepumph]

We also want to make the pattern clear in EnumerationProperty, about passing in the appropriate validValues (which could be a subset of the available Enumeration. I'll potentially make a new issue to support that.

That was done in https://github.com/phetsims/axon/issues/374

[zepumph]

@chrisklus you should be all good here. I also wrote unit tests to ensure some behavior. Please note that when subtyping, you really cannot trust or use EnumerationValue.enumeration. As a result, I would recommend coding around that, probably just using MySubtypeEnumeration.enumeration always (as a static), and not value => value.enumeration.

Please review, and we can pick up the conversation over in https://github.com/phetsims/phet-core/issues/101.

[chrisklus]

Thanks @zepumph! Everything done in this issue looks great. I am still running into an issue when using my enum subtype with EnumerationProperty. I think it is related to the additional validation done with the phetioType, since that is still using value.enumeration:

const options = merge( {
      validValues: value.enumeration!.values,
      phetioType: Property.PropertyIO( EnumerationIO<T>( {
        enumeration: value.enumeration!
      } ) )
    }, providedOptions );

Maybe enumeration could be an option to EnumerationProperty?

Here is a patch to reproduce:

```diff Index: js/game/model/Subitizer.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/game/model/Subitizer.ts b/js/game/model/Subitizer.ts --- a/js/game/model/Subitizer.ts (revision 0a572e6658866ef26fc3666f853723baa786d6c7) +++ b/js/game/model/Subitizer.ts (date 1645218972449) @@ -28,7 +28,7 @@ import NumberPlayConstants from '../../common/NumberPlayConstants.js'; import NumberPlayQueryParameters from '../../common/NumberPlayQueryParameters.js'; import numberPlay from '../../numberPlay.js'; -import CountingObjectType from '../../../../counting-common/js/common/model/CountingObjectType.js'; +import SubitizeObjectType from './SubitizeObjectType.js'; // types type PredeterminedShapes = { @@ -132,7 +132,7 @@ public readonly objectSize: number; public readonly isInputEnabledProperty: BooleanProperty; private timeToShowShapeProperty: IReadOnlyProperty; - public readonly objectTypeProperty: EnumerationProperty; + public readonly objectTypeProperty: EnumerationProperty; private isDelayStarted: boolean; private timeSinceDelayStarted: number; public readonly isLoadingBarAnimatingProperty: BooleanProperty; @@ -185,7 +185,18 @@ this.isInputEnabledProperty = new BooleanProperty( false ); // the object type of the current shape - this.objectTypeProperty = new EnumerationProperty( CountingObjectType.DOG ); + this.objectTypeProperty = new EnumerationProperty( SubitizeObjectType.DOG, { + validValues: SubitizeObjectType.enumeration.values + } ); + + // or this + // + // this.objectTypeProperty = new EnumerationProperty( SubitizeObjectType.DOG, { + // validValues: [ + // SubitizeObjectType.DOG, SubitizeObjectType.APPLE, SubitizeObjectType.BUTTERFLY, SubitizeObjectType.BALL, + // SubitizeObjectType.CIRCLE + // ] + // } ); // how long the shape is visible when shown, in seconds. This is a derived Property instead of a constant because // the time that the shape is shown is increased if the user gets the answer wrong multiple times. @@ -313,7 +324,10 @@ * Sets this.objectTypeProperty with a new object type for the current challenge. */ private setRandomCountingObjectType(): void { - this.objectTypeProperty.value = dotRandom.sample( CountingObjectType.enumeration.values.slice() ); + this.objectTypeProperty.value = dotRandom.sample( [ + SubitizeObjectType.DOG, SubitizeObjectType.APPLE, SubitizeObjectType.BUTTERFLY, SubitizeObjectType.BALL, + SubitizeObjectType.CIRCLE + ] ); } /** Index: js/game/view/SubitizerNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/game/view/SubitizerNode.ts b/js/game/view/SubitizerNode.ts --- a/js/game/view/SubitizerNode.ts (revision 0a572e6658866ef26fc3666f853723baa786d6c7) +++ b/js/game/view/SubitizerNode.ts (date 1645218325331) @@ -19,7 +19,7 @@ import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; import SubitizeRevealButton from './SubitizeRevealButton.js'; import NumberPlayConstants from '../../common/NumberPlayConstants.js'; -import CountingObjectType from '../../../../counting-common/js/common/model/CountingObjectType.js'; +import SubitizeObjectType from '../model/SubitizeObjectType.js'; // constants const BACKGROUND_RECTANGLE_CORNER_RADIUS = 10; @@ -94,7 +94,7 @@ points.forEach( point => { let object; // TODO: This should be SubitizeObjectType.CIRCLE, but there is a bug - if ( subitizer.objectTypeProperty.value === CountingObjectType.PAPER_NUMBER ) { + if ( subitizer.objectTypeProperty.value === SubitizeObjectType.CIRCLE ) { object = new Circle( scaleMVT.modelToViewDeltaX( subitizer.objectSize / 2 ), { fill: Color.BLACK } ); } else {

[zepumph]

I did not apply your patch, but I added some unit tests. Is this what you were thinking?!

[chrisklus]

Sorry for the delay - that's exactly what i was thinking, thanks! I too was trying to craft up something like firstOptions when trying this out. Commits look great and using this in practice is working really well.

I'm ready to close, and will update #101.