Create a node for the screen/homescreen group and add a visible property

[kathy-phet]

To make it easier for clients to mimic a one-screen sim, we will put the home screen plus screen icons/names into a node grouping container, and then instrument that grouping to be able to have visible property that can be toggled on and off. Optionally, an enabled property could be available too for clients to prevent premature movement to another screen ... but not sure that is needed.

@arouinfar - Can you look at the tree and work with @jonathanolson on implementation today/tomorrow?

[zepumph]

This may be obsolete if instead we create a property that takes the value of the screens query parameter and updates the sim accordingly.

[kathy-phet]

I thought we didn't want to go that direction because screens doesn't work after using studio. And we want clients to have all four screens available through the api which doesn't work if the api is limited to one screen.

[arouinfar]

@kathy-phet the proposal is for there to be a Property that accepts a string formatted like the one used with the screens query parameter. The idea is that a client would create a single Standard PhET-iO Wrapper and then later set this new Property to something like 1,2 to only include the first two screens.

[jonathanolson]

Implemented the Property for this (sim.general.view.screensStringProperty), and did a lot of the dynamic layout work above so that this should update seamlessly.

One unintuitive thing is that you can currently be on a screen (1) and change the screens to (2,3) and it won't force a change of screens. I didn't know what was best (if I implement going to e.g. screen 2 in that case, it changes the focus in studio and makes things difficult).

@arouinfar can you review behavior for this change?

[jonathanolson]

@zepumph would you be able to review these changes?

Notably, selectScreens separation makes it particularly tricky, as we need the output of one section of it BEFORE we can create the HomeScreen. I had to add an earlier callback for this, but in general my solution is ugly in this way. Perhaps it would be best to separate selectScreens out into two functions? Thoughts?

[arouinfar]

@jonathanolson I reviewed screensSelectedProperty in the context of Buoyancy which has 4 screens. The navbar layout updates as expected when changing the value and looks great overall. Things also look fine when running Studio with screens -- screensSelectedProperty is read-only, though the valid values start to look a bit odd. For example running Studio with screens=3,4 has these valid values.

Presumably, the screens have been re-indexed upon startup. It looks odd, but seems acceptable given that screensStringProperty is read-only.

One unintuitive thing is that you can currently be on a screen (1) and change the screens to (2,3) and it won't force a change of screens. I didn't know what was best (if I implement going to e.g. screen 2 in that case, it changes the focus in studio and makes things difficult).

There are two cases where things get weird.

1. From the home screen, set screensStringProperty to a single screen value (e.g. "3") or to some value that excludes selectedScreenProperty.

2. Set screensStringProperty to some value that excludes the current screenProperty.

In case (1), it would be nice to set homeScreen.model.selectedScreenProperty to the first screen listed in screensStringProperty. If the value is 4,2,1, the selectedScreenProperty could be set to the 4th screen, corresponding to the leftmost button the home screen.

Similarly in (2), it would be nice to set general.model.screenProperty to the first screen listed in screensStringProperty. If the user is looking at screen 3 when setting the screens to 4,2,1, I would want to set screenProperty to the 4th (now 1st) screen.

However, in both of these situations, it seems rude to change selectedScreenProperty or screenProperty out from under the users unless their value corresponds to a screen that is being hidden with screensStringProperty.

I'm not sure what the best path forward is. It seems like we would need some complicated logic to have selectedScreenProperty or screenProperty fallback to the first screen in screensStringProperty. Perhaps this is a situation where we advise clients to make adjustments to selectedScreenProperty and/or screenProperty on their own. I'm curious to hear what @zepumph would advise.

[jonathanolson]

Yes, I was waffling back-and-forth between doing something (what you described) in those situations, or doing nothing.

[zepumph]

This is so fun! Thanks for all the hard work.

• I definitely think that we should have the screenProperty listen to this string, and if it is no longer available in the list, move to the "next available", I'm not sure if it matters too much which one that is.
• I noticed that the valid values are sorta in a weird order: Perhaps some work to sort these:
  
• In general I think that the best path forward is for us to discuss these changes together. I don't understand a fair bit of the complexity, and it would be helpful to discuss in order for me to see how we may improve it.

[kathy-phet]

Just reviewed briefly, so sorry if I'm missing something.

• Seems like re-indexing the screens could cause confusion, so remember the valid screens - so screen 3 is always screen 3, seems like it would be better.
• Having radio buttons of the valid values seems like it will get quite long for a 5 screen sim? Can we do validity checking when its entered, but not provide it as a radio button selection? Use documentation to provide directions?
• What is the vision here if the ?screens=3,4 is entered in Studio? Does it mean the sim loads with just those screens available and then you can't get back sceens 1,2? Or are screens 1,2 always available. If no ?screens= query is used with Studio, are all 4 screens available from the API, even if you use Studio's settings to start with screen 1,2?

[zepumph]

What is the vision here if the ?screens=3,4 is entered in Studio? Does it mean the sim loads with just those screens available and then you can't get back sceens 1,2? Or are screens 1,2 always available. If no ?screens= query is used with Studio, are all 4 screens available from the API, even if you use Studio's settings to start with screen 1,2?

I'm excited to discuss this this afternoon at 1:40, but basically my hope was to deprecate the ?screens query parameter as a PhET-iO feature, since this instrumented Property is a much more streamlined approach to support this both on startup AND for dynamic mutation during the runtime of the sim.

[jonathanolson]

There actually was an easy way to prevent the "reindexing", I've switched to it with the above commit.

[jonathanolson]

Future TODOs:

• Add phetioDocumentation
• Code review (together with @zepumph)

We renamed it to screensIncludedProperty, ordered the valid values, have the selected and active screens in the sim adjust nicely when it changes, and have those changes not mess with selection in studio. It's also not read-only anymore, even if the screens query parameter.

[jonathanolson]

Also notably, having it just be a string "Property" doesn't work well. When trying to change "1,2" to "1,2,3", it goes through an intermediate "broken" state of "1,2," (extra comma), and fails validation. It seems like for most cases, an explicit list is fine.

[samreid]

The screensIncludedProperty looks great and leads to a great UX for Gravity and Orbits:

However, it is a bit more unwieldy for a 4-screen sim like Graphing Quadratics (zoomed out so I could fit everything one one page):

[samreid]

For the UI, I was picturing something like https://stackoverflow.com/questions/10588607/tutorial-for-html5-dragdrop-sortable-list which could look like:

Maybe with grab handles so it looks more draggable.

Also, the underlying type could be ArrayIO<NumberIO> instead of StringIO

[samreid]

screensIncludedProperty will be Array<number> instead of String

general.model
  screens
    selectedScreenProperty (renamed from screenProperty, migration rule needed)
    availableScreensProperty
    isUserNavigableProperty

Add a linked element from the new navbar group so the visibleProperty links back to isUserNavigableProperty

[jonathanolson]

Implemented above, @samreid can you review?

[samreid]

@jonathanolson the commits and behavior look good. I added a commit with a migration rule so 1.5 states will load into 1.6 gravity and orbits. There was one other flaw noted in https://github.com/phetsims/studio/issues/268 which was corrected. All else seems good to me. Tagging @zepumph in case he wants another look or knows of more to do. Otherwise, closing.

[samreid]

As discovered in phetsims/chipper#946, there are TODOs in the code referring to this issue. Reopening.

[samreid]

Fixed, closing.