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

fix(power): Fix battery state / charge devices info. #710

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

l-const
Copy link
Contributor

@l-const l-const commented Oct 19, 2024

Fixes #561.

Changes:

Before:

screenshot-2024-10-19-20-50-11

After:

screenshot-2024-10-19-20-15-40

Wrongfully using upower.on_battery (https://upower.freedesktop.org/docs/UPower.html) property instead of upower.device.state (https://upower.freedesktop.org/docs/Device.html) . The first one indicates whether it is a PC or laptop not if it is charging. The sibling properties of the object is LidIsClose :

screenshot-2024-10-19-21-01-34
screenshot-2024-10-19-21-01-55

I also found what KDE and lxqt is doing and what is using to derive this info:

  1. Wrong tooltip and left click info in pending-charge state lxqt/lxqt-powermanagement#295 (comment)
  2. https://github.com/KDE/solid/blob/master/src/solid/devices/frontend/battery.h#L201-L208 ( main KDE librarry abstration used for both)
  3. An unrelated issue about the icon and this exact info into the main upower repo: https://gitlab.freedesktop.org/upower/upower/-/merge_requests/20
  4. Actual, implementation : https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/linux/up-device-idevice.c?ref_type=heads#L285-294

Note: LQXT seems to have more complicated logic with the icons used, but nevertheless you can see what they use here to pick the right .svg: https://github.com/lxqt/lxqt-powermanagement/blob/master/src/iconproducer.cpp#L292-L300

    switch (state)
    {
    case Solid::Battery::FullyCharged:
        svg.replace(QL1S("STATE_MARKER"), star);
        break;
    case Solid::Battery::Charging:
        svg.replace(QL1S("STATE_MARKER"), plus);
        break;
    case Solid::Battery::Discharging:
        svg.replace(QL1S("STATE_MARKER"), minus);
        break;
    default:
        svg.replace(QL1S("STATE_MARKER"), QString{});
    }

Sending the dbus message from terminal:

❯ dbus-send --print-reply             --system             --dest=org.freedesktop.UPower             /org/freedesktop/UPower/devices/battery_hidpp_battery_0     
        org.freedesktop.DBus.Properties.GetAll             string:org.freedesktop.UPower.Device | grep -C 2 "State"
      )
      dict entry(
         string "State"
         variant             uint32 2
      )

~

2 -> Discharging

screenshot-2024-10-19-20-18-11
screenshot-2024-10-19-20-18-23

screenshot-2024-10-19-20-20-44

Caveat:

I only have a workstation , so I can't test the battery-applet (it doesn't show at all, but if there is a bug that there is it is hard to notice as i expect for laptop users to who only is visible to not show as charged and they might not notice that this is not changing when plugging it on power, or it might work because it is the main device?!!) that I believe is also wrong here:

https://github.com/pop-os/cosmic-applets/blob/master/cosmic-applet-battery/src/app.rs#L104-L127

I did test the on_battery example of the upower_dbus crate where is showing for my workstation that is false -> that would be interpreted that my workstation /desktop PC was charging with the current code!!!:
screenshot-2024-10-19-21-13-27

Edit: Found gnome-shell's implementation here: https://github.com/GNOME/gnome-shell/blob/d60bdc95faf3607ec579558074cf1ca919db041a/js/ui/status/system.js#L19-L85

Like i said for the system battery (not peripherals) they use the DisplayDevice (an alias object for the system) and the Upower.device.state interface/property

const BUS_NAME = 'org.freedesktop.UPower';
const OBJECT_PATH = '/org/freedesktop/UPower/devices/DisplayDevice';

const DisplayDeviceInterface = loadInterfaceXML('org.freedesktop.UPower.Device');
const PowerManagerProxy = Gio.DBusProxy.makeProxyWrapper(DisplayDeviceInterface);

................
...............


 // The icons
        let chargingState = this._proxy.State === UPower.DeviceState.CHARGING
            ? '-charging' : '';
        let fillLevel = 10 * Math.floor(this._proxy.Percentage / 10);
        const charged =
            this._proxy.State === UPower.DeviceState.FULLY_CHARGED ||
            (this._proxy.State === UPower.DeviceState.CHARGING && fillLevel === 100);
        const icon = charged
            ? 'battery-level-100-charged-symbolic'
            : `battery-level-${fillLevel}${chargingState}-symbolic`;

@mmstick mmstick merged commit b8f0be5 into pop-os:master Oct 22, 2024
3 checks passed
@mmstick
Copy link
Member

mmstick commented Oct 22, 2024

Thanks!

@l-const l-const deleted the power-charging-symbol branch October 22, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power & Battery show that any connected devices are currently charging
2 participants