Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

how to add enabled/disabled feature to UI components? #257

Closed
pixelzoom opened this issue Sep 1, 2016 · 124 comments
Closed

how to add enabled/disabled feature to UI components? #257

pixelzoom opened this issue Sep 1, 2016 · 124 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 1, 2016

Developer consensus was to use this pattern for implementing enabled/disabled feature: #241 (comment). But as @samreid hinted at in #241 (comment), adding this feature currently requires duplicate/boilerplate code. I've recently added this feature to ComboBox and NumberControl, and thought we should discuss other methods of adding the feature.

Here's the relevant code that needs to be added:

function MyComponent( ..., options ) {

  options = _.extend( {
    enabledProperty: new Property( true ),
    disabledOpacity: 0.5, // {number} opacity used to make the control look disabled
  }, options );

  // validate options
  assert && assert( options.disabledOpacity >= 0 && options.disabledOpacity <= 1,
    'invalid disabledOpacity: ' + options.disabledOpacity );

  // @public
  this.enabledProperty = options.enabledProperty;

  // enable/disable the component
  var enabledObserver = function( enabled ) {
    self.pickable = enabled;
    self.opacity = enabled ? 1.0 : options.disabledOpacity;
  };
  this.enabledProperty.link( enabledObserver );

  // @private called by dispose
  this.disposeMyComponent = function() {
    self.enabledProperty.unlink( enabledObserver );
  };
}

return inherit( Supertype, MyComponent, {

  // @public
  dispose: function() {
    this.disposeMyComponent();
  },

  // @public
  setEnabled: function( enabled ) { this.enabledProperty.value = enabled; },
  set enabled( value ) { this.setEnabled( value ); },

  // @public
  getEnabled: function() { return this.enabledProperty.value; },
  get enabled() { return this.getEnabled(); }
} );
@jessegreenberg
Copy link
Contributor

Options discussed 9/1/16 meeting:

  • Investigate a mixin pattern
  • Add this functionality directly to scenery/Node.js
  • Create some subtype of Node.js that handles this for components that can be disabled

Adding functionality directly to scenery/Node.js is the favorite for now. But would modifying Node.js impact memory or are there other concerns? Assigning to @jonathanolson and @ariel-phet for comments.

@samreid
Copy link
Member

samreid commented Sep 6, 2016

@jessegreenberg @pixelzoom and @jonathanolson, does this still need to be marked for developer meeting?

@pixelzoom
Copy link
Contributor Author

Yes, it should remain marked for developer meeting. @ariel-phet is going to get feedback from @jonathanolson before the next meeting, when we will discuss again.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 7, 2016

Looks like @jonathanolson created #19 for this way back on 9/5/13. I'm going to close that issue, since the only useful information is a pointer to phetsims/scenery#17.

In phetsims/scenery#17, there was discussion of adding this feature to Node. But it looks like the conclusion was to not add this feature directly to Node, and to use a sun mix-in instead.

@pixelzoom
Copy link
Contributor Author

Also worth noting.... At the 9/1/16 meeting, no one seemed to have a clear concept of how "mix-in" is currently implemented, as used by @jonathanolson in scenery. And there were concerns about whether the current mix-in approach prevents the mix-in from accidentally overwriting something in the base type.

@ariel-phet
Copy link

Admission - I spaced on asking @jonathanolson to take a look at this on our skype call, but just requested him to take a look today. He might also be attending dev meeting remotely tomorrow.

@jonathanolson
Copy link
Contributor

It seems unlikely we'd want to bake in the "only a fixed level of opacity can be used to indicate disabled" into Scenery.

Setting pickability also seems weird (usually don't want to set it to false, but null which is the default). Additionally, when it's set to false, input events will go right through it to what is behind. Presumably instead the desired behavior is that it would absorb (but not respond to) the events.

Furthermore, the disposeMyComponent appears to be specific to each implemented type (presumably with a dispose function that would forward to it?).

This seems like something that we shouldn't bake into Scenery. However I've previously discussed considering adding disposal to Nodes and node trees, as many times this would be very useful.

no one seemed to have a clear concept of how "mix-in" is currently implemented

After the last discussion with the team, the current mix-in strategy is to have a function that takes the type as input, and modifies the prototype, e.g.:

Paintable.mixin( Path );

modifies the prototype of Path so that it contains functions specific to being paintable (Text and Path can both have fills and strokes, so the fill/stroke logic is isolated to the Paintable mixin). Much of it is just statements equivalent to:

var proto = Path.prototype;
proto.someFunction = function() { ... }

but it can also add/modify behavior in more advanced usages.

But would modifying Node.js impact memory or are there other concerns?

Not concerned about memory, just about the above.


Additionally, I'm not sure I understand why the enabledObserver is unlinked from the property in the code in the top comment. The property and observer are both owned by the same object (MyComponent), and would be garbage collected at the same time, correct?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 7, 2016

@jonathanolson said:

It seems unlikely we'd want to bake in the "only a fixed level of opacity can be used to indicate disabled" into Scenery.

Some (most?) of the current implementations have an option (disabledOpacity) for specifying the opacity, as indicated in the example shown in #257 (comment).

Also discussed at dev meeting (but not captured in comments above) was that using opacity would be the default strategy (since that is the approach that was arrived at via consensus), with the ability to specify a different strategy via options.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 7, 2016

@jonathanolson asked:

I'm not sure I understand why the enabledObserver is unlinked from the property in the code in the top comment.

Because enabledProperty can be provide by the client via options. So we don't know whether that Property is owned by the client or the component. This is the pattern that was agreed on in #241 (comment).

@jonathanolson
Copy link
Contributor

Also discussed at dev meeting (but not captured in comments above) was that using opacity would be the default strategy (since that is the approach that was arrived at via consensus), with the ability to specify a different strategy via options.

Sounds good to me, although it sounds like this should be something like a mix-in (rather than in Node.js directly). The mix-in could live in Scenery, or if it's Sun-specific, in Sun?

Because enabledProperty can be provide by the client via options. So we don't know whether that Property is owned by the client or the component. This is the pattern that was agreed on in #241 (comment).

Sounds good, I overlooked that it could be provided in options.

@samreid
Copy link
Member

samreid commented Sep 8, 2016

Sounds good to me, although it sounds like this should be something like a mix-in (rather than in Node.js directly).

@jonathanolson I don't disagree but could you clarify your reasoning here?

@jonathanolson
Copy link
Contributor

jonathanolson commented Sep 8, 2016

Sounds good to me, although it sounds like this should be something like a mix-in (rather than in Node.js directly).

@jonathanolson I don't disagree but could you clarify your reasoning here?

Node already provides the needed tools (opacity or other visual changes are available) and is much more general than representing just UI controls.

Being "enabled" seems like it's part of the element's (e.g. a button's) model. The button's view can then decide whether being disabled means lower opacity, a different gradient, or another effect.

We don't just use nodes for UI controls, but also use them for particles in many sims. I would not want to also add 'position' and 'velocity' vectors to Node because of this. The particle's controller modifies the Node's translation (usually) based on the model position. Similarly, a button's controller modifies the Node's opacity (usually) based on whether the button's model is enabled. Node just isn't the place to stuff the enabled/position/velocity/etc.-style logic.

@ariel-phet ariel-phet removed their assignment Sep 8, 2016
@samreid
Copy link
Member

samreid commented Sep 8, 2016

@jonathanolson mentioned we could create ComponentNode.js which adds enabled/disabled features (instead of using a mixin).

@samreid
Copy link
Member

samreid commented Sep 8, 2016

Let's leave this issue open and unassigned until someone needs to deal more with enabled features. When someone needs to add enabled/disabled to a new component, this issue will be assigned to them for investigating adding ComponentNode.js to scenery and using it.

@samreid
Copy link
Member

samreid commented Sep 8, 2016

@pixelzoom suggests putting ComponentNode.js in sun, that's OK with me.

@samreid
Copy link
Member

samreid commented Sep 8, 2016

@pixelzoom volunteered to take it for a test drive.

@samreid samreid assigned pixelzoom and unassigned jonathanolson Sep 8, 2016
pixelzoom added a commit that referenced this issue Sep 13, 2016
@pixelzoom
Copy link
Contributor Author

I implemented the enabled/disabled feature in base type UIComponent, which is checked into master but not used by any code that is checked into master. I experimented with using it in subtypes ComboBox and NumberControl, and it worked as expected. I did not commit changes to ComboBox and NumberControl, but I'm sure you can see how they'd use UIComponent.

UIComponent contains several TODO items that I'd like to discuss. I'll label for developer meeting to see who wants to be involved in further discussion/work on this issue. If any developers want to comment here on the TODO items, go for it.

@zepumph
Copy link
Member

zepumph commented Sep 29, 2020

Over in #585 (comment) @pixelzoom said:

While working on another issue, I discovered that in addition to EnabledComponent, there is EnabledNode and EnabledPhetioObject. They are all documented the same, all authored by @zepumph, and all updated recently. @zepumph can you please clarify (or better yet document) the relationship between these 3 classes, and when each one should be used?

I will answer here because this issue is out for review on this exact topic (and the EnabledComponent hierarchy in general).

The paper trail for this is from #257 (comment) and the comment right below from April 15th. A trait depends on certain characteristics of the class that it mixes in, so rather than force our only enabled-related mixin to be used on Node (the subtypiest subtype we need to support EnabledComponent logic for), @jbphet and I thought it would be more clear if a hierarchy was created to only expose the particular behavior you expected in it.

Alternatively, code would read like:

if ( this.hasOwnProperty( 'opacity') ){
 . . . . 
}
if ( this.hasOwnProperty( 'isPhetioInstrumented') ){
 . . . . 
}

I prefer the current strategy, and its associated unit tests. I used @mixes to jsdoc the relationship, and it parallels the hierarchy of the types its named for (PhetioObject-> Node). Would you please recommend how you would like this to change @pixelzoom?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 12, 2020

Thanks for the clarification about EnabledComponent vs EnabledNode vs EnabledPhetioObject.

@zepumph I'm confused about EnabledPhetioObject. It's header documentation says:

/**
 * Trait for PhetioObjects that will add an enabledProperty and confirm its PhET-iO instrumentation is as expected.
 * @author Michael Kauzmann (PhET Interactive Simulations)
 */

I don't see where it adds enabledProperty. It doesn't even verify that this.enabledProperty exists. As far as I can tell, all it does it verify that options.enabledProperty (if it exists) is instrumented. So the doc seems to be incorrect.

I'm also wondering why it inspects options.enabledProperty. Don't we want to verify that this.enabledProperty is instrumented?

@pixelzoom pixelzoom assigned zepumph and pixelzoom and unassigned pixelzoom Oct 12, 2020
@samreid
Copy link
Member

samreid commented Oct 13, 2020

I don't see where it adds enabledProperty. It doesn't even verify that this.enabledProperty exists.

Is it covered by this line?

    // mixin general EnabledComponent logic (parent mixin)
    EnabledComponent.mixInto( type );

@zepumph
Copy link
Member

zepumph commented Oct 13, 2020

Yes thanks @samreid, even though that logic is from the supertype, I still thought it worth mentioning since that is the main functionality of this Type.

Is it not ok to depend on functionality of the supertype? I don't fee like we ever assert that this.visible is a boolean after extending Node and calling super() in the constructor. initializeEnabledComponent is the super call for this mixin hierarchy.

@pixelzoom
Copy link
Contributor Author

@zepumph and I collaborated on refinements to EnabledComponent, as found in the above commit. Summary:

  • enabledProperty and enabledPropertyOptions are not mutually exclusive, because subclasses have a legitimate need to specify default values for enabledPropertyOptions. So if enabledProperty is provided, enabledPropertyOptions are simply ignored.

  • Added option enabled, which sets the initial value of enabledProperty if it's created by EnabledComponent. Again, simply ignored if enabledProperty is provided.

  • Added an assertion in initializeEnabledComponent to verify that we're not going to redefine this.enabledProperty.

@samreid
Copy link
Member

samreid commented Oct 17, 2020

CT is showing errors when a DerivedProperty is passed in as enabledProperty, for instance, in Number Play:

number-play : fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/number-play/number-play_en.html?continuousTest=%7B%22test%22%3A%5B%22number-play%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1602966524474%22%2C%22timestamp%22%3A1602967915033%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught Error: Cannot set values directly to a DerivedProperty, tried to set: false
Error: Cannot set values directly to a DerivedProperty, tried to set: false
at DerivedProperty.set (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/axon/js/DerivedProperty.js:120:24)
at RoundArrowButton.setEnabled (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/sun/js/buttons/RoundButtonView.js:213:38)
at RoundArrowButton.set enabled [as enabled] (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/sun/js/buttons/RoundButtonView.js:191:31)
at RoundArrowButton.onEnabledChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/fractions-common/js/common/view/RoundArrowButton.js:64:18)
at DerivedProperty.link (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/axon/js/Property.js:387:5)
at new RoundArrowButton (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/fractions-common/js/common/view/RoundArrowButton.js:54:26)
at new ShapeGroupNode (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/fractions-common/js/building/view/ShapeGroupNode.js:154:41)
at Function.createIcon (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/fractions-common/js/building/view/ShapeGroupNode.js:355:22)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/fractions-common/js/building/view/ShapeGroupStackNode.js:22:113
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1602966524474/phet-core/js/EnumerationMap.js:23:23
id: Bayes Chrome
Snapshot from 10/17/2020, 2:28:44 PM

This is because RoundArrowButton sets the enabledProperty value in onEnabledChange:

    // @private {function}
    this.enabledListener = this.onEnabledChange.bind( this );
    this.enabledProperty.link( this.enabledListener );
  }

  /**
   * Sets whether this is enabled.
   * @private
   *
   * @param {boolean} enabled
   */
  onEnabledChange( enabled ) {
    this.enabled = this.enabledProperty.value;
  }

This can be averted by changing the DerivedProperty to only throw an error if you try to set a different value. I'm not sure if this is a good solution, or if there should be a guard somewhere else. But setting the same value to a Property is a no-op, so perhaps setting the same value to a DerivedProperty should also be a no-op? I'll commit it and request review.

UPDATE: Also onEnabledChange( enabled ) { seems buggy because it ignores its argument.

@samreid
Copy link
Member

samreid commented Oct 17, 2020

I committed the change to DerivedProperty, @pixelzoom can you please review/discuss/advise?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 18, 2020

@samreid said:

... But setting the same value to a Property is a no-op, so perhaps setting the same value to a DerivedProperty should also be a no-op?

I don't think that should be the case. Any attempt to directly set a DerivedProperty is incorrect.

EDIT: Thinking about this more, I have a strong opinion about phetsims/axon@9b01dfc - it should be reverted.

@samreid
Copy link
Member

samreid commented Oct 18, 2020

Here is a patch that reverts the change to DerivedProperty and moves the guard to RoundButtonView.

Index: main/sun/js/buttons/RoundButtonView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/sun/js/buttons/RoundButtonView.js	(revision a52e5196b2c8a3c02064b005f6142e9b294af415)
+++ main/sun/js/buttons/RoundButtonView.js	(date 1602983778136)
@@ -210,7 +210,9 @@
    */
   setEnabled( value ) {
     assert && assert( typeof value === 'boolean', 'RoundButtonView.enabled must be a boolean value' );
-    this.buttonModel.enabledProperty.set( value );
+    if ( !this.buttonModel.enabledProperty.equalsValue( value ) ) {
+      this.buttonModel.enabledProperty.set( value );
+    }
   }
 
   /**
Index: main/axon/js/DerivedProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/axon/js/DerivedProperty.js	(revision 9b01dfccb2456017b771abff767bd3fe2b317e6f)
+++ main/axon/js/DerivedProperty.js	(date 1602983684323)
@@ -118,9 +118,7 @@
    * @public
    */
   set( value ) {
-    if ( !this.equalsValue( value ) ) {
-      throw new Error( 'Cannot set values directly to a DerivedProperty, tried to set: ' + value );
-    }
+    throw new Error( 'Cannot set values directly to a DerivedProperty, tried to set: ' + value );
   }
 
   /**

The problem is that EnabledComponent allows the user to pass in a Property, and some client code passes in a DerivedProperty, which does not support set. Do you prefer this patch? If so, won't we end up proliferating guards like this to other places? This example is in RoundButtonView, but maybe we would need one in RectangularButtonView, and elsewhere--should it be factored out?

@zepumph
Copy link
Member

zepumph commented Oct 19, 2020

I hadn't yet thought about the fact that EnabledComponent.enabledProperty should able to be a passed DerivedProperty. I feel like it is a case we need to support. In this instance, it is RoundArrowButton's fault for not supporting the enabledProperty interface, in which options.enabledProperty is of type Property (which could be derived).

Do you prefer this patch?

I do not think this patch gets to the bottom of things. First ask ourselves what is the enabledProperty logic in RoundButtonView doing? It seems like it is vestigial to a time when we didn't run enabled on a Property, and instead things were based on MutableOptionsNode, see the conversion in phetsims/fractions-common@a203a54.

I would recommend removing this code because sun button supertypes now have this enabled support build-in. I tested this patch by running fractions-equality on the first screen. RoundArrowButtons are the colored spinner buttons for changing the fraction.

Index: js/common/view/RoundArrowButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/RoundArrowButton.js	(revision fe9a2b55924b4fcf9cdd6d9b23ccc8b91dc1a11e)
+++ js/common/view/RoundArrowButton.js	(date 1603126483509)
@@ -45,34 +45,6 @@
     options.yContentOffset = arrowPath.centerY;
 
     super( options );
-
-    // @private {Property.<boolean>}
-    this.enabledProperty = options.enabledProperty;
-
-    // @private {function}
-    this.enabledListener = this.onEnabledChange.bind( this );
-    this.enabledProperty.link( this.enabledListener );
-  }
-
-  /**
-   * Sets whether this is enabled.
-   * @private
-   *
-   * @param {boolean} enabled
-   */
-  onEnabledChange( enabled ) {
-    this.enabled = this.enabledProperty.value;
-  }
-
-  /**
-   * Releases references.
-   * @public
-   * @override
-   */
-  dispose() {
-    this.enabledProperty.unlink( this.enabledListener );
-
-    super.dispose();
   }
 }
 

If so, won't we end up proliferating guards like this to other places?

There are 263 usages of enabled(Property\.value)? = in the project. I looked through maybe 50, and found that most of the cases were changing instances. I feel like we will just need to make sure that all usages of enabledProperty work with the central interface we want (which includes DerivedProperty).

I would recommending making a new issue to apply this patch so that responsible dev can double check on it.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 19, 2020

@zepumph said:

In this instance, it is RoundArrowButton's fault for not supporting the enabledProperty interface, in which options.enabledProperty is of type Property (which could be derived).

I think I understand the spirit of this comment. But to clarify... RoundArrowButton DOES support the Property API (which includes setValue) and cannot possibly forsee the fact that subclasses of Property (like DerivedProperty) may break that contract. The "fault" here is with the design of DerivedProperty - it extends Property, and then immediately breaks the Property contract for 4 methods, including setValue. This is an anti-pattern.

@samreid asked in #257 (comment):

... Do you prefer this patch? If so, won't we end up proliferating guards like this to other places? This example is in RoundButtonView, but maybe we would need one in RectangularButtonView, and elsewhere--should it be factored out?

I don't like this logic being moved to RoundButtonView (or other classes) either. It's a hack to work around a major design flaw in DerivedProperty (see #257 (comment)). I don't know how to correct that design flaw at this point.

Preferable to the hack would be to remove the need to set enabledProperty. That problem is present only because of the first bullet in #515 (comment).

The best practical solution is to examine the clients that are creating these buttons, and ask why they are passing in {DerivedProperty} enabledProperty and then expecting to call button.setEnabled.

I'm also curious... How are you dealing with this for visibleProperty et. al. in phetsims/scenery#1046?

Example:

const visibleProperty = new DerivedProperty(...);
...
const myNode = new Node( {
  visibleProperty: visibleProperty
} );
...
myNode.visible = ...;

@pixelzoom
Copy link
Contributor Author

This issue dealt with how to add enabled/disabled feature to UI component. We decided that mixin was the correct pattern, and @zepumph implemented the EnabledComponent stack. Conversion to EnableComponent has been completed in #585. So this issue is done, and ready to close.

The issue of {DerivedProperty} enabledProperty that @samreid brought up in #257 (comment) is worthy of a separate issue. And I believe that it's not specific to enabledProperty, but relevant for dependency injection of any Property. So I've moved that discussion to #641.

Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants