-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PTRun][WindowWalker Plugin] Display the application icon for a runni… (#7770) #28543
base: main
Are you sure you want to change the base?
Conversation
…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
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/WindowProcess.cs
Show resolved
Hide resolved
IcoPath = WindowWalkerSettings.Instance.UseWindowIconInResults ? string.Empty : icon, | ||
Icon = WindowWalkerSettings.Instance.UseWindowIconInResults ? () => | ||
{ | ||
return x.Result.WindowIcon ?? x.Result.Process.ProcessIcon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need two caches / icon sources? A window should have only one icon?
(I am worry about slow down PT Run search.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
Where can I find the logs that were mentioned in #7770 ? Edit: I made a test application which launches 100 windows just to test |
@marpe |
Sorry, missed it, will look into it |
I gathered the query cost numbers from 10 runs (in debug mode) each with over 50 windows opened:
|
@marpe |
I think the load on my machine might have been fluctuating a bit (I was running just about every application I have :P) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 And as far as I know we cache the window results. But it is to long ago to be sure. 😅 |
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? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
{ | ||
if (_processIdsToIconsCache.Count > 7000) | ||
{ | ||
Debug.Print("Clearing Process Icon Cache because it's size is " + _processIdsToIconsCache.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "it's" with "its" for correct grammar in the debug message.
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 is powered by AI, so mistakes are possible. Review output carefully before use.
{ | ||
if (_windowHandlesToIconsCache.Count > 7000) | ||
{ | ||
Debug.Print("Clearing Window Icon Cache because it's size is " + _windowHandlesToIconsCache.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the grammar in the debug message by replacing "it's" with "its".
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); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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 usingIcon.ExtractAssociatedIcon
and if that fails usesSystemIcons.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 forQueryFullProcessImageName
is capped at 1000 characters. Might want to retry the query with an increased buffer size if it fails instead.