Enable virtual terminal processing on Windows #511
Conversation
|
I suspect that this code needs to be locked behind a build tag. It's illegal to use the windows syscall package on the wrong platform, even if conditional. |
|
For an example of how to do it, try looking at |
|
I was trying to avoid doing that because it would introduce new files, but done that now. Let me know if I should move or rename them. |
| var mode uint32 | ||
| err = windows.GetConsoleMode(windows.Handle(hStdout), &mode) | ||
| if err == nil { | ||
| windows.SetConsoleMode(windows.Handle(hStdout), mode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) |
There was a problem hiding this comment.
Does Node do this by default? How was this working before?
There was a problem hiding this comment.
Yeah, executing console.log("\x1B[38;2;255;0;0mTesting"); via Node shows that it works, so I assume Node just does that by default.
There was a problem hiding this comment.
I don't see them doing in their code at all; the constant is defined but seeming unused...
There was a problem hiding this comment.
It seems to be used here: https://github.com/nodejs/node/blob/59f00d713299bfe7d18d15c4504ac14bd5b428a8/deps/uv/src/win/tty.c#L2312
There was a problem hiding this comment.
Alright, so checking this, uv__determine_vterm_state is only called when uv_tty_init is called; I get the feeling that Node only calls this when it knows the output is a TTY, via https://github.com/nodejs/node/blob/d615a3cfcbc357ccebd07a37fa0002fee162d9ce/src/tty_wrap.cc#L152, But I don't see this used either!
I wonder if there's just a plain package somewhere that sets ENABLE_VIRTUAL_TERMINAL_PROCESSING...
There was a problem hiding this comment.
Well, as you've already seen with trying to locate ENABLE_VIRTUAL_TERMINAL_PROCESSING, Github's search results are not always conclusive.
I don't really know Node internals, but I can guarantee you that ENABLE_VIRTUAL_TERMINAL_PROCESSING does need to be set for ANSI escape sequences to work on Windows. (Although apparently not on Windows 11)
There was a problem hiding this comment.
To be clear, I think this PR is right, at least how to enable it, but I'm just trying to verify we won't break something like LSP or piping where the output is not a TTY.
Given https://github.com/jedib0t/go-pretty/blob/b14745cbcf545d2704cdd6f97163344b0fe683a3/text/ansi_windows.go#L22 looks similar and does not appear to be conditional at all, so I very much suspect this PR is good as-is.
There was a problem hiding this comment.
I checked, the LSP is totally fine. I'm going to approve this 😄
| var mode uint32 | ||
| err = windows.GetConsoleMode(windows.Handle(hStdout), &mode) | ||
| if err == nil { | ||
| windows.SetConsoleMode(windows.Handle(hStdout), mode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) |
There was a problem hiding this comment.
I checked, the LSP is totally fine. I'm going to approve this 😄
|
Once again, thanks for sending this. I'm happy to see that your first time using Go was successful. |
|
It's easier than ever to pick up a new language in the age of AI. ^^ |
Before:
After:
Fixes #480
(I've never used Go before and I didn't verify this works on MacOS or Linux.)