Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cannot determine if dispose requirements have been met #266

Closed
pixelzoom opened this issue Jul 31, 2019 · 2 comments
Closed

cannot determine if dispose requirements have been met #266

pixelzoom opened this issue Jul 31, 2019 · 2 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 31, 2019

Related to code review #247

  • 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.

Searching for dispose() {, I find only 2 definitions of dispose. Due to deficiencies in #244 and #265, I have no way to determine if this is sufficient. implementation-notes.md should include this information in a general description of memory management. See for example the "Memory Management" subsection of https://github.com/phetsims/gas-properties/blob/master/doc/implementation-notes.md#general-considerations.

@pixelzoom
Copy link
Contributor Author

Ditto for:

  • 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 screen view that would never be removed from the scene graph.

@chrisklus
Copy link
Contributor

The two defined dispose functions are in EnergyChunkNode and EnergyChunkWanderController. Another dynamic type, ElementFollower, has a method stopFollowing which unlinks its locationBeingFollowedProperty.

A similar blanket statement was added to implementation notes over in #265

There are also not any dynamically allocated UI common-code components used in this sim.

@jbphet and I are ready to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants