phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Instrumentation bug for NumberDisplay `valueText.valueStringProperty` #821

Closed pixelzoom closed 11 months ago

pixelzoom commented 11 months ago

Discovered while instrumenting My Solar System for https://github.com/phetsims/my-solar-system/issues/237.

NumberDisplay provides the ability to opt-out of instrumenting its valueText (Text/RichText) subcomponent, a request that has been made numerous times by PhET-iO designers. That is accomplished like this:

const numberDisplay = new NumberDisplay( ...,  {
  textOptions: {
    tandem: Tandem.OPT_OUT
  },
  tandem: options.tandem.createTandem( 'numberDisplay' )
} );

The bug is that valueText has child valueStringProperty, and it is instrumented as valueText.valueStringProperty regardless of the value of options.textOptions.tandem. Here's the relevant code that contains the bug:

    // value
    const Constructor = options.useRichText ? RichText : Text;
207 const valueTextTandem = options.tandem?.createTandem( 'valueText' );
    const valueStringProperty = new DerivedStringProperty( ..., {
      tandem: valueTextTandem?.createTandem( Text.STRING_PROPERTY_TANDEM_NAME )
    } );

    const valueTextOptions = combineOptions<TextOptions | RichTextOptions>( {}, options.textOptions, {
      maxWidth: null // we are handling maxWidth manually, so we don't want to provide it initially.
    } );

    const valueText: Text | RichText = new Constructor( valueStringProperty, combineOptions<TextOptions | RichTextOptions>( {
      tandem: valueTextTandem
    }, valueTextOptions ) );

The bug is on line 207, noted above. The assignment to valueTextTandem is not considering that the default of "valueText" may have been overridden via options.textOptions.tandem.

Fixing this will undoubtedly result in a PhET-iO API change for one or more sims, and may also require migration rules.

@zepumph @samreid @arouinfar FYI.

pixelzoom commented 11 months ago

I discussed how to proceed with @zepumph and @samreid.

  1. I'll fix the bug in NumberDisplay.
  2. I'll generate all API files with grunt generate-phet-io-api --stable, then review and commit any that have changed.
  3. @zepumph and @samreid will handle changes related to migration rules.
  4. I'll review the changes for migration rules.
pixelzoom commented 11 months ago

The fix is in https://github.com/phetsims/scenery-phet/commit/63d08531bcc98229cc727306cc925be88a594837.

Surprisingly, grunt generate-phet-io-api --stable says that no API files are changed. So that means no migration rules.

@zepumph @samreid please check my work, then feel free to close.

samreid commented 11 months ago

I reviewed the commits and they look correct to me, thanks. Closing.