Code review #165

pixelzoom · 2021-08-27T02:06:27Z

PhET Code-Review Checklist (a.k.a "CRC")

The responsible dev is responsible for removing the irrelevant parts
A checked-off item doesn't mean "no problem here", it means "it was reviewed"
Problems can be noted in side issues that reference this issue, or through // REVIEW comments in the code

GitHub Issues

The following standard GitHub issues should exist. If these issues are missing, or have not been completed, pause code review until the issues have been created and addressed by the responsible dev.

results of memory testing for brands=phet, see Memory testing #130
performance testing and sign-off, see Performance testing and sign-off #131
review of pointer areas, see Review of pointer areas #124
credits (will not be completed until after RC testing), see credits #2

Build and Run Checks

If any of these items fail, pause code review.

Does the sim build without warnings or errors?
Does the html file size seem reasonable, compared to other similar sims?
Does the sim start up? (unbuilt and built versions)
Does the sim experience any assertion failures? (run with query parameter ea)
Does the sim pass a scenery fuzz test? (run with query parameters fuzz&ea)
Does the sim behave correctly when listener order is shuffled? (run with query parameters ea&shuffleListeners and ea&shuffleListeners&fuzz)
Does the sim output any deprecation warnings? Run with ?deprecationWarnings. Do not use deprecated methods in new code. See RewardNode uses deprecate Node.toImage vegas#95, vegas.RewardNode uses deprecated Node.toImage.`

Memory Leaks

Does a heap comparison using Chrome Developer Tools indicate a memory leak? (Describing this process is beyond the scope of this document.) Test on a version built using grunt --minify.mangle=false. Compare to testing results done by the responsible developer. Results can be found in Memory testing #130.
For each common-code component (sun, scenery-phet, vegas, …) that opaquely registers observers or listeners, is there a call to that component’s dispose function, or is it obvious why it isn't necessary, or is there documentation about why dispose isn't called? An example of why no call to dispose is needed is if the component is used in a ScreenView that would never be removed from the scene graph.
Are there leaks due to registering observers or listeners? The following guidelines should be followed unless documentation (in-line or in implementation-notes.md) describes why following them is not necessary.
- AXON: Property.link or lazyLink is accompanied by unlink.
- AXON: Property.multilink is accompanied by unmultilink.
- AXON: Creation of Multilink is accompanied by dispose.
- AXON: Creation of DerivedProperty is accompanied by dispose.
- AXON: Emitter.addListener is accompanied by removeListener.
- AXON: ObservableArrayDef.element*Emitter.addListener is accompanied by ObservableArrayDef.element*Emitter.removeListener
- SCENERY: Node.addInputListener is accompanied by removeInputListener
- TANDEM: Creation of an instrumented PhetioObject is accompanied by dispose.
Do all types that require a dispose function have one? This should expose a public dispose function that calls this.disposeMyType(), where disposeMyType is a private function declared in the constructor. MyType should exactly match the filename.

Performance

Play with sim, identify any obvious performance issues. Examples: animation that slows down with large numbers of objects; animation that pauses or "hitches" during garbage collection.
N/A If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter webgl=false)

Usability

Are UI components sufficiently responsive? (especially continuous UI components, such as sliders)
Are pointer areas optimized, especially for touch? (run with query parameter showPointerAreas)
Do pointer areas overlap? (run with query parameter showPointerAreas) Overlap may be OK in some cases, depending on the z-ordering (if the front-most object is supposed to occlude pointer areas) and whether objects can be moved.

Internationalization

Are there any strings that are not internationalized, and does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter stringTest=X. You should see nothing but 'X' strings.)
Does the sim layout gracefully handle internationalized strings that are longer than the English strings? (run with query parameters stringTest=double and stringTest=long)
Does the sim stay on the sim page (doesn't redirect to an external page) when running with the query parameter stringTest=xss? This test passes if sim does not redirect, OK if sim crashes or fails to fully start. Only test on one desktop platform. For PhET-iO sims, additionally test ?stringTest=xss in Studio to make sure i18n strings didn't leak to phetioDocumentation, see https://github.com/phetsims/phet-io/issues/1377
Avoid using concatenation to create strings that will be visible in the user interface. Use StringUtils.fillIn and a string pattern to ensure that strings are properly localized.
Use named placeholders (e.g. "{{value}} {{units}}") instead of numbered placeholders (e.g. "{0} {1}").
Make sure the string keys are all perfect, because they are difficult to change after 1.0.0 is published. Guidelines for string keys are:

(1) Strings keys should generally match their values. E.g.:
```
"helloWorld": {
  value: "Hello World!"
},
"quadraticTerms": {
  value: "Quadratic Terms"
}
```
(2) If a string key would be exceptionally long, use a key name that is an abbreviated form of the string value, or that captures the purpose/essence of the value. E.g.:
```
// key is abbreviated
"iWentToTheStore": {
  value: "I went to the store to get milk, eggs, butter, and sugar."
},

// key is based on purpose
"describeTheScreen": {
  value: "The Play Area is a small room. The Control Panel has buttons, a checkbox, and radio buttons to change conditions in the room."
}
```
(3) If string key names would collide, use your judgment to disambiguate. E.g.:
```
"simplifyTitle": {
   value: "Simplify!"
},
"simplifyCheckbox": {
   value: "simplify"
}
```
(4) String keys for screen names should have the general form "screen.{{screenName}}". E.g.:
```
  "screen.explore": {
    "value": "Explore"
  },
```
(5) String patterns that contain placeholders (e.g. "My name is {{first}} {{last}}") should use keys that are unlikely to conflict with strings that might be needed in the future. For example, for "{{price}}" consider using key "pricePattern" instead of "price", if you think there might be a future need for a "price" string.
(6) It is acceptable to prefix families of strings with a prefix, like so:

  "material.water": {
    "value": "Water"
  },
  "material.wood": {
    "value": "Wood"
  },
  "shape.block": {
    "value": "Block"
  },
  "shape.cone": {
    "value": "Cone"
  },

Nested substructure is not yet fully supported.

N/A If the sim was already released, make sure none of the original string keys have changed. If they have changed, make sure any changes have a good reason and have been discussed with @jbphet (it is likely that an issue like Translations will lose some strings the next time this is published off of master gravity-force-lab#166 should be created).

Repository Structure

The repository name should correspond to the sim title. For example, if the sim title is "Wave Interference", then the repository name should be "wave-interference".
Are all required files and directories present?
For a sim repository named “my-repo”, the general structure should look like this (where assets/, images/, mipmaps/ or sounds/ may be omitted if the sim doesn’t have those types of resource files).
```
my-repo/
  assets/
  doc/
    images/
      *see annotation
    model.md
    implementation-notes.md
  images/
    license.json
  js/
    (see section below)
  mipmaps/
    license.json
  sound/
    license.json
  dependencies.json
  .gitignore
  my-repo_en.html
  my-repo-strings_en.json
  Gruntfile.js
  LICENSE
  package.json
  README.md
```
*Any images used in model.md or implementation-notes.md should be added here. Images specific to aiding with documentation do not need their own license.
Verify that the same image file is not present in both images/ and mipmaps/. If you need a mipmap, use it for all occurrences of the image.
Is the js/ directory properly structured?
All JavaScript source should be in the js/ directory. There should be a subdirectory for each screen (this also applies for single-screen sims, where the subdirectory matches the repo name). For a multi-screen sim, code shared by 2 or more screens should be in a js/common/ subdirectory. Model and view code should be in model/ and view/ subdirectories for each screen and common/. For example, for a sim with screens “Introduction” and “Lab”, the general directory structure should look like this:
```
my-repo/
  js/
  common/
    model/
    view/
  introduction/
    model/
    view/
  lab/
    model/
    view/
  my-repo-main.js
  myRepo.js
  myRepoStrings.js
```
Do filenames use an appropriate prefix? Some filenames may be prefixed with the repository name, e.g. MolarityConstants.js in molarity. If the repository name is long, the developer may choose to abbreviate the repository name, e.g. EEConstants.js in expression-exchange. If the abbreviation is already used by another respository, then the full name must be used. For example, if the "EE" abbreviation is already used by expression-exchange, then it should not be used in equality-explorer. Whichever convention is used, it should be used consistently within a repository - don't mix abbreviations and full names.
Is there a file in assets/ for every resource file in sound/ and images/? Note that there is not necessarily a 1:1 correspondence between asset and resource files; for example, several related images may be in the same .ai file. Check license.json for possible documentation of why some reesources might not have a corresponding asset file.
Was the README.md generated by grunt published-README or grunt unpublished-README?
Does package.json refer to any dependencies that are not used by the sim?
Is the LICENSE file correct? (Generally GPL v3 for sims and MIT for common code, see this thread for additional information).
Does .gitignore match the one in simula-rasa?
In GitHub, verify that all non-release branches have an associated issue that describes their purpose.
Are there any GitHub branches that are no longer needed and should be deleted?
Does model.md adequately describe the model, in terms appropriate for teachers? model.md #126
Does implementation-notes.md adequately describe the implementation, with an overview that will be useful to future maintainers? CODE REVIEWER: Please review implementation-notes and wrap up implementation-notes.md #127.
Sim-specific query parameters (if any) should be identified and documented in one .js file in js/common/ or js/ (if there is no common/). The .js file should be named {{PREFIX}}QueryParameters.js, for example ArithmeticQueryParameters.js for the aritmetic repository, or FBQueryParameters.js for Function Builder (where the FB prefix is used).
Query parameters that are public-facing should be identified using public: true in the schema.
All sims should use a color file named MyRepoColors.js or, if using abbreviations, MRColors.js, and use ProfileColorProperty where appropriate, even if they have a single (default) profile (to support color editing and PhET-iO Studio). The ColorProfile pattern was converted to *Colors.js files in PhET-iO instrumentation for ColorProfile scenery-phet#515. Please see [GasPropertiesColors.js](https://github.com/phetsims/gas -properties/blob/master/js/common/GasPropertiesColors.js) for a good example.

Coding Conventions

This section deals with PhET coding conventions. You do not need to exhaustively check every item in this section, nor do you necessarily need to check these items one at a time. The goal is to determine whether the code generally meets PhET standards.

For example, both of these are acceptable:

Property.multilink(
  [ styleProperty, activeProperty, colorProperty ],
  ( style, active, color ) => {
    // some algorithm that uses style and active
} );

Property.multilink(
  [ styleProperty, activeProperty, colorProperty ],
  ( style, active ) => {
    // some algorithm that uses style and active
} );

This is not acceptable, because the 3rd parameter is incorrect.

Property.multilink(
  [ styleProperty, activeProperty, colorProperty ],
  ( style, active, lineWidth ) => {
    // some algorithm that uses style and active
} );

ES5 getters/setters defined in sims should be used judiciously when a Property exists, only when the benefit of conciseness outweighs the potential loss of readability. If ES5 getters/setters exist, try to not mix usage of them with Property access.
The model and view code should be written such that it makes no assumptions about the animation frame rate this it will encounter. The default max dt (delta time) value is defined in Screen.js, and smaller values should be used if the default is too large. There is no minimum value for dt, which implies that there is no maximum supported frame
rate. The sim should be able to handle this. For an example of a problem that resulted from assuming that a max rate of 60 FPS would be the norm, please see this GitHub issue.

Documentation

This section deals with PhET documentation conventions. You do not need to exhaustively check every item in this section, nor do you necessarily need to check these items one at a time. The goal is to determine whether the code generally meets PhET standards.

/**
 * Updates this node.
 * @abstract
 * @protected
 */
 update() {
   throw new Error( 'update must be implemented by subclass' );
 }

Type Expressions

This section deals with PhET conventions for type expressions. You do not need to exhaustively check every item in this section, nor do you necessarily need to check these items one at a time. The goal is to determine whether the code generally meets PhET standards.

Type expressions should conform approximately to Google Closure Compiler syntax. PhET stretches the syntax in many cases (beyond the scope of this document to describe).
Prefer the most basic/restrictive type expression when defining APIs. For example, if a client only needs to know that a parameter is {Node}, don’t describe the parameter as {Rectangle}.
All method parameters should have type expressions. For example @param {number} width.
In sim-specific code, options and fields should have type expressions when their type is not obvious from the context. “Obvious” typically means that the value type is clearly shown in the righthand-side of the definition. E.g. const width = 42 clear shows that width is {number}. E.g. const checkbox = new Checkbox(…) clearly shows that checkbox is {Checkbox}. If the type is obvious from the context, the developer may still provide a type expression at his/her discretion. Examples:
```
// @public {GameState} the current state of the game
this.gameState = this.computeGameState();

// @public (read-only) the width of the container
this.containerWidth = 150;

// @private the checkbox used to show particles
this.particlesVisibleCheckbox = new Checkbox(...);
```

In common code repositories all options and fields should have type expressions, regardless of their visibility, and regardless whether their type is obvious from the context. If the same examples from above appeared in common code:

// @public {GameState} the current state of the game
this.gameState = this.computeGameState();

// @public (read-only) {number} the width of the container
this.containerWidth = 150;

// @private {Checkbox} the checkbox used to show particles
this.particlesVisibleCheckbox = new Checkbox(...);

Type expressions for Enumeration values should be annotated as instances of that Enumeration, see examples in https://github.com/phetsims/phet-core/blob/master/js/Enumeration.js for more.

/**
 * @param {LeftOrRight} - whichHand
 */
 function getHand( whichHand ){
   if( whichHand === LeftOrRight.LEFT ){
     return new LeftHand();
   }
   else if( whichHand === LeftOrRight.RIGHT ){
     return new RightHand();
   }
  }

Type expressions for functions have a variety of possibilities, increasing in complexity depending on the case. In general note that {function} is not enough information. Here are some better options:
1. The most basic option it to use Google Closure Type syntax, for more info see https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System. This specifies the param/return types, but nothing more. Here are some examples:
  - @param {function()} noParamsAndNoReturnValue
  - @param {function(number)} giveMeNumberAndReturnNothing
  - @param {function(number, number):Vector2} getVector2
  - @param {function(new:Node)} createNode - a function that takes the Node constructor
2. When needing to be a bit more specific, add a name to parameters of the function. Sometimes this is all that is needed for clarity on what the param does:
  - @param {function(model:MyModel, length:number, name:string): Node} getLengthNode
  - @param {function(aSelfExplanatoryNameForAString:string): Node} getStringNode
3. If (2) isn't enough, use English to explain the parameters and return values. This is easy because they are named, and can be easily mentioned:
  - @param {function(model:MyModel, length:number, name:string): Node} getLengthNode - returns the length Node that you have always wanted, name is the name of the source of your aspirations, length is a special number according to the following 24 criteria. . .
4. If needing more complexity, or using jsdoc rendering tools (like PhET-iO documentation does), you must use a JSDoc compatible format, not (2) or (3), and you may need to use the more complicated solution. See JSDoc docs for more info. Here is an example of a named callback:
```
/**
 * @name mySpecialCallback
 * Converts a string to a number
 * @param {string}
 * @returns {number}
 */
/**
 * @param {mySpecialCallback} callback
*/
x = function( callback) { callback( 'still chocolate' ) };
```
Type expressions for anonymous Objects have a variety of possibilities, increasing in complexity depending on the case.
1. When the documentation is close by, then {Object} is still acceptable. This mainly applies to options and similar patterns:
  - @param {Object} [options] // this is great because of the extend call 5 lines down
2. When using an Object with specific properties, name them and their types like so:
  - @param {name:string, address:{street:string}, returnNode:function(number):Node, [shoeSize:number]} personalData // note that shoeSize is optional here
3. When you need a bit more explanation, keep the same type expression as (2), but feel free to outline specifics in English after the param name.
```
@ param {name:string, address:{street:string}, returnNode:function(number):Node, [shoeSize:number]} personalData - use english after to explain pieces of this
    (if needed, outline properties on their own lines)
    name is something
    address is something else
    returnNode does this thing
```
4. Not all objects have named keys like (2) and (3). Here is how to document dictionary-like Objects, where each key is some type, and the value is another type. For key value pairs use this:
  - {Object.<string, number>} Where keys are strings, and values are numbers.
  - {Object.<phetioID:string, count:number>} - naming each of these can help identify them too. Feel free to explain in English after the type expression if needed.
5. If things are too complicated for the above cases, use a *Def.js file (especially is used in more than one file), or a @typedef declaration right above the jsdoc that uses the typedef.
Look for cases where the use of type expressions involving Property subclasses are incorrect. Because of the structure of the Property class hierarchy, specifying type-specific Properties ({BooleanProperty}, {NumberProperty},...) may be incorrect, because it precludes values of type {DerivedProperty} and {DynamicProperty}. Similarly, use of {DerivedProperty} and {DynamicProperty} precludes values of (e.g.) {BooleanProperty}. Especially in common code, using {Property.<TYPE>} is typically correct, unless some specific feature of the Property subclass is required. For example, {Property.<boolean>} instead of {BooleanProperty}.

Visibility Annotations

This section deals with PhET conventions for visibility annotations. You do not need to exhaustively check every item in this section, nor do you necessarily need to check these items one at a time. The goal is to determine whether the code generally meets PhET standards.

Because JavaScript lacks visibility modifiers (public, protected, private), PhET uses JSdoc visibility annotations to document the intent of the programmer, and define the public API. Visibility annotations are required for anything that JavaScript makes public. Information about these annotations can be found here. (Note that other documentation systems like the Google Closure Compiler use slightly different syntax in some cases. Where there are differences, JSDoc is authoritative. For example, use Array.<Object> or Object[] instead of Array<Object>). PhET guidelines for visibility annotations are as follows:

Use @public for anything that is intended to be part of the public API.
Use @protected for anything that is intended for use by subtypes.
Use @private for anything that is NOT intended to be part of the public or protected API.
Put qualifiers in parenthesis after the annotation, for example:
- To qualify that something is read-only, use @public (read-only). This indicates that the given Property (AND its value) should not be changed by outside code (e.g. a Property should not have its value changed)
- To qualify that something is public to a specific repository, use (for example) @public (scenery-internal)
- For something made public solely for a11y, use @public (a11y)
- For something made public solely for phet-io, use @public (phet-io)
- Separate multiple qualifiers with commas. For example: @public (scenery-internal, read-only)

For JSDoc-style comments, the annotation should appear in context like this:

/**
 * Creates the icon for the "Energy" screen, a cartoonish bar graph.
 * @returns {Node}
 * @public
 */

For Line comments, the annotation can appear like this:

// @public {function(listener:function)} - Adds a listener
addListener: function( listener ) { /*...*/ }

Verify that every JavaScript property and function has a visibility annotation. Here are some helpful regular expressions to search for these declarations as PhET uses them.
- Regex for property assignment like x.y = something: [\w]+\.[\w]+\s=
- Regex for function declarations: [\w]+: function\(

Math Libraries

DOT/Utils.toFixed or DOT/Utils.toFixedNumber should be used instead of toFixed. JavaScript's toFixed is notoriously buggy. Behavior differs depending on browser, because the spec doesn't specify whether to round or floor.

IE11

IE is no longer supported. With that in mind remove IE-specific workarounds
Use string.includes and string.startsWith where possible.

Organization, Readability, and Maintainability

Accessibility

This section may be omitted if the sim has not been instrumented with accessibility features. Accessibility includes various features, not all are always include. Ignore sections that do not apply.

General

Are accessibility features integrated well into the code. They should be added in a maintainable way, even if that requires upfront refactoring. See the A11y section of implementation-notes.md.

Alternative Input

Does the sim pass an accessibility fuzz test? (run with query parameters fuzzBoard&ea)
Does this sim use specific keyboard shortcuts that overlap with global shortcuts? See Keyboard help content for hotkeys #93

Interactive Description

N/A

PhET-iO

This section may be omitted if the sim has not been instrumented for PhET-iO, but is likely good to glance at no matter.

Does instrumentation follow the conventions described in PhET-iO Instrumentation Guide? This could be an extensive bullet. At the very least, be sure to know what amount of instrumentation this sim supports. Describing this further goes beyond the scope of this document.
N/A PhET-iO instantiates different objects and wires up listeners that are not present in the PhET-branded simulation. It needs to be tested separately for memory leaks. To help isolate the nature of the memory leak, this test should be run separately from the PhET brand memory leak test. Test with a colorized Data Stream, and Studio (easily accessed from phetmarks). Compare to testing results done by the responsible developer and previous releases.
N/A Make sure unused PhetioObject instances are disposed, which unregisters their tandems.
Make sure JOIST dt values are used instead of Date.now() or other Date functions. Perhaps try phet.joist.elapsedTime. Though this has already been mentioned, it is necessary for reproducible playback via input events and deserves a comment in this PhET-iO section.
Are random numbers using DOT/dotRandom as an imported module (not a global), and all doing so after modules are declared (non-statically)? For example, the following methods (and perhaps others) should not be used: Math.random, _.shuffle, _.sample, _.random.
This also deserves re-iteration due to its effect on record/playback for PhET-iO.
Like JSON, keys for undefined values are omitted when serializing objects across frames. Consider this when determining whether toStateObject should use null or undefined values.
PhET prefers to use the term "position" to refer to the physical (x,y) position of objects. This applies to both brands, but is more important for the PhET-iO API. See location vs position phet-info#126
Are your IOType state methods violating the API of the core type by accessing private fields?
When defining a boolean Property to indicate whether something is enabled, use AXON/EnabledProperty. This should be done in both the model and the view.

The text was updated successfully, but these errors were encountered:

pixelzoom · 2021-08-27T02:10:19Z

This sim is ready for code review. All code has been cleaned up, and I completed a "self code review" in #163.

This is a big sim, ~15,000 lines of code. It makes heavy use of bamboo charts, and it has a relatively involved model. It will take some time to review. Definitely start by reading model.md and implementation-notes.md.

In order to stay on track for 9/30/21 publication, this code review needs to be completed no later than 9/13/2021.

@kathy-phet @arouinfar please assign a reviewer.

kathy-phet · 2021-09-01T20:50:12Z

@jonathanolson will do the Fourier code review, and also simultaneously be effectively code reviewing - or at least another perspective and eyes on - Bamboo to some degree.

@pixelzoom EDIT:
Also in Slack on 9/1:

Kathy Perkins 2:52 PM
@jonathanolson - Here is the code review issue for Fourier! Thanks! @pixelzoom is back on 9/13, so goal is completion by then. #165

Jonathan Olson 2:54 PM
Sounds good, thanks!

Chris Malley 3:38 PM
A quick update… Fourier sailed through dev-lite testing — testing is done, with a couple of quick things to address. So full dev test can start as soon as we’re done with code review, and I’ll have time to address code-review issues while I’m on vacation. So while 9/13 is the goal for completing code review, sooner is better, and will increase the chances that we can hit 9/30 production deploy.

Jonathan Olson 3:38 PM
About an hour ago Density is blocked on responses, so I did start the review

…umberControl, #165

…es#165, #716

…165

pixelzoom · 2021-09-21T04:06:20Z

@jonathanolson said in Slack:

Finished the initial code inspection, committed REVIEW comments and minor improvements tagged to #165. I'll be going through the rest of the checklist, and have some general notes too. Very nice code, pleasure to read!

Thanks. I've addressed all of the REVIEW comments, see commits above, feel free to review. A few were promoted to GitHub issues, linked above.

Let me know when you've completed the rest of the checklist.

pixelzoom · 2021-09-21T21:53:22Z

@jonathanolson and I discussed a few "polish" issues over Zoom, and I summarized them in #176.

@jonathanolson will continue working on the remaining items in the CRC checklist, so that we can proceed with dev testing.

jonathanolson · 2021-09-22T17:40:52Z

Max-width on second string "Choose your level"?

pixelzoom · 2021-09-22T18:02:21Z

I did have a maxWidth set for "Choose Your Level", but it was commented out. Looks like an unintentional change in 3a3b1f8. Fixed in the above commit.

jonathanolson · 2021-09-23T00:51:22Z

Performance is a bit slow on Wave Packet (pi/4 spacing), is it still similar to when performance was signed off?
If it's helpful, 160cbe7 could be done by passing { visibleProperty: this.visibleProperty } when creating the ClockFaceNode (as a general principle, no changes needed here).

Otherwise review all done, looking great!

pixelzoom · 2021-09-23T01:14:54Z

Performance is a bit slow on Wave Packet (pi/4 spacing), is it still similar to when performance was signed off?

@jonathanolson Were you testing performance with a built version? Performance of the unbuilt version is quite a bit slower, due to assertions. If you were testing with a built version, what was your test platform?

Nothing significant has changed since performance sign-off in #131.

If it's helpful, 160cbe7 could be done by passing { visibleProperty: this.visibleProperty } when creating the ClockFaceNode (as a general principle, no changes needed here).

Yes, I realize that, thanks. In this case, clockFaceNode is instantiated before super is called, so this.visibleProperty does not exist. So I chose to call clockFaceNode.setVisibleProperty rather than move instantiation after the constructor and call addChild.

jonathanolson · 2021-09-23T01:22:12Z

I was testing performance with a built version. Sounds like this issue can be closed then?

pixelzoom · 2021-09-23T01:36:45Z

Since you report that it was a built version, I'm going to reopen #131 (performance sign-off) to have @arouinfar explicitly check the Wave Packet screen with pi/4 spacing.

And yes, this issue can be closed.

pixelzoom added the dev:code-review label Aug 27, 2021

pixelzoom assigned kathy-phet and arouinfar Aug 27, 2021

pixelzoom mentioned this issue Aug 27, 2021

implementation-notes.md #127

Closed

kathy-phet assigned jonathanolson and unassigned kathy-phet and arouinfar Sep 1, 2021

kathy-phet added the priority:2-high label Sep 1, 2021

jonathanolson mentioned this issue Sep 8, 2021

Chart scaling documentation #173

Closed

jonathanolson added a commit that referenced this issue Sep 17, 2021

Review notes and minor fixes, see #165

65710e8

jonathanolson added a commit that referenced this issue Sep 20, 2021

Review notes, see #165

bfb64dc

jonathanolson added a commit that referenced this issue Sep 20, 2021

Review notes, see #165

6b9acf4

pixelzoom added a commit that referenced this issue Sep 20, 2021

fix naming of HorizontalDimensionalArrowsNode, #165

030c8a3

pixelzoom added a commit that referenced this issue Sep 20, 2021

fix non-existent font option for SumSymbolNode, #165

0578974

pixelzoom added a commit that referenced this issue Sep 20, 2021

document why NumberControl title is '', #165

c3931c7

pixelzoom added a commit that referenced this issue Sep 20, 2021

use strings.join for info dialog layout, #165

bc42d76

pixelzoom added a commit that referenced this issue Sep 20, 2021

address bogus FWMConstants import, #165

1a9a147

pixelzoom added a commit that referenced this issue Sep 20, 2021

delete bogus visibleProperty option to CanvasLinePlot, #165

8903668

pixelzoom added a commit that referenced this issue Sep 20, 2021

document why domainProperty.link is the responsibility of WavePacketN…

4b98e0f

…umberControl, #165

pixelzoom added a commit that referenced this issue Sep 20, 2021

add missing type expressions for params, #165

549b407

pixelzoom added a commit that referenced this issue Sep 20, 2021

add missing default options and merge, #165

48c9c4c

pixelzoom added a commit that referenced this issue Sep 20, 2021

delete options in instantiation of EmphasizedHarmonics, #165

93019fb

pixelzoom added a commit that referenced this issue Sep 20, 2021

document why I'm using for instead of map, #165

a908c87

pixelzoom added a commit that referenced this issue Sep 20, 2021

fix bogus FWMConstants misspelled import, #165

1f174e7

pixelzoom added a commit that referenced this issue Sep 20, 2021

delete options in instantiation of EmphasizedHarmonics, #165

307ee06

pixelzoom added a commit that referenced this issue Sep 20, 2021

replace for with map, #165

56f880d

pixelzoom added a commit that referenced this issue Sep 20, 2021

pass our visibleProperty to clockFaceNode, #165

160cbe7

pixelzoom added a commit to phetsims/sun that referenced this issue Sep 21, 2021

migrate pointer-area options to Checkbox, phetsims/fourier-making-wav…

d276701

…es#165, #716

pixelzoom mentioned this issue Sep 21, 2021

Investigate duplication related to CanvasLinePlot. #174

Closed

pixelzoom added a commit that referenced this issue Sep 21, 2021

delete REVIEW comment after promoting it to an issue, #165, #174

5bb5909

pixelzoom mentioned this issue Sep 21, 2021

ChartCanvasNode and the need to call update. phetsims/bamboo#45

Open

pixelzoom added a commit that referenced this issue Sep 21, 2021

promote REVIEW comment to phetsims/bamboo#45, #165

b432a87

pixelzoom added a commit that referenced this issue Sep 21, 2021

add doc about how BarTrack listeners are used to emphasize harmonics, #…

2557556

…165

pixelzoom mentioned this issue Sep 21, 2021

AmplitudeSlider exposes problems with vertical Slider. #175

Closed

pixelzoom mentioned this issue Sep 21, 2021

Code polishes #176

Closed

6 tasks

This was referenced Sep 22, 2021

CanvasLinePlot's lack of support for color Property phetsims/bamboo#42

Open

Having a separate implementation for Canvas is problematic in the longterm. phetsims/bamboo#46

Open

Code review phetsims/bamboo#47

Open

jonathanolson assigned pixelzoom Sep 22, 2021

pixelzoom added a commit that referenced this issue Sep 22, 2021

uncomment maxWidth for "Choose You Level", #165

7723d56

pixelzoom removed their assignment Sep 22, 2021

jonathanolson assigned pixelzoom and unassigned jonathanolson Sep 23, 2021

pixelzoom closed this as completed Sep 23, 2021

This was referenced Sep 23, 2021

Performance testing and sign-off #131

Closed

Fourier: Making Waves Main checklist #1

Open

Team testing prior to RC #178

Closed

Font for summation index is too large in Discrete screen. #186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review #165

Code review #165

pixelzoom commented Aug 27, 2021 •

edited by jonathanolson

Loading

pixelzoom commented Aug 27, 2021 •

edited

Loading

kathy-phet commented Sep 1, 2021 •

edited by pixelzoom

Loading

pixelzoom commented Sep 21, 2021 •

edited

Loading

pixelzoom commented Sep 21, 2021

jonathanolson commented Sep 22, 2021

pixelzoom commented Sep 22, 2021 •

edited

Loading

jonathanolson commented Sep 23, 2021 •

edited by pixelzoom

Loading

pixelzoom commented Sep 23, 2021 •

edited

Loading

jonathanolson commented Sep 23, 2021

pixelzoom commented Sep 23, 2021 •

edited

Loading

Code review #165

Code review #165

Comments

pixelzoom commented Aug 27, 2021 • edited by jonathanolson Loading

PhET Code-Review Checklist (a.k.a "CRC")

GitHub Issues

Build and Run Checks

Memory Leaks

Performance

Usability

Internationalization

Repository Structure

Coding Conventions

Documentation

Type Expressions

Visibility Annotations

Math Libraries

IE11

Organization, Readability, and Maintainability

Accessibility

General

Alternative Input

Interactive Description

PhET-iO

pixelzoom commented Aug 27, 2021 • edited Loading

kathy-phet commented Sep 1, 2021 • edited by pixelzoom Loading

pixelzoom commented Sep 21, 2021 • edited Loading

pixelzoom commented Sep 21, 2021

jonathanolson commented Sep 22, 2021

pixelzoom commented Sep 22, 2021 • edited Loading

jonathanolson commented Sep 23, 2021 • edited by pixelzoom Loading

pixelzoom commented Sep 23, 2021 • edited Loading

jonathanolson commented Sep 23, 2021

pixelzoom commented Sep 23, 2021 • edited Loading

pixelzoom commented Aug 27, 2021 •

edited by jonathanolson

Loading

pixelzoom commented Aug 27, 2021 •

edited

Loading

kathy-phet commented Sep 1, 2021 •

edited by pixelzoom

Loading

pixelzoom commented Sep 21, 2021 •

edited

Loading

pixelzoom commented Sep 22, 2021 •

edited

Loading

jonathanolson commented Sep 23, 2021 •

edited by pixelzoom

Loading

pixelzoom commented Sep 23, 2021 •

edited

Loading

pixelzoom commented Sep 23, 2021 •

edited

Loading