Adding PowerNotifcation APIs #1035
Conversation
ghost
added
the
|
|
||
| namespace ProjectReunionPowerTests | ||
| { | ||
| constexpr auto TIMEOUT = 2000; |
There was a problem hiding this comment.
Why 2000?
What's the expected average and worst case timeouts you'll encounter?
There was a problem hiding this comment.
TBH I just choose a value where I was sure that it will succeed. Informal testing shows they take <500 ms and the Power team also didn't have data on this. Do you have any suggestions?
There was a problem hiding this comment.
Or, change this to a std::chrono::duration. Makes it more explicit with what you mean.
There was a problem hiding this comment.
The variable name has been changed. I didn't use chrono as it would need another level of conversion and the variable name makes it clear.
Will take suggestions on a possible better time out value though. :)
Changed check_hresult to Throw_if_failed Moved/removed some variables Added background thread event firing More NIT changes
|
I wanted to push after the renaming to AppSDK but wasn't sure how long that would take and wanted this to be updated from my side. |
Other NIT changes
| </PropertyGroup> | ||
| <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'"> | ||
| <ClCompile> | ||
| <WarningLevel>Level3</WarningLevel> |
DrusTheAxe
left a comment
DrusTheAxe
left a comment
There was a problem hiding this comment.
Please flip warnings to level4 (in this PR or a follow up) and correct anything it flags
|
The actual PR has only Level4. Did I miss something?
#1142
Thanks,
H.
From: Howard Kapustein ***@***.***>
Sent: Wednesday, August 11, 2021 10:33 PM
To: microsoft/WindowsAppSDK ***@***.***>
Cc: Huzaifa Danish ***@***.***>; Author ***@***.***>
Subject: Re: [microsoft/WindowsAppSDK] Adding PowerNotifcation APIs (#1035)
@DrusTheAxe approved this pull request.
Please flip warnings to level4 (in this PR or a follow up) and correct anything it flags
|
|
My comment predates the Level4 change (or maybe I was looking at an older commit in Codeflow when I made that comment?) But all good now :-) |
DrusTheAxe
added
area-Lifecycle
DrusTheAxe
left a comment
DrusTheAxe
left a comment
There was a problem hiding this comment.
Some minor comments but otherwise looks good
| { | ||
| wil::unique_handle event(CreateEvent(nullptr, false, false, nullptr)); | ||
| THROW_LAST_ERROR_IF_NULL(event.get()); | ||
| auto value = EnergySaverStatus::Disabled; |
There was a problem hiding this comment.
| <packages> | ||
| <package id="Microsoft.Taef" version="10.58.210222006-develop" targetFramework="native" /> | ||
| <package id="Microsoft.Windows.CppWinRT" version="2.0.210707.1" targetFramework="native" /> | ||
| <package id="Microsoft.Windows.ImplementationLibrary" version="1.0.210204.1" targetFramework="native" /> |
There was a problem hiding this comment.
WIL reference is rather long in the tooth
I noticed lots of projects referenced WIL but varying versions (4 different ones!). I just updated them and CsWinRT references to latest (which WindowsAppSDK_DLL was using). Take a look at latest commits in #1208
BTW don't update this through VS' UI, it won't update all the references in *.vcxproj. Global search/replace is more effective
|
Is this PR current or obsolete? |
|
It is old. Apologies for the confusion. Closing. |
Adding the PowerNotification APIs as mentioned in this spec