[PTRun][WindowWalker Plugin] Display the application icon for a runni… (#7770) #28543

marpe · 2023-09-13T15:31:18Z

Display the application icon for a running process in the search results ([Run][WindowWalker Plugin] Display the application icon for a running process #7770)
Add a setting to opt-out of showing icon
Adapt tests

Summary of the Pull Request

Attempts to retrieve and show the window icon (through SendMessage), if it fails attempts to retrieve an icon by querying the process file name and using Icon.ExtractAssociatedIcon and if that fails uses SystemIcons.Application. The icons are cached in static dictionaries.

Added a setting to opt-out of showing the icons (reverting to previous default icon) and updated the test for the settings.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manually tested the functions for retrieving the icons (both window and process). In practice haven't observed a situation where all of the first SendMessage(WM_ICON, ...) attempts have failed.

One thing that bugs me is that if attempts to get the window icon fails and the process file path has a length greater than 1000, it will always show the SystemIcons.Application fallback because the buffer used for QueryFullProcessImageName is capped at 1000 characters. Might want to retry the query with an increased buffer size if it fails instead.

…ng process * Display the application icon for a running process in the search results (microsoft#7770) * Add a setting to opt-out of showing icon * Adapt tests

marpe · 2023-09-13T15:32:10Z

@microsoft-github-policy-service agree

htcfreek

I have added some thoughts, questions and findings.

Does it make sense to add a description in the settings ui, that showing the real icons may slow down the search?

And please add code to clear the icon caches after the reached a count of x. We should prevent caching outdated data and spamming physical memary. You can look at the process cache and hiw it's done there.

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/WindowProcess.cs

htcfreek · 2023-09-13T16:28:03Z

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/ResultHelper.cs

+                    IcoPath = WindowWalkerSettings.Instance.UseWindowIconInResults ? string.Empty : icon,
+                    Icon = WindowWalkerSettings.Instance.UseWindowIconInResults ? () =>
+                    {
+                        return x.Result.WindowIcon ?? x.Result.Process.ProcessIcon;


Why do we need two caches / icon sources? A window should have only one icon?

(I am worry about slow down PT Run search.)

Well different windows under the same process can have different icons. We could just set IcoPath to the process file path instead of storing a cache in WindowWalker if the WindowIcon couldn't be retrieved for whatever reason. I find it a bit strange that the docs for WM_GETICON says:

If sending a WM_GETICON message to a window returns 0, next try calling the GetClassLongPtr function for the window.

even though ICON_SMALL2 seems to be the icon that would act as a fallback given it's description:

Retrieves the small icon provided by the application. If the application does not provide one, the system uses the system-generated icon for that window.

So we have one icon from window and a (fallback) icon for the process. Then the two caches make sense.

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/WindowWalkerSettings.cs

htcfreek · 2023-09-13T16:49:08Z

@marpe
Please do a performance test with 50+ open windows from many different apps. (The last time I implemented bigger changes I was asked to do this by @crutkas .)

…etProcessFilePathFromID

marpe · 2023-09-15T15:14:48Z

@marpe Please do a performance test with 50+ open windows from many different apps. (The last time I implemented bigger changes I was asked to do this by @crutkas .)

Where can I find the logs that were mentioned in #7770 ?

Edit: I made a test application which launches 100 windows just to test WM_GETICON with SendMessage and it takes < 1ms to fetch the icon handles for all 100.

htcfreek · 2023-09-15T18:30:24Z

Where can I find the logs that were mentioned in #7770 ?

@marpe
You can find the log file in %localappdata%\microsoft\PowerToys\PowerToys Run\Logs. In the file you can search for query cost.

htcfreek · 2023-09-15T18:33:53Z

Does it make sense to add a description in the settings ui, that showing the real icons may slow down the search?

And please add code to clear the icon caches after the reached a count of x. We should prevent caching outdated data and spamming physical memary. You can look at the process cac

@marpe
Did you read this?

marpe · 2023-09-15T18:57:38Z

Does it make sense to add a description in the settings ui, that showing the real icons may slow down the search?

And please add code to clear the icon caches after the reached a count of x. We should prevent caching outdated data and spamming physical memary. You can look at the process cac

@marpe
Did you read this?

Sorry, missed it, will look into it

…out the potential slow down

marpe · 2023-09-16T14:18:21Z

I gathered the query cost numbers from 10 runs (in debug mode) each with over 50 windows opened:

With Icons (ms)	Without Icons (ms)
643	861
597	672
683	686
758	674
644	677
626	709
665	663
683	701
698	770
636	646
Avg: 663.3	Avg: 705.9

htcfreek · 2023-09-16T15:37:17Z

@marpe
interesting. looks like you improved something.

marpe · 2023-09-16T17:55:25Z

I think the load on my machine might have been fluctuating a bit (I was running just about every application I have :P)

jaimecbernardo

Thank you for the contribution!

The issue I see here is that we're using Process IDs and handles as keys for the cache. These get reused and can be reused as soon as they are freed, meaning these might be shown incorrectly. Can we make it so that an entry in the cache would remain valid only if it was inserted in the last 60 seconds? (Thinking this would make the cache useful when we're doing the same search intearaction to speed it up but would try to get icons for the next search)
What do you think?

htcfreek · 2023-09-20T10:13:23Z

Thank you for the contribution!

The issue I see here is that we're using Process IDs and handles as keys for the cache. These get reused and can be reused as soon as they are freed, meaning these might be shown incorrectly. Can we make it so that an entry in the cache would remain valid only if it was inserted in the last 60 seconds? (Thinking this would make the cache useful when we're doing the same search intearaction to speed it up but would try to get icons for the next search)
What do you think?

The Hwnd thing was used in the paste for the window<>process cache too. For the process<>icon cache we could switch to the exe name. But do you have a better idea for replacing hWnd?

And as far as I know we cache the window results. But it is to long ago to be sure. 😅

marpe · 2023-09-22T12:25:37Z

I think it would be a good idea to setup an automated benchmark so that the performance can be compared more accurately across changes, my manual testing didn't really offer much insight. Caching might not have much of an impact even?

jaimecbernardo · 2023-10-04T10:21:20Z

I think it would be a good idea to setup an automated benchmark so that the performance can be compared more accurately across changes, my manual testing didn't really offer much insight. Caching might not have much of an impact even?

It's doing some system calls to get the icon, so we definitely don't want to get it every search (as in each character inputted triggers another search) 🤔 Perhaps better would be to be able to get it async?

Jay-o-Way · 2023-11-13T16:02:35Z

Ping @marpe ?

marpe · 2023-11-18T13:25:04Z

Ping @marpe ?

Sorry I haven't had any more spare time spend on this, and I likely won't for some time. If anyone's willing to pick this up, please do.

Copilot

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

Comments suppressed due to low confidence (2)

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/WindowProcess.cs:137

[nitpick] Consider replacing the hard-coded value 7000 with a named constant to improve clarity and maintainability of the cache size threshold.

if (_processIdsToIconsCache.Count > 7000)

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/Window.cs:125

[nitpick] Consider defining a named constant for the cache size threshold (7000) rather than using a literal, to improve readability and ease future adjustments.

if (_windowHandlesToIconsCache.Count > 7000)

Copilot · 2025-02-19T10:24:05Z

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/WindowProcess.cs

+                    {
+                        if (_processIdsToIconsCache.Count > 7000)
+                        {
+                            Debug.Print("Clearing Process Icon Cache because it's size is " + _processIdsToIconsCache.Count);


Replace "it's" with "its" for correct grammar in the debug message.

Suggested change

Debug.Print("Clearing Process Icon Cache because it's size is " + _processIdsToIconsCache.Count);

Debug.Print("Clearing Process Icon Cache because its size is " + _processIdsToIconsCache.Count);

Copilot · 2025-02-19T10:24:05Z

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/Window.cs

+                    {
+                        if (_windowHandlesToIconsCache.Count > 7000)
+                        {
+                            Debug.Print("Clearing Window Icon Cache because it's size is " + _windowHandlesToIconsCache.Count);


Correct the grammar in the debug message by replacing "it's" with "its".

Suggested change

Debug.Print("Clearing Window Icon Cache because it's size is " + _windowHandlesToIconsCache.Count);

Debug.Print("Clearing Window Icon Cache because its size is " + _windowHandlesToIconsCache.Count);

[PTRun][WindowWalker Plugin] Display the application icon for a runni…

77d6d02

…ng process * Display the application icon for a running process in the search results (microsoft#7770) * Add a setting to opt-out of showing icon * Adapt tests

This comment has been minimized.

Sign in to view

Add HICONSM to spell-check

e5968b1

htcfreek reviewed Sep 13, 2023

View reviewed changes

src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/WindowWalkerSettings.cs Outdated Show resolved Hide resolved

marpe added 2 commits September 15, 2023 16:51

Add missing member summaries and rename GetProcessFileNameFromID to G…

c1f2c78

…etProcessFilePathFromID

Change default value for UseWindowIconInResults from true to false

19232bd

marpe added 2 commits September 16, 2023 15:00

Add a description to the UseWindowIconInResults setting, informing ab…

a16ba56

…out the potential slow down

Clear icon caches if they reach more than 7000 items

10901bb

Reorder members

fef9b28

jaimecbernardo requested changes Sep 20, 2023

View reviewed changes

yeelam-gordon requested review from moooyo and Copilot February 19, 2025 10:22

Copilot AI reviewed Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PTRun][WindowWalker Plugin] Display the application icon for a runni… (#7770) #28543

[PTRun][WindowWalker Plugin] Display the application icon for a runni… (#7770) #28543

marpe commented Sep 13, 2023

marpe commented Sep 13, 2023

This comment has been minimized.

htcfreek left a comment •

edited

Loading

htcfreek Sep 13, 2023

marpe Sep 15, 2023

htcfreek Sep 15, 2023

htcfreek commented Sep 13, 2023

marpe commented Sep 15, 2023 •

edited

Loading

htcfreek commented Sep 15, 2023

htcfreek commented Sep 15, 2023 •

edited

Loading

marpe commented Sep 15, 2023

marpe commented Sep 16, 2023 •

edited

Loading

htcfreek commented Sep 16, 2023

marpe commented Sep 16, 2023

jaimecbernardo left a comment

htcfreek commented Sep 20, 2023 •

edited

Loading

marpe commented Sep 22, 2023

jaimecbernardo commented Oct 4, 2023

Jay-o-Way commented Nov 13, 2023

marpe commented Nov 18, 2023

Copilot AI left a comment

Copilot AI Feb 19, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Feb 19, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

	Debug.Print("Clearing Process Icon Cache because it's size is " + _processIdsToIconsCache.Count);
	Debug.Print("Clearing Process Icon Cache because its size is " + _processIdsToIconsCache.Count);

	Debug.Print("Clearing Window Icon Cache because it's size is " + _windowHandlesToIconsCache.Count);
	Debug.Print("Clearing Window Icon Cache because its size is " + _windowHandlesToIconsCache.Count);

[PTRun][WindowWalker Plugin] Display the application icon for a runni… (#7770) #28543

Are you sure you want to change the base?

[PTRun][WindowWalker Plugin] Display the application icon for a runni… (#7770) #28543

Conversation

marpe commented Sep 13, 2023

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

marpe commented Sep 13, 2023

This comment has been minimized.

htcfreek left a comment • edited Loading

Choose a reason for hiding this comment

htcfreek Sep 13, 2023

Choose a reason for hiding this comment

marpe Sep 15, 2023

Choose a reason for hiding this comment

htcfreek Sep 15, 2023

Choose a reason for hiding this comment

htcfreek commented Sep 13, 2023

marpe commented Sep 15, 2023 • edited Loading

htcfreek commented Sep 15, 2023

htcfreek commented Sep 15, 2023 • edited Loading

marpe commented Sep 15, 2023

marpe commented Sep 16, 2023 • edited Loading

htcfreek commented Sep 16, 2023

marpe commented Sep 16, 2023

jaimecbernardo left a comment

Choose a reason for hiding this comment

htcfreek commented Sep 20, 2023 • edited Loading

marpe commented Sep 22, 2023

jaimecbernardo commented Oct 4, 2023

Jay-o-Way commented Nov 13, 2023

marpe commented Nov 18, 2023

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Feb 19, 2025

Choose a reason for hiding this comment

Copilot AI Feb 19, 2025

Choose a reason for hiding this comment

htcfreek left a comment •

edited

Loading

marpe commented Sep 15, 2023 •

edited

Loading

htcfreek commented Sep 15, 2023 •

edited

Loading

marpe commented Sep 16, 2023 •

edited

Loading

htcfreek commented Sep 20, 2023 •

edited

Loading