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.

Sign up for GitHub

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] Fix for background colours besides black #1488

Closed
wants to merge 1 commit into from
Closed

[windows] Fix for background colours besides black #1488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/colorprint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,16 @@ void ColorPrintf(std::ostream& out, LogColor color, const char* fmt,
CONSOLE_SCREEN_BUFFER_INFO buffer_info;
GetConsoleScreenBufferInfo(stdout_handle, &buffer_info);
const WORD old_color_attrs = buffer_info.wAttributes;

WORD new_attr;

// We need to flush the stream buffers into the console before each
// SetConsoleTextAttribute call lest it affect the text that is already
// printed but has not yet reached the console.
fflush(stdout);
new_attr = (buffer_info.wAttributes & ~7);
new_attr &= ~8; // Since 'wAttributes' could have been hi-intensity at startup.
Comment on lines +145 to +146
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are 7 and 8 here? Can we avoid magic numbers?

Copy link
Author

Choose a reason for hiding this comment

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

I simply mask off the foreground bits (~7) and the hi-intensity background colour bit (~8).
Should be clear. And there's no WinCon values for these AFAIK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is https://docs.microsoft.com/en-us/windows/console/console-screen-buffers#character-attributes lying and there are no macros for these magic numbers?

Copy link
Author

Choose a reason for hiding this comment

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

I give up.

Copy link
Member

Choose a reason for hiding this comment

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

the comment suggests there's a bug here. BACKGROUND_INTENSITY is 0x0080 i think. FOREGROUND_INTENSITY is 0x0008.

same with the masking of ~7 if you're trying to mask off the backgrounds.. not sure how this works (i am not a windows dev though).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me rephraze: can you not just replace 8 with BACKGROUND_INTENSITY and 7 with FOREGROUND_INTENSITY?

Comment on lines +145 to +146
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gvanem To clarify/reiterate, what happens if you literally apply the following change:

Suggested change
new_attr = (buffer_info.wAttributes & ~7);
new_attr &= ~8; // Since 'wAttributes' could have been hi-intensity at startup.
new_attr = (buffer_info.wAttributes & ~FOREGROUND_INTENSITY);
new_attr &= ~BACKGROUND_INTENSITY; // Since 'wAttributes' could have been hi-intensity at startup.

Does it not compile?
The problem is that no one will have a clue what those 7 and 8 really meant,
and the docs imply that there are named macros, so we should use them.

SetConsoleTextAttribute(stdout_handle,
GetPlatformColorCode(color) | FOREGROUND_INTENSITY);
new_attr | GetPlatformColorCode(color) | FOREGROUND_INTENSITY);
vprintf(fmt, args);

fflush(stdout);
Expand Down