Add isShowingProperty to dialog? #369
Comments
|
It should be easy enough to change I also see that AboutDialog.js and UpdateDialog.js use |
|
I'm not clear on whether @zepumph was proposing to add |
I was thinking of continuing to have it be a private instance, but to expose it as a read only Property to the PhET-iO api. @pixelzoom there are two ways to instrument a property (lower case) for PhET-iO. In this example we COULD add getter and listeners methods to DialogIO like
Again we don't NEED to convert it, but so far it has been very nice to only interface with Properties when needing the set/get/listener functionality of something. In terms of it being a general problem. I'm not sure I agree. I could be confusing for clients though if sometimes we use Property, and sometimes we use |
|
@zepumph said:
The doc for SpectrumWindowDialog |
|
I was thinking no. @samreid please correct me if I am wrong. But to me it seems like is you don't want to show the Dialog, then you hide the button that displayed the Dialog, rather than having a button that displays nothing. |
|
I don't follow the previous comment. The state of the control used to display the Dialog should have nothing to do with this. Taking into account that we are considering adding |
|
Btw... The fact that we're having to negotiate the visibility and responsibilities of this Property is the very reason why I don't agree that adding such Properties (that exist solely to provide messages to the PhET-iO data stream) is a good approach. |
|
Related to this discussion... SpectrumWindowDialog is now LightSpectrumDialog. |
I think in general, PhET-iO should have an API for controlling and observing whether dialogs are visible. For example, one wrapper may ask students a question about a phenomenon that the student has to answer without the dialog, then the wrapper pops open the dialog. Having a setVisible/getVisible/linkVisible behavior seems best handled by a Property. |
|
I think a design meeting about this is needed. I can forsee many problems with allowing PhET-iO to control visibility and change the control flow (as proposed by @samreid in #369 (comment)). |
zepumph
added
status:blocks-publication
and removed
meeting:design
phet-io:collaboration
labels
|
The phet-io team discussed this today. We think it is appropriate to create a Property in Dialog to expose whether or not the Dialog is "shown" to phet-io. For now we think that this Property should be read only. But we should recognize that potentially there will come a phet-io design in which there is now "Open Dialog" button that you can press instead of directly interfacing with the Dialog itself. On top of customization (showing/hiding the dialog programmatically), we also want to make sure that we recognize the limitation of the data stream in this way currently. Here is what it looks like to open the AboutDialog in Faradays Law right now: There is no event for the Dialog being displayed. Seeing this hole in the data stream, we want to add a read only property now, for the data stream. We will wait for a design meeting to discuss the interoperability with the shown property. @samreid will take care of that Marking as blocks publication for any sim that has a dialog other than the about dialog (like Molecules and Light). |
|
Rather than something odd/nonstandard like |
|
The preceding commit uses |
|
Attempt to remove |
|
I was able to find many (all?) of the usages of hide by searching the regex I was brought to this issue from #437, and because I saw that |
|
Yes, I've made intermediate progress on this issue, I'd like to remove show/hide but haven't got to it yet. |
|
I made progress in my working copy converting show/hide to setVisible(true|false), but was concerned that it may not be a good match. I reached out to @zepumph to discuss it, and we discussed that currently Dialog has an API that doesn't match typical Node APIs. For instance, most dialogs are currently created like so: callback: function() {
if ( !aboutDialog ) {
var phetButton = sim.navigationBar.phetButton;
aboutDialog = new AboutDialog( sim.name, sim.version, sim.credits, Brand, sim.locale, phetButton,
tandem.createTandem( 'aboutDialog' ) );
}
aboutDialog.show();
},That is, they are lazily created and require an extra I was wondering if we should eliminate show/hide and use setVisible(true|false) instead, and make the Maybe one way to proceed will be to go with a read-only isShowing Property for now, and wait until larger team discussions about where we want to go with Dialog (if anywhere) before doing more intensive work. |
|
Here is a patch of changes that move more occurrences of hide/show to setVisible(true|false): |
|
In the preceding commits, I added a PHET-iO read-only property that indicates whether a dialog is showing. I tested it with Molecules and Light and it behaves as expected. On the one hand, I'm somewhat interested in revising the Dialog API like so it matches typical Node API: const dialog = new Dialog(content); // already visible
phet.joist.sim.dialogLayer.addChild(dialog);
// to hide temporarily
dialog.setVisible(false);
// to remove
dialog.detach();On the other hand, the existing API exemplified in #369 (comment) may be in a "not broken, don't fix it" situation. But I'm concerned about PhET-iO and accessibility issues and thinking that unifying with standard Node API could make things clearer. But that would be a much more invasive change and would require the dev team to sign off. I'll create a separate issue for that. |
|
I opened #455 for further discussion with @zepumph @jonathanolson and @pixelzoom. @zepumph and @pixelzoom do you think this issue can be closed? |
|
I haven't been following this issue very closely, and haven't reviewed the solution. So as I mentioned in #369 (comment), I'm going to defer to others on Dialog. |
|
This looks like just what I was thinking, nice work!. I think we are good to close, carrying on in #455. |
|
Potential memory leak for |
related to phetsims/molecules-and-light#185,
In SpectrumWindowDialog.js [CM: now LightSpectrumDialog.js] in MAL, there is this block:
For phet-io especially, but probably elsewhere this may be nice to have in Dialog. I see a boolean property for
isShowing, but maybe it could be promoted to a Property?@pixelzoom @andrealin this is not high priority all, and I don't know the current status of Dialog work, but I know you two have been in there heavily recently, is this something we could do while working in that file? It will make life better for phet-io in the future, even though there isn't active work being done in MAL.
The text was updated successfully, but these errors were encountered: