Improve and Fix Incorrect Color Parsing

doc/terminal-survey.md

@@ -37,11 +37,15 @@ A list of terminals that were tested for support of DA1 (`CSI c`) and `OSC 10` /
 
 <br>
 
-**ℹ️ Note:**
+**ℹ️ Note 1:**
 Some Linux terminals are omitted since they all use the `vte` library behind the scenes. \
 Here's a non-exhaustive list: GNOME Terminal, (GNOME) Console, MATE Terminal, XFCE Terminal, (GNOME) Builder, (elementary) Terminal, LXTerminal, Guake.
 
-[^1]: The response does not use the `XParseColor` format but rather a CSS-like hex code (e.g. `#AAAAAA`).
+**ℹ️ Note 2:**
+If not otherwise noted, the terminals respond using the `rgb:r(rrr)/g(ggg)/b(bbbb)` format.
+See [Color Strings](https://www.x.org/releases/current/doc/libX11/libX11/libX11.html#Color_Strings) for details on what is theoretically possible.
+
+[^1]: Responds using the `#r(rrr)g(ggg)b(bbb)` format.
 
 [Contour]: https://contour-terminal.org/
 [QTerminal]: https://github.com/lxqt/qterminal

src/color.rs

@@ -46,47 +46,6 @@ impl From<rgb::RGB16> for Color {
     }
 }
 
-impl Color {
-    /// Parses an X11 color (see `man xparsecolor`).
-    #[cfg(unix)]
-    pub(crate) fn parse_x11(input: &str) -> Option<Self> {
-        let raw_parts = input.strip_prefix("rgb:")?;
-        let mut parts = raw_parts.split('/');
-        let r = parse_channel(parts.next()?)?;
-        let g = parse_channel(parts.next()?)?;
-        let b = parse_channel(parts.next()?)?;
-        Some(Color { r, g, b })
-    }
-
-    // Some terminals (only Terminology found so far) respond with a
-    // CSS-like hex color code.
-    #[cfg(unix)]
-    pub(crate) fn parse_css_like(input: &str) -> Option<Self> {
-        let raw_parts = input.strip_prefix('#')?;
-        let len = raw_parts.len();
-        if len == 6 {
-            let r = parse_channel(&raw_parts[..2])?;
-            let g = parse_channel(&raw_parts[2..4])?;
-            let b = parse_channel(&raw_parts[4..])?;
-            Some(Color { r, g, b })
-        } else {
-            None
-        }
-    }
-}
-
-#[cfg(unix)]
-fn parse_channel(input: &str) -> Option<u16> {
-    let len = input.len();
-    // From the xparsecolor man page:
-    //   h indicates the value scaled in 4 bits,
-    //   hh the value scaled in 8 bits,
-    //   hhh the value scaled in 12 bits, and
-    //   hhhh the value scaled in 16 bits, respectively.
-    let shift = (1..=4).contains(&len).then_some(16 - 4 * len as u16)?;
-    Some(u16::from_str_radix(input, 16).ok()? << shift)
-}
-
 // Implementation of determining the perceived lightness
 // follows this excellent answer: https://stackoverflow.com/a/56678483
 
@@ -116,7 +75,6 @@ fn luminance_to_perceived_lightness(luminance: f64) -> u8 {
 }
 
 #[cfg(test)]
-#[allow(clippy::unwrap_used)]
 mod tests {
     use super::*;
 
@@ -135,17 +93,4 @@ mod tests {
         };
         assert_eq!(100, white.perceived_lightness())
     }
-
-    #[test]
-    #[cfg(unix)]
-    fn parses_css_like_color() {
-        assert_eq!(
-            Color {
-                r: 171 << 8,
-                g: 205 << 8,
-                b: 239 << 8
-            },
-            Color::parse_css_like("#ABCDEF").unwrap()
-        )
-    }
 }

src/lib.rs

@@ -88,6 +88,8 @@ use thiserror::Error;
 
 mod color;
 mod os;
+#[cfg(unix)]
+mod xparsecolor;
 
 #[cfg(unix)]
 mod xterm;

src/xparsecolor.rs

@@ -0,0 +1,193 @@
+use crate::Color;
+
+/// Parses a color value that follows the `XParseColor` format.
+/// See https://www.x.org/releases/current/doc/libX11/libX11/libX11.html#Color_Strings
+/// for a reference of what `XParseColor` supports.
+///
+/// Not all formats are supported, just the ones that are returned
+/// by the tested terminals. Feel free to open a PR if you encounter
+/// a terminal that returns a different format.
+pub(crate) fn xparsecolor(input: &str) -> Option<Color> {
+    if let Some(stripped) = input.strip_prefix('#') {
+        parse_sharp(stripped)
+    } else if let Some(stripped) = input.strip_prefix("rgb:") {
+        parse_rgb(stripped)
+    } else {
+        None
+    }
+}
+
+/// From the `xparsecolor` man page:
+/// > For backward compatibility, an older syntax for RGB Device is supported,
+/// > but its continued use is not encouraged. The syntax is an initial sharp sign character
+/// > followed by a numeric specification, in one of the following formats:
+/// >
+/// > The R, G, and B represent single hexadecimal digits.
+/// > When fewer than 16 bits each are specified, they represent the most significant bits of the value
+/// > (unlike the `rgb:` syntax, in which values are scaled).
+/// > For example, the string `#3a7` is the same as `#3000a0007000`.
+fn parse_sharp(input: &str) -> Option<Color> {
+    const NUM_COMPONENTS: usize = 3;
+    let len = input.len();
+    if len % NUM_COMPONENTS == 0 && len <= NUM_COMPONENTS * 4 {
+        let chunk_size = input.len() / NUM_COMPONENTS;
+        let r = parse_channel_shifted(&input[0..chunk_size])?;
+        let g = parse_channel_shifted(&input[chunk_size..chunk_size * 2])?;
+        let b = parse_channel_shifted(&input[chunk_size * 2..])?;
+        Some(Color { r, g, b })
+    } else {
+        None
+    }
+}
+
+fn parse_channel_shifted(input: &str) -> Option<u16> {
+    let value = u16::from_str_radix(input, 16).ok()?;
+    Some(value << ((4 - input.len()) * 4))
+}
+
+/// From the `xparsecolor` man page:
+/// > An RGB Device specification is identified by the prefix `rgb:` and conforms to the following syntax:
+/// > ```text
+/// > rgb:<red>/<green>/<blue>
+/// >
+/// >     <red>, <green>, <blue> := h | hh | hhh | hhhh
+/// >     h := single hexadecimal digits (case insignificant)
+/// > ```
+/// > Note that *h* indicates the value scaled in 4 bits,
+/// > *hh* the value scaled in 8 bits, *hhh* the value scaled in 12 bits,
+/// > and *hhhh* the value scaled in 16 bits, respectively.
+fn parse_rgb(input: &str) -> Option<Color> {
+    let mut parts = input.split('/');
+    let r = parse_channel_scaled(parts.next()?)?;
+    let g = parse_channel_scaled(parts.next()?)?;
+    let b = parse_channel_scaled(parts.next()?)?;
+    if parts.next().is_none() {
+        Some(Color { r, g, b })
+    } else {
+        None
+    }
+}
+
+fn parse_channel_scaled(input: &str) -> Option<u16> {
+    let len = input.len();
+    if (1..=4).contains(&len) {
+        let max = u32::pow(16, len as u32) - 1;
+        let value = u32::from_str_radix(input, 16).ok()?;
+        Some((u16::MAX as u32 * value / max) as u16)
+    } else {
+        None
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // Tests adapted from alacritty/vte:
+    // https://github.com/alacritty/vte/blob/ed51aa19b7ad060f62a75ec55ebb802ced850b1a/src/ansi.rs#L2134
+    #[test]
+    fn parses_valid_rgb_color() {
+        assert_eq!(
+            xparsecolor("rgb:f/e/d"),
+            Some(Color {
+                r: 0xffff,
+                g: 0xeeee,
+                b: 0xdddd,
+            })
+        );
+        assert_eq!(
+            xparsecolor("rgb:11/aa/ff"),
+            Some(Color {
+                r: 0x1111,
+                g: 0xaaaa,
+                b: 0xffff
+            })
+        );
+        assert_eq!(
+            xparsecolor("rgb:f/ed1/cb23"),
+            Some(Color {
+                r: 0xffff,
+                g: 0xed1d,
+                b: 0xcb23,
+            })
+        );
+        assert_eq!(
+            xparsecolor("rgb:ffff/0/0"),
+            Some(Color {
+                r: 0xffff,
+                g: 0x0,
+                b: 0x0
+            })
+        );
+    }
+
+    #[test]
+    fn fails_for_invalid_rgb_color() {
+        assert!(xparsecolor("rgb:").is_none()); // Empty
+        assert!(xparsecolor("rgb:f/f").is_none()); // Not enough channels
+        assert!(xparsecolor("rgb:f/f/f/f").is_none()); // Too many channels
+        assert!(xparsecolor("rgb:f//f").is_none()); // Empty channel
+        assert!(xparsecolor("rgb:ffff/ffff/fffff").is_none()); // Too many digits for one channel
+    }
+
+    // Tests adapted from alacritty/vte:
+    // https://github.com/alacritty/vte/blob/ed51aa19b7ad060f62a75ec55ebb802ced850b1a/src/ansi.rs#L2142
+    #[test]
+    fn parses_valid_sharp_color() {
+        assert_eq!(
+            xparsecolor("#1af"),
+            Some(Color {
+                r: 0x1000,
+                g: 0xa000,
+                b: 0xf000,
+            })
+        );
+        assert_eq!(
+            xparsecolor("#1AF"),
+            Some(Color {
+                r: 0x1000,
+                g: 0xa000,
+                b: 0xf000,
+            })
+        );
+        assert_eq!(
+            xparsecolor("#11aaff"),
+            Some(Color {
+                r: 0x1100,
+                g: 0xaa00,
+                b: 0xff00
+            })
+        );
+        assert_eq!(
+            xparsecolor("#110aa0ff0"),
+            Some(Color {
+                r: 0x1100,
+                g: 0xaa00,
+                b: 0xff00
+            })
+        );
+        assert_eq!(
+            xparsecolor("#1100aa00ff00"),
+            Some(Color {
+                r: 0x1100,
+                g: 0xaa00,
+                b: 0xff00
+            })
+        );
+        assert_eq!(
+            xparsecolor("#123456789ABC"),
+            Some(Color {
+                r: 0x1234,
+                g: 0x5678,
+                b: 0x9ABC
+            })
+        );
+    }
+
+    #[test]
+    fn fails_for_invalid_sharp_color() {
+        assert!(xparsecolor("#").is_none()); // Empty
+        assert!(xparsecolor("#1234").is_none()); // Not divisible by three
+        assert!(xparsecolor("#123456789ABCDEF").is_none()); // Too many components
+    }
+}

src/xterm.rs

@@ -1,4 +1,5 @@
 use self::io_utils::{read_until2, TermReader};
+use crate::xparsecolor::xparsecolor;
 use crate::{Color, ColorScheme, Error, QueryOptions, Result};
 use std::env;
 use std::io::{self, BufRead, BufReader, Write as _};
@@ -74,7 +75,7 @@ fn parse_response(response: String, prefix: &str) -> Result<Color> {
                 .strip_suffix('\x07')
                 .or(response.strip_suffix("\x1b\\"))
         })
-        .and_then(|c| Color::parse_x11(c).or_else(|| Color::parse_css_like(c)))
+        .and_then(xparsecolor)
         .ok_or_else(|| Error::Parse(response))
 }