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

Should pointOnParabolaProperty be read-only? #195

Closed
arouinfar opened this issue Jun 20, 2023 · 4 comments
Closed

Should pointOnParabolaProperty be read-only? #195

arouinfar opened this issue Jun 20, 2023 · 4 comments

Comments

@arouinfar
Copy link

For #180

The Focus & Directrix screen has manipulators for the vertex, focus, and a movable point along the parabola. The Point on Parobla has a settable pointOnParabolaProperty, but there is no way to enforce that the point remains on the parabola.

image

I'm not sure how useful it is for the client to have API control of pointOnParabolaProperty. However, I think it would also be a bit of a nasty surprise to make this read-only if a client has been relying on it. We allow API control of things like positionProperty even though we can't validate the coordinates. There, we instruct clients to predetermine coordinates in Studio and use 'Get Command' for later use in their wrapper.

I see two options:

(1) Make pointOnParabolaProperty read-only.
(2) Leave it alone and document in examples.md.

I'm not sure which is preferable. If this was for 1.0, I would go with (1) but I'm leaning towards (2) due to migration concerns. @pixelzoom thoughts?

@pixelzoom
Copy link
Contributor

pointOnParabolaProperty is not something that the client should be setting, so I think it should be read-only. Let's discuss via Zoom.

@pixelzoom
Copy link
Contributor

@arouinfar and I discussed. pointOnParabolaProperty should be read-only. I'll make it so, and verify whether a migration rule is needed.

@pixelzoom
Copy link
Contributor

Done in the above commit. This is an API change, but no migration rule appears to be required (according to the Migration wrapper). @arouinfar please review, close if OK.

@pixelzoom pixelzoom removed their assignment Jun 21, 2023
@arouinfar
Copy link
Author

Looks good, thanks @pixelzoom. Closing.

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