Skip to content

Commit

Permalink
Fixed: Improve handling of empty ';' SGR sequences
Browse files Browse the repository at this point in the history 
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/
  • Loading branch information
@fornwall
fornwall committed Aug 13, 2024
1 parent a5de3d3 commit f80b464
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 47 deletions.
73 changes: 27 additions & 46 deletions terminal-emulator/src/main/java/com/termux/terminal/TerminalEmulator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,6 @@ public final class TerminalEmulator {
private String mTitle;
private final Stack<String> mTitleStack = new Stack<>();

/** If processing first character of first parameter of {@link #ESC_CSI}. */
private boolean mIsCSIStart;
/** The last character processed of a parameter of {@link #ESC_CSI}. */
private Integer mLastCSIArg;

/** The cursor position. Between (0,0) and (mRows-1, mColumns-1). */
private int mCursorRow, mCursorCol;

Expand Down Expand Up @@ -1393,8 +1388,6 @@ private void doEsc(int b) {
break;
case '[':
continueSequence(ESC_CSI);
mIsCSIStart = true;
mLastCSIArg = null;
break;
case '=': // DECKPAM
setDecsetinternalBit(DECSET_BIT_APPLICATION_KEYPAD, true);
Expand Down Expand Up @@ -1765,7 +1758,7 @@ private void doCsi(int b) {
private void selectGraphicRendition() {
if (mArgIndex >= mArgs.length) mArgIndex = mArgs.length - 1;
for (int i = 0; i <= mArgIndex; i++) {
int code = mArgs[i];
int code = getArg(i, 0, false);
if (code < 0) {
if (mArgIndex > 0) {
continue;
Expand Down Expand Up @@ -1823,7 +1816,10 @@ private void selectGraphicRendition() {
if (i + 4 > mArgIndex) {
Logger.logWarn(mClient, LOG_TAG, "Too few CSI" + code + ";2 RGB arguments");
} else {
int red = mArgs[i + 2], green = mArgs[i + 3], blue = mArgs[i + 4];
int red = getArg(i + 2, 0, false);
int green = getArg(i + 3, 0, false);
int blue = getArg(i + 4, 0, false);

if (red < 0 || green < 0 || blue < 0 || red > 255 || green > 255 || blue > 255) {
finishSequenceAndLogError("Invalid RGB: " + red + "," + green + "," + blue);
} else {
Expand All @@ -1837,7 +1833,7 @@ private void selectGraphicRendition() {
i += 4; // "2;P_r;P_g;P_r"
}
} else if (firstArg == 5) {
int color = mArgs[i + 2];
int color = getArg(i + 2, 0, false);
i += 2; // "5;P_s"
if (color >= 0 && color < TextStyle.NUM_INDEXED_COLORS) {
if (code == 38) {
Expand Down Expand Up @@ -2116,44 +2112,29 @@ private void scrollDownOneLine() {
*
* https://vt100.net/docs/vt510-rm/chapter4.html#S4.3.3
* */
private void parseArg(int inputByte) {
int[] bytes = new int[]{inputByte};
// Only doing this for ESC_CSI and not for other ESC_CSI_* since they seem to be using their
// own defaults with getArg*() calls, but there may be missed cases
if (mEscapeState == ESC_CSI) {
if ((mIsCSIStart && inputByte == ';') || // If sequence starts with a ; character, like \033[;m
(!mIsCSIStart && mLastCSIArg != null && mLastCSIArg == ';' && inputByte == ';')) { // If sequence contains sequential ; characters, like \033[;;m
bytes = new int[]{'0', ';'}; // Assume 0 was passed
}
}

mIsCSIStart = false;

for (int b : bytes) {
if (b >= '0' && b <= '9') {
if (mArgIndex < mArgs.length) {
int oldValue = mArgs[mArgIndex];
int thisDigit = b - '0';
int value;
if (oldValue >= 0) {
value = oldValue * 10 + thisDigit;
} else {
value = thisDigit;
}
if (value > 9999)
value = 9999;
mArgs[mArgIndex] = value;
}
continueSequence(mEscapeState);
} else if (b == ';') {
if (mArgIndex < mArgs.length) {
mArgIndex++;
private void parseArg(int b) {
if (b >= '0' && b <= '9') {
if (mArgIndex < mArgs.length) {
int oldValue = mArgs[mArgIndex];
int thisDigit = b - '0';
int value;
if (oldValue >= 0) {
value = oldValue * 10 + thisDigit;
} else {
value = thisDigit;
}
continueSequence(mEscapeState);
} else {
unknownSequence(b);
if (value > 9999)
value = 9999;
mArgs[mArgIndex] = value;
}
continueSequence(mEscapeState);
} else if (b == ';') {
if (mArgIndex < mArgs.length) {
mArgIndex++;
}
mLastCSIArg = b;
continueSequence(mEscapeState);
} else {
unknownSequence(b);
}
}

Expand Down
27 changes: 26 additions & 1 deletion terminal-emulator/src/test/java/com/termux/terminal/TerminalTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@ public void testSelectGraphics() {
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
enterString("\033[31;;m");
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
enterString("\033[31;m");
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
enterString("\033[31;;41m");
assertEquals(TextStyle.COLOR_INDEX_FOREGROUND, mTerminal.mForeColor);
assertEquals(1, mTerminal.mBackColor);
enterString("\033[0m");
assertEquals(TextStyle.COLOR_INDEX_BACKGROUND, mTerminal.mBackColor);

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

// Multiple parameters at once:
enterString("\033[38;5;178;48;5;179;m");
enterString("\033[38;5;178;48;5;179m");
assertEquals(178, mTerminal.mForeColor);
assertEquals(179, mTerminal.mBackColor);

// Omitted parameter means zero:
enterString("\033[38;5;m");
assertEquals(0, mTerminal.mForeColor);
assertEquals(179, mTerminal.mBackColor);
enterString("\033[48;5;m");
assertEquals(0, mTerminal.mForeColor);
assertEquals(0, mTerminal.mBackColor);

// 24 bit colors:
enterString(("\033[0m")); // Reset fg and bg colors.
enterString("\033[38;2;255;127;2m");
Expand All @@ -205,6 +220,16 @@ public void testSelectGraphics() {
enterString("\033[38;2;300;127;2;48;2;1;300;254m");
assertEquals(expectedForeground, mTerminal.mForeColor);
assertEquals(expectedBackground, mTerminal.mBackColor);

// 24 bit colors, omitted parameter means zero:
enterString("\033[38;2;255;127;m");
expectedForeground = 0xff000000 | (255 << 16) | (127 << 8);
assertEquals(expectedForeground, mTerminal.mForeColor);
assertEquals(expectedBackground, mTerminal.mBackColor);
enterString("\033[38;2;123;;77m");
expectedForeground = 0xff000000 | (123 << 16) | 77;
assertEquals(expectedForeground, mTerminal.mForeColor);
assertEquals(expectedBackground, mTerminal.mBackColor);
}

public void testBackgroundColorErase() {
Expand Down

0 comments on commit f80b464

Please sign in to comment.