Dialog: decouple show/hide/dispose

[zepumph]

In #422 @pixelzoom and I talked about Dialog's hide function. Basically it acts as a dispose, but it is confusing and not well documented. There currently isn't a dispose function. I think at the very least, the hide function should call dispose, to separate functionality and make it more clear. Tagging for dev meeting to see the best way to change this.

Tagging #359 as the parent issue for dialog improvement.

[pixelzoom]

@zepumph said:

I think at the very least, the hide function should call dispose, to separate functionality and make it more clear.

Imo, hide should not call dispose. That prevents the client from repeatedly hiding/showing the same Dialog instance.

[zepumph]

I agree, separating them is preferable and better.

[jonathanolson]

Hiding not disposing sounds required if we're re-using dialogs. I think at one point in the the thought was dialogs would never be reused.

[jessegreenberg]

I think at one point in the the thought was dialogs would never be reused.

hide has a comment

// dispose dialog - a new one will be created on show()

show is also adding listeners that are being disposed in hide. I agree that they should be decoupled and I can work on this. @zepumph can I address with #359 or is this causing high priority issues?

[zepumph]

I just saw #359. That seems like a great place to address this, it's not too high priority. I did go through the project and make all dialogs that are being re used, created new each time. See phetsims/curve-fitting@3ba3419 and phetsims/make-a-ten@2a2d4d1 for examples.

These will need potentially need to be adapted to whatever use case we decide on.

[jessegreenberg]

@jessegreenberg will work on this as part of #359.

[zepumph]

@jessegreenberg while working on https://github.com/phetsims/phet-io/issues/1143, @samreid and I noticed that dispose was being called on the Dialog before the phet-io event could 'end'. We added this
line setTimeout( function() { self.hide(); }, 0 ); into Dialog's closebutton listener to hack around this issue temporarily. Once dispose and hide have been separated, the setTimeout can be removed.

[pixelzoom]

@zepumph said:

We added this line setTimeout( function() { self.hide(); }, 0 ); into Dialog's closebutton listener to hack around this issue temporarily. Once dispose and hide have been separated, the setTimeout can be removed.

Please add documentation to the code indicating why this was added, when it can be removed, and reference this issue.

[zepumph]

Good point, thanks

[pixelzoom]

Dialog.hide currently calls Dialog.diposeDialog, rather than Dialog.dipose. This means that it's impossible to properly cleanup subtypes for Dialog. For example:

[7/10/17, 1:46:23 PM] Chris Malley: My immediate problem is in MOTHA. I have a Dialog subtype, SnapshotDialog. When I call hide, it calls Dialog.disposeDialog, instead of Dialog.dispose. SnapshotDialog has stuff that it needs to cleanup, and there is no way that I can call it's dispose function. I was previously overriding Dialog.hide and calling ShapshotDialog.dispose, but that now fails due to recent scenery changes that prevent dispose from being called twice.
[7/10/17, 1:46:45 PM] Chris Malley: So I have a memory leak that I can't plug.

[jessegreenberg]

This issue in particular is causing lots of immediate problems for Dialogs. Instead of waiting for a new implementation in #359, we should look into this problem now.

[pixelzoom]

Here's pseudo code that shows the responsibilities of these functions.

show: function() {
  if ( !this.visible ) {
    // do whatever is required to make the Dialog visible
  }
},

hide: function() {
  if ( this.visible ) {
    // do whatever is required to make the Dialog invisible
  }
},

dispose: function() {
  this.hide();
  this.disposeDialog();
  Panel.prototype.dispose.call( this );
}

[pixelzoom]

If a client wants to reuse a Dialog, then the client is responsible for alternately calling show and hide.

When a client is done with a Dialog, it's the client's responsibility to call dispose for the Dialog.

The client is also responsible for disposing of the Dialog's content.

[jessegreenberg]

Thanks @pixelzoom, that looks great. Since the client is going to be responsible for disposing Dialogs we will have to go through and make sure that every show has a corresponding dispose or hide, including Dialogs that are created from the PhetMenu.

[pixelzoom]

@jessegreenberg @zepumph thanks for tackling this. If you need someone to review, I'll be happy to.

[jessegreenberg]

Thanks very much @pixelzoom, we do need a reviewer. Assigning to you.

[jessegreenberg]

Prior to this change, the common way to create a Dialog was

var button = new Button( {
  listener: function() {
    new Dialog().show();
  }
} );

Since the Dialog can hide itself, creating a new one every time will create a memory leak since hide no longer disposes.

We replaced this with

// Dialog's implementation requires sim bounds during construction so needs to be constructed
// lazily.
var dialog = null;
var button = new Button( {
  listener: function() {
    if ( !dialog ) {
      dialog = new Dialog();
    }
    dialog.show();
  }
} );

so Dialogs are usually reused, but Client can dispose a Dialog if necessary.

[jessegreenberg]

In #359 @zepumph said

In Dialog I still see

listener: function() {

          // This setTimeout call is a workaround until hide and dispose are separated. It should be removed once
          // https://github.com/phetsims/joist/issues/424 is complete.
          setTimeout( function() { self.hide(); }, 0 );
        },
        accessibleFire: function() {

          // the active element must be focused after the Dialog is hidden, so this must also be wrapped in a
          // timeout. This should also be removed once https://github.com/phetsims/joist/issues/424 is complete.
          setTimeout( function() { self.focusActiveElement(); }, 0 );
        },

Do you think these setTimeout calls are ready to be removed?

I am not sure why this was necessary and I don't know how to test if they can be removed. @zepumph can you please look into if they can be removed?

[jessegreenberg]

@zepumph said:

If I remember correctly, hide calling dispose would happen before the closeButton's 'fire' event could "finish." In terms of PhET-iO, this meant that the 'end' emitter for the fire event would never be called, because a child of the fire event, would destroy the component itself. I'm not sure about the accessibility listener.

And recommended that we test to make sure the Dialogs are OK in the mirror-inputs wrapper after removing the setTimeouts.

[zepumph]

I removed these two set timeouts, and tested by using keyboard navigation on BASE to see that on close, the focus highlight updated back to the original nav bar button (for phetmenu and the keyboard nav button).

Next I used phet-io mirror inputs wrapper to see that opening an about dialog, and then closing it with the x button were both repeated correctly in the downstream sims.

PhET-iO and phet brand fuzz tests are passing as well as the phet-io iframe-api tests and grunt phet build.

Back to you @jessegreenberg and @pixelzoom for review.

[jessegreenberg]

Great, thanks for removing and testing @zepumph.

[pixelzoom]

Changes in Dialog look great. Pattern for reusing dialogs looks reasonable, about what I expected. The only thing that I didn't fully review were the changes to joist for Dialog subtypes (About, Update, Options,...) I skimmed those changes, didn't see anything obviously wrong, and trust that all is well since nothing is failing in automated testing.

So... Closing!

[samreid]

There are still TODOs marked for this issue, discovered during phetsims/chipper#946

[pixelzoom]

The report says "TODOs" plural, but I only found 1 TODO, in MOTHA, deleted in the above commit. phetsims/chipper#946 doesn't provide any clear instructions on how to test this, so closing.