-
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
[PowerRun] Fix sleep function #28370
base: main
Are you sure you want to change the base?
Conversation
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.
Hi, Thanks for opening the PR.
What are we actually doing here? This is lots of magic numbers. The code should have extensive comments to explain what's going on and with links to documentation if possible.
Everyone is going to look at this and think "What? Why?"
…y constant values
This comment has been minimized.
This comment has been minimized.
…riggering the Modern Standby mode
Some more background to help reviewers understand the context. PowerRun triggers sleep via SetSuspendState(bHibernate=false), whose behavior highly differentiates between system configurations:
To fix the gap (for modern laptops having S0-idle enabled), the PR author suggests to use a "screen off" message as a replacement, which seems to work. Here are a list of reported issues: |
@@ -91,7 +96,11 @@ internal static List<Result> GetSystemCommands(bool isUefi, bool splitRecycleBin | |||
IcoPath = $"Images\\sleep.{iconTheme}.png", | |||
Action = c => | |||
{ | |||
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => NativeMethods.SetSuspendState(false, true, true)); |
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.
For laptops that are still using S1-S3 instead of S0-idle, does the new "monitor power off" message put the device into the correct sleep state? Do we need to branch the actions into 2 cases? For example:
NativeMethods.GetPwrCapabilities(out SystemPowerCapabilities powerCapabilities);
if (powerCapabilities.AoAc) // modern standby is enabled
{
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => Task.Run(() => _ = NativeMethods.SendMessage(HWNDBROADCAST, WMSYSCOMMAND, SCMONITORPOWER, POWEROFF)));
}
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => NativeMethods.SetSuspendState(false, true, true));
NativeMethods.cs for GetPwrCapabilities
[DllImport("Powrprof.dll", SetLastError = true)]
public static extern bool GetPwrCapabilities(out SystemPowerCapabilities lpSystemPowerCapabilities);
ref:
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.
Hi @der3318,
Thank you for your comment!
Indeed, I could not test the changes on a system with traditional S1-3 power modes. It is also not clear, where the boundaries are (until which Windows version for example) and whether the change makes the sleep function worse on those systems or not.
I will investigate deeper this topic. Your code suggestion seems to be logical, thank you very much!!
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.
Hi @der3318
I have tested the code you proposed. Albight it seems logical, it did not work for me, the GetPwrCapabilities function did return false response, AoAc was false on my system, which is faulty. I've experimented with it, but it was always the same.
My conclusion in the current situation is that in this form I cannot extend the code.
I will keep analysing the problem. Just letting you know.
Thanks for the contribution!
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 effort. Sadly I am not aware of any issue regarding API GetPwrCapabilities and the field AoAc, which has been publicly used since at least XP.
Just to confirm 😊 - is it possible that you are not marshaling the struct SYSTEM_POWER_CAPABILITIES correctly? As I just noticed that MSFT's documentation regarding this struct is inaccurate. If you take a look at the actual winnt.h (e.g., the one under C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um), it should be:
typedef struct {
...
BOOLEAN ProcessorThrottle;
BYTE ProcessorMinThrottle;
+ #if (NTDDI_VERSION < NTDDI_WINXP)
BYTE ProcessorThrottleScale;
BYTE spare2[4];
+ #else
BYTE ProcessorMaxThrottle;
BOOLEAN FastSystemS4;
BOOLEAN Hiberboot;
BOOLEAN WakeAlarmPresent;
BOOLEAN AoAc;
+ #endif
BOOLEAN DiskSpinDown;
...
} SYSTEM_POWER_CAPABILITIES, *PSYSTEM_POWER_CAPABILITIES;
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.
Hi @der3318,
Yes indeed, there was a glitch in the marshaling. I've rewritten it and it works. I thank you for your colaboration!!
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.
Gave it a test, but it doesn't seem to be working well for me.
When I choose "Sleep" from the start menu, it goes to sleep and then wakes up after I press a keystroke.
When I choose "Sleep" from PowerToys, it goes to sleep but everytime I press a key it seems to try to wake up but then go right to sleep again right away. To wake it up, I have to keep mashing my keyboard to avoid sending it to sleep again. Doesn't seem like this is good behavior.
@AkazaRenn commented #10557 (comment)
MIT license project too |
@Jay-o-Way , the solution here when I tested it put my PC on a sleep loop. It's still a WIP. |
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 2 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (1)
- .github/actions/spell-check/expect.txt: Language not supported
Comments suppressed due to low confidence (1)
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/Commands.cs:107
- The new behavior introduced by SendMessage to initiate Modern Standby mode is not covered by tests. Consider adding tests to validate this behavior.
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => Task.Run(() => _ = NativeMethods.SendMessage(HWNDBROADCAST, WMSYSCOMMAND, SCMONITORPOWER, POWEROFF)));
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 2 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (1)
- .github/actions/spell-check/expect.txt: Language not supported
Comments suppressed due to low confidence (3)
src/modules/launcher/Plugins/Microsoft.PowerToys.Run.Plugin.System/Components/Commands.cs:107
- Ensure that the SendMessage method is covered by tests to validate the new behavior.
return ResultHelper.ExecuteCommand(confirmCommands, Resources.Microsoft_plugin_sys_sleep_confirmation, () => Task.Run(() => _ = NativeMethods.SendMessage(HWNDBROADCAST, WMSYSCOMMAND, SCMONITORPOWER, POWEROFF)));
src/modules/launcher/Wox.Plugin/Common/Win32/NativeMethods.cs:115
- The parameters for SendMessage should be IntPtr or uint to match the Win32 API signature more closely.
public static extern int SendMessage(int hWnd, int hMsg, int wParam, int lParam);
src/modules/launcher/Wox.Plugin/Common/Win32/NativeMethods.cs:700
- [nitpick] The field 'spare3' in SystemPowerCapabilities struct is private and unused. Consider clarifying its purpose or removing it if not needed.
private readonly byte[] spare3;
Summary of the Pull Request
[PowerRun] Update sleep function: Send message to put PC in Modern Standby mode
PR Checklist
Detailed Description of the Pull Request / Additional comments
Sending message to the system which switches off the monitors, which again initiates the Modern standby mode.
Sending it in a background task to be able to close the PowerRun UI.
Validation Steps Performed
Tested locally