Closed samreid closed 6 months ago
Relevant code in StopwatchNode.ts:
const stopwatchVisibleListener = ( visible: boolean ) => {
300 this.visible = visible;
if ( visible ) {
this.moveToFront();
}
else {
// interrupt user interactions when the stopwatch is made invisible
this.interruptSubtreeInput();
}
};
Line 300 is the issue; it fails if a DerivedProperty is provided, which would be the natural way to address this in PDL.
In code review https://github.com/phetsims/projectile-data-lab/issues/32:
- [ ] We would like to request assistance on https://github.com/phetsims/projectile-data-lab/issues/180
Here's a patch that allows StopWatchNode to work correctly with a readonly (e.g. derived) visibleProperty
. It's low-risk that this will break other sims, but you should test (I did not).
Subject: [PATCH] link to https://github.com/phetsims/scenery-phet/issues/843 in comments
---
Index: js/StopwatchNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/StopwatchNode.ts b/js/StopwatchNode.ts
--- a/js/StopwatchNode.ts (revision 12c83d2e3eebd0fbd4f35e5064d27f4790ca572f)
+++ b/js/StopwatchNode.ts (date 1709588457640)
@@ -177,6 +177,7 @@
otherControls: [],
includePlayPauseResetButtons: true,
+ visibleProperty: stopwatch.isVisibleProperty,
// Tandem is required to make sure the buttons are instrumented
tandem: Tandem.REQUIRED,
@@ -299,7 +300,6 @@
}
const stopwatchVisibleListener = ( visible: boolean ) => {
- this.visible = visible;
if ( visible ) {
this.moveToFront();
}
In PDLStopwatchNode, you'll want to do something like:
const options = optionize<PDLStopwatchNodeOptions, SelfOptions, StopwatchNodeOptions>()( {
- visibleProperty: stopwatch.isVisibleProperty,
+ visibleProperty: new DerivedProperty.and(
+. [ stopwatch.isVisibleProperty, PDLPreferences.autoGenerateDataProperty ],
+ ( stopwatchIsVisible, autoGenerateData ) => stopwatchIsVisible && !autoGenerateData ),
Back to @samreid and @matthew-blackman for next steps.
@samreid and I implemented the recommendation from @pixelzoom in the following patch:
This is working well in PDL, and we'd like to do further testing on other sims before committing this to common code.
I ran the snapshot comparison test with no working copy changes and got this result:
Here are the results. The middle column is a synthetic test I did where I intentionally changed the title of balancing act to make sure it was caught (and it was). It also revealed some spurious changes:
I do not see problems specific to the stopwatch based on those changes alone. Next I manually tested Kepler's Laws, Capacitor Lab: Basics and Circuit Construction Kit: AC and they all behaved as expected. I think we should commit and see if CT finds anything else.
Precommit hooks seem ok:
node chipper/js/scripts/precommit-hook-multi.js
detected changed repos: projectile-data-lab, scenery-phet
projectile-data-lab: Success
scenery-phet: Success
Done in 77897ms
From https://github.com/phetsims/projectile-data-lab/issues/146 we would like to automatically hide the stopwatch when auto-generate data is enabled. We tried to do so but ran into a problem that StopwatchNode does not support a DerivedProperty for
visibleProperty
since it sets its own visibility viathis.visible = ...
. We may reach out to @pixelzoom for support on this.