-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
win: implement battery (Draft) #4271
base: v1.x
Are you sure you want to change the base?
Conversation
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
The results in my VM (no battery) are the following:
I also ran the tests on a Windows laptop, which resulted in:
Which is essentialy the same data I get on the browser API. |
Initial implementation of uv battery api for Win32
#include "winapi.h" | ||
|
||
int uv__estimate_remaining_charging_time_in_seconds(SYSTEM_POWER_STATUS* sys_power_status) { | ||
int remaining; |
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.
Style nits:
- 2 space indent, not 4
- lines should be <= 80 columns
This function is only used in this file so it should be static
. That sys_power_status
argument is used a lot so I'd give it a short name like sps
or simply just p
.
if (sys_power_status->ACLineStatus != 1) { | ||
remaining = -1; | ||
goto out; | ||
} |
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.
No need to goto here, you can just return early, likewise below:
if (sys_power_status->ACLineStatus != 1)
return -1;
(for some reason I can't post it as a diff)
int err; | ||
|
||
if (!GetSystemPowerStatus(&sys_power_status)) { | ||
err = /* Not sure... */1; |
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.
Probably should call GetLastError()
and convert with uv_translate_sys_error()
?
I will update this PR soon |
@oluan : do you intend to add implementations for other OS'es? |
Are the maintainers likely to accept this new API? |
// (battery and/or UPS's) are empty | ||
// Convert minutes into seconds | ||
if (info->charging > 0) info->charging *= 60; | ||
|
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.
this function only populates the charging
field (which doesn't exist, see below) of the info
structure, the other fields should be given meaningful values
// Anything else the estimated minutes remaining until all power sources | ||
// (battery and/or UPS's) are empty | ||
// Convert minutes into seconds | ||
if (info->charging > 0) info->charging *= 60; |
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.
there is no field named charging
, did you intend this to be the discharge_time_in_secs
field?
Looks like I missed the corresponding #4258 for Linux/macos so my comments here do not appear to be relevant |
This is my initial implementation (WIP) of the battery API on Windows.
I left some comments in the code.