ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
33.67k stars 2.47k forks source link

`tty.Config.setColor` is no longer unified across Escape Codes/Windows Console APIs #15976

Open squeek502 opened 1 year ago

squeek502 commented 1 year ago

This is something of a follow-up to https://github.com/ziglang/zig/pull/15806.

Before that PR, all colors had an implicit 'bold,' and all colors in the Windows API had an implicit FOREGROUND_INTENSITY.

I've pushed an update to make colors not bold by default (the ;1m), which kind of defeated the purpose of .bold

This makes sense to do from the perspective of ANSI escape sequences, but it complicates making setColor work similarly when the Config is .windows_api. In the Windows API, there's no builtin way to persist something like .bold, so attempting to use .bold and then set a color does not have the intended effect--the 'bold' is always lost after setting the color.

About bold

Historically, 'bold' has been implemented as making the text a brighter color rather than actually bold. From Wikipedia:

Quite a few terminals implemented "bold" (SGR code 1) as a brighter color rather than a different font, thus providing 8 additional foreground colors.

This is still an option in e.g. Gnome terminal ('Show bold text in bright colors'), and is the default behavior in Windows Terminal. This doesn't really affect the problem, but it's worth noting when thinking about potential solutions.

A visualization

This is cmd.exe in Windows Terminal with the default font/settings:

colors-bold-default

(note: dim is not expected to match, there's no 'dim' equivalent in the Windows Console API)

Note that with modified settings to use a font that supports bold text and to render bold text as bold only (instead of the default setting of rendering bold text as 'bright colors'), the difference is less impactful visually but the difference can still be seen:

Output with modified settings ![colors-bold-as-bold](https://github.com/ziglang/zig/assets/2389051/27f15616-0899-43a6-8cd5-000e646c8a75)
Code used to generate this output ```zig const std = @import("std"); const is_windows = @import("builtin").os.tag == .windows; pub fn main() !void { const stderr = std.io.getStdErr(); if (is_windows) { _ = std.os.windows.kernel32.SetConsoleOutputCP(65001); } const ansi_config = std.io.tty.Config{ .escape_codes = {} }; std.debug.print("ANSI Escape Codes:\n", .{}); try dumpColors(stderr.writer(), ansi_config); if (is_windows) { var info: std.os.windows.CONSOLE_SCREEN_BUFFER_INFO = undefined; std.debug.assert(std.os.windows.kernel32.GetConsoleScreenBufferInfo(stderr.handle, &info) == std.os.windows.TRUE); const console_config = std.io.tty.Config{ .windows_api = .{ .handle = stderr.handle, .reset_attributes = info.wAttributes, } }; std.debug.print("\nConsole API:\n", .{}); try dumpColors(stderr.writer(), console_config); } } fn dumpColors(writer: anytype, config: std.io.tty.Config) !void { inline for (@typeInfo(std.io.tty.Color).Enum.fields) |field| { try config.setColor(writer, @field(std.io.tty.Color, field.name)); try writer.writeAll("■"); if (@field(std.io.tty.Color, field.name) == .black) { try config.setColor(writer, .reset); } try writer.writeAll(" "); try writer.writeAll(field.name); try writer.writeByteNTimes(' ', 20 - field.name.len); try config.setColor(writer, .bold); try config.setColor(writer, @field(std.io.tty.Color, field.name)); try writer.writeAll("■"); if (@field(std.io.tty.Color, field.name) == .black) { try config.setColor(writer, .reset); try config.setColor(writer, .bold); } try writer.writeAll(" bold "); try writer.writeAll(field.name); try config.setColor(writer, .reset); try writer.writeByteNTimes(' ', 20 - field.name.len); try config.setColor(writer, .dim); try config.setColor(writer, @field(std.io.tty.Color, field.name)); try writer.writeAll("■"); if (@field(std.io.tty.Color, field.name) == .black) { try config.setColor(writer, .reset); try config.setColor(writer, .dim); } try writer.writeAll(" dim "); try writer.writeAll(field.name); try config.setColor(writer, .reset); try writer.writeAll("\n"); } } ```

Potential solutions

I personally see some merit to all of the potential solutions, and I may be missing some potential solutions.

cc @linusg if you'd like to weigh in

andrewrk commented 1 year ago

Thanks for this writeup, @squeek502.

If you have a preference, I'd be happy to back your suggestion.

Personally, I think it was nice how it was before:

Go back to every-color-is-bold/every-color-is-intense. This gives us an easy path to cross-API conformity but makes the setColor API less useful/flexible than it could be.

I think it's perfectly reasonable to have a minimal API that is explicitly designed to be the cross-platform common denominator. I also think it's reasonable to have an API that is intended to provide maximum flexibility on all platforms. As for which one belongs in the standard library, I could see an argument either way, but I can say for sure that the zig compiler itself wants the former.

linusg commented 1 year ago

I saw that I got tagged in the original issue but missed it, apologies!


While I understand the desire to have equally simple and capable APIs regardless of the platform, setting a text rendering to bold for any color at the same time decreases usability of the API for environments (in this specific case, terminals) where colored text rendering is supported.

I've patched my local zig checkout to have 5d3f3cae64759091ad6dc7b1e2e7f15728c8b05a partially reverted:

image

The bold+color combination is too much for my personal taste, and ultimately not something I've asked it to do (I told it "make this text colored").

So if this became the default again I'd revert to implementing my own text styling, and obviously I'd like to avoid that. I would be happy with this being the default and some way of opting out, however - even if that's just a flag in the tty.Config that modifies the internal behavior of setColor() (which can keep taking a const Config).

squeek502 commented 1 year ago

I think it might be okay to keep the Windows Console API behaving differently, as the behavior is not too different (can basically explain it as 'bold and dim styles are not supported by the Windows Console API'), and since https://github.com/ziglang/zig/pull/16080 has been merged, Windows will use ANSI escape codes more often.

Although one thing that is a problem is that the order of setting the style and the color matters:

// ends up as red but not bold with .windows_api
try tty_conf.setColor(writer, .bold);
try tty_conf.setColor(writer, .red);

// ends up as bright white with .windows_api
try tty_conf.setColor(writer, .red);
try tty_conf.setColor(writer, .bold);

This is kind of an unsolvable problem with the current API. We could make .bold do nothing on .windows_api to avoid the issue there, but the same problem would exist with .dim and that's less solvable since .dim on its own is something that's used pretty often.


Another possible way to address this would be to add an API that applies both a style and a color in one call. Something like:

pub const Color = enum {
    // ...
};
pub const Style = enum {
    dim,
    bold,
    reset,
};

pub const Config = union(enum) {
    // ...

    /// Set both color and style
    pub fn setColorAndStyle(conf: Config, out_stream: anytype, color: Color, style: Style) !void {
        // ...
    }

    /// Note: Bold and dim styles are lost during this function when using .windows_api,
    ///       use setColorAndStyle to get consistent behavior across all APIs
    pub fn setColor(conf: Config, out_stream: antype, color: Color) !void {
        // ...
    }

    /// Note: Bold and dim styles are not persisted across color changes when using .windows_api,
    ///       use setColorAndStyle to get consistent behavior across all APIs
    pub fn setStyle(conf: Config, out_stream: anytype, style: Style) !void {
        // ...
    }
}

I'm sure it could be improved a bit more but that'd be the idea.

andrewrk commented 1 year ago

is this related to #16526?