Closed matthew-blackman closed 1 year ago
This implementation is working OK:
The proposed approach utilizes the existing interface for PhET-iO statefulness and makes use of the PhET-iO Group. This allows for ease of maintenance through migration engines. However, there is some complexity in that it currently does not allow for creating a circuit in the first screen and then loading it in the second screen. Additionally, it saves the entire state of the circuit, including the selected circuit element, which may or may not be desired.
Also, it relies on the setting of partial state, which is used in other places such as gravity and orbits for restoring scenes, but is not very widely used.
I discussed this with @zepumph @matthew-blackman and @arouinfar and we agreed the UI seems reasonable. The constraint to only be able to load screen1 circuits into screen1 is reasonable. The phet-io implementation is reasonable. Here is our list of desired improvements and quesions:
Current patch:
This patch solves all the problems listed above and works well in testing. I haven't pushed yet because I don't know how to verify that this won't disrupt other sims get/set buttons. It's difficult to know where to look for those and to determine if one was inadvertently dropped.
I found a way to write the studio logic so that it was nondisruptive to existing code, and also a reasonable long-term solution for Circuit Construction Kit. There was one new line of logic in PhetioElementView which I believe @zepumph and I wrote, which I will save for a separate commit in case it's problematic.
After reading it through, it seems pretty safe. @matthew-blackman and @arouinfar can you please test this behavior? The main problem for me is the error message is very verbose (unreadable json state values) if you try to paste from one screen to another.
@samreid this is working, but I've noticed an issue with the Copy Set Value Code button. With the exception of the first time the Copy Set Value Code is used, you need to press Get Value before Copy Set Value Code to successfully copy the correct circuit state.
Steps to reproduce:
circuitConstructionKitDc.introScreen.model.circuit
.Thanks for identifying this problem, it appears that this behavior happens for all studio get/set/copy controls and is not specific to circuits. Searching the history of the implementation led me to this comment from @zepumph: https://github.com/phetsims/studio/issues/245#issuecomment-1029358335
- [x] Add "Copy Set Value Code" that will also error if there is invalid json. IF the text area is blank, then it calls "Get Value" function first to fill in the text area, then copies that value into somthing like
phetioClient.invoke( . . . .. )
(note the extra alias needed inwindow.phetioClient = window.phetio.phetioClient; // extra alias for flexibility and consistency with Wrapper template.
)
The popup tooltip also says:
'Copy to clipboard a line of code that invokes this customization on phetioClient.'
So it appears the design/implementation is to copy out of the text area instead of copying the value out of the sim. I wonder if this behavior was more apparent in simpler situations, where the value can be seen and understood at a glance. (Difficult to see what the circuit value means because it is very long).
I wonder if the implementation was proposed this way to support the following use case (I):
Brainstorming ways forward
I feel we may be better off to design this component around "least surprise" rather than "most flexible". That being said, I also feel it may be important to support the "use case (I)" listed above. If we do change the semantics, we would probably want to re-maintenance release all PhET-iO sims with this feature for consistency. It would be unfortunate if we had sims where "Copy Set Value Code" means different things.
@arouinfar @samreid and I agree that 'Copy Set Value Code' should have a consistent functionality, independent of whether the state textbox is populated or not. Either clicking 'Copy Set Value Code' should always repopulate the state textbox, or it never should.
@samreid proposed the following solution: Any time the text box is empty, the 'Copy Set Value Code' button should be disabled. The client will then have to click the 'Get Value' button before clicking 'Copy Set Value Code'. This will set up a consistent UX in which a client must always click 'Get Value' before 'Copy Set Value Code' when updating state.
We discussed whether this needs to be maintenance released into prior PhET-iO sims, and agree that it does not. Those sims did not exhibit this type of problem because their states are much simpler, and we want to avoid the cost of that maintenance release.
In the future, @samreid @arouinfar and I would like to consider how this function could be leveraged in the PhET brand version of the sim, so that teachers could save/load circuits without using the full functionality of PhET-iO. @samreid proposed that this could involve some PhET-iO features being utilized in the PhET brand version of the sim.
This is beyond the scope of the CCK-DC PhET-iO milestone, but we agree that the save/load feature has a high priority for our teacher clients, outside of the PhET-iO licensing model.
This has been discussed in https://github.com/phetsims/circuit-construction-kit-common/issues/151, which we would like to reopen after this milestone is complete.
I added logic that enables/disables the Copy Set Value Code button based on the text area. While I was there, it seemed natural to also disable the Set button if the text area is empty, so I did that too. This could use a code review and testing. Also, I tested and it works well on Chrome, but cross-browser testing will be important here due to the nature of the DOM and input listeners.
UPDATE: I labeled as status:blocks-publication as a way to indicate to other sims that this is code that is committed to master but not fully vetted.
Today @arouinfar and @matthew-blackman and I reviewed this implementation and agree it seems nice. We just need to do more cross-platform testing on each browser, being careful to test:
@samreid @matthew-blackman I tested across a variety of browsers and the behavior looks good, closing.
There isn’t any clear way for clients to populate specific circuits into the play area. They can get/set state, but that is the state for the ENTIRE sim, not just the circuit elements. We would probably want to do something similar for the tracks in ESP. Model.circuit or model.circuit.circuitElements could have a “Copy Set Value Code” button so instructional designers could extract the appropriate command to be used later in the wrapper.
We are not proposing a change that is visible in PhET brand or creating a circuit language that is human-readable.
Example use case: A client should be able to add custom buttons to load specific circuits, eg 'SERIES CIRCUIT' and 'PARALLEL CIRCUIT'. In creating the invoke statements that these buttons would trigger, the client should be able to copy/paste from studio using model.circuit or model.circuit.circuitElemnents.
@samreid and I discussed that we will not be able to leverage the preexisting get/set interface that works for axon properties, and may need to either 1) build a similar UI to appear within studio or 2) rely on PhET-iO clients running a command in the dev tools.
This is related to https://github.com/phetsims/circuit-construction-kit-common/issues/151, and the conversation was reopened during review of https://github.com/phetsims/circuit-construction-kit-common/issues/917