Serialize color names by packing them into `DynamicColor` by tomcur · Pull Request #39 · linebender/color

tomcur · 2024-11-13T16:11:30Z

To perform correct CSS Color 4 serialization of colors, some details of how a color was declared have to be preserved. Specifically, CSS Color Module Level 4 § 15.2 and the two sections on Lab and Oklab following it specify that

a color declared as a named CSS/X11 color, should serialize to that (normalized) color name; and
a color declared as one of the named color space functions – i.e., not the color() function – should serialize to that function. (Exceptions being hsl() and hwb(), those serialize to rgb().)

This implementation uses the padding currently available on DynamicColor to pack in the color name the DynamicColor was created from, or whether a named color space function was used; an implementation of the ideas in #26 (comment).

DynamicColor now contains Flags, and the existing Missing color components are moved into Flags. In this implementation, only parse_color sets the color name or color space function name.

color/src/serialize.rs

color/src/dynamic.rs

DJMcNab

Looks good! Some more passing thoughts.

DJMcNab · 2024-11-14T08:58:20Z

color/src/missing.rs

+// 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);


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)

Raph had mentioned (somewhere) that he would rename Missing to Flags or something if he went down this path, I think.

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.

DJMcNab · 2024-11-14T08:58:52Z

color/src/missing.rs

+    /// 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> {


Can we use this in Debug somewhere?

As Flags and Missing are opaque, we could Debug format to, for example:

Flags { missing: Missing( 0b00000000, ), name: 33, named: true, color_name: Some( "red", ), }

color/src/serialize.rs

DJMcNab

What's needed to move this out of draft?

color/src/missing.rs

tomcur · 2024-11-16T11:57:15Z

What's needed to move this out of draft?

I think when #39 (comment) is resolved, we can move this out of draft.

tomcur · 2024-11-18T13:53:23Z

color/src/serialize.rs

+        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)",
+            ),
+            // the next two currently fail, but should succeed (ASCII uppercase codepoints should
+            // be lowercased)
+            // ("ReD", "red"),
+            // ("RgB(1,1,1)", "rgb(1, 1, 1)"),
+            // 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)"),
+        ] {


I believe this is the correct serialization as specified by CSS Color 4. To leave case normalization out of the crate, callers wanting case-insensitivity could be instructed to str::make_ascii_lowercase. Otherwise the parser requires some work to get performant insensitive matching.

raphlinus · 2024-11-27T01:55:49Z

I'm having trouble making a go/no-go decision on this. I like the idea of better CSS compatibility, and the implementation seems fine (though I didn't dig deep), but I also like the idea of the code being minimal, and there are applications that will not need this functionality. I know we talked about this in office hours but don't feel consensus emerged.

Does anyone have a strong feeling either way? My gut feeling is that it's easier to add later if there's a strong need than remove, but if it is going in anyway, now is a good time to do it.

tomcur · 2024-12-02T14:41:08Z

I don't have strong feelings about this. The main argument to include it would be that it's not straightforward to implement on top of the library. Users would have to implement their own parsing and create a wrapping type. It certainly grows the lines of code though, but at least has little runtime impact (and no memory impact).

waywardmonkeys · 2024-12-10T07:20:35Z

@tomcur They wouldn't necessarily have to do a wrapping type ...they could just do a very quick linear scan of the colors for a matching one. Sure, it is moderately gross, but it is something they'd opt into.

tomcur · 2024-12-10T09:47:06Z

That is true, but they'd still need custom parsing, unfortunately. The CSS spec says the color parsed from the string red serializes to red, rgb(1, 0, 0) serializes to rgb(1, 0, 0) and color(srgb 1 0 0) serializes to color(srgb 1 0 0). To be able to do that, some information about the source has to be retained, either in DynamicColor, or as part of the return value from parse_color. If this library provides no support for that, it has to be through a custom parser outside of the library.

waywardmonkeys · 2024-12-10T10:59:31Z

Good points. I lean towards this landing for 0.2. @raphlinus ?

raphlinus

I am ok with landing this, it seems like a nice convenience for users, and I'm not too worried about the cost.

nicoburns · 2024-12-11T22:40:42Z

I also like the idea of the code being minimal, and there are applications that will not need this functionality.

The main argument to include it would be that it's not straightforward to implement on top of the library.

This sounds very much like it ought to exist behind a feature flag to me! (but I think the short term it doesn't matter too much - we can always and then feature flag it in the future version).

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. Squashed: hsl() and hwb() serialize to rgb() Naming consistency Check size only during test Test all named colors Mixed case test Separate into two `u8`s consistency Debug impl Clean up Clippy Use Missing directly Reduce api surface Module name Simplify Debug Module header Inline more Docs --------- Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

waywardmonkeys · 2024-12-12T10:12:20Z

color/src/x11_colors.rs

 }

-pub(crate) fn lookup_palette(s: &str) -> Option<[u8; 4]> {
+pub(crate) fn lookup_palette(s: &str) -> Option<usize> {


Could you rename this to lookup_palette_index or similar? Maybe even with css_palette? Or should this not be duplicated and moved to the palette code with a single function definition?

The rename: e227926, 013c90b.

Moving it around is a bit more difficult as the file is auto-generated, but I'm unsure what you mean by duplication of the function.

tomcur · 2024-12-12T10:45:59Z

color/src/flags.rs

+    #[warn(
+        clippy::missing_const_for_fn,
+        reason = "can be made const with MSRV 1.83"
+    )]


I've added warning lints in 70c1983 as suggested in #91, but interestingly Clippy 0.1.83 doesn't warn. I did bump workspace.package.rust-version in Cargo.toml.

Hmm. I guess it doesn't know about the mutability change.

Will be caught in a future release: rust-lang/rust-clippy#13839

tomcur · 2024-12-12T11:43:46Z

Rebasing is done, but I also made some larger changes to color/src/serialize.rs especially. It's along the same lines, but fixes some edge cases that I've now also added as tests. I think another look from someone before merging is a good idea.

DJMcNab

I've not reasoned about this too carefully, but the code quality is excellent

DJMcNab · 2024-12-12T14:47:21Z

color/src/flags.rs

+
+    /// Set the flags to indicate the color was specified as one of the named colors. `name_ix` is
+    /// the index into [`crate::x11_colors::NAMES`].
+    pub(crate) fn set_named_color(&mut self, name_ix: usize) {


This will never be called from user code, right? (I ask because otherwise color_name could panic in release mode)

That's right, only the parser calls this.

DJMcNab · 2024-12-12T14:55:20Z

color/src/parse.rs

-            "oklch" => parser.lch(1.0, 0.004, ColorSpaceTag::Oklch),
-            "hsl" | "hsla" => parser.hsl(),
-            "hwb" => parser.hwb(),
+            "rgb" | "rgba" => parser.rgb().map(set_from_named_color_space),


Not a huge fan of this pattern, but I'm not sure how to avoid it...

You mean the map(set_from_named_color_space)? I agree; I remember I tried some alternatives and that this pattern was the least bad.

DJMcNab · 2024-12-12T14:58:34Z

color/src/serialize.rs

+    #[test]
+    fn roundtrip_named_colors() {
+        for name in crate::x11_colors::NAMES {
+            let result = format!("{}", parse_color(name).unwrap());


Technically all of these tests could share the same allocated String. That probably wouldn't actually be better...

True, though I think the current conciseness is also nice. But if it were thousands of strings...

tomcur mentioned this pull request Nov 13, 2024

Serialization of color names #26

Open

tomcur force-pushed the packed-names branch 2 times, most recently from ac22029 to 9a13385 Compare November 13, 2024 16:23

tomcur commented Nov 13, 2024

View reviewed changes

color/src/serialize.rs Show resolved Hide resolved

DJMcNab reviewed Nov 13, 2024

View reviewed changes

color/src/dynamic.rs Outdated Show resolved Hide resolved

DJMcNab reviewed Nov 14, 2024

View reviewed changes

tomcur mentioned this pull request Nov 14, 2024

Implementation of Rgba8 type #41

Merged

DJMcNab reviewed Nov 15, 2024

View reviewed changes

color/src/missing.rs Outdated Show resolved Hide resolved

tomcur force-pushed the packed-names branch from c296642 to c1d5721 Compare November 16, 2024 10:53

tomcur force-pushed the packed-names branch from 7330285 to da84143 Compare November 18, 2024 10:06

tomcur marked this pull request as ready for review November 18, 2024 10:37

tomcur changed the title ~~Pack color name into DynamicColor flags~~ Serialize color names by packing them into DynamicColor Nov 18, 2024

tomcur commented Nov 18, 2024

View reviewed changes

waywardmonkeys added this to the 0.2.0 milestone Nov 22, 2024

raphlinus approved these changes Dec 11, 2024

View reviewed changes

tomcur force-pushed the packed-names branch from 7fe1040 to ff03335 Compare December 12, 2024 10:04

Serde

3697b73

waywardmonkeys reviewed Dec 12, 2024

View reviewed changes

tomcur added 4 commits December 12, 2024 11:12

Changelog

523874b

Rename x11_colors::lookup_palette, add some docs

e227926

Fix double newline

013c90b

Add warning lints for future-const methods

70c1983

tomcur added 2 commits December 12, 2024 11:35

Make const

8d1a220

Fmt

82c3177

tomcur commented Dec 12, 2024

View reviewed changes

tomcur added 11 commits December 12, 2024 11:53

Replace potentialy flakey test for now

96f98cc

Add more test cases

bc5ec08

Docs

dd040f3

Add more test cases

f469831

Fix serialization of non-parsed DynamicColors, add test cases

a978736

Simplify and add test

86ee20f

Clippy

d18c173

Parsed hex color serialization

b41cc28

The scale is 0-255, not 0-1

641f792

Fix test

3043694

Move compile-time test into tests module

6900a9e

DJMcNab approved these changes Dec 12, 2024

View reviewed changes

tomcur added this pull request to the merge queue Dec 12, 2024

Merged via the queue into linebender:main with commit 6c4419c Dec 12, 2024
15 checks passed

tomcur deleted the packed-names branch December 12, 2024 17:41

Conversation

tomcur commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomcur commented Nov 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphlinus commented Nov 27, 2024

Uh oh!

tomcur commented Dec 2, 2024

Uh oh!

waywardmonkeys commented Dec 10, 2024

Uh oh!

tomcur commented Dec 10, 2024

Uh oh!

waywardmonkeys commented Dec 10, 2024

Uh oh!

raphlinus left a comment

Choose a reason for hiding this comment

Uh oh!

nicoburns commented Dec 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

tomcur commented Nov 13, 2024 •

edited

Loading

tomcur Nov 14, 2024 •

edited

Loading

tomcur Nov 16, 2024 •

edited

Loading

tomcur commented Dec 12, 2024 •

edited

Loading