Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use more universal ANSI sequence for 'clear screen and clear buffer' #57701

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

colinta
Copy link
Contributor

@colinta colinta commented Mar 8, 2024

Fixes #57700
Fixes #29829
Fixes #57894
Closes #59315

Tiny tiny patch to bring just a little more terminal support to tsc --watch. In my case, iTerm2 needs this ESC [ 3J sequence in order to clear screen and remove scroll history.

If there's a setting in iTerm2 to change this, I would happily do that, but I didn't see anything.

  92049 passing (6m)

Finished do-runtests-parallel in 6m 22.9s
Completed runtests-parallel in 6m 57.3s
✨  Done in 417.81s.

@mrazauskas
Copy link

For what it’s worth, the ansi-escapes library has two similarly named methods: clearScreen() and clearTerminal().

clearScreen() is the same as current tsc --watch implementation.

clearTerminal() is more complex and it is based on Jest's implementation (see sindresorhus/ansi-escapes#6 and https://github.com/jestjs/jest/blob/6b120fbc5d1cadb3ef205bd1d48549155bed5dd1/packages/jest-util/src/specialChars.ts#L18-L20)

I am pointing to ansi-escapes, because it is rather widely used. Not sure if there are any standards in this field.

@sandersn sandersn added this to Not started in PR Backlog Mar 18, 2024
@sandersn sandersn moved this from Not started to Needs merge in PR Backlog Mar 20, 2024
@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Mar 20, 2024
@jakebailey
Copy link
Member

jakebailey commented Mar 20, 2024

If we're going to do this, I'm somewhat inclined to take the more popular approach of using the same sequence that ansi-escapes/jest take, though that appears to be platform specific which may make our tests unhappy.

I'll have to ask for feedback on this from the team.

@colinta
Copy link
Contributor Author

colinta commented Mar 22, 2024

I wonder if we even need to make a distinction between mac and windows

echo -n -e "\x1B[2J\x1B[3J\x1BH\x1B[0f"

Works on all my terminals (Terminal, iTerm, & VSCode). I don't have a Windows machine to test on, though. (I updated the PR to use this sequence)

@colinta colinta changed the title Use 'standard' ANSI sequence for 'clear screen and clear buffer' Use more universal ANSI sequence for 'clear screen and clear buffer' Apr 24, 2024
@colinta
Copy link
Contributor Author

colinta commented Apr 25, 2024

The passage of time made me forget the ANSI sequence I had settled on, but I remembered this morning and updated this PR one more time.

So, the sequence in there is based on the one from ansi-sequences. It's just a combination of the two, without the OS check.

@jakebailey
Copy link
Member

I believe that this also fixes #29829 (which I found via people commenting duplicates on the related issues to this PR's).

Planning on doing some local testing, though, it's just a lot of terminal emulators to test...

If we are doing this, I hope nobody relies on our specific escape sequence. I think not, but rather they depend on other watch messages.

@jakebailey
Copy link
Member

jakebailey commented Jul 22, 2024

I did some testing of this PR and #59315.

old behavior:

  • linux, wsl vscode: can't scroll up ✅
  • linux, wsl terminal: can't scroll up ✅
  • linux, foot: can't scroll up ✅
  • linux, tilix (vte): can't scroll up ✅
  • windows 11, vscode, pwsh: can't scroll up ✅
  • windows 11, terminal, pwsh: can't scroll up ✅
  • windows 11, terminal, cmd: can't scroll up ✅
  • windows 11, console host, cmd: can't scroll up ✅
  • macos, kitty: can't scroll up ✅
  • macos, terminal: can scroll up
  • macos, iterm: can scroll up

We emit 0x1Bc, causing #57894 (and I assume #29829 too).

#57701

  • linux, wsl vscode: can't scroll up ✅
  • linux, wsl terminal: can't scroll up ✅
  • linux, foot: can't scroll up ✅
  • linux, tilix (vte): can't scroll up ✅
  • windows 11, vscode, pwsh: can't scroll up ✅
  • windows 11, terminal, pwsh: can't scroll up ✅
  • windows 11, terminal, cmd: can't scroll up ✅
  • windows 11, console host, cmd: can't scroll up ✅
  • macos, kitty: can't scroll up ✅
  • macos, terminal: (changed) can't scroll up
  • macos, iterm: (changed) iterm asks for permission to clear screen, then can't scroll up

No 0x1Bc, so should fix #57894.

#59315

  • linux, wsl vscode: can't scroll up ✅
  • linux, wsl terminal: can't scroll up ✅
  • linux, foot: can't scroll up ✅
  • linux, tilix (vte): can't scroll up ✅
  • windows 11, vscode, pwsh: can't scroll up ✅
  • windows 11, terminal, pwsh: can scroll up
  • windows 11, terminal, cmd: can scroll up
  • windows 11, console host, cmd: can scroll up
  • macos, kitty: can't scroll up ✅
  • macos, terminal: can't scroll up ✅
  • macos, terminal: (changed) can't scroll up
  • macos, iterm: (changed) iterm asks for permission to clear screen, then can't scroll up

No 0x1Bc, so should fix #57894.


I have not tested on something like Windows 10 with cmd, though; but I'm fairly confident that this PR is good.

@jakebailey
Copy link
Member

Using this launch task in our repo:

        {
            "label": "tsc: watch ./src with built",
            "type": "shell",
            "command": "node",
            "args": ["${workspaceFolder}/built/local/tsc.js", "--build", "${workspaceFolder}/src", "--watch"],
            "group": "build",
            "isBackground": true,
            "problemMatcher": [
                "$tsc-watch"
            ]
        },

Shows that the problem matcher in VS Code also works fine:

image

image

Then undo the bad code:

image

@mrazauskas
Copy link

Out of curiosity: what if \u001B[2J\u001B[3J\u001B[H is always emitted regardless of platform? Seemed like it is not covered in #57701 (comment). Or I misunderstood?

@jakebailey
Copy link
Member

That's exactly what this PR does, if I'm understanding you correctly.

diff --git a/src/compiler/sys.ts b/src/compiler/sys.ts
index 32d7af09c03e7..cf9318afdd199 100644
--- a/src/compiler/sys.ts
    b/src/compiler/sys.ts
@@ -1619,7  1619,7 @@ export let sys: System = (() => {
             setTimeout,
             clearTimeout,
             clearScreen: () => {
-                process.stdout.write("\x1Bc");
                 process.stdout.write("\x1B[2J\x1B[3J\x1BH\x1B[0f");
             },
             setBlocking: () => {
                 const handle = (process.stdout as any)?._handle as { setBlocking?: (value: boolean) => void; };

@mrazauskas
Copy link

That's exactly what this PR does, if I'm understanding you correctly.

Almost. I am suggesting to skip \x1B[0f.

@jakebailey
Copy link
Member

I can test that, but what effect are you trying to get by omitting that?

@jakebailey
Copy link
Member

jakebailey commented Jul 22, 2024

Tested, and omitting that seems to leave a bunch of space above the log, not actually clearing the screen (and the extra empty lines are scrollable). So it seems to be required.

@mrazauskas
Copy link

mrazauskas commented Jul 22, 2024

Thanks. That’s interesting. I thought that \x1B[H and \x1B[0f both do the same thing. Only testing can proof that.

@mrazauskas
Copy link

Sorry, me again. Seems like \x1BH is missing [ in this PR, but has it in #59315. Shouldn’t it read \x1B[H?

@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Jul 22, 2024

If I'm reading this correctly, \x1BH sets a horizontal tab stop whereas \x1B[H is interpreted as pressing the Home key and \x1B[0f moves the cursor to a particular position in the viewport (it seems to take 2 coordinates so I'm not sure what only providing 1 implies).

Edit: Ah, the rules for the omission are noted below: "If <x> and <y> are omitted, they will be set to 1;1." So I guess \x1B[0f is equivalent to \x1B[0;1f?

Edit 2: I think I was misreading the bit about the Home key. It's the other way around - pressing the Home key in "Normal Mode" generates the CUP sequence \x1B[H which is equivalent to \x1B[1;1H... I think.

Edit 3: And going full circle, "If <n> is omitted or equals 0, it will be treated as a 1" so \x1B[0f == \x1B[0;1f == \x1B[1;1f and \x1B[H == \x1B[1;1H which should both do the same thing. But \x1BH (missing the [) should mean something else.

@ehoogeveen-medweb
Copy link

To add to the above, \x1B[3J should do nothing according to this as 3 is not a valid value for <n>. So technically the following should both be equivalent and sufficient:

  • \x1B[2J\x1B[H
  • \x1B[2J\x1B[f

But maybe there are differences between terminals that mean you need more.

@jakebailey
Copy link
Member

I'm going through all of the combos now (fixing the potential typo, trying those two shorter sequences).

@ehoogeveen-medweb
Copy link

Ah, that page is incomplete - 3J indeed clears the scrollback, as commented here (and probably in other places). So you do need both:

  • \x1B[2J\x1B[3J\x1B[H
  • \x1B[2J\x1B[3J\x1B[f

@ehoogeveen-medweb
Copy link

According to this, CUP (\x1B[H here) should be preferred over HVP (\x1B[f here) for "new applications". Either way they should be equivalent :)

@jakebailey
Copy link
Member

jakebailey commented Jul 22, 2024

#57701 but with \x1B[H typo fix

  • linux, wsl vscode: can't scroll up ✅
  • linux, wsl terminal: can't scroll up ✅
  • linux, foot: can't scroll up ✅
  • linux, tilix (vte): can't scroll up ✅
  • windows 11, vscode, pwsh: can't scroll up ✅
  • windows 11, terminal, pwsh: can't scroll up ✅
  • windows 11, terminal, cmd: can't scroll up ✅
  • windows 11, console host, cmd: can't scroll up ✅
  • macos, kitty: can't scroll up ✅
  • macos, terminal: can't scroll up ✅
  • macos, iterm: can't scroll up ✅

Just \x1B[2J\x1B[3J\x1B[H

  • linux, wsl vscode: can't scroll up ✅
  • linux, wsl terminal: can't scroll up ✅
  • linux, foot: can't scroll up ✅
  • linux, tilix (vte): can't scroll up ✅
  • windows 11, vscode, pwsh: can't scroll up ✅
  • windows 11, terminal, pwsh: can't scroll up ✅
  • windows 11, terminal, cmd: can't scroll up ✅
  • windows 11, console host, cmd: can't scroll up ✅
  • macos, kitty: can't scroll up ✅
  • macos, terminal: can't scroll up ✅
  • macos, iterm: can't scroll up ✅

Just \x1B[2J\x1B[3J\x1B[f

  • linux, wsl vscode: can't scroll up ✅
  • linux, wsl terminal: can't scroll up ✅
  • linux, foot: can't scroll up ✅
  • linux, tilix (vte): can't scroll up ✅
  • windows 11, vscode, pwsh: can't scroll up ✅
  • windows 11, terminal, pwsh: can't scroll up ✅
  • windows 11, terminal, cmd: can't scroll up ✅
  • windows 11, console host, cmd: can't scroll up ✅
  • macos, kitty: can't scroll up ✅
  • macos, terminal: can't scroll up ✅
  • macos, iterm: can't scroll up ✅

However, not including H or f causes extra empty lines to be printed, so one of those must be present.

@jakebailey
Copy link
Member

According to this, CUP (\x1B[H here) should be preferred over HVP (\x1B[f here) for "new applications". Either way they should be equivalent :)

That was the next question, which one to choose! Seems like we should be good with \x1B[2J\x1B[3J\x1B[H. Likely, the current PR only works because the typo didn't matter due to [f.

@jakebailey jakebailey merged commit 3f2a9e4 into microsoft:main Jul 22, 2024
32 checks passed
@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Jul 22, 2024

For completeness:

  • The old sequence \x1Bc (ESC c) was RIS (Reset to Initial State)
  • The new sequence \x1B[2J \x1B[3J \x1B[H (CSI 2 J CSI 3 J CSI H) is two ED (Erase in Display) sequences followed by a CUP (CUrsor Position) sequence
  • Of these, CSI 3 J to clear scrollback is something that wasn't supported on the VT510 but was added as an extension to xterm and adopted by other terminals; see e.g. here (Erase Saved Lines)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Done
PR Backlog
  
Needs merge
7 participants