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

insufficient documentation for calls to link, addListener, etc. #156

Closed
1 task
pixelzoom opened this issue Aug 28, 2019 · 3 comments
Closed
1 task

insufficient documentation for calls to link, addListener, etc. #156

pixelzoom opened this issue Aug 28, 2019 · 3 comments

Comments

@pixelzoom
Copy link
Contributor

For this item in code review #143:

  • Are there leaks due to registering observers or listeners? The following guidelines should be followed unless there it is obviously no need to unlink, or documentation (in-line or in the implementation nodes)added about why following them is not necessary. Unlink is not needed for properties contained in classes that are never disposed of,
    such as primary model and view classes that exist for the duration of the sim.
    - [ ] AXON: Property.link is accompanied by Property.unlink.
    - [ ] AXON: Creation of DerivedProperty is accompanied by dispose.
    - [ ] AXON: Creation of Multilink is accompanied by dispose.
    - [ ] AXON: Events.on is accompanied by Events.off.
    - [ ] AXON: Emitter.addListener is accompanied by Emitter.removeListener.
    - [ ] SCENERY: Node.on is accompanied by Node.off
    - [ ] TANDEM: PhET-iO instrumented PhetioObject instances should be disposed.

I inspected all occurrences of "link(". Sometimes there's a comment, e.g. in CurveFittingModel:

      // These are unlinked in removePoint
      point.positionProperty.link( this.updateCurveFit );
      point.isInsideGraphProperty.link( this.updateCurveFit );
      point.deltaProperty.link( this.updateCurveFit );

But many times there's no information about whether unlink is needed or not, or why. E.g. in CurveFittingModel:

      // validate Property values and update curve fit
      this.orderProperty.link( order => {

        // ensure the order is 1, 2 or 3: linear, quadratic or cubic
        assert && assert( order === 1 || order === 2 || order === 3, `invalid order: ${order}` );
        this.updateCurveFit();
      } );
      this.fitProperty.link( this.updateCurveFit );

There's also no description of the sim's general policy in implementation-notes.md, which would make such comments unnecessary when they conform to that policy. E.g. something like "all Properties exist for the lifetime of the sim and don't need to be unlinked unless otherwise noted in code comments".

I also inspected DerivedProperty instantiations, and there's no info about whether they need to be dispose and why.

Ditto for Emitter addListener.

So there's some work to be done here, documenting which registrations need or don't need to be unregistered.

@SaurabhTotey
Copy link
Member

I tried finding all missing comments, but it is possible that I may have missed a few. Assigning to @pixelzoom to review.

@pixelzoom
Copy link
Contributor Author

@SaurabhTotey I found a few more in the above commit. Feel free to close after reviewing.

@pixelzoom pixelzoom assigned SaurabhTotey and unassigned pixelzoom Aug 31, 2019
SaurabhTotey pushed a commit that referenced this issue Sep 1, 2019
See #156
@SaurabhTotey
Copy link
Member

It all seems good to me. Closing.

@SaurabhTotey SaurabhTotey removed their assignment Sep 1, 2019
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

2 participants