[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] battery.el, upower fixes
From: |
Stefan Monnier |
Subject: |
Re: [PATCH] battery.el, upower fixes |
Date: |
Mon, 27 Jan 2020 09:43:32 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> Fixes problems users experiencing with `battery-upower'.
> Also checks fore UPower is availability
Thanks, Evgeny.
I'm on a desktop right now so can't test it, but here are some comments:
> Subject: [PATCH] [fix] battery.el (battery-upower-prop)
Fine.
> - Commentary about UPower support
>
> - Interface typofix, use "org.freedesktop.UPower.Device" interface
> instead of "org.freedesktop.UPower"
>
> - Use "DisplayDevice" as `battery-upower-device'. Making
> `battery-upower' to work out-of-box for most users.
>
> - Detect upower availability. Set `battery-status-function' to
> `battery-upower' in case upower is available.
Here, please try and use the ChangeLog format. E.g.
* lisp/battery.el (battery-upower-device): Use "DisplayDevice".
Should make `battery-upower' work out-of-box for most users.
(battery-upower-prop): Interface typofix, use
"org.freedesktop.UPower.Device" interface instead of
"org.freedesktop.UPower".
(battery-status-function): Detect upower availability.
Use `battery-upower' in case UPower is available.
> ;; There is at present support for GNU/Linux, macOS and Windows. This
> -;; library supports both the `/proc/apm' file format of Linux version
> -;; 1.3.58 or newer and the `/proc/acpi/' directory structure of Linux
> -;; 2.4.20 and 2.6. Darwin (macOS) is supported by using the `pmset'
> -;; program. Windows is supported by the GetSystemPowerStatus API call.
> +;; library supports UPower (https://upower.freedesktop.org) via D-Bus
> +;; API or the `/proc/apm' file format of Linux version 1.3.58 or newer
> +;; and the `/proc/acpi/' directory structure of Linux 2.4.20 and 2.6.
> +;; Darwin (macOS) is supported by using the `pmset' program. Windows
> +;; is supported by the GetSystemPowerStatus API call.
Not a criticism, but I just notice that this doesn't mention the
/sys/class/power_supply interface used in more recent kernels. If you
can update this comment accordingly it would be even better (but if you
don't, that's fine as well).
> -(defcustom battery-upower-device "battery_BAT1"
> +(defcustom battery-upower-device "DisplayDevice"
Could add some comment explaining why "DisplayDevice" is a good choice
(and what it means)? Maybe a simple URL pointing to some
UPower documentation?
Intuitively, without any knowledge of UPower (or D-Bus for that matter),
this choice of name sounds rather odd (sounds like the it's talking
about the screen rather than the battery).
> +(declare-function dbus-list-known-names "dbus.el" (bus))
> +
> (defcustom battery-status-function
> - (cond ((and (eq system-type 'gnu/linux)
> + (cond ((member "org.freedesktop.UPower" (dbus-list-known-names :system))
> + #'battery-upower)
Since causes an error when I try it because `dbus` is not yet loaded
when we call this function. Also, please make sure it still behaves
properly if Emacs was built without D-Bus support.
> (concat "/org/freedesktop/UPower/devices/" (or device
> battery-upower-device))
> - "org.freedesktop.UPower"
> + "org.freedesktop.UPower.Device"
Here, as well, if you happen to have a URL handy that documents this
part of the UPower interface, it would make a good comment.
Stefan