Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AboutDialog should be created during startup #558

Closed
samreid opened this issue Apr 24, 2019 · 16 comments
Closed

AboutDialog should be created during startup #558

samreid opened this issue Apr 24, 2019 · 16 comments

Comments

@samreid
Copy link
Member

samreid commented Apr 24, 2019

From https://github.com/phetsims/phet-io/issues/1454, we would like to simplify the PhET-iO API by creating the about dialog during startup. But we would also like to postpone network activity until the dialog is shown, if that is not too much trouble.

Mentioning @jonathanolson because this overlaps with #557

Also, we should check that this doesn't put too much additional burden on the heap or startup time.

@pixelzoom
Copy link
Contributor

This is a slippery slope. Dialogs should generally be created on demand, and eagerly creating the About dialog doesn't address the problem generally. Are we also going to create the (optional) Options dialog? What are the alternatives?

@pixelzoom
Copy link
Contributor

I'm confused. Reading https://github.com/phetsims/phet-io/issues/1454, it sounds like there's an intention to "support instrumented element that is dynamic/lazily created". Why is that not applicable to the About dialog?

@samreid
Copy link
Member Author

samreid commented Apr 25, 2019

Dialogs should generally be created on demand

Why? Do they incur too much CPU time during startup? Too much memory usage on the heap? Better programming practice? Other runtime considerations?

it sounds like there's an intention to "support instrumented element that is dynamic/lazily created"

The strategy we used for Group is to provide an archetypal element that completely describes the PhET-iO structure/api (subtandems and all) for members of the group. If we apply that pattern to dynamic/lazily created elements, then we might end up with something like this: create a "synthetic" About Dialog to signify the API structure, then when the button is pressed, create the "real" About Dialog. That plan creates 2x too many About Dialogs! I foresee the following possibilities:

a) Create the dialogs and other things on startup
b) Somehow capture the static API from the baseline file and use that as the "archetype"

(b) sounds complicated and brittle, so I wanted to investigate (a) more to see if it is feasible.

In simulations, we often create dialogs lazily, but do not apply this practice to other parts of the sim. For instance in Wave Interference Screen 1, the simulation starts by showing the Water Scene, but the Sound and Light scenes are eagerly (not lazily) created. That is good because they become immediately interoperable for PhET-iO. I don't know how we've arrived at the policy of "create everything in the sim eagerly except for dialogs", but I'm inclined to investigate that as a pattern, including investigating the impact on heap and startup time.

@pixelzoom
Copy link
Contributor

I don't know how we've arrived at the policy of "create everything in the sim eagerly except for dialogs", but I'm inclined to investigate that as a pattern, including investigating the impact on heap and startup time.

We are not creating everything eagerly, we are creating the initial state of all screens eagerly.

We create screens eagerly because the time delay to create and switch to a new screen was determine to be (in general) unacceptable. It's assumed that students will visit each screen, and we decided that it was a preferable UX to have the cost to create all of the screen incurred on startup.

Requiring everything to be created eagerly is imo an unacceptable limitation. It makes no sense to eagerly create things that may never need to be created. About dialog certainly falls into that category. The only reason we're having this discussion is that we've waited until far too late in this process to fully consider how PhET-iO works with dynamic creation.

@samreid
Copy link
Member Author

samreid commented Apr 25, 2019

Some ideas on the spectrum from easy to difficult:

  1. Dynamic things are uninstrumented. They are not customizable and we don't get data stream for them
  2. Dynamic things are converted to static, created eagerly.
  3. Dynamic things are created eagerly during baseline creation, but not during the sim runtime. This is part of the Group or PhetioObjectPlaceholder paradigm.

For the latter: if we reuse the same code for creating the "prototypes" as the real instances, it could look something like:

Index: js/PhetMenu.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetMenu.js	(revision ac1aeb89f6688f3ac8c3ca443b8e277487d22edf)
+++ js/PhetMenu.js	(date 1556206001000)
@@ -121,6 +121,13 @@
     var optionsDialog = null;
     var updateDialog = null;
 
+    const createAboutDialog = () => {
+      aboutDialog = new AboutDialog( sim.name, sim.version, sim.credits, Brand, sim.locale, phetButton,
+        tandem.createTandem( 'aboutDialog' ) )
+    };
+
+    printBaselineFile && createAboutDialog();
+
     /*
      * Description of the items in the menu. See Menu Item for a list of properties for each itemDescriptor
      */
@@ -309,8 +316,7 @@
         separatorBefore: isPhETBrand,
         callback: function() {
           if ( !aboutDialog ) {
-            aboutDialog = new AboutDialog( sim.name, sim.version, sim.credits, Brand, sim.locale, phetButton,
-              tandem.createTandem( 'aboutDialog' ) );
+            createAboutDialog();
           }
           aboutDialog.show();
         },
  1. We could use a high-level adapter object like so:
    // Dialogs that could be constructed by the menu. The menu will create a dialog the
    // first time the item is selected, and they will be reused after that.  Must
    // be created lazily because Dialog requires Sim to have bounds during construction
    var aboutDialog = new AboutDialogScaffolding();// is eagerly created and has the API we want
    var optionsDialog = null;
    var updateDialog = null;

    class AboutDialogScaffolding(){

      create(){
        if (this.dialog===null){
          this.dialog = new AboutDialog(this.logo||null)/// no tandem

          //or
          this.dialog.setLogo(this.logo);
        }
      }

      setLogo(logo){
        if (this.dialog){
          this.dialog.setLogo(logo)
        }else{
          this.logo = logo;
        }
      }
    }

    AboutDialogScaffoldingIO{
      setLogo(logo){}
      addListener(listener){}
    }

    // client
    invoke('setLogo',myLogo) // eager

    //..

This has the advantage that it keeps a lightweight item always in the API, but doesn't do the heavy lifting until necessary. However, because this involves uninstrumenting the instance (including 100% of its subtree), we lose all interoperability for the nested substructure. Including data stream, focusability, API calls, etc.

@samreid
Copy link
Member Author

samreid commented May 22, 2019

We measured the size of the about dialog to be about 1MB, so we would like to create it lazily. Not sure what approach to use, it will depend somewhat how we want to see it in Studio, which is a question for PhET-iO design meeting. Do we want it to show in studio as "grayed out" where you can see the API but not interact with it until created? Is it OK to have 2 different ways of having non-existent things (groups + lazies)?

Also, how will we combine these, like a group with lazy items. Or a lazy item structured under another lazy item?

@chrisklus says maybe the about dialog could be streamlined to not take up 1MB.

@samreid
Copy link
Member Author

samreid commented May 23, 2019

@kathy-phet suggests: let's not instrument the About Dialog for now. We can take time to make sure we design Group and/or Lazy things properly.

@zepumph
Copy link
Member

zepumph commented May 23, 2019

Note that this will currently break phet-io/a11y together, see Display.focus setter:

    set focus( value ) {

      // If in phet-io brand, a11y is enabled, and the focus is not null
      if ( window.phet && phet.phetio && phet.chipper.accessibility && value ) {
        var node = value.trail.lastNode();
        assert && assert( node.isPhetioInstrumented(),
          'When running phet-io mode, all focusable instances must be instrumented.' );
      }

zepumph added a commit to phetsims/balloons-and-static-electricity that referenced this issue May 23, 2019
zepumph added a commit to phetsims/gravity-force-lab-basics that referenced this issue May 23, 2019
zepumph added a commit to phetsims/forces-and-motion-basics that referenced this issue May 23, 2019
zepumph added a commit to phetsims/resistance-in-a-wire that referenced this issue May 23, 2019
zepumph added a commit to phetsims/capacitor-lab-basics that referenced this issue May 23, 2019
zepumph added a commit to phetsims/graphing-quadratics that referenced this issue May 23, 2019
zepumph added a commit to phetsims/molecules-and-light that referenced this issue May 23, 2019
zepumph added a commit to phetsims/arithmetic that referenced this issue May 23, 2019
zepumph added a commit to phetsims/hookes-law that referenced this issue May 23, 2019
zepumph added a commit to phetsims/friction that referenced this issue May 23, 2019
zepumph added a commit to phetsims/ohms-law that referenced this issue May 23, 2019
zepumph added a commit to phetsims/molarity that referenced this issue May 23, 2019
zepumph added a commit to phetsims/chains that referenced this issue May 23, 2019
zepumph added a commit to phetsims/bumper that referenced this issue May 23, 2019
@zepumph
Copy link
Member

zepumph commented May 23, 2019

In the above commits I made the following changes:

@samreid please review.

@samreid
Copy link
Member Author

samreid commented May 25, 2019

Are values for phetioIDIndices ever read or used by anything? Or is that for data stream output only?

@zepumph
Copy link
Member

zepumph commented May 25, 2019

Just for data stream output. Focus changes are downstream of Keyboard related input actions, so a11y input record playback works largely the same way as traditional input.

@samreid
Copy link
Member Author

samreid commented May 25, 2019

TARGET_SUBSTITUTE_KEY is difficult for me to understand. For example:

// Special case for target since we can't set that read-only property. Instead use a substitute key.
if ( key === 'target' ) {
  domEvent[ TARGET_SUBSTITUTE_KEY ] = eventObject[ key ];
}

Why is 'target' the appropriate substitute? What does it mean? When/why is this code (i.e. deserializeDomEvent) called?

Other changes look good, I'm just trying to understand the changes in Input.js better.

@samreid samreid removed their assignment May 25, 2019
@zepumph
Copy link
Member

zepumph commented May 30, 2019

event.target is a read-only field that the browser set's on an event. It is very important for a11y input that we know what HTML element received the event. This is because in the PDOM to go from HTMLElement -> Node, we keep the TrailID (string) as a data attribute on the HTMLElement. When serializing the event, we make sure to keep track of the trailID from the target:
image

When deserializing (needed for phet-io playback), we are not able to set that back to the target, so we use a substitute key that we can set to:

image

Please feel free to reach out and we can talk more if this still doesn't make sense.

@zepumph
Copy link
Member

zepumph commented Jun 8, 2019

Removing from blocks publication.

@samreid
Copy link
Member Author

samreid commented Jun 24, 2019

Remarking as blocks publication because this seems it should be done for GQIO11

@samreid
Copy link
Member Author

samreid commented Jun 24, 2019

@chrisklus and I reviewed the changes and have a better understanding of TARGET_SUBSTITUTE_KEY. We think this issue can be closed.

@samreid samreid closed this as completed Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants