Disposable problems

[zepumph]

From phetsims/scenery#1494 (comment) @pixelzoom said:

In #1494 (comment), one of my concerns about Disposable and the direction of this issue was:

(4) It requires a new "base class of everything"

Today I took a not-too-close look at Dispoable, and so far I've found:

• 1 serious memory leak, see Add support for dynamic locale to Keyboard Help scenery-phet#769 (comment)
• 1 very-important field that should not be public and mutable (see public isDisposed = false)
• Methods that are annotated as @deprecated.

And this issue is currently unassigned!

In its current state, Disposable is kind of scary. And since it's the base class of PhetioObject, cleaning this up should be high (top?) priority. Raising priority and labeling for dev meeting discussion.

I agree! Let's clean up disposer and its usages, since we aren't going to use this among the above.

[zepumph]

Questions:

• Should a passed in disposeEmitter be disposed on dispose? I don't think so, since it goes against how we always handle dependency injection and disposal.

[pixelzoom]

Should a passed in disposeEmitter be disposed on dispose? ...

I don't understand why setDisposeEmitter is provided -- it's an unnecessary convenience, and only raises questions like this. My vote is to remove this feature. But if you choose to keep it, no, Disposable should not dispose of anything that it did not instantiate.

A couple of other things with Disposable:

• Unless there's a use case for instantiating Disposable directly, the constructor should be protected.
• All of the fields should be documented.

[zepumph]

I am really liking this cleanup. There were 37 usages of disposer, and each one felt like it was an obfuscating pattern, not assisting with much.

I believe I have done all items that @pixelzoom has mentioned, and I confirmed that there is no memory leak when creating 40 keyboard help dialogs in Ratio and Proportion (only 2MB increase)

[pixelzoom]

Changes look really great, thanks for doing this.

I'm not entirely sure if this._isDisposed = true should be done after this._disposeEmitter.emit(). I can imagine a listener checking isDisposed and getting confused. But you documented it well:

  // Marked true when this Disposable has had dispose() called on it (after disposeEmitter is fired)
  private _isDisposed = false;

... so I guess leave it as is?

Back to @zepumph in case there's anything else to be done.

[zepumph]

Great thought! After a discussion with @samreid a week ago or so, I feel like it makes sense that the flag is for "i have completely disposed" and not "I am currently disposing" which is much more challenging to calculate and to utilize effectively. We actually removing an exact assertion that you are mentioning, and instead reorganized the callback to guarantee that the view would only dispose once the model had already disposed fully.

I'm going to close this. Thanks for the speedy review.