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

Convert simulations to use EnabledProperty #340

Closed
samreid opened this issue Oct 31, 2020 · 5 comments
Closed

Convert simulations to use EnabledProperty #340

samreid opened this issue Oct 31, 2020 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Oct 31, 2020

In phetsims/scenery#1100 @zepumph introduced EnabledProperty and it is working nicely. There are 16 occurrences of enabledProperty = new BooleanProperty and another 9 occurrences of enabledProperty = new Property. For instance:

Masses and Springs:

    // Property that toggles whether the gravity and spring force checkboxes are enabled
    const enabledProperty = new BooleanProperty( model.forcesModeProperty.value === ForcesMode.FORCES, {
      phetioFeatured: true
    } );

Should many or all of these be converted to use EnabledProperty instead? If so, should this be a chip-away for responsible-devs, or something @zepumph or I should do?

@zepumph
Copy link
Member

zepumph commented Nov 2, 2020

I feel like this is not really necessary. I don't feel like it is buying us enough to warrant going back to convert old usages. A great time to do this would be when outfitting a sim with PhET-iO, since having a consistent phet-io API for enabledProperty is valuable.

Part of my recommendation is not believing that EnabledProperty is the end-all pattern that is here to stay for good.. Up to you!

@zepumph zepumph assigned samreid and unassigned zepumph Nov 2, 2020
@samreid
Copy link
Member Author

samreid commented Nov 2, 2020

Thanks for your comments. I'm inclined to add a step to the instrumentation guide to consider this, but one distinction is that EnabledProperty is by default phet-io featured. @arouinfar do you think than many of the instrumented enabledProperty instances (predominantly in models) should be featured? If so, that is a good argument for moving these to use EnabledProperty. Also, can you think of any of these that would need a name other than enabledProperty?

@samreid samreid assigned arouinfar and unassigned samreid Nov 2, 2020
@zepumph
Copy link
Member

zepumph commented Nov 12, 2020

bringing to phet-io meeting to move this along.

@zepumph
Copy link
Member

zepumph commented Nov 12, 2020

This type does three things:

  1. default documentation
  2. default phetioFeatured: true
  3. Assert that the tandem name is enabledProperty. Not changeable.

Designers during today's meeting felt like the above was appropriate. We can convert simulations now.

Likely we will do this lazily. @samreid can you update the code review documentation to mention that this should be used when appropriate?

@samreid
Copy link
Member Author

samreid commented Nov 16, 2020

I added a code review checklist item about using EnabledProperty. I also decided to add an "opt-out" option for the strict name check--that seemed better than writing that in an issue or checklist. @zepumph can you please review? It should be quick.

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

3 participants