Generalize Stopwatch and StopwatchNode

[samreid]

The design for Circuit Construction Kit: AC calls for a stopwatch. I noticed that gas-properties/js/common/model/Stopwatch.js and gas-properties/js/common/view/StopwatchNode.js look like good candidates for generalization. They have features that most other sims will need to leverage as well, including moveToFront, interrupting user interactions, dragging, drag bounds, etc. I would be duplicating most of this for the development for CCK, and some of this may already be duplicated in other sims with draggable stopwatches. The main sim-specific parts (units and color) could be moved to options.

Discovered in conjunction with phetsims/scenery-phet#530

@pixelzoom what do you think?

UPDATE: Tagging for phetsims/circuit-construction-kit-common#449

[pixelzoom]

This seems reasonable. But I won't have time to evaluate for about a week. In the meantime, gas-properties is in the middle of RC testing for a maintenance release. I'd appreciate it if we didn't make changes to gas-properties master until that maintenance release is published, because it may complicate cherry picking.

[pixelzoom]

By the way... There are a couple of things that I really don't like about StopwatchNode, issues that are rooted in TimerNode and TimerReadoutNode:

• The timer stops when it reaches a max. That behavior would be OK as an option (not sure when I'd want to use it) but it's unacceptable as the only behavior. Imo, a design flaw.
• The value display is not general. It can only show "HH:MM:SS" or a value to 2 decimal places. For example, if I wanted to show only "5 ps" or "5.3 ps" (which would have been nice) in Gas Properties, then I'm out of luck. This smells like something that was sim-specific and not properly generalized.

For both of these flaws, we chose to live with them in Gas Properties.

[pixelzoom]

I said:

... I'd appreciate it if we didn't make changes to gas-properties master until that maintenance release is published, because it may complicate cherry picking.

When I wrote that, Gas Properties 1.0.1 RC testing was close to completion. But it has now been downgraded to "medium" priority (see phetsims/qa#415 (comment)) and the QA queue looks pretty full. phetsims/circuit-construction-kit-common#449 says "Publish CCK AC 1.0 by January 31, 2020" so maybe that's OK. Mentioning @samreid and @ariel-phet, in case there needs to be any priority adjustment.

[pixelzoom]

Stopwatch looks like it's already generalized. It has no dependencies on GAS_PROPERTIES other than namespace. It does refer to "time delta, in ps" but there's no code that depends on ps.

StopwatchNode does have dependencies on GAS_PROPERTIES

• GasPropertiesQueryParameters.origin can just be deleted, or replaced with ?dev.
• GasPropertiesColorProfile.stopwatchBackgroundColorProperty can be replaced (and deleted) with 'rgb( 80, 130, 230 )', unless there's a general need to dynamically change background color.
• DragBoundsProperty could be moved into StopwatchNode.js as an inner class, but it's also used by CollisionCounterNode,

Assigning back to @samreid. Feel free to proceed when Gas Properties 1.0.1 has been published, addressing (or not) concerns that I noted in #170 (comment) as the design team sees fit.

[pixelzoom]

One thing that I just noticed... All Stopwatch Properties are instrumented for PhET-iO. One of those Properties is this.visiblePropery, which may be irrelevant (or undesirable for instrumentation) if you are dragging the Stopwatch out of a toolbox. In Gas Properties, visibility was controlled via a checkbox, and StopwatchNode observes this Property. Not sure what you'll want to do here design-wise.

[samreid]

Tagging as potential work for phetsims/circuit-construction-kit-common#528

[samreid]

Assigning back to @samreid. Feel free to proceed when Gas Properties 1.0.1 has been published

Gas Properties 1.0.3 has been published. I'll take a look around.

[samreid]

Note that the proposed renamings in phetsims/scenery-phet#530 will affect how this work can be done, since there could be naming collisions.

[pixelzoom]

I understand that work has resumed on this. Are you addressing any of the problems identified in #170 (comment)?

[samreid]

I reviewed both of those requests but have not begun investigating them. My initial commits will focus on:

• moving Stopwatch to scenery-phet
• renaming SCENERY_PHET/TimerNode to SCENERY_PHET/StopwatchNode
• Adding features from GAS_PROPERTIES/StopwatchNode to SCENERY_PHET/StopwatchNode and adapting the API accordingly
• Updating all simulation to use the new API

[samreid]

I've completed an initial pass of this work, I'll begin commits and pushes. I've marked several TODOs referring to this issue which should be done after initial pushes, or, in many cases, converted to new issues.

[samreid]

I opened issues for other repos where review will be required. I'll now push changes for Gas Properties and common code changes linked to this issue. Note there are still 9 TODOs marked referencing this issue. As noted above, these should addressed after initial pushes, or converted to issues. I'm still working on this and will reassign to @pixelzoom once it's ready for review. I'll hold off on requesting reviews in the other repos (linked above) until this issue is complete, so we can make sure the API is finalized.

[pixelzoom]

There are still problems in both Stopwatch and StopwatchNode.

In Stopwatch, you didn't follow the pattern of putting disposal in this.disposeStopwatch, and dispose was missing override annotation. Since those fixes were straightforward, I handled them in the 2 commits above.

In StopwatchNode:

• There's no need for this.dragListener, it can be let dragListener.

• Comment "may be reassigned below, if draggable" is incorrect. "May be assigned below if dragging is constrained to bounds".

• Anonyomous new DragListener is still not cleaned up, and is leaking because it registers a tandem. You need to keep a reference to it and dispose of it.

• StopwatchNode.dispose is missing @override.

Back to @samreid to address these remaining issues.

[samreid]

Comment "may be reassigned below, if draggable" is incorrect. "May be assigned below if dragging is constrained to bounds".

Those are conflated at the moment, the docs for that option are:

visibleBoundsProperty: null, // {Property.<Bounds2>|null} if provided, the node is draggable within the bounds

That is to say, if visibleBoundsProperty is not provided, the node is not draggable at all. How do you recommend to proceed?

[samreid]

There's no need for this.dragListener, it can be let dragListener.

I made that change in phetsims/scenery-phet@e3f49a7 then CT pointed out that it is used for drag forwarding. So restored it in phetsims/scenery-phet@49c38f2 and annotated it as @public.

[pixelzoom]

CT pointed out that it is used for drag forwarding

Where is this.dragListener used for drag forwarding? And is it wise to make something that part of the public API when it may be null?

if visibleBoundsProperty is not provided, the node is not draggable at all. How do you recommend to proceed?

Sounds like a bug. If there are no drag bounds, then StopwatchNode should still be draggable, with no constraints on the drag bounds.

[samreid]

Would you recommend a boolean isDraggable option? Would you mind a check that if isDraggable is true, that the visibleBoundsProperty is required, at least until we have our first sim design where the timer should be draggable out of bounds?

Where is this.dragListener used for drag forwarding?

For example:
https://github.com/phetsims/masses-and-springs/blob/6fbdc6f98c707fdb2c59ac4a6ec5f7843fc77ae5/js/common/view/ToolboxPanel.js#L136-L137

And is it wise to make something that part of the public API when it may be null?

The existing implementation seems straightforward to me, but I'd love to hear a better one.

[pixelzoom]

I guess we can still with visibleBoundsProperty, since I don't see a need to drag outside the visible bounds. Just bringing it up because that wasn't my original intention when I added this feature for Gas Properties.

Thanks for clarifying about this.dragListener, current API seems reasonable. Might be nice to add a comment about why it's @public.

[samreid]

Good idea, I added a note. Anything else to do for this issue?

[pixelzoom]

Looks good. Thanks for tackling this issue. Closing.