[windows] Fix for background colours besides black #1488
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| new_attr = (buffer_info.wAttributes & ~7); | ||
| new_attr &= ~8; // Since 'wAttributes' could have been hi-intensity at startup. |
There was a problem hiding this comment.
What are 7 and 8 here? Can we avoid magic numbers?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is https://docs.microsoft.com/en-us/windows/console/console-screen-buffers#character-attributes lying and there are no macros for these magic numbers?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Let me rephraze: can you not just replace 8 with BACKGROUND_INTENSITY and 7 with FOREGROUND_INTENSITY?
| new_attr = (buffer_info.wAttributes & ~7); | ||
| new_attr &= ~8; // Since 'wAttributes' could have been hi-intensity at startup. |
There was a problem hiding this comment.
@gvanem To clarify/reiterate, what happens if you literally apply the following 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.
Fix for issue #1487.