fix: Running CLI editors does not work when piped to rainbow, unlike when piped to the Ruby lolcat - #7
Conversation
Piping interactive editors like nano or vim previously caused hangs or broken output due to improper terminal handling. This aligns stdin/TTY behavior with the original Ruby lolcat implementation. Fixes arsham#6
arsham
left a comment
arsham
left a comment
There was a problem hiding this comment.
Thank you for taking the time to investigate this and put a change together.
I have left a few comments where I think the current implementation can duplicate or consume parts of escape sequences. In particular, scanning ahead inside a range loop does not advance the iterator past the sequence, CSI sequences can have non-alphabetic final bytes, and OSC/DCS terminators need to be consumed in full. We should also account for a sequence being split across multiple calls to Write, since io.Copy does not guarantee that writes align with terminal-command boundaries.
We could explore changing this to an index-controlled or stateful scanner and adding focused tests that verify the control bytes are preserved exactly once while only the printable text is colourised. Cases such as cursor visibility, bracketed paste, OSC terminated by BEL and ST, adjacent sequences, and a sequence split across two writes would give us confidence that Nano and Vim output is handled correctly.
Could you also provide before-and-after benchmarks for the change? The existing BenchmarkLightPaint is not enough to measure this path: its reader is consumed on the first iteration, and it does not contain terminal escape sequences. It would be useful to benchmark representative plain text, Unicode text, and editor-like CSI/OSC streams with the input reset for each iteration, b.SetBytes, and allocation reporting, then include a benchstat comparison.
Finally, could you include a reproducible Nano or Vim example showing the behaviour before and after the change?
Thanks again for working on this. I think with the parser corrected and the behaviour covered by tests and measurements, this will be much easier to evaluate confidently.
|
|
||
| data = colorMatch.ReplaceAll(data, []byte("")) | ||
| for _, c := range string(data) { | ||
| for i, c := range string(data) { |
There was a problem hiding this comment.
How do you feel about using an explicitly controlled byte index here instead of range? After writing seq[:j], continue only advances the range loop by one rune, so the bytes inside the sequence will be visited and colourised again. For example, with \x1b[2Jx, we first write \x1b[2J unchanged and then process [2J again as visible text. An index that advances by the full sequence length would avoid duplicating the consumed bytes.
| j := 1 | ||
| if j < len(seq) && seq[j] == '[' { | ||
| for j++; j < len(seq); j++ { | ||
| if (seq[j] >= 'A' && seq[j] <= 'Z') || (seq[j] >= 'a' && seq[j] <= 'z') { |
There was a problem hiding this comment.
Do you reckon we could check the complete CSI final-byte range (0x40 through 0x7e) here? Valid CSI sequences do not always end in a letter. For example, bracketed-paste sequences end in ~, so with the current condition something like \x1b[200~text may consume text as part of the escape sequence. It would be good to add a regression test for a ~-terminated sequence as well.
| } | ||
| } | ||
| } else if j < len(seq) && (seq[j] == ']' || seq[j] == 'P' || seq[j] == 'X' || seq[j] == '^') { | ||
| for j++; j < len(seq) && seq[j] != 0x1b && seq[j] != 0x07; j++ { |
There was a problem hiding this comment.
I would recommend consuming the complete terminator here. At the moment seq[:j] excludes BEL, and for an ST terminator it stops before the ESC in ESC \. This means the terminator is processed separately and the backslash may be colourised as visible text. Could we preserve both BEL and the complete ESC \ terminator, with tests for both forms?
| var ( | ||
| // We remove all previous paintings to create a new rainbow. | ||
| colorMatch = regexp.MustCompile("^\033" + `\[\d+(;\d+)?(;\d+)?[mK]`) | ||
| colorMatch = regexp.MustCompile(`\033\[[0-9;]*[mK]`) |
There was a problem hiding this comment.
Can you document and test the intended behaviour here please? This changes the previous anchored match into a global removal, so all matching SGR and erase-line sequences are removed before the new parser has a chance to preserve them. I am not sure whether we want to remove every existing m/K sequence or only colours previously produced by rainbow. A test covering an embedded \x1b[K would help make that contract clear.
| data = colorMatch.ReplaceAll(data, []byte("")) | ||
| for _, c := range string(data) { | ||
| for i, c := range string(data) { | ||
| if c == '\033' { |
There was a problem hiding this comment.
How do you feel about accounting for escape sequences split across calls to Write? io.Copy is allowed to split the input at arbitrary byte boundaries, so one call could end with \x1b[?25 and the next begin with l. Since the parser is currently stateless per call, that sequence can't be reconstructed. I think we need either a small pending-sequence buffer or another streaming approach, together with a split-write regression test.
2 participants
Closes #6