Undercurl patch breaks SGR colon fix

[TheWanderer1983]

Hello all,
Back in May ST patched in colon support for SGR attributes (https://git.suckless.org/st/commit/5dbcca49263be094fc38159c297458ae323ef647.html). Here is the commit for st-fleximatch aa59574

This fixed the problem I was having with aerc which uses Vaxis. However, the undercurl patch (https://st.suckless.org/patches/undercurl/) patch breaks this fix.

[bakkeby]

I think I see what you mean; if you use semicolon as the separator instead of colon in the escape codes for undercurl, then it doesn't render undercurl. This is because the undercurl patch specifically uses colons.

Might be worth getting this fixed in the undercurl patch considering the SGR patch you referred to.

[bakkeby]

I played around a bit and I see how the undercurl patch breaks escape codes that uses colon instead of semicolon, which is probably what you were after.

Perhaps we can get rid of that readcolonargs function altogether.

[bakkeby]

The undercurl patch stores additional args in a separate array and relies on colon to be for its own use only.

Expanding the existing int arg[ESC_ARG_SIZ]; in CSIEscape might have been better in this case, but it would break a lot of existing code and patches so overall having a separate array is the path of least resistance here.

As such I think the easiest way to address this issue is to only read those extra arguments when the escape codes of 4 and 58 are used (4 is to set underline, and 58 is to set underline colour).

Proposed changes:

diff --git a/st.c b/st.c
index 4721ee2..e1286d3 100644
--- a/st.c
+++ b/st.c
@@ -170,7 +170,7 @@ static void csihandle(void);
 static void dcshandle(void);
 #endif // SIXEL_PATCH
 #if UNDERCURL_PATCH
-static void readcolonargs(char **, int, int[][CAR_PER_ARG]);
+static void readcolonargs(char **, int, int[][CAR_PER_ARG], int sep);
 #endif // UNDERCURL_PATCH
 static void csiparse(void);
 static void csireset(void);
@@ -1483,20 +1483,21 @@ tnewline(int first_col)

 #if UNDERCURL_PATCH
 void
-readcolonargs(char **p, int cursor, int params[][CAR_PER_ARG])
+readcolonargs(char **p, int cursor, int params[][CAR_PER_ARG], int sep)
 {
        int i = 0;
+
        for (; i < CAR_PER_ARG; i++)
                params[cursor][i] = -1;

-       if (**p != ':')
+       if (**p != sep)
                return;

        char *np = NULL;
        i = 0;

-       while (**p == ':' && i < CAR_PER_ARG) {
-               while (**p == ':')
+       while (**p == sep && i < CAR_PER_ARG) {
+               while (**p == sep)
                        (*p)++;
                params[cursor][i] = strtol(*p, &np, 10);
                *p = np;
@@ -1528,11 +1529,13 @@ csiparse(void)
                        v = -1;
                csiescseq.arg[csiescseq.narg++] = v;
                p = np;
-               #if UNDERCURL_PATCH
-               readcolonargs(&p, csiescseq.narg-1, csiescseq.carg);
-               #endif // UNDERCURL_PATCH
                if (sep == ';' && *p == ':')
                        sep = ':'; /* allow override to colon once */
+               #if UNDERCURL_PATCH
+               if (v == 4 || v == 58)
+                       readcolonargs(&p, csiescseq.narg-1, csiescseq.carg, sep);
+               #endif // UNDERCURL_PATCH
+
                if (*p != sep || csiescseq.narg == ESC_ARG_SIZ)
                        break;
                p++;

For testing purposes you would only need to add that one if statement if (v == 4 || v == 58), the rest is just to be able to use semicolon or colon in the escape codes.

[veltza]

Don't add support for semicolons in the underline/undercurl sequence (4), because it will likely break many applications. For example, if an application wants to set the underline and italic attributes at the same time using the sequence \e[4;3m, the code will think it means the undercurl attribute only \e[4:3m.

Also the new mixed solution in st is so wrong.

For example, this works correctly in st:

printf '\e[3;38:5:2mGreen italic text\n'

But if you swap the parameters, it doesn't work anymore in st:

printf '\e[38:5:2;3mGreen italic text\n'

They're both valid sequences, so I'm starting to lose faith in the suckless guys and what they're doing.

[bakkeby]

@veltza I see what you mean now. The code assumes that the separator is a semicolon and allows for the separator to be replaced by a colon once. So if the sequence starts with a colon then it that will be considered as the override and it can't ever be replaced with a semicolon later on.

So a sequence with only colons will work.

printf '\e[38:5:2:3mGreen italic text\n'

I had a quick look to see what could be done to support both ways and was experimenting with this.

diff --git a/st.c b/st.c
index e1286d3..1202463 100644
--- a/st.c
+++ b/st.c
@@ -1511,7 +1511,8 @@ csiparse(void)
 {
        char *p = csiescseq.buf, *np;
        long int v;
-       int sep = ';'; /* colon or semi-colon, but not both */
+       int sep = 0; /* colon or semi-colon */
+       int override = 1;

        csiescseq.narg = 0;
        if (*p == '?') {
@@ -1529,8 +1530,12 @@ csiparse(void)
                        v = -1;
                csiescseq.arg[csiescseq.narg++] = v;
                p = np;
-               if (sep == ';' && *p == ':')
-                       sep = ':'; /* allow override to colon once */
+               if (override && ((sep == ';' && *p == ':') || (sep == ':' && *p == ';'))) {
+                       sep = *p;
+                       override = 0; /* allow separator to be overridden once */
+               }
+               if (sep == 0 && (*p == ':' || *p == ';'))
+                       sep = *p;
                #if UNDERCURL_PATCH
                if (v == 4 || v == 58)
                        readcolonargs(&p, csiescseq.narg-1, csiescseq.carg, sep);

It is not pretty but seems to work with your second case (starts with colon, then changes to semi-colons).

[veltza]

I don't think it's going to work that way. Let me explain.

First, here is a simplified version of the SGR rules:

1. Parameters (bold, italic, etc.) are separated by semicolons and they are order independent.
2. Subparameters (r, g, b, etc.) are separated by colons and can be order dependent.

(I recommend that you read the ECMA-48 document as there is more on that subject. Wikipedia also has good background information about it.)

A brief history goes that XTerm misunderstood the specification and started using semicolons in color codes 38 and 48, and other terminal developers followed along. A few years later, XTerm noticed the mistake and tried to fix it, but the ship had already sailed. And now terminal emulators try to support both semicolons and colons in subparameters.

So, in my opinion, the robust solution is to use readcolonargs() function or similar to collect subparameters in the subparameter table. And then, in tdefcolor() function, the color parameters are parsed either from the subparameter table or from the following SGR parameters.

In st-sx I use a slightly modified version of readcolonargs() function and I have also modified tdefcolor() function to handle both semicolon and colon separated subparameters in color codes 38, 48 and 58. Although my solution works now, it is not future proof. So I want to rewrite it at some point and therefore I don't want to send it to the upstream as is.

[veltza]

@bakkeby, I had some time to play with your patch and found that it has a serious flaw. The problem is that the csiparse function is used to parse all CSI sequences and not just SGR sequences.

So, for example, if the application wants to move the cursor to position (4,3), it sends the sequence \e[4;3H. Now the code below thinks that the first argument (4) means underline style and parses the second argument (3) into the carg array:

	if (v == 4 || v == 58)
		readcolonargs(&p, csiescseq.narg-1, csiescseq.carg, sep);

But the code below looks for the second argument in the arg array and doesn't find it there. So the end result is that the cursor moves to position (4,0):

	case 'H': /* CUP -- Move to <row> <col> */
	case 'f': /* HVP */
		DEFAULT(csiescseq.arg[0], 1);
		DEFAULT(csiescseq.arg[1], 1);
		tmoveato(csiescseq.arg[1]-1, csiescseq.arg[0]-1);
		break;

I tried to find different solutions to fix this, but I always ran into new issues. So I came to the conclusion that I can't fix this without rewriting it like I did in st-sx.

So @TheWanderer1983, could you email the author of the undercurl patch and ask him to fix the patch so that it works with the latest st commits and then push the fixed version to the suckless server. Maybe he's smarter than me and find an easy solution to this issue.

[TheWanderer1983]

Yes, I can do that. Also thanks for maintaining your fork of ST-sx. Please continue to work on it :) regards Dylan

…

On Sat Sep 7, 2024 at 10:35 PM AEST, veltza wrote: @bakkeby, I had some time to play with your patch and found that it has a serious flaw. The problem is that the csiparse function is used to parse all CSI sequences and not just SGR sequences. So, for example, if the application wants to move the cursor to position **(4,3)**, it sends the sequence `\e[4;3H`. Now the code below thinks that the first argument (4) means underline style and parses the second argument (3) into the **carg** array: ```c if (v == 4 || v == 58) readcolonargs(&p, csiescseq.narg-1, csiescseq.carg, sep); ``` But the code below looks for the second argument in the **arg** array and doesn't find it there. So the end result is that the cursor moves to position **(4,0)**: ```c case 'H': /* CUP -- Move to <row> <col> */ case 'f': /* HVP */ DEFAULT(csiescseq.arg[0], 1); DEFAULT(csiescseq.arg[1], 1); tmoveato(csiescseq.arg[1]-1, csiescseq.arg[0]-1); break; ``` I tried to find different solutions to fix this, but I always ran into new issues. So I came to the conclusion that I can't fix this without rewriting it like I did in st-sx. So @TheWanderer1983, could you email the author of [the undercurl patch](https://st.suckless.org/patches/undercurl/) and ask him to fix the patch so that it works with the latest st commits and then push the fixed version to the suckless server. Maybe he's smarter than me and find an easy solution to this issue.

[veltza]

@bakkeby I tried to warn you that your solution wouldn't work, but you just merged it anyway and now you have serious issues.

Do the following:

1. Enable UNDERCURL_PATCH
2. Compile
3. Run the compiled st
4. Edit st.c using nvim --clean st.c or vim --clean st.c
5. Turn on line numbering :set number
6. Move to line 4 and you should notice that the cursor jumped to the left edge.
7. Now try to move to the right or try to edit the line. You can't.

The same thing happens with Micro editor. Fun stuff, right?

[bakkeby]

@veltza thanks for the ping. That push was not intentional and have reverted it now.