phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Vertex label missing from loaded premade circuit #981

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.2.1

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/908, if I add a label to a vertex, save the circuit and then load it into the console of Studio OR a Standard Wrapper, the vertex label is missing. Labels that were added to other circuit elements, such as bulbs and batteries, are still present. If I add vertex labels and launch the Standard Wrapper, the labels are there.

Steps to reproduce

  1. On the intro screen, combine 2 elements
  2. Label that vertex
  3. Go to `circuitConstructionKitDc.introScreen.model.circuit' and press 'Get Value' and then 'Copy Set Value Code'
  4. Press the Reset All button and launch Standard Wrapper
  5. In the console of the Standard Wrapper, paste the code

Visuals Original Circuit:

Screenshot 2023-03-01 at 2 16 41 PM

Loaded Circuit in Standard Wrapper:

Screenshot 2023-03-01 at 2 18 37 PM
matthew-blackman commented 1 year ago

@samreid @arouinfar and I investigated this and found that the issue is arising from the fact that vertex labels use LocalizedStringProperty, while circuit element labels use resular StringProperty. If we switch all labels to allow clients to be able to set locale-specific labels, the labels cannot also be included in the circuit state.

We discussed this and agree that it is more important for PhET-iO clients to be able to include labels in the circuit state for save/load functionality than having labels implement locale-switching. This would mean using StringProperty for all labels instead of LocalizedStringProperty.

@samreid and I will investigate further to see if it is possible to have locale-switching on labels, and also have those labels save into the circuit state. We estimate that this would take about 2 hours to implement, but would like to defer that work to client-specific requests in the interest of publication. For this RC, we would like to prioritize the statefulness of labels over locale-switching for labels.

samreid commented 1 year ago

Localizable string state is stored in {sim}.general.model.stringsState. So in addition to

    getValue: {
      returnType: ObjectLiteralIO,
      parameterTypes: [],
      implementation: function( this: Circuit ) {
        return phet.phetio.phetioEngine.phetioStateEngine.getState( this );
      },
      documentation: 'Gets the current value of the circuit on this screen.'
    },

We would need to tack on matching keys from fragments like:

  "circuitConstructionKitDc.general.model.stringsState": {
    "data": {
      "circuitConstructionKitDc.introScreen.model.circuit.vertexGroup.vertex_0.labelStringProperty": {
        "en": "TEST6"
      }
    }
  },

Alternatively, we would need to rearchitect things so that some strings can store their deltas outside of general.model.stringsState. Either way sounds fragile and like it will lead to long-term complications. So we will start by un-localizing the vertex strings for now. Clients can still apply locale-specific customizations via API calls.

samreid commented 1 year ago

Fixed and ready for review and testing.

matthew-blackman commented 1 year ago

Reviewed https://github.com/phetsims/circuit-construction-kit-common/commit/fd8bc3d23c1be0ca6b662d0b6e4848eea09bba72 and everything looks good.

Tested in studio and the vertex labels are now setting with the circuit state. @arouinfar please confirm and feel free to close if all looks good.

Nancy-Salpepi commented 1 year ago

Looks good in master for me too!

samreid commented 1 year ago

@arouinfar please confirm and feel free to close if all looks good.

If it seems good, please leave open and mark as ready-for-cherry-picking.

arouinfar commented 1 year ago

Looks good!

samreid commented 1 year ago

For QA, please test as described in the top comment.

Nancy-Salpepi commented 1 year ago

Looks good in rc.3! Closing