Checkbox mini-sprint #914
Assignees
Comments
|
@zepumph @samreid I reviewed Checkbox in several sims using the list we generated as a guide. I focused on the design-related aspects, ignoring things like migration. The tree structure of Checkbox is very consistent across sims and follows this pattern:
|
|
This patch adds My first review question about this proposal is that the implementation would be at odds with outside code trying to control the pickability, like Second review question is about how/how much to factor out between occurrences like this one and ComboBox and other cases. For instance, we have @zepumph can you please review and advise? Subject: [PATCH] Add displayOnlyProperty to Checkbox, see https://github.com/phetsims/sun/issues/914
---
Index: js/Checkbox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Checkbox.ts b/js/Checkbox.ts
--- a/js/Checkbox.ts (revision bfd00648a383eb16915781d62a98fce079511cf2)
+++ b/js/Checkbox.ts (date 1731601312264)
@@ -25,6 +25,8 @@
import Tandem from '../../tandem/js/Tandem.js';
import Utterance, { TAlertable } from '../../utterance-queue/js/Utterance.js';
import sun from './sun.js';
+import Property from '../../axon/js/Property.js';
+import BooleanProperty from '../../axon/js/BooleanProperty.js';
// constants
const BOOLEAN_VALIDATOR = { valueType: 'boolean' };
@@ -84,6 +86,10 @@
// Handles layout of the content, rectangles and mouse/touch areas
private readonly constraint: CheckboxConstraint;
+ // For use via PhET-iO, see https://github.com/phetsims/sun/issues/451
+ // This is not generally controlled by the user, so it is not reset when the Reset All button is pressed.
+ private readonly displayOnlyProperty: Property<boolean>;
+
public constructor( property: PhetioProperty<boolean>, content: Node, providedOptions?: CheckboxOptions ) {
const options = optionize<CheckboxOptions, SelfOptions, ParentOptions>()( {
@@ -234,6 +240,16 @@
tandemName: 'property'
} );
+ this.displayOnlyProperty = new BooleanProperty( false, {
+ tandem: options.tandem.createTandem( 'displayOnlyProperty' ),
+ phetioFeatured: true,
+ phetioDocumentation: 'disables interaction and makes it appear like a display that shows the current selection'
+ } );
+ this.displayOnlyProperty.link( displayOnly => {
+ checkboxNode.visible = !displayOnly;
+ this.pickable = !displayOnly;
+ } );
+
// support for binder documentation, stripped out in builds and only runs when ?binder is specified
assert && phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'sun', 'Checkbox', this );
@@ -247,6 +263,7 @@
this.checkedNode.dispose();
checkboxNode.dispose();
fireListener.dispose();
+ this.displayOnlyProperty.dispose();
if ( property.hasListener( checkboxCheckedListener ) ) {
property.unlink( checkboxCheckedListener ); |
|
Today in phet-io meeting we discussed the need for the toggleAction and firedEmitter. We certainly don't need both, but perhaps we don't need either. We should discuss what researchers need from UI components, because likely it is just looking at the model properties. |
samreid
added a commit
that referenced
this issue
Dec 4, 2024
…have a LinkedElement Property, see #914
I added an assertion for this. It seemed to be passing local aqua fuzzing, so let's double check on CT. But I'm not convinced that an assertion is the right level for this, we may want another validation flag.
|
There is no way for a Property to know if it will later be consumed by a CheckBox, so that cannot inform the Property default metadata. Should we add an assertion for this case?
Subject: [PATCH] Use Tandem.VALIDATION, see https://github.com/phetsims/sun/issues/914
---
Index: js/Checkbox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Checkbox.ts b/js/Checkbox.ts
--- a/js/Checkbox.ts (revision 73a4e885e0bb273251f52007795ee99f4cd0e7a4)
+++ b/js/Checkbox.ts (date 1733342312504)
@@ -237,6 +237,10 @@
if ( assert && Tandem.VALIDATION && this.isPhetioInstrumented() ) {
assert && assert( property.isPhetioInstrumented(), 'Property should be instrumented if Checkbox is instrumented' );
assert && assert( options.phetioLinkProperty, 'Property should be linked if Checkbox is instrumented' );
+
+ if ( this.phetioFeatured ) {
+ assert && assert( property.phetioFeatured, 'Property should be featured in the controlling Checkbox is featured' );
+ }
}
// support for binder documentation, stripped out in builds and only runs when ?binder is specified
UPDATE: with this patch, here is a list of repos that fail this assertion: That list was long enough for me to double check before visiting each site and adding phetioFeatured to those Properties. (Noting each simulation may have many cases). @arouinfar or @zepumph what do you think? |
samreid
added a commit
that referenced
this issue
Dec 4, 2024
zepumph
added a commit
that referenced
this issue
Dec 4, 2024
zepumph
added a commit
to phetsims/blackbody-spectrum
that referenced
this issue
Dec 4, 2024
zepumph
added a commit
to phetsims/faradays-electromagnetic-lab
that referenced
this issue
Dec 4, 2024
|
More display only Property notes: Subject: [PATCH] Add displayOnlyProperty to Checkbox, see https://github.com/phetsims/sun/issues/914
---
Index: js/Checkbox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Checkbox.ts b/js/Checkbox.ts
--- a/js/Checkbox.ts (revision 018efb020653313d2b4083829447c893896b0e21)
+++ b/js/Checkbox.ts (date 1733348415402)
@@ -6,7 +6,9 @@
* @author Chris Malley (PixelZoom, Inc.)
*/
+import BooleanProperty from '../../axon/js/BooleanProperty.js';
import PhetioProperty from '../../axon/js/PhetioProperty.js';
+import Property from '../../axon/js/Property.js';
import validate from '../../axon/js/validate.js';
import Bounds2 from '../../dot/js/Bounds2.js';
import { m3 } from '../../dot/js/Matrix3.js';
@@ -84,6 +86,10 @@
// Handles layout of the content, rectangles and mouse/touch areas
private readonly constraint: CheckboxConstraint;
+ // For use via PhET-iO, see https://github.com/phetsims/sun/issues/451
+ // This is not generally controlled by the user, so it is not reset when the Reset All button is pressed.
+ private readonly displayOnlyProperty: Property<boolean>;
+
public constructor( property: PhetioProperty<boolean>, content: Node, providedOptions?: CheckboxOptions ) {
const options = optionize<CheckboxOptions, SelfOptions, ParentOptions>()( {
@@ -245,6 +251,17 @@
// }
}
+ this.displayOnlyProperty = new BooleanProperty( false, {
+ tandem: options.tandem.createTandem( 'displayOnlyProperty' ),
+ phetioFeatured: true,
+ phetioDocumentation: 'disables interaction for use as a legend'
+ } );
+ this.displayOnlyProperty.link( displayOnly => {
+ checkboxNode.visible = !displayOnly;
+ this.tagName = displayOnly ? 'p' : 'input'; // TODO: this take away focusability, but still includes the text in the PDOM. Otherwise it is focusable and says "checkbox, unavailable" on NVDA.
+ this.inputEnabled = !displayOnly;
+ } );
+
// support for binder documentation, stripped out in builds and only runs when ?binder is specified
assert && window.phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'sun', 'Checkbox', this );
@@ -258,6 +275,7 @@
this.checkedNode.dispose();
checkboxNode.dispose();
fireListener.dispose();
+ this.displayOnlyProperty.dispose();
if ( property.hasListener( checkboxCheckedListener ) ) {
property.unlink( checkboxCheckedListener ); |
|
We made good progress trying to understand the relationship between enabled and displayOnly, and how it effects inputEnabled. Here is a patch that we likely could have committed, but I didn't test enough. |
samreid
added a commit
to phetsims/assert
that referenced
this issue
Dec 5, 2024
samreid
added a commit
to phetsims/charges-and-fields
that referenced
this issue
Dec 5, 2024
samreid
added a commit
to phetsims/molarity
that referenced
this issue
Dec 5, 2024
zepumph
added a commit
to phetsims/scenery
that referenced
this issue
Dec 6, 2024
|
more discussion about how best to account for gates that want to control inputEnabled: Subject: [PATCH] https://github.com/phetsims/sun/issues/914
---
Index: js/Checkbox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Checkbox.ts b/js/Checkbox.ts
--- a/js/Checkbox.ts (revision 2096c82ddc24bacb4d7c86a091a9b154ab6c7e9e)
+++ b/js/Checkbox.ts (date 1733767471749)
@@ -6,7 +6,11 @@
* @author Chris Malley (PixelZoom, Inc.)
*/
+import BooleanProperty from '../../axon/js/BooleanProperty.js';
+import DerivedProperty from '../../axon/js/DerivedProperty.js';
import PhetioProperty from '../../axon/js/PhetioProperty.js';
+import Property from '../../axon/js/Property.js';
+import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
import validate from '../../axon/js/validate.js';
import Bounds2 from '../../dot/js/Bounds2.js';
import { m3 } from '../../dot/js/Matrix3.js';
@@ -84,6 +88,10 @@
// Handles layout of the content, rectangles and mouse/touch areas
private readonly constraint: CheckboxConstraint;
+ // For use via PhET-iO, see https://github.com/phetsims/sun/issues/451
+ // This is not generally controlled by the user, so it is not reset when the Reset All button is pressed.
+ private readonly displayOnlyProperty: Property<boolean>;
+
public constructor( property: PhetioProperty<boolean>, content: Node, providedOptions?: CheckboxOptions ) {
const options = optionize<CheckboxOptions, SelfOptions, ParentOptions>()( {
@@ -209,14 +217,67 @@
} );
this.addInputListener( fireListener );
+ this.displayOnlyProperty = new BooleanProperty( false, {
+ tandem: options.tandem.createTandem( 'displayOnlyProperty' ),
+ phetioFeatured: true,
+ phetioDocumentation: 'disables interaction for use as a legend'
+ } );
+
+ // A dynamic property that can work with multiple dependencies to keep the right value.
+ // TODO: Use TinyForwardingProperty or DynamicProperty as a container to hold a single DerivedProperty that changes as gates update (added or removed)?
+ // TODO: Can we use GatedBooleanProperty for this still? PhET-iO makes things challenging, but perhaps we can add assertion to the dynamic stuff that we aren't PhET-iO instrumented when we need this.
+ // TODO: Remember that issue about how we want to be able to swap out two instances in the PhET-iO API without calling it a disposal of a static element.
+ this.setInputEnabledProperty( new GatedDynamicBooleanProperty( {
+ gates: [ this.enabledProperty, DerivedProperty.not( this.displayOnlyProperty ) ]
+ } ) );
+
+
+ // TODO: But this is still horrible for PhET-iO, we'll keep talking.
+ this.setInputEnabledProperty( new DerivedProperty( [ this.inputEnabledProperty.target, new GatedDynamicBooleanProperty( {
+ gates: [ this.enabledProperty, DerivedProperty.not( this.displayOnlyProperty ) ]
+ } ) ] ) );
+
+ // const x = ....;
+ // x.addGate( this.enabledProperty );
+ // x.addGate( DerivedProperty.not( this.displayOnlyProperty ) );
+
+ //////////////////////////////
+ // A the usage site
+ const myCheckbox = new Checkbox();
+ const x2 = myCheckbox.inputEnabledProperty.targetProperty; // or myCheckbox.gatedInputEnabledPropertygetter();
+ affirm( myCheckbox.inputEnabledProperty.targetProperty instanceof GatedDynamicBooleanProperty );
+ x2.addGate();
+
+
+ // Other ideas that got us here
+ // // this.addInputEnabledPropertyGate( DerivedProperty.not( this.displayOnlyProperty ) );
+ //
+ // this.inputEnabledProperty.addGate(!DerivedProperty.not( this.displayOnlyProperty ));
+ //
+ // const x = new DisplayOnlyProperty( this.enabledProperty, this.inputEnabledProperty );
+ // x.addInputEnabledGate( this.enabledProperty );
+ // x.addInputEnabledGate( anotherGate );
+ // // x.addInputEnabledGate( DerivedProperty.not( this.displayOnlyProperty ) );
+ //
+ // this.addInputEnabledPropertyGate( DerivedProperty.not( this.displayOnlyProperty ) );
+
// sync with property
const checkboxCheckedListener = ( checked: boolean ) => {
this.checkedNode.visible = checked;
this.uncheckedNode.visible = !checked;
- this.pdomChecked = checked;
+ if ( !this.displayOnlyProperty.value ) {
+ this.pdomChecked = checked; // TODO????
+ }
};
+
property.link( checkboxCheckedListener );
+ this.displayOnlyProperty.link( displayOnly => {
+ checkboxNode.visible = !displayOnly;
+ this.tagName = displayOnly ? 'p' : 'input'; //TODO: still a work in progress.
+ checkboxCheckedListener && checkboxCheckedListener( property.value );
+ } );
+
// Apply additional options
this.mutate( options );
@@ -256,6 +317,7 @@
this.checkedNode.dispose();
checkboxNode.dispose();
fireListener.dispose();
+ this.displayOnlyProperty.dispose();
if ( property.hasListener( checkboxCheckedListener ) ) {
property.unlink( checkboxCheckedListener );
@@ -266,6 +328,14 @@
};
}
+ public override set inputEnabledProperty( property: TReadOnlyProperty<boolean> | null ) {
+ assert && assert( false, 'Checkbox.inputEnabledProperty is read-only' );
+ }
+
+ public override setInputEnabledProperty( newTarget: TReadOnlyProperty<boolean> | null ): this {
+ assert && assert( false, 'Checkbox.inputEnabledProperty is read-only' );
+ }
+
public override dispose(): void {
this.constraint.dispose();
|
jessegreenberg
pushed a commit
to phetsims/perennial
that referenced
this issue
Dec 10, 2024
zepumph
added a commit
to phetsims/density-buoyancy-common
that referenced
this issue
Dec 13, 2024
zepumph
added a commit
that referenced
this issue
Dec 13, 2024
|
zepumph
added a commit
to phetsims/solar-system-common
that referenced
this issue
Dec 13, 2024
zepumph
added a commit
to phetsims/natural-selection
that referenced
this issue
Dec 13, 2024
3 tasks
zepumph
added a commit
that referenced
this issue
Dec 13, 2024
|
Alright. ToggleAction was removed, and more opt-ins have been done. Over to @arouinfar to take a look through other usages to see if we want any more displayOnlyProperty opt-ins.
@samreid can you please review my commits? |
|
The code commits look good, and I added a bit of documentation. Leaving assigned to @arouinfar for steps identified above. |
|
from standup today, @arouinfar is going to create a separate issue to go over current hydrogen sims that may want this added. Above @samreid and I made one more minor change to remove an unused option. Also please note that likely the linked element code in Checkbox will change over in #921. Over to @arouinfar. I believe this issue just needs a review of the current checkbox instrumentation level, and the look and feel of displayOnlyProperty. |
44 tasks
|
All good here. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The PhET-iO team is revisiting many common code components to make improvements and add value.
UPDATE: THIS LIST WAS EXPANDED IN PHET-IO MEETING AGENGA, SEE NOTEST FROM 11/14.
PhET-iO:
Other
UPDATE: THIS LIST WAS EXPANDED IN PHET-IO MEETING AGENGA, SEE NOTEST FROM 11/14.
The text was updated successfully, but these errors were encountered: