[CmdPal] WindowWalker Show the actual window icon instead of the process icon#42316
Conversation
This comment has been minimized.
This comment has been minimized.
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowWalker/Helpers/NativeMethods.cs
Outdated
Show resolved
Hide resolved
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowWalker/Helpers/NativeMethods.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowWalker/Components/Window.cs
Fixed
Show fixed
Hide fixed
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowWalker/Helpers/NativeMethods.cs
Fixed
Show fixed
Hide fixed
| return true; | ||
| } | ||
|
|
||
| iconHandle = NativeMethods.GetClassLongPtr(hwnd, Win32Constants.GCLP_HICONSM); |
Check failure
Code scanning / check-spelling
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
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Jiří Polášek <[email protected]>
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.Unrecognized words (4)dfx These words are not needed and should be removedDFX WorktreeTo 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 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 positivesIf items relate to a ...
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@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 |
|
Regardless of that, I really appreciate your interest — from the issue itself all the way to the code review! |
|
WOW what an amazing coincidence! |
There was a problem hiding this comment.
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
UseWindowIconboolean setting with localized strings - Implemented
TryGetWindowIcon()method to retrieve window icons via Win32 APIs - Updated
SwitchToWindowCommandto 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
| icon = System.Drawing.Icon.FromHandle((IntPtr)result); | ||
| NativeMethods.DestroyIcon((IntPtr)result); |
There was a problem hiding this comment.
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.
| icon = System.Drawing.Icon.FromHandle((IntPtr)result); | ||
| NativeMethods.DestroyIcon((IntPtr)result); |
There was a problem hiding this comment.
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.
| icon = System.Drawing.Icon.FromHandle((IntPtr)result); | ||
| NativeMethods.DestroyIcon((IntPtr)result); |
There was a problem hiding this comment.
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.
| icon = System.Drawing.Icon.FromHandle(iconHandle); | ||
| NativeMethods.DestroyIcon((IntPtr)iconHandle); |
There was a problem hiding this comment.
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.
| icon = System.Drawing.Icon.FromHandle(iconHandle); | ||
| NativeMethods.DestroyIcon((IntPtr)iconHandle); |
There was a problem hiding this comment.
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.
| dataWriter.StoreAsync().AsTask().Wait(); | ||
| dataWriter.FlushAsync().AsTask().Wait(); |
There was a problem hiding this comment.
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.
| dataWriter.StoreAsync().AsTask().Wait(); | |
| dataWriter.FlushAsync().AsTask().Wait(); | |
| dataWriter.StoreAsync().AsTask().GetAwaiter().GetResult(); | |
| dataWriter.FlushAsync().AsTask().GetAwaiter().GetResult(); |
…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]>

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.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Actully, The
ThumbnailHelperalready contains code that converts anIntPtrhIconinto anIRandomAccessStream, as shown below:Without modifying (or using) this code, I implemented the almost same logic directly in
SwitchToWindowCommand(calling the async code withWaitto block synchronously). The reasons are:hIconpointer seems risky—especially in a class likeThumbnailHelper.Therefore, I implemented behavior that is nearly identical to the snippet above.
I did use
using/Disposewhere appropriate, but theInMemoryRandomAccessStreamcreated forIconInfo.FromStreamappears to use internal referencing; disposing it would be incorrect. For that reason I didn’t wrap it in ausing. I’m not entirely sure whether GC will handle this cleanly.However, based on the implementation of
FromStreamitself and its usage elsewhere (e.g., inThumbnailHelper), this seems to be the correct usage pattern, though I’m not entirely sure.Validation Steps Performed