search

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

Several elements should be read-only #854

Closed arouinfar closed 1 year ago

arouinfar commented 2 years ago

@samreid please set these to phetioReadOnly: true.

samreid commented 2 years ago

model.circuit.realLightBulbGroup.archetype.resistanceProperty (real bulbs don't have user-adjustable reistance)

Right, but giving an API to PhET-iO clients allows them to choose the value, without making it user-adjustable. Should clients be able to choose that resistance value?

view.circuitNode.*NodeGroup.archetype.visibleProperty

For the archetype ones, we do not yet support customizable archetypes. Are these requests for when https://github.com/phetsims/phet-io/issues/1856 is further along?

view.meterNodes.*.probeTextNode.readoutText.textProperty

I don't see those as instrumented any more. Should I resurrect it?

samreid commented 2 years ago

All issues addressed or asked about in the preceding comment. @arouinfar can you please review?

arouinfar commented 1 year ago

Thanks @samreid. I verified the checked items in https://github.com/phetsims/circuit-construction-kit-common/issues/854#issue-1133585268, but those related to strings are now moot.

model.circuit.realLightBulbGroup.archetype.resistanceProperty (real bulbs don't have user-adjustable resistance)

Right, but giving an API to PhET-iO clients allows them to choose the value, without making it user-adjustable. Should clients be able to choose that resistance value?

Excellent point @samreid. It's also now user-adjustable with the Get Value/Set Value functionality within Studio, so we should leave this as-is.

view.circuitNode.*NodeGroup.archetype.visibleProperty

For the archetype ones, we do not yet support customizable archetypes. Are these requests for when https://github.com/phetsims/phet-io/issues/1856 is further along?

Those related to the vertices should definitely be read-only. Things can get messy if clients are able to specify startVertexProperty, endVertexProperty, and positionProperty. The behavior in master looks good to me.

I don't remember the rationale for this particular visibleProperty, but we can more review archetypes in more detail in https://github.com/phetsims/phet-io/issues/1856.

view.meterNodes.*.probeTextNode.readoutText.textProperty

I don't see those as instrumented any more. Should I resurrect it?

I thought it was weird for clients to be able to edit the readout strings such as this one, which is why I asked for the textProperty to be read-only.

image

However, I think it may be useful to clients to know what value is being displayed in the meters, since it differs slightly from the underlying currentProperty or voltageProperty being measured. Conceptually, I think this is analogous to measuringTapeNode.readoutStringProperty in GAO.

I still see the readoutText in Studio, but I don't see a value in the Element Panel, so don't think it's working as expected.

image
samreid commented 1 year ago

OK we updated the tandems and marked things as readonly. @arouinfar ready for review.

arouinfar commented 1 year ago

I now see dependencies under the readoutText, for example:

image

However, I think it may be useful to clients to know what value is being displayed in the meters, since it differs slightly from the underlying currentProperty or voltageProperty being measured. Conceptually, I think this is analogous to measuringTapeNode.readoutStringProperty in GAO.

The readoutText links back to the underlying currentProperty so it's not analogous to the GAO example. However, my initial concern was that the readoutText was editable and then later lacking useful information in Studio. Those issues have been resolved, so we can close.