Replace ?showHomeScreen=false with ?bypassHomeScreen

[samreid]

In https://github.com/phetsims/joist/issues/445 we are building out more joist functionality regarding the homescreen, and will be adding a new query parameter ?homeScreen=false which creates a sim with no home screen and no homescreen icon. There is an existing query parameter showHomeScreen which seems like it would be easy to confuse with the new one. We recommend replacing ?showHomeScreen=false with ?bypassHomeScreen for clarity. We think this is rarely used outside of PhET (probably rarely used within PhET as well), but wanted to check with other developers before making this change.

@jbphet @pixelzoom @jonathanolson @jessegreenberg @Denz1994 @mbarlow12 ok to proceed? If you are the last one to chime in, please reassign to me or @zepumph.

[pixelzoom]

Lots of other questions related to "cloning" screens (regardless of how it's done): If the same screen appears multiple times, how do you propose to avoid tandem id collisions? Will multiple copies of the same screen need to be configured in different ways? What other problems might this cause? Do we really need to do this?...

[pixelzoom]

My input on https://github.com/phetsims/joist/issues/451#issuecomment-340930916... More complexity, more things to break, more difficult to develop sims. I'm not digging this at all.

[samreid]

If the same screen appears multiple times, how do you propose to avoid tandem id collisions?

A unique tandem can be given to each clone.

Will multiple copies of the same screen need to be configured in different ways?

Yes, cloning a screen without changing anything seems to provide no advantage. The clones can be customized with PhET-iO API calls.

What other problems might this cause?

It will transform a relatively static API into a dynamic API, which is unconventional and "tricky".

Do we really need to do this?...

That's a question for @kathy-phet but I think it won't take too long to create a rough draft. (The main costs will be in maintenance and porting sims to use screen factories instead of instances).

[pixelzoom]

@samreid said:

The main costs will be in maintenance and porting sims to use screen factories instead of instances

So yet another cost for PhET-iO, converting something relatively simple to something complex, "unconventional and tricky". In https://github.com/phetsims/phet-io/issues/1137#issuecomment-307514863 I identified one of the big management questions as:

• [ ] How much of an increase in sim cost is acceptable? How much is expected?

@ariel-phet @kathy-phet Has anyone asked this question or answered it? Imo the incremental cost of PhET-iO is very high, perhaps dangerously high.

[samreid]

I'm going to commit a proposed strategy for implementing this, we can use it for more concrete discussion and delete it if we wish. Please see https://github.com/phetsims/phet-io/issues/1247 for details.

[pixelzoom]

Since you used BLL as an example... I'd appreciate it if you kept this out of BLL master.

[samreid]

Given the other commits in https://github.com/phetsims/phet-io/issues/1247 here's the BLL main that will make it work:

  var screenFactories = [
    function( tandemName ) {
      return new ConcentrationScreen( tandem.createTandem( tandemName || 'concentrationScreen' ) );
    }, function( tandemName ) {
      return new BeersLawScreen( tandem.createTandem( tandemName || 'beersLawScreen' ) );
    }
  ];

  SimLauncher.launch( function() {
    var screens = Sim.createScreens( screenFactories );
    var sim = new Sim( beersLawLabTitleString, screens, simOptions );
    sim.start();
  } );

You can test it by launching http://localhost/phet-io-wrappers/create-screens/create-screens.html?sim=beers-law-lab

[samreid]

@pixelzoom requested that these changes remain out of bll/master for now. We should review the proposed changes and decide if we are going to support this feature and if this is the way we want to proceed.

[samreid]

My main reason for pursuing https://github.com/phetsims/phet-io/issues/1247 now was because it had the potential to impact how we proceed with the proposal in https://github.com/phetsims/joist/issues/451#issuecomment-340848683. I chose a preprocessing strategy and https://github.com/phetsims/joist/issues/451#issuecomment-340848683 will still work properly on the cloned screens.

I'll investigate https://github.com/phetsims/joist/issues/451#issuecomment-340848683 next.

[zepumph]

In https://github.com/phetsims/phet-io/issues/1247#issuecomment-341171937 @kathy-phet said we will not support screen replication, too many red flags pointed out by @pixelzoom in https://github.com/phetsims/phet-io/issues/1247#issuecomment-341138950.

[pixelzoom]

11/2/2017 dev meeting: As proposed in https://github.com/phetsims/joist/issues/451#issuecomment-340873770, we will make these query parameters error out for single-screen sims.

[samreid]

I added the homeScreen query parameter, and it throws an assertion error when used on a single screen sim.

[samreid]

I added the initialScreen query parameter, here are some tests to check the behavior:

Note that the screens for FAMB are: net force, motion, friction, acceleration http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&initialScreen=4&screens=2,4&homeScreen=true => home screen + motion | acceleration

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&initialScreen=4&screens=2,4&homeScreen=false => no home screen + motion | acceleration

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&initialScreen=3 => home screen available + net force | motion | friction | acceleration

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&initialScreen=3&homeScreen=false&screens=1,2 => error

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&initialScreen=3&homeScreen=false&screens=1,1 => error

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&homeScreen=false&screens=1,4&initialScreen=0 => we must allow initialScreen=0 when homeScreen=false (since initialScreen defaults to 0), so leave this as no error and just start on lowest initial screen.

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&homeScreen=false&screens=1,4&initialScreen=4 => home screen is available but starts on acceleration screen

It would be nice if we could automatically test these behaviors, but I don't see a simple way to get there. @phet-steele could you run the preceding tests and recommend some other combinations of query parameters that should also be tested?

After we confirm the behavior is as-desired and have a set of tests to run (even if manual), then we can reach out to @pixelzoom for review.

[pixelzoom]

In Slack, @phet-steele said:

While I’m testing https://github.com/phetsims/joist/issues/451, are we expecting that ?showHomeScreen and ?screenIndex will NOT do anything? Looks like there was talk about deleting those two, but I still see them in initialize-globals.js

I confirmed that showHomeScreen and screenIndex are still in initialize-globals.js. Searching for "queryParameters.showHomeScreen" and "queryParameters.screenIndex", I see no occurrences, so those query parameters are apparently no longer used. They should be deleted from initialize-globals.js.

[samreid]

Thanks, I'll remove them now.

[samreid]

It looks like navbar items aren't centered when the home button is hidden:

[samreid]

I removed screenIndex and showHomeScreen and confirmed that http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&initialScreen=4&screens=2,4&homeScreen=false still works as expected.

[pixelzoom]

@samreid said:

It looks like navbar items aren't centered when the home button is hidden:

Is that a new problem, or did it exist before the changes made for this issue?

[samreid]

With the home button, things are (more or lesss?) centered:

This is the first time we are experimenting with removing the home button. I think it was removed with setVisible(false) rather than a strategy that updates the layout.

[phet-steele]

Is that are new problem, or did it exist before the changes made for this issue?

Looks like it happened when we moved the home screen button (so probably unrelated to this issue). The home screen button maybe used to not be considered when centering screen icons?

Production:

2.3.0-rc.3:

[samreid]

Thanks, I'll create a new issue for centering things and I'll note that centering should still work with or without the home button.

[phet-steele]

@samreid these are working great. You have a pretty comprehensive list of tests in https://github.com/phetsims/joist/issues/451#issuecomment-341620806.

One question, did you intend for your last link to have homeScreen=true? Otherwise, this link certainly does not have the home screen available.

http://localhost/forces-and-motion-basics/forces-and-motion-basics_en.html?brand=phet&ea&homeScreen=false&screens=1,4&initialScreen=4 => home screen is available but starts on acceleration screen

After we confirm the behavior is as-desired and have a set of tests to run (even if manual), then we can reach out to @pixelzoom for review.

Looks like I should hand this off to @pixelzoom.

[zepumph]

Reminder to myself: When this is done, it should be updated in https://github.com/phetsims/joist/issues/445#issuecomment-341248255

[samreid]

One question, did you intend for your last link to have homeScreen=true?

I think I meant to say "home screen is not available", thanks @phet-steele

[pixelzoom]

In https://github.com/phetsims/query-string-machine/issues/30#issuecomment-341507927, we agreed to note query parameters that are intended for external use. That has not been done for initialScreen and homeScreen. I'll take care of it now.

[zepumph]

I'm not sure if we want to go down this road, but it may be nice to have a jsDoc @ block annotation that defines this. There is an @external but it has little different use. If we go down this road could we adapt it?

[pixelzoom]

With ?screens=1,3&initialScreen=2 the assertion failure has this message:

Assertion failed: screen not found

The message should identify the screen index. I'll take care of this.

[pixelzoom]

This combination seems odd: ?homeScreen=false&screens=1,3&initialScreen=0. We've explicitly asked for the home screen to be the initial screen, but there is no home screen. This succeeds because initialScreen is ignored if its value is 0 (see Sim.js line 127), which seems a bit brittle. And the default value of initialScreen is 0, so we can't detect that it was explicitly set to '0'. I'm OK with this, but wanted to point it out in case anyone else thinks it should be handled.

[samreid]

I added an assertion error that fails for the combination of homeScreen=false and initialScreen=0, @pixelzoom can you please review?

[pixelzoom]

I added an assertion error ...

Hmmm... That reminds me. Assertions are not appropriate for checking any of these query parameters. They should throw Error regardless of whether assertions are enabled.

In general... assert is fine for checking query parameters that are for internal use. But for query parameters available for external, use throw Error.

[samreid]

I asked on slack:

Is our idiom for errors like so?

if (problematic) throw new Error('…'); or should I use assert(!probematic,'error message') without the &&? or create some sort of new error function for use in cases like this?

[pixelzoom]

@samreid In your post to Slack, you didn't provide information about the nature of "problematic". And that determines whether to use assert of Error. Please describe your understanding of when it's appropriate to use assertions vs throwing an Error.

(If you use assert without assert &&, it will fail when assertions are disabled.)

[pixelzoom]

From http://www.suodenjoki.dk/us/archive/2011/assert.htm (bad grammar and all):

“an assertion is documentation that reason about how the design and the code should work. They are pre-conditions, post-conditions and loop invariants that the programmer make up to document expectations for a programs execution. It is the programmers contract with his design understanding.” … “Don’t use assert for error handling. It is not intended for this. It has nothing to do with error handling or exception handling at all.”

[samreid]

In the preceding commit I converted the Sim.js query string checks from assertions to errors. @pixelzoom can you please review?

[pixelzoom]

Looks good. I fixed one of the error messages, which was referring to the old screenIndex query param.

Closing.