> -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 <[email protected]>
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)
{
