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 instrument vectorVisibilityProperties #224

Closed
zepumph opened this issue Feb 27, 2020 · 6 comments
Closed

How to instrument vectorVisibilityProperties #224

zepumph opened this issue Feb 27, 2020 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

From #219, @arouinfar had the review comment like:

It's not really clear why projectileMotion.introScreen.view.vectorVisibilityProperties is in the view. This seems analogous to gravityAndOrbits.modelScreen.model.showGravityForceProperty (both toggle the visibility of a vector in the play area), which is in the model.

I see. I also see graphingQuadratics.exploreScreen.view.viewProperties, which makes me feel like it is a bit better to just go with how the sim has implemented these. I can see them fitting in either, if there is a strong preference, then we can probably refactor, but to be honest I would prefer to see them in the view. They have nothing to do with the projectiles' model, and instead only with how information is displayed (hence the view). This is not to say that the GAO instrumentation is incorrect, but perhaps the decision in GAO would be different if there were multiple (4 in PM and 7 in GQ).

What do you think @arouinfar @kathy-phet?

@zepumph
Copy link
Member Author

zepumph commented Feb 27, 2020

While we are at it over here, we should discuss the name too, from this comment in #219:

The "VisibilityProperties" ending to projectileMotion.introScreen.view.vectorVisibilityProperties seems pretty close to visibleProperty, which is a bit of a loaded meaning. Perhaps we should name it something different? @kathy-phet can't remember what we decided for this.

I personally think that these Properties are in fact controlling the visibility of items, and items that aren't instrumented, and so don't have their own visibleProperty (which I think is the right thing to do). Thus to me the name makes sense. What might you prefer instead?

@arouinfar
Copy link
Contributor

@zepumph thanks for the explanation! I can see how the vectorVisibilityProperties makes more sense in the view. Since our initial review we've also seen other sims with similar view properties. I'm happy to leave things the way they are. I'll leave this open so I can double check with @kathy-phet the next time we review this sim.

@arouinfar
Copy link
Contributor

Reviewed in #244

@zepumph several sims collect view properties in a container called viewProperties, and we'd like to do the same here. Can you rename vectorVisbilityProperties to viewProperties? This would apply to all relevant screens.

@zepumph
Copy link
Member Author

zepumph commented May 10, 2021

This issue should be reviewed at the next time Projectile Motion is worked on.

@zepumph
Copy link
Member Author

zepumph commented Jul 26, 2021

Alright, classes and tandems have been renamed to viewProperties. We now have:

projectileMotion.introScreen.view.viewProperties.
projectileMotion.vectorsScreen.view.viewProperties.
projectileMotion.dragScreen.view.viewProperties.

Anything else here @arouinfar?

@arouinfar
Copy link
Contributor

Looks good, thanks!

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

2 participants