Hi,
Some comments below:
On Fri, Dec 10, 2021 at 05:53:40PM -0500, Raheman Vaiya wrote:
> The attached patch adds full support for the OSC 10/11/12/4 control
> sequences.
> These are used by programs like vim and theme.sh to query and set terminal
> colours and are supported by most major terminals (e.g alacrittty, kitty,
> libvte). I submitted a patch to the wiki some time ago, but it didn't
xterm supports it too.
> include
> support for query sequences. I am posting it here because I feel it would
> be a
> good candidate for inclusion into st itself.
>
> You can test it using https://github.com/lemnos/theme.sh by running
>
> theme.sh zenburn
> theme.sh -p
>
> Don't hesitate to let me know if I have made any grievous errors :P.
>
> Regards,
> Raheman
> diff --git a/config.def.h b/config.def.h
> index 6f05dce..3932e83 100644
> --- a/config.def.h
> +++ b/config.def.h
> _AT_@ -127,9 +127,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 = 255;
> +unsigned int defaultbg = 254;
> +unsigned int defaultcs = 256;
> static unsigned int defaultrcs = 257;
The values in the config are changed here. Is it intended? If so, why?
>
> /*
> diff --git a/st.c b/st.c
> index a9338e1..a469512 100644
> --- a/st.c
> +++ b/st.c
> _AT_@ -1842,6 +1842,32 @@ csireset(void)
> memset(&csiescseq, 0, sizeof(csiescseq));
> }
>
> +void
> +osc4_color_response(int num)
> +{
> + int n;
> + char buf[32];
> + unsigned char r,g,b;
> +
> + xgetcolor(num, &r,&g,&b);
> + n = sprintf(buf, "\033]4;%d;rgb:%02x%02x/%02x%02x/%02x%02x\007", num, r, r, g, g, b, b);
> +
I prefer snprintf here, just in case. The arguments for sprintf are too long and
should be wrapped.
> + ttywrite(buf, n, 1);
> +}
> +
> +void
> +osc_color_response(int index, int num)
> +{
> + int n;
> + char buf[32];
> + unsigned char r,g,b;
> +
> + xgetcolor(index, &r,&g,&b);
> + n = sprintf(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)
> {
> _AT_@ -1880,6 +1906,45 @@ strhandle(void)
> }
> }
> return;
> + 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 %d\n", p);
The argument p is not int. The compiler (clang) also gives a warning about it.
> + 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 %d\n", p);
> + else
> + redraw();
> + break;
> + case 10:
Case 10 should be ordered before case 11.
> + 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 %d\n", p);
> + else
> + redraw();
> + break;
> case 4: /* color set */
> if (narg < 3)
> break;
> _AT_@ -1887,7 +1952,10 @@ strhandle(void)
> /* FALLTHROUGH */
> case 104: /* color reset, here p = NULL */
> j = (narg > 1) ? atoi(strescseq.args[1]) : -1;
The variable j should be checked and also the xgetcolor and/or functions using
it should have range checks. It seems dc.col can be out-of-bounds with some
input. I also noticed a crash here while testing.
> - 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..47d76ca 100644
> --- a/st.h
> +++ b/st.h
> _AT_@ -107,6 +107,7 @@ char *getsel(void);
>
> size_t utf8encode(Rune, char *);
>
> +void xgetcolor(int x, unsigned char *r, unsigned char *g, unsigned char *b);
xgetcolor should be below xstrdup and as a separate line.
> void *xmalloc(size_t);
> void *xrealloc(void *, size_t);
> char *xstrdup(const char *);
> _AT_@ -123,3 +124,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..3761fd0 100644
> --- a/x.c
> +++ b/x.c
> _AT_@ -799,6 +799,14 @@ xloadcols(void)
> loaded = 1;
> }
>
> +void
> +xgetcolor(int x, unsigned char *r, unsigned char *g, unsigned char *b)
> +{
> + *r = dc.col[x].color.red >> 8;
> + *g = dc.col[x].color.green >> 8;
> + *b = dc.col[x].color.blue >> 8;
> +}
> +
> int
> xsetcolorname(int x, const char *name)
> {
This patch needs a bit more care and testing to be able to accept it.
Thanks for your efforts,
--
Kind regards,
Hiltjo
Received on Sat Dec 11 2021 - 13:01:09 CET