Pack color name into DynamicColor flags
#39
Conversation
A rough implementation of the ideas in linebender#26 (comment). The ergonomics require some tweaking, and perhaps the performance too, though it should be easy to write this in a form where the compiler can easily optimize the bitwise operations. I haven't verified what the compiler does with the current form. An alternative is to keep `Missing` unchanged, and add something similar to `Flags` as a new field on `DynamicColor`. But adding a new field makes constructing `DynamicColor` a bit less ergonomic.
color/src/serialize.rs
Outdated
| for (specified, expected) in [ | ||
| ("rgb(1,1,1)", "rgb(1, 1, 1)"), | ||
| // TODO: output rounding? Otherwise the tests should check for approximate equality | ||
| // (and not string equality) for these conversion cases | ||
| ( | ||
| "hwb(740deg 20% 30% / 50%)", | ||
| "rgba(178.5, 93.50008, 50.999996, 0.5)", | ||
| ), | ||
| // currently fails, but should succeed (values should be clamped at parse-time) | ||
| // ("rgb(1.1,1,1)", "rgb(1, 1, 1)"), | ||
| ("color(srgb 1.0 1.0 1.0)", "color(srgb 1 1 1)"), | ||
| ("rosybrown", "rosybrown"), | ||
| ("red", "red"), | ||
| ("transparent", "transparent"), | ||
| ("yellowgreen", "yellowgreen"), | ||
| ] { | ||
| let result = format!("{}", parse_color(specified).unwrap()); | ||
| assert_eq!( | ||
| result, | ||
| expected, | ||
| "Failed serializing specified color `{specified}`. Expected: `{expected}`. Got: `{result}`." | ||
| ); |
There was a problem hiding this comment.
I think this is the correct behavior based on https://www.w3.org/TR/css-color-4/#serializing-color-values.
color/src/missing.rs
Outdated
| // indicates whether the color was generated from a named color or named color space function. The | ||
| // remaining bits indicate the named color. | ||
| #[derive(Default, Clone, Copy, PartialEq, Eq, Debug)] | ||
| pub struct Flags(u16); |
There was a problem hiding this comment.
I don't really like packing the missing/named segments together when we still have 8 bits of unused padding. I don't think it's especially obvious that users will be constructing DynamicColor from its raw components.
(This comment is more to start discussion, not to request an immediately actioned change)
There was a problem hiding this comment.
Raph had mentioned (somewhere) that he would rename Missing to Flags or something if he went down this path, I think.
There was a problem hiding this comment.
DynamicColor can fit 24 additional bits (of which Missing currently takes 8). That can be done with an 8-bit type and a 16-bit type. If ergonomics of constructing DynamicColor doesn't matter too much, I think I would also prefer using two fields. Alternatively, Flags could become
struct Flags {
missing: u8,
name: u8,
}(leaving 8 bits unused), where, for example, name == 0 indicates the value is not from a named color or named color space, name == 255 indicates it is from a named color space function, and other values indicate it is from a named color and are the 1-based index into crate::x11_colors::NAMES.
| /// If the color was constructed from a named color, returns that name. | ||
| /// | ||
| /// See also [`parse_color`][crate::parse_color]. | ||
| pub fn color_name(self) -> Option<&'static str> { |
There was a problem hiding this comment.
Can we use this in Debug somewhere?
| pub struct Flags(u16); | ||
| pub struct Flags { | ||
| /// A bitset of missing color components. | ||
| missing: u8, |
There was a problem hiding this comment.
Would it make sense for this to be Missing directly?
A rough implementation of the ideas in #26 (comment).
The ergonomics require some tweaking, and perhaps the performance too, though it should be easy to write this in a form where the compiler can easily optimize the bitwise operations. I haven't verified what the compiler does in the current form.
An alternative is to keep
Missingunchanged, and add something similar toFlagsas a new field onDynamicColor. But adding a new field makes constructingDynamicColora bit less ergonomic.