phetsims / greenhouse-effect

"Greenhouse Effect" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Identify things that are not disposable #336

Closed pixelzoom closed 11 months ago

pixelzoom commented 11 months ago

For code review https://github.com/phetsims/greenhouse-effect/issues/331 ...

  • [ ] 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. Note that it's also acceptable (and encouraged!) to describe what needs to be disposed in implementation-notes.md.
  • [ ] 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. ...

Since implementation-notes.md #326 is not completed, there's no documentation yet about what should/not be disposed.

I see only 4 occurrences of public override dispose and no uses of isDisposable or Disposable.assertNotDisposable. That leads me to believe that there are (a lot of?) things that are not disposable that are not being (defensively) treated as such. And calling dispose on those things would probably not fail, but would result in a memory leak.

@jbphet we should probably discuss this one.

pixelzoom commented 11 months ago

@jbphet and I discussed on Zoom. Take aways:

jbphet commented 11 months ago

I've implemented the items @pixelzoom and I discussed (see the comment above) and regression tested. It all looks good and seems to behave correctly, at least so far. Closing.