Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

marpe
Copy link

@marpe marpe commented Sep 13, 2023

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
Copy link
Author

marpe commented Sep 13, 2023

@microsoft-github-policy-service agree

@github-actions

This comment has been minimized.

Copy link
Collaborator

@htcfreek htcfreek left a 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.

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

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.)

Copy link
Author

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.

Copy link
Collaborator

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.

@htcfreek
Copy link
Collaborator

@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 .)

@marpe
Copy link
Author

marpe commented Sep 15, 2023

@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
Copy link
Collaborator

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
Copy link
Collaborator

htcfreek commented Sep 15, 2023

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
Copy link
Author

marpe commented Sep 15, 2023

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

@marpe
Copy link
Author

marpe commented Sep 16, 2023

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
Copy link
Collaborator

@marpe
interesting. looks like you improved something.

@marpe
Copy link
Author

marpe commented Sep 16, 2023

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

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a 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?

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 20, 2023

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
Copy link
Author

marpe commented Sep 22, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

Ping @marpe ?

@marpe
Copy link
Author

marpe commented Nov 18, 2023

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.

Copy link
Contributor

@Copilot 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.

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);
Copy link
Preview

Copilot AI Feb 19, 2025

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.

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 is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options
{
if (_windowHandlesToIconsCache.Count > 7000)
{
Debug.Print("Clearing Window Icon Cache because it's size is " + _windowHandlesToIconsCache.Count);
Copy link
Preview

Copilot AI Feb 19, 2025

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".

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);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants