phetsims / rutherford-scattering

"Rutherford Scattering" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 3 forks source link

RSBaseScreenView controlPanel is immediately disposed #161

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

Noted while investigating #158.

RSBaseScreenView creates this.controlPanel, line 195:

    // @protected - collect all control panels in a single panel
    this.controlPanel = this.createControlPanel( controlPanels );
    this.addChild( this.controlPanel );

RutherfordAtomScreenView and PlumPuddingAtomScreenView are the only subclasses of RSBaseScreenView. One of the first things they do is dispose of this.controlPanel and create a new one. E.g. in PlumPuddingAtomScreenView constructor:

    RSBaseScreenView.call( this, model, scaleString, createSpaceNode, {
      includePlumPuddingLegend: true
    } );

    // dispose and remove the old control panel
    this.removeChild( this.controlPanel );
    this.controlPanel.dispose();
...
    this.controlPanel = this.createControlPanel( panels );
    this.addChild( this.controlPanel );
pixelzoom commented 5 years ago

This was added to RutherfordAtomConstructor for #156

    // {Node} control panel is created below by sceneProperty listener, to correspond to scene
    this.controlPanel = null;

That's a problem, because the control panel created by RSBaseScreenView doesn't get removed or disposed.

pixelzoom commented 5 years ago

@jessegreenberg I went ahead and fully addressed this. I removed this.controlPanel from RSBaseScreenView, and made the subclasses fully responsible for controlPanel.

I compared master to the published version, to verify that the control panels looked the same. And I tested with ?ea&fuzz.

Please review.

jessegreenberg commented 5 years ago

Awesome, looks great thanks. I also compared against published version and the layout looks the same.