feedback on initial PhET-iO instrumentation

[pixelzoom]

Related to #92 (PhET-iO instrumentation).

The initial pass at PhET-iO instrumention has been completed, and can be test-driven at https://phet-dev.colorado.edu/html/ph-scale/1.4.0-dev.11/phet-io/.

Since I don't know what the deadline is, I don't know how to prioritize this issue. The minimum that I'll need is 2 weeks to make changes, and 2 weeks to get the sim through QA.

Here's what needs to be done:

• Verify all client requests listed in pH Scale: PhET-iO Design.

• Identify and describe any additional general requirements. (The "General" section of pH Scale: PhET-iO Design is currently empty.)

• Review the structure of the tree in Studio. Identify changes/additions (names, structure, linked elements, etc.)

• Review the data stream using one of the data stream wrappers.

[kathy-phet]

Update - @arouinfar and I spent ~2 hours on this and completed review of Micro and Macro screens. We will do another session next week. Overall, looking really nice @pixelzoom. Amy will make a few issues.

[arouinfar]

@pixelzoom here are our comments/questions from the review of the Macro and Micro screens.

• We would like macroScreen.model.autoFillEnabledProperty and microScreen.model.autoFillEnabledProperty hooked up to a common query parameter to allow phet-brand users to access this feature. A flag called autoFill seems appropriate.

• Is model.dropper.emptyProperty ever false? Under what circumstance would the dropper ever be empty?

• view.drainFaucetNode and view.waterFaucetNode should probably both have a linked property that points back to the appropriate model.x.flowRateProperty. However, the flowRateProperty is read only, so we are not sure if adding *property is appropriate here. Does there need to be some property in the view that sets the flowRateProperty in the model?

• These properties should be read only, since the sim should control when these buttons are disabled rather than the client.
  microScreen.view.graphNode.linearGraphNode.zoomButtonGroup.zoomInButton.enabledProperty
microScreen.view.graphNode.linearGraphNode.zoomButtonGroup.zoomOutButton.enabledProperty

• Would it be possible to factor out all of the graph values into something like microScreen.view.graphNode.values? Currently, the same value appears in both the linearGraphNode and the logarithmicGraphNode (e.g. microScreen.view.graphNode.linearGraphNode.valueH2OProperty and microScreen.view.graphNode.logarithmicGraphNode.valueH2OProperty). To further complicate things, these values don't have units because they switch between concentration (mol/L) and quantity (mol) which could get confusing when filtering the data stream. Proposed structure:

• graphNode
  • values
    • valueConcentrationH2OProperty
    • valueQuantityH2OProperty
    • Repeat for H3O and OH

[pixelzoom]

Thanks @arouinfar. Each of the 5 checklist items in #117 (comment) is worthy of its own GitHub issue, which I've created and linked to this issue.

[arouinfar]

@kathy-phet and I reviewed the My Solution screen in studio, concluding our design review. I've opened the above issues (and I have two more to open tomorrow).

These items seem a bit smaller, so I'll ask them here rather than in their own issues.

• mySolutionsScreen.view.graphNode.graphScaleProperty is technically irrelevant on this screen since graph scale is always logarithmic. We don't see any particular harm in leaving it around since it labels the scale, but it should be phetioReadOnly: true.

• In the tree, the drag handlers of the GraphIndicatorNodes are siblings of the GraphIndicatorNode. For example:
  mySolutionsScreen.view.graphNode.logarithmicGraphNode.indicatorH3ODragHandler and
  mySolutionsScreen.view.graphNode.logarithmicGraphNode.indicatorH3ONode
  We think it would be more intuitive for clients if the drag handler could be moved under the corresponding GraphIndicatorNode, but we aren't sure if it's doable/reasonable.

[pixelzoom]

These items seem a bit smaller, so I'll ask them here rather than in their own issues.

There is no issue too small for a GitHub issue :). So I've moved these to their own issue.

[pixelzoom]

@arouinfar said:

... concluding our design review.

@arouinfar There are 2 items unchecked in #117 (comment). It sounds like you've reviewed phetioID names. Have you reviewed the data stream using one of the wrappers?

[pixelzoom]

Labeling for design meeting to discuss what else needs to be done here.

[pixelzoom]

3/5/20 design meeting: @arouinfar and @kathy-phet will look at data stream, prioritizing high-frequency.

[arouinfar]

Data stream has been reviewed, see above issue for phetioHighFrequency recommendations.