starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Can we get rid of the argb.h header? #16055

New Issue
Open
opened 2026-01-31 04:56:11 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 04:56:11 +00:00
Owner
Copy Link

Originally created by @j4james on GitHub (Dec 6, 2021).

Description of the new feature/enhancement

The argb.h header redefines the RGB macro so that it creates an COLORREF with the alpha set to 255 rather than 0, and it also provide an ARGB macro for creating a color with a custom alpha value. The only place this is really used is in the Terminal constructor, setting the background color with an alpha value of 0.

My understanding is that the intent was to have most colors being opaque, but the default background being transparent when using acrylic in the DX renderer. However, if we just left the COLORREF alpha as 0, it should still end up being set correctly at render time by the GetAttributeColors call. See here:

bb71179a24/src/cascadia/TerminalCore/terminalrenderdata.cpp (L93-L99)

And if I'm correct in my understanding, there are actually a few places in the code where we are messing with the alpha values that really shouldn't be necessary, and things would be a lot simpler if we just left the alpha alone.

Proposed technical implementation details (optional)

  1. Delete the argb.h header file and replace all uses of ARGB with RGB.
  2. Get rid of the SetColorTableAlpha function that is used to set color table alpha to 255.
  3. Drop all the unnecessary til::color::with_alpha calls. See below for more details.

When casting a til::color to a COLORREF, the alpha is automatically set to zero, so there should never be a need to precede that cast with a with_alpha(0) call. Cases like this:

094273b995/src/cascadia/TerminalCore/Terminal.cpp (L189)
and this:
094273b995/src/cascadia/TerminalCore/Terminal.cpp (L1346)

Similarly, when constructing a til::color from a COLORREF, the alpha is automatically set to 255, so there should never be a need to follow that with a with_alpha(255) call. Cases like this:

094273b995/src/cascadia/TerminalControl/ControlCore.cpp (L1104)
and this:
094273b995/src/cascadia/TerminalCore/Terminal.cpp (L1316)

Another variant of this is constructing a til::color from a COLORREF and adding OPACITY_OPAQUE to set the alpha to 255. That serves no purpose if the constructor is going to do the exact same thing. For example here:

2353349fe5/src/renderer/dx/CustomTextRenderer.cpp (L351)

Finally, there are a few cases that return a color where the alpha is already set to 255, which I think should not require a with_alpha(255) call. I'm not absolutely certain about these, but as much as I've tried I've never found a way to set these colors without a 255 alpha.

094273b995/src/cascadia/TerminalControl/TermControl.cpp (L453)
094273b995/src/cascadia/TerminalCore/Terminal.cpp (L140)
094273b995/src/cascadia/TerminalCore/Terminal.cpp (L145)

I'm happy to create a PR with all of the changes proposed above, but I just want to make sure I haven't misunderstood something, and there's not actually a good reason why we're doing things this way.

Originally created by @j4james on GitHub (Dec 6, 2021). # Description of the new feature/enhancement The [`argb.h`](https://github.com/microsoft/terminal/blob/fb597ed304ec6eef245405c9652e9b8a029b821f/src/inc/argb.h) header redefines the `RGB` macro so that it creates an `COLORREF` with the alpha set to 255 rather than 0, and it also provide an `ARGB` macro for creating a color with a custom alpha value. The only place this is really used is in the `Terminal` constructor, setting the background color with an alpha value of 0. My understanding is that the intent was to have most colors being opaque, but the default background being transparent when using acrylic in the DX renderer. However, if we just left the `COLORREF` alpha as 0, it should still end up being set correctly at render time by the `GetAttributeColors` call. See here: https://github.com/microsoft/terminal/blob/bb71179a24ff186eab999676732afff8a345cc12/src/cascadia/TerminalCore/terminalrenderdata.cpp#L93-L99 And if I'm correct in my understanding, there are actually a few places in the code where we are messing with the alpha values that really shouldn't be necessary, and things would be a lot simpler if we just left the alpha alone. # Proposed technical implementation details (optional) 1. Delete the `argb.h` header file and replace all uses of `ARGB` with `RGB`. 2. Get rid of the `SetColorTableAlpha` function that is used to set color table alpha to 255. 3. Drop all the unnecessary `til::color::with_alpha` calls. See below for more details. When casting a `til::color` to a `COLORREF`, the alpha is automatically set to zero, so there should never be a need to precede that cast with a `with_alpha(0)` call. Cases like this: https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalCore/Terminal.cpp#L189 and this: https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalCore/Terminal.cpp#L1346 Similarly, when constructing a `til::color` from a `COLORREF`, the alpha is automatically set to 255, so there should never be a need to follow that with a `with_alpha(255)` call. Cases like this: https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalControl/ControlCore.cpp#L1104 and this: https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalCore/Terminal.cpp#L1316 Another variant of this is constructing a `til::color` from a `COLORREF` and adding `OPACITY_OPAQUE` to set the alpha to 255. That serves no purpose if the constructor is going to do the exact same thing. For example here: https://github.com/microsoft/terminal/blob/2353349fe5ad613f6efdb832daef19375acffb6c/src/renderer/dx/CustomTextRenderer.cpp#L351 Finally, there are a few cases that return a color where the alpha is already set to 255, which I _think_ should not require a `with_alpha(255)` call. I'm not absolutely certain about these, but as much as I've tried I've never found a way to set these colors without a 255 alpha. https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalControl/TermControl.cpp#L453 https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalCore/Terminal.cpp#L140 https://github.com/microsoft/terminal/blob/094273b995352ff9d80f36279c297488b756df1d/src/cascadia/TerminalCore/Terminal.cpp#L145 I'm happy to create a PR with all of the changes proposed above, but I just want to make sure I haven't misunderstood something, and there's not actually a good reason why we're doing things this way.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-CodeHealth Help Wanted Issue-Task Needs-Triage Product-Meta Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#16055
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?