Skip to content

Commit

Permalink
convert instrumented pickableProperty to inputEnabledProperty, #1158
Browse files Browse the repository at this point in the history
  • Loading branch information
@zepumph
zepumph committed Mar 17, 2021
1 parent f4f046a commit 6102095
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 85 deletions.
101 changes: 29 additions & 72 deletions js/nodes/Node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ import PhetioObject from '../../../tandem/js/PhetioObject.js';
import Tandem from '../../../tandem/js/Tandem.js';
import BooleanIO from '../../../tandem/js/types/BooleanIO.js';
import IOType from '../../../tandem/js/types/IOType.js';
import NullableIO from '../../../tandem/js/types/NullableIO.js';
import ParallelDOM from '../accessibility/pdom/ParallelDOM.js';
import Instance from '../display/Instance.js';
import Renderer from '../display/Renderer.js';
Expand All @@ -188,7 +187,6 @@ let globalIdCounter = 1;
const scratchBounds2 = Bounds2.NOTHING.copy(); // mutable {Bounds2} used temporarily in methods
const scratchMatrix3 = new Matrix3();

const PICKABLE_PROPERTY_TANDEM_NAME = 'pickableProperty';
const ENABLED_PROPERTY_TANDEM_NAME = EnabledProperty.TANDEM_NAME;
const VISIBLE_PROPERTY_TANDEM_NAME = 'visibleProperty';
const INPUT_ENABLED_PROPERTY_TANDEM_NAME = 'inputEnabledProperty';
Expand All @@ -201,7 +199,6 @@ const NODE_OPTION_KEYS = [
'visibleProperty', // {Property.<boolean>|null} - Sets forwarding of the visibleProperty, see setVisibleProperty() for more documentation
'visible', // {boolean} - Whether the Node is visible, see setVisible() for more documentation

'pickablePropertyPhetioInstrumented', // {boolean} - When true, create an instrumented pickableProperty when this Node is instrumented, see setPickablePropertyPhetioInstrumented() for more documentation
'pickableProperty', // {Property.<boolean|null>|null} - Sets forwarding of the pickableProperty, see setPickableProperty() for more documentation
'pickable', // {boolean|null} - Whether the Node is pickable, see setPickable() for more documentation

Expand Down Expand Up @@ -258,7 +255,6 @@ const DEFAULT_OPTIONS = {
visible: true,
opacity: 1,
pickable: null,
pickablePropertyPhetioInstrumented: false,
enabled: true,
enabledPropertyPhetioInstrumented: false,
inputEnabled: true,
Expand Down Expand Up @@ -348,7 +344,7 @@ class Node extends PhetioObject {
// @private {TinyForwardingProperty.<boolean|null>} - See setPickable() and setPickableProperty()
// NOTE: This is fired synchronously when the pickability of the Node is toggled
this._pickableProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.pickable,
DEFAULT_OPTIONS.pickablePropertyPhetioInstrumented, this.onPickablePropertyChange.bind( this ) );
false, this.onPickablePropertyChange.bind( this ) );

// @public {TinyForwardingProperty.<boolean>} - See setEnabled() and setEnabledProperty()
this._enabledProperty = new TinyForwardingProperty( DEFAULT_OPTIONS.enabled,
Expand Down Expand Up @@ -4003,7 +3999,7 @@ class Node extends PhetioObject {
* @returns {Node} for chaining
*/
setPickableProperty( newTarget ) {
return this._pickableProperty.setTargetProperty( this, PICKABLE_PROPERTY_TANDEM_NAME, newTarget );
return this._pickableProperty.setTargetProperty( this, null, newTarget );
}

/**
Expand All @@ -4016,29 +4012,6 @@ class Node extends PhetioObject {
this.setPickableProperty( property );
}

/**
* Handles linking and checking child PhET-iO Properties such as visibleProperty and pickableProperty.
* @public
*
* @param {string} tandemName - the name for the child tandem
* @param {Property.<boolean>|undefined|null} oldProperty - same typedef as TinyForwardingProperty.targetProperty
* @param {Property.<boolean>|undefined|null} newProperty - same typedef as TinyForwardingProperty.targetProperty
*/
updateLinkedElementForProperty( tandemName, oldProperty, newProperty ) {
assert && assert( oldProperty !== newProperty, 'should not be called on same values' );

// Only update linked elements if this Node is instrumented for PhET-iO
if ( this.isPhetioInstrumented() ) {

oldProperty && oldProperty.isPhetioInstrumented() && this.removeLinkedElements( oldProperty );

const tandem = this.tandem.createTandem( tandemName );
if ( newProperty && newProperty.isPhetioInstrumented() && tandem !== newProperty.tandem ) {
this.addLinkedElement( newProperty, { tandem: tandem } );
}
}
}

/**
* Get this Node's pickableProperty. Note! This is not the reciprocal of setPickableProperty. Node.prototype._pickableProperty
* is a TinyForwardingProperty, and is set up to listen to changes from the pickableProperty provided by
Expand Down Expand Up @@ -4067,28 +4040,6 @@ class Node extends PhetioObject {
return this.getPickableProperty();
}

/**
* Use this to automatically create a forwarded, PhET-iO instrumented pickableProperty internal to Node. This is different
* from visible because pickable by default doesn't not create this forwarded Property.
* @public
*
* @param {boolean} pickablePropertyPhetioInstrumented
* @returns {Node} - for chaining
*/
setPickablePropertyPhetioInstrumented( pickablePropertyPhetioInstrumented ) {
return this._pickableProperty.setTargetPropertyInstrumented( pickablePropertyPhetioInstrumented, this );
}

/**
* See setPickablePropertyPhetioInstrumented() for more information
* @public
*
* @param {boolean} value
*/
set pickablePropertyPhetioInstrumented( value ) {
this.setPickablePropertyPhetioInstrumented( value );
}

/**
* Sets whether this Node (and its subtree) will allow hit-testing (and thus user interaction), controlling what
* Trail is returned from node.trailUnderPoint().
Expand Down Expand Up @@ -4173,6 +4124,30 @@ class Node extends PhetioObject {
// TODO: invalidate the cursor somehow? #150
}


/**
* Handles linking and checking child PhET-iO Properties such as visibleProperty and enabledProperty.
* @public
*
* @param {string} tandemName - the name for the child tandem
* @param {Property.<boolean>|undefined|null} oldProperty - same typedef as TinyForwardingProperty.targetProperty
* @param {Property.<boolean>|undefined|null} newProperty - same typedef as TinyForwardingProperty.targetProperty
*/
updateLinkedElementForProperty( tandemName, oldProperty, newProperty ) {
assert && assert( oldProperty !== newProperty, 'should not be called on same values' );

// Only update linked elements if this Node is instrumented for PhET-iO
if ( this.isPhetioInstrumented() ) {

oldProperty && oldProperty.isPhetioInstrumented() && this.removeLinkedElements( oldProperty );

const tandem = this.tandem.createTandem( tandemName );
if ( newProperty && newProperty.isPhetioInstrumented() && tandem !== newProperty.tandem ) {
this.addLinkedElement( newProperty, { tandem: tandem } );
}
}
}

/**
* Sets what Property our enabledProperty is backed by, so that changes to this provided Property will change this
* Node's enabled, and vice versa. This does not change this._enabledProperty. See TinyForwardingProperty.setTargetProperty()
Expand Down Expand Up @@ -6618,11 +6593,12 @@ class Node extends PhetioObject {
// the default, instrumented visibleProperty is conditionally created. We don't want to store these on the Node,
// and thus they aren't support through `mutate()`.
visiblePropertyOptions: null,
pickablePropertyOptions: null,
enabledPropertyOptions: null,
inputEnabledPropertyOptions: null
}, config );

assert && assert( !config.hasOwnProperty( 'pickablePropertyOptions' ), 'DEBUG, remove me!!!!' );

// Track this, so we only override our visibleProperty once.
const wasInstrumented = this.isPhetioInstrumented();

Expand All @@ -6643,24 +6619,6 @@ class Node extends PhetioObject {
}, config.visiblePropertyOptions ) )
);

this._pickableProperty.initializePhetio( this, PICKABLE_PROPERTY_TANDEM_NAME, () => new Property( this.pickable, merge( {

// by default, use the value from the Node
phetioReadOnly: this.phetioReadOnly,
tandem: this.tandem.createTandem( PICKABLE_PROPERTY_TANDEM_NAME ),
phetioType: Property.PropertyIO( NullableIO( BooleanIO ) ),
phetioFeatured: true, // Since this property is opt-in, we typically only opt-in when it should be featured
phetioDocumentation: 'Sets whether the node will be pickable (and hence interactive). Pickable can take one ' +
'of three values:<br>' +
'<ul>' +
'<li>null: pass-through behavior. Nodes with input listeners are pickable, but nodes without input listeners won\'t block events for nodes behind it.</li>' +
'<li>false: The node cannot be interacted with, and it blocks events for nodes behind it.</li>' +
'<li>true: The node can be interacted with (if it has an input listener).</li>' +
'</ul>' +
'For more about Scenery node pickability, please see <a href="http://phetsims.github.io/scenery/doc/implementation-notes#pickability">http://phetsims.github.io/scenery/doc/implementation-notes#pickability</a>'
}, config.pickablePropertyOptions ) )
);

this._enabledProperty.initializePhetio( this, ENABLED_PROPERTY_TANDEM_NAME, () => new EnabledProperty( this.enabled, merge( {

// by default, use the value from the Node
Expand All @@ -6681,9 +6639,8 @@ class Node extends PhetioObject {
phetioDocumentation: 'Sets whether the node will have input enabled for (and hence interactive). This is a ' +
'subset of enabledProperty, which controls if input is enabled as well as changing style ' +
'etc.'
}, config.pickablePropertyOptions ) )
}, config.inputEnabledPropertyOptions ) )
);

}
}

Expand Down
11 changes: 5 additions & 6 deletions js/nodes/NodeAPI.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import PropertyAPI from '../../../axon/js/PropertyAPI.js';
import merge from '../../../phet-core/js/merge.js';
import PhetioObjectAPI from '../../../tandem/js/PhetioObjectAPI.js';
import BooleanIO from '../../../tandem/js/types/BooleanIO.js';
import NullableIO from '../../../tandem/js/types/NullableIO.js';
import scenery from '../scenery.js';
import Node from './Node.js';

Expand All @@ -30,9 +29,9 @@ class NodeAPI extends PhetioObjectAPI {
phetioType: Property.PropertyIO( BooleanIO )
},

pickablePropertyPhetioInstrumented: false,
pickablePropertyOptions: {
phetioType: Property.PropertyIO( NullableIO( BooleanIO ) ),
inputEnabledPropertyPhetioInstrumented: false,
inputEnabledPropertyOptions: {
phetioType: Property.PropertyIO( BooleanIO ),
phetioFeatured: true
},

Expand All @@ -47,10 +46,10 @@ class NodeAPI extends PhetioObjectAPI {
// @public (read-only)
this.visibleProperty = new PropertyAPI( options.visiblePropertyOptions );

if ( options.pickablePropertyPhetioInstrumented ) {
if ( options.inputEnabledPropertyPhetioInstrumented ) {

// @public (read-only)
this.pickableProperty = new PropertyAPI( options.pickablePropertyOptions );
this.inputEnabledProperty = new PropertyAPI( options.inputEnabledPropertyOptions );
}

if ( options.enabledPropertyPhetioInstrumented ) {
Expand Down
12 changes: 5 additions & 7 deletions js/nodes/NodeTests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,7 @@ QUnit.test( 'Trail and Node transform equivalence', assert => {
if ( Tandem.PHET_IO_ENABLED ) {

QUnit.test( 'Node instrumented visibleProperty', assert => testInstrumentedNodeProperty( assert, 'visible', 'visibleProperty', 'setVisibleProperty', true ) );

QUnit.test( 'Node instrumented pickableProperty', assert => testInstrumentedNodeProperty( assert, 'pickable', 'pickableProperty', 'setPickableProperty', Node.DEFAULT_OPTIONS.pickablePropertyPhetioInstrumented ) );


QUnit.test( 'Node instrumented enabledProperty', assert => testInstrumentedNodeProperty( assert, 'enabled', 'enabledProperty', 'setEnabledProperty', Node.DEFAULT_OPTIONS.enabledPropertyPhetioInstrumented ) );

QUnit.test( 'Node instrumented inputEnabledProperty', assert => testInstrumentedNodeProperty( assert, 'inputEnabled', 'inputEnabledProperty', 'setInputEnabledProperty', Node.DEFAULT_OPTIONS.inputEnabledPropertyPhetioInstrumented ) );
Expand Down Expand Up @@ -421,17 +419,17 @@ if ( Tandem.PHET_IO_ENABLED ) {
QUnit.test( 'PhET-iO API Validation', assert => {
phetioAPITest( assert, new NodeAPI( {
enabledPropertyPhetioInstrumented: true,
pickablePropertyPhetioInstrumented: true
inputEnabledPropertyPhetioInstrumented: true
} ), 'node', tandem => new Node( {
enabledPropertyPhetioInstrumented: true,
pickablePropertyPhetioInstrumented: true,
inputEnabledPropertyPhetioInstrumented: true,
tandem: tandem
} ) );

phetioAPITest( assert, new NodeAPI( {
pickablePropertyPhetioInstrumented: true
inputEnabledPropertyPhetioInstrumented: true
} ), 'node', tandem => new Node( {
pickablePropertyPhetioInstrumented: true,
inputEnabledPropertyPhetioInstrumented: true,
tandem: tandem
} ) );

Expand Down

0 comments on commit 6102095

Please sign in to comment.