Improve Disposal patterns in the project

[zepumph]

While talking to @jonathanolson about an incredible amount of new disposal code needed over in phetsims/scenery-phet#769 (comment) because of dynamic StringProperties, we brainstormed some ways to improve this pattern to limit friction. I'll try some stuff out and see how it goes, and then report back.

Our current idea is a disposer option to Node/PhetioObject which is a TEmitter (disposeEmitter) or an object with a dispose emitter. Then setting it will automatically listen for when I should dispose myself. This can be wired up on construction, so the dispose code path can be much simpler.

[zepumph]

@jonathanolson can you take an initial review please. I created a base type rather than packing this into PhetioObject. Disposable lives in AXON. I created unit tests, hard coded a Node mutator key (should it be in ParallelDOM?), and went through usages of disposeEmitters (even local variables) and converted them to use Disposable properites instead to test things out. Many cases still add listeners to the disposeEmitter, which I know isn't the end goal, but I did use disposer a few times as well. I wasn't quite ready to go all in, as I would like to hear your thoughts first. Marking as ready for review with high priority. Let's make sure this doesn't sit here in master.

[zepumph]

Here is a specific question to help define the pattern:

• Please take a look at GrabReleaseKeyboardHelpSection and describe how we should implement the new pattern. All items needing disposal are dependency injected into the supertype, and so I cannot say dispose: this like I want to. Here is patch that demonstrates what I wish I could do for disposal.

Index: js/keyboard/help/GrabReleaseKeyboardHelpSection.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/keyboard/help/GrabReleaseKeyboardHelpSection.ts b/js/keyboard/help/GrabReleaseKeyboardHelpSection.ts
--- a/js/keyboard/help/GrabReleaseKeyboardHelpSection.ts	(revision c24c717f720756268cef1076136e4f8ba1b4926f)
+++ b/js/keyboard/help/GrabReleaseKeyboardHelpSection.ts	(date 1668121879851)
@@ -37,22 +37,25 @@
 
     // the visible heading string
     const headingStringProperty = new PatternStringProperty( SceneryPhetStrings.keyboardHelpDialog.grabOrReleaseHeadingPatternStringProperty, {
-      thing: thingAsTitle
+      thing: thingAsTitle,
+      disposer: this
     } );
 
     // the visible label string
     const labelStringProperty = new PatternStringProperty( SceneryPhetStrings.keyboardHelpDialog.grabOrReleaseLabelPatternStringProperty, {
-      thing: thingAsLowerCase
+      thing: thingAsLowerCase,
+      disposer: this
     } );
 
     // the string for the PDOM
     const descriptionStringProperty = new PatternStringProperty( SceneryPhetStrings.a11y.keyboardHelpDialog.grabOrReleaseDescriptionPatternStringProperty, {
-      thing: thingAsLowerCase
+      thing: thingAsLowerCase,
+      disposer: this
     } );
 
-    const spaceKeyNode = TextKeyNode.space();
-    const enterKeyNode = TextKeyNode.enter();
-    const icons = KeyboardHelpIconFactory.iconOrIcon( spaceKeyNode, enterKeyNode );
+    const spaceKeyNode = TextKeyNode.space( { disposer: this } );
+    const enterKeyNode = TextKeyNode.enter( { disposer: this } );
+    const icons = KeyboardHelpIconFactory.iconOrIcon( spaceKeyNode, enterKeyNode, { disposer: this } );
     const labelWithContentRow = KeyboardHelpSectionRow.labelWithIcon( labelStringProperty, icons, {
       labelInnerContent: descriptionStringProperty,
       iconOptions: {
@@ -61,14 +64,6 @@
     } );
 
     super( headingStringProperty, [ labelWithContentRow ], options );
-    this.disposeEmitter.addListener( () => {
-      headingStringProperty.dispose();
-      labelStringProperty.dispose();
-      descriptionStringProperty.dispose();
-      spaceKeyNode.dispose();
-      enterKeyNode.dispose();
-      icons.dispose();
-    } );
   }
 }
 

[zepumph]

A ping here would be great, @jbphet @samreid and I ran into this new code today and would like to start using it elsewhere.

[zepumph]

A couple of thoughts about disposal improvements, and potential things to discuss at a dev meeting around disposal:

Give Disposable tour

Disposal questions:
How in control should a Node be of disposing its children?
How explicit should should disposal be?
Is it bad to dispose many Nodes that don't need it?
Do we dispose Layout boxes?

Improvements in Node to assist in disposing with dependency injection in mind:

• Node.disposeChildren:boolean option?
• Potential Node feature:
  • Node's "owned children"
  • "disposeAllChildren"
  • "disposeAllChildrenRecursive"
  • Opt out of certain children due to dependency injection (by marking the dependency explicitly);
  • opt out of direct children only? Or any descendants?
  • default to no owned children.

[zepumph]

After a memory leak that I just found in Disposable (above), I'm going to mark this as blocking publication until we can work out more of the kinks here.

[zepumph]

• I don't think that the disposer option setter is working with mixins/DelayedMutate. Apply this patch and notice that it fails to call my custom disposer:

Index: js/common/view/RAPKeyboardHelpContent.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/RAPKeyboardHelpContent.ts b/js/common/view/RAPKeyboardHelpContent.ts
--- a/js/common/view/RAPKeyboardHelpContent.ts	(revision adcb9d9b47229b4cfcf71783b3ec03842df167fd)
+++ b/js/common/view/RAPKeyboardHelpContent.ts	(date 1669420003328)
@@ -16,8 +16,9 @@
 import TextKeyNode from '../../../../scenery-phet/js/keyboard/TextKeyNode.js';
 import ratioAndProportion from '../../ratioAndProportion.js';
 import RatioAndProportionStrings from '../../RatioAndProportionStrings.js';
-import { EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
+import { combineOptions, EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
 import KeyboardHelpSectionRow from '../../../../scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.js';
+import TinyEmitter from '../../../../axon/js/TinyEmitter.js';
 
 type SelfOptions = EmptySelfOptions;
 export type RAPKeyboardHelpContentOptions = SelfOptions & TwoColumnKeyboardHelpContentOptions;
@@ -66,29 +67,35 @@
 }
 
 class BothHandsHelpSection extends KeyboardHelpSection {
-  private readonly disposeBothHandsHelpSection: () => void;
+
+  public constructor( providedOptions?: KeyboardHelpSectionOptions ) {
 
-  public constructor( options?: KeyboardHelpSectionOptions ) {
+    const options = combineOptions<KeyboardHelpSectionOptions>( {
+      disposer: new TinyEmitter()
+    }, providedOptions );
 
-    const wOrSIcon = KeyboardHelpIconFactory.iconRow( [ new LetterKeyNode( 'W' ), new LetterKeyNode( 'S' ) ] );
+    options.disposer.addListener( () => {
+      console.log( 'why not ever called?' );
+    } );
+    const disposerOptions = { disposer: options.disposer };
+
+    const wOrSIcon = KeyboardHelpIconFactory.iconRow( [ new LetterKeyNode( 'W' ), new LetterKeyNode( 'S', disposerOptions ) ] );
     const moveLeftHand = KeyboardHelpSectionRow.labelWithIcon( RatioAndProportionStrings.moveLeftHandStringProperty,
       wOrSIcon, {
         labelInnerContent: RatioAndProportionStrings.a11y.keyboardHelp.leftHandDescriptionStringProperty
       } );
 
-    const arrowKeysRow = KeyboardHelpIconFactory.upDownArrowKeysRowIcon();
     const moveRightHand = KeyboardHelpSectionRow.labelWithIcon( RatioAndProportionStrings.moveRightHandStringProperty,
-      arrowKeysRow, {
+      KeyboardHelpIconFactory.upDownArrowKeysRowIcon( disposerOptions ), {
         labelInnerContent: RatioAndProportionStrings.a11y.keyboardHelp.rightHandDescriptionStringProperty
       } );
 
-    const shiftIcon = TextKeyNode.shift();
     const moveInSmallerSteps = KeyboardHelpSectionRow.labelWithIcon( RatioAndProportionStrings.moveHandsInSmallerStepsStringProperty,
-      shiftIcon, {
+      TextKeyNode.shift( { disposer: options.disposer } ), {
         labelInnerContent: RatioAndProportionStrings.a11y.keyboardHelp.handsInSmallerStepsDescriptionStringProperty
       } );
 
-    const numberToNumberIcon = KeyboardHelpIconFactory.iconToIcon( new NumberKeyNode( 0 ), new NumberKeyNode( 9 ) );
+    const numberToNumberIcon = KeyboardHelpIconFactory.iconToIcon( new NumberKeyNode( 0 ), new NumberKeyNode( 9 ), disposerOptions );
     const jumpBothHands = KeyboardHelpSectionRow.labelWithIcon( RatioAndProportionStrings.jumpBothHandsStringProperty,
       numberToNumberIcon, {
         labelInnerContent: RatioAndProportionStrings.a11y.keyboardHelp.jumpBothHandsDescriptionStringProperty
@@ -96,18 +103,6 @@
 
     super( RatioAndProportionStrings.moveBothHandsSimultaneouslyStringProperty,
       [ moveLeftHand, moveRightHand, moveInSmallerSteps, jumpBothHands ], options );
-
-    this.disposeBothHandsHelpSection = () => {
-      wOrSIcon.dispose();
-      numberToNumberIcon.dispose();
-      arrowKeysRow.dispose();
-      shiftIcon.dispose();
-    };
-  }
-
-  public override dispose(): void {
-    this.disposeBothHandsHelpSection();
-    super.dispose();
   }
 }
 

[jonathanolson]

This seems like a discussion item, not a "review of code" item currently, so I'm removing the review label.

[samreid]

This issue was marked as blocking publication just after #1494 (comment), Dec 5. @zepumph or @jonathanolson is that still accurate?

[jonathanolson]

I don't feel like it's blocking personally, but I want to hear from @zepumph

[zepumph]

Through all discussion with @samreid and @jonathanolson since creating Disposable and the disposer option, I feel quite confident that Disposable should stick around as a super type. I don't, however, see a future in disposer, as other strategies discussed in this issue are likely to end up being better implementations. With that in mind I added deprecated markings to the disposer feature of Disposable, and now feel good unassigning until we have bandwidth to find the next worthy disposing pattern.

I also want to note that I started out just now attempting to go through the 35 or so usages of disposer to get rid of them, and I really didn't like that work. disposer clearly fills a niche that we don't have great support for right now (disposing things that don't get variable names, for one).

Unassigning.

[zepumph]

In phetsims/scenery-phet#769 (comment) @pixelzoom and I were very excited about a Node.disposeDeep that iterates through children and calls dispose on them. This doesn't have the same over-bearing "all or nothing" feel that some of the other strategies have, as it will be up to the caller if they want to use the aggressive disposeDeep function.

Also it seems like disposeDeep should be graceful to if things are called twice, basically like "just blow it all away and I don't care what it takes". That would be helpful for the keyboard help content, but I'm unsure about what else.

[pixelzoom]

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.

[zepumph]

Let's handle Disposable over in phetsims/axon#433, and keep brainstorming about larger improvements over here in this epic level issue.

[zepumph]

Oh, probably #1523 is the spot for a disposeDeep investigation.

[zepumph]

Disposable is in much better shape since phetsims/axon#433. It has been the base class for over a year and has only been helpful and made things cleaner for me. I don't need to solve for any other hard disposal issues. Node children disposal or disposeDeep don't feel like they would be generally adopted, or as valuable as they would be challenging to create. At least for me. I hope that if any others feel differently about this issue, that they will reopen, refocus, and reprioritize it. Closing