r-devel / r-project-sprint-2023

Material for the R project sprint
https://contributor.r-project.org/r-project-sprint-2023/
17 stars 3 forks source link

3-digit hex colors #74

Closed pmur002 closed 1 year ago

pmur002 commented 1 year ago

Discussed in https://github.com/r-devel/r-project-sprint-2023/discussions/73

Originally posted by **pmur002** August 30, 2023 A [post on fosstodon](https://fosstodon.org/@coolbutuseless/110972696890598490) suggests adding support for `"#RGB"` colours in R (in addition to existing `"#RRGGBB"` colours. This may not be an urgent need and might be a small change, but it would still require the complete modify, build, test, check, document process to be covered. And that is assuming there are no unintended consequences ...
malcolmbarrett commented 1 year ago

patch

Index: src/library/grDevices/man/col2rgb.Rd
===================================================================
--- src/library/grDevices/man/col2rgb.Rd        (revision 85029)
+++ src/library/grDevices/man/col2rgb.Rd        (working copy)
@@ -15,9 +15,8 @@
 \arguments{
   \item{col}{vector of any of the three kinds of \R color specifications,
     i.e., either a color name (as listed by \code{\link{colors}()}), a
-    hexadecimal string of the form \code{"#rrggbb"} or
-    \code{"#rrggbbaa"} (see \code{\link{rgb}}), or a positive integer
-    \code{i} meaning \code{\link{palette}()[i]}.}
+    hexadecimal string of the form specified in \code{\link{rgb}}, or a
+    positive integer \code{i} meaning \code{\link{palette}()[i]}.}
   \item{alpha}{logical value indicating whether the alpha channel (opacity)
     values should be returned.}
 }
@@ -58,6 +57,11 @@
 col2rgb(c(red = "red", hex = "#abcdef"))
 col2rgb(c(palette = 1:3))

+# long and short form of hexadecimal notation
+col2rgb(c(long = "#559955", short = "#595"))
+# with alpha
+col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+
 ##-- NON-INTRODUCTORY examples --

 grC <- col2rgb(paste0("gray", 0:100))
Index: src/library/grDevices/man/rgb.Rd
===================================================================
--- src/library/grDevices/man/rgb.Rd    (revision 85029)
+++ src/library/grDevices/man/rgb.Rd    (working copy)
@@ -57,11 +57,16 @@
   \code{green} or \code{alpha}.
 }
 \value{
-  A character vector with elements of 7 or 9 characters, \code{"#"}
-  followed by the red, blue, green and optionally alpha values in
-  hexadecimal (after rescaling to \code{0 ... 255}).  The optional alpha
-  values range from \code{0} (fully transparent) to \code{255} (opaque).
+  A character vector beginning with \code{"#"} followed by the red, 
+  blue, green and optionally alpha values in hexadecimal (after 
+  rescaling to \code{0 ... 255}).  The optional alpha values range 
+  from \code{0} (fully transparent) to \code{255} (opaque). 

+  This can be in the long hexadecimal form (e.g., \code{"#rrggbb"} or
+  \code{"#rrggbbaa"}) or the short form (e.g, \code{"#rgb"} or
+  \code{"#rgba"}). The short form is expanded to the long form by 
+  replicating digits (not by adding zeroes).
+
   \R does \strong{not} use \sQuote{premultiplied alpha}.
 }
 \seealso{
Index: src/library/grDevices/src/colors.c
===================================================================
--- src/library/grDevices/src/colors.c  (revision 85029)
+++ src/library/grDevices/src/colors.c  (working copy)
@@ -1359,13 +1359,24 @@
        g = 16 * hexdigit(rgb[3]) + hexdigit(rgb[4]);
        b = 16 * hexdigit(rgb[5]) + hexdigit(rgb[6]);
        break;
+    case 5: 
+       a = 17 * hexdigit(rgb[4]);
+    case 4:
+       r = 17 * hexdigit(rgb[1]);
+       g = 17 * hexdigit(rgb[2]);
+       b = 17 * hexdigit(rgb[3]);
+       break;
     default:
        error(_("invalid RGB specification"));
     }
-    if (strlen(rgb) == 7)
-       return R_RGB(r, g, b);
-    else
-       return R_RGBA(r, g, b, a);
+
+    switch(strlen(rgb)) {
+    case 7: 
+    case 4:
+        return R_RGB(r, g, b);
+    default:
+        return R_RGBA(r, g, b, a);
+    }
 }

 /* External Color Name to Internal Color Code */

test output

Unsurprisingly, the example tests now fail

Testing examples for package ‘grDevices’
  comparing ‘grDevices-Ex.Rout’ to ‘grDevices-Ex.Rout.save’ ... NOTE
  696,709d695
  < > # long and short form of hexadecimal notation
  < > col2rgb(c(long = "#559955", short = "#595"))
  <       long short
  < red     85    85
  < green  153   153
  < blue    85    85
  < > # with alpha
  < > col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
  <       long short
  < red     85    85
  < green  153   153
  < blue    85    85
  < alpha  187   187
  < > 

Is this the sort of thing that should be included in the patch?

make check-all otherwise gave us an OK status. We did see test differences for other parts of the codebase we didn't touch. Is that expected?

bastistician commented 1 year ago

Just commenting on

Is this the sort of thing that should be included in the patch?

Especially if a patch changes reference output of examples or tests, I think it is very useful to include its diff with the patch. Anecdotally, when I was working on Rd conversion bugs, I often created two svn commits, one that just adds a test case and the pre-patch reference outputs from Rd conversion, and a second commit that fixes the bug and includes the diff of the reference output, so that the effect of the bug fix is very clear from that commit.

In this case here, new examples are added and, in my opinion, also then, the diff of the .Rout.save file is a useful thing to include in the provided patch, making it more self-explanatory because example output is included.

MichaelChirico commented 1 year ago

The short form is expanded to the long form by replicating digits (not by adding zeroes).

I might clarify with an example:

The short form is expanded to the long form by replicating digits (not by adding zeroes, e.g. #abc becomes #aabbcc).

MichaelChirico commented 1 year ago

17 * hexdigit(rgb[4])

For future reader sanity's sake, I might add a comment clarifying the magic number 17 (it's "obvious" in the context of this issue, but that context is not evident in the source itself)

malcolmbarrett commented 1 year ago

Thanks for the suggestions @MichaelChirico! I've included them

@bastistician thank you as well for the clarification. One nuisance I bumped into is that there are other differences in that output file from the changes we made (I guess other things are different that are not incorporated into the test snapshots yet?). So, just running a diff gives some extra output that's not relevant to the patch. Would you recommend hand editing the file or diff to avoid that?

Here's the current diff. The test output component is towards the bottom, as are all of the irrelevant lines

Index: src/library/grDevices/man/col2rgb.Rd
===================================================================
--- src/library/grDevices/man/col2rgb.Rd        (revision 85029)
+++ src/library/grDevices/man/col2rgb.Rd        (working copy)
@@ -15,9 +15,8 @@
 \arguments{
   \item{col}{vector of any of the three kinds of \R color specifications,
     i.e., either a color name (as listed by \code{\link{colors}()}), a
-    hexadecimal string of the form \code{"#rrggbb"} or
-    \code{"#rrggbbaa"} (see \code{\link{rgb}}), or a positive integer
-    \code{i} meaning \code{\link{palette}()[i]}.}
+    hexadecimal string of the form specified in \code{\link{rgb}}, or a
+    positive integer \code{i} meaning \code{\link{palette}()[i]}.}
   \item{alpha}{logical value indicating whether the alpha channel (opacity)
     values should be returned.}
 }
@@ -58,6 +57,11 @@
 col2rgb(c(red = "red", hex = "#abcdef"))
 col2rgb(c(palette = 1:3))

+# long and short form of hexadecimal notation
+col2rgb(c(long = "#559955", short = "#595"))
+# with alpha
+col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+
 ##-- NON-INTRODUCTORY examples --

 grC <- col2rgb(paste0("gray", 0:100))
Index: src/library/grDevices/man/rgb.Rd
===================================================================
--- src/library/grDevices/man/rgb.Rd    (revision 85029)
+++ src/library/grDevices/man/rgb.Rd    (working copy)
@@ -57,11 +57,17 @@
   \code{green} or \code{alpha}.
 }
 \value{
-  A character vector with elements of 7 or 9 characters, \code{"#"}
-  followed by the red, blue, green and optionally alpha values in
-  hexadecimal (after rescaling to \code{0 ... 255}).  The optional alpha
-  values range from \code{0} (fully transparent) to \code{255} (opaque).
+  A character vector beginning with \code{"#"} followed by the red, 
+  blue, green and optionally alpha values in hexadecimal (after 
+  rescaling to \code{0 ... 255}).  The optional alpha values range 
+  from \code{0} (fully transparent) to \code{255} (opaque). 

+  This can be in the long hexadecimal form (e.g., \code{"#rrggbb"} or
+  \code{"#rrggbbaa"}) or the short form (e.g, \code{"#rgb"} or
+  \code{"#rgba"}). The short form is expanded to the long form by 
+  replicating digits (not by adding zeroes), e.g., \code{"#rgb"} becomes
+  \code{"#rrggbb"}.
+
   \R does \strong{not} use \sQuote{premultiplied alpha}.
 }
 \seealso{
Index: src/library/grDevices/src/colors.c
===================================================================
--- src/library/grDevices/src/colors.c  (revision 85029)
+++ src/library/grDevices/src/colors.c  (working copy)
@@ -1345,7 +1345,7 @@
 }

-/* #RRGGBB[AA] String to Internal Color Code */
+/* #RRGGBB[AA] or #RGB[A] String to Internal Color Code */
 static rcolor rgb2col(const char *rgb)
 {
     unsigned int r = 0, g = 0, b = 0, a = 0; /* -Wall */
@@ -1359,13 +1359,26 @@
        g = 16 * hexdigit(rgb[3]) + hexdigit(rgb[4]);
        b = 16 * hexdigit(rgb[5]) + hexdigit(rgb[6]);
        break;
+    case 5: 
+    // calculate using 17 because each hex digit is included twice when expanding
+    // from the short-form hex code to the long-form 
+       a = 17 * hexdigit(rgb[4]);
+    case 4:
+       r = 17 * hexdigit(rgb[1]);
+       g = 17 * hexdigit(rgb[2]);
+       b = 17 * hexdigit(rgb[3]);
+       break;
     default:
        error(_("invalid RGB specification"));
     }
-    if (strlen(rgb) == 7)
-       return R_RGB(r, g, b);
-    else
-       return R_RGBA(r, g, b, a);
+
+    switch(strlen(rgb)) {
+    case 7: 
+    case 4:
+        return R_RGB(r, g, b);
+    default:
+        return R_RGBA(r, g, b, a);
+    }
 }

 /* External Color Name to Internal Color Code */
Index: tests/Examples/grDevices-Ex.Rout.save
===================================================================
--- tests/Examples/grDevices-Ex.Rout.save       (revision 85029)
+++ tests/Examples/grDevices-Ex.Rout.save       (working copy)
@@ -1,5 +1,5 @@

-R Under development (unstable) (2023-07-31 r84791) -- "Unsuffered Consequences"
+R Under development (unstable) (2023-08-28 r85029) -- "Unsuffered Consequences"
 Copyright (C) 2023 The R Foundation for Statistical Computing
 Platform: x86_64-pc-linux-gnu

@@ -716,6 +716,20 @@
 green        0       83      208
 blue         0      107       79
 > 
+> # long and short form of hexadecimal notation
+> col2rgb(c(long = "#559955", short = "#595"))
+      long short
+red     85    85
+green  153   153
+blue    85    85
+> # with alpha
+> col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+      long short
+red     85    85
+green  153   153
+blue    85    85
+alpha  187   187
+> 
 > ##-- NON-INTRODUCTORY examples --
 > 
 > grC <- col2rgb(paste0("gray", 0:100))
@@ -1526,7 +1540,7 @@
 > stopifnot(setequal(cNms[isC],
 +      c("white","black","blue","cyan","green","magenta","red","yellow")))
 > ## End(Don't show)
-> table(is.gray <- tc[,1] == tc[,2] & tc[,2] == tc[,3])# (397, 105)
+> table(is.gray <- tc[,1] == tc[,2] & tc[,2] == tc[,3])  # (397, 105)

 FALSE  TRUE 
   397   105 
@@ -3584,15 +3598,15 @@
 > ## time-dependent ==> ignore diffs:
 > ## IGNORE_RDIFF_BEGIN
 > pretty(Sys.Date())
-[1] "2023-07-29" "2023-07-30" "2023-07-31" "2023-08-01" "2023-08-02"
-[6] "2023-08-03"
+[1] "2023-08-28" "2023-08-29" "2023-08-30" "2023-08-31" "2023-09-01"
+[6] "2023-09-02"
 > pretty(Sys.time(), n = 10)
- [1] "2023-07-31 10:42:05 CEST" "2023-07-31 10:42:06 CEST"
- [3] "2023-07-31 10:42:07 CEST" "2023-07-31 10:42:08 CEST"
- [5] "2023-07-31 10:42:09 CEST" "2023-07-31 10:42:10 CEST"
- [7] "2023-07-31 10:42:11 CEST" "2023-07-31 10:42:12 CEST"
- [9] "2023-07-31 10:42:13 CEST" "2023-07-31 10:42:14 CEST"
-[11] "2023-07-31 10:42:15 CEST"
+ [1] "2023-08-30 13:25:39 UTC" "2023-08-30 13:25:40 UTC"
+ [3] "2023-08-30 13:25:41 UTC" "2023-08-30 13:25:42 UTC"
+ [5] "2023-08-30 13:25:43 UTC" "2023-08-30 13:25:44 UTC"
+ [7] "2023-08-30 13:25:45 UTC" "2023-08-30 13:25:46 UTC"
+ [9] "2023-08-30 13:25:47 UTC" "2023-08-30 13:25:48 UTC"
+[11] "2023-08-30 13:25:49 UTC"
 > ## IGNORE_RDIFF_END
 > 
 > pretty(as.Date("2000-03-01")) # R 1.0.0 came in a leap year
@@ -4106,40 +4120,6 @@
 + utopia <- X11Font("-*-utopia-*-*-*-*-*-*-*-*-*-*-*-*")
 + X11Fonts(utopia = utopia)
 + })
-> X11Fonts()
-$serif
-[1] "-*-times-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$sans
-[1] "-*-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$mono
-[1] "-*-courier-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Times
-[1] "-adobe-times-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Helvetica
-[1] "-adobe-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$CyrTimes
-[1] "-cronyx-times-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$CyrHelvetica
-[1] "-cronyx-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Arial
-[1] "-monotype-arial-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Mincho
-[1] "-*-mincho-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-> X11Fonts("mono")
-$mono
-[1] "-*-courier-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-> utopia <- X11Font("-*-utopia-*-*-*-*-*-*-*-*-*-*-*-*")
-> X11Fonts(utopia = utopia)
 > ## IGNORE_RDIFF_END
 > 
 > 
@@ -4424,7 +4404,7 @@
 > cleanEx()
 > options(digits = 7L)
 > base::cat("Time elapsed: ", proc.time() - base::get("ptime", pos = 'CheckExEnv'),"\n")
-Time elapsed:  38.446 0.619 39.236 0 0 
+Time elapsed:  31.186 0.549 31.768 0 0 
 > grDevices::dev.off()
 null device 
           1 
hanneoberman commented 1 year ago

Comment on line 1348 should be adjusted to match the short form hex:

/* #RRGGBB[AA] String to Internal Color Code */

Which might be a good place to explain the 17 too?

malcolmbarrett commented 1 year ago

Good point @hanneoberman. I'll just update the above diff since it's a minor change

EllaKaye commented 1 year ago

r.e. the 17, is this better dealt with as a comment, or editing the code itself for case 4: to e.g. r = 16 * hexdigit(rgb[1]) + hexdigit(rgb[1]);? This makes it less 'magic' and echos the corresponding line for case 7:

malcolmbarrett commented 1 year ago

Maybe that should be the comment, that they're equivalent? Then you can see it without the (admittedly minor) extra function call

EllaKaye commented 1 year ago

@malcolmbarrett That seems like a good balance!

malcolmbarrett commented 1 year ago

Ok here is the diff with all the feedback thus far

Index: src/library/grDevices/man/col2rgb.Rd
===================================================================
--- src/library/grDevices/man/col2rgb.Rd        (revision 85029)
+++ src/library/grDevices/man/col2rgb.Rd        (working copy)
@@ -15,9 +15,8 @@
 \arguments{
   \item{col}{vector of any of the three kinds of \R color specifications,
     i.e., either a color name (as listed by \code{\link{colors}()}), a
-    hexadecimal string of the form \code{"#rrggbb"} or
-    \code{"#rrggbbaa"} (see \code{\link{rgb}}), or a positive integer
-    \code{i} meaning \code{\link{palette}()[i]}.}
+    hexadecimal string of the form specified in \code{\link{rgb}}, or a
+    positive integer \code{i} meaning \code{\link{palette}()[i]}.}
   \item{alpha}{logical value indicating whether the alpha channel (opacity)
     values should be returned.}
 }
@@ -58,6 +57,11 @@
 col2rgb(c(red = "red", hex = "#abcdef"))
 col2rgb(c(palette = 1:3))

+# long and short form of hexadecimal notation
+col2rgb(c(long = "#559955", short = "#595"))
+# with alpha
+col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+
 ##-- NON-INTRODUCTORY examples --

 grC <- col2rgb(paste0("gray", 0:100))
Index: src/library/grDevices/man/rgb.Rd
===================================================================
--- src/library/grDevices/man/rgb.Rd    (revision 85029)
+++ src/library/grDevices/man/rgb.Rd    (working copy)
@@ -57,11 +57,17 @@
   \code{green} or \code{alpha}.
 }
 \value{
-  A character vector with elements of 7 or 9 characters, \code{"#"}
-  followed by the red, blue, green and optionally alpha values in
-  hexadecimal (after rescaling to \code{0 ... 255}).  The optional alpha
-  values range from \code{0} (fully transparent) to \code{255} (opaque).
+  A character vector beginning with \code{"#"} followed by the red, 
+  blue, green and optionally alpha values in hexadecimal (after 
+  rescaling to \code{0 ... 255}).  The optional alpha values range 
+  from \code{0} (fully transparent) to \code{255} (opaque). 

+  This can be in the long hexadecimal form (e.g., \code{"#rrggbb"} or
+  \code{"#rrggbbaa"}) or the short form (e.g, \code{"#rgb"} or
+  \code{"#rgba"}). The short form is expanded to the long form by 
+  replicating digits (not by adding zeroes), e.g., \code{"#rgb"} becomes
+  \code{"#rrggbb"}.
+
   \R does \strong{not} use \sQuote{premultiplied alpha}.
 }
 \seealso{
Index: src/library/grDevices/src/colors.c
===================================================================
--- src/library/grDevices/src/colors.c  (revision 85029)
+++ src/library/grDevices/src/colors.c  (working copy)
@@ -1345,7 +1345,7 @@
 }

-/* #RRGGBB[AA] String to Internal Color Code */
+/* #RRGGBB[AA] or #RGB[A] String to Internal Color Code */
 static rcolor rgb2col(const char *rgb)
 {
     unsigned int r = 0, g = 0, b = 0, a = 0; /* -Wall */
@@ -1359,13 +1359,25 @@
        g = 16 * hexdigit(rgb[3]) + hexdigit(rgb[4]);
        b = 16 * hexdigit(rgb[5]) + hexdigit(rgb[6]);
        break;
+    case 5: 
+    // Equivalent to 16 * hexdigit(rgb[4]) + hexdigit(rgb[4]);
+       a = 17 * hexdigit(rgb[4]);
+    case 4:
+       r = 17 * hexdigit(rgb[1]);
+       g = 17 * hexdigit(rgb[2]);
+       b = 17 * hexdigit(rgb[3]);
+       break;
     default:
        error(_("invalid RGB specification"));
     }
-    if (strlen(rgb) == 7)
-       return R_RGB(r, g, b);
-    else
-       return R_RGBA(r, g, b, a);
+
+    switch(strlen(rgb)) {
+    case 7: 
+    case 4:
+        return R_RGB(r, g, b);
+    default:
+        return R_RGBA(r, g, b, a);
+    }
 }

 /* External Color Name to Internal Color Code */
Index: tests/Examples/grDevices-Ex.Rout.save
===================================================================
--- tests/Examples/grDevices-Ex.Rout.save       (revision 85029)
+++ tests/Examples/grDevices-Ex.Rout.save       (working copy)
@@ -1,5 +1,5 @@

-R Under development (unstable) (2023-07-31 r84791) -- "Unsuffered Consequences"
+R Under development (unstable) (2023-08-28 r85029) -- "Unsuffered Consequences"
 Copyright (C) 2023 The R Foundation for Statistical Computing
 Platform: x86_64-pc-linux-gnu

@@ -716,6 +716,20 @@
 green        0       83      208
 blue         0      107       79
 > 
+> # long and short form of hexadecimal notation
+> col2rgb(c(long = "#559955", short = "#595"))
+      long short
+red     85    85
+green  153   153
+blue    85    85
+> # with alpha
+> col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+      long short
+red     85    85
+green  153   153
+blue    85    85
+alpha  187   187
+> 
 > ##-- NON-INTRODUCTORY examples --
 > 
 > grC <- col2rgb(paste0("gray", 0:100))
@@ -1526,7 +1540,7 @@
 > stopifnot(setequal(cNms[isC],
 +      c("white","black","blue","cyan","green","magenta","red","yellow")))
 > ## End(Don't show)
-> table(is.gray <- tc[,1] == tc[,2] & tc[,2] == tc[,3])# (397, 105)
+> table(is.gray <- tc[,1] == tc[,2] & tc[,2] == tc[,3])  # (397, 105)

 FALSE  TRUE 
   397   105 
@@ -3584,15 +3598,15 @@
 > ## time-dependent ==> ignore diffs:
 > ## IGNORE_RDIFF_BEGIN
 > pretty(Sys.Date())
-[1] "2023-07-29" "2023-07-30" "2023-07-31" "2023-08-01" "2023-08-02"
-[6] "2023-08-03"
+[1] "2023-08-28" "2023-08-29" "2023-08-30" "2023-08-31" "2023-09-01"
+[6] "2023-09-02"
 > pretty(Sys.time(), n = 10)
- [1] "2023-07-31 10:42:05 CEST" "2023-07-31 10:42:06 CEST"
- [3] "2023-07-31 10:42:07 CEST" "2023-07-31 10:42:08 CEST"
- [5] "2023-07-31 10:42:09 CEST" "2023-07-31 10:42:10 CEST"
- [7] "2023-07-31 10:42:11 CEST" "2023-07-31 10:42:12 CEST"
- [9] "2023-07-31 10:42:13 CEST" "2023-07-31 10:42:14 CEST"
-[11] "2023-07-31 10:42:15 CEST"
+ [1] "2023-08-30 13:25:39 UTC" "2023-08-30 13:25:40 UTC"
+ [3] "2023-08-30 13:25:41 UTC" "2023-08-30 13:25:42 UTC"
+ [5] "2023-08-30 13:25:43 UTC" "2023-08-30 13:25:44 UTC"
+ [7] "2023-08-30 13:25:45 UTC" "2023-08-30 13:25:46 UTC"
+ [9] "2023-08-30 13:25:47 UTC" "2023-08-30 13:25:48 UTC"
+[11] "2023-08-30 13:25:49 UTC"
 > ## IGNORE_RDIFF_END
 > 
 > pretty(as.Date("2000-03-01")) # R 1.0.0 came in a leap year
@@ -4106,40 +4120,6 @@
 + utopia <- X11Font("-*-utopia-*-*-*-*-*-*-*-*-*-*-*-*")
 + X11Fonts(utopia = utopia)
 + })
-> X11Fonts()
-$serif
-[1] "-*-times-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$sans
-[1] "-*-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$mono
-[1] "-*-courier-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Times
-[1] "-adobe-times-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Helvetica
-[1] "-adobe-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$CyrTimes
-[1] "-cronyx-times-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$CyrHelvetica
-[1] "-cronyx-helvetica-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Arial
-[1] "-monotype-arial-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-$Mincho
-[1] "-*-mincho-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-> X11Fonts("mono")
-$mono
-[1] "-*-courier-%s-%s-*-*-%d-*-*-*-*-*-*-*"
-
-> utopia <- X11Font("-*-utopia-*-*-*-*-*-*-*-*-*-*-*-*")
-> X11Fonts(utopia = utopia)
 > ## IGNORE_RDIFF_END
 > 
 > 
@@ -4424,7 +4404,7 @@
 > cleanEx()
 > options(digits = 7L)
 > base::cat("Time elapsed: ", proc.time() - base::get("ptime", pos = 'CheckExEnv'),"\n")
-Time elapsed:  38.446 0.619 39.236 0 0 
+Time elapsed:  31.186 0.549 31.768 0 0 
 > grDevices::dev.off()
 null device 
           1 
MichaelChirico commented 1 year ago

(16 + 1) * hexdigit(.) would be clear and IIRC many compilers/settings will optimize this by computing 16+1 at compile time. Still I think it leaves implicit what this code is doing, and it would be helpful to spell it out (IMO).

malcolmbarrett commented 1 year ago

Personally, I don't think that's clearer than our updated version (I think I'd be more caught off guard by the 16+1), but I hold this opinion very lightly

bastistician commented 1 year ago

@bastistician thank you as well for the clarification. One nuisance I bumped into is that there are other differences in that output file from the changes we made (I guess other things are different that are not incorporated into the test snapshots yet?). So, just running a diff gives some extra output that's not relevant to the patch. Would you recommend hand editing the file or diff to avoid that?

Example outputs can be system-dependent. For (base) packages that use reference output of examples as part of their tests, such "unstable" examples can be put between

## IGNORE_RDIFF_BEGIN
## IGNORE_RDIFF_END

That way they are still run, but tools::Rdiff() will ignore any differences for that output when checking (see its documentation). You can actually see one such case in the above diff excerpt (from the examples in pretty.Date.Rd):

 > ## time-dependent ==> ignore diffs:
 > ## IGNORE_RDIFF_BEGIN
 > pretty(Sys.Date())

Personally, as I like clean patches, I use meld or ediff in Emacs to carry over only the relevant portions of the diff from the newly generated .Rout file of the make check run to the reference .Rout.save file that is to be committed, excluding also the diff in the header (the revision number for that output will be incorrect, it is for a future revision) and the runtime diff.

(In practice, reference outputs sometimes get only updated after the fact, but I think for a formally submitted patch, it is desirable to cover the corresponding diff.)

EllaKaye commented 1 year ago

While we're on this, I'm wondering, are there other functions in base R that take a hex colour code as an argument that might benefit from similar tweaks to accept a 3-digit version?

gmbecker commented 1 year ago

Agree 16+1 is clearer. You still need to know that hex is base 16, and what multiplying by the base your number is represented in does, but that could be clarified in a comment

On Wed, Aug 30, 2023, 6:01 PM Malcolm Barrett @.***> wrote:

Personally, I don't think that's clearer than our updated version (I think I'd be more caught off guard by the 16+1), but I hold this opinion very lightly

— Reply to this email directly, view it on GitHub https://github.com/r-devel/r-project-sprint-2023/issues/74#issuecomment-1699541802, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG53MJ7WEYF7QHH7TRD37TXX5WW3ANCNFSM6AAAAAA4EHBMPU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

pmur002 commented 1 year ago

Nice work!

A couple of suggestions:

calls

pmur002 commented 1 year ago

It should not be necessary in this case (though I have been embarrassingly wrong about that in the past), but another thing to consider when modifying graphics code is not just that it runs, but that it produces the correct (graphical) output.

I use my 'gdiff' package for that. For example, to check that your patched version of r-devel is producing the same result as the latest r-devel, for all of the examples in the 'grid' package, you could do something like ...

library(gdiff)

Rcontrol <- "/path/to/latest/r-devel/Rscript" 
Rtest <- "/path/to/your/patched/r-devel/Rscript"

## Compare control with test output (runs in separate worker sessions)
gridResult <- gdiffPackage("grid",
                           controlDir=file.path(tempdir(), "gridControl"),
                           testDir=file.path(tempdir(), "gridTest"),
                           compareDir=file.path(tempdir(), "gridCompare"),
                           device=pngDevice(),
                           session=list(control=localSession(Rpath=Rcontrol),
                                        test=localSession(Rpath=Rtest)))
print(gridResult, detail=FALSE)

If the results are not all identical, there should be PNG diffs in the compareDir to see where any differences are occurring.

Similar tests could be done for 'grDevices' and 'graphics' and 'lattice'.

Before I commit to r-devel I also like to check 'ggplot2' (because breaking that breaks a LOT of CRAN). That involves R CMD check like for any package, plus use of 'gdiff'. The latter can be a bit more work because you need to install ALL of the 'ggplot2' depends/suggests for all of its examples to run and it takes a while and it can be quite disappointing to lose the results of a run. So I use something a bit more manual like the code below ...

## More manual (run directly in relevant R sessions)
## Rcontrol
gdiffPackageOutput("ggplot2",
                   "/tmp/ggplotControl",
                   device=list(pngDevice()))
## Rtest
gdiffPackageOutput("ggplot2",
                   "/tmp/ggplotTest",
                   device=list(pngDevice()))
## Any old R
ggplotResult <- gdiffCompare("/tmp/ggplotControl",
                             "/tmp/ggplotTest",
                             "/tmp/ggplotCompare")
print(ggplotResult, detail=FALSE)
## REMEMBER to rm the /tmp/ directories!!!

Again, this checking should not be necessary in this case because we are only adding a new way to specify colours, but (successfully) running the tests is more comforting than just crossing your fingers. And it might be generally useful for you to have a play with 'gdiff'.

hanneoberman commented 1 year ago

Code to live test:

We manually tested the above functions in the devel and patched versions. We ensured no errors and compared the visual output by eye.

long_cols <- c("#1166ee", "#334455")
short_cols <- c("#16e", "#345")

long_palette <- palette(long_cols)
short_palette <- palette(short_cols)

stopifnot(all.equal(long_palette, short_palette))

rgb_col <- col2rgb(c(long = "#559955", short = "#595"))
stopifnot(all.equal(rgb_col[, "long"], rgb_col[, "short"]))

rgba_col <- col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
stopifnot(all.equal(rgba_col[, "long"], rgba_col[, "short"]))

op <- par(bg = "#1166ee")
par(bg = "#16e") # error in unpatched version, runs without error in patched version
par(op)

# Tests run to visually inspect whether 3-digit hex codes work in functions downstream from rgb2col
# Need to be set up for cairo!
png(bg = "#1166ee", type = "cairo")
plot(1:10)
dev.off()

png(bg = "#16e", type = "cairo")
plot(1:10)
dev.off()

# Only on macOS
quartz(bg = "#1166ee")
plot(1:10)
dev.off()

quartz(bg = "#16e")
plot(1:10)
dev.off()

plot(c(100, 250), c(300, 450), type = "n", xlab = "", ylab = "")
image <- as.raster(matrix(0:1, ncol = 5, nrow = 3))
rasterImage(image, 100, 300, 150, 350, interpolate = FALSE)

# Only on Windows
windows(bg = "#1166ee")
plot(1:10)
dev.off()

windows(bg = "#16e")
plot(1:10)
dev.off()

Currently setting up gdiff for testing - will report back later this afternoon!

malcolmbarrett commented 1 year ago

gdiff test results look good:

# above code (...)
> print(gridResult, detail=FALSE)
Identical files [257/257]

# above code (...)
> print(ggplotResult, detail=FALSE)
Identical files [1901/1901]
malcolmbarrett commented 1 year ago

Here's the cleaned-up patch with all feedback included. @pmur002 I think we addressed everything, so please take another look when you are able. This patch does not include the above tests. We think the test that is added via our documentation example covers the non-visual use cases, so we decided not to include them in a separate test file. Let us know if there's any of the above you'd prefer to be in the patch.

Index: src/library/grDevices/man/col2rgb.Rd
===================================================================
--- src/library/grDevices/man/col2rgb.Rd    (revision 85031)
+++ src/library/grDevices/man/col2rgb.Rd    (working copy)
@@ -15,9 +15,8 @@
 \arguments{
   \item{col}{vector of any of the three kinds of \R color specifications,
     i.e., either a color name (as listed by \code{\link{colors}()}), a
-    hexadecimal string of the form \code{"#rrggbb"} or
-    \code{"#rrggbbaa"} (see \code{\link{rgb}}), or a positive integer
-    \code{i} meaning \code{\link{palette}()[i]}.}
+    hexadecimal string of the form specified in \code{\link{rgb}} (see
+    Details), positive integer \code{i} meaning \code{\link{palette}()[i]}.}
   \item{alpha}{logical value indicating whether the alpha channel (opacity)
     values should be returned.}
 }
@@ -30,6 +29,13 @@
   are coerced to character: in all other cases the class is
   ignored when doing the coercion.)

+  Hexadecimal colors can be in the long hexadecimal form (e.g., 
+  \code{"#rrggbb"} or \code{"#rrggbbaa"}) or the short form (e.g, \code{"#rgb"}
+  or \code{"#rgba"}). The short form is expanded to the long form by 
+  replicating digits (not by adding zeroes), e.g., \code{"#rgb"} becomes
+  \code{"#rrggbb"}. The short form is only valid for input; the output 
+  will always be in the long form.
+
   Zero and negative values of \code{col} are an error.
 }
 \value{
@@ -58,6 +64,11 @@
 col2rgb(c(red = "red", hex = "#abcdef"))
 col2rgb(c(palette = 1:3))

+# long and short form of hexadecimal notation
+col2rgb(c(long = "#559955", short = "#595"))
+# with alpha
+col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+
 ##-- NON-INTRODUCTORY examples --

 grC <- col2rgb(paste0("gray", 0:100))
Index: src/library/grDevices/man/rgb.Rd
===================================================================
--- src/library/grDevices/man/rgb.Rd    (revision 85031)
+++ src/library/grDevices/man/rgb.Rd    (working copy)
@@ -57,10 +57,10 @@
   \code{green} or \code{alpha}.
 }
 \value{
-  A character vector with elements of 7 or 9 characters, \code{"#"}
-  followed by the red, blue, green and optionally alpha values in
-  hexadecimal (after rescaling to \code{0 ... 255}).  The optional alpha
-  values range from \code{0} (fully transparent) to \code{255} (opaque).
+  A character vector beginning with \code{"#"} followed by the red, 
+  blue, green and optionally alpha values in hexadecimal (after 
+  rescaling to \code{0 ... 255}).  The optional alpha values range 
+  from \code{0} (fully transparent) to \code{255} (opaque). 

   \R does \strong{not} use \sQuote{premultiplied alpha}.
 }
Index: src/library/grDevices/src/colors.c
===================================================================
--- src/library/grDevices/src/colors.c  (revision 85031)
+++ src/library/grDevices/src/colors.c  (working copy)
@@ -1345,7 +1345,7 @@
 }

-/* #RRGGBB[AA] String to Internal Color Code */
+/* #RRGGBB[AA] or #RGB[A] String to Internal Color Code */
 static rcolor rgb2col(const char *rgb)
 {
     unsigned int r = 0, g = 0, b = 0, a = 0; /* -Wall */
@@ -1359,13 +1359,25 @@
    g = 16 * hexdigit(rgb[3]) + hexdigit(rgb[4]);
    b = 16 * hexdigit(rgb[5]) + hexdigit(rgb[6]);
    break;
+    case 5: 
+   // Equivalent to 16 * hexdigit(rgb[4]) + hexdigit(rgb[4]);
+   a = (16 + 1) * hexdigit(rgb[4]);
+    case 4:
+   r = (16 + 1) * hexdigit(rgb[1]);
+   g = (16 + 1) * hexdigit(rgb[2]);
+   b = (16 + 1) * hexdigit(rgb[3]);
+   break;
     default:
    error(_("invalid RGB specification"));
     }
-    if (strlen(rgb) == 7)
-   return R_RGB(r, g, b);
-    else
-   return R_RGBA(r, g, b, a);
+
+    switch(strlen(rgb)) {
+    case 7: 
+    case 4:
+        return R_RGB(r, g, b);
+    default:
+        return R_RGBA(r, g, b, a);
+    }
 }

 /* External Color Name to Internal Color Code */
Index: src/library/graphics/man/par.Rd
===================================================================
--- src/library/graphics/man/par.Rd (revision 85031)
+++ src/library/graphics/man/par.Rd (working copy)
@@ -607,12 +607,18 @@
   in terms of their RGB components with a string of the form
   \code{"#RRGGBB"} where each of the pairs \code{RR}, \code{GG},
   \code{BB} consist of two hexadecimal digits giving a value in the
-  range \code{00} to \code{FF}.  Colors can also be specified by giving
-  an index into a small table of colors, the \code{\link{palette}}:
-  indices wrap round so with the default palette of size 8, \code{10} is
-  the same as \code{2}.  This provides compatibility with S.  Index
-  \code{0} corresponds to the background color.  Note that the palette
-  (apart from \code{0} which is per-device) is a per-session setting.
+  range \code{00} to \code{FF}.  Hexadecimal colors can be in the long
+  hexadecimal form (e.g., \code{"#rrggbb"} or \code{"#rrggbbaa"}) or the
+  short form (e.g, \code{"#rgb"} or \code{"#rgba"}). The short form is
+  expanded to the long form by replicating digits (not by adding zeroes),
+  e.g., \code{"#rgb"} becomes \code{"#rrggbb"}. The short form is only 
+  valid for input; the output will always be in the long form. Colors can
+  also be specified by giving an index into a small table of colors, the 
+  \code{\link{palette}}: indices wrap round so with the default palette 
+  of size 8, \code{10} is the same as \code{2}.  This provides 
+  compatibility with S.  Index \code{0} corresponds to the background 
+  color.  Note that the palette (apart from \code{0} which is per-device)
+  is a per-session setting.

   Negative integer colours are errors.

Index: tests/Examples/grDevices-Ex.Rout.save
===================================================================
--- tests/Examples/grDevices-Ex.Rout.save   (revision 85031)
+++ tests/Examples/grDevices-Ex.Rout.save   (working copy)
@@ -1,5 +1,5 @@

-R Under development (unstable) (2023-07-31 r84791) -- "Unsuffered Consequences"
+R Under development (unstable) (2023-08-28 r85029) -- "Unsuffered Consequences"
 Copyright (C) 2023 The R Foundation for Statistical Computing
 Platform: x86_64-pc-linux-gnu

@@ -716,6 +716,20 @@
 green        0       83      208
 blue         0      107       79
 > 
+> # long and short form of hexadecimal notation
+> col2rgb(c(long = "#559955", short = "#595"))
+      long short
+red     85    85
+green  153   153
+blue    85    85
+> # with alpha
+> col2rgb(c(long = "#559955BB", short = "#595B"), alpha = TRUE)
+      long short
+red     85    85
+green  153   153
+blue    85    85
+alpha  187   187
+> 
 > ##-- NON-INTRODUCTORY examples --
 > 
 > grC <- col2rgb(paste0("gray", 0:100))

svn patch rgb.diff --dry-run ran successfully on a fresh copy of R devel

pmur002 commented 1 year ago

Thanks everyone, especially for the extra testing. All looks pretty good to me. Now committed to r-devel (r85039).

A few follow up points for your interest/amusement:

Thanks again for all your work. I think we have a very nice result.

EllaKaye commented 1 year ago

Woohoo! This has been a really fun and interesting project to work on. Thanks to @pmur002 for your mentorship and to @georgestagg, @malcolmbarrett, @hanneoberman, @nzgwynn for being such excellent team mates, to @MichaelChirico, @bastistician and @gmbecker for your input and, of course, @coolbutuseless for the original idea and suggested C fix 😃

malcolmbarrett commented 1 year ago

Incredible, thank you, everyone!!

Re: alpha, yes, those were the exact thoughts we had, down to comparing existing behavior. Our mistake for not logging them here!