Closed kathy-phet closed 1 year ago
This may be obsolete if instead we create a property that takes the value of the screens
query parameter and updates the sim accordingly.
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.
@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.
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?
@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?
@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.
From the home screen, set screensStringProperty
to a single screen value (e.g. "3"
) or to some value that excludes selectedScreenProperty
.
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.
Yes, I was waffling back-and-forth between doing something (what you described) in those situations, or doing nothing.
This is so fun! Thanks for all the hard work.
Just reviewed briefly, so sorry if I'm missing something.
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.
There actually was an easy way to prevent the "reindexing", I've switched to it with the above commit.
Future TODOs:
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.
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.
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):
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
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
Implemented above, @samreid can you review?
@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.
As discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs in the code referring to this issue. Reopening.
Fixed, closing.
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?