Waves Intro code review

samreid commented 5 years ago

@kathy-phet requested a code review for the changes that allowed us to adapt Wave Interference into Waves Intro, where each scene is split up into a separate screen. The two files that were introduced to support this change are BasicScreen.js and MediumScreen.js. I'll self-assign at first to make sure they are ready for review.

samreid commented 5 years ago

@pixelzoom can you please review these two files at your convenience? It's less than 100 lines of code and I believe we are hoping to get this published for the beginning of school in the fall.

pixelzoom commented 5 years ago

I just did pull-all.sh in preparation for this review. Waves Intro starts up OK. But Wave Interference is failing at startup:

assert.js:22 Uncaught Error: Assertion failed: initial scene should be specified
   at window.assertions.assertFunction (assert.js:22)
   at new BasicScreen (BasicScreen.js:36)
   at new WavesScreen (WavesScreen.js:32)
   at wave-interference-main.js:50
   at Namespace.window.phet.joist.launchSimulation (SimLauncher.js:50)
   at doneLoadingImages (SimLauncher.js:71)
   at Object.launch (SimLauncher.js:125)
   at wave-interference-main.js:39
   at Object.execCb (require-2.3.6.js:1696)
   at Module.check (require-2.3.6.js:883)

Back to @samreid to fix before I continue review.

pixelzoom commented 5 years ago

While you're fixing...

If I understand the doc for MediumScreen, each Screen in Waves Intro is creating all 3 scenes, but only showing one scene. This seems incredibility wasteful, not to mention problematic for PhET-iO. You need 3 scene instances, but you're creating 9 scenes (3 screens x 3 scenes) and ignoring 6 of them. Something that takes a list of scenes would be more general and flexible -- provide all 3 scenes for WavesScreen, provide only the 1 desired scene for each Screen in Waves Intro.

Also recommended to rename BasicScreen to BaseScreen, as it is described as "Base class for WavesScreen and MediumScreen".

samreid commented 5 years ago

I reverted the incorrect assertion but will need to take a closer at the options and the preceding comment after lunch.

samreid commented 5 years ago

I renamed BasicScreen => BaseScreen and switched to a pattern that only creates components for the specified screens. @pixelzoom can you please review?

UPDATE: Also, @arouinfar can you please test on phettest after these changes, testing in particular to verify that nothing is broken with respect to (a) layering and (b) making sure that each scene has the appropriate features?

arouinfar commented 5 years ago

@arouinfar can you please test on phettest after these changes, testing in particular to verify that nothing is broken with respect to (a) layering and (b) making sure that each scene has the appropriate features?

@samreid nothing appears to have been disturbed.

arouinfar commented 5 years ago

@samreid actually I have to walk that back. The ResetAllButton is broken. When pressed, it gets stuck like this. The sim resets, but is frozen until the browser is refreshed.

samreid commented 5 years ago

Thanks @arouinfar, I fixed the reset problem. Can you please re-test?

arouinfar commented 5 years ago

The ResetAllButton is now working in master. Thanks @samreid.

pixelzoom commented 5 years ago

Review is completed.

Overall:

[x] While I don't like the number of tests for the existence of scenes that this solution requires (e.g. if ( model.waterScene )), I understand how you got here, and it seems like a justifiable compromise. Recommended to say something about this in either WaveModel.js or implementation-notes.js -- noting what you're doing and why.

Specific issues:

[x] Screen names are not localized. They currently appear as string literals in waves-intro-main.js (e.g. name: 'Water'), and need to be moved to waves-intro-strings_en.json.
[x] The API for Array indexOf is documented as "-1 if not found". Tests like this one in WaveModel.js should be changed accordingly:
if ( options.scenes.indexOf( 'waterScene' ) >= 0 )
if ( options.scenes.indexOf( 'waterScene' ) === -1 )
[x] wave-interference/js/medium/ directory violates PhET directory structure conventions. There is no "medium" screen. Recommended to move MediumScreen.js to either wave-interference/js/common/ or waves-intro/js/common/. The latter may be more appropriate, since MediumScreen.js is only used in waves-intro, but I also understand the desire to keep all code in wave-interference.

samreid commented 5 years ago

Thanks for the review, I addressed the points raised in the preceding comment. @pixelzoom can you please re-review? Close if all is well.

pixelzoom commented 5 years ago

I reviewed commits, looks good. Closing.

phetsims / wave-interference

Waves Intro code review #414