-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Delete Callback
and use observers to control widgets for 0.17
#21247
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
Conversation
I take it you're interested in shipping this for 0.17 then? |
//! matures, so please exercise caution if you are using this as a reference for your own code, | ||
//! and note that there are still "user experience" issues with this API. | ||
use bevy::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need to rethink this example, but I can do this in follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure theres much to do here outside of adding support for multi-event-observers. Curious to see what you come up with though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My complaints are mostly that this example is very poorly motivated pedagogically, especially if we're using observers for behavior by default :)
I realize that this is very last-minute. However, I really, really dislike the idea of shipping callbacks to our users and then doing an immediate rug-pull on them - "oh, all that callback code you just wrote? You can throw it away now." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not any deep feedback yet, other than:
- I love this at a first glance
- I would encourage taking a look at the prior art of the
observe
function before merging this. Or even better, get a review in from the authors!
|
Callback
and use observers to control widgets for 0.17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me, the API feels more natural with observers.
Co-authored-by: Ben Frankel <[email protected]>
Co-authored-by: Ben Frankel <[email protected]>
Co-authored-by: Ben Frankel <[email protected]>
Co-authored-by: Ben Frankel <[email protected]>
Read code changes and successfully tested widget integration into New approach looks a lot better.
This is a huge improvement, I think this should be released in bevy 0.17 as it is easier to use and sets better example for future UI development. 🎉 I still do not like how Radio widgets and RadioGroup works. It would be better for each widget to have separate events. But I could partially override behavior with my own. RadioButtons and RadioGroup shouldn't make assumptions that there is exactly one active radio button. |
First impressions as I review this:
Deeper thoughts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a solid improvement to ergonomics and complexity, and we should merge it for 0.17. I'm happy with the implementation of this, and I've tested the feathers
example: everything seems to be working as intended.
observe
isn't a perfect solution, but it's much closer to what I think we should be using in the long term and has a clear migration path.
Like I said above, I'll defer the decision on where observe
should live (and how widely we should encourage its use for 0.17).
Co-authored-by: Alice Cecile <[email protected]>
From Alice:
I think this is very important. It has never been clear to me how the design patterns that work on React functions (inputs/props) would work once we replace most of our scene functions with scene assets. I really like the new approach and will like it even more once #17607 is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be unsurprising that I'm on board for this, given my campaign here. Very happy with how this turned out.
I like that the mechanism for controlled vs uncontrolled is simpler and more explicit.
It would be better for each widget to have separate events
Strongly agree here. This was my takeaway as well, and it is the pattern I would like to encourage in general. I don't think we need to do that for 0.17 / in this PR, as we're already getting this in last minute, and this is still "experimental".
I'll defer the decision on where observe should live (and how widely we should encourage its use for 0.17).
I strongly believe that we should not move observe
to bevy_ecs
as it violates the "non-component things should not masquerade as components" principle. I've held that line up until this point, and I will continue to do so. This is what bsn!
is for.
I view including observe
here (hidden behind the experimental flag and scoped to the widgets work) as a targeted "middle ground" compromise in the interest of not hamstringing the UX of this work while we wait for bsn!
.
Removed the `Callback` type. Updated all widgets and examples. Added an `observe` helper which adds an observer via a bundle effect. Note: I would have updated the release notes, but they are gone already. --------- Co-authored-by: Ben Frankel <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Carter Anderson <[email protected]>
Removed the `Callback` type. Updated all widgets and examples. Added an `observe` helper which adds an observer via a bundle effect. Note: I would have updated the release notes, but they are gone already. --------- Co-authored-by: Ben Frankel <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Carter Anderson <[email protected]>
Removed the
Callback
type.Updated all widgets and examples.
Added an
observe
helper which adds an observer via a bundle effect.Note: I would have updated the release notes, but they are gone already.