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: detect ANSI support in more terminals #15282

Closed
wants to merge 1 commit into from

Conversation

nolanderc
Copy link
Contributor

Checks if the underlying console supports ANSI escape sequences.

Resolves an issue where bold text would render as completely white in Windows Terminal, which was unreadable on a light background.

@perillo
Copy link
Contributor

perillo commented Apr 18, 2023

@nolanderc Does #15206 fix this issue?

@nolanderc
Copy link
Contributor Author

@perillo I don’t think so, because the code emitting the colors uses hardcoded values for windows unless ANSI escapes are detected. That is, even with the linked PR, the hardcoded values would still be used:

pub fn setColor(conf: Config, out_stream: anytype, color: Color) !void {

@perillo
Copy link
Contributor

perillo commented Apr 18, 2023

@perillo I don’t think so, because the code emitting the colors uses hardcoded values for windows unless ANSI escapes are detected. That is, even with the linked PR, the hardcoded values would still be used:

Color codes are all hardcoded.

On Windows we have:

 .White, .Bold => windows.FOREGROUND_RED | windows.FOREGROUND_GREEN | windows.FOREGROUND_BLUE | windows.FOREGROUND_INTENSITY

and on an ANSI compatible terminal, we have

"\x1b[37;1m"

that corresponds to white color with bold or increased intensity.

So it seems the issue is the use of ANSI colors on a terminal having a white background/light color theme.

@nolanderc
Copy link
Contributor Author

Ah yes, I seem to have forgotten to add a line to my commit... Originally, I had intended to split the white and bold styles into their own escape sequences. That way foreground colors are not overriden by the bold style, instead using the default (white on dark backgrounds, black on light ones).

@nolanderc
Copy link
Contributor Author

Nevermind, that was already there... 😅

@squeek502
Copy link
Collaborator

squeek502 commented May 22, 2023

I am in full support of this. @nolanderc you might want to rebase this branch on latest master so that it passes CI to make it more likely to get merged.


The new build runner means that on Windows the SetConsoleTextAttribute colors are not preserved when a test case fails. In combination with #13816 / #13723, this makes the output for expectEqualSlices much less useful.

With the changes in this PR, the colors are preserved (at least with Windows Terminal).

Before this PR:

15206-before

After this PR:

15206-after

@@ -226,6 +226,13 @@ pub const File = struct {
/// Test whether ANSI escape codes will be treated as such.
pub fn supportsAnsiEscapeCodes(self: File) bool {
if (builtin.os.tag == .windows) {
if (!os.isatty(self.handle)) return false;
Copy link
Collaborator

@squeek502 squeek502 May 22, 2023

Choose a reason for hiding this comment

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

This is redundant and can (should?) be removed, the implementation of os.isatty on Windows is:

        if (isCygwinPty(handle))
            return true;

        var out: windows.DWORD = undefined;
        return windows.kernel32.GetConsoleMode(handle, &out) != 0;

@andrewrk
Copy link
Member

andrewrk commented Jun 18, 2023

Closing abandoned PR (CI failures, unaddressed review comments). @squeek502 please feel free to pick this up in a new PR if you are interested.

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.

4 participants