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

From: Raheman Vaiya <r.vaiya_AT_gmail.com>
Date: Sun, 12 Dec 2021 01:29:32 -0500

> -static unsigned int defaultcs = 256;
>> +unsigned int defaultfg = 255;
>> +unsigned int defaultbg = 254;
>> +unsigned int defaultcs = 256;

> The values in the config are changed here. Is it intended? If so, why?

The colours 0 and 7 are distinct and can be set independently from the
foreground and background colours in most terminals. I modified defaultbg
and
defaultfg to reflect this. In retrospect, I probably should have made them >
255 to avoid conflicting with legitimate palette colours. I have done so in
the
amended patch.

>> + 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.

Ok.

>> + 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.

Oops. I'm not sure why my gcc didn't produce a warning.

> Case 10 should be ordered before case 11.

Ok.

> 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.

Ok.

>> +void xgetcolor(int x, unsigned char *r, unsigned char *g, unsigned char
*b);

> xgetcolor should be below xstrdup and as a separate line.

Will do.

Thanks for reviewing the patch :). I have attached an amended copy below.
Let
me know if anything else requires attention.

Regards,
Raheman

On Sat, Dec 11, 2021 at 7:01 AM Hiltjo Posthuma <hiltjo_AT_codemadness.org>
wrote:

> 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__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__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__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__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__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__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__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 Sun Dec 12 2021 - 07:29:32 CET

This archive was generated by hypermail 2.3.0 : Tue Dec 14 2021 - 04:12:31 CET