Add get_power_state()

[yunline]

Fixes: #1570

Test Code:

import pygame

power = pygame.system.get_power_state()
if power is not None:
    print(f"battery_percent: {power.battery_percent}%")
    print(f"battery_seconds: {power.battery_seconds}")
    print(f"on_battery: {power.on_battery}")
    print(f"no_battery: {power.no_battery}")
    print(f"charging: {power.charging}")
    print(f"charged: {power.charged}")
    print(f"plugged_in: {power.plugged_in}")
    print(f"has_battery: {power.has_battery}")

else:
    print("Power state is unknown.")

pygame-ce 2.4.0.dev1 (SDL 2.26.5, Python 3.10.6)
battery_percent: 100%
battery_seconds: None
on_battery: False    
no_battery: False    
charging: False      
charged: True        
plugged_in: True     
has_battery: True 

[novialriptide]

Why not have these as properties? I know that you get these all these values via SDL_GetPowerInfo(), but I have a feeling that it would make sense to have these as individual properties in the pygame API.

[Starbuck5]

Reading this over, my main concern is that there are magic values (-1) to indicate unknown for some fields, but if the whole thing is unknown all the fields are None.

I see two other possibilities:

• Return None for seconds and percent instead of -1 when the power state is valid but they cannot be determined.
• Return None instead of a dictionary on powerstate unknown.

What do you think?

Another thing:
I like how you gave examples in the documentation, good call there. I think it would be valuable to also port some of the cautionary language in the SDL docs about battery stuff: https://wiki.libsdl.org/SDL2/SDL_GetPowerInfo (for example-- the values are estimates by the hardware, some systems give percent or time remaining but not both.)

[Starbuck5]

Just realized this should have a basic test in the test suite.

[Starbuck5]

The implementation looks good, good work.

I'd still like a little further discussion about the API. Maybe @ankith26 or @zoldalma999 has a take?

[ankith26]

Yes I do have a take on a few alternative API ideas, one of which I mentioned in #1570
As previously mentioned (a while back) my issue with this API is if I want to a specific check, I have to check 2-3 variables (say if I want to display a "green battery icon" in a game only when the battery is fully charged, I have to do something like battery["has_battery"] and battery["plugged_in"] and not battery["charging"] with this API instead of 1 with the SDL API)

However, I'm not strongly against this PR, I would also be happy if we went with this, but just added an extra field to the dict that is the "raw SDL data as an enum" (so in my above example, I would like to instead do battery["state"] == pygame.PowerState.CHARGED). The rest of the fields would basically be convenience attributes that could be useful based on what the user is doing.

[zoldalma999]

Since I was asked about this

I think using None as absent value for integer values (battery_seconds and battery_percent) is a bit weird. Wouldn't -1 be better? Or maybe don't even set unknown values? And then we can return an empty dict instead of None as well.

I do agree with Ankith, exposing the sdl constant as an extra field could be helpful to some people. However if we want to go with the enum way, maybe a general api for adding and handling enums would be nice (assuming we want to continue doing this in other places as well?). Or do we just hack it in through a python file or something like that?

However, that is out of the scope of this pr. So I am up to just merge this without the const, and do it in a later pr

[yunline]

Wouldn't -1 be better? Or maybe don't even set unknown values? And then we can return an empty dict instead of None as well.

None is more pythonic than -1.
I think setting the unknown values to None is better then removing keys. This makes the length of dict consistent.

[yunline]

@ankith26 Adding an enumerate class may be too complicated for this PR.

Is it OK to add some constants instead? Like this:

battery["raw_state"] == pygame.POWERSTATE_CHARGED

Or maybe open another PR to handle the enum things (?

[Starbuck5]

One potential problem with adding the powerstate as a enum/flag/constant like this is that it undermines the whole "return None if state is unknown thing"

Either the SDL state should be decomposed into its data components, or just the SDL state should be reported?

It's a shame to get this far through implementation and still not have a decided API. Ankith wants the SDL state, I want the separated data, yet neither of us are so dead set in our approach. I just want to decide the API.

I think it's either the current API or it returns a dictionary of {state, battery_percent, time} (AKA a direct port of SDL_GetPowerInfo)

[MyreMylar]

It sounds like everyone would be nearly happy if we added a 'state' enum corresponding to the SDL_POWERSTATE values (without SDL_POWERSTATE_UNKNOWN -
which is already covered by the function returning None) to the returned dictionary.

Looks like adding an enum C side is a little complicated but I did find a useful Stackoverflow answer on the topic here:
https://stackoverflow.com/a/69281896

So you'd have a user API something like:

power_info = pygame.system.get_power_state()

if power_info["state"] == pygame.system.PowerState.CHARGED:
    # display green light

It might be nice if the returned power_info was in the style of a dataclass instead of a dictionary, and if we are having a state field we should probably make the overall name of the function and returned data match the SDL name. So we might end up with an API like:

power_info = pygame.system.get_power_info()

if power_info.state == pygame.system.PowerState.CHARGED:
   # display green light
elif power_info.charging:
   print("Amount charged:", power_info.battery_percent)

Though now that I've typed that I kind of hate it as it feels like we would have two different APIs to get battery status of "charging" versus "charged".

Instead of a state enum, why don't we just add two other boolean fields charged and 'on_battery' to the 'dataclass'/dictionary? That seems like it would cover the two remaining most common questions people will have about batteries & power. I don't think people will care specifically about obtaining the case of a machine with no battery, that also isn't plugged in - because that is impossible.

Anyway then you'd end up with:

power_state = pygame.system.get_power_state()

if power_state.on_battery:
   #activate power saving mode
if power_state.charged:
   # display green light
elif power_state.charging:
  # display charging info
   print("Amount charged:", power_info.battery_percent)

I assume you can create a dataclass-like instance of a class via normal class creating even if it doesn't have all the same features, as it won't really matter for the end user. It'll just reduce the typing of the dictionary-esque padding e.g. power_info["stuff"] versus power_info.stuff. Data classes seem to be the new, post 3.7, pythonic style:

https://stackoverflow.com/questions/4163018/create-an-object-using-pythons-c-api

[dr0id]

Strings in an interface are prone to typos (should be found by testing thought).

The other thing I recommend is to wrap what you don't own, e.g. don't pass the SDL enum directly, wrap it instead.

The DataClass example encapsulates the data better and could make clear, that those values are read only (only setter properties?).

[yunline]

class _PowerState:
    battery_seconds: int | None
    battery_percent: int | None

    # Attributes from SDL flags
    on_battery: bool # Not plugged in, running on the battery
    no_battery: bool # Plugged in, no battery available
    charging: bool # Plugged in, charging battery
    charged: bool # Plugged in, battery charged

    # Additional attributes for convenience
    plugged_in = not on_battery
    has_battery = on_battery or not no_battery


def get_power_state() -> _PowerState | None:
    ...

Does this satisfy all the requirements?

[oddbookworm]

LGTM! I just have one minor nitpick that can be ignored if it's a bad suggestion

[Starbuck5]

Looks great, thanks @yunline!

I'd like you to squash the commits down a bit before merging. Once you do that, go ahead and merge. :)