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

windows: add support for native virtual terminal #15206

Closed

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Apr 8, 2023

Unfortunately I'm unable to test that, when ENABLE_VIRTUAL_TERMINAL_PROCESSING is available, the Windows console supports all the escape codes used by the compiler and standard library.

From https://web.archive.org/web/20230000000000*/https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences, the official documentation was only added in 2022.

The os.isatty function for windows supports the optional Cygwin
(probably rarely used on recent Windows installation), but does not
support the virtual terminal processing introduced in Windows 10 1511.

Implement it. Add the SetConsoleMode function in kernel32,
and define the constant ENABLE_VIRTUAL_TERMINAL_PROCESSING in isatty.
Comment on lines +3202 to +3203
// Support for ANSI escapes sequences was added to Windows 10 1511
// on 2016.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads like either 1. a version check should be used here or 2. an assertion should be added.

Copy link
Contributor Author

@perillo perillo Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to advertise this feature, but probably it is not necessary.

A version check is not necessary, since if SetConsoleMode returns 0, then the feature is not supported.

if (windows.kernel32.GetConsoleMode(handle, &mode) == 0) return false;

mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
return windows.kernel32.SetConsoleMode(handle, mode) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Does this affect the use case of using dumb as argument to the windows console?
  2. Did you compare how other applications are doing it, ie Windows Terminal? It sounds very fishy, that 2 kernel32 calls are necessary to query the console state.

Copy link
Contributor Author

@perillo perillo Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Does this affect the use case of using dumb as argument to the windows console?

I'm not aware of any dumb arguments for the windows console. Note that I use Windows only for testing software compatibility.

2. Did you compare how other applications are doing it, ie [Windows Terminal](https://github.com/microsoft/terminal)? It sounds very fishy, that 2 kernel32 calls are necessary to query the console state.

AFAIK, Windows Terminal enables this the feature by default.

The problem, that I did not consider, is how to detect Windows Terminal.
With the current PR, escape codes will be disabled (not tested) when a Zig program runs on a Windows Terminal, since calling GetConsoleMode will fail (not tested).

See microsoft/terminal#1040.

Is anyone using Zig on a Windows Terminal?

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Does this affect the use case of using dumb as argument to the windows console?

    1. Did you compare how other applications are doing it, ie Windows Terminal? It sounds very fishy, that 2 kernel32 calls are necessary to query the console state.

Note that the first call is to get the current state and the second it to set it. It is the same as with Unix termios.

Also, about the Console API and Windows Terminal: calling GetConsoleMode should not fail.

@squeek502
Copy link
Collaborator

Windows Terminal seems to set ENABLE_VIRTUAL_TERMINAL_PROCESSING automatically.

In regular cmd.exe:

15206-cmd

In Windows Terminal:

15206-terminal


Test program:

const std = @import("std");

pub extern "kernel32" fn SetConsoleMode(in_hConsoleHandle: std.os.windows.HANDLE, in_Mode: std.os.windows.DWORD) callconv(std.os.windows.WINAPI) std.os.windows.BOOL;

const ENABLE_VIRTUAL_TERMINAL_PROCESSING = 0x0004;

pub fn main() !void {
    const handle = std.io.getStdErr().handle;
    var mode: std.os.windows.DWORD = undefined;
    if (std.os.windows.kernel32.GetConsoleMode(handle, &mode) == 0) {
        @panic("couldn't get console mode");
    }

    const enabled = mode & ENABLE_VIRTUAL_TERMINAL_PROCESSING != 0;
    std.debug.print("{}\n", .{enabled});
    std.debug.print("\x1b[32;1myep\x1b[0m\n", .{});

    mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
    const should_now_be_enabled = SetConsoleMode(handle, mode) != 0;

    std.debug.print("{}\n", .{should_now_be_enabled});
    std.debug.print("\x1b[32;1myep\x1b[0m\n", .{});
}

@perillo
Copy link
Contributor Author

perillo commented May 22, 2023

@squeek502 There is some documentation in https://ss64.com/nt/syntax-ansi.html.

@squeek502
Copy link
Collaborator

squeek502 commented May 22, 2023

All I was showing was that Zig setting ENABLE_VIRTUAL_TERMINAL_PROCESSING is unnecessary in Windows Terminal--it's already set. The escape sequences are valid/correct and are there to show how the console renders them.

To make a more general comment about the PR, I'm unsure if setting ENABLE_VIRTUAL_TERMINAL_PROCESSING is something Zig should be doing unconditionally like this. This is similar to #7600 (see also #12400 and #14411) in that it affects all processes run in the console session, not just the current process, so it's not clear cut if attempting to force ENABLE_VIRTUAL_TERMINAL_PROCESSING to be enabled is always the 'right' thing to do. If we can get away with just #15282, it seems like that might be the 'safer' way to go.

@andrewrk andrewrk closed this Oct 19, 2023
@expikr
Copy link
Contributor

expikr commented Nov 21, 2023

To make a more general comment about the PR, I'm unsure if setting ENABLE_VIRTUAL_TERMINAL_PROCESSING is something Zig should be doing unconditionally like this. This is similar to #7600 (see also #12400 and #14411) in that it affects all processes run in the console session, not just the current process, so it's not clear cut if attempting to force ENABLE_VIRTUAL_TERMINAL_PROCESSING to be enabled is always the 'right' thing to do. If we can get away with just #15282, it seems like that might be the 'safer' way to go.

conversely, this would also mean that if utf8 is becoming unconditionally prescribed, enabling virtual terminal in the same vein would also be fair game?

@squeek502
Copy link
Collaborator

This is similar to #7600 (see also #12400 and #14411) in that it affects all processes run in the console session, not just the current process

I was wrong about this, apologies for not checking my assumptions. See #20172 for a revivial of the spirit of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants