Re: master 64e25cd: More robust NS hex colour string parsing

Re: master 64e25cd: More robust NS hex colour string parsing

[Pip Cet]

On Mon, Jun 8, 2020 at 12:07 PM Mattias Engdegård
<mattiase@savannah.gnu.org> wrote:
> branch: master
> commit 64e25cde324b2e270acf82958abb59018e67f841
> Author: Mattias Engdegård <mattiase@acm.org>
> Commit: Mattias Engdegård <mattiase@acm.org>
>
>     More robust NS hex colour string parsing
>
>     Invalid arguments to color-values, such as "#abcdefg" or "#1234", or
>     valid ones like "#111222333", should not yield nonsense values.
>
>     * src/nsterm.m (ns_get_color):
>     Only accept "#RGB" strings with 1-4 digits per components, equal number
>     of digits each, and no trailing characters.  Parse 12-bit colours
>     correctly.
> ---
>  src/nsterm.m | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 1953138..3dc7e1d 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -2399,20 +2399,23 @@ ns_get_color (const char *name, NSColor **col)
>      scaling = (snprintf (hex, sizeof hex, "%s", name + 4) - 2) / 3;
>    else if (name[0] == '#')        /* An old X11 format; convert to newer */
>      {
> -      int len = (strlen(name) - 1);
> -      int start = (len % 3 == 0) ? 1 : len / 4 + 1;
> -      int i;
> -      scaling = strlen(name+start) / 3;
> -      for (i = 0; i < 3; i++)
> -       sprintf (hex + i * (scaling + 1), "%.*s/", scaling,
> -                name + start + i * scaling);
> -      hex[3 * (scaling + 1) - 1] = '\0';
> +      int len = 0;
> +      while (isxdigit (name[len + 1]))
> +        len++;
> +      if (name[len + 1] == '\0' && len >= 1 && len <= 12 && len % 3 == 0)
> +        {
> +          scaling = len / 3;
> +          for (int i = 0; i < 3; i++)
> +            sprintf (hex + i * (scaling + 1), "%.*s/", scaling,
> +                     name + 1 + i * scaling);
> +          hex[3 * (scaling + 1) - 1] = '\0';
> +        }
>      }

I believe there's very similar code for the X case, where we also
translate #fff to rgb:f/f/f. That code has annoyed me lately by
producing nonsensical GCC warnings, maybe you'd like to replace it
with yours? Having two different versions of this seems superfluous
(I'm not doubting the correctness of your version, which I guess means
I'm doubting that of mine).

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

8 juni 2020 kl. 14.26 skrev Pip Cet <pipcet@gmail.com>:

> I believe there's very similar code for the X case, where we also
> translate #fff to rgb:f/f/f. That code has annoyed me lately by
> producing nonsensical GCC warnings, maybe you'd like to replace it
> with yours? Having two different versions of this seems superfluous

Thought crossed my mind too. I'm not sure if it makes sense to share the code that transforms #RRGGBB to rgb:RR/GG/BB; it's not a very natural abstraction, and just why are we doing this string transformation step in the first place.

The X11 backend does it so that it can use Xlib for the rest, which is a dubious saving since the string-munging code isn't much simpler than what would be needed to parse the original string to RGB values right away.

The Windows code looks more robust, except that it appears to apply a funny numeric conversion for 4, 12 and 16 bits/colour. It doesn't look quite right at all.

We should probably have a single, correct parser for the numeric colour strings #RGB, rgb:R/G/B, and rgbi:R/G/B instead of three subtly broken ones. X11 and NS additionally have their own formats that I suppose we need to retain for compatibility.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

8 juni 2020 kl. 14.26 skrev Pip Cet <pipcet@gmail.com>:

> Having two different versions of this seems superfluous

We had four versions. Here is a patch replacing them all.
Only really tested for NS; someone needs to check the X and Windows backends.


[-- Attachment #2: 0001-Consolidate-RGB-string-parsers.patch --]
[-- Type: application/octet-stream, Size: 20040 bytes --]

From b18f1384091b03af8ce5c7cd96d6a78919303036 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 12 Jun 2020 18:12:37 +0200
Subject: [PATCH] Consolidate #RGB string parsers

Use a single parser of colour strings in the #RGB, rgb:R/G/B and
rgbi:R/G/B formats, replacing four existing ones.  Previously,
error-checking was spotty, handling of the rgbi: format not always
present, and normalisation of the result was sometimes incorrect.

* src/dispextern.h: New prototype.
* src/xfaces.c (parse_hex_comp, parse_float_comp, parse_color_spec)
(Fcolor_values_from_numeric_string): New functions.
* test/src/xfaces-tests.el (xfaces-color-values-from-numeric-string):
New test.
* lisp/term/tty-colors.el (tty-color-standard-values):
Use color-values-from-numeric-string, replacing old parser.
* src/nsterm.m (ns_get_color):
* src/w32fns.c (x_to_w32_color):
* src/xterm.c (x_parse_color): Use parse_color_spec, replacing old
parsers.
---
 lisp/term/tty-colors.el  |  58 +--------------
 src/dispextern.h         |   2 +
 src/nsterm.m             |  59 +++++----------
 src/w32fns.c             | 157 ++-------------------------------------
 src/xfaces.c             | 112 ++++++++++++++++++++++++++++
 src/xterm.c              |  49 ++++--------
 test/src/xfaces-tests.el |  23 ++++++
 7 files changed, 176 insertions(+), 284 deletions(-)

diff --git a/lisp/term/tty-colors.el b/lisp/term/tty-colors.el
index 39ca2d3627..d50509e105 100644
--- a/lisp/term/tty-colors.el
+++ b/lisp/term/tty-colors.el
@@ -923,62 +923,8 @@ tty-color-standard-values
 COLOR (see the info node `(emacs) Colors'), regardless of whether
 the terminal can display it, so the return value should be the
 same regardless of what display is being used."
-  (let ((len (length color)))
-    (cond ((and (>= len 4) ;; HTML/CSS/SVG-style "#XXYYZZ" color spec
-		(eq (aref color 0) ?#)
-		(member (aref color 1)
-			'(?0 ?1 ?2 ?3 ?4 ?5 ?6 ?7 ?8 ?9
-			     ?a ?b ?c ?d ?e ?f
-                             ?A ?B ?C ?D ?E ?F)))
-	   ;; Translate the string "#XXYYZZ" into a list of numbers
-	   ;; (XX YY ZZ), scaling each to the {0..65535} range.  This
-	   ;; follows the HTML color convention, where both "#fff" and
-	   ;; "#ffffff" represent the same color, white.
-	   (let* ((ndig (/ (- len 1) 3))
-		  (maxval (1- (ash 1 (* 4 ndig))))
-		  (i1 1)
-		  (i2 (+ i1 ndig))
-		  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-	     (list
-	      (/ (* (string-to-number
-		     (substring color i1 i2) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i2 i3) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i3 i4) 16)
-		    65535)
-		 maxval))))
-	  ((and (>= len 9) ;; X-style rgb:xx/yy/zz color spec
-		(string= (substring color 0 4) "rgb:"))
-	   ;; Translate the string "rgb:XX/YY/ZZ" into a list of
-	   ;; numbers (XX YY ZZ), scaling each to the {0..65535}
-	   ;; range.  "rgb:F/F/F" is white.
-	   (let* ((ndig (/ (- len 3) 3))
-		  (maxval (1- (ash 1 (* 4 (- ndig 1)))))
-		  (i1 4)
-		  (i2 (+ i1 ndig))
-		  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-	     (list
-	      (/ (* (string-to-number
-		     (substring color i1 (- i2 1)) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i2 (- i3 1)) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i3 (1- i4)) 16)
-		    65535)
-		 maxval))))
-	  (t
-	   (cdr (assoc color color-name-rgb-alist))))))
+  (or (color-values-from-numeric-string color)
+      (cdr (assoc color color-name-rgb-alist))))
 
 (defun tty-color-translate (color &optional frame)
   "Given a color COLOR, return the index of the corresponding TTY color.
diff --git a/src/dispextern.h b/src/dispextern.h
index 0b1f3d14ae..e1d6eddc41 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3514,6 +3514,8 @@ #define RGB_PIXEL_COLOR COLORREF
                                        Lisp_Object);
 extern bool tty_defined_color (struct frame *, const char *, Emacs_Color *,
                                bool, bool);
+bool parse_color_spec (const char *,
+                       unsigned short *, unsigned short *, unsigned short *);
 
 Lisp_Object tty_color_name (struct frame *, int);
 void clear_face_cache (bool);
diff --git a/src/nsterm.m b/src/nsterm.m
index 3dc7e1db7c..0e405fc017 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2341,9 +2341,6 @@ so some key presses (TAB) are swallowed by the system.  */
    See https://lists.gnu.org/r/emacs-devel/2009-07/msg01203.html.  */
 {
   NSColor *new = nil;
-  static char hex[20];
-  int scaling = 0;
-  float r = -1.0, g, b;
   NSString *nsname = [NSString stringWithUTF8String: name];
 
   NSTRACE ("ns_get_color(%s, **)", name);
@@ -2386,51 +2383,31 @@ so some key presses (TAB) are swallowed by the system.  */
     }
 
   /* First, check for some sort of numeric specification.  */
-  hex[0] = '\0';
-
-  if (name[0] == '0' || name[0] == '1' || name[0] == '.')  /* RGB decimal */
+  unsigned short r16, g16, b16;
+  if (parse_color_spec (name, &r16, &g16, &b16))
     {
-      NSScanner *scanner = [NSScanner scannerWithString: nsname];
-      [scanner scanFloat: &r];
-      [scanner scanFloat: &g];
-      [scanner scanFloat: &b];
-    }
-  else if (!strncmp(name, "rgb:", 4))  /* A newer X11 format -- rgb:r/g/b */
-    scaling = (snprintf (hex, sizeof hex, "%s", name + 4) - 2) / 3;
-  else if (name[0] == '#')        /* An old X11 format; convert to newer */
-    {
-      int len = 0;
-      while (isxdigit (name[len + 1]))
-        len++;
-      if (name[len + 1] == '\0' && len >= 1 && len <= 12 && len % 3 == 0)
-        {
-          scaling = len / 3;
-          for (int i = 0; i < 3; i++)
-            sprintf (hex + i * (scaling + 1), "%.*s/", scaling,
-                     name + 1 + i * scaling);
-          hex[3 * (scaling + 1) - 1] = '\0';
-        }
+      *col = [NSColor colorForEmacsRed: r16 / 65535.0
+                                 green: g16 / 65535.0
+                                  blue: b16 / 65535.0
+                                 alpha: 1.0];
+      unblock_input ();
+      return 0;
     }
-
-  if (hex[0])
+  else if (name[0] == '0' || name[0] == '1' || name[0] == '.')
     {
-      unsigned int rr, gg, bb;
-      float fscale = (1 << (scaling * 4)) - 1;
-      if (sscanf (hex, "%x/%x/%x", &rr, &gg, &bb))
+      /* RGB decimal */
+      NSScanner *scanner = [NSScanner scannerWithString: nsname];
+      float r, g, b;
+      if (   [scanner scanFloat: &r] && r >= 0 && r <= 1
+          && [scanner scanFloat: &g] && g >= 0 && g <= 1
+          && [scanner scanFloat: &b] && b >= 0 && b <= 1)
         {
-          r = rr / fscale;
-          g = gg / fscale;
-          b = bb / fscale;
+          *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
+          unblock_input ();
+          return 0;
         }
     }
 
-  if (r >= 0.0F)
-    {
-      *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
-      unblock_input ();
-      return 0;
-    }
-
   /* Otherwise, color is expected to be from a list */
   {
     NSEnumerator *lenum, *cenum;
diff --git a/src/w32fns.c b/src/w32fns.c
index e595b0285a..ab864332e7 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -864,161 +864,14 @@ x_to_w32_color (const char * colorname)
 
   block_input ();
 
-  if (colorname[0] == '#')
+  unsigned short r, g, b;
+  if (parse_color_spec (colorname, &r, &g, &b))
     {
-      /* Could be an old-style RGB Device specification.  */
-      int size = strlen (colorname + 1);
-      char *color = alloca (size + 1);
-
-      strcpy (color, colorname + 1);
-      if (size == 3 || size == 6 || size == 9 || size == 12)
-	{
-	  UINT colorval;
-	  int i, pos;
-	  pos = 0;
-	  size /= 3;
-	  colorval = 0;
-
-	  for (i = 0; i < 3; i++)
-	    {
-	      char *end;
-	      char t;
-	      unsigned long value;
-
-	      /* The check for 'x' in the following conditional takes into
-		 account the fact that strtol allows a "0x" in front of
-		 our numbers, and we don't.  */
-	      if (!isxdigit (color[0]) || color[1] == 'x')
-		break;
-	      t = color[size];
-	      color[size] = '\0';
-	      value = strtoul (color, &end, 16);
-	      color[size] = t;
-	      if (errno == ERANGE || end - color != size)
-		break;
-	      switch (size)
-		{
-		case 1:
-		  value = value * 0x10;
-		  break;
-		case 2:
-		  break;
-		case 3:
-		  value /= 0x10;
-		  break;
-		case 4:
-		  value /= 0x100;
-		  break;
-		}
-	      colorval |= (value << pos);
-	      pos += 0x8;
-	      if (i == 2)
-		{
-		  unblock_input ();
-		  XSETINT (ret, colorval);
-		  return ret;
-		}
-	      color = end;
-	    }
-	}
-    }
-  else if (strnicmp (colorname, "rgb:", 4) == 0)
-    {
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 4;
-      for (i = 0; i < 3; i++)
-	{
-	  char *end;
-	  unsigned long value;
-
-	  /* The check for 'x' in the following conditional takes into
-	     account the fact that strtol allows a "0x" in front of
-	     our numbers, and we don't.  */
-	  if (!isxdigit (color[0]) || color[1] == 'x')
-	    break;
-	  value = strtoul (color, &end, 16);
-	  if (errno == ERANGE)
-	    break;
-	  switch (end - color)
-	    {
-	    case 1:
-	      value = value * 0x10 + value;
-	      break;
-	    case 2:
-	      break;
-	    case 3:
-	      value /= 0x10;
-	      break;
-	    case 4:
-	      value /= 0x100;
-	      break;
-	    default:
-	      value = ULONG_MAX;
-	    }
-	  if (value == ULONG_MAX)
-	    break;
-	  colorval |= (value << pos);
-	  pos += 0x8;
-	  if (i == 2)
-	    {
-	      if (*end != '\0')
-		break;
-	      unblock_input ();
-	      XSETINT (ret, colorval);
-	      return ret;
-	    }
-	  if (*end != '/')
-	    break;
-	  color = end + 1;
-	}
+      unblock_input ();
+      /* Throw away the low 8 bits and return 0xBBGGRR.  */
+      return make_fixnum ((b & 0xff00) << 8 | (g & 0xff00) | r >> 8);
     }
-  else if (strnicmp (colorname, "rgbi:", 5) == 0)
-    {
-      /* This is an RGB Intensity specification.  */
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 5;
-      for (i = 0; i < 3; i++)
-	{
-	  char *end;
-	  double value;
-	  UINT val;
 
-	  value = strtod (color, &end);
-	  if (errno == ERANGE)
-	    break;
-	  if (value < 0.0 || value > 1.0)
-	    break;
-	  val = (UINT)(0x100 * value);
-	  /* We used 0x100 instead of 0xFF to give a continuous
-	     range between 0.0 and 1.0 inclusive.  The next statement
-	     fixes the 1.0 case.  */
-	  if (val == 0x100)
-	    val = 0xFF;
-	  colorval |= (val << pos);
-	  pos += 0x8;
-	  if (i == 2)
-	    {
-	      if (*end != '\0')
-		break;
-	      unblock_input ();
-	      XSETINT (ret, colorval);
-	      return ret;
-	    }
-	  if (*end != '/')
-	    break;
-	  color = end + 1;
-	}
-    }
   /* I am not going to attempt to handle any of the CIE color schemes
      or TekHVC, since I don't know the algorithms for conversion to
      RGB.  */
diff --git a/src/xfaces.c b/src/xfaces.c
index cf155288bd..471c49ae52 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -220,6 +220,7 @@ Copyright (C) 1993-1994, 1998-2020 Free Software Foundation, Inc.
 #include "sysstdio.h"
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <math.h>
 
 #include "lisp.h"
 #include "character.h"
@@ -819,6 +820,116 @@ load_pixmap (struct frame *f, Lisp_Object name)
                             Color Handling
  ***********************************************************************/
 
+/* Parse fractional hex digits at S ending right before E.
+   Set *DST to the value normalised to 65535 and return true on success,
+   false otherwise.  */
+static bool
+parse_hex_comp (const char *s, const char *e, unsigned short *dst)
+{
+  int n = e - s;
+  if (n <= 0 || n > 4)
+    return false;
+  int val = 0;
+  for (; s < e; s++)
+    {
+      int digit;
+      if (*s >= '0' && *s <= '9')
+        digit = *s - '0';
+      else if (*s >= 'A' && *s <= 'F')
+        digit = *s - 'A' + 10;
+      else if (*s >= 'a' && *s <= 'f')
+        digit = *s - 'a' + 10;
+      else
+        return false;
+      val = (val << 4) | digit;
+    }
+  int maxval = (1 << (n * 4)) - 1;
+  *dst = (unsigned)val * 65535 / maxval;
+  return true;
+}
+
+/* Parse floating-point number at S ending right before E.
+   Return the number if in the range [0,1]; otherwise -1.  */
+static double
+parse_float_comp (const char *s, const char *e)
+{
+  char *end;
+  double x = strtod (s, &end);
+  return (end == e && x >= 0 && x <= 1) ? x : -1;
+}
+
+/* Parse S as a numeric colour specification and set *R, *G and *B.
+   Return true on success, false on failure.
+   Recognised formats:
+
+    "#RGB", with R, G and B hex strings of equal length, 1-4 digits each
+    "rgb:R/G/B", with R, G and B hex strings, 1-4 digits each
+    "rgbi:R/G/B", with R, G and B numbers in [0,1]
+
+   The result is normalised to a maximum value of 65535 per component.  */
+bool
+parse_color_spec (const char *s,
+                  unsigned short *r, unsigned short *g, unsigned short *b)
+{
+  int len = strlen (s);
+  if (s[0] == '#')
+    {
+      if ((len - 1) % 3 == 0)
+        {
+          int n = (len - 1) / 3;
+          return (   parse_hex_comp (s + 1 + 0 * n, s + 1 + 1 * n, r)
+                  && parse_hex_comp (s + 1 + 1 * n, s + 1 + 2 * n, g)
+                  && parse_hex_comp (s + 1 + 2 * n, s + 1 + 3 * n, b));
+        }
+    }
+  else if (strncmp (s, "rgb:", 4) == 0)
+    {
+      char *sep1, *sep2;
+      return ((sep1 = strchr (s + 4, '/')) != NULL
+              && (sep2 = strchr (sep1 + 1, '/')) != NULL
+              && parse_hex_comp (s + 4, sep1, r)
+              && parse_hex_comp (sep1 + 1, sep2, g)
+              && parse_hex_comp (sep2 + 1, s + len, b));
+    }
+  else if (strncmp (s, "rgbi:", 5) == 0)
+    {
+      char *sep1, *sep2;
+      double red, green, blue;
+      if ((sep1 = strchr (s + 5, '/')) != NULL
+          && (sep2 = strchr (sep1 + 1, '/')) != NULL
+          && (red = parse_float_comp (s + 5, sep1)) >= 0
+          && (green = parse_float_comp (sep1 + 1, sep2)) >= 0
+          && (blue = parse_float_comp (sep2 + 1, s + len)) >= 0)
+        {
+          *r = lrint (red * 65535);
+          *g = lrint (green * 65535);
+          *b = lrint (blue * 65535);
+          return true;
+        }
+    }
+  return false;
+}
+
+DEFUN ("color-values-from-numeric-string",
+       Fcolor_values_from_numeric_string, Scolor_values_from_numeric_string,
+       1, 1, 0,
+       doc: /* Parse STRING as a numeric colour and return (R G B).
+Recognised formats are:
+
+ #RGB, where R, G and B are hex strings of equal length, 1-4 digits each
+ rgb:R/G/B, where R, G, and B are hex strings, 1-4 digits each
+ rgbi:R/G/B, where R, G and B are numbers in [0,1].
+
+The result is normalised to a maximum value of 65535 per component.
+If STRING is not in one of the above forms, return nil.  */)
+  (Lisp_Object string)
+{
+  unsigned short r, g, b;
+  return (parse_color_spec (SSDATA (string), &r, &g, &b)
+          ? list3i (r, g, b)
+          : Qnil);
+}
+
 /* Parse RGB_LIST, and fill in the RGB fields of COLOR.
    RGB_LIST should contain (at least) 3 lisp integers.
    Return true iff RGB_LIST is OK.  */
@@ -7018,4 +7129,5 @@ syms_of_xfaces (void)
   defsubr (&Sinternal_face_x_get_resource);
   defsubr (&Sx_family_fonts);
 #endif
+  defsubr (&Scolor_values_from_numeric_string);
 }
diff --git a/src/xterm.c b/src/xterm.c
index 7989cecec7..2a7c9d0a14 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -2389,44 +2389,23 @@ #define HEX_COLOR_NAME_LENGTH 32
 Status x_parse_color (struct frame *f, const char *color_name,
 		      XColor *color)
 {
+  /* Don't pass #RGB strings directly to XParseColor, because that
+     follows the X convention of zero-extending each channel
+     value: #f00 means #f00000.  We want the convention of scaling
+     channel values, so #f00 means #ff0000, just as it does for
+     HTML, SVG, and CSS.  */
+  unsigned short r, g, b;
+  if (parse_color_spec (color_name, &r, &g, &b))
+    {
+      color->red = r;
+      color->green = g;
+      color->blue = b;
+      return 1;
+    }
+
   Display *dpy = FRAME_X_DISPLAY (f);
   Colormap cmap = FRAME_X_COLORMAP (f);
   struct color_name_cache_entry *cache_entry;
-
-  if (color_name[0] == '#')
-    {
-      /* Don't pass #RGB strings directly to XParseColor, because that
-	 follows the X convention of zero-extending each channel
-	 value: #f00 means #f00000.  We want the convention of scaling
-	 channel values, so #f00 means #ff0000, just as it does for
-	 HTML, SVG, and CSS.
-
-	 So we translate #f00 to rgb:f/0/0, which X handles
-	 differently. */
-      char rgb_color_name[HEX_COLOR_NAME_LENGTH];
-      int len = strlen (color_name);
-      int digits_per_channel;
-      if (len == 4)
-	digits_per_channel = 1;
-      else if (len == 7)
-	digits_per_channel = 2;
-      else if (len == 10)
-	digits_per_channel = 3;
-      else if (len == 13)
-	digits_per_channel = 4;
-      else
-	return 0;
-
-      snprintf (rgb_color_name, sizeof rgb_color_name, "rgb:%.*s/%.*s/%.*s",
-		digits_per_channel, color_name + 1,
-		digits_per_channel, color_name + digits_per_channel + 1,
-		digits_per_channel, color_name + 2 * digits_per_channel + 1);
-
-      /* The rgb form is parsed directly by XParseColor without
-	 talking to the X server.  No need for caching.  */
-      return XParseColor (dpy, cmap, rgb_color_name, color);
-    }
-
   for (cache_entry = FRAME_DISPLAY_INFO (f)->color_names; cache_entry;
        cache_entry = cache_entry->next)
     {
diff --git a/test/src/xfaces-tests.el b/test/src/xfaces-tests.el
index 5ed16c9e51..ea349445a6 100644
--- a/test/src/xfaces-tests.el
+++ b/test/src/xfaces-tests.el
@@ -24,4 +24,27 @@ xfaces-color-distance
   (should (equal (color-distance "#222222" "#ffffff")
                  (color-distance "#ffffff" "#222222"))))
 
+(ert-deftest xfaces-color-values-from-numeric-string ()
+  (should (equal (color-values-from-numeric-string "#f05")
+                 '(#xffff #x0000 #x5555)))
+  (should (equal (color-values-from-numeric-string "#1fb0C5")
+                 '(#x1f1f #xb0b0 #xc5c5)))
+  (should (equal (color-values-from-numeric-string "#1f8b0AC5e")
+                 '(#x1f81 #xb0aa #xc5eb)))
+  (should (equal (color-values-from-numeric-string "#1f83b0ADC5e2")
+                 '(#x1f83 #xb0ad #xc5e2)))
+  (should (equal (color-values-from-numeric-string "#1f83b0ADC5e2g") nil))
+  (should (equal (color-values-from-numeric-string "#1f83b0ADC5e20") nil))
+  (should (equal (color-values-from-numeric-string "#12345") nil))
+  (should (equal (color-values-from-numeric-string "rgb:f/23/28a")
+                 '(#xffff #x2323 #x28a2)))
+  (should (equal (color-values-from-numeric-string "rgb:1234/5678/09ab")
+                 '(#x1234 #x5678 #x09ab)))
+  (should (equal (color-values-from-numeric-string "rgb:0//0") nil))
+  (should (equal (color-values-from-numeric-string "rgbi:0/0.5/0.1")
+                 '(0 32768 6554)))
+  (should (equal (color-values-from-numeric-string "rgbi:1e-3/1.0e-2/1e0")
+                 '(66 655 65535)))
+  (should (equal (color-values-from-numeric-string "rgbi:0/0.5/10") nil)))
+
 (provide 'xfaces-tests)
-- 
2.21.1 (Apple Git-122.3)

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 12 Jun 2020 18:59:36 +0200
> Cc: emacs-devel@gnu.org
> 
> 8 juni 2020 kl. 14.26 skrev Pip Cet <pipcet@gmail.com>:
> 
> > Having two different versions of this seems superfluous
> 
> We had four versions. Here is a patch replacing them all.

Were the 4 versions identical or different?  If the latter, what were
the differences, and which of the features that call them will from
now on behave differently?

Re: master 64e25cd: More robust NS hex colour string parsing

[Basil L. Contovounesios]

Mattias Engdegård <mattiase@acm.org> writes:

> Only really tested for NS; someone needs to check the X and Windows backends.

I get this warning:

  xterm.c:2379: warning: macro "HEX_COLOR_NAME_LENGTH" is not used [-Wunused-macros]
   2379 | #define HEX_COLOR_NAME_LENGTH 32

Otherwise seems to compile and run fine.  I tried:

0. emacs -Q
1. M-x list-colors-display RET
2. M-x set-background-color RET #abc RET

Is there something else I should be doing to test this?

Thanks,

-- 
Basil

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2020-06-12 built on thunk
Repository revision: 54efe18959591faa1087051c878abe470f53a28f
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O2 -march=native' --config-cache
 --prefix=/home/blc/.local --with-x-toolkit=lucid
 --with-file-notification=yes --with-x'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT
LIBOTF ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

12 juni 2020 kl. 19.33 skrev Eli Zaretskii <eliz@gnu.org>:

> Were the 4 versions identical or different?  If the latter, what were
> the differences, and which of the features that call them will from
> now on behave differently?

They were not semantically equivalent but clearly intended to be. Only the X and Windows versions accepted the rgbi: format; most if not all had bad error-checking (simple typos silently giving nonsense values), and the Windows version appears to have normalised incorrectly in some cases (this is from reading the code with no Windows machine handy).

The new code should parse a superset of what the old code did, detect more mistakes, and be as accurate as we can make it.

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 12 Jun 2020 21:00:10 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> 12 juni 2020 kl. 19.33 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Were the 4 versions identical or different?  If the latter, what were
> > the differences, and which of the features that call them will from
> > now on behave differently?
> 
> They were not semantically equivalent but clearly intended to be. Only the X and Windows versions accepted the rgbi: format; most if not all had bad error-checking (simple typos silently giving nonsense values), and the Windows version appears to have normalised incorrectly in some cases (this is from reading the code with no Windows machine handy).
> 
> The new code should parse a superset of what the old code did, detect more mistakes, and be as accurate as we can make it.

Thanks, but I don't think I understand the answer well enough to make
up my mind regarding the proposed code.  The error checking aside, are
the return values of the original code the same as of the proposed
unified code?  If not, which of the 4 current versions differ, and
how?

Re: master 64e25cd: More robust NS hex colour string parsing

[Pip Cet]

Mattias Engdegård <mattiase@acm.org> writes:

> 12 juni 2020 kl. 19.33 skrev Eli Zaretskii <eliz@gnu.org>:
>
>> Were the 4 versions identical or different?  If the latter, what were
>> the differences, and which of the features that call them will from
>> now on behave differently?
>
> They were not semantically equivalent but clearly intended to be.

Only quite recently; the X version was changed to use the standard
scaling convention (#fff means white) a while back, and at the time I
preferred the approach of still letting Xlib handle the request, and
passing through whatever nonsensical values we might have gotten after
the '#' verbatim.

That said, I think there's currently a standard interpretation of 16-bit
color values (sRGB or something close enough), and I see no harm in your
proposed change provided that this is so (I think it's probably a good
idea to have things in one place so we can change it to a better color
representation once the technology is there. L-a-b is essentially
equivalent to RGB, except the components may be negative (not all of
them, obviously), so if we move to using three floats for representing a
color, they shouldn't be required to be strictly in the [0,1] range.)

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> Date: Fri, 12 Jun 2020 22:11:10 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> The error checking aside, are the return values of the original code
> the same as of the proposed unified code?  If not, which of the 4
> current versions differ, and how?

Also, the return value in case of error seems to change the
externally-visible behavior:

> +If STRING is not in one of the above forms, return nil.  */)
> +  (Lisp_Object string)
> +{
> +  unsigned short r, g, b;
> +  return (parse_color_spec (SSDATA (string), &r, &g, &b)
> +          ? list3i (r, g, b)
> +          : Qnil);
> +}

At least tty-color-standard-values seems to never return nil for an
RGB spec, but now it will, right?  Can its callers cope with such a
return value?

And I wonder how the other callers of parse_color_spec will behave if
it detects an error and returns false.  Did you audit the code of the
callers to see if this could cause trouble?

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

12 juni 2020 kl. 21.11 skrev Eli Zaretskii <eliz@gnu.org>:

> The error checking aside, are
> the return values of the original code the same as of the proposed
> unified code?

Returned values for well-formed input are identical, except the low bits in some cases on Windows (try "#123"); that bug has been fixed.
Malformed arguments are now consistently rejected. Previously they were only sometimes, depending on the platform and the nature of the error.

> At least tty-color-standard-values seems to never return nil for an
> RGB spec, but now it will, right?  Can its callers cope with such a
> return value?

Not sure what you mean here. Mind giving a concrete example?

> And I wonder how the other callers of parse_color_spec will behave if
> it detects an error and returns false.

Exactly the way they do now when they detect an error: treated as a colour not recognised.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

12 juni 2020 kl. 21.15 skrev Pip Cet <pipcet@gmail.com>:

> Only quite recently; the X version was changed to use the standard
> scaling convention

Thanks for the perspective, and for driving through the reform at the time. The suggested patch obviously builds on top of your work.

I got fed up with silently getting nonsense from mistakes like #12345 -- lax error checking is just unhelpful.
There are probably a lot more duplication and inconsistencies between the backends, which is to be expected in a system that evolved during a long period of time.

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 13 Jun 2020 12:17:09 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> 12 juni 2020 kl. 21.11 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > The error checking aside, are
> > the return values of the original code the same as of the proposed
> > unified code?
> 
> Returned values for well-formed input are identical

That's good to know, thanks.

> except the low bits in some cases on Windows (try "#123")

I don't think I understand what I should try; please elaborate.

> Malformed arguments are now consistently rejected.

How exactly are they rejected?  I see the return value of
parse_color_spec, but what happens in its callers, and what happens on
the Lisp levels when those callers are called?

> > At least tty-color-standard-values seems to never return nil for an
> > RGB spec, but now it will, right?  Can its callers cope with such a
> > return value?
> 
> Not sure what you mean here. Mind giving a concrete example?

The first two branches of the 'cond' would always return a list before
your changes, but after your changes they could return nil if
color-values-from-numeric-string (not the best name, btw) returns nil
and the input is of one of the two forms parsed by those two branches.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

13 juni 2020 kl. 13.59 skrev Eli Zaretskii <eliz@gnu.org>:

>> except the low bits in some cases on Windows (try "#123")
> 
> I don't think I understand what I should try; please elaborate.

Try (color-values "#123"). The correct result is (#x1111 #x2222 #x3333).
If you get something else on Windows, then that is a bug fixed by the patch.
If the patch doesn't help, then it may be a bug in the patch.

Note that the Windows backend will still discard the low 8 bits in any case; (color-values "#123456789abc") will probably return (#x1212 #x5656 #x9a9a). The patch doesn't change that.

>> Malformed arguments are now consistently rejected.
> 
> How exactly are they rejected?

In the same way that detected errors were rejected before: as if given an undefined colour such as "fuchsia green".

>  I see the return value of
> parse_color_spec, but what happens in its callers, and what happens on
> the Lisp levels when those callers are called?

None of that has changed.

>> Not sure what you mean here. Mind giving a concrete example?
> 
> The first two branches of the 'cond' would always return a list before
> your changes, but after your changes they could return nil if
> color-values-from-numeric-string (not the best name, btw) returns nil
> and the input is of one of the two forms parsed by those two branches.

This is still a tad vague and speculative. Can you give a concrete example of an argument that, in your opinion, isn't handled correctly?
As far as I can tell, well-formed arguments yield correct values and others are rejected in the same way as before, although error-checking now is less spotty.

I don't particularly like that function name either. What about one of these:

 parse-color
 parse-color-spec
 parse-color-string
 color-values-from-spec
 color-from-string-spec
 color-spec-to-values

Xlib uses the term 'numerical colour specification', but color-values-from-numerical-color-specification is a mouthful even in Lisp.

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 13 Jun 2020 17:39:29 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> Try (color-values "#123"). The correct result is (#x1111 #x2222 #x3333).

Why is that the correct value?  I get (#x1010 #x2020 #x3030); why is
that wrong?

> >> Not sure what you mean here. Mind giving a concrete example?
> > 
> > The first two branches of the 'cond' would always return a list before
> > your changes, but after your changes they could return nil if
> > color-values-from-numeric-string (not the best name, btw) returns nil
> > and the input is of one of the two forms parsed by those two branches.
> 
> This is still a tad vague and speculative.

Just follow the code, it should be very clear: those two branches
always return a list of values.  No example should be needed.

> As far as I can tell, well-formed arguments yield correct values and
> others are rejected in the same way as before

No, that's not true, as should be obvious from examining the code.
Previously, any "#..." string whose length was 4 or longer would
return a list of values, even if it wasn't well-formed; now some of
them will return nil.

> I don't particularly like that function name either. What about one of these:
> 
>  parse-color
>  parse-color-spec
>  parse-color-string
>  color-values-from-spec
>  color-from-string-spec
>  color-spec-to-values

color-values-from-rgb-spec?

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

13 juni 2020 kl. 17.58 skrev Eli Zaretskii <eliz@gnu.org>:

>> Try (color-values "#123"). The correct result is (#x1111 #x2222 #x3333).
> 
> Why is that the correct value?  I get (#x1010 #x2020 #x3030); why is
> that wrong?

It violates the HTML/CSS convention which was agreed upon in bug#36304 and followed by the other backends. Single-digit hex numbers are scaled by 65535/15, two-digit numbers by 65535/255, and three-digit numbers by 65535/4095.

> Just follow the code, it should be very clear: those two branches
> always return a list of values.  No example should be needed.

An example could help resolve misunderstanding, and if we go back-and-forth on what you think is a simple matter it's a clear sign that one is definitely needed.

> No, that's not true, as should be obvious from examining the code.
> Previously, any "#..." string whose length was 4 or longer would
> return a list of values, even if it wasn't well-formed; now some of
> them will return nil.

(tty-color-values "#xyz") returned nil (and still does), contradicting your claim.

I meant that the manner of rejection remains unchanged, not the set of rejected arguments, which is a consequence of improved error-checking, very much by design.
Not only was it previously lacking, its coverage varied wildly between backends. That means that hardly any code could have abused the lax checking while still working on multiple platforms. Of course, the unpredictable behaviour on malformed input made this a very dubious endeavour in the first place.

> color-values-from-rgb-spec?

Thank you, but that would preclude addition of non-RGB formats in the future, such as HSV or XIE XYZ. Nothing in the interface forces the specification to be RGB. In fact, Xlib accepts several non-RGB formats.

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 13 Jun 2020 18:44:04 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> 13 juni 2020 kl. 17.58 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> >> Try (color-values "#123"). The correct result is (#x1111 #x2222 #x3333).
> > 
> > Why is that the correct value?  I get (#x1010 #x2020 #x3030); why is
> > that wrong?
> 
> It violates the HTML/CSS convention which was agreed upon in bug#36304 and followed by the other backends. Single-digit hex numbers are scaled by 65535/15, two-digit numbers by 65535/255, and three-digit numbers by 65535/4095.

But the code you want to supplant explicitly does something different:

	      value = strtoul (color, &end, 16);
	      color[size] = t;
	      if (errno == ERANGE || end - color != size)
		break;
	      switch (size)
		{
		case 1:
		  value = value * 0x10;
		  break;
		case 2:
		  break;
		case 3:
		  value /= 0x10;
		  break;
		case 4:
		  value /= 0x100;
		  break;
		}

That multiplication by 0x10 cannot be a typo, it's a deliberate
interpretation of #f as #f0, not as #0f or #ff.

Of course, single-digit hex RGB specs are rarely if ever used these
days, AFAIK, so maybe this isn't a problem in practice.  But still, I
wonder what could we lose here or break.

> > Just follow the code, it should be very clear: those two branches
> > always return a list of values.  No example should be needed.
> 
> An example could help resolve misunderstanding, and if we go back-and-forth on what you think is a simple matter it's a clear sign that one is definitely needed.
> 
> > No, that's not true, as should be obvious from examining the code.
> > Previously, any "#..." string whose length was 4 or longer would
> > return a list of values, even if it wasn't well-formed; now some of
> > them will return nil.
> 
> (tty-color-values "#xyz") returned nil (and still does), contradicting your claim.

How about (tty-color-values "#12345")? does it contradict yours?

> I meant that the manner of rejection remains unchanged, not the set of rejected arguments, which is a consequence of improved error-checking, very much by design.
> Not only was it previously lacking, its coverage varied wildly between backends. That means that hardly any code could have abused the lax checking while still working on multiple platforms. Of course, the unpredictable behaviour on malformed input made this a very dubious endeavour in the first place.

You are changing the behavior, so get ready to hear bug reports, is
all I'm saying.

> > color-values-from-rgb-spec?
> 
> Thank you, but that would preclude addition of non-RGB formats in the future, such as HSV or XIE XYZ. Nothing in the interface forces the specification to be RGB. In fact, Xlib accepts several non-RGB formats.

Then color-values-from-color-spec, I guess.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

13 juni 2020 kl. 19.09 skrev Eli Zaretskii <eliz@gnu.org>:

> But the code you want to supplant explicitly does something different:

Yes, which is part of the reason why it's being supplanted.

> Of course, single-digit hex RGB specs are rarely if ever used these
> days, AFAIK, so maybe this isn't a problem in practice.  But still, I
> wonder what could we lose here or break.

This isn't going to be a problem in practice.

> How about (tty-color-values "#12345")? does it contradict yours?

No, it is an error that is (correctly) rejected by the new code in the same way that #xyz is currently rejected.

> You are changing the behavior, so get ready to hear bug reports, is
> all I'm saying.

That's fine, I'll be ready.
It's impossible to fix bugs without changing behaviour, so if we want the former, we have to allow for the latter, within bounds appropriate for the situation.

> Then color-values-from-color-spec, I guess.

Thank you, but a smidgen too many 'color' in there. Would color-values-from-spec do?

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 13 Jun 2020 19:29:33 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> > How about (tty-color-values "#12345")? does it contradict yours?
> 
> No, it is an error that is (correctly) rejected by the new code in the same way that #xyz is currently rejected.

But it isn't rejected by the current code.  Which was my point all
along.

> > Then color-values-from-color-spec, I guess.
> 
> Thank you, but a smidgen too many 'color' in there.

I don't think so, no.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

12 juni 2020 kl. 20.33 skrev Basil L. Contovounesios <contovob@tcd.ie>:

>  xterm.c:2379: warning: macro "HEX_COLOR_NAME_LENGTH" is not used [-Wunused-macros]
>   2379 | #define HEX_COLOR_NAME_LENGTH 32

Thank you very much, I'll fix that.

> Is there something else I should be doing to test this?

If things look right they probably are, I suppose.

You could try (set-background-color "#f0f") and confirm the induced wave of nausea.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

13 juni 2020 kl. 19.35 skrev Eli Zaretskii <eliz@gnu.org>:

> But it isn't rejected by the current code.  Which was my point all
> along.

Since "#12345" is malformed it should be rejected, and will be.

>>> Then color-values-from-color-spec, I guess.
>> 
>> Thank you, but a smidgen too many 'color' in there.
> 
> I don't think so, no.

Very well, I have no strong opinion here so let's go with it.

Updated patch attached, with the name change, and the warning found by Basil fixed.


[-- Attachment #2: 0001-Consolidate-RGB-string-parsers.patch --]
[-- Type: application/octet-stream, Size: 20268 bytes --]

From a06d9d76f94f8bc3afdf92962fa88d43fff67d09 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 12 Jun 2020 18:12:37 +0200
Subject: [PATCH] Consolidate #RGB string parsers

Use a single parser of colour strings in the #RGB, rgb:R/G/B and
rgbi:R/G/B formats, replacing four existing ones.  Previously,
error-checking was spotty, handling of the rgbi: format not always
present, and normalisation of the result was sometimes incorrect.

* src/dispextern.h: New prototype.
* src/xfaces.c (parse_hex_comp, parse_float_comp, parse_color_spec)
(Fcolor_values_from_color_spec): New functions.
* test/src/xfaces-tests.el (xfaces-color-values-from-color-spec):
New test.
* lisp/term/tty-colors.el (tty-color-standard-values):
Use color-values-from-color-spec, replacing old parser.
* src/nsterm.m (ns_get_color):
* src/w32fns.c (x_to_w32_color):
* src/xterm.c (x_parse_color): Use parse_color_spec, replacing old
parsers.
(HEX_COLOR_NAME_LENGTH): Remove #define.
---
 lisp/term/tty-colors.el  |  58 +--------------
 src/dispextern.h         |   2 +
 src/nsterm.m             |  59 +++++----------
 src/w32fns.c             | 157 ++-------------------------------------
 src/xfaces.c             | 112 ++++++++++++++++++++++++++++
 src/xterm.c              |  51 ++++---------
 test/src/xfaces-tests.el |  23 ++++++
 7 files changed, 176 insertions(+), 286 deletions(-)

diff --git a/lisp/term/tty-colors.el b/lisp/term/tty-colors.el
index 39ca2d3627..dda7fcc369 100644
--- a/lisp/term/tty-colors.el
+++ b/lisp/term/tty-colors.el
@@ -923,62 +923,8 @@ tty-color-standard-values
 COLOR (see the info node `(emacs) Colors'), regardless of whether
 the terminal can display it, so the return value should be the
 same regardless of what display is being used."
-  (let ((len (length color)))
-    (cond ((and (>= len 4) ;; HTML/CSS/SVG-style "#XXYYZZ" color spec
-		(eq (aref color 0) ?#)
-		(member (aref color 1)
-			'(?0 ?1 ?2 ?3 ?4 ?5 ?6 ?7 ?8 ?9
-			     ?a ?b ?c ?d ?e ?f
-                             ?A ?B ?C ?D ?E ?F)))
-	   ;; Translate the string "#XXYYZZ" into a list of numbers
-	   ;; (XX YY ZZ), scaling each to the {0..65535} range.  This
-	   ;; follows the HTML color convention, where both "#fff" and
-	   ;; "#ffffff" represent the same color, white.
-	   (let* ((ndig (/ (- len 1) 3))
-		  (maxval (1- (ash 1 (* 4 ndig))))
-		  (i1 1)
-		  (i2 (+ i1 ndig))
-		  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-	     (list
-	      (/ (* (string-to-number
-		     (substring color i1 i2) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i2 i3) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i3 i4) 16)
-		    65535)
-		 maxval))))
-	  ((and (>= len 9) ;; X-style rgb:xx/yy/zz color spec
-		(string= (substring color 0 4) "rgb:"))
-	   ;; Translate the string "rgb:XX/YY/ZZ" into a list of
-	   ;; numbers (XX YY ZZ), scaling each to the {0..65535}
-	   ;; range.  "rgb:F/F/F" is white.
-	   (let* ((ndig (/ (- len 3) 3))
-		  (maxval (1- (ash 1 (* 4 (- ndig 1)))))
-		  (i1 4)
-		  (i2 (+ i1 ndig))
-		  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-	     (list
-	      (/ (* (string-to-number
-		     (substring color i1 (- i2 1)) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i2 (- i3 1)) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i3 (1- i4)) 16)
-		    65535)
-		 maxval))))
-	  (t
-	   (cdr (assoc color color-name-rgb-alist))))))
+  (or (color-values-from-color-spec color)
+      (cdr (assoc color color-name-rgb-alist))))
 
 (defun tty-color-translate (color &optional frame)
   "Given a color COLOR, return the index of the corresponding TTY color.
diff --git a/src/dispextern.h b/src/dispextern.h
index 0b1f3d14ae..e1d6eddc41 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3514,6 +3514,8 @@ #define RGB_PIXEL_COLOR COLORREF
                                        Lisp_Object);
 extern bool tty_defined_color (struct frame *, const char *, Emacs_Color *,
                                bool, bool);
+bool parse_color_spec (const char *,
+                       unsigned short *, unsigned short *, unsigned short *);
 
 Lisp_Object tty_color_name (struct frame *, int);
 void clear_face_cache (bool);
diff --git a/src/nsterm.m b/src/nsterm.m
index 3dc7e1db7c..0e405fc017 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2341,9 +2341,6 @@ so some key presses (TAB) are swallowed by the system.  */
    See https://lists.gnu.org/r/emacs-devel/2009-07/msg01203.html.  */
 {
   NSColor *new = nil;
-  static char hex[20];
-  int scaling = 0;
-  float r = -1.0, g, b;
   NSString *nsname = [NSString stringWithUTF8String: name];
 
   NSTRACE ("ns_get_color(%s, **)", name);
@@ -2386,51 +2383,31 @@ so some key presses (TAB) are swallowed by the system.  */
     }
 
   /* First, check for some sort of numeric specification.  */
-  hex[0] = '\0';
-
-  if (name[0] == '0' || name[0] == '1' || name[0] == '.')  /* RGB decimal */
+  unsigned short r16, g16, b16;
+  if (parse_color_spec (name, &r16, &g16, &b16))
     {
-      NSScanner *scanner = [NSScanner scannerWithString: nsname];
-      [scanner scanFloat: &r];
-      [scanner scanFloat: &g];
-      [scanner scanFloat: &b];
-    }
-  else if (!strncmp(name, "rgb:", 4))  /* A newer X11 format -- rgb:r/g/b */
-    scaling = (snprintf (hex, sizeof hex, "%s", name + 4) - 2) / 3;
-  else if (name[0] == '#')        /* An old X11 format; convert to newer */
-    {
-      int len = 0;
-      while (isxdigit (name[len + 1]))
-        len++;
-      if (name[len + 1] == '\0' && len >= 1 && len <= 12 && len % 3 == 0)
-        {
-          scaling = len / 3;
-          for (int i = 0; i < 3; i++)
-            sprintf (hex + i * (scaling + 1), "%.*s/", scaling,
-                     name + 1 + i * scaling);
-          hex[3 * (scaling + 1) - 1] = '\0';
-        }
+      *col = [NSColor colorForEmacsRed: r16 / 65535.0
+                                 green: g16 / 65535.0
+                                  blue: b16 / 65535.0
+                                 alpha: 1.0];
+      unblock_input ();
+      return 0;
     }
-
-  if (hex[0])
+  else if (name[0] == '0' || name[0] == '1' || name[0] == '.')
     {
-      unsigned int rr, gg, bb;
-      float fscale = (1 << (scaling * 4)) - 1;
-      if (sscanf (hex, "%x/%x/%x", &rr, &gg, &bb))
+      /* RGB decimal */
+      NSScanner *scanner = [NSScanner scannerWithString: nsname];
+      float r, g, b;
+      if (   [scanner scanFloat: &r] && r >= 0 && r <= 1
+          && [scanner scanFloat: &g] && g >= 0 && g <= 1
+          && [scanner scanFloat: &b] && b >= 0 && b <= 1)
         {
-          r = rr / fscale;
-          g = gg / fscale;
-          b = bb / fscale;
+          *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
+          unblock_input ();
+          return 0;
         }
     }
 
-  if (r >= 0.0F)
-    {
-      *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
-      unblock_input ();
-      return 0;
-    }
-
   /* Otherwise, color is expected to be from a list */
   {
     NSEnumerator *lenum, *cenum;
diff --git a/src/w32fns.c b/src/w32fns.c
index e595b0285a..ab864332e7 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -864,161 +864,14 @@ x_to_w32_color (const char * colorname)
 
   block_input ();
 
-  if (colorname[0] == '#')
+  unsigned short r, g, b;
+  if (parse_color_spec (colorname, &r, &g, &b))
     {
-      /* Could be an old-style RGB Device specification.  */
-      int size = strlen (colorname + 1);
-      char *color = alloca (size + 1);
-
-      strcpy (color, colorname + 1);
-      if (size == 3 || size == 6 || size == 9 || size == 12)
-	{
-	  UINT colorval;
-	  int i, pos;
-	  pos = 0;
-	  size /= 3;
-	  colorval = 0;
-
-	  for (i = 0; i < 3; i++)
-	    {
-	      char *end;
-	      char t;
-	      unsigned long value;
-
-	      /* The check for 'x' in the following conditional takes into
-		 account the fact that strtol allows a "0x" in front of
-		 our numbers, and we don't.  */
-	      if (!isxdigit (color[0]) || color[1] == 'x')
-		break;
-	      t = color[size];
-	      color[size] = '\0';
-	      value = strtoul (color, &end, 16);
-	      color[size] = t;
-	      if (errno == ERANGE || end - color != size)
-		break;
-	      switch (size)
-		{
-		case 1:
-		  value = value * 0x10;
-		  break;
-		case 2:
-		  break;
-		case 3:
-		  value /= 0x10;
-		  break;
-		case 4:
-		  value /= 0x100;
-		  break;
-		}
-	      colorval |= (value << pos);
-	      pos += 0x8;
-	      if (i == 2)
-		{
-		  unblock_input ();
-		  XSETINT (ret, colorval);
-		  return ret;
-		}
-	      color = end;
-	    }
-	}
-    }
-  else if (strnicmp (colorname, "rgb:", 4) == 0)
-    {
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 4;
-      for (i = 0; i < 3; i++)
-	{
-	  char *end;
-	  unsigned long value;
-
-	  /* The check for 'x' in the following conditional takes into
-	     account the fact that strtol allows a "0x" in front of
-	     our numbers, and we don't.  */
-	  if (!isxdigit (color[0]) || color[1] == 'x')
-	    break;
-	  value = strtoul (color, &end, 16);
-	  if (errno == ERANGE)
-	    break;
-	  switch (end - color)
-	    {
-	    case 1:
-	      value = value * 0x10 + value;
-	      break;
-	    case 2:
-	      break;
-	    case 3:
-	      value /= 0x10;
-	      break;
-	    case 4:
-	      value /= 0x100;
-	      break;
-	    default:
-	      value = ULONG_MAX;
-	    }
-	  if (value == ULONG_MAX)
-	    break;
-	  colorval |= (value << pos);
-	  pos += 0x8;
-	  if (i == 2)
-	    {
-	      if (*end != '\0')
-		break;
-	      unblock_input ();
-	      XSETINT (ret, colorval);
-	      return ret;
-	    }
-	  if (*end != '/')
-	    break;
-	  color = end + 1;
-	}
+      unblock_input ();
+      /* Throw away the low 8 bits and return 0xBBGGRR.  */
+      return make_fixnum ((b & 0xff00) << 8 | (g & 0xff00) | r >> 8);
     }
-  else if (strnicmp (colorname, "rgbi:", 5) == 0)
-    {
-      /* This is an RGB Intensity specification.  */
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 5;
-      for (i = 0; i < 3; i++)
-	{
-	  char *end;
-	  double value;
-	  UINT val;
 
-	  value = strtod (color, &end);
-	  if (errno == ERANGE)
-	    break;
-	  if (value < 0.0 || value > 1.0)
-	    break;
-	  val = (UINT)(0x100 * value);
-	  /* We used 0x100 instead of 0xFF to give a continuous
-	     range between 0.0 and 1.0 inclusive.  The next statement
-	     fixes the 1.0 case.  */
-	  if (val == 0x100)
-	    val = 0xFF;
-	  colorval |= (val << pos);
-	  pos += 0x8;
-	  if (i == 2)
-	    {
-	      if (*end != '\0')
-		break;
-	      unblock_input ();
-	      XSETINT (ret, colorval);
-	      return ret;
-	    }
-	  if (*end != '/')
-	    break;
-	  color = end + 1;
-	}
-    }
   /* I am not going to attempt to handle any of the CIE color schemes
      or TekHVC, since I don't know the algorithms for conversion to
      RGB.  */
diff --git a/src/xfaces.c b/src/xfaces.c
index cf155288bd..0a2dec1cff 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -220,6 +220,7 @@ Copyright (C) 1993-1994, 1998-2020 Free Software Foundation, Inc.
 #include "sysstdio.h"
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <math.h>
 
 #include "lisp.h"
 #include "character.h"
@@ -819,6 +820,116 @@ load_pixmap (struct frame *f, Lisp_Object name)
                             Color Handling
  ***********************************************************************/
 
+/* Parse fractional hex digits at S ending right before E.
+   Set *DST to the value normalised to 65535 and return true on success,
+   false otherwise.  */
+static bool
+parse_hex_comp (const char *s, const char *e, unsigned short *dst)
+{
+  int n = e - s;
+  if (n <= 0 || n > 4)
+    return false;
+  int val = 0;
+  for (; s < e; s++)
+    {
+      int digit;
+      if (*s >= '0' && *s <= '9')
+        digit = *s - '0';
+      else if (*s >= 'A' && *s <= 'F')
+        digit = *s - 'A' + 10;
+      else if (*s >= 'a' && *s <= 'f')
+        digit = *s - 'a' + 10;
+      else
+        return false;
+      val = (val << 4) | digit;
+    }
+  int maxval = (1 << (n * 4)) - 1;
+  *dst = (unsigned)val * 65535 / maxval;
+  return true;
+}
+
+/* Parse floating-point number at S ending right before E.
+   Return the number if in the range [0,1]; otherwise -1.  */
+static double
+parse_float_comp (const char *s, const char *e)
+{
+  char *end;
+  double x = strtod (s, &end);
+  return (end == e && x >= 0 && x <= 1) ? x : -1;
+}
+
+/* Parse S as a numeric colour specification and set *R, *G and *B.
+   Return true on success, false on failure.
+   Recognised formats:
+
+    "#RGB", with R, G and B hex strings of equal length, 1-4 digits each
+    "rgb:R/G/B", with R, G and B hex strings, 1-4 digits each
+    "rgbi:R/G/B", with R, G and B numbers in [0,1]
+
+   The result is normalised to a maximum value of 65535 per component.  */
+bool
+parse_color_spec (const char *s,
+                  unsigned short *r, unsigned short *g, unsigned short *b)
+{
+  int len = strlen (s);
+  if (s[0] == '#')
+    {
+      if ((len - 1) % 3 == 0)
+        {
+          int n = (len - 1) / 3;
+          return (   parse_hex_comp (s + 1 + 0 * n, s + 1 + 1 * n, r)
+                  && parse_hex_comp (s + 1 + 1 * n, s + 1 + 2 * n, g)
+                  && parse_hex_comp (s + 1 + 2 * n, s + 1 + 3 * n, b));
+        }
+    }
+  else if (strncmp (s, "rgb:", 4) == 0)
+    {
+      char *sep1, *sep2;
+      return ((sep1 = strchr (s + 4, '/')) != NULL
+              && (sep2 = strchr (sep1 + 1, '/')) != NULL
+              && parse_hex_comp (s + 4, sep1, r)
+              && parse_hex_comp (sep1 + 1, sep2, g)
+              && parse_hex_comp (sep2 + 1, s + len, b));
+    }
+  else if (strncmp (s, "rgbi:", 5) == 0)
+    {
+      char *sep1, *sep2;
+      double red, green, blue;
+      if ((sep1 = strchr (s + 5, '/')) != NULL
+          && (sep2 = strchr (sep1 + 1, '/')) != NULL
+          && (red = parse_float_comp (s + 5, sep1)) >= 0
+          && (green = parse_float_comp (sep1 + 1, sep2)) >= 0
+          && (blue = parse_float_comp (sep2 + 1, s + len)) >= 0)
+        {
+          *r = lrint (red * 65535);
+          *g = lrint (green * 65535);
+          *b = lrint (blue * 65535);
+          return true;
+        }
+    }
+  return false;
+}
+
+DEFUN ("color-values-from-color-spec",
+       Fcolor_values_from_color_spec, Scolor_values_from_color_spec,
+       1, 1, 0,
+       doc: /* Parse STRING as a numeric colour and return (R G B).
+Recognised formats are:
+
+ #RGB, where R, G and B are hex strings of equal length, 1-4 digits each
+ rgb:R/G/B, where R, G, and B are hex strings, 1-4 digits each
+ rgbi:R/G/B, where R, G and B are numbers in [0,1].
+
+The result is normalised to a maximum value of 65535 per component.
+If STRING is not in one of the above forms, return nil.  */)
+  (Lisp_Object string)
+{
+  unsigned short r, g, b;
+  return (parse_color_spec (SSDATA (string), &r, &g, &b)
+          ? list3i (r, g, b)
+          : Qnil);
+}
+
 /* Parse RGB_LIST, and fill in the RGB fields of COLOR.
    RGB_LIST should contain (at least) 3 lisp integers.
    Return true iff RGB_LIST is OK.  */
@@ -7018,4 +7129,5 @@ syms_of_xfaces (void)
   defsubr (&Sinternal_face_x_get_resource);
   defsubr (&Sx_family_fonts);
 #endif
+  defsubr (&Scolor_values_from_color_spec);
 }
diff --git a/src/xterm.c b/src/xterm.c
index 7989cecec7..6340700cb8 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -2376,8 +2376,6 @@ x_query_frame_background_color (struct frame *f, XColor *bgcolor)
   x_query_colors (f, bgcolor, 1);
 }
 
-#define HEX_COLOR_NAME_LENGTH 32
-
 /* On frame F, translate the color name to RGB values.  Use cached
    information, if possible.
 
@@ -2389,44 +2387,23 @@ #define HEX_COLOR_NAME_LENGTH 32
 Status x_parse_color (struct frame *f, const char *color_name,
 		      XColor *color)
 {
+  /* Don't pass #RGB strings directly to XParseColor, because that
+     follows the X convention of zero-extending each channel
+     value: #f00 means #f00000.  We want the convention of scaling
+     channel values, so #f00 means #ff0000, just as it does for
+     HTML, SVG, and CSS.  */
+  unsigned short r, g, b;
+  if (parse_color_spec (color_name, &r, &g, &b))
+    {
+      color->red = r;
+      color->green = g;
+      color->blue = b;
+      return 1;
+    }
+
   Display *dpy = FRAME_X_DISPLAY (f);
   Colormap cmap = FRAME_X_COLORMAP (f);
   struct color_name_cache_entry *cache_entry;
-
-  if (color_name[0] == '#')
-    {
-      /* Don't pass #RGB strings directly to XParseColor, because that
-	 follows the X convention of zero-extending each channel
-	 value: #f00 means #f00000.  We want the convention of scaling
-	 channel values, so #f00 means #ff0000, just as it does for
-	 HTML, SVG, and CSS.
-
-	 So we translate #f00 to rgb:f/0/0, which X handles
-	 differently. */
-      char rgb_color_name[HEX_COLOR_NAME_LENGTH];
-      int len = strlen (color_name);
-      int digits_per_channel;
-      if (len == 4)
-	digits_per_channel = 1;
-      else if (len == 7)
-	digits_per_channel = 2;
-      else if (len == 10)
-	digits_per_channel = 3;
-      else if (len == 13)
-	digits_per_channel = 4;
-      else
-	return 0;
-
-      snprintf (rgb_color_name, sizeof rgb_color_name, "rgb:%.*s/%.*s/%.*s",
-		digits_per_channel, color_name + 1,
-		digits_per_channel, color_name + digits_per_channel + 1,
-		digits_per_channel, color_name + 2 * digits_per_channel + 1);
-
-      /* The rgb form is parsed directly by XParseColor without
-	 talking to the X server.  No need for caching.  */
-      return XParseColor (dpy, cmap, rgb_color_name, color);
-    }
-
   for (cache_entry = FRAME_DISPLAY_INFO (f)->color_names; cache_entry;
        cache_entry = cache_entry->next)
     {
diff --git a/test/src/xfaces-tests.el b/test/src/xfaces-tests.el
index 5ed16c9e51..72bfba5192 100644
--- a/test/src/xfaces-tests.el
+++ b/test/src/xfaces-tests.el
@@ -24,4 +24,27 @@ xfaces-color-distance
   (should (equal (color-distance "#222222" "#ffffff")
                  (color-distance "#ffffff" "#222222"))))
 
+(ert-deftest xfaces-color-values-from-color-spec ()
+  (should (equal (color-values-from-color-spec "#f05")
+                 '(#xffff #x0000 #x5555)))
+  (should (equal (color-values-from-color-spec "#1fb0C5")
+                 '(#x1f1f #xb0b0 #xc5c5)))
+  (should (equal (color-values-from-color-spec "#1f8b0AC5e")
+                 '(#x1f81 #xb0aa #xc5eb)))
+  (should (equal (color-values-from-color-spec "#1f83b0ADC5e2")
+                 '(#x1f83 #xb0ad #xc5e2)))
+  (should (equal (color-values-from-color-spec "#1f83b0ADC5e2g") nil))
+  (should (equal (color-values-from-color-spec "#1f83b0ADC5e20") nil))
+  (should (equal (color-values-from-color-spec "#12345") nil))
+  (should (equal (color-values-from-color-spec "rgb:f/23/28a")
+                 '(#xffff #x2323 #x28a2)))
+  (should (equal (color-values-from-color-spec "rgb:1234/5678/09ab")
+                 '(#x1234 #x5678 #x09ab)))
+  (should (equal (color-values-from-color-spec "rgb:0//0") nil))
+  (should (equal (color-values-from-color-spec "rgbi:0/0.5/0.1")
+                 '(0 32768 6554)))
+  (should (equal (color-values-from-color-spec "rgbi:1e-3/1.0e-2/1e0")
+                 '(66 655 65535)))
+  (should (equal (color-values-from-color-spec "rgbi:0/0.5/10") nil)))
+
 (provide 'xfaces-tests)
-- 
2.21.1 (Apple Git-122.3)

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 13 Jun 2020 19:56:02 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> > But it isn't rejected by the current code.  Which was my point all
> > along.
> 
> Since "#12345" is malformed it should be rejected, and will be.

That is definitely a change in behavior, isn't it?  For some reason,
you don't regard this point I'm making seriously.  Why not?

> Use a single parser of colour strings in the #RGB, rgb:R/G/B and
                         ^^^^^^
> rgbi:R/G/B formats, replacing four existing ones.  Previously,
> error-checking was spotty, handling of the rgbi: format not always
> present, and normalisation of the result was sometimes incorrect.
               ^^^^^^^^^^^^^
Our convention is to use the US English spelling.

> +/* Parse fractional hex digits at S ending right before E.
> +   Set *DST to the value normalised to 65535 and return true on success,
> +   false otherwise.  */  ^^^^^^^^^^

Likewise here.

Please also describe in more detail the value put in *DST, I don't
think it's clear enough.

> +/* Parse floating-point number at S ending right before E.
> +   Return the number if in the range [0,1]; otherwise -1.  */
> +static double
> +parse_float_comp (const char *s, const char *e)

The commentary doesn't explain what is the "comp" part of the name
about.

> +/* Parse S as a numeric colour specification and set *R, *G and *B.
                           ^^^^^^
Spelling again.

> +   The result is normalised to a maximum value of 65535 per component.  */
                    ^^^^^^^^^^
And here.

> +DEFUN ("color-values-from-color-spec",
> +       Fcolor_values_from_color_spec, Scolor_values_from_color_spec,
> +       1, 1, 0,
> +       doc: /* Parse STRING as a numeric colour and return (R G B).
                                            ^^^^^^
> +Recognised formats are:
   ^^^^^^^^^^
> + #RGB, where R, G and B are hex strings of equal length, 1-4 digits each
> + rgb:R/G/B, where R, G, and B are hex strings, 1-4 digits each
> + rgbi:R/G/B, where R, G and B are numbers in [0,1].
> +
> +The result is normalised to a maximum value of 65535 per component.
                 ^^^^^^^^^^
> +If STRING is not in one of the above forms, return nil.  */)

Spelling.

I think this doc string is too terse.  I would rephrase the beginning
as follows:

    Convert a color SPEC into a list of standard RGB values.

  Value is a list of the form (R G B), where R, G, and B are the
  integer values, the intensities of the primary colors.
  The argument SPEC should be a string in one of the following formats:

In the "rgbi" description, I think we should mention explicitly that
the components are floating-point numbers.

> +  return (parse_color_spec (SSDATA (string), &r, &g, &b)
> +          ? list3i (r, g, b)
> +          : Qnil);

What happens if the argument is not a string?  What should happen?

Finally, the Lisp primitive needs a NEWS entry and perhaps also a
place in the ELisp manual.

Thanks.

Re: master 64e25cd: More robust NS hex colour string parsing

[Basil L. Contovounesios]

Mattias Engdegård <mattiase@acm.org> writes:

> Updated patch attached, with the name change, and the warning found by Basil fixed.

Tested with list-colors-display, (set-background-color "#f0f"),
and (set-background-color "#12345"), and things seems fine here.

-- 
Basil

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2020-06-13 built on thunk
Repository revision: b714a333c8420cf8a3824f62f071fd73ebf9afdb
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O2 -march=native' --config-cache
 --prefix=/home/blc/.local --with-x-toolkit=lucid
 --with-file-notification=yes --with-x'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT
LIBOTF ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

13 juni 2020 kl. 20.40 skrev Eli Zaretskii <eliz@gnu.org>:

>> Since "#12345" is malformed it should be rejected, and will be.
> 
> That is definitely a change in behavior, isn't it?

Of course -- it is a bug fix, and as such a change in behaviour by definition.
It was one of the prime motivations behind these changes: I really had it after writing #12345 etc and getting nonsense values.
Note that this particular input is already firmly rejected by several if not all of the other backends; it isn't as if it provides a reliably mechanism that anyone is likely to use.

> Our convention is to use the US English spelling.

Replaced everywhere. (Misspelling on purpose is difficult!)

> Please also describe in more detail the value put in *DST, I don't
> think it's clear enough.

Elaborated.

> The commentary doesn't explain what is the "comp" part of the name
> about.

Names extended to ..._color_comp and comments amended to indicate that they parse a colour component.

> I think this doc string is too terse.  I would rephrase the beginning
> as follows:
> 
>    Convert a color SPEC into a list of standard RGB values.
> 
>  Value is a list of the form (R G B), where R, G, and B are the
>  integer values, the intensities of the primary colors.
>  The argument SPEC should be a string in one of the following formats:

Thank you, but I don't think we need to describe how RGB works here. I've provided more detail about the return value according to your example in other respects.

> In the "rgbi" description, I think we should mention explicitly that
> the components are floating-point numbers.

Done.

> What happens if the argument is not a string?  What should happen?

Thanks for catching that! An accidental omission.

> Finally, the Lisp primitive needs a NEWS entry and perhaps also a
> place in the ELisp manual.

Actually, now that you made me think of it, I'd rather make it internal since it isn't intended for general use; it's only exported to Lisp because one backend (TTY) needs to access it that way. In the attached patch, it has an internal- prefix. Not extremely important either way, of course.

(Or should it be internal-- with a double hyphen? Not sure how we do it nowadays.)

> Thanks.

Thanks for reviewing again! Oh, and if you (or someone else) could confirm that it compiles and works on Windows, I'd be most grateful.


[-- Attachment #2: 0001-Consolidate-RGB-string-parsers.patch --]
[-- Type: application/octet-stream, Size: 20730 bytes --]

From ae60457ab0711216b8dd03c6a3c5c23238ad624c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 12 Jun 2020 18:12:37 +0200
Subject: [PATCH] Consolidate #RGB string parsers

Use a single parser of color strings in the #RGB, rgb:R/G/B and
rgbi:R/G/B formats, replacing four existing ones.  Previously,
error-checking was spotty, handling of the rgbi: format not always
present, and normalization of the result was sometimes incorrect.

* src/dispextern.h: New prototype.
* src/xfaces.c (parse_hex_color_comp, parse_float_color_comp)
(parse_color_spec, Finternal-color_values_from_color_spec): New functions.
* test/src/xfaces-tests.el (xfaces-internal-color-values-from-color-spec):
New test.
* lisp/term/tty-colors.el (tty-color-standard-values):
Use internal-color-values-from-color-spec, replacing old parser.
* src/nsterm.m (ns_get_color):
* src/w32fns.c (x_to_w32_color):
* src/xterm.c (x_parse_color): Use parse_color_spec, replacing old
parsers.
(HEX_COLOR_NAME_LENGTH): Remove #define.
---
 lisp/term/tty-colors.el  |  58 +--------------
 src/dispextern.h         |   2 +
 src/nsterm.m             |  59 +++++----------
 src/w32fns.c             | 157 ++-------------------------------------
 src/xfaces.c             | 116 +++++++++++++++++++++++++++++
 src/xterm.c              |  51 ++++---------
 test/src/xfaces-tests.el |  23 ++++++
 7 files changed, 180 insertions(+), 286 deletions(-)

diff --git a/lisp/term/tty-colors.el b/lisp/term/tty-colors.el
index 39ca2d3627..73e2431822 100644
--- a/lisp/term/tty-colors.el
+++ b/lisp/term/tty-colors.el
@@ -923,62 +923,8 @@ tty-color-standard-values
 COLOR (see the info node `(emacs) Colors'), regardless of whether
 the terminal can display it, so the return value should be the
 same regardless of what display is being used."
-  (let ((len (length color)))
-    (cond ((and (>= len 4) ;; HTML/CSS/SVG-style "#XXYYZZ" color spec
-		(eq (aref color 0) ?#)
-		(member (aref color 1)
-			'(?0 ?1 ?2 ?3 ?4 ?5 ?6 ?7 ?8 ?9
-			     ?a ?b ?c ?d ?e ?f
-                             ?A ?B ?C ?D ?E ?F)))
-	   ;; Translate the string "#XXYYZZ" into a list of numbers
-	   ;; (XX YY ZZ), scaling each to the {0..65535} range.  This
-	   ;; follows the HTML color convention, where both "#fff" and
-	   ;; "#ffffff" represent the same color, white.
-	   (let* ((ndig (/ (- len 1) 3))
-		  (maxval (1- (ash 1 (* 4 ndig))))
-		  (i1 1)
-		  (i2 (+ i1 ndig))
-		  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-	     (list
-	      (/ (* (string-to-number
-		     (substring color i1 i2) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i2 i3) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i3 i4) 16)
-		    65535)
-		 maxval))))
-	  ((and (>= len 9) ;; X-style rgb:xx/yy/zz color spec
-		(string= (substring color 0 4) "rgb:"))
-	   ;; Translate the string "rgb:XX/YY/ZZ" into a list of
-	   ;; numbers (XX YY ZZ), scaling each to the {0..65535}
-	   ;; range.  "rgb:F/F/F" is white.
-	   (let* ((ndig (/ (- len 3) 3))
-		  (maxval (1- (ash 1 (* 4 (- ndig 1)))))
-		  (i1 4)
-		  (i2 (+ i1 ndig))
-		  (i3 (+ i2 ndig))
-                  (i4 (+ i3 ndig)))
-	     (list
-	      (/ (* (string-to-number
-		     (substring color i1 (- i2 1)) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i2 (- i3 1)) 16)
-		    65535)
-		 maxval)
-	      (/ (* (string-to-number
-		     (substring color i3 (1- i4)) 16)
-		    65535)
-		 maxval))))
-	  (t
-	   (cdr (assoc color color-name-rgb-alist))))))
+  (or (internal-color-values-from-color-spec color)
+      (cdr (assoc color color-name-rgb-alist))))
 
 (defun tty-color-translate (color &optional frame)
   "Given a color COLOR, return the index of the corresponding TTY color.
diff --git a/src/dispextern.h b/src/dispextern.h
index 0b1f3d14ae..e1d6eddc41 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3514,6 +3514,8 @@ #define RGB_PIXEL_COLOR COLORREF
                                        Lisp_Object);
 extern bool tty_defined_color (struct frame *, const char *, Emacs_Color *,
                                bool, bool);
+bool parse_color_spec (const char *,
+                       unsigned short *, unsigned short *, unsigned short *);
 
 Lisp_Object tty_color_name (struct frame *, int);
 void clear_face_cache (bool);
diff --git a/src/nsterm.m b/src/nsterm.m
index 3dc7e1db7c..0e405fc017 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2341,9 +2341,6 @@ so some key presses (TAB) are swallowed by the system.  */
    See https://lists.gnu.org/r/emacs-devel/2009-07/msg01203.html.  */
 {
   NSColor *new = nil;
-  static char hex[20];
-  int scaling = 0;
-  float r = -1.0, g, b;
   NSString *nsname = [NSString stringWithUTF8String: name];
 
   NSTRACE ("ns_get_color(%s, **)", name);
@@ -2386,51 +2383,31 @@ so some key presses (TAB) are swallowed by the system.  */
     }
 
   /* First, check for some sort of numeric specification.  */
-  hex[0] = '\0';
-
-  if (name[0] == '0' || name[0] == '1' || name[0] == '.')  /* RGB decimal */
+  unsigned short r16, g16, b16;
+  if (parse_color_spec (name, &r16, &g16, &b16))
     {
-      NSScanner *scanner = [NSScanner scannerWithString: nsname];
-      [scanner scanFloat: &r];
-      [scanner scanFloat: &g];
-      [scanner scanFloat: &b];
-    }
-  else if (!strncmp(name, "rgb:", 4))  /* A newer X11 format -- rgb:r/g/b */
-    scaling = (snprintf (hex, sizeof hex, "%s", name + 4) - 2) / 3;
-  else if (name[0] == '#')        /* An old X11 format; convert to newer */
-    {
-      int len = 0;
-      while (isxdigit (name[len + 1]))
-        len++;
-      if (name[len + 1] == '\0' && len >= 1 && len <= 12 && len % 3 == 0)
-        {
-          scaling = len / 3;
-          for (int i = 0; i < 3; i++)
-            sprintf (hex + i * (scaling + 1), "%.*s/", scaling,
-                     name + 1 + i * scaling);
-          hex[3 * (scaling + 1) - 1] = '\0';
-        }
+      *col = [NSColor colorForEmacsRed: r16 / 65535.0
+                                 green: g16 / 65535.0
+                                  blue: b16 / 65535.0
+                                 alpha: 1.0];
+      unblock_input ();
+      return 0;
     }
-
-  if (hex[0])
+  else if (name[0] == '0' || name[0] == '1' || name[0] == '.')
     {
-      unsigned int rr, gg, bb;
-      float fscale = (1 << (scaling * 4)) - 1;
-      if (sscanf (hex, "%x/%x/%x", &rr, &gg, &bb))
+      /* RGB decimal */
+      NSScanner *scanner = [NSScanner scannerWithString: nsname];
+      float r, g, b;
+      if (   [scanner scanFloat: &r] && r >= 0 && r <= 1
+          && [scanner scanFloat: &g] && g >= 0 && g <= 1
+          && [scanner scanFloat: &b] && b >= 0 && b <= 1)
         {
-          r = rr / fscale;
-          g = gg / fscale;
-          b = bb / fscale;
+          *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
+          unblock_input ();
+          return 0;
         }
     }
 
-  if (r >= 0.0F)
-    {
-      *col = [NSColor colorForEmacsRed: r green: g blue: b alpha: 1.0];
-      unblock_input ();
-      return 0;
-    }
-
   /* Otherwise, color is expected to be from a list */
   {
     NSEnumerator *lenum, *cenum;
diff --git a/src/w32fns.c b/src/w32fns.c
index e595b0285a..ab864332e7 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -864,161 +864,14 @@ x_to_w32_color (const char * colorname)
 
   block_input ();
 
-  if (colorname[0] == '#')
+  unsigned short r, g, b;
+  if (parse_color_spec (colorname, &r, &g, &b))
     {
-      /* Could be an old-style RGB Device specification.  */
-      int size = strlen (colorname + 1);
-      char *color = alloca (size + 1);
-
-      strcpy (color, colorname + 1);
-      if (size == 3 || size == 6 || size == 9 || size == 12)
-	{
-	  UINT colorval;
-	  int i, pos;
-	  pos = 0;
-	  size /= 3;
-	  colorval = 0;
-
-	  for (i = 0; i < 3; i++)
-	    {
-	      char *end;
-	      char t;
-	      unsigned long value;
-
-	      /* The check for 'x' in the following conditional takes into
-		 account the fact that strtol allows a "0x" in front of
-		 our numbers, and we don't.  */
-	      if (!isxdigit (color[0]) || color[1] == 'x')
-		break;
-	      t = color[size];
-	      color[size] = '\0';
-	      value = strtoul (color, &end, 16);
-	      color[size] = t;
-	      if (errno == ERANGE || end - color != size)
-		break;
-	      switch (size)
-		{
-		case 1:
-		  value = value * 0x10;
-		  break;
-		case 2:
-		  break;
-		case 3:
-		  value /= 0x10;
-		  break;
-		case 4:
-		  value /= 0x100;
-		  break;
-		}
-	      colorval |= (value << pos);
-	      pos += 0x8;
-	      if (i == 2)
-		{
-		  unblock_input ();
-		  XSETINT (ret, colorval);
-		  return ret;
-		}
-	      color = end;
-	    }
-	}
-    }
-  else if (strnicmp (colorname, "rgb:", 4) == 0)
-    {
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 4;
-      for (i = 0; i < 3; i++)
-	{
-	  char *end;
-	  unsigned long value;
-
-	  /* The check for 'x' in the following conditional takes into
-	     account the fact that strtol allows a "0x" in front of
-	     our numbers, and we don't.  */
-	  if (!isxdigit (color[0]) || color[1] == 'x')
-	    break;
-	  value = strtoul (color, &end, 16);
-	  if (errno == ERANGE)
-	    break;
-	  switch (end - color)
-	    {
-	    case 1:
-	      value = value * 0x10 + value;
-	      break;
-	    case 2:
-	      break;
-	    case 3:
-	      value /= 0x10;
-	      break;
-	    case 4:
-	      value /= 0x100;
-	      break;
-	    default:
-	      value = ULONG_MAX;
-	    }
-	  if (value == ULONG_MAX)
-	    break;
-	  colorval |= (value << pos);
-	  pos += 0x8;
-	  if (i == 2)
-	    {
-	      if (*end != '\0')
-		break;
-	      unblock_input ();
-	      XSETINT (ret, colorval);
-	      return ret;
-	    }
-	  if (*end != '/')
-	    break;
-	  color = end + 1;
-	}
+      unblock_input ();
+      /* Throw away the low 8 bits and return 0xBBGGRR.  */
+      return make_fixnum ((b & 0xff00) << 8 | (g & 0xff00) | r >> 8);
     }
-  else if (strnicmp (colorname, "rgbi:", 5) == 0)
-    {
-      /* This is an RGB Intensity specification.  */
-      const char *color;
-      UINT colorval;
-      int i, pos;
-      pos = 0;
-
-      colorval = 0;
-      color = colorname + 5;
-      for (i = 0; i < 3; i++)
-	{
-	  char *end;
-	  double value;
-	  UINT val;
 
-	  value = strtod (color, &end);
-	  if (errno == ERANGE)
-	    break;
-	  if (value < 0.0 || value > 1.0)
-	    break;
-	  val = (UINT)(0x100 * value);
-	  /* We used 0x100 instead of 0xFF to give a continuous
-	     range between 0.0 and 1.0 inclusive.  The next statement
-	     fixes the 1.0 case.  */
-	  if (val == 0x100)
-	    val = 0xFF;
-	  colorval |= (val << pos);
-	  pos += 0x8;
-	  if (i == 2)
-	    {
-	      if (*end != '\0')
-		break;
-	      unblock_input ();
-	      XSETINT (ret, colorval);
-	      return ret;
-	    }
-	  if (*end != '/')
-	    break;
-	  color = end + 1;
-	}
-    }
   /* I am not going to attempt to handle any of the CIE color schemes
      or TekHVC, since I don't know the algorithms for conversion to
      RGB.  */
diff --git a/src/xfaces.c b/src/xfaces.c
index cf155288bd..308509a026 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -220,6 +220,7 @@ Copyright (C) 1993-1994, 1998-2020 Free Software Foundation, Inc.
 #include "sysstdio.h"
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <math.h>
 
 #include "lisp.h"
 #include "character.h"
@@ -819,6 +820,120 @@ load_pixmap (struct frame *f, Lisp_Object name)
                             Color Handling
  ***********************************************************************/
 
+/* Parse hex color component at S ending right before E.
+   Set *DST to the value normalized so that the maximum for the
+   number of digits given becomes 65535, and return true on success,
+   false otherwise.  */
+static bool
+parse_hex_color_comp (const char *s, const char *e, unsigned short *dst)
+{
+  int n = e - s;
+  if (n <= 0 || n > 4)
+    return false;
+  int val = 0;
+  for (; s < e; s++)
+    {
+      int digit;
+      if (*s >= '0' && *s <= '9')
+        digit = *s - '0';
+      else if (*s >= 'A' && *s <= 'F')
+        digit = *s - 'A' + 10;
+      else if (*s >= 'a' && *s <= 'f')
+        digit = *s - 'a' + 10;
+      else
+        return false;
+      val = (val << 4) | digit;
+    }
+  int maxval = (1 << (n * 4)) - 1;
+  *dst = (unsigned)val * 65535 / maxval;
+  return true;
+}
+
+/* Parse floating-point color component at S ending right before E.
+   Return the number if in the range [0,1]; otherwise -1.  */
+static double
+parse_float_color_comp (const char *s, const char *e)
+{
+  char *end;
+  double x = strtod (s, &end);
+  return (end == e && x >= 0 && x <= 1) ? x : -1;
+}
+
+/* Parse S as a numeric color specification and set *R, *G and *B.
+   Return true on success, false on failure.
+   Recognized formats:
+
+    "#RGB", with R, G and B hex strings of equal length, 1-4 digits each
+    "rgb:R/G/B", with R, G and B hex strings, 1-4 digits each
+    "rgbi:R/G/B", with R, G and B numbers in [0,1]
+
+   The result is normalized to a maximum value of 65535 per component.  */
+bool
+parse_color_spec (const char *s,
+                  unsigned short *r, unsigned short *g, unsigned short *b)
+{
+  int len = strlen (s);
+  if (s[0] == '#')
+    {
+      if ((len - 1) % 3 == 0)
+        {
+          int n = (len - 1) / 3;
+          return (   parse_hex_color_comp (s + 1 + 0 * n, s + 1 + 1 * n, r)
+                  && parse_hex_color_comp (s + 1 + 1 * n, s + 1 + 2 * n, g)
+                  && parse_hex_color_comp (s + 1 + 2 * n, s + 1 + 3 * n, b));
+        }
+    }
+  else if (strncmp (s, "rgb:", 4) == 0)
+    {
+      char *sep1, *sep2;
+      return ((sep1 = strchr (s + 4, '/')) != NULL
+              && (sep2 = strchr (sep1 + 1, '/')) != NULL
+              && parse_hex_color_comp (s + 4, sep1, r)
+              && parse_hex_color_comp (sep1 + 1, sep2, g)
+              && parse_hex_color_comp (sep2 + 1, s + len, b));
+    }
+  else if (strncmp (s, "rgbi:", 5) == 0)
+    {
+      char *sep1, *sep2;
+      double red, green, blue;
+      if ((sep1 = strchr (s + 5, '/')) != NULL
+          && (sep2 = strchr (sep1 + 1, '/')) != NULL
+          && (red = parse_float_color_comp (s + 5, sep1)) >= 0
+          && (green = parse_float_color_comp (sep1 + 1, sep2)) >= 0
+          && (blue = parse_float_color_comp (sep2 + 1, s + len)) >= 0)
+        {
+          *r = lrint (red * 65535);
+          *g = lrint (green * 65535);
+          *b = lrint (blue * 65535);
+          return true;
+        }
+    }
+  return false;
+}
+
+DEFUN ("internal-color-values-from-color-spec",
+       Finternal_color_values_from_color_spec,
+       Sinternal_color_values_from_color_spec,
+       1, 1, 0,
+       doc: /* Parse STRING as a numeric color and return (RED GREEN BLUE).
+Recognised formats for STRING are:
+
+ #RGB, where R, G and B are hex numbers of equal length, 1-4 digits each
+ rgb:R/G/B, where R, G, and B are hex numbers, 1-4 digits each
+ rgbi:R/G/B, where R, G and B are floating-point numbers in [0,1]
+
+The result is normalized to a maximum value of 65535 per component,
+forming a list of three integers in [0,65535].
+If STRING is not in one of the above forms, return nil.  */)
+  (Lisp_Object string)
+{
+  CHECK_STRING (string);
+  unsigned short r, g, b;
+  return (parse_color_spec (SSDATA (string), &r, &g, &b)
+          ? list3i (r, g, b)
+          : Qnil);
+}
+
 /* Parse RGB_LIST, and fill in the RGB fields of COLOR.
    RGB_LIST should contain (at least) 3 lisp integers.
    Return true iff RGB_LIST is OK.  */
@@ -7018,4 +7133,5 @@ syms_of_xfaces (void)
   defsubr (&Sinternal_face_x_get_resource);
   defsubr (&Sx_family_fonts);
 #endif
+  defsubr (&Sinternal_color_values_from_color_spec);
 }
diff --git a/src/xterm.c b/src/xterm.c
index 7989cecec7..6340700cb8 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -2376,8 +2376,6 @@ x_query_frame_background_color (struct frame *f, XColor *bgcolor)
   x_query_colors (f, bgcolor, 1);
 }
 
-#define HEX_COLOR_NAME_LENGTH 32
-
 /* On frame F, translate the color name to RGB values.  Use cached
    information, if possible.
 
@@ -2389,44 +2387,23 @@ #define HEX_COLOR_NAME_LENGTH 32
 Status x_parse_color (struct frame *f, const char *color_name,
 		      XColor *color)
 {
+  /* Don't pass #RGB strings directly to XParseColor, because that
+     follows the X convention of zero-extending each channel
+     value: #f00 means #f00000.  We want the convention of scaling
+     channel values, so #f00 means #ff0000, just as it does for
+     HTML, SVG, and CSS.  */
+  unsigned short r, g, b;
+  if (parse_color_spec (color_name, &r, &g, &b))
+    {
+      color->red = r;
+      color->green = g;
+      color->blue = b;
+      return 1;
+    }
+
   Display *dpy = FRAME_X_DISPLAY (f);
   Colormap cmap = FRAME_X_COLORMAP (f);
   struct color_name_cache_entry *cache_entry;
-
-  if (color_name[0] == '#')
-    {
-      /* Don't pass #RGB strings directly to XParseColor, because that
-	 follows the X convention of zero-extending each channel
-	 value: #f00 means #f00000.  We want the convention of scaling
-	 channel values, so #f00 means #ff0000, just as it does for
-	 HTML, SVG, and CSS.
-
-	 So we translate #f00 to rgb:f/0/0, which X handles
-	 differently. */
-      char rgb_color_name[HEX_COLOR_NAME_LENGTH];
-      int len = strlen (color_name);
-      int digits_per_channel;
-      if (len == 4)
-	digits_per_channel = 1;
-      else if (len == 7)
-	digits_per_channel = 2;
-      else if (len == 10)
-	digits_per_channel = 3;
-      else if (len == 13)
-	digits_per_channel = 4;
-      else
-	return 0;
-
-      snprintf (rgb_color_name, sizeof rgb_color_name, "rgb:%.*s/%.*s/%.*s",
-		digits_per_channel, color_name + 1,
-		digits_per_channel, color_name + digits_per_channel + 1,
-		digits_per_channel, color_name + 2 * digits_per_channel + 1);
-
-      /* The rgb form is parsed directly by XParseColor without
-	 talking to the X server.  No need for caching.  */
-      return XParseColor (dpy, cmap, rgb_color_name, color);
-    }
-
   for (cache_entry = FRAME_DISPLAY_INFO (f)->color_names; cache_entry;
        cache_entry = cache_entry->next)
     {
diff --git a/test/src/xfaces-tests.el b/test/src/xfaces-tests.el
index 5ed16c9e51..34cda07e5b 100644
--- a/test/src/xfaces-tests.el
+++ b/test/src/xfaces-tests.el
@@ -24,4 +24,27 @@ xfaces-color-distance
   (should (equal (color-distance "#222222" "#ffffff")
                  (color-distance "#ffffff" "#222222"))))
 
+(ert-deftest xfaces-internal-color-values-from-color-spec ()
+  (should (equal (internal-color-values-from-color-spec "#f05")
+                 '(#xffff #x0000 #x5555)))
+  (should (equal (internal-color-values-from-color-spec "#1fb0C5")
+                 '(#x1f1f #xb0b0 #xc5c5)))
+  (should (equal (internal-color-values-from-color-spec "#1f8b0AC5e")
+                 '(#x1f81 #xb0aa #xc5eb)))
+  (should (equal (internal-color-values-from-color-spec "#1f83b0ADC5e2")
+                 '(#x1f83 #xb0ad #xc5e2)))
+  (should (equal (internal-color-values-from-color-spec "#1f83b0ADC5e2g") nil))
+  (should (equal (internal-color-values-from-color-spec "#1f83b0ADC5e20") nil))
+  (should (equal (internal-color-values-from-color-spec "#12345") nil))
+  (should (equal (internal-color-values-from-color-spec "rgb:f/23/28a")
+                 '(#xffff #x2323 #x28a2)))
+  (should (equal (internal-color-values-from-color-spec "rgb:1234/5678/09ab")
+                 '(#x1234 #x5678 #x09ab)))
+  (should (equal (internal-color-values-from-color-spec "rgb:0//0") nil))
+  (should (equal (internal-color-values-from-color-spec "rgbi:0/0.5/0.1")
+                 '(0 32768 6554)))
+  (should (equal (internal-color-values-from-color-spec "rgbi:1e-3/1.0e-2/1e0")
+                 '(66 655 65535)))
+  (should (equal (internal-color-values-from-color-spec "rgbi:0/0.5/10") nil)))
+
 (provide 'xfaces-tests)
-- 
2.21.1 (Apple Git-122.3)

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

15 juni 2020 kl. 10.31 skrev Mattias Engdegård <mattiase@acm.org>:

> <0001-Consolidate-RGB-string-parsers.patch>

Is the patch acceptable, or does anything need to be altered? It seems that we are nearly done.

Re: master 64e25cd: More robust NS hex colour string parsing

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 21 Jun 2020 09:48:24 +0200
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> 
> 15 juni 2020 kl. 10.31 skrev Mattias Engdegård <mattiase@acm.org>:
> 
> > <0001-Consolidate-RGB-string-parsers.patch>
> 
> Is the patch acceptable, or does anything need to be altered? It seems that we are nearly done.

I have no further comments.

Re: master 64e25cd: More robust NS hex colour string parsing

[Mattias Engdegård]

21 juni 2020 kl. 16.59 skrev Eli Zaretskii <eliz@gnu.org>:

>> Is the patch acceptable, or does anything need to be altered? It seems that we are nearly done.
> 
> I have no further comments.

Thank you, pushed to master.