phetsims / molecules-and-light

"Molecules and Light" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 5 forks source link

unbelievably bad PhET-iO support in SpectrumWindowDialog #185

Open pixelzoom opened 6 years ago

pixelzoom commented 6 years ago

Noticed while working on #184 in SpectrumWindowDialog.

There's some really awful stuff in SpectrumWindowDialog, "done primarily for PhET-iO support". I can't tell who is responsible based on GitHub commits.

Here's the relevant part of the implementation:

function SpectrumWindowDialog( content, spectrumWindowButton, tandem ) {
...
    // Create a property that both signals changes to the 'shown' state and can also be used to show/hide the dialog
    // remotely.  This is done primarily for PhET-iO support.  TODO: Move into the Dialog type?
    this.shownProperty = new Property( false, {
      tandem: tandem.createTandem( 'shownProperty' ),
      phetioType: PropertyIO( BooleanIO )
    } );

    var shownListener = function( shown ) {
      if ( shown ) {
        Dialog.prototype.show.call( self );
      }
      else {
        // hide the dialog
        Dialog.prototype.hide.call( self );
      }
    };
    this.shownProperty.lazyLink( shownListener )
...
}

return inherit( Dialog, SpectrumWindowDialog, {
    hide: function() {
      this.shownProperty.value = false;
    },
    show: function() {
      this.shownProperty.value = true;
    },
    ...
} 

So many things wrong with this, but the 2 big things are:

(1) There's something being done here for PhET-iO support that is not at all specific to this dialog, and belongs in Dialog. Is it really necessary? If it's not necessary, delete it. If it's necessary, move it to Dialog so that other dialogs benefit.

(2) Overriding show and hide to do something different, then calling the supertype's show and hide in a Property listener has an anti-pattern code smell. If this is moved to Dialog, let's implement a real solution.

(CM edited.)

jessegreenberg commented 6 years ago

This TODO is at least a year old and the feature hasn't been added to Dialog in that time, maybe it is no longer required.

I am not very familiar so removing my assignment, let me know if I can help.

samreid commented 6 years ago

I'm not planning to work on this until we revisit Molecules and Light, unless it is blocking work for Dialog. @pixelzoom please reassign me if this is blocking work on Dialog.

samreid commented 5 years ago

Same problem in gene-expression-essentials/js/multiple-cells/view/FluorescentCellsPictureDialog.js