Code Review

[marlitas]

Code review to be performed by @zepumph to check on the following:

• Common code as well as sim-specific infrastructure for Region and Culture preferences
• Code changes in the NLI repo used to implement dynamic locales and region and culture character sets
• Compatibility with Region and Culture preferences and PhET-iO

[zepumph]

• [x] ?regionAndCulture query parameter should be public:true probably. (noted in https://github.com/phetsims/joist/issues/943)

[zepumph]

Message Deleted, I was wrong and this is already done.

[zepumph]

• I see that you added a color profile. Can you explain the motivation for this? Only interested as it pertains to the code review, since I'm getting my bearings on what the scope of the changes are.
• We discussed today about how some of the dynamic locale implementation could be better if accompanied with larger scale refactoring's and architecture changes. In general I would say that is totally fine to punt on, and to feel confident that it is outside the scope of the work of this release. I would really like to documentation to accompany this as much as possible. Verbose description to accompany gross or awkward decisions will greatly help any future maintainer of the sim (not that your code is gross). For example in https://github.com/phetsims/number-line-integers/blob/278ea9f928ea9cd88947cbb27f3d681245b4791a/js/explore/view/BankPointControllerNode.js#L160-L162. I wonder if it wouldn't be helpful to describe the context of why it was changed to be nested in an if statement. My guess is something like, "because of dynamic layout needs, this is gross because of X,Y,Z. . . ". That is ok! I'd prefer over explanation.
• I have only looked at the NLI code for about 10 minutes, and to me it has already well surpassed the threshold that I would have had to convert it to TypeScript if I were working here. There are variable and class renames, an added ColorProfile, updates to many files for dynamic locale, all of which I am finding very challenging to keep straight (as I'm sure that @marlitas has been feeling up to this point). I say this only to ask how similar future sim republications will be to this amount of work. If they are in this range I strongly recommend that TypeScript conversion accompany this. I also want to make sure that we understand the scope of that work. In most of my cases, converting a sim to TypeScript has been between 6-10 hours of work.
• In general we don't like abbreviations. I wonder if ExplorerCharacterSetMulti should be ExplorerCharacterSetMulticultural?
• I strongly recommend against the word "wrapper" for ExplorersWrapper. I understand why it was used, but can we come up with something without so much baggage?
• Should character sets be in a separate directory?

added 11/20:

• Instead of ExploresWrapper.createVisibleProperty() can you use DerivedProperty.valueEqualsConstant?
• All the strings added to NLI should live in joist_en strings html, even if their only usages are in NLI. That way we can reuse them everywhere we want. Just noting redundantly, please be VERY CONFIDENT about the name of the string keys, as they will live forever.
• Likely nothing to do here but I wanted to note, that in the de translation of NumberLineIntegersStrings.elevationAmountStringProperty, there is an extra space at the end that causes a layout annoyance: '‪{{value}} m ‬'
• I think it would be nice to have a central spot where all supported region and culture values are defined, and to be able to have some brief documentation (in code) about what the target of each is. I'm writing this because I'm a little confused about what to expect in Multicultural. Perhaps here: https://github.com/phetsims/joist/blob/e61c11c6bfd0481bba8c26fe110e3febd23ba3ef/js/preferences/RegionAndCulturePortrayal.ts#L31-L37. This is probably has much more design nuance and I don't think we need to go overboard, but just something about the target for this style.
• the package.json flag supportsDynamicLocale should be in simFeatures, not at the top level of phet (also see https://github.com/phetsims/chipper/issues/1410 to help this next time). (DONE in https://github.com/phetsims/number-line-integers/commit/54babaa29b545de9adf033e3be6c83c9e088143b)
• It would be nice to document where the drawing code came from for MoneyJarShapes.MONEY_BOX_SVG_STRING. Like what asset etc.
• It is sad to have lost the git history for PiggyBankNode, but probably not worth trying to resurrect it at this point (piggyBankShapes too).

[zepumph]

I made it to ExplorersWrapper today.

[zepumph]

I looked through all NLI-specific changes, and next I will look at changes in number-line-common.

[zepumph]

Alright I have gone through the common and distance number line repos to inspect changes. I played with the number-line-integers sim, looking for spots where the character sets and or dynamic locale seemed wrong or strange. I also worked with @marlitas over in https://github.com/phetsims/joist/issues/943 to familiarize myself with the common code for region and culture. Things are looking really good! Thanks for all you hard work @marlitas and let me know if you have any other thoughts or want to sync up about anything.

[marlitas]

I see that you added a color profile. Can you explain the motivation for this? Only interested as it pertains to the code review, since I'm getting my bearings on what the scope of the changes are.

Mostly because I thought that's the new pattern for color constants in sims. If that's how things are moving forward I thought it was an easy thing to create and add in with minor implications...

I strongly recommend against the word "wrapper" for ExplorersWrapper. I understand why it was used, but can we come up with something without so much baggage?

Totally fine with a rename... What about ExplorersController?

Should character sets be in a separate directory?

I was wondering about this... It felt strange to add another directory on top of model and view just for character sets though. It feels like it would break that pattern.

Instead of ExploresWrapper.createVisibleProperty() can you use DerivedProperty.valueEqualsConstant?

Sure, never used it before :-)

I think it would be nice to have a central spot where all supported region and culture values are defined, and to be able to have some brief documentation (in code) about what the target of each is. I'm writing this because I'm a little confused about what to expect in Multicultural. Perhaps here: https://github.com/phetsims/joist/blob/e61c11c6bfd0481bba8c26fe110e3febd23ba3ef/js/preferences/RegionAndCulturePortrayal.ts#L31-L37. This is probably has much more design nuance and I don't think we need to go overboard, but just something about the target for this style.

I can bring this up with AM. There's been a lot of discussion about this from a design perspective throughout.

It is sad to have lost the git history for PiggyBankNode, but probably not worth trying to resurrect it at this point (piggyBankShapes too).

Oh bummer, I'm sorry. I need to be better about checking on that with renames. I tend to forget about preserving the git history. Thanks for the reminder.

If they are in this range I strongly recommend that TypeScript conversion accompany this.

I agree I will do this moving forward.

[marlitas]

Remainder TODO:

• [x] Verbose description to accompany gross or awkward decisions will greatly help any future maintainer of the sim (not that your code is gross).

• [x] In general we don't like abbreviations. I wonder if ExplorerCharacterSetMulti should be ExplorerCharacterSetMulticultural?

• [x] I strongly recommend against the word "wrapper" for ExplorersWrapper. I understand why it was used, but can we come up with something without so much baggage?

• [x] Instead of ExploresWrapper.createVisibleProperty() can you use DerivedProperty.valueEqualsConstant?

• [x] All the strings added to NLI should live in joist_en strings html, even if their only usages are in NLI. That way we can reuse them everywhere we want. Just noting redundantly, please be VERY CONFIDENT about the name of the string keys, as they will live forever.

• [x] I think it would be nice to have a central spot where all supported region and culture values are defined, and to be able to have some brief documentation (in code) about what the target of each is.

• [x] It would be nice to document where the drawing code came from for MoneyJarShapes.MONEY_BOX_SVG_STRING. Like what asset etc.

[zepumph]

Totally fine with a rename... What about ExplorersController?

How about Explorer. Where it could have like Explorer.flying, etc. Another idea if we still like the suffix, "Character" or "CharacterSet"? It seems to fit with how we talk about region and culture and design speak. Perhaps I'm wrong though and that means something else. One sim could have three Character subtypes for africa (an explorer, a banker, etc). Each "Character" can have different visuals associated with it. Just a thought, no strong preference.

I was wondering about this... It felt strange to add another directory on top of model and view just for character sets though. It feels like it would break that pattern.

What about inside the view? explore/view/characters/

[marlitas]

How about Explorer. Where it could have like Explorer.flying, etc. Another idea if we still like the suffix, "Character" or "CharacterSet"?

I'm not a fan of Explorer, it sounds kind of model-ey to me. We had Kicker and KickerNode in CAV with Kicker being the model. I think ExplorerCharacters could work. I think plural would be helpful since it really is organizing and managing all of the explorers. I'll go ahead and make that change.

What about inside the view? explore/view/characters/ Love it.

[zepumph]

ExplorerCharacters

Can you help me understand where this class fits into the notion of a "character set"? Isn't this a single character in that set (now a portrayal)? Like you choose "usa", and then that sets a property to this class, so I'm worried that having two things that both have some semblance of plurality may be more confusing on the general preference side, even though it may be less confusing for the usages.

Part of this code review is to really lock in the definitions of the feature. RegionAndCulturePortrayal is not longer called "CharacterSet", that must have been done for a reason, so perhaps let's name this subtype to match that decision. Maybe now, after the rename, that actually frees up the term "Characters" to be best used to describe all the different entitires in the portrayal.

ugg. Thanks for letting me talk out loud.

Still think Characters is best? I think I just might!

[marlitas]

Can you help me understand where this class fits into the notion of a "character set"?

In the context of this sim it is three separate versions of the same character. So maybe ExplorerVersions?

In an ideal world I would refactor NLI to have an ExplorerNode that the PointController can just pull in and know how to handle. But in the spirit of disturbing the architecture as little as possible this ExplorerCharacters class was born. It is not a class that I would expect to be needed in every sim, and currently NLI is the only one I know for sure needs it. I believe LV added it into the Area Model sims just in case, but we haven't dug into the code enough to find out if it's actually needed there.

RegionAndCulturePortrayal is not longer called "CharacterSet", that must have been done for a reason, so perhaps let's name this subtype to match that decision.

Yeah I've been wondering about that, and was kind of dreading doing it until someone else pointed it out. You're right though. CM was confused when he saw RegionAndCultureCharacterSet, thinking that they were alphabet characters and not cartoon characters. That's when we changed to portrayal. I think that's probably more accurate and prone to less confusion... Fine I'll change it. 😅

Still think Characters is best? I think I just might!

Maybe... but I'm also not hating Versions... I also am getting to the point where my brain doesn't care and is fine with whatever name you throw at me... I'd probably be good with Potato at this point... 🤷🏽

[marlitas]

REVIEW, were all of these changed because of dynamic layout? Can you describe to me why they needed to be changed, and add doc about that if you think it is helpful.

const COIN_DEPOSIT_ANIMATION_START_Y = -95; // above the money jar, in screen coordinates, empirically determined
const COIN_DEPOSIT_ANIMATION_END_Y = -60; // inside the money jar, in screen coordinates, empirically determined
const COIN_WITHDRAWAL_ANIMATION_START_Y = 70; // inside the money jar, in screen coordinates, empirically determined
const COIN_WITHDRAWAL_ANIMATION_END_Y = 95; // below the money jar, in screen coordinates, empirically determined
...
const coinClipArea = Shape.rectangle( -100, -150, 200, 300 );
coinClipArea.moveTo( -20, -69 );
coinClipArea.lineTo( -20, 78 );
coinClipArea.lineTo( 20, 78 );
coinClipArea.lineTo( 20, -69 );
coinClipArea.close();
coinAnimationLayer.clipArea = coinClipArea;

These were changed because the artwork changed from a piggy bank to a money jar. I did update the documentation to reference the money jar now, but I don't know if any other documentation would be helpful.

[marlitas]

@Luisav1 can you help me with renaming any classes that extend RegionAndCulturePotrayal from CharacterSet to Portrayal? For example: ExplorerCharacterSet would become ExplorerPortrayal. We have to do this in the following repos:

• [x] area-model-common
• [x] arithmetic
• [x] energy-skate-park
• [x] soccer-common
• [x] number-line-common
• [x] number-line-distance
• [x] number-line-integers

[marlitas]

I had a hard time getting specific descriptions from designers on each region and culture portrayal so I went ahead with a basic description that I hope helps, but not quite sure it solves what @zepumph was hoping for.

I believe all the code review items have now been addressed. Over to @zepumph to look over and close if all looks well.

[zepumph]

Thanks! This is all looking really great. I definitely know how annoying it can be to handle a rename so late in the process of creating (one of my sims was renamed about 2 months after initial development #neverforget).

• It may be nice to have another look at usages of "character sets". Some doc looks out of date.
• The same for "CharacterSet", though most is local var and doc.
• Do we want to rename the localization option from characterSet?
• It would definitely be good to make sure terminology is settled and clear for the feature: https://github.com/phetsims/joist/blob/bd5efdf47413f488b3bed37bad5dec0f3428374d/js/preferences/PreferencesModel.ts#L90-L93
• RE: https://github.com/phetsims/joist/blob/8c300f6621a4351c89251dc5346a079bf4cb05e2/js/preferences/RegionAndCulturePortrayal.ts#L35. Does this mean that each time you refresh you get a different value? Cool! Let's make that a bit clearer.
• Looks like you forgot OCEANIA in your 12/23 doc.

[marlitas]

@Luisav1 can you update the characterSets terminology in Balancing Act? I know you've been working there, so I didn't want to interfere.

Once that's wrapped up we can assign to @zepumph again.

[Luisav1]

Oops, I forgot we are going with portrayals now. It's all renamed in Balancing Act. Assigning back to @zepumph.

[zepumph]

• I just did another search on our code base for character set, and there are still some usages. Some seem vestigial so I committed a change to portrayal. Others I had some questions about. I understand that "character set" was largely renamed to "portrayal" late in the game, but in my experience if both terminologies stick around, it will be confusing indefinitely (mostly coming from history with description features). Please let me know if those shouldn't have been renamed.
• In this paragraph it mentions that this is still being developed. To me it seems out of date, and we should just say what is currently in the panel. It is good stuff! Let's be confident about it in its current state.
• More generally, @pixelzoom mentioned on slack that it may be nice to add to the doc ad the top of RegionAndCulturePortrayal to mention that it can be more than just character artwork that changes.
• There are still usages in the dev all babel strings for ESP (searching for characterSets.). I don't think we want to get rid of those, but I thought I'd mention it.

Thanks for you patience here as we solidify this.

[marlitas]

In this paragraph it mentions that this is still being developed. To me it seems out of date, and we should just say what is currently in the panel. It is good stuff! Let's be confident about it in its current state.

It's talking about the localization tab in general which is true. It is still being developed and it's current state is definitely more of a "this works until we have time to finish it." I would not say that any designer feels it is completed.

[marlitas]

More generally, @pixelzoom mentioned on slack that it may be nice to add to the doc ad the top of RegionAndCulturePortrayal to mention that it can be more than just character artwork that changes.

I added the description used in the sim. Not sure if it does enough, but there are multiple places that state: "Portrayals of people, places, or objects"

[marlitas]

This should all be addressed. Back to @zepumph and @pixelzoom

[zepumph]

Great thanks. Ready to close for me.

[pixelzoom]

👍🏻