Skip to content

Commit 5b5e719

Browse files
fornwallagnostic-apollo
fornwall
authored and
agnostic-apollo
committed
Fixed: Improve handling of empty ';' SGR sequences
Currently the Termux terminal emulator prints "HI" in red with: ```sh printf "\e[31;m HI \e[0m" ``` This is not how other terminals (tested on xterm, gnome-terminal, alacritty and the mac built in terminal) handle it, since they parse ""\e[31;m" as "\e[31;0m", where the "0" resets the colors. This change aligns with other terminals, as well as improves performance by avoiding allocating a new int[] array for each byte processed by `parseArg()`, and most importantly simplifies things by removing the `mIsCSIStart` and `mLastCSIArg` state, preparing for supporting ':' separated sub parameters such as used in https://sw.kovidgoyal.net/kitty/underlines/
1 parent f3e7388 commit 5b5e719

File tree

2 files changed

+53
-47
lines changed

2 files changed

+53
-47
lines changed

‎terminal-emulator/src/main/java/com/termux/terminal/TerminalEmulator.java

Lines changed: 27 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -126,11 +126,6 @@ public final class TerminalEmulator {
126126
private String mTitle;
127127
private final Stack<String> mTitleStack = new Stack<>();
128128

129-
/** If processing first character of first parameter of {@link #ESC_CSI}. */
130-
private boolean mIsCSIStart;
131-
/** The last character processed of a parameter of {@link #ESC_CSI}. */
132-
private Integer mLastCSIArg;
133-
134129
/** The cursor position. Between (0,0) and (mRows-1, mColumns-1). */
135130
private int mCursorRow, mCursorCol;
136131

@@ -1390,8 +1385,6 @@ private void doEsc(int b) {
13901385
break;
13911386
case '[':
13921387
continueSequence(ESC_CSI);
1393-
mIsCSIStart = true;
1394-
mLastCSIArg = null;
13951388
break;
13961389
case '=': // DECKPAM
13971390
setDecsetinternalBit(DECSET_BIT_APPLICATION_KEYPAD, true);
@@ -1762,7 +1755,7 @@ private void doCsi(int b) {
17621755
private void selectGraphicRendition() {
17631756
if (mArgIndex >= mArgs.length) mArgIndex = mArgs.length - 1;
17641757
for (int i = 0; i <= mArgIndex; i++) {
1765-
int code = mArgs[i];
1758+
int code = getArg(i, 0, false);
17661759
if (code < 0) {
17671760
if (mArgIndex > 0) {
17681761
continue;
@@ -1820,7 +1813,10 @@ private void selectGraphicRendition() {
18201813
if (i + 4 > mArgIndex) {
18211814
Logger.logWarn(mClient, LOG_TAG, "Too few CSI" + code + ";2 RGB arguments");
18221815
} else {
1823-
int red = mArgs[i + 2], green = mArgs[i + 3], blue = mArgs[i + 4];
1816+
int red = getArg(i + 2, 0, false);
1817+
int green = getArg(i + 3, 0, false);
1818+
int blue = getArg(i + 4, 0, false);
1819+
18241820
if (red < 0 || green < 0 || blue < 0 || red > 255 || green > 255 || blue > 255) {
18251821
finishSequenceAndLogError("Invalid RGB: " + red + "," + green + "," + blue);
18261822
} else {
@@ -1834,7 +1830,7 @@ private void selectGraphicRendition() {
18341830
i += 4; // "2;P_r;P_g;P_r"
18351831
}
18361832
} else if (firstArg == 5) {
1837-
int color = mArgs[i + 2];
1833+
int color = getArg(i + 2, 0, false);
18381834
i += 2; // "5;P_s"
18391835
if (color >= 0 && color < TextStyle.NUM_INDEXED_COLORS) {
18401836
if (code == 38) {
@@ -2113,44 +2109,29 @@ private void scrollDownOneLine() {
21132109
*
21142110
* https://vt100.net/docs/vt510-rm/chapter4.html#S4.3.3
21152111
* */
2116-
private void parseArg(int inputByte) {
2117-
int[] bytes = new int[]{inputByte};
2118-
// Only doing this for ESC_CSI and not for other ESC_CSI_* since they seem to be using their
2119-
// own defaults with getArg*() calls, but there may be missed cases
2120-
if (mEscapeState == ESC_CSI) {
2121-
if ((mIsCSIStart && inputByte == ';') || // If sequence starts with a ; character, like \033[;m
2122-
(!mIsCSIStart && mLastCSIArg != null && mLastCSIArg == ';' && inputByte == ';')) { // If sequence contains sequential ; characters, like \033[;;m
2123-
bytes = new int[]{'0', ';'}; // Assume 0 was passed
2124-
}
2125-
}
2126-
2127-
mIsCSIStart = false;
2128-
2129-
for (int b : bytes) {
2130-
if (b >= '0' && b <= '9') {
2131-
if (mArgIndex < mArgs.length) {
2132-
int oldValue = mArgs[mArgIndex];
2133-
int thisDigit = b - '0';
2134-
int value;
2135-
if (oldValue >= 0) {
2136-
value = oldValue * 10 + thisDigit;
2137-
} else {
2138-
value = thisDigit;
2139-
}
2140-
if (value > 9999)
2141-
value = 9999;
2142-
mArgs[mArgIndex] = value;
2143-
}
2144-
continueSequence(mEscapeState);
2145-
} else if (b == ';') {
2146-
if (mArgIndex < mArgs.length) {
2147-
mArgIndex++;
2112+
private void parseArg(int b) {
2113+
if (b >= '0' && b <= '9') {
2114+
if (mArgIndex < mArgs.length) {
2115+
int oldValue = mArgs[mArgIndex];
2116+
int thisDigit = b - '0';
2117+
int value;
2118+
if (oldValue >= 0) {
2119+
value = oldValue * 10 + thisDigit;
2120+
} else {
2121+
value = thisDigit;
21482122
}
2149-
continueSequence(mEscapeState);
2150-
} else {
2151-
unknownSequence(b);
2123+
if (value > 9999)
2124+
value = 9999;
2125+
mArgs[mArgIndex] = value;
2126+
}
2127+
continueSequence(mEscapeState);
2128+
} else if (b == ';') {
2129+
if (mArgIndex < mArgs.length) {
2130+
mArgIndex++;
21522131
}
2153-
mLastCSIArg = b;
2132+
continueSequence(mEscapeState);
2133+
} else {
2134+
unknownSequence(b);
21542135
}
21552136
}
21562137

‎terminal-emulator/src/test/java/com/termux/terminal/TerminalTest.java

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,13 @@ public void testSelectGraphics() {
163163
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
164164
enterString("\033[31;;m");
165165
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
166+
enterString("\033[31;m");
167+
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
168+
enterString("\033[31;;41m");
169+
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
170+
assertEquals(1, mTerminal.mBackColor);
171+
enterString("\033[0m");
172+
assertEquals(TextStyle.COLOR_INDEX_BACKGROUND, mTerminal.mBackColor);
166173

167174
// 256 colors:
168175
enterString("\033[38;5;119m");
@@ -178,10 +185,18 @@ public void testSelectGraphics() {
178185
assertEquals(129, mTerminal.mBackColor);
179186

180187
// Multiple parameters at once:
181-
enterString("\033[38;5;178;48;5;179;m");
188+
enterString("\033[38;5;178;48;5;179m");
182189
assertEquals(178, mTerminal.mForeColor);
183190
assertEquals(179, mTerminal.mBackColor);
184191

192+
// Omitted parameter means zero:
193+
enterString("\033[38;5;m");
194+
assertEquals(0, mTerminal.mForeColor);
195+
assertEquals(179, mTerminal.mBackColor);
196+
enterString("\033[48;5;m");
197+
assertEquals(0, mTerminal.mForeColor);
198+
assertEquals(0, mTerminal.mBackColor);
199+
185200
// 24 bit colors:
186201
enterString(("\033[0m")); // Reset fg and bg colors.
187202
enterString("\033[38;2;255;127;2m");
@@ -205,6 +220,16 @@ public void testSelectGraphics() {
205220
enterString("\033[38;2;300;127;2;48;2;1;300;254m");
206221
assertEquals(expectedForeground, mTerminal.mForeColor);
207222
assertEquals(expectedBackground, mTerminal.mBackColor);
223+
224+
// 24 bit colors, omitted parameter means zero:
225+
enterString("\033[38;2;255;127;m");
226+
expectedForeground = 0xff000000 | (255 << 16) | (127 << 8);
227+
assertEquals(expectedForeground, mTerminal.mForeColor);
228+
assertEquals(expectedBackground, mTerminal.mBackColor);
229+
enterString("\033[38;2;123;;77m");
230+
expectedForeground = 0xff000000 | (123 << 16) | 77;
231+
assertEquals(expectedForeground, mTerminal.mForeColor);
232+
assertEquals(expectedBackground, mTerminal.mBackColor);
208233
}
209234

210235
public void testBackgroundColorErase() {

0 commit comments

Comments
 (0)