Closed zepumph closed 1 year ago
Questions:
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:
protected
.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)
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.
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.
From https://github.com/phetsims/scenery/issues/1494#issuecomment-1495230168 @pixelzoom said:
I agree! Let's clean up
disposer
and its usages, since we aren't going to use this among the above.