Update WT's icon at runtime to match high-contrast as applicable #7971

DHowett · 2020-10-20T01:19:43Z

This commit introduces 8 more variants of the .ICO file, embeds the
right ones into WindowsTerminal.exe, and adds code that will select the
most appropriate icon at runtime.

Since we're a Centennial application, the "application" icon inside our
package isn't used by the shell for the taskbar thumbnails or the
Alt-Tab window.

To quote J. Tippet,

I believe there are two possible fixes:

Fix the OS shell to prefer the MRT icon instead of preferring the
win32 icon

Add alternate versions of /res/terminal.ico
The 1st fix is clearly better, since it benefits any hybrid app. But
the 2nd fix is much easier, since it'd just take about an hour to gin up
a new .ico file and hack the .RC file to refer to it when building the
preview flavor.

... and to quote Michael Ratanapintha,

Basically, if your MSIX-packaged desktop app's image resources are
separate files or even separate MSIX packages, they may be loaded by
MRT. If they're embedded in the .exe, they're the old-fashioned Win32
resources Mr. Tippet is referring to.

This is the "2nd fix."

Fixes #6777

Co-authored-by: Jeffrey Tippet jtippet@ntdev.microsoft.com

src/cascadia/WindowsTerminal/icon.cpp

metathinker · 2020-10-20T02:57:59Z

To quote J. Tippet (@jtippet),
... and to quote M. Ratanapintha (@metathinker),

I don't know how Jeff likes to be called, but I personally prefer to be called just Michael, thank you very much 🙂

DHowett · 2020-10-20T02:59:08Z

@metathinker uniformity defeated common sense. Thanks!

metathinker · 2020-10-20T03:00:59Z

Oh, I like both words of my name. I just find it easier to teach people how to spell and pronounce my first name, though :)

src/cascadia/WindowsTerminal/resource.h

src/cascadia/WindowsTerminal/icon.cpp

jtippet · 2020-10-20T23:31:29Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

@@ -147,6 +148,8 @@ void IslandWindow::_HandleCreateWindow(const WPARAM, const LPARAM lParam) noexce
    ShowWindow(_window.get(), nCmdShow);

    UpdateWindow(_window.get());
+
+    UpdateWindowIconForActiveMetrics(_window.get());


did you also mean to call this from WM_THEMECHANGED ?

I did, but I chose to defer that until we also get WM_SETTINGSCHANGED.

jtippet · 2020-10-20T23:36:09Z

src/cascadia/WindowsTerminal/resource.h

+#define IDI_APPICON_HC_W 103
+
+#define IDI_APPICON_DEV 104
+#define IDI_APPICON_DEV_HC_B 105


so i am just now noticing that the DEV_HC icons do not actually have the "DEV" badge, even though the SVG has it. that appears to be a bug left over from when i monkeyed with the icons. i just tried running the script, and the icons got better... i wonder how i overlooked doing this? but can i request that you slip this in, while you're at it?

.\Generate-TerminalAssets.ps1 -path .\Terminal_Dev.svg -HighContrastPath .\Terminal_Dev_HC.svg -Destination .\images-dev

Sure! Good catch.

jtippet · 2020-10-20T23:39:41Z

src/cascadia/WindowsTerminal/resource.h

+#define IDI_APPICON_HC_W 103
+
+#define IDI_APPICON_DEV 104
+#define IDI_APPICON_DEV_HC_B 105


btw, maybe also update .\Generate-TerminalAssets.ps1 to automate the creation of the win32 assets (terminal-Pre_contrast-black.ico et al).

jtippet · 2020-10-20T23:44:16Z

src/cascadia/WindowsTerminal/WindowsTerminal.rc

@@ -53,6 +53,16 @@ END
 // Icon with lowest ID value placed first to ensure application icon
 // remains consistent on all systems.
 IDI_APPICON             ICON                    "..\\..\\..\\res\\terminal.ico"


this feels like the sort of thing that would be appropriate to use #ifdefs on -- there's no need to ship all icons in all variants, and no need to spend CPU at runtime to compute the build variant (imho). does your project system expose any preprocessor macros for this?

i'm suggesting something like

#if OFFICIAL_BUILD IDI_APPICON ICON "res\\terminal.ico" IDI_APPICON_HC ICON "res\\terminal_contrast-black.ico" #elif PREVIEW_BUILD IDI_APPICON ICON "res\\terminal-Pre.ico" IDI_APPICON_HC ICON "res\\terminal-Pre_contrast-black.ico" #else IDI_APPICON ICON "res\\terminal-Dev.ico" IDI_APPICON_HC ICON "res\\terminal-Dev_contrast-black.ico" #endif

So, right now Terminal doesn't build different variants for its different packages.

Our original intent with this build tooling was that a single build would produce both packages from the same assets such that our ring promotion engineer (so, me) could take the same package and stage it for both Preview and Stable.

Given that we re-spin every stable build anew off backported changes from the last preview, I'm not sure we're benefitting from that, but I'm not sure this is the place to change that. I'll happily hold this change back until I do that other thing.

jtippet · 2020-10-20T23:50:38Z

I don't know how Jeff likes to be called, but I personally prefer to be called just Michael, thank you very much 🙂

To avoid ambiguity with other Jeffs, I prefer to be called 272fc969-1ffe-4e01-9589-5914c08d1e9a.

DHowett · 2020-10-20T23:59:15Z

This explains a lot.

Our build pipeline was originally set up such that we could take any binaries from the Terminal build and seamlessly re-package them with the release or preview livery. My initial plan was to stamp a stable and preview build at the same time, out of the same bits, to make ring promotion easier. I've never done that. For the last five releases, we've just re-cut a new stable build along with the new preview build, usually because we want to backport some fixes to stable. This commit introduces preprocessor defines, detectable through CL and RC, for any project that wants them. Right now, that's just going to be WindowsTerminal.vcxproj (since it hosts the icons and the app entry point). This list may be extended to include wt (the shim executable) and the shell extension at some future date. This will greatly simplify the logic in #7971, as we'll no longer need to detect if we're dev or preview at runtime. It may also simplify the logic in the shell extension for determining whether we're Dev or not.

This commit introduces 8 more variants of the .ICO file, embeds them into WindowsTerminal.exe, and adds code that will select the most appropriate icon at runtime. Since we're a Centennial application, the "application" icon inside our package isn't used by the shell for the taskbar thumbnails or the Alt-Tab window. To quote J. Tippet, > I believe there are two possible fixes: > > 1. Fix the OS shell to prefer the MRT icon instead of preferring the > win32 icon > 2. Add alternate versions of /res/terminal.ico > The 1st fix is clearly better, since it benefits any hybrid app. But > the 2nd fix is much easier, since it'd just take about an hour to gin up > a new .ico file and hack the .RC file to refer to it when building the > preview flavor. ... and to quote M. Ratanapintha, > Basically, if your MSIX-packaged desktop app's image resources are > separate files or even separate MSIX packages, they may be loaded by > MRT. If they're embedded in the .exe, they're the old-fashioned Win32 > resources Mr. Tippet is referring to. This is the "2nd fix." Fixes #6777

jtippet · 2020-10-22T20:20:34Z

So, uh, someone wrote a linting tool for .ICO files, and it really doesn't like your .ICOs. I guess ImageMagick isn't magic at making .ICOs?

PS C:\> dotnet IcoInfo.dll -i images\terminal_contrast-white.ico
File: images\terminal_contrast-white.ico
terminal_contrast-white.ico(1): Warning ICO241: ICO files require the embedded PNG image to be encoded in RGBA32 format; this is GrayscaleAlpha
  Frame #1
    Encoding:      PNG
    Bytes on disk: 1886
                   Actual     Claimed in ICO header
    Width:            256                       256
    Height:           256                       256
    Bit depth:         16                        32

terminal_contrast-white.ico(2): Error ICO131: BITMAPINFOHEADER.biXPelsPerMeter should be 0, was 5038.
More information online:
    ICO131: https://github.com/jtippet/IcoTools/wiki/ICO131
    ICO241: https://github.com/jtippet/IcoTools/wiki/ICO241

.ICO files can contain multiple images, which are differentiated by <Height, Width, ColorDepth>. (These days, color depth is obsolete and you can just use 32bpp everywhere.) When you call LoadImageW on an icon resource, the loader tries to find a perfect match for the size & depth. If it can't, it finds the image that's heuristically "closest", then uses a crappy scaling algorithm to resize it.

Rather than (a) paying the CPU cost of resizing at runtime, (b) paying the memory cost of having a bitmap in memory versus one that's just on disk, and (c) letting win32k's crappy scaler handle your bitmap, it's really better to ship an ICO bundled with prepared image sizes that you expect to use. (In this way, a .ICO is a lot like a MRT bundle, except worse in pretty much every way).

I think you're trying to do this right, since the .PS1 script mentions a variety of win32 asset sizes. But whatever ImageMagick is doing... you wound up with a .ICO that has a single 256x256 image in it. That's pretty much guaranteed to need to be scaled down whenever it's used.

As an additional wrinkle: any individual image in a .ICO can either be stored as a .BMP file or as a .PNG file. But in both cases, .ICO only tolerates a kooky subset of the respective image formats. For PNG, it only supports RGBA32 encoding; you can't just use any old PNG encoding. This gotcha will getcha bad, because the OS has at least two separate .ICO decoders: one in user32, and another in WIC. The user32 decoder tolerates any valid PNG file, but WIC will reject your image if it's not exactly RGBA32. So your icon will be broken in some GUI surfaces, depending on whether that GUI uses user32 or WIC to draw icons.

DHowett · 2020-10-22T20:22:46Z

This is strange. VS and other tools are showing all 6 icon variants in the .ico file. Shell handles it well, too. That's quite curious.

DHowett · 2020-10-22T20:24:38Z

Sorry-- by "well" I mean that shell uses the 16x16 variant in the 16x16 surfaces.

jtippet · 2020-10-22T20:25:06Z

Oh so it is... that's because the tool is bailing out when it sees the next error

terminal_contrast-white.ico(2): Error ICO131: BITMAPINFOHEADER.biXPelsPerMeter should be 0, was 5038.

jtippet · 2020-10-22T20:26:36Z

Ok, let me retract most of the commentary about putting multiple images in there. I'll stick with the comment about the 256x256 tile though: that needs to be converted to RGBA32, or else the 256px icon won't show up in any WIC-rasterized surface.

DHowett · 2020-10-23T00:28:45Z

(your icon toolset is very cool. Do you want me to snag the broken files from this commit and file an issue titled, generally, "this bad icon file makes IcoInfo upset"?)

jtippet · 2020-10-23T01:07:05Z

I already reported the bug to ImageMagick, who were amazingly quick to respond: ImageMagick/ImageMagick#2756

Meanwhile, I know you didn't expect to spend multiple days on this PR, and I acknowledge that much of that is noise from me. So I've generated compliant icons here, if you want to just cherry-pick them: jtippet@5c2e611

They were created as the output of:

del terminal.ico
icocat -i terminal.ico -s _intermediate.standard.256.png
icocat -i terminal.ico -s _intermediate.standard.64.png
. . . all other sizes . . .
icocrush -i terminal.ico --write-in-place --preferred-format PngLargeImages --emit-masked-image-pixel Compliant

They contain a structure like this, which resembles the original terminal.ico:

File: images-Pre\terminal_contrast-white.ico
  Frame #1
    Encoding:      PNG
    Bytes on disk: 9814
                   Actual     Claimed in ICO header
    Width:            256                       256
    Height:           256                       256
    Bit depth:         32                        32

  Frame #2
    Encoding:      Bitmap
    Bitmap type:   32-bit ARGB
    Bytes on disk: 16952
                   Actual     Claimed in ICO header
    Width:             64                        64
    Height:            64                        64
    Bit depth:         32                        32

  Frame #3
    Encoding:      Bitmap
    Bitmap type:   32-bit ARGB
    Bytes on disk: 9656
                   Actual     Claimed in ICO header
    Width:             48                        48
    Height:            48                        48
    Bit depth:         32                        32

  Frame #4
    Encoding:      Bitmap
    Bitmap type:   32-bit ARGB
    Bytes on disk: 4280
                   Actual     Claimed in ICO header
    Width:             32                        32
    Height:            32                        32
    Bit depth:         32                        32

  Frame #5
    Encoding:      Bitmap
    Bitmap type:   32-bit ARGB
    Bytes on disk: 1736
                   Actual     Claimed in ICO header
    Width:             20                        20
    Height:            20                        20
    Bit depth:         32                        32

  Frame #6
    Encoding:      Bitmap
    Bitmap type:   32-bit ARGB
    Bytes on disk: 2456
                   Actual     Claimed in ICO header
    Width:             24                        24
    Height:            24                        24
    Bit depth:         32                        32

  Frame #7
    Encoding:      Bitmap
    Bitmap type:   32-bit ARGB
    Bytes on disk: 1144
                   Actual     Claimed in ICO header
    Width:             16                        16
    Height:            16                        16
    Bit depth:         32                        32

Needless tangent

There's kind of an interesting tradeoff on whether to use BMP or PNG for any particular image. In this case, the PNG is always smaller than the BMP, even for the 16x16 image. So if you say "shrink the disk, at all costs", you'd encode everything as PNG. But there's a couple downsides to that:

Windows XP doesn't support PNG in ICOs, so you'll confuse anyone running ... that.
PNGs take more CPU to decode and display.

So I decided to encode the 256x256 as PNG, and everything smaller as BMP. I chose this heuristic because that's what your original terminal.ico does ;). If you were to do PNGs everywhere, it'd bring the filesize from 43kb to 12kb, which is kind of surprising, but not worth forcing someone to load up a PNG decoder just to look at your app's icon.

DHowett · 2020-10-23T01:11:13Z

I seriously appreciate that. Thank you.

PNGs for 256x256 and BMPs for everything else is totally fine with me.

zadjii-msft

This is pretty neat, not gonna lie

We need a better story for our icons. The preview icon at size <=32 is "tucked in" so that we didn't compromise the available horizontal space to make the lozenge float off the edge. Regenerating the .ico from the original sources makes it a small blurry mess.

ghost · 2020-10-28T00:04:07Z

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.

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 · 2020-11-11T18:12:32Z

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

Handy links:

ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 20, 2020

DHowett commented Oct 20, 2020

View reviewed changes

src/cascadia/WindowsTerminal/icon.cpp Outdated Show resolved Hide resolved

WSLUser mentioned this pull request Oct 20, 2020

Optimize images across the repo #7979

Closed

DHowett requested a review from zadjii-msft October 20, 2020 22:21

jtippet reviewed Oct 20, 2020

View reviewed changes

DHowett changed the title ~~Update IslandWindow's icon to match dev/pre and hc/not-hc~~ Update WT's icon at runtime to match dev/pre and hc/not-hc Oct 20, 2020

DHowett mentioned this pull request Oct 21, 2020

Expose WindowsTerminalBranding to the preprocessor (optionally) #7986

Merged

DHowett added 5 commits October 21, 2020 15:24

Make better

db57695

Generate .ico files

eb3a134

Regenerate dev, generate dev+pre+default .ico files

ff72200

Don't do runtime detection of the package type.

01ffc19

DHowett force-pushed the dev/duhowett/hax/icons branch from d7c6690 to 01ffc19 Compare October 21, 2020 23:24

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:26

zadjii-msft self-assigned this Oct 22, 2020

Improve interoperability of the .ICO files' encoding

ef5403d

zadjii-msft approved these changes Oct 27, 2020

View reviewed changes

zadjii-msft assigned DHowett and unassigned zadjii-msft Oct 27, 2020

miniksa approved these changes Oct 27, 2020

View reviewed changes

DHowett added 3 commits October 27, 2020 16:27

Merge remote-tracking branch 'origin/main' into dev/duhowett/hax/icons

aa4b7a4

Update the icon when HC turns on/off

5b89c5a

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 28, 2020

DHowett changed the title ~~Update WT's icon at runtime to match dev/pre and hc/not-hc~~ Update WT's icon at runtime to match high-contrast when applicable Oct 28, 2020

DHowett changed the title ~~Update WT's icon at runtime to match high-contrast when applicable~~ Update WT's icon at runtime to match high-contrast as applicable Oct 28, 2020

FOR THE LOVE OF

b501768

ghost merged commit 5a1c931 into main Oct 28, 2020

ghost deleted the dev/duhowett/hax/icons branch October 28, 2020 00:39

ghost mentioned this pull request Nov 11, 2020

Terminal Preview and Terminal have the same icon in Alt-Tab and Win-Tab window switchers #6777

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update WT's icon at runtime to match high-contrast as applicable #7971

Update WT's icon at runtime to match high-contrast as applicable #7971

DHowett commented Oct 20, 2020 •

edited

Loading

metathinker commented Oct 20, 2020

DHowett commented Oct 20, 2020

metathinker commented Oct 20, 2020 •

edited

Loading

jtippet Oct 20, 2020

DHowett Oct 20, 2020

jtippet Oct 20, 2020

DHowett Oct 21, 2020

jtippet Oct 20, 2020

jtippet Oct 20, 2020

DHowett Oct 21, 2020

jtippet commented Oct 20, 2020

DHowett commented Oct 20, 2020

jtippet commented Oct 22, 2020

DHowett commented Oct 22, 2020

DHowett commented Oct 22, 2020

jtippet commented Oct 22, 2020

jtippet commented Oct 22, 2020

DHowett commented Oct 23, 2020

jtippet commented Oct 23, 2020

DHowett commented Oct 23, 2020

zadjii-msft left a comment

ghost commented Oct 28, 2020

ghost commented Nov 11, 2020

Update WT's icon at runtime to match high-contrast as applicable #7971

Update WT's icon at runtime to match high-contrast as applicable #7971

Conversation

DHowett commented Oct 20, 2020 • edited Loading

metathinker commented Oct 20, 2020

DHowett commented Oct 20, 2020

metathinker commented Oct 20, 2020 • edited Loading

jtippet Oct 20, 2020

Choose a reason for hiding this comment

DHowett Oct 20, 2020

Choose a reason for hiding this comment

jtippet Oct 20, 2020

Choose a reason for hiding this comment

DHowett Oct 21, 2020

Choose a reason for hiding this comment

jtippet Oct 20, 2020

Choose a reason for hiding this comment

jtippet Oct 20, 2020

Choose a reason for hiding this comment

DHowett Oct 21, 2020

Choose a reason for hiding this comment

jtippet commented Oct 20, 2020

DHowett commented Oct 20, 2020

jtippet commented Oct 22, 2020

DHowett commented Oct 22, 2020

DHowett commented Oct 22, 2020

jtippet commented Oct 22, 2020

jtippet commented Oct 22, 2020

DHowett commented Oct 23, 2020

jtippet commented Oct 23, 2020

Needless tangent

DHowett commented Oct 23, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Oct 28, 2020

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 Nov 11, 2020

DHowett commented Oct 20, 2020 •

edited

Loading

metathinker commented Oct 20, 2020 •

edited

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.