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

["Mattias Engdegård" <mattiase@acm.org>]

[-- 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)