win,tty: add support for ANSI codes in win10 v1511 #889
Conversation
|
As discussed in #721 |
|
Not sure who to ping on windows stuff |
|
I'll take a look as soon as I can. |
|
/cc @miniksa |
|
Hey there! I was the one to actually implement this in Windows 10 v1511. :) There's some documentation available here that might help: https://msdn.microsoft.com/en-us/library/windows/desktop/mt638032(v=vs.85).aspx However, it looks like you've got a handle on it already with this pull request. I would just recommend against checking the OS version using kernel32 that way because it is possible for someone to be on v1511+ and be using the Legacy console which doesn't support these sequences. The preferred way that we've been suggesting to detect whether the support is available is to do probe-and-set (you can see an example in the contribution to Docker client here: https://github.com/docker/docker/blob/master/pkg/term/term_windows.go). Basically, try to get the current console mode, perform binary or with the additional virtual terminal flag onto it, and set. If it rejects it with STATUS_INVALID_PARAMETER, then you're on an old version of the OS or the user is on a compatibility mode console that doesn't support it. If it goes through with return code 0 (STATUS_SUCCESSFUL), you're golden. :) The support has been made much more robust for the Anniversary update coming this summer, so if you're finding missing functionality, it might already be on the way! If you have any particular questions or concerns on the implementation, let me know and I'll see if I can help answer them! You can also prod my ANSI sequence partner in crime, Mike (@zadjii-msft). Thanks, |
|
Thanks for chiming in! Much appreciated. |
imran-iq
force-pushed
the
win10ansi
branch
3 times, most recently
from
34cddca
to
8a570fa
Compare
|
So updated the code as per comments (for testing purposes put a printf in my code to let me know we are using my code, its not actually in the commit I hope!) Heres a console using legacy mode running my modified test(our emulated implementation): And non legacy mode(new code that uses built in support): |
|
Looks good to me! |
|
If I can get one more LGTM (preferably from a libuv member) ill go ahead and land this and close #721 |
|
I'll review it this week. |
|
FWIW, I tested this technique on windows 10, and While colors do work on win10, I couldn't get bold to work, eventhough in their example at https://msdn.microsoft.com/en-us/library/windows/desktop/mt638032(v=vs.85).aspx it does seem to work. Maybe it was just sensitive to the way the escape sequence was constructed (e.g. maybe they expect bold to be before/after the color, or maybe just one SGR code per escape.. didn't check deeper - but according to the docs the sequence looks good - just simple color and bold), but the same sequence works on linux on every terminal I tested (VTE based, KDE terminal and st.suckless.org). FYI. |
|
@avih Thanks for the information, but we're going to need to know exactly which sequence was emitted when we didn't react appropriately if we're going to fix it. Can you provide that detail? cc: @zadjii-msft |
|
Not right now. I was experimenting with the win10 console support on another project, and I didn't yet dig deep (and didn't try it at all with libuv). For now I'd just suggest to experiment with few simple SGR sequences which include bold and a one or two colors (e.g. 1;31 or 30;41;1 etc) and just make sure it works. If I'll have more concrete findings, I'll report back. |
|
@avih I'll wait to hear your concrete findings. |
| @@ -121,6 +121,18 @@ static char uv_tty_default_fg_bright = 0; | |||
| static char uv_tty_default_bg_bright = 0; | |||
| static char uv_tty_default_inverse = 0; | |||
|
|
|||
| #ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING | |||
| #define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004 | |||
There was a problem hiding this comment.
can you make this # define ... (we do this in other places too, it helps readability)
|
@avih @miniksa I do see color and bold working together nicely: http://imgur.com/GilDcMB @iwuzhere great work! left some comments. |
|
@saghul That looks like you're using ConEmu or some derivative, not the default Windows Console Host. It has a different rendering pipeline than ours which is known to be able to do ANSI color even before we added it to Windows 10, so that's not really verification. :( |
imran-iq
force-pushed
the
win10ansi
branch
2 times, most recently
from
727ca2a
to
567e2d2
Compare
|
Updated once again. |
| } | ||
|
|
||
| if (!MultiByteToWideChar(CP_UTF8, 0, buf.base, buf.len, | ||
| utf16_buf, utf16_buf_used)) { |
There was a problem hiding this comment.
can you line these up vertically?
There was a problem hiding this comment.
Just to clarify, do you mean like this? (for the calls to MultiByteToWideChar)
MultiByteToWideChar(CP_UTF8,
0,
buf.base,
buf.len,
utf16_buf,
utf16_buf_used)
|
LGTM with a tiny style nit. |
|
Updated! |
|
Rubber-stamp LGTM! 🎉 |
|
@iwuzhere can you land this one? I want to make a WIP libuv upgrade PR to Node to see if smoke comes out ;-) |
PR-URL: #889 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
|
Landed 58ccfd4 |
|
|
||
| FLUSH_TEXT(); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The indentation is off here.
Causes regressions on Windows 10 in applications that use ANSI codes. Also revert one-liner commit 0895ccf ("win: fix typo in type name"). This reverts commit 0895ccf. This reverts commit 58ccfd4. Refs: nodejs/node#9542 Refs: libuv#889 Refs: libuv#1135
In uv_tty_write_bufs, if the console supports Virtual Terminal sequences, we try to convert the passed in utf8 buffer to utf16. However, we need to check if the buffer is of non-zero length- otherwise, MultiByteToWideChar returns an error. Fixes: #1135 Fixes: nodejs/node#9542 PR-URL: #1139 Refs: #889 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Imran Iqbal <imran@imraniqbal.org>
Causes regressions on Windows 10 in applications that use ANSI codes. Also reverts commit 0895ccf ("win: fix typo in type name") and commit d0c2641 ("win, tty: handle empty buffer in uv_tty_write_bufs".) This reverts commit d0c2641. This reverts commit 0895ccf. This reverts commit 58ccfd4. Refs: nodejs/node#9542 Refs: libuv#889 Refs: libuv#1135
Causes regressions on Windows 10 in applications that use ANSI codes. Also reverts commit 0895ccf ("win: fix typo in type name") and commit d0c2641 ("win, tty: handle empty buffer in uv_tty_write_bufs".) This reverts commit d0c2641. This reverts commit 0895ccf. This reverts commit 58ccfd4. PR-URL: libuv#1138 Refs: libuv#1135 Refs: libuv#889 Refs: nodejs/node#9542 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Imran Iqbal <imran@imraniqbal.org> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
To bring back support for ANSI codes in win10 v1511. This reverts commit 8cbabaa. PR-URL: libuv#1143 Refs: libuv#1138 Refs: libuv#889 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Imran Iqbal <imran@imraniqbal.org>
Make sure there's enough room in the output buffer by dynamically allocating memory in case the size of the buffer needs to be greater than 8192 characters. PR-URL: libuv#1143 Refs: libuv#1138 Refs: libuv#889 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Imran Iqbal <imran@imraniqbal.org>
Use windows 10's v1511 built in support for ANSI codes instead of emulating