Simplify the handling of alpha values in the color table #11900

j4james · 2021-12-08T00:25:34Z

This PR attempts to minimize the amount of fiddling we do with the alpha
color components, by storing all colors with a zero alpha (the default
for COLORREF values) and then leaving it up to the renderer to adjust
the final alpha value as required (which it was already doing anyway).

This gets rid of the argb.h header file, which was originally being
used to produce COLORREF values with custom alpha components, and thus
is no longer required. Anywhere that was using the ARGB macro is now
using a standard RGB macro with a 0 alpha.

The Utils::SetColorTableAlpha method has also been removed, since that
was only really used to force an alpha of 255 on all the color table
entries, which isn't necessary.

There were also a number of places where we were using
til::color::with_alpha, to switch alpha components back and forth
between 0 and 255, which have now been removed. Some of these were
essentially noops, because the til::color class already applied the
appropriate alpha changes when converting from or to a COLORREF.

I've manually run a few attribute rendering tests to check that the
colors were still working correctly, and the default background color is
appropriately transparent when in acrylic mode.

Closes #11885

j4james

My one concern is that I can't run the local settings tests, and I assume they don't run in CI either, so it's possible that something might be broken there. I don't think that's likely, but it would be good if someone could confirm that.

j4james · 2021-12-09T00:27:47Z

src/cascadia/LocalTests_SettingsModel/ColorSchemeTests.cpp

        std::array<COLORREF, COLOR_TABLE_SIZE> expectedCampbellTable;
        const auto campbellSpan = gsl::make_span(expectedCampbellTable);
        Utils::InitializeColorTable(campbellSpan);
-        Utils::SetColorTableAlpha(campbellSpan, 0);


This shouldn't be necessary, because the COLORREFs are automatically initialized with zero alpha when cast from til::color (which is what happens in the InitializeColorTable method).

j4james · 2021-12-09T00:30:27Z

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

-        VERIFY_ARE_EQUAL(ARGB(0, 0x45, 0x67, 0x89), terminalSettings4->CursorColor()); // from profile (no color scheme)
+        VERIFY_ARE_EQUAL(RGB(0x23, 0x45, 0x67), terminalSettings2->CursorColor()); // from profile (trumps color scheme)
+        VERIFY_ARE_EQUAL(RGB(0x34, 0x56, 0x78), terminalSettings3->CursorColor()); // from profile (not set in color scheme)
+        VERIFY_ARE_EQUAL(RGB(0x45, 0x67, 0x89), terminalSettings4->CursorColor()); // from profile (no color scheme)
        VERIFY_ARE_EQUAL(DEFAULT_CURSOR_COLOR, terminalSettings5->CursorColor()); // default


Since we're no longer including argb.h, the standard RGB macro will create COLORREFs with a zero alpha, which is no different from the previous ARGB usage here.

zadjii-msft

Thanks for cleaning up my mess!

carlos-zamora

Thanks!

carlos-zamora · 2021-12-15T18:01:38Z

@msftbot merge this in 10 minutes

ghost · 2021-12-15T18:01:44Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 15 Dec 2021 18:11:40 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

carlos-zamora · 2021-12-15T18:01:49Z

@msftbot merge this in 10 minutes

ghost · 2021-12-15T18:01:56Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 15 Dec 2021 18:11:54 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

Thanks for doing this! Puzzlingly, removing knowledge of alpha and alpha transition from a bunch of these layers will make future work on alpha colors easier. I love it.

ghost · 2021-12-15T18:06:40Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 5 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2022-02-03T18:22:36Z

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

j4james added 5 commits December 7, 2021 18:45

Remove all usage of argb.h

04e3eb1

Drop unnecessary with_alpha calls.

775249d

Drop unnecessary OPACITY_OPAQUE.

2c3a8ed

Drop more unnecessary with_alpha calls.

7f7c029

Get rid of the SetColorTableAlpha function.

3f65919

ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products. labels Dec 8, 2021

j4james marked this pull request as ready for review December 9, 2021 00:23

j4james commented Dec 9, 2021

View reviewed changes

zadjii-msft approved these changes Dec 15, 2021

View reviewed changes

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 15, 2021

ghost requested review from miniksa, carlos-zamora, DHowett, leonMSFT, PankajBhojwani and lhecker December 15, 2021 14:55

carlos-zamora approved these changes Dec 15, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 15, 2021

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 15, 2021

DHowett approved these changes Dec 15, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 15, 2021

ghost merged commit 8dfdfc4 into microsoft:main Dec 15, 2021

j4james deleted the nuke-argb branch December 28, 2021 20:54

ghost mentioned this pull request Feb 3, 2022

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the handling of alpha values in the color table #11900

Simplify the handling of alpha values in the color table #11900

j4james commented Dec 8, 2021 •

edited by DHowett

Loading

j4james left a comment

j4james Dec 9, 2021

j4james Dec 9, 2021

zadjii-msft left a comment

carlos-zamora left a comment

carlos-zamora commented Dec 15, 2021

ghost commented Dec 15, 2021

carlos-zamora commented Dec 15, 2021

ghost commented Dec 15, 2021

DHowett left a comment

ghost commented Dec 15, 2021

ghost commented Feb 3, 2022

Simplify the handling of alpha values in the color table #11900

Simplify the handling of alpha values in the color table #11900

Conversation

j4james commented Dec 8, 2021 • edited by DHowett Loading

j4james left a comment

Choose a reason for hiding this comment

j4james Dec 9, 2021

Choose a reason for hiding this comment

j4james Dec 9, 2021

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora commented Dec 15, 2021

ghost commented Dec 15, 2021

carlos-zamora commented Dec 15, 2021

ghost commented Dec 15, 2021

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Dec 15, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Feb 3, 2022

j4james commented Dec 8, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.