Re: [PATCH] battery.el, upower fixes

[Stefan Monnier <monnier@iro.umontreal.ca>]

> 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