Should Dialog API be unified with Node API?

[samreid]

This came from discussion in #369. The typical client-side API for Node is:

const myNode = new MyNode(); // node is visible but not in the scene graph
myParent.add(myNode); // node is added to the scene graph and can now be used

The way we typically use Dialog is like so:

let myDialog = null;
button.addListener(()=>{
   if (myDialog===null){
      myDialog = new MyDialog(); // myDialog is visible but not "isShowing" and not added to the scene graph
   } 
   myDialog.show(); //sets isShowing to true and adds to the scene graph
});    

There are a few problems with the latter API.

• The Dialog is not available to PhET-iO on simulation startup, so it cannot be customized or interacted with or shown. Something would need to trigger its creation before it could be interoperable.
• The Dialog has visibleProperty which is always true and an additional isShowingProperty which is true if the node has been added to the scene graph. I'm sure our clients could learn to understand this and deal with it, but it seems confusing.
• General asymmetry with other Node APIs. Dialog extends Node, but it has a pretty different way of being displayed.

Here is one proposed API that overcomes some of the problems (and introduces new problems):

const myDialog = new MyDialog({visible:false}); // dialog is not visible and not part of the scene graph
phet.joist.sim.dialogPane.addChild(myDialog);  // dialog can be seen and interacted with.
button.addListener(()=>myDialog.setVisible(true));    
// close button calls setVisible(false)

My first concern is performance. Is it a problem to always have the MyDialog in memory and in a layer (though invisible)? @jonathanolson would be best suited to answer that question since it requires an understanding of scenery implementation detail. If it is problematic to always have the MyDialog in the scene graph, we could try something like this:

const myDialog = new MyDialog(); // dialog is visible but not part of the scene graph
button.addListener(()=>phet.joist.sim.dialogPane.addChild(myDialog));    
// close button calls detach()

Also note that Dialog show/hide currently has a11y specific logic that we would need to move if we choose one of these other paradigms.

Requesting initial comments from @jonathanolson @pixelzoom and @zepumph.

[pixelzoom]

The way we typically use Dialog is like so: ...

That's only because Dialog had (has?) a dispose problem. So Dialogs were (are?) reused as a way of avoiding that problem.

The Dialog is not available to PhET-iO on simulation startup ...

This could be true of any UI component. A solution should not depend on the existence of a Dialog at startup. Dialogs should be created when it makes the most sense to create them, which is usually on demand.

The Dialog has visibleProperty which is always true and an additional isShowingProperty ...

I see no reason to have 2 Properties; visibleProperty should be sufficient, isShowingProperty is unnecessary. This is no different than any other Node. You simply shouldn't be setting/getting visibleProperty if a Node hasn't been added to the scene graph.

General asymmetry with other Node APIs. Dialog extends Node, but it has a pretty different way of being displayed. ...

What does "it has a pretty different way of being displayed" mean? I agree that Dialog will (eventually) have a different way of being added to the scene graph, since it will be associated with something like Java's glass pane. But I don't understand why the API to show/hide a Dialog is not simply setVisible.

Here is one proposed API that overcomes some of the problems (and introduces new problems):

const myDialog = new MyDialog({visible:false});

The default state of a Dialog should be visible: false, clients should not have to set this.

phet.joist.sim.dialogPane.addChild(myDialog); // dialog can be seen and interacted with.

Dialog cannot be seen until dialog.visible = true, which happens in the next statement in the example.

[samreid]

What does "it has a pretty different way of being displayed" mean?

The API and implementation are different than for typical scenery Nodes.

// normal way to creating and showing a scenery Node
const myNode = new MyNode();
myParent.addChild(myNode);

// API for creating and showing a dialog
const myDialog = new MyDialog();
myDialog.show();

[pixelzoom]

Where are you getting this API? It doesn't match current reality, nor does it match anything that I recall discussing. The only difference in APIs is that Dialogs are (or should be) invisible by default, and needs to be made visible to "show" the dialog. So:

// normal way to creating and showing a scenery Node
const myNode = new MyNode();
myParent.addChild(myNode);

// API for creating and showing a dialog
const myDialog = new MyDialog();  // defaults to invisible
dialogParent.addChild( myDialog );
// ... and when it's time to show the dialog:
myDialog.visible = true;

[samreid]

Where are you getting this API? It doesn't match current reality, nor does it match anything that I recall discussing.

There are 24 matches of dialog.show() across our repos. For example, here is one in Equality Explorer:

dialog = dialog || new OopsDialog( numberTooBigString );
dialog.show();

[pixelzoom]

Ah, I see... In the current implementation, window.phet.joist.sim.showPopup and hidePopup are handling add/remove from scenegraph.

So to answer the original question, which was "Should Dialog API be unified with Node API?"...

No, the APIs should not be unified, because Node and Dialog have different needs:

• Node is added to the scenegraph by the client. Dialog is added to the scenegraph automatically - currently the responsibility of window.phet.joist.sim, in the future the responsibility of something like Java's glass pane.
• Node is typically visible by default. Dialog is typically invisible by default. Dialog setVisible should be overridden to do what show and hide do now.

Typical uses would look like:

// Node
const myNode = new MyNode(); // visible:true by default
myParent.addChild(myNode); // explicitly added to the scenegraph
myNode = false; // explicitly made invisible
myNode = true; // explicitly made visible

// Dialog
const myDialog = new MyDialog(); // visible:false by default, implicitly added to the scenegraph
myDialog.visible = true; // explicitly made visible, "show"
myDialog.visible = false; // explicitly made invisible, "hide"

[samreid]

I understand that Dialog has different needs than a typical Node. But can you please clarify the flaws with this proposed API?

const myDialog = new MyDialog();
phet.joist.sim.glassPane.addChild(myDialog);

Is it mainly that phet.joist.sim.glassPane would get copied around?

[pixelzoom]

It's not clear what you're proposing in the previous comment. Is Dialog visible:true by default? Is phet.joist.sim.glassPane.addChild/removeChild the mechanism to show/hide the Dialog? If that's the case...

Client code should not be explicitly adding popups (dialogs, list boxes, menus,...) to a global like phet.joist.sim.glassPane. That should be encapsulated in the popups and in sun/joist.

[samreid]

Is Dialog visible:true by default?

Yes, as it is for Node. I'm trying to understand the disadvantages of unifying the Dialog API with the Node API.

Is phet.joist.sim.glassPane.addChild/removeChild the mechanism to show/hide the Dialog?

As proposed in #455 (comment) hide would be done via detach, which is essentially parent.removeChild(). Showing the node would be done with addChild as it is done for Node.

That should be encapsulated in the popups and in sun/joist.

We could encapsulate that in Dialog like so:

const myDialog = new MyDialog(); // dialog is visible but not part of the scenery graph
myDialog.addToGlassPane(); // dialog is added to phet.joist.sim.glassPane or equivalent

const myDialog = new MyDialog(); // visible:false by default, implicitly added to the scenegraph

I'm trying to understand whether an API that deviates from Node's defaults is advantageous here, and if so, why. Also, would we ever want to add a Dialog to a parent node other than the glass pane? What about during testing?

[pixelzoom]

would we ever want to add a Dialog to a parent node other than the glass pane

Hard to say without knowing more about PhET's "glass pane" feature. I doubt that there is a single glass pane instance. At a minimum, I think we'll need a glass pane per ScreenView (for screen-specific popups), and a global glass pane for the Sim (for PhET menu, About dialog, Options dialog, ...)

[samreid]

In that case, we may need a way for clients to specify the dialog parent. Possibly something like:

// Option A
new Dialog({parent: myScreen.glassPane}) // defaults to parent: phet.joist.sim.glassPane

// Option B
myScreen.glassPane.addChild(myDialog);

[pixelzoom]

What I've learned from this discussion is that deciding what the "best" API is for Dialog can't (and shouldn't) be done without an understanding of how Dialogs (and other popups) will ultimately be displayed.

[zepumph]

After reading through this issue, I don't see an obvious improvement in changing the API. My vote would be to do nothing.

[pixelzoom]

Ditto.

[samreid]

This seems like it warrants further deliberation once we resume development on dialogs, unassigning until then.