ScreenIcon does not support dynamic icon

[pixelzoom]

While working on https://github.com/phetsims/calculus-grapher/issues/139, I discovered that ScreenIcon makes the (now incorrect) assumption that its iconNode has a static size. That's not the case if the iconNode contains a string Property that may change via PhET-iO or via change of locale.

[pixelzoom]

I spot checked quite a few sims and did not see any regressions.

@jessegreenberg randomly selected to review. Please close if this looks OK.

[jessegreenberg]

Change looks good! I also spot checked a few sims and don't see any layout differences. I couldn't find a dynamic screen icon to test this improvement specifically but trust it works.

The iconNode is not owned by the ScreenIcon, is this a place where the listener needs to be disposed? I would say "no" because the ScreenIcon exists for life of sim in phet brand, but I don't know about phet-io cases.

[pixelzoom]

Thanks @jessegreenberg. You are correct that, to properly dispose of a ScreenIcon, the iconNode.localBoundsProperty listener needs to be removed. You're also correct that ScreenIcons are never disposed -- and that includes for PhET-iO. But just to be safe/future-proof, I'll go ahead and implement a proper dispose method.

[pixelzoom]

Looks like I need to address a TypeScript problem with Screen.ts in https://github.com/phetsims/joist/issues/902 before I can add dispose to ScreenIcon. I'm going to stash my patch here in the meantime:

patch

```diff Subject: [PATCH] implement dispose for ScreenIcon, https://github.com/phetsims/joist/issues/901 --- Index: js/Screen.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Screen.ts b/js/Screen.ts --- a/js/Screen.ts (revision 928a209c598b5756e712eeb1ace440e1a7dea25a) +++ b/js/Screen.ts (date 1674598265426) @@ -84,8 +84,8 @@ public readonly nameProperty: TReadOnlyProperty; public readonly showScreenIconFrameForNavigationBarFill: string | null; - public readonly homeScreenIcon: Node | null; - public navigationBarIcon: Node | null; + public readonly homeScreenIcon: ScreenIcon | null; + public navigationBarIcon: ScreenIcon | null; public readonly showUnselectedHomeScreenIconFrame: boolean; public readonly createKeyboardHelpNode: null | ( ( tandem: Tandem ) => Node ); // joist-internal public readonly pdomDisplayNameProperty: TReadOnlyProperty; @@ -160,9 +160,10 @@ // Create a default homeScreenIcon, using the Screen's background color if ( !options.homeScreenIcon ) { - options.homeScreenIcon = new Rectangle( 0, 0, MINIMUM_HOME_SCREEN_ICON_SIZE.width, MINIMUM_HOME_SCREEN_ICON_SIZE.height, { + options.homeScreenIcon = new ScreenIcon( new Rectangle( 0, 0, MINIMUM_HOME_SCREEN_ICON_SIZE.width, MINIMUM_HOME_SCREEN_ICON_SIZE.height ), { fill: options.backgroundColorProperty.value, - stroke: 'black' + maxIconWidthProportion: 1, + maxIconHeightProportion: 1 } ); } @@ -172,8 +173,8 @@ } // Validate icon sizes - validateIconSize( options.homeScreenIcon, MINIMUM_HOME_SCREEN_ICON_SIZE, HOME_SCREEN_ICON_ASPECT_RATIO, 'homeScreenIcon' ); - validateIconSize( options.navigationBarIcon, MINIMUM_NAVBAR_ICON_SIZE, NAVBAR_ICON_ASPECT_RATIO, 'navigationBarIcon' ); + validateIconSize( options.homeScreenIcon!, MINIMUM_HOME_SCREEN_ICON_SIZE, HOME_SCREEN_ICON_ASPECT_RATIO, 'homeScreenIcon' ); + validateIconSize( options.navigationBarIcon!, MINIMUM_NAVBAR_ICON_SIZE, NAVBAR_ICON_ASPECT_RATIO, 'navigationBarIcon' ); if ( assert && this.isPhetioInstrumented() ) { assert && assert( _.endsWith( options.tandem.phetioID, Tandem.SCREEN_TANDEM_NAME_SUFFIX ), 'Screen tandems should end with Screen suffix' ); ```

[pixelzoom]

dispose was added in the above commit. Closing.