Theming feedback

[UkoeHB]

I reviewed the WIP theme framework. It's quite impressive what you're building.

A few thoughts:

• ThemeData should probably be ThemeData<C> so each theme can handle its data separately (how does/should this work for layered theming?)

• It’s confusing to have PartialEq for style attributes, since you are testing if the attributes match (not if their values are the same).

• DynamicStyleController should have a .new() so it’s easier to make custom controllers.

• StyleAnimation, LoopedAnimationConfig, AnimationLoop, AnimatedBundle<T> should derive Reflect, Serialize and Deserialize. And the type bound on AnimatedBundle<T> should include Reflect + FromReflect + Serialize + for<'de> Deserialize<'de>.

  • Basically I need these derives on types in order to load them from file: Reflect, Default, Debug, Clone, PartialEq, Serialize, Deserialize. And FromReflect is an extra bound on type parameters because the Reflect derive implements it.

• When a theme is updated, it is not propagated to children. Is that intended? (looking at refresh_theme)

• Possible renames: StyleAnimation -> AnimationSettings, AnimationBundle -> AnimationVals.

[eidloi]

Thanks! To answer your questions:

• ThemeData should probably be ThemeData<C> so each theme can handle its data separately (how does/should this work for layered theming?)

ThemeData isn't generic over C because I planned ThemeData to hold common things, with room for custom properties. Meaning, I don't want to have a primary_color defined for every widget. It might make sense to have an other C specific theme data provided to these callbacks, but there could be widgets that rely on data from another widgets theme. A frequent example is text: Widgets that render text often want to customize some parts of the text style, but take the theme default as base. Also, I am at the point where I start knocking this out, so it might change slightly. The idea should stay, but maybe not the implementation :D

• It’s confusing to have PartialEq for style attributes, since you are testing if the attributes match (not if their values are the same).

I used / abused PartialEq since the attributes, for all intent and purposes logically equal based on what they style. You can never have different types of style applied to the same property. They also completely override each other when the same type is compared. This lets me shorthand all look-ups with .contains() and similar cases wherever equality is tested and I haven't found a scenario (yet) where I would want to compare the contents themselves. That said, I'll try to amend this if possible. There aren't many places this is used to do the matching.

• DynamicStyleController should have a .new() so it’s easier to make custom controllers.

DynamicStyleController was only intended to be used with via the builder as a transparent wrapper around the StyleAnimation but I can see how it could be useful, so will amend.

• Trait bounds: yeah makes sense, the reason I haven't added these yes it because I wanted to minimize the requirements. I'll try to add them, normally bevy already implements the traits on Ts. I'll also probably remove Copy from the bounds, I don't see if I use it anywhere.

• When a theme is updated, it is not propagated to children. Is that intended? (looking at refresh_theme)

refresh_theme applies a theme on a target. If you look at process_theme_update, this is the system that detects changed Theme<C>s and invokes refresh_theme for all entities With<C>. In general, entities look up the chain of themes applied to them towards the root (parent-wise) and apply them in reverse order. A theme at the root will provide style for all C entities, but style can be overridden per attribute down the chain.

• Possible renames: StyleAnimation -> AnimationSettings, AnimationBundle -> AnimationVals.

StyleAnimation isn't just a setting, it also has the implementation for updating, but I might decouple that since most of it is static anyway. As for AnimationVals yeah, I can kinda see how that is a better name for it!

[eidloi]

• And the type bound on AnimatedBundle<T> should include Reflect + FromReflect + Serialize + for<'de> Deserialize<'de>.

This isn't necessary I think. I added the Reflect, Serialize, Deserialize on the bundle and it doesn't complain about the trait bound. We can add it later if necessary, but I think Default is enough for Reflect (or maybe Clone + Default, which is what we have).

[eidloi]

I have addressed point 2, 3, & 4 for now. PartialEq cannot be added to StaticBundle though, since not all bevy types implement PartialEq (i.e. ZIndex & ImageScaleMode)

[UkoeHB]

Thanks for the updates :)

In general, entities look up the chain of themes applied to them towards the root (parent-wise) and apply them in reverse order. A theme at the root will provide style for all C entities, but style can be overridden per attribute down the chain.

Right, but what if a theme at the root changes? The descendants won't detect it. For example, changing the default font size.

[eidloi]

Thanks for the updates :)

In general, entities look up the chain of themes applied to them towards the root (parent-wise) and apply them in reverse order. A theme at the root will provide style for all C entities, but style can be overridden per attribute down the chain.

Right, but what if a theme at the root changes? The descendants won't detect it. For example, changing the default font size.

It doesn't matter where the theme is located in the tree, if a Theme<C> changes, all entities With<C> will re-apply their theme:

    fn process_theme_update(
        q_targets: Query<Entity, With<C>>,
        q_added_targets: Query<Entity, Added<C>>,
        q_removed_themes: RemovedComponents<Theme<C>>,
        q_changed_themes: Query<(Entity, &Theme<C>), Changed<Theme<C>>>,
        theme_data: Res<ThemeData>,
        mut commands: Commands,
    ) {
        if theme_data.is_changed()
            || q_removed_themes.len() > 0
            || q_changed_themes.iter().count() > 0
        {
            for entity in &q_targets {
                commands.entity(entity).refresh_theme::<C>();
            }
        } else {
            for entity in &q_added_targets {
                commands.entity(entity).refresh_theme::<C>();
            }
        }
    }

[UkoeHB]

Ohh I didn't catch that, thanks :)

[eidloi]

I renamed StyleAnimation -> AnimationSettings, AnimattedBundle -> AnimatedVals and StaticBundle -> StaticVals.