On Sun, Dec 12, 2021 at 01:29:32AM -0500, Raheman Vaiya wrote:
> > -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 <hil...@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
> > > @@ -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
> > > @@ -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)
> > > {
> > > @@ -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;
> > > @@ -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
> > > @@ -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 *);
> > > @@ -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
> > > @@ -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
> >
> >
> diff --git a/config.def.h b/config.def.h
> index 6f05dce..91ab8ca 100644
> --- a/config.def.h
> +++ b/config.def.h
> @@ -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 */
> };
>
>
> @@ -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
> @@ -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)
> {
> @@ -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;
> @@ -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
> @@ -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;
> @@ -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
> @@ -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)
> {
Hi,
Pushed and added small code-style changes and a follow-up fix.
Thanks for the patch,
--
Kind regards,
Hiltjo
