-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Improve NodeBundle ergonomics #5503
Conversation
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.
These are good changes, I think the only thing is to find a better name for "transparent" as it doesn't capture the fact that is allows passing through input, or at least that is my intuition when I see the name.
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.
👍
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 like this. Approved. Just left a note about a little tweak to the docs.
Co-authored-by: Ida Iyes <[email protected]>
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 like these changes quite a bit, although I think it's a bit odd that there are with_X
methods for style and color, but not the other fields. While I understand that style and color are by far the most common case, I don't like the inconsistency this brings to the API.
There is a second inconsistency introduced, as the following two things are now equivalent:
.spawn_bundle(NodeBundle {
style: Style {
todo!()
},
..NodeBundle::container()
})
.spawn_bundle(NodeBundle::container().with_style(Style {
todo!()
}))
I definitely prefer the latter pattern, but again, I don't like the inconsistency introduced. We should probably pick a convention for this, although I'm willing to merge with it as is.
I also have a few naming quibbles :p.
crates/bevy_ui/src/entity.rs
Outdated
@@ -39,6 +42,38 @@ pub struct NodeBundle { | |||
pub computed_visibility: ComputedVisibility, | |||
} | |||
|
|||
impl NodeBundle { | |||
pub fn new(style: Style, color: impl Into<UiColor>) -> Self { |
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.
While I like this method a lot, I think using the name new
here is confusing. I'm used to new
being the generic constructor, but this constructor is clearly meant as a "simpler, common-case constructor". Maybe a name like basic
would make more sense?
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.
Yeah, I wasn't sure about that one either. It's pretty common in languages with variadics to just have a bunch of new
with different overloads, but in this case the rust way would probably be something like NodeBundle::from_style_and_color()
but that's way too long for something that common. I'm not a big fan of basic
but right now I don't have any better idea. Maybe NodeBundle::styled()
.
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 like NodeBundle::styled()
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.
Me too!
Co-authored-by: Rose Peck <[email protected]>
The main reason I only used |
Ah, that makes sense. If this is how |
I renamed new to styled and added a with_focus_policy for the rare cases where you would want a styled node with pass-through. |
bors try |
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.
Looks good to me. Nice work!
First, the I don't think we should go down the "bundle builder" road without more careful consideration. It complicates things for a number of reasons:
Of course, UI ergonomics and UX are a hot / high priority topic. Builders might be the solution. But given that nobody else has pushed back / tried to have this conversation, I'm going to force it :) Some alternatives to consider (please try to come up with more):
Note that some of these ideas are not mutually exclusive / could be adopted in combination. I also have no strong opinions yet, other than that this is a conversation worth having.
These thoughts apply to that PR as well / I would have left them there if I had been able to review it. |
I have to admit, I was a bit too focused on working around the current status quo than actually make a more deliberate change for the better. I think the biggest issue is that Here's a few changes and new bundles I believe would help alleviate the need for specialized constructor.
These bundles having smart defaults will certainly help a bit, but it still has various issues that also needs to be addressed at some point. Using I believe there are also some issues with introducing even more bundles that contain almost the same things. It will become easier and easier to forget to remove or add a component to each of these bundles whenever we start needing even more components on those bundles. For example, some people are thinking about splitting #[flat_bundle(NodeBundle)]
struct ImageBundle {
image: Image,
alt_text: String
} Another small note, using A few more future possibilities that would make UI code simpler to build:
This got a bit out of scope of the original PR, but as I said at the start, I was admittedly too focused on improving a very limited subset of the overall problem. I have a bunch more issues that makes abstracting bundle construction in user code hard, but this is too out of scope. |
The feedback is extremely valuable and the push-back is valid, so thank you :)
I got to admit I'm guilty of the same. @IceSentry, your ideas of improving the bundle situation is good I think, and we should extract them into an issue for later. I do believe that in order to verify the approach and confirm if this is indeed the best approach we need to step back and get a better understanding of our high level targets and where we want to end up. I've opened the discussion #5604 for this reason and I would love both of your input there. |
I think I'm going to close this out until we come to a consensus on direction. Not trying to shut down the conversation, just trying to filter attention. Feel free to respond here if you want this reopened. |
Objective
NodeBundle
s are useful when working with complex UIs but they are very verbose, especially when you just want to use them as a wrapper element since the defaults don't work well for those cases. There's also a lot of repetitions likestyle: Style
andcolor: Color
.Solution
NodeBundle
to improve ergonomics. This was highly inspired by the recent changes toTextBundle
NodeBundle::new
: most use ofNodeBundle
is done with aStyle
and aColor
we should make this simpler to do.NodeBundle::container
: It's often useful to be able to wrap multiple element under a single node. To do that you always need a node with a transparent color and a focus policy that let's interaction go through it. The default values don't do that in this case so the best way to fix that in a non-breaking way is to have a new constructor.Self::with_style
andSelf::with_color
: This is mostly useful when you spawn a transparent node and you want to cchange a flex property on the style. the with_color is not as useful but I don't think there's any downsides in including it.Changelog
NodeBundle
NodeBundle::new(Style, Color)
,NodeBundle::container()
,NodeBundle::with_style()
andNodeBundle::with_color()