Re: [hackers][st][PATCH] Add support for OSC color sequences.

From: Quentin Rameau <quinq_AT_fifth.space>
Date: Tue, 14 Dec 2021 08:16:28 +0100

Hi Raheman,

Just from a coding standpoint,

> diff --git a/config.def.h b/config.def.h
> index 6f05dce..91ab8ca 100644
> --- a/config.def.h
> +++ b/config.def.h
> _AT_@ -120,6 +120,8 @@ static const char *colorname[] = {
> /* more colors can be added after 255 to use with DefaultXX */
> "#cccccc",
> "#555555",
> + "gray90", /* default foreground colour */
> + "black", /* default background colour */
> };
>
>
> _AT_@ -127,9 +129,9 @@ static const char *colorname[] = {
> * Default colors (colorname index)
> * foreground, background, cursor, reverse cursor
> */
> -unsigned int defaultfg = 7;
> -unsigned int defaultbg = 0;
> -static unsigned int defaultcs = 256;
> +unsigned int defaultfg = 258;
> +unsigned int defaultbg = 259;
> +unsigned int defaultcs = 256;
> static unsigned int defaultrcs = 257;
>
> /*
> diff --git a/st.c b/st.c
> index a9338e1..ad5c8e8 100644
> --- a/st.c
> +++ b/st.c
> _AT_@ -1842,6 +1842,42 @@ csireset(void)
> memset(&csiescseq, 0, sizeof(csiescseq));
> }
>
> +void
> +osc4_color_response(int num)
> +{
> + int n;
> + char buf[32];
> + unsigned char r,g,b;
> +
> + if (xgetcolor(num, &r,&g,&b)) {
> + fprintf(stderr, "erresc: failed to fetch osc4 color %d\n", num);
> + return;
> + }
> +
> + n = snprintf(buf, sizeof buf, "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> + num, r, r, g, g, b, b);
> +
> + ttywrite(buf, n, 1);
> +}
> +
> +void
> +osc_color_response(int index, int num)
> +{
> + int n;
> + char buf[32];
> + unsigned char r,g,b;
> +
> + if (xgetcolor(index, &r,&g,&b)) {
> + fprintf(stderr, "erresc: failed to fetch osc color %d\n", index);
> + return;
> + }
> +
> + n = snprintf(buf, sizeof buf, "\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x\007",
> + num, r, r, g, g, b, b);
> +
> + ttywrite(buf, n, 1);
> +}
> +
> void
> strhandle(void)
> {

It seems that both osc4_color_response and osc_color response are the exact
same function, just the print format string differs.
This could be factorized with only a parameter for switching the string.

> _AT_@ -1880,6 +1916,45 @@ strhandle(void)
> }
> }
> return;
> + case 10:
> + if (narg < 2)
> + break;
> +
> + p = strescseq.args[1];
> +
> + if (!strcmp(p, "?"))
> + osc_color_response(defaultfg, 10);
> + else if (xsetcolorname(defaultfg, p))
> + fprintf(stderr, "erresc: invalid foreground color: %s\n", p);
> + else
> + redraw();
> + break;
> + case 11:
> + if (narg < 2)
> + break;
> +
> + p = strescseq.args[1];
> +
> + if (!strcmp(p, "?"))
> + osc_color_response(defaultbg, 11);
> + else if (xsetcolorname(defaultbg, p))
> + fprintf(stderr, "erresc: invalid background color: %s\n", p);
> + else
> + redraw();
> + break;
> + case 12:
> + if (narg < 2)
> + break;
> +
> + p = strescseq.args[1];
> +
> + if (!strcmp(p, "?"))
> + osc_color_response(defaultcs, 12);
> + else if (xsetcolorname(defaultcs, p))
> + fprintf(stderr, "erresc: invalid cursor color: %s\n", p);
> + else
> + redraw();
> + break;
> case 4: /* color set */
> if (narg < 3)
> break;

Again, case 10, 11, and 12, looks to be the exact same code, only the
parameter passed to osc_color_response differ, but you already have
that variable at hand.
Those three cases could be joined in a fallthrough that would call a
function with that code just differing depending on the parameter value.

> _AT_@ -1887,7 +1962,10 @@ strhandle(void)
> /* FALLTHROUGH */
> case 104: /* color reset, here p = NULL */
> j = (narg > 1) ? atoi(strescseq.args[1]) : -1;
> - if (xsetcolorname(j, p)) {
> +
> + if (!strcmp(p, "?"))
> + osc4_color_response(j);
> + else if (xsetcolorname(j, p)) {
> if (par == 104 && narg <= 1)
> return; /* color reset without parameter */
> fprintf(stderr, "erresc: invalid color j=%d, p=%s\n",
> diff --git a/st.h b/st.h
> index fa2eddf..519b9bd 100644
> --- a/st.h
> +++ b/st.h
> _AT_@ -111,6 +111,8 @@ void *xmalloc(size_t);
> void *xrealloc(void *, size_t);
> char *xstrdup(const char *);
>
> +int xgetcolor(int x, unsigned char *r, unsigned char *g, unsigned char *b);
> +
> /* config.h globals */
> extern char *utmp;
> extern char *scroll;
> _AT_@ -123,3 +125,4 @@ extern char *termname;
> extern unsigned int tabspaces;
> extern unsigned int defaultfg;
> extern unsigned int defaultbg;
> +extern unsigned int defaultcs;
> diff --git a/x.c b/x.c
> index 89786b8..8a16faa 100644
> --- a/x.c
> +++ b/x.c
> _AT_@ -799,6 +799,19 @@ xloadcols(void)
> loaded = 1;
> }
>
> +int
> +xgetcolor(int x, unsigned char *r, unsigned char *g, unsigned char *b)
> +{
> + if (!BETWEEN(x, 0, dc.collen))
> + return 1;
> +
> + *r = dc.col[x].color.red >> 8;
> + *g = dc.col[x].color.green >> 8;
> + *b = dc.col[x].color.blue >> 8;
> +
> + return 0;
> +}
> +
> int
> xsetcolorname(int x, const char *name)
> {
Received on Tue Dec 14 2021 - 08:16:28 CET