Skip to content

[CmdPal] WindowWalker Show the actual window icon instead of the process icon#42316

Merged
michaeljolley merged 9 commits intomicrosoft:mainfrom
Lee-WonJun:feature/issue-42260-windows-icon
Oct 20, 2025
Merged

[CmdPal] WindowWalker Show the actual window icon instead of the process icon#42316
michaeljolley merged 9 commits intomicrosoft:mainfrom
Lee-WonJun:feature/issue-42260-windows-icon

Conversation

@Lee-WonJun
Copy link
Copy Markdown
Contributor

image

Summary of the Pull Request

This is a PR for issue #42260.
It targets CmdPal’s WindowWalker and changes the icon retrieval to use SendMessage to obtain the window’s actual icon, instead of using the process icon.

To support this, I added a new configuration option.

image

PR Checklist

Detailed Description of the Pull Request / Additional comments

Actully, The ThumbnailHelper already contains code that converts an IntPtr hIcon into an IRandomAccessStream, as shown below:

 private static MemoryStream GetMemoryStreamFromIcon(IntPtr hIcon)
 {
     var memoryStream = new MemoryStream();

     // Ensure disposing the icon before freeing the handle
     using (var icon = Icon.FromHandle(hIcon))
     {
         icon.ToBitmap().Save(memoryStream, System.Drawing.Imaging.ImageFormat.Png);
     }

     // Clean up the unmanaged handle without risking a use-after-free.
     NativeMethods.DestroyIcon(hIcon);

     memoryStream.Position = 0;
     return memoryStream;
 }

 private static async Task<IRandomAccessStream?> FromHIconToStream(IntPtr hIcon)
 {
     var stream = new InMemoryRandomAccessStream();

     using var memoryStream = GetMemoryStreamFromIcon(hIcon); // this will DestroyIcon hIcon
     using var outputStream = stream.GetOutputStreamAt(0);
     using var dataWriter = new DataWriter(outputStream);

     dataWriter.WriteBytes(memoryStream.ToArray());
     await dataWriter.StoreAsync();
     await dataWriter.FlushAsync();

     return stream;
 }

Without modifying (or using) this code, I implemented the almost same logic directly in SwitchToWindowCommand (calling the async code with Wait to block synchronously). The reasons are:

  1. I wanted to limit changes to the WindowWalker project area. I don’t expect other extensions to need this behavior.
  2. Because this is resource-related work, exposing a public helper that pulls memory from an hIcon pointer seems risky—especially in a class like ThumbnailHelper.

Therefore, I implemented behavior that is nearly identical to the snippet above.

I did use using/Dispose where appropriate, but the InMemoryRandomAccessStream created for IconInfo.FromStream appears to use internal referencing; disposing it would be incorrect. For that reason I didn’t wrap it in a using. I’m not entirely sure whether GC will handle this cleanly.

However, based on the implementation of FromStream itself and its usage elsewhere (e.g., in ThumbnailHelper), this seems to be the correct usage pattern, though I’m not entirely sure.

Validation Steps Performed

@github-actions

This comment has been minimized.

@jiripolasek
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

return true;
}

iconHandle = NativeMethods.GetClassLongPtr(hwnd, Win32Constants.GCLP_HICONSM);

Check failure

Code scanning / check-spelling

Unrecognized Spelling

[HICONSM](#security-tab) is not a recognized word. \(unrecognized-spelling\)
/// <summary>
/// Retrieves a handle to the small icon associated with the class.
/// </summary>
public const int GCLP_HICONSM = -34;

Check failure

Code scanning / check-spelling

Unrecognized Spelling

[HICONSM](#security-tab) is not a recognized word. \(unrecognized-spelling\)
@github-actions

This comment has been minimized.

Co-authored-by: Jiří Polášek <[email protected]>
@github-actions
Copy link
Copy Markdown

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (4)

dfx
fdx
nullref
worktree

These words are not needed and should be removed DFX Worktree

To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:Lee-WonJun/PowerToys.git repository
on the feature/issue-42260-windows-icon branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/c635c2f3f714eec2fcf27b643a1919b9a811ef2e/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/18447200250/attempts/1' &&
git commit -m 'Update check-spelling metadata'
If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

@jiripolasek
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@michaeljolley michaeljolley added the Product-Command Palette Refers to the Command Palette utility label Oct 12, 2025
@jiripolasek
Copy link
Copy Markdown
Collaborator

@Lee-WonJun I'm not a maintainer — my apologies if there was any confusion. The true masters of Command Palette have already noticed this PR. I believe merges to main are currently limited to fixes until the next version is out (soon^TM).

That said, I did pull this PR and LGTM

@Lee-WonJun
Copy link
Copy Markdown
Contributor Author

Regardless of that, I really appreciate your interest — from the issue itself all the way to the code review!

Copy link
Copy Markdown
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

You'll never believe this, but I was actually just writing this same code this weekend:
image

So yes we should 100% do this

@Lee-WonJun
Copy link
Copy Markdown
Contributor Author

WOW what an amazing coincidence!

@niels9001 niels9001 added this to the PowerToys 0.96 milestone Oct 16, 2025
@michaeljolley michaeljolley merged commit f28d009 into microsoft:main Oct 20, 2025
10 checks passed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new setting to WindowWalker that allows users to choose between displaying window icons or process icons. The change introduces a "Use window icons" toggle setting that, when enabled, retrieves and displays the actual icon associated with each window instead of the process executable's icon.

  • Added a new UseWindowIcon boolean setting with localized strings
  • Implemented TryGetWindowIcon() method to retrieve window icons via Win32 APIs
  • Updated SwitchToWindowCommand to conditionally use window or process icons based on the setting

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
Properties/Resources.resx Added localized strings for the new "Use window icons" setting
Properties/Resources.Designer.cs Auto-generated properties for the new localized strings
Icons.cs Added a generic fallback app icon constant
Helpers/SettingsManager.cs Registered the new UseWindowIcon toggle setting
Helpers/ISettingsInterface.cs Added UseWindowIcon property to the settings interface
Helpers/NativeMethods.cs Added P/Invoke declarations for GetClassLongPtr, DestroyIcon, and related Win32 constants
Components/Window.cs Implemented TryGetWindowIcon method to retrieve window icons using multiple fallback strategies
Commands/SwitchToWindowCommand.cs Updated constructor to use window or process icons based on the setting
Tests/.../Settings.cs Updated test settings class to include the new UseWindowIcon parameter
.github/.../expect.txt Added HICONSM to spell-check dictionary
Files not reviewed (1)
  • src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowWalker/Properties/Resources.Designer.cs: Language not supported

Comment on lines +208 to +209
icon = System.Drawing.Icon.FromHandle((IntPtr)result);
NativeMethods.DestroyIcon((IntPtr)result);
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Calling DestroyIcon immediately after Icon.FromHandle is incorrect. Icon.FromHandle does not make a copy of the icon handle, so destroying it invalidates the Icon object. The icon should only be destroyed after it's no longer needed (after disposal). The existing ThumbnailHelper.GetMemoryStreamFromIcon pattern shows the correct approach: dispose the Icon first within a using block, then call DestroyIcon.

Copilot uses AI. Check for mistakes.
Comment on lines +215 to +216
icon = System.Drawing.Icon.FromHandle((IntPtr)result);
NativeMethods.DestroyIcon((IntPtr)result);
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Calling DestroyIcon immediately after Icon.FromHandle is incorrect. Icon.FromHandle does not make a copy of the icon handle, so destroying it invalidates the Icon object. The icon should only be destroyed after it's no longer needed (after disposal). The existing ThumbnailHelper.GetMemoryStreamFromIcon pattern shows the correct approach: dispose the Icon first within a using block, then call DestroyIcon.

Copilot uses AI. Check for mistakes.
Comment on lines +222 to +223
icon = System.Drawing.Icon.FromHandle((IntPtr)result);
NativeMethods.DestroyIcon((IntPtr)result);
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Calling DestroyIcon immediately after Icon.FromHandle is incorrect. Icon.FromHandle does not make a copy of the icon handle, so destroying it invalidates the Icon object. The icon should only be destroyed after it's no longer needed (after disposal). The existing ThumbnailHelper.GetMemoryStreamFromIcon pattern shows the correct approach: dispose the Icon first within a using block, then call DestroyIcon.

Copilot uses AI. Check for mistakes.
Comment on lines +231 to +232
icon = System.Drawing.Icon.FromHandle(iconHandle);
NativeMethods.DestroyIcon((IntPtr)iconHandle);
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Icons retrieved via GetClassLongPtr should not be destroyed with DestroyIcon, as they are class icons owned by the window class. Only icons returned by WM_GETICON (which creates copies) should be destroyed. Remove the DestroyIcon call for class icons.

Copilot uses AI. Check for mistakes.
Comment on lines +239 to +240
icon = System.Drawing.Icon.FromHandle(iconHandle);
NativeMethods.DestroyIcon((IntPtr)iconHandle);
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Icons retrieved via GetClassLongPtr should not be destroyed with DestroyIcon, as they are class icons owned by the window class. Only icons returned by WM_GETICON (which creates copies) should be destroyed. Remove the DestroyIcon call for class icons.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +44
dataWriter.StoreAsync().AsTask().Wait();
dataWriter.FlushAsync().AsTask().Wait();
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

Using .Wait() on async operations in a constructor can cause deadlocks. Consider making this code synchronous or refactoring to avoid blocking async calls. You could use GetAwaiter().GetResult() as a safer alternative, or restructure to perform icon loading asynchronously.

Suggested change
dataWriter.StoreAsync().AsTask().Wait();
dataWriter.FlushAsync().AsTask().Wait();
dataWriter.StoreAsync().AsTask().GetAwaiter().GetResult();
dataWriter.FlushAsync().AsTask().GetAwaiter().GetResult();

Copilot uses AI. Check for mistakes.
mirmirmirr pushed a commit to mirmirmirr/PowerToys that referenced this pull request Nov 9, 2025
…ocess icon (microsoft#42316)

<!-- Enter a brief description/summary of your PR here. What does it
fix/what does it change/how was it tested (even manually, if necessary)?
-->

<img width="629" height="767" alt="image"
src="https://github.com/user-attachments/assets/bc093640-db9d-4bc8-bc33-53729e692850"
/>


## Summary of the Pull Request

This is a PR for issue **microsoft#42260**.
It targets **CmdPal’s WindowWalker** and changes the icon retrieval to
use **SendMessage** to obtain the window’s actual icon, instead of using
the **process icon**.

To support this, I added a new configuration option.

<img width="400" height="401" alt="image"
src="https://github.com/user-attachments/assets/1a2d97a8-ff95-40b0-be42-746c2b1409d4"
/>


<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist

- [ ] Closes: microsoft#42260
- [ ] **Communication:** @jiripolasek 
- [ ] **Tests:** Added/updated and all pass
- [ ] **Localization:** All end-user-facing strings can be localized
- [ ] **Dev docs:** Added/updated
- [ ] **New binaries:** Added on the required places
- [ ] [JSON for
signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json)
for new binaries
- [ ] [WXS for
installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs)
for new binaries and localization folder
- [ ] [YML for CI
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml)
for new test projects
- [ ] [YML for signed
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml)
- [ ] **Documentation updated:** If checked, please file a pull request
on [our docs
repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys)
and link it here: #xxx

<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments


Actully, The `ThumbnailHelper` already contains code that converts an
`IntPtr` `hIcon` into an `IRandomAccessStream`, as shown below:

```
 private static MemoryStream GetMemoryStreamFromIcon(IntPtr hIcon)
 {
     var memoryStream = new MemoryStream();

     // Ensure disposing the icon before freeing the handle
     using (var icon = Icon.FromHandle(hIcon))
     {
         icon.ToBitmap().Save(memoryStream, System.Drawing.Imaging.ImageFormat.Png);
     }

     // Clean up the unmanaged handle without risking a use-after-free.
     NativeMethods.DestroyIcon(hIcon);

     memoryStream.Position = 0;
     return memoryStream;
 }

 private static async Task<IRandomAccessStream?> FromHIconToStream(IntPtr hIcon)
 {
     var stream = new InMemoryRandomAccessStream();

     using var memoryStream = GetMemoryStreamFromIcon(hIcon); // this will DestroyIcon hIcon
     using var outputStream = stream.GetOutputStreamAt(0);
     using var dataWriter = new DataWriter(outputStream);

     dataWriter.WriteBytes(memoryStream.ToArray());
     await dataWriter.StoreAsync();
     await dataWriter.FlushAsync();

     return stream;
 }
```

Without modifying (or using) this code, I implemented the almost same
logic directly in `SwitchToWindowCommand` (calling the async code with
`Wait` to block synchronously). The reasons are:

1. I wanted to limit changes to the **WindowWalker** project area. I
don’t expect other extensions to need this behavior.
2. Because this is resource-related work, exposing a public helper that
pulls memory from an `hIcon` pointer seems risky—especially in a class
like `ThumbnailHelper`.

Therefore, I implemented behavior that is nearly identical to the
snippet above.

I did use `using`/`Dispose` where appropriate, but the
`InMemoryRandomAccessStream` created for `IconInfo.FromStream` appears
to use internal referencing; disposing it would be incorrect. For that
reason I didn’t wrap it in a `using`. I’m not entirely sure whether GC
will handle this cleanly.

However, based on the implementation of `FromStream` itself and its
usage elsewhere (e.g., in `ThumbnailHelper`), this seems to be the
correct usage pattern, though I’m not entirely sure.

<!-- Describe how you validated the behavior. Add automated tests
wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

---------

Co-authored-by: Jiří Polášek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Good to merge after release Product-Command Palette Refers to the Command Palette utility

Projects

None yet

6 participants