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

Awake Updates - PROMETHEAN_09082024 #34717

Merged
merged 18 commits into from
Sep 26, 2024
Merged

Awake Updates - PROMETHEAN_09082024 #34717

merged 18 commits into from
Sep 26, 2024

Conversation

dend
Copy link
Collaborator

@dend dend commented Sep 8, 2024

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Sep 9, 2024
@gokcekantarci
Copy link
Contributor

Hi I tested these changes with installer and debugger. I tested at least 10 times with each one. Icon did not appear 2 times. These are logs.

[18:08:18.8488434] [Info] TrayHelper::SetShellIcon Could not set the shell icon. Action: Update and error code: -2147467259. HIcon handle is 765595613 and HWnd is 3543010 [18:08:18.9020742] [Error] Program::AwakeUnhandledExceptionCatcher System.ComponentModel.Win32Exception (0x80004005): Failed to change tray icon. Action: Update and error code: -2147467259 at Awake.Core.TrayHelper.SetShellIcon(IntPtr hWnd, String text, Icon icon, TrayIconAction action) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Core\TrayHelper.cs:line 195 at Awake.Core.Manager.SetPassiveKeepAwake(Boolean updateSettings) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Core\Manager.cs:line 389 at Awake.Core.Manager.CompleteExit(Int32 exitCode) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Core\Manager.cs:line 317 at Awake.Program.Exit(String message, Int32 exitCode) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Program.cs:line 166 at Awake.Program.<>c__DisplayClass20_0.<HandleCommandLineArguments>b__0() in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Program.cs:line 198 [18:08:18.9023480] [Error] Program::AwakeUnhandledExceptionCatcher at Awake.Core.TrayHelper.SetShellIcon(IntPtr hWnd, String text, Icon icon, TrayIconAction action) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Core\TrayHelper.cs:line 195 at Awake.Core.Manager.SetPassiveKeepAwake(Boolean updateSettings) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Core\Manager.cs:line 389 at Awake.Core.Manager.CompleteExit(Int32 exitCode) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Core\Manager.cs:line 317 at Awake.Program.Exit(String message, Int32 exitCode) in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Program.cs:line 166 at Awake.Program.<>c__DisplayClass20_0.<HandleCommandLineArguments>b__0() in C:\Users\gokce\source\repos\PowerToys\src\modules\awake\Awake\Program.cs:line 198

Since it didn't happen every time, I couldn't find the root cause of it with testing. However, when I add the following line of code to SetShellIcon function I get the same error every time. To prevent this, default values ​​can be written to _notifyIconData at startup.

Icon

Also, when the icon is visible, it is not clickable in most of my attempts.

Lastly, when the icon is visible, when I change Awake mode from settings (even if it is not clickable) icon changes. That looks good to me.

@dend
Copy link
Collaborator Author

dend commented Sep 25, 2024

@gokcekantarci did you pull the latest changes from this PR specifically and compiled the build? You can even go as far as clone my fork and check there. I've seen folks previously state that they tested the PR when they were testing the main branch. Can you share the local commit ID that you have that you're testing against?

I cannot reproduce the issue you're referring to in my build. Are you saying that when you're setting the _notifyIconData to default the error pops up? I would expect that to be the case since that's not what the API expects.

Also - did the icon not show at startup, when you changed the settings, or due some other behavior? Did you change it through the Settings app? The log that you are showing is telling me that the error was triggered on exit - can you share the full logs here, please?

Re: clickable icon, are you launching the project from VS with PowerToys running, or separately through the command line/app executable?

@gokcekantarci
Copy link
Contributor

Hi @dend,

@gokcekantarci did you pull the latest changes from this PR specifically and compiled the build? You can even go as far as clone my fork and check there. I've seen folks previously state that they tested the PR when they were testing the main branch. Can you share the local commit ID that you have that you're testing against?

I first cloned your forked repo and tested with main branch in there. Latest commit id is 83e0be7.

I cannot reproduce the issue you're referring to in my build. Are you saying that when you're setting the _notifyIconData to default the error pops up? I would expect that to be the case since that's not what the API expects.

I agree with you. I think it is appropriate for writing this error log and not showing icon when encounters this kind of error. But I saw this error twice after trying it maybe 30 times. I think a suitable default behavior can be added to this. For example, if such an problem occurs, Awake can be run in passive mode and the icon can be also set in this way.

Also - did the icon not show at startup, when you changed the settings, or due some other behavior? Did you change it through the Settings app? The log that you are showing is telling me that the error was triggered on exit - can you share the full logs here, please?

I don't know exactly how the issue occurs. When it happened for the first time, I examined the logs and saw the same thing as I shared above. Then I tried different things to reproduce the error. These are like restarting the PC, restarting Powertoys, deleting %localappdata%\Microsoft\PowerToys, reinstalling Powertoys, disable/enable from settings,. I deleted the logs without saving them to a separate location, thinking that issue might be reproduced. I'm sorry about that. Then I tried to reproduce it with the code. When I added _notifyIconData = default; line and run with exe and also in VS, icon is missing and logs are same.

Re: clickable icon, are you launching the project from VS with PowerToys running, or separately through the command line/app executable?

I tested with debugger in VS and also I tested with exe after building code and installer.

@dend
Copy link
Collaborator Author

dend commented Sep 26, 2024

Screenshot of the context menu being shown in the tray for PowerToys Awake

@gokcekantarci I just tested this on Windows 10 and the context menu correctly appears, launched directly from the debugger.

@dend
Copy link
Collaborator Author

dend commented Sep 26, 2024

But I also got it into a state where it does not show the menu, so something is definitely off @gokcekantarci. Investigating.

@dend
Copy link
Collaborator Author

dend commented Sep 26, 2024

@gokcekantarci did some more digging and I found the issue - it's a problem with the synchronization context I was using when trying to create the shell icon. The GetMessage function in some cases can be invoked on another thread, resulting in no messages being received, meaning that you won't be able to get the reaction to a click done. It's being addressed in a short commit here in a few minutes.

Copy link
Contributor

@gokcekantarci gokcekantarci left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the fix.

@jaimecbernardo
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Rubberstamp LGTM! The most in depth review was done by Gokce, but I did some quick tests as well. Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit 49a8282 into microsoft:main Sep 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In for .85 Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants