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

feat: add Style::with() method to overlay one UI Style onto another #12493

Closed
wants to merge 1 commit into from

Conversation

awwsmm
Copy link
Contributor

@awwsmm awwsmm commented Mar 15, 2024

Objective

  • it should be easy to apply multiple Styles to a node (like CSS classes)
  • the fields of an overlaid Style should overwrite the fields of a base Style
  • default (unset) values should not overwrite non-default (set) values

Solution

  • added a new with method to Style to patch one Style onto another

Considerations

  • this solution is not ideal. I think all fields of Style should be Optional, which would better align to CSS, but this is a much bigger change. Right now, default values imply that a value is unset, but that's not necessarily true. Option better aligns with the idea of a set / unset value.
  • surely there is a way to rewrite this using reflection, but I'm not very familiar with that part of Bevy

@awwsmm awwsmm changed the title add Style::with() method to overlay one UI Style onto another feat: add Style::with() method to overlay one UI Style onto another Mar 15, 2024
Comment on lines +496 to +500
display: if patch.display == Self::DEFAULT.display {
self.display
} else {
patch.display
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each field, if the patch value is the default value, ignore it and keep self's value. Otherwise, use the patch value.

@SolarLiner SolarLiner added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets labels Mar 15, 2024
@hymm
Copy link
Contributor

hymm commented Mar 15, 2024

what advantage does this have over somthing like:

*style = Style {
  display: Display::None,
  ..*style
};

@awwsmm
Copy link
Contributor Author

awwsmm commented Mar 15, 2024

what advantage does this have over somthing like:

*style = Style {
  display: Display::None,
  ..*style
};

With this new notation, this is equivalent to

let patch = Style {
  display: Display::None,
  ..default()
};

*style.with(patch);

Consider a larger example, as well

let base = Style {
  position_type: PositionType::Absolute,
  top: Val::Px(0.),
  width: Val::Percent(100.),
  ..default()
};

let class_a = Style {
  align_items: AlignItems::Center,
  border: when_debugging(UiRect::all(Val::Px(1.))),
  ..default()
};

let class_b = Style {
  height: css_variables.header_height,
  border: UiRect::bottom(Val::Px(2.0)),
  padding: css_variables.header_padding,
  ..default()
};

With current notation, applying class_a and class_b on top of base requires explicitly specifying which fields are meant to come from each class

Style {
  align_items: class_a.align_items,
  height: class_b.height,
  border: class_b.border, // careful! this is class_b, not class_a
  padding: class_b.padding,
  ..base
}

This is error-prone (and a bit verbose). With this new method, this can be rewritten as just

base.with(class_a).with(class_b)

...making it easier to define arbitrary Styles and then apply them on top of each other, like adding classes in CSS.

@viridia
Copy link
Contributor

viridia commented Mar 16, 2024

I have done a fair amount experimentation with merging of styles. As you pointed out, not all properties of Style are optional, so merging isn't consistent. Other approaches I have tried:

  1. Maintaining a separate "sparse" representation of styles, which is the approach I used in Quill. For example, a stylesheet could be a vector containing enums representing different style properties. This means if there's only one style property defined, the amount of memory consumed is minimal. These "sparse" styles could then be "compiled" into a "dense" style. This compilation could not only include mutating the Style type, but adding and removing components such as BackgroundColor.

  2. Styles as functions, which is the approach I'm working with in bevy_reactor. A set of styles is a slice of function pointers, each function is called in turn, functions can mutate the Style component, add or remove BackgroundColor, BorderColor and so on. There's a StyleBuilder API which makes these modifications easy, it's fluent Rust syntax but the overall structure is very CSS-like.

@awwsmm
Copy link
Contributor Author

awwsmm commented Mar 16, 2024

In general, I think having a default value mean "unset" is not a great design. That's what Option is for. "Default as unset" makes it very difficult to, for example, fold or reduce over a Vec of Styles (or NodeBundles or ButtonBundles or wherever your styling is). But this is the pattern in Bevy. I'm not sure how to rectify that. It forces introduction of functions like this to work around it.

@awwsmm
Copy link
Contributor Author

awwsmm commented Mar 17, 2024

Closing this PR as it's probably not the best approach, and bikeshedding on this PR is maybe not helpful.

@awwsmm awwsmm closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants