Add support for dynamic locale to Keyboard Help

[pixelzoom]

Keyboard Help (scenery-phet/js/keyboard) currently does not support dynamic strings, and @jessegreenberg reports that dynamic layout is untested.

In https://github.com/phetsims/geometric-optics/issues/441, I'm adding support for dynamic layout to Geometric Optics, and it has many translated strings that appear in Keyboard Help. I'm going to convert what is needed for Geometric Optics, then pass along to @jessegreenberg to finish the rest.

This is blocking for https://github.com/orgs/phetsims/projects/44.

[pixelzoom]

Ugh, this code is still in JavaScript. This would be so much easier if it was in TypeScript, because it's a type change fromstring to string | TReadOnlyProperty<string> type change.

[pixelzoom]

I made good progress for Geometric Optics, see screenshot below. Everything that's sim-specific is now supported by common code. I haven't seen any layout problems yet.

What I addressed in common code:

• All of the utility functions in KeyboardHelpSectionRow.
• All of the common-code help sections, except SliderControlsKeyboardHelpSection.
• Converted remaining files to TypeScript (wish I'd done this first)

TODO:

• [x] Add support for dynamic strings in SliderControlsKeyboardHelpSection - see ComboBoxKeyboardHelpSection for an example, this blocks Geometric Optics
• [x] Add support for dynamic strings in KeyboardHelpSection.getGrabReleaseHelpSection
• [x] Address TODO comments that reference this issue
• [x] PhET-iO instrumentation? There's no PhET-iO instrumentation anywhere in this code. So making StringProperties discoverable in Studio by instrumenting their associated Text/RichText nodes is not possible. I don't know if that's s requirement for https://github.com/orgs/phetsims/projects/44 - you should check with @kathy-phet.

Over to @jessegreenberg to complete this work.

[zepumph]

This is a big one for me over in https://github.com/phetsims/ratio-and-proportion/issues/499, co-assigning.

[pixelzoom]

Reviewing scenery-phet issues for Q4 planning...

There has been no progress on the TODO list in https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1232096259. Assigning to @kathy-phet to prioritize.

[zepumph]

Subset of https://github.com/phetsims/scenery/issues/1298

[zepumph]

I removed the two TODOs in KeyboardHelpSection, because we are supporting (from an API perspective) passing in a TReadOnlyProperty to the PDOM/Voicing code (readingBlockContent/innerContent).

[zepumph]

• [x] With all the new PatternStringProperties being created, we need to implement dispose for KeyboardHelpSection to prevent memory leaks with phet-io state recreating the KeyboardHelpDialog.

[zepumph]

• Please note that the dispose issue is quite a bit more intense than I originally thought. Ratio and Proportion racked up 70MB of memory leak in a few minutes setting a new KeyboardHelpDialog every frame. I believe that each icon created with Text in KeyboardHelpIconFactory will need to support dispose. This is definitely a blocking issue for PhET-iO sims with a keyboard help dialog. UPDATE: I think this is a lie. Likely the leaks are elsewhere. We never dispose the KeyboardHelpDialog content. It lives forever, even though the dialog comes and goes. I created https://github.com/phetsims/joist/issues/874 to help grok the confusion there.

If we need to support disposal of KeyboardHelpSections, then maybe we should start with this patch:

```diff Index: scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts b/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts --- a/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts (revision c7a36059fca7e5d4b24bb63e7d3ed8b8ef13aefc) +++ b/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts (date 1666385944805) @@ -52,6 +52,7 @@ export default class SliderControlsKeyboardHelpSection extends KeyboardHelpSection { public static readonly ArrowKeyIconDisplay = ArrowKeyIconDisplay; + private readonly disposeSliderControlsKeyboardHelpSection: () => void; public constructor( providedOptions?: SliderControlsKeyboardHelpSectionOptions ) { @@ -175,7 +176,32 @@ // assemble final content for KeyboardHelpSection const content = [ adjustSliderRow, adjustSliderInSmallerStepsRow, adjustInLargerStepsRow, jumpToMinimumRow, jumpToMaximumRow ]; + super( options.headingString, content, options ); + + this.disposeSliderControlsKeyboardHelpSection = () => { + keyboardHelpDialogVerbSliderStringProperty.dispose(); + keyboardHelpDialogVerbInSmallerStepsStringProperty.dispose(); + keyboardHelpDialogVerbInLargerStepsStringProperty.dispose(); + keyboardHelpDialogDefaultStepsStringProperty.dispose(); + keyboardHelpDialogSmallerStepsStringProperty.dispose(); + keyboardHelpDialogLargerStepsStringProperty.dispose(); + jumpToMinimumStringProperty.dispose(); + jumpToMaximumStringProperty.dispose(); + jumpToMinimumDescriptionStringProperty.dispose(); + jumpToMaximumDescriptionStringProperty.dispose(); + + [ shiftKeysString, keysStringProperty ].forEach( property => { + + // We only own these for disposal if a PatternStringProperty + property instanceof PatternStringProperty && property.dispose(); + } ); + }; + } + + public override dispose(): void { + this.disposeSliderControlsKeyboardHelpSection(); + super.dispose(); } } Index: scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts b/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts --- a/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts (revision c7a36059fca7e5d4b24bb63e7d3ed8b8ef13aefc) +++ b/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts (date 1666385282172) @@ -9,6 +9,7 @@ * @author Jesse Greenberg (PhET Interactive Simulations) */ +import Emitter from '../../../../axon/js/Emitter.js'; import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import optionize, { combineOptions } from '../../../../phet-core/js/optionize.js'; import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js'; @@ -85,6 +86,8 @@ // when the row is activated with a click. public readonly readingBlockContent: VoicingResponse | null; + public disposeEmitter: Emitter; + public constructor( text: Text | RichText, label: Node, icon: Node, providedOptions?: KeyboardHelpSectionRowOptions ) { const options = optionize()( { readingBlockContent: null @@ -94,6 +97,11 @@ this.label = label; this.icon = icon; this.readingBlockContent = options.readingBlockContent; + this.disposeEmitter = new Emitter(); + } + + public dispose(): void { + this.disposeEmitter.emit(); } /** @@ -124,9 +132,14 @@ iconBox.innerContent = options.labelInnerContent; - return new KeyboardHelpSectionRow( labelText, labelBox, iconBox, { + const keyboardHelpSectionRow = new KeyboardHelpSectionRow( labelText, labelBox, iconBox, { readingBlockContent: options.readingBlockContent || options.labelInnerContent } ); + keyboardHelpSectionRow.disposeEmitter.addListener( () => { + labelText.dispose(); + labelIconGroup.dispose(); + } ); + return keyboardHelpSectionRow; } /** Index: scenery-phet/js/keyboard/help/KeyboardHelpSection.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts b/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts --- a/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts (revision c7a36059fca7e5d4b24bb63e7d3ed8b8ef13aefc) +++ b/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts (date 1666385775689) @@ -79,6 +79,8 @@ private readonly iconVBox: VBox; private readonly contentHBox: HBox; + private readonly disposeKeyboardHelpSection: () => void; + public static readonly DEFAULT_VERTICAL_ICON_SPACING = DEFAULT_VERTICAL_ICON_SPACING; /** @@ -164,6 +166,15 @@ this.keyboardHelpSectionRows = content; this.setReadingBlockNameResponse( this.generateReadingBlockNameResponse() ); + + this.disposeKeyboardHelpSection = () => { + headingText.dispose(); + }; + } + + public override dispose(): void { + this.disposeKeyboardHelpSection(); + super.dispose(); } /** Index: ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts b/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts --- a/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts (revision f8c92c9b2dcdd581b6551a55bdf302588abeca0d) +++ b/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts (date 1666385153425) @@ -24,8 +24,10 @@ class RAPKeyboardHelpContent extends TwoColumnKeyboardHelpContent { + private readonly disposeRAPKEyboardHelpContent: () => void; + /** - * @param challengeHelpSection - keyboard help section for determining how to change the target ratio + * @param challengeHelpSection - keyboard help section for determining how to change the target ratio. We will dispose this! * @param [providedOptions] */ public constructor( challengeHelpSection: KeyboardHelpSection, providedOptions?: RAPKeyboardHelpContentOptions ) { @@ -43,10 +45,23 @@ withCheckboxContent: true } ); - const leftContent = [ moveLeftOrRightHandHelpSection, new BothHandsHelpSection() ]; + const bothHandsHelpSection = new BothHandsHelpSection(); + const leftContent = [ moveLeftOrRightHandHelpSection, bothHandsHelpSection ]; const rightContent = [ challengeHelpSection, basicActionsHelpSection ]; super( leftContent, rightContent, providedOptions ); + + this.disposeRAPKEyboardHelpContent = () => { + moveLeftOrRightHandHelpSection.dispose(); + bothHandsHelpSection.dispose(); + challengeHelpSection.dispose(); + basicActionsHelpSection.dispose(); + }; + } + + public override dispose(): void { + this.disposeRAPKEyboardHelpContent(); + super.dispose(); } }

[zepumph]

UPDATE: I will likely apply the above disposal patch after proceeding/finishing with https://github.com/phetsims/joist/issues/874

[zepumph]

Also note much of this was covered by https://github.com/phetsims/scenery-phet/issues/780

[zepumph]

Not blocking for PhET brand.

[samreid]

@zepumph indicated that he and @pixelzoom are taking the lead on this, still I wanted to check the status.

I tested Gravity and Orbits fuzzing in the state wrapper and saw large memory leaks. 100MB over a minute. There is no keyboard help dialog. But I saw the downstream sim creating more and more PreferencesDialogs. Perhaps the state engine clears the capsules before re-triggering creation? The places where this may show up in use cases:

• Loading a state into Studio over and over
• Running a playback via states
• Even just opening the state wrapper and opening the preferences dialog is leaking at about 1MB/sec. Would probably crash after a while

I confirmed that PhET brand gravity and orbits is not leaking.

[pixelzoom]

I'm seeing huge memory increases for both phet and phet-io brands, and I doubt that it's all related to keyboard help.

For geometric-optics (master, unbuilt):

For brand=phet, I see a large increase in heap size. The first 3 snapshots were formerly 42MB, 50MB, 54MB (see https://github.com/phetsims/geometric-optics/issues/372). They are now 59MB, 88MB, 85MB.

For brand=phet-io, I see a MASSIVE increase in heap size. The first 3 snapshots were formerly 44MB, 54MB, 56MB (see https://github.com/phetsims/geometric-optics/issues/373). They are now 92MB, 300MB, 128MB. The 92MB is at startup, before keyboard help has even been opened.

These tests were run with unbuilt versions, while the original tests were run with built versions. So I expected to see some difference, but nothing this extreme.

What is responsible for this huge increase in memory?

[samreid]

(At least part of the) memory issue is due to https://github.com/phetsims/chipper/issues/1323

[samreid]

I measured that it takes about 70ms and 1MB to create a PreferencesDialog in Gravity and Orbits PhET brand.

[zepumph]

With @samreid today I got to a good stopping point.

Here were some pretty nice memory leak testing tips that I will note here.

1. Take a memory profile on startup without doing anything. We will compare to this throughout
2. On startup, create and dispose 10 KeyboardHelpDialogs with something like this (@samreid in https://github.com/phetsims/joist/issues/878 was able to get away with creating 100):

     for ( let i = 0; i < 10; i++ ) {
       keyboardHelpDialogCapsule.getElement();
       keyboardHelpDialogCapsule.disposeElement();
     }

3. Open dev tools, take a memory snap shot, and sort by deltas, comparing against the initial. Look for spots where there are 10 new, and 0 deleted, with a delta of 10 (one per dialog). That is a great bet that you have a memory leak!
4. For dynamic strings, basically you can just search for all Text, RichText, and VoicingText instances to make sure they are being disposed.

[zepumph]

This is definitely a lot better, but we should still take a look to ensure that all is good when creating a lot in the state wrapper.

[zepumph]

I think I helped this issue a bit over in https://github.com/phetsims/joist/issues/878 because of the Dialog work. No matter, after creating 100 keyboard help dialogs and disposing them on startup. The memory only increased from 66->68MB. I feel like our dispose work is done here for Ratio and Proportion, Friction, and the common code that it uses. I tried to test the other common code content as I went, but I am not as confident in it.

[zepumph]

@pixelzoom, it may be good to touch base here with you. Can you do a review and let me know if you think I'm on the right track. The main files to investigate are KeyboardHelpSection and KeyboardHelpSectionRow. Perhaps the best way forward would be for you to do some memory testing in GO. Steps to do a test I was happy with.

1. load GO and take a memory snapshot
2. Apply this patch, reload, and take a memory snapshot.
3. I saw memory at 58MB when creating and then disposing 0 keyboardHelpDialogs. 171MB for 10 and 400MB for 30.

Index: js/KeyboardHelpButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/KeyboardHelpButton.ts b/js/KeyboardHelpButton.ts
--- a/js/KeyboardHelpButton.ts  (revision 0d162fab6359e197a55a634316e0a02e3008ffb0)
+++ b/js/KeyboardHelpButton.ts  (date 1667430198865)
@@ -72,7 +72,7 @@
     super( icon, backgroundColorProperty, options );

     keyboardHelpDialogCapsule = new PhetioCapsule<KeyboardHelpDialog>( tandem => {
-
+      console.log( 'new keyboardDialog' );
       // Wrap in a node to prevent DAG problems if archetypes are also created
       return new KeyboardHelpDialog( screens, screenProperty, {
         tandem: tandem,
@@ -87,6 +87,11 @@
     backgroundColorProperty.link( backgroundColor => {
       icon.image = backgroundColor.equals( Color.BLACK ) ? keyboardIcon_png : keyboardIconOnWhite_png;
     } );
+
+    for ( let i = 0; i < 10; i++ ) {
+      keyboardHelpDialogCapsule.getElement();
+      keyboardHelpDialogCapsule.disposeElement();
+    }
   }
 }

This memory leak is because GO-specific content isn't disposing its rows/sections/icons. I think that the best review would be to take things for a test drive. Let me know if you want to discuss anything.

[zepumph]

Two questions here:

1. Should individual letter keys be translatable? Perhaps we should put that trust into translators to do the right thing. @jessegreenberg what do you think?
2. I noticed in RAP in hebrew that some keys were translated to blank. Why?

[zepumph]

I spoke with @jessegreenberg and we definitely agree that letter keys should be translatable since word keys are. Unassigning him and making a new issue to tackle that.

Also @pixelzoom please note that I am working on https://github.com/phetsims/scenery-phet/issues/787 so potentially we won't need to dispose many of those icons.

[zepumph]

Moving to a hopeful commit point here in the next hour or so. Things are looking good! I have been able to remove a lot of the boilerplate from above commits.

[pixelzoom]

@zepumph since you're still actively working on this issue, I'm going to add you to assignees.

[zepumph]

At this time I would recommend @pixelzoom hold off on further review. @jonathanolson and I brainstormed some new patterns for disposal and are playing with them over in https://github.com/phetsims/scenery/issues/1494. They have potentially to greatly reduce the boilerplate needed for disposing keyboard help content. Unassigning @pixelzoom for now.

[pixelzoom]

The 3 issues linked immediately above are for sims that will fail in the State wrapper because they do not support dispose for keyboard-help content. When common-code support propers dispose of keyboard-help content, those sim-specific issues should be addressed.

[pixelzoom]

@zepumph @jonathanolson FYI, in https://github.com/phetsims/ph-scale/issues/249#issuecomment-1319265006, we needed to unblock ph-scale for publication. So we decided to disable alternative input and comment out createKeyboardHelpNode in all screens, to workaround the memory leak issue.

[zepumph]

• [x] For this issue (from https://github.com/phetsims/ratio-and-proportion/issues/525): KeyboardHelpSection.generateReadingBlockNameResponse should use a DerivedProperty to formulate these strings

[zepumph]

A bit more investigation into current memory leaks and also some refactoring into the new disposer pattern today. Some success, but things are going a bit slowly. I'll keep cranking next week.

[zepumph]

As of this morning, creating and disposing 50 keyboard help dialogs only created 4MB of heap (from 61 -> 65). I think the memory leak has been fixed.

Same with Friction from 37.5->41

[zepumph]

There are just a couple more items here before I'm ready to give it back to @pixelzoom:

• review above topics for checkboxes etc
• consider converting more keyboard help stuff to the new disposer pattern.

[pixelzoom]

It sounds like @zepumph would like me to review. So self-assigning and labeling for review.

I'm not familiar with "the new disposer pattern". I don't see it described in this issue, or in PhET's software design patterns doc. @zepumph can you please catch me up?

[zepumph]

I still have a few loose ends before i toss it back to you. The disposer pattern is still getting worked out, but see Disposable.ts and the disposer option for a sneak peak. Ill let you know about your next steps.

[zepumph]

From https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1232096259:

PhET-iO instrumentation? There's no PhET-iO instrumentation anywhere in this code. So making StringProperties discoverable in Studio by instrumenting their associated Text/RichText nodes is not possible. I don't know if that's s requirement for https://github.com/orgs/phetsims/projects/44 - you should check with @kathy-phet.

We do not want to add instrumentation to the content of the help dialog, but just like with other non-important text, it is possible to customize within the model strings section of studio.

I believe all else is done.

@pixelzoom I think I'm ready for a review here. Things are quite scattered, but I believe the best way to proceed is to follow steps in https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1301473507 to show the keyboard memory leak when disposing, and to help convey progress as you implement dispose. Please feel free to use the new disposer pattern demonstrated in many spots like GrabReleaseKeyboardHelpSection and ComboBoxKeyboardHelpSection. Let me know if you want to pair or have any questions.

[pixelzoom]

I figured out how this works, but...

The original problem was that we needed to delete all children of a Node in some cases, specifically in the case of keyboard help where we had a memory leak. Instead we got a framework for "dispose strategies" that is overly-clever, challenging to understand/maintain, and invasive (integrated into PhET's most basic classes, like PhetioObject).

This is an excellent example of the framework trap antipattern. From Learning Agile, Chapter 7:

When developers overplan for edge cases or add too many hooks, they're being too clever. Instead of thinking about building code that's simple and easy to change, they're thinking about building code that's too complex, too abstract, or solves tomorrow's problems and not just today's.

This brings us to what we call the framework trap, an antipattern that stems from extreme developer cleverness. The framework trap is what we call it when a developer has to solve a single problem or perform a task, but instead of just writing code to do that one thing, he writes a larger framework that can be used in the future to solve similar problems or perform similar tasks.”

Specific review comments about Disposable and its use:

• Use sites are obfuscated. When children use their parent's disposeEmitter as their disposer, they will be disposed when dispose is called on that parent. That's even complicated to write out as a sentence. And it is not at all obvious when looking at use sites like GrabReleaseKeyboardHelpSection.

• { disposer: options.disposeEmitter } is heavily duplicated at use sites.

• It's easy to forget to pass disposer to some child, thereby not disposing that child, and leaking memory. So much for the framework solving the original problem.

• Disposable.ts header documentation is:

 * Type to implement disposal strategies, with a disposer option and a disposeEmitter.

This 1-liner is a woefully inadequate. It tells me almost nothing ("Type to implement disposal strategies"), it's somewhat incorrect (it's not a "type", it's a class and associated types), and it includes details that don't belong in an overview ("with a disposer option and a disposeEmitter"). As a framework, it needs overview, responsibilities, motivation, examples,...

• In Disposalbe.ts, this comment sounds scary. The this.disposeMyClass pattern is used everywhere. Why do we need to "be careful"? Is this a problem in general, or in specific situations where that pattern is (mis)used? Examples?

  // Called after all code that is directly in `dispose()` methods, be careful with mixing this pattern and the
  // `this.disposeMyClass()` pattern.
  public readonly _disposeEmitter: TEmitter = new TinyEmitter();

• In Disposable.ts, this cast is really disappointing. Casts are sometimes necessary when porting JavaScript to TypeScript. If you're resorting to a cast in new TypeScript code, you need to rethink your design.

// type case because instanceof check isn't flexible enough to check for all that implement TDisposable
return ( this._disposer instanceof Disposable ? this._disposer._disposeEmitter : this._disposer ) as TEmitter;

So... I'm so not a fan of this solution, in so many ways. It's overly-clever. It's complexity on top of complexity. It's invasive. It's a framework that no one needs. And I predict that it will come back to haunt PhET. I recommend deleting it and replacing with a simpler solution that addresses the original problem.

A few simpler solutions that would have solved the original problem:

• Do this in each parent that needs to dispose its children. It's simple, obvious, and local.

public override dispose() {
  this.children.forEach( child => child.dispose() );
  super.dispose();
}

• Add the above to Node, so that it's even more obvious at call sites:

public disposeDeep() {
  this.children.forEach( child => child.dispose() );
  super.dispose();
}

• Change the signature of dispose to public dispose( deep = false ): void, and use dispose( true ) when you want to dispose all children.

[samreid]

This issue reminded me of https://github.com/phetsims/axon/issues/93#issuecomment-215510393 where we discussed a similar pattern. I wanted to take this proposal for a test drive, so I updated a CCK file to use this pattern, and it turned out OK:

```diff Subject: [PATCH] Add SimVersion for use in MigrationEngine, see https://github.com/phetsims/phet-io/issues/1899 --- Index: js/model/ACVoltage.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/model/ACVoltage.ts b/js/model/ACVoltage.ts --- a/js/model/ACVoltage.ts (revision 13048a728d780aad982b483389ccd17344d665ed) +++ b/js/model/ACVoltage.ts (date 1670907227215) @@ -64,32 +64,38 @@ this.maximumVoltageProperty = new NumberProperty( options.voltage, { tandem: tandem.createTandem( 'maximumVoltageProperty' ), - range: ACVoltage.MAX_VOLTAGE_RANGE + range: ACVoltage.MAX_VOLTAGE_RANGE, + disposer: this } ); this.frequencyProperty = new NumberProperty( ACVoltage.DEFAULT_FREQUENCY, { tandem: tandem.createTandem( 'frequencyProperty' ), - range: ACVoltage.FREQUENCY_RANGE + range: ACVoltage.FREQUENCY_RANGE, + disposer: this } ); this.phaseProperty = new NumberProperty( 0, { range: new Range( -180, 180 ), tandem: tandem.createTandem( 'phaseProperty' ), - units: MathSymbols.DEGREES + units: MathSymbols.DEGREES, + disposer: this } ); // These more specific Properties are ANDed with isEditableProperty, so it works as another gate. this.isPhaseEditableProperty = new BooleanProperty( true, { - tandem: tandem.createTandem( 'isPhaseEditableProperty' ) + tandem: tandem.createTandem( 'isPhaseEditableProperty' ), + disposer: this } ); this.isFrequencyEditableProperty = new BooleanProperty( true, { - tandem: tandem.createTandem( 'isFrequencyEditableProperty' ) + tandem: tandem.createTandem( 'isFrequencyEditableProperty' ), + disposer: this } ); this.isVoltageEditableProperty = new BooleanProperty( true, { - tandem: tandem.createTandem( 'isVoltageEditableProperty' ) + tandem: tandem.createTandem( 'isVoltageEditableProperty' ), + disposer: this } ); this.time = 0; @@ -100,17 +106,6 @@ return [ this.frequencyProperty, this.phaseProperty, this.maximumVoltageProperty, ...super.getCircuitProperties() ]; } - // Dispose of this and PhET-iO instrumented children, so they will be unregistered. - public override dispose(): void { - this.maximumVoltageProperty.dispose(); - this.frequencyProperty.dispose(); - this.phaseProperty.dispose(); - this.isPhaseEditableProperty.dispose(); - this.isFrequencyEditableProperty.dispose(); - this.isVoltageEditableProperty.dispose(); - super.dispose(); - } - /** * @param time - total elapsed time * @param dt - delta between last frame and current frame ```

To me, it feels more declarative and less imperative. But perhaps not a big advantage in this simple case. @zepumph and @jonathanolson can you please recommend a few usage sites where I can see this pattern helps even more?

[zepumph]

I swear I wasn't ignoring https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1346823550. There is a lot there, and I have been thinking deeply about how best to proceed there. I would like to pick up general disposal strategies over in https://github.com/phetsims/scenery/issues/1494 and come back here to implement whatever patterns we deem best there. Marking on hold.

[zepumph]

Over in https://github.com/phetsims/scenery/issues/1494 I have become confident enough in using disposeEmitter to make sure we take care of memory issues. I found a few others above, and I think there is one more Text to do. I'll come back soon.

[zepumph]

Ok. After the above commits, I'm feeling good about the memory management of the keyboard help dialog. I just tested Ratio and Proportion, and that creating/disposing 50 and then 100 keyboard help dialogs only increases the memory by ~1MB. Please note that disposer is now deprecated, and we can work towards getting rid of it once we come up with a better solution over in https://github.com/phetsims/scenery/issues/1494

From here, I think we are ready for @pixelzoom to take a look at this same comment from a while ago. This is a review/test drive, where by working in one of your sims, you can also note about how things are working generally.

[pixelzoom]

@zepumph said:

From here, I think we are ready for @pixelzoom to take a look at this same comment from a while ago.

I have no idea what you're asking me to do here. Please clarify.

[zepumph]

Sorry, I meant to re-link to it. Instructions in https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1301473507 talk about how to make sure that a keyboard help dialog for your sim doesn't have a memory leak in PhET-iO. Common code should be set up to handle this at this point, so it is most likely just disposing your sim-specific components. Please see RAPKeyboardHelpContent as an example. I do not believe that this is the cleanest code, but our disposal pattern has never been, instead we have preferred verbosity and consistency. Let me know if you'd like to discuss.

[pixelzoom]

@zepumph said:

Instructions in https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1301473507 talk about how to make sure that a keyboard help dialog for your sim doesn't have a memory leak in PhET-iO.

I still don't understand. Am I still supposed to apply the patch, as the comment instructs? Why don't I see the patch code in KeyboardHelpButton.ts? What exactly am I supposed to be reviewing?

@zepumph Let's schedule a time to review this together on Zoom, rather than tossing the issue back and forth.

[pixelzoom]

Disposable.ts has a memory leak. The line added in this diff is missing, so disposeEmitter listeners are never freed.

  public dispose(): void {
    assert && assert( !this.isDisposed, 'Disposable can only be disposed once' );
    this._disposeEmitter.emit();
+   this._disposeEmitter.dispose();
    this.isDisposed = true;
  }

[pixelzoom]

@zepumph and I reviewed on Zoom. Summary:

• There's a memory leak in Disposable.ts, see https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1495060141

• Let's not make KeyboardHelpSection dispose its content. It's confusing having one special case that's unlike anything else. Address TODO comments in GOKeyboardHelpContent.ts when this has been done.

• Investigate having a method like disposeDeep that calls dispose for a branch of the tree rooted as some Node. Nodes in that branch would still need to have proper dispose methods. But disposeDeep would be a way to say (for example) "yes, I want my keyboard-help content and everything it involves to be disposed".

Assigning to @zepumph to address those things.

Leaving this assigned to me also. I'm going to commit changes to GOSim.ts and GOKeyboardHelpContent.ts. My next step is to test using the patch in https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1301473507, which @zepumph clarified is a way of testing keyboard-help disposal without having to use the State Wrapper.

[pixelzoom]

Using the procedure and patch in https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1301473507, here are heap sizes for geometric-optics:

• 65.9MB with 0 KeyboardHelpDialog instances
• 67.6MB with 10 instances
• 68.5MB with 100 instances

In https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1301473507, @zepumph tells me what to expect when there's a leak, but doesn't tell me what to expect when there's not a leak. I'd expect the heap sizes to be the same, which they are not. I took another look through GOKeyboardHelpContent.ts, and I don't see anything that I'm failing to dispose.

So... I'm guessing that there's still a small leak somewhere in joist or scenery-phet.

@zepumph I'm going to unassign myself. Let me know if you need anything else.

[zepumph]

See https://github.com/phetsims/axon/issues/433 for the issues discussed above with Disposable.

Investigate having a method like disposeDeep that calls dispose for a branch of the tree rooted as some Node. Nodes in that branch would still need to have proper dispose methods. But disposeDeep would be a way to say (for example) "yes, I want my keyboard-help content and everything it involves to be disposed".

Should be done in https://github.com/phetsims/scenery/issues/1523.

As for the addition of 4MB or so when creating 100 dialogs, I do not believe that this is an issue. It is very possible that this is mostly/some in the browser/view logic weeds, and I have spent enough time diving through memory testing tools to know that it is likely not worth the effort to find them at this time.

I have also gotten rid of the automatic disposal of KeyboardHelpSectionRows in KeyboardHelpSection.

@pixelzoom will you please review this issue. I am not sure what else is left that won't be handled by side issues.

[pixelzoom]

It looks like we've covered everything here. Good work, and closing.

I'm going to do https://github.com/phetsims/ph-scale/issues/253 and https://github.com/phetsims/fourier-making-waves/issues/227 (which were linked above) while this is fresh in my mind.