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

InvertedBooleanProperty should not be settable, or should have 2-way binding #111

Closed
samreid opened this issue Jun 3, 2020 · 6 comments
Closed
Assignees

Comments

@samreid
Copy link
Member

samreid commented Jun 3, 2020

During phetsims/wave-interference#426 it was recommended to use InvertedBooleanProperty. I discovered there is a problem that you can set its value independently.

const x = new BooleanProperty(false);
const y = new InvertedBooleanProperty(x);
y.value = false;

Have you considered implementing InvertedBooleanProperty using DynamicProperty, which supports one-way or two-way binding?

Have you considered implementing InvertedBooleanProperty as a DerivedProperty, like:

new DerivedProperty( [ xProperty ], x => !x )
@samreid samreid changed the title InvertedBooleanProperty should not be settable InvertedBooleanProperty should not be settable, or should have 2-way binding Jun 3, 2020
jbphet added a commit that referenced this issue Jun 3, 2020
@jbphet
Copy link
Contributor

jbphet commented Jun 3, 2020

Have you considered implementing InvertedBooleanProperty using DynamicProperty...?
Have you considered implementing InvertedBooleanProperty as a DerivedProperty...?

I hadn't thought about it much at all, I just wanted it and whipped it up quickly. It's a good point that it can be set independently, which is undesirable. I've revised it to use the DerivedProperty approach described above, and this makes it very compact.

Good suggestion @samreid. Good to go? Do we need to think about passing in a tandem or options?

@jbphet jbphet assigned samreid and unassigned jbphet Jun 3, 2020
@samreid
Copy link
Member Author

samreid commented Jun 3, 2020

I recommend passing through options so usage sites can pass options as necessary.

@samreid samreid assigned jbphet and unassigned samreid Jun 3, 2020
jbphet added a commit that referenced this issue Jun 5, 2020
@jbphet
Copy link
Contributor

jbphet commented Jun 5, 2020

Options added. Are we done here?

@jbphet jbphet assigned samreid and unassigned jbphet Jun 5, 2020
samreid added a commit that referenced this issue Jun 8, 2020
@samreid
Copy link
Member Author

samreid commented Jun 8, 2020

The options look great, I just added one improvement to the JSDoc. The last thing is it seems this belongs in axon--nothing about it seems tambo specific (except that tambo may use it).

Finally, I noticed the definitions of DerivedProperty.and and DerivedProperty.or and was wondering if this should be defined as DerivedProperty.not to fit in better.

@jbphet
Copy link
Contributor

jbphet commented Jun 8, 2020

I noticed the definitions of DerivedProperty.and and DerivedProperty.or and was wondering if this should be defined as DerivedProperty.not to fit in better.

That seems great, I didn't realize it was an option. I went with it, please review and close if you're good with it.

@jbphet jbphet assigned samreid and unassigned jbphet Jun 8, 2020
@samreid
Copy link
Member Author

samreid commented Jun 9, 2020

Looks great, thanks! Closing.

@samreid samreid closed this as completed Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants