[PATCH] battery.el, upower fixes

[PATCH] battery.el, upower fixes

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 114 bytes --]

Fixes problems users experiencing with `battery-upower'.

Also checks fore UPower is availability

Thanks

-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 312 bytes --]

[-- Attachment #2: 0001-fix-battery.el-battery-upower-prop.patch --]
[-- Type: text/x-patch, Size: 2588 bytes --]

From 124ca9d38ae6752f32b88a7c45111ea1bb423829 Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Mon, 27 Jan 2020 01:26:26 +0300
Subject: [PATCH] [fix] battery.el (battery-upower-prop)

- 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.

All this makes `M-x battery RET' to work out-of-box using upower
interface in case of its availability
---
 lisp/battery.el | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/lisp/battery.el b/lisp/battery.el
index 49b01d5b54..057629318e 100644
--- a/lisp/battery.el
+++ b/lisp/battery.el
@@ -23,10 +23,11 @@
 ;;; Commentary:
 
 ;; 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.
 
 ;;; Code:
 
@@ -45,14 +46,18 @@
   :type 'regexp
   :group 'battery)
 
-(defcustom battery-upower-device "battery_BAT1"
+(defcustom battery-upower-device "DisplayDevice"
   "Upower battery device name."
   :version "26.1"
   :type 'string
   :group '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)
+        ((and (eq system-type 'gnu/linux)
 	      (file-readable-p "/proc/apm"))
 	 #'battery-linux-proc-apm)
 	((and (eq system-type 'gnu/linux)
@@ -547,7 +552,7 @@ The following %-sequences are provided:
    :system
    "org.freedesktop.UPower"
    (concat "/org/freedesktop/UPower/devices/" (or device battery-upower-device))
-   "org.freedesktop.UPower"
+   "org.freedesktop.UPower.Device"
    pname))
 
 (defun battery-upower ()
-- 
2.17.1

Re: [PATCH] battery.el, upower fixes

[Stefan Monnier]

> 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

Re: [PATCH] battery.el, upower fixes

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Mon, 27 Jan 2020 02:04:30 +0300
> 
> Fixes problems users experiencing with `battery-upower'.
> 
> Also checks fore UPower is availability

Thanks, a couple of comments below, in addition to what Stefan wrote:

> -(defcustom battery-upower-device "battery_BAT1"
> +(defcustom battery-upower-device "DisplayDevice"
>    "Upower battery device name."
>    :version "26.1"

Please update the :version tag to 28.1, since you are changing the
default value for that version.

Also, I think if D-Bus is not compiled into Emacs, the default value
should be what we had before these changes.

Re: [PATCH] battery.el, upower fixes

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

Need some assistance guys.

I want to write code such as:

  (defcustom battery-status-function
    (cond ((battery-upower-usable-p)
           #'battery-upower)
    ....)

  (defun battery-upower-usable-p ()
     ...)

However such code triggers error:

   cond: Symbol’s function definition is void: battery-upower-usable-p

On fresh Emacs start and M-x battery RET

What is the best approach to write defcustoms with forward references to
functions?

Thanks

-- 
lg

[-- Attachment #2: Type: text/html, Size: 817 bytes --]

Re: [PATCH] battery.el, upower fixes

[Andreas Schwab]

On Jan 29 2020, Evgeny Zajcev wrote:

> What is the best approach to write defcustoms with forward references to
> functions?

The only way to go is to define the function before the point it is
called first.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] battery.el, upower fixes

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 468 bytes --]

Ok, here is the updated version.  I rewrote it in the way to work
out-of-box for most (with single battery) users.
Checked it works fine with no D-Bus compiled in.  Or with disabled "upowerd"

Few users on different laptops tried this code and it works out-of-box for
them.

I did not documented power_supply interface, English is not my native
language, somebody could do it much better.  Also I have no knowledge at
all about power_supply interface.

Thanks

-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 671 bytes --]

[-- Attachment #2: 0001-Make-M-x-battery-RET-work-out-of-box-for-UPower-user.patch --]
[-- Type: text/x-patch, Size: 12748 bytes --]

From 7f8219f12452e6a9409a413c0a4ef255a0d0e496 Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Mon, 27 Jan 2020 01:26:26 +0300
Subject: [PATCH] Make 'M-x battery RET' work out-of-box for UPower users.

* battery.el (battery-upower-prop): Autodetect usable battery device.
  (battery-upower-line-power-device): Autodetect usable line power
  device.
  (battery-status-function): Detect usable UPower service to use
  'battery-upower' as status function.
  (battery-upower): Speedup.  Request D-Bus only once, fetching all
  the properties at once.
  (battery-upower-usable-p): New func, return non-nil if UPower
  service is usable.
  (battery-upower-device-list, battery-upower-device-all-properties,
  battery-upower-device-property): New functions to work with UPower
  devices.
  (battery-upower-dbus-service, battery-upower-dbus-interface,
  battery-upower-dbus-path, battery-upower-dbus-device-interface,
  battery-upower-dbus-device-path): New constants describing UPower
  D-Bus service.
---
 lisp/battery.el | 244 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 171 insertions(+), 73 deletions(-)

diff --git a/lisp/battery.el b/lisp/battery.el
index 49b01d5b54..9b402406da 100644
--- a/lisp/battery.el
+++ b/lisp/battery.el
@@ -23,14 +23,16 @@
 ;;; Commentary:
 
 ;; 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.
 
 ;;; Code:
 
 (require 'timer)
+(require 'dbus)
 (eval-when-compile (require 'cl-lib))
 \f
 (defgroup battery nil
@@ -45,14 +47,174 @@ battery-linux-sysfs-regexp
   :type 'regexp
   :group 'battery)
 
-(defcustom battery-upower-device "battery_BAT1"
-  "Upower battery device name."
-  :version "26.1"
-  :type 'string
+\f
+;;; `upowerd' interface.
+(require 'dbus)
+
+(defconst battery-upower-dbus-service "org.freedesktop.UPower"
+  "Well-known UPower service name for the D-Bus system.")
+
+(defconst battery-upower-dbus-interface "org.freedesktop.UPower"
+  "The interface to UPower.
+See URL `https://upower.freedesktop.org/docs/'.")
+
+(defconst battery-upower-dbus-path "/org/freedesktop/UPower"
+  "D-Bus path to talk to UPower service.")
+
+(defconst battery-upower-dbus-device-interface
+  (concat battery-upower-dbus-interface ".Device")
+  "The Device interface of the UPower.
+See URL `https://upower.freedesktop.org/docs/Device.html'.")
+
+(defconst battery-upower-dbus-device-path
+  (concat battery-upower-dbus-path "/devices")
+  "D-Bus path to talk to devices part of the UPower service.")
+
+(defconst battery-upower-types
+  '((0 . :unknown) (1 . :line-power) (2 . :battery)
+    (3 . :ups) (4 . :monitor) (5 . :mouse)
+    (6 . :keyboard) (7 . :pda) (8 . :phone))
+  "Type of the device.")
+
+(defconst battery-upower-states
+  '((0 . "unknown") (1 . "charging") (2 . "discharging")
+    (3 . "empty") (4 . "fully-charged") (5 . "pending-charge")
+    (6 . "pending-discharge"))
+  "Alist of battery power states.
+Only valid for `:battery' devices.")
+
+(defsubst battery-upower-device-property (device property)
+  "Get value of the single PROPERTY for the UPower DEVICE."
+  (dbus-get-property
+   :system battery-upower-dbus-service
+   (expand-file-name device battery-upower-dbus-device-path)
+   battery-upower-dbus-device-interface
+   property))
+
+(defun battery-upower-device-all-properties (device)
+  "Return value for all available properties for the UPower DEVICE."
+  (dbus-get-all-properties
+   :system battery-upower-dbus-service
+   (expand-file-name device battery-upower-dbus-device-path)
+   battery-upower-dbus-device-interface))
+
+(defun battery-upower-device-list ()
+  "Return list of all available UPower devices.
+Each element is the cons cell in form: (DEVICE . DEVICE-TYPE)."
+  (mapcar (lambda (device-path)
+            (let* ((device (file-relative-name
+                            device-path battery-upower-dbus-device-path))
+                   (type-num (battery-upower-device-property device "Type")))
+              (cons device (or (cdr (assq type-num battery-upower-types))
+                               :unknown))))
+          (dbus-call-method :system battery-upower-dbus-service
+                            battery-upower-dbus-path
+                            battery-upower-dbus-interface
+                            "EnumerateDevices")))
+
+(declare-function cl-find "cl-seq" (cl-item cl-seq &rest cl-keys))
+
+;;;###autoload
+(defun battery-upower-device-autodetect (device-type)
+  "Return first matching UPower device of DEVICE-TYPE."
+  (let ((devices (battery-upower-device-list)))
+    (car (cl-find device-type devices :key 'cdr))))
+
+(defcustom battery-upower-device
+  (dbus-ignore-errors
+    (battery-upower-device-autodetect :battery))
+  "UPower device of the `:battery' type.
+Use `battery-upower-device-list' to list all available UPower devices."
+  :version "28.1"
+  :type 'string-or-null-p
   :group 'battery)
 
+(defcustom battery-upower-line-power-device
+  (dbus-ignore-errors
+    (battery-upower-device-autodetect :line-power))
+  "UPower device of the `:line-power' type.
+Use `battery-upower-device-list' to list all available UPower devices."
+  :version "28.1"
+  :type 'string-or-null-p
+  :group 'battery)
+
+(defun battery-upower-usable-p ()
+  "Return non-nil if UPower interface is available and usable.
+If D-Bus support is not compiled into Emacs, then return nil.
+Issue warring if `battery-upower-device' or
+`battery-upower-line-power-device' is invalid.
+Return nil if `battery-upower-device' is invalid.
+However accept invalid `battery-upower-line-power-device' value,
+just showing warning."
+  (when (and battery-upower-device
+             (dbus-ping :system battery-upower-dbus-service))
+    (let ((bat-type (battery-upower-device-property
+                     battery-upower-device "Type"))
+          (lp-type (when battery-upower-line-power-device
+                     (battery-upower-device-property
+                      battery-upower-line-power-device "Type"))))
+      (unless (eq (cdr (assq lp-type battery-upower-types)) :line-power)
+        (display-warning 'battery (format "\
+Unknown `:line-power' UPower device `battery-upower-line-power-device' (%S).
+AC status won't be shown correctly."
+                                          battery-upower-line-power-device)
+                         :warning))
+
+      (if (eq (cdr (assq bat-type battery-upower-types)) :battery)
+          t
+
+        (display-warning 'battery (format "\
+Unknown `:battery' UPower device `battery-upower-device' (%S).
+Can't use UPower as source for the battery.
+Use `battery-upower-device-list' to list all the UPower devices."
+                                          battery-upower-device)
+                         :warning)
+        nil))))
+
+(defun battery-upower ()
+  "Get battery status from dbus Upower interface.
+This function works only in systems with `upowerd' daemon
+running.
+
+The following %-sequences are provided:
+%c Current capacity (mWh)
+%p Battery load percentage
+%r Current rate
+%B Battery status (verbose)
+%L AC line status (verbose)
+%s Remaining time (to charge or discharge) in seconds
+%m Remaining time (to charge or discharge) in minutes
+%h Remaining time (to charge or discharge) in hours
+%t Remaining time (to charge or discharge) in the form `h:min'"
+  (let* ((bat-props (battery-upower-device-all-properties battery-upower-device))
+         (percents (cdr (assoc "Percentage" bat-props)))
+         (time-to-empty (cdr (assoc "TimeToEmpty" bat-props)))
+         (time-to-full (cdr (assoc "TimeToFull" bat-props)))
+         (state (cdr (assoc "State" bat-props)))
+         (energy (cdr (assoc "Energy" bat-props)))
+         (energy-rate (cdr (assoc "EnergyRate" bat-props)))
+         (online-p (when battery-upower-line-power-device
+                     (battery-upower-device-property
+                      battery-upower-line-power-device "Online")))
+         (seconds (if online-p time-to-full time-to-empty))
+         (minutes (when seconds (/ seconds 60)))
+         (hours (when minutes (/ minutes 60)))
+         (remaining-time (when hours (format "%d:%02d" hours (mod minutes 60)))))
+    (list (cons ?c (if energy (number-to-string (round (* 1000 energy))) "N/A"))
+          (cons ?p (if percents (number-to-string (round percents)) "N/A"))
+          (cons ?r (if energy-rate (concat (number-to-string energy-rate) " W") "N/A"))
+          (cons ?B (if state (cdr (assq state battery-upower-states)) "unknown"))
+          (cons ?L (if online-p "on-line"
+                     (if battery-upower-line-power-device "off-line" "unknown")))
+          (cons ?s (if seconds (number-to-string seconds) "N/A"))
+          (cons ?m (if minutes (number-to-string minutes) "N/A"))
+          (cons ?h (if hours (number-to-string hours) "N/A"))
+          (cons ?t (or remaining-time "N/A")))))
+
 (defcustom battery-status-function
-  (cond ((and (eq system-type 'gnu/linux)
+  (cond ((battery-upower-usable-p)
+         #'battery-upower)
+        ((and (eq system-type 'gnu/linux)
 	      (file-readable-p "/proc/apm"))
 	 #'battery-linux-proc-apm)
 	((and (eq system-type 'gnu/linux)
@@ -538,70 +700,6 @@ battery-linux-sysfs
                     (t "N/A"))))))
 
 \f
-(declare-function dbus-get-property "dbus.el"
-                  (bus service path interface property))
-
-;;; `upowerd' interface.
-(defsubst battery-upower-prop (pname &optional device)
-  (dbus-get-property
-   :system
-   "org.freedesktop.UPower"
-   (concat "/org/freedesktop/UPower/devices/" (or device battery-upower-device))
-   "org.freedesktop.UPower"
-   pname))
-
-(defun battery-upower ()
-  "Get battery status from dbus Upower interface.
-This function works only in systems with `upowerd' daemon
-running.
-
-The following %-sequences are provided:
-%c Current capacity (mWh)
-%p Battery load percentage
-%r Current rate
-%B Battery status (verbose)
-%L AC line status (verbose)
-%s Remaining time (to charge or discharge) in seconds
-%m Remaining time (to charge or discharge) in minutes
-%h Remaining time (to charge or discharge) in hours
-%t Remaining time (to charge or discharge) in the form `h:min'"
-  (let ((percents (battery-upower-prop "Percentage"))
-        (time-to-empty (battery-upower-prop "TimeToEmpty"))
-        (time-to-full (battery-upower-prop "TimeToFull"))
-        (state (battery-upower-prop "State"))
-        (online (battery-upower-prop "Online" "line_power_ACAD"))
-        (energy (battery-upower-prop "Energy"))
-        (energy-rate (battery-upower-prop "EnergyRate"))
-        (battery-states '((0 . "unknown") (1 . "charging")
-                          (2 . "discharging") (3 . "empty")
-                          (4 . "fully-charged") (5 . "pending-charge")
-                          (6 . "pending-discharge")))
-        seconds minutes hours remaining-time)
-    (cond ((and online time-to-full)
-           (setq seconds time-to-full))
-          ((and (not online) time-to-empty)
-           (setq seconds time-to-empty)))
-    (when seconds
-      (setq minutes (/ seconds 60)
-            hours (/ minutes 60)
-	    remaining-time (format "%d:%02d" hours (mod minutes 60))))
-    (list (cons ?c (or (and energy
-                            (number-to-string (round (* 1000 energy))))
-                       "N/A"))
-          (cons ?p (or (and percents (number-to-string (round percents)))
-                       "N/A"))
-          (cons ?r (or (and energy-rate
-                            (concat (number-to-string energy-rate) " W"))
-                       "N/A"))
-          (cons ?B (or (and state (cdr (assoc state battery-states)))
-                       "unknown"))
-          (cons ?L (or (and online "on-line") "off-line"))
-          (cons ?s (or (and seconds (number-to-string seconds)) "N/A"))
-          (cons ?m (or (and minutes (number-to-string minutes)) "N/A"))
-          (cons ?h (or (and hours (number-to-string hours)) "N/A"))
-          (cons ?t (or remaining-time "N/A")))))
-
-\f
 ;;; `apm' interface for BSD.
 (defun battery-bsd-apm ()
   "Get APM status information from BSD apm binary.
-- 
2.17.1

Re: [PATCH] battery.el, upower fixes

[Stefan Monnier]

> Ok, here is the updated version.  I rewrote it in the way to work
> out-of-box for most (with single battery) users.
> Checked it works fine with no D-Bus compiled in.  Or with disabled "upowerd"

I get an error on `cl-find` that's a void function.

> I did not documented power_supply interface, English is not my native
> language, somebody could do it much better.  Also I have no knowledge at
> all about power_supply interface.

No problem, I'll take care of that afterwards.

> * battery.el (battery-upower-prop): Autodetect usable battery device.
>   (battery-upower-line-power-device): Autodetect usable line power
>   device.
>   (battery-status-function): Detect usable UPower service to use
>   'battery-upower' as status function.
>   (battery-upower): Speedup.  Request D-Bus only once, fetching all
>   the properties at once.
>   (battery-upower-usable-p): New func, return non-nil if UPower
>   service is usable.
>   (battery-upower-device-list, battery-upower-device-all-properties,
>   battery-upower-device-property): New functions to work with UPower
>   devices.
>   (battery-upower-dbus-service, battery-upower-dbus-interface,
>   battery-upower-dbus-path, battery-upower-dbus-device-interface,
>   battery-upower-dbus-device-path): New constants describing UPower
>   D-Bus service.

Looks good, thanks.

Your patch failed to apply to `master`, tho; it seems like it was made
against an "old" version of battery.el (maybe from Emacs-26.3?).

>  (require 'timer)
> +(require 'dbus)
[...]
> -(defcustom battery-upower-device "battery_BAT1"
> -  "Upower battery device name."
> -  :version "26.1"
> -  :type 'string
> +\f
> +;;; `upowerd' interface.
> +(require 'dbus)

You (require 'dbus) twice.

> +(defsubst battery-upower-device-property (device property)

Don't make it a defubst: there's no point trying to optimize calls to
this function.

> +(declare-function cl-find "cl-seq" (cl-item cl-seq &rest cl-keys))

Better just (require 'cl-lib) since it seems like the above
`declare-function` (which promises that the function will have be made
available by the time we call it) is lying.
[ BTW, this use of `cl-find` can be replaced by `rassq`.  ]

> +;;;###autoload
> +(defun battery-upower-device-autodetect (device-type)

Why do we need to autoload it?

> +(defcustom battery-upower-device
> +  (dbus-ignore-errors
> +    (battery-upower-device-autodetect :battery))

I think there's a possibility that the set of devices changes over time
(e.g. on my machine (battery-upower-device-list) currently only shows
battery_BAT0, but I can insert a second battery and I'd expect the list
to then include a battery_BAT1 as well).  So maybe we should instead
allow a special value (e.g. nil) to mean "autodetect" and then do the
autodetection dynamically.  WDYT?

> +  "UPower device of the `:battery' type.
> +Use `battery-upower-device-list' to list all available UPower devices."
> +  :version "28.1"
> +  :type 'string-or-null-p
>    :group 'battery)

`string-or-null-p` is not a known widget type.  You probably need to use
(choice string (const nil)) or something like that instead.

On a related note: the battery.el in `master` by default uses the sysfs
interface and combines all available batteries in there into the single
output result (for those users whose laptop has 2 batteries).
[ But let's keep this for a later patch.  ]

> +(defun battery-upower ()

Could you keep this function where it was, so the patch shows clearly
what was changed?

BTW, I wonder if `battery-upower-usable-p` should maybe only check
(dbus-ping :system battery-upower-dbus-service) and nothing else, so we
use upower if the service is available and even we have it
mis-configured.  It also also have the benefit that the mere act of
loading `battery.el` (which can happen even without any intention to use
`battery`) wouldn't emit warnings when the device names are wrong, IMO.

Also, it'd be nice to keep/move more of the code underneath the `defcustom`s.


        Stefan

Re: [PATCH] battery.el, upower fixes

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +(defcustom battery-upower-device
>> +  (dbus-ignore-errors
>> +    (battery-upower-device-autodetect :battery))
>
> I think there's a possibility that the set of devices changes over time
> (e.g. on my machine (battery-upower-device-list) currently only shows
> battery_BAT0, but I can insert a second battery and I'd expect the list
> to then include a battery_BAT1 as well).  So maybe we should instead
> allow a special value (e.g. nil) to mean "autodetect" and then do the
> autodetection dynamically.  WDYT?

Usually, D-Bus services send a signal when a device is added/removed. IIUC
https://upower.freedesktop.org/docs/UPower.html,
org.freedesktop.UPower.DeviceAdded and org.freedesktop.UPower.DeviceRemoved
serve for that purpose. So you shall subscribe to those signals and
handle them accordingly.

>         Stefan

Best regards, Michael.

Re: [PATCH] battery.el, upower fixes

[Stefan Monnier]

>>> +(defcustom battery-upower-device
>>> +  (dbus-ignore-errors
>>> +    (battery-upower-device-autodetect :battery))
>>
>> I think there's a possibility that the set of devices changes over time
>> (e.g. on my machine (battery-upower-device-list) currently only shows
>> battery_BAT0, but I can insert a second battery and I'd expect the list
>> to then include a battery_BAT1 as well).  So maybe we should instead
>> allow a special value (e.g. nil) to mean "autodetect" and then do the
>> autodetection dynamically.  WDYT?
>
> Usually, D-Bus services send a signal when a device is added/removed. IIUC
> https://upower.freedesktop.org/docs/UPower.html,
> org.freedesktop.UPower.DeviceAdded and org.freedesktop.UPower.DeviceRemoved
> serve for that purpose. So you shall subscribe to those signals and
> handle them accordingly.

Sure, but you wouldn't want to set a `defcustom` in response to that, so
the `defcustom` should have a separate setting to mean "autodetect".


        Stefan

Re: [PATCH] battery.el, upower fixes

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 5878 bytes --]

вт, 4 февр. 2020 г. в 22:55, Stefan Monnier <monnier@iro.umontreal.ca>:

> > Ok, here is the updated version.  I rewrote it in the way to work
> > out-of-box for most (with single battery) users.
> > Checked it works fine with no D-Bus compiled in.  Or with disabled
> "upowerd"
>
> I get an error on `cl-find` that's a void function.
>
>
Yeah, will get rid of it

> I did not documented power_supply interface, English is not my native
> > language, somebody could do it much better.  Also I have no knowledge at
> > all about power_supply interface.
>
> No problem, I'll take care of that afterwards.
>
> > * battery.el (battery-upower-prop): Autodetect usable battery device.
> >   (battery-upower-line-power-device): Autodetect usable line power
> >   device.
> >   (battery-status-function): Detect usable UPower service to use
> >   'battery-upower' as status function.
> >   (battery-upower): Speedup.  Request D-Bus only once, fetching all
> >   the properties at once.
> >   (battery-upower-usable-p): New func, return non-nil if UPower
> >   service is usable.
> >   (battery-upower-device-list, battery-upower-device-all-properties,
> >   battery-upower-device-property): New functions to work with UPower
> >   devices.
> >   (battery-upower-dbus-service, battery-upower-dbus-interface,
> >   battery-upower-dbus-path, battery-upower-dbus-device-interface,
> >   battery-upower-dbus-device-path): New constants describing UPower
> >   D-Bus service.
>
> Looks good, thanks.
>
> Your patch failed to apply to `master`, tho; it seems like it was made
> against an "old" version of battery.el (maybe from Emacs-26.3?).
>
>
oh, sorry, I thought I've applied it to Emacs28, Will reapply to master

>  (require 'timer)
> > +(require 'dbus)
> [...]
> > -(defcustom battery-upower-device "battery_BAT1"
> > -  "Upower battery device name."
> > -  :version "26.1"
> > -  :type 'string
> > +
> > +;;; `upowerd' interface.
> > +(require 'dbus)
>
> You (require 'dbus) twice.
>
> > +(defsubst battery-upower-device-property (device property)
>
> Don't make it a defubst: there's no point trying to optimize calls to
> this function.
>
> > +(declare-function cl-find "cl-seq" (cl-item cl-seq &rest cl-keys))
>
> Better just (require 'cl-lib) since it seems like the above
> `declare-function` (which promises that the function will have be made
> available by the time we call it) is lying.
> [ BTW, this use of `cl-find` can be replaced by `rassq`.  ]
>
>

Yeah, `rassq' is just what is needed here

> +;;;###autoload
> > +(defun battery-upower-device-autodetect (device-type)
>
> Why do we need to autoload it?


Hmm, maybe no reason actually.  My thought was about providing some way for
user to autodetect :battery and :line-power device in his init.el and
explicitly write something like:

  (setq battery-upower-device (battery-upower-device-autodetect :battery))

So he states in the config, that he has upower and want error to be rised
if upower service is not available.

This matters especially if we make `battery-upower-device' to be nil by
default in order to silently autodetect upower device and fallback to other
battery backend if upower service is not available.

> +(defcustom battery-upower-device
> > +  (dbus-ignore-errors
> > +    (battery-upower-device-autodetect :battery))
>
> I think there's a possibility that the set of devices changes over time
> (e.g. on my machine (battery-upower-device-list) currently only shows
> battery_BAT0, but I can insert a second battery and I'd expect the list
> to then include a battery_BAT1 as well).  So maybe we should instead
> allow a special value (e.g. nil) to mean "autodetect" and then do the
> autodetection dynamically.  WDYT?
>
> > +  "UPower device of the `:battery' type.
> > +Use `battery-upower-device-list' to list all available UPower devices."
> > +  :version "28.1"
> > +  :type 'string-or-null-p
> >    :group 'battery)
>
> `string-or-null-p` is not a known widget type.  You probably need to use
> (choice string (const nil)) or something like that instead.
>
>
noted.

On a related note: the battery.el in `master` by default uses the sysfs
> interface and combines all available batteries in there into the single
> output result (for those users whose laptop has 2 batteries).
> [ But let's keep this for a later patch.  ]
>
> > +(defun battery-upower ()
>
> Could you keep this function where it was, so the patch shows clearly
> what was changed?
>
> BTW, I wonder if `battery-upower-usable-p` should maybe only check
> (dbus-ping :system battery-upower-dbus-service) and nothing else, so we
> use upower if the service is available and even we have it
> mis-configured.  It also also have the benefit that the mere act of
> loading `battery.el` (which can happen even without any intention to use
> `battery`) wouldn't emit warnings when the device names are wrong, IMO.
>
>
If `battery.el` is loaded unintentionally, then all the custom vars will
have suitable values to have no warnings.  However if user explicitly set
`battery-upower-device` to invalid value, then warning will arise on
battery.el load.  Otherwise (no warning on invalid
`battery-upower-device`), if upower service is available -
`battery-status-function` will be set to upower, and `M-x battery RET` will
produce N/A values, and there is no clue for the user that he just have
invalid value for the `battery-upower-device`.

Also, it'd be nice to keep/move more of the code underneath the
> `defcustom`s.
>
>
Reason why I've put defcustom beneath all the functions is:
>> What is the best approach to write defcustoms with forward references to
>> functions?

> The only way to go is to define the function before the point it is
> called first.

-- 
lg

[-- Attachment #2: Type: text/html, Size: 7972 bytes --]

Re: [PATCH] battery.el, upower fixes

[Stefan Monnier]

> Hmm, maybe no reason actually.  My thought was about providing some way for
> user to autodetect :battery and :line-power device in his init.el and
> explicitly write something like:
>
>   (setq battery-upower-device (battery-upower-device-autodetect :battery))
>
> So he states in the config, that he has upower and want error to be rised
> if upower service is not available.

But

    (setq battery-status-function #'battery-upower)

already does that in a more direct and reliable way, no?

> If `battery.el` is loaded unintentionally, then all the custom vars will
> have suitable values to have no warnings.  However if user explicitly set
> `battery-upower-device` to invalid value, then warning will arise on
> battery.el load.

Exactly, and even if the user did not intend to use battery in this
session (Elisp files can be loaded "spuriously").

> Otherwise (no warning on invalid `battery-upower-device`), if upower
> service is available - `battery-status-function` will be set to
> upower, and `M-x battery RET` will produce N/A values, and there is no
> clue for the user that he just have invalid value for the
> `battery-upower-device`.

No: `M-x battery` will emit the warning.


        Stefan

Re: [PATCH] battery.el, upower fixes

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

ср, 5 февр. 2020 г. в 04:27, Stefan Monnier <monnier@iro.umontreal.ca>:

> > Hmm, maybe no reason actually.  My thought was about providing some way
> for
> > user to autodetect :battery and :line-power device in his init.el and
> > explicitly write something like:
> >
> >   (setq battery-upower-device (battery-upower-device-autodetect
> :battery))
> >
> > So he states in the config, that he has upower and want error to be rised
> > if upower service is not available.
>
> But
>
>     (setq battery-status-function #'battery-upower)
>
> already does that in a more direct and reliable way, no?
>
>
Yes, reasonable indeed.

> If `battery.el` is loaded unintentionally, then all the custom vars will
> > have suitable values to have no warnings.  However if user explicitly set
> > `battery-upower-device` to invalid value, then warning will arise on
> > battery.el load.
>
> Exactly, and even if the user did not intend to use battery in this
> session (Elisp files can be loaded "spuriously").
>
> > Otherwise (no warning on invalid `battery-upower-device`), if upower
> > service is available - `battery-status-function` will be set to
> > upower, and `M-x battery RET` will produce N/A values, and there is no
> > clue for the user that he just have invalid value for the
> > `battery-upower-device`.
>
> No: `M-x battery` will emit the warning.
>
>
So `battery-upower` should check the correctness of the
`battery-upower-device` on every call?  It would make it heavier, but of
course more reliable, because user might change the value of the
`batter-upower-device` in runtime

We might have `nil` values for the `battery-upower-device` and
`battery-upower-line-power-device` meaning "autodetect".  This will require
additional call to D-Bus (battery-upower-device-list) on every call to
`battery-upower`, probably this is OK.  If user want extra speed he just
set right values for that vars.  Sounds good?

Having `nil` values for those defcustom vars, also won't require putting
them after all the functions

-- 
lg

[-- Attachment #2: Type: text/html, Size: 2785 bytes --]

Re: [PATCH] battery.el, upower fixes

[Stefan Monnier]

>> No: `M-x battery` will emit the warning.
> So `battery-upower` should check the correctness of the
> `battery-upower-device` on every call?  It would make it heavier, but of
> course more reliable, because user might change the value of the
> `batter-upower-device` in runtime
>
> We might have `nil` values for the `battery-upower-device` and
> `battery-upower-line-power-device` meaning "autodetect".  This will require
> additional call to D-Bus (battery-upower-device-list) on every call to
> `battery-upower`, probably this is OK.  If user want extra speed he just
> set right values for that vars.  Sounds good?

Right, we have to decouple the custom vars `battery-upower-device` and
`battery-upower-line-power-device` from the actual list of devices.
We can do this by recomputing the actual list (and checking its
validity) every time time.

Or if the performance impact matters, we can do this computation on the
first call and then check every time that the custom vars haven't
changed (and ask for D-Bus to calls us back when the list needs to be
changed).

> Having `nil` values for those defcustom vars, also won't require putting
> them after all the functions

Right.


        Stefan

Re: [PATCH] battery.el, upower fixes

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

When these changes are instakked, would you please tell me?
I would like to see if they work on Trisquel with my laptop.

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [PATCH] battery.el, upower fixes

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 1433 bytes --]

ср, 5 февр. 2020 г. в 06:03, Stefan Monnier <monnier@iro.umontreal.ca>:

> >> No: `M-x battery` will emit the warning.
> > So `battery-upower` should check the correctness of the
> > `battery-upower-device` on every call?  It would make it heavier, but of
> > course more reliable, because user might change the value of the
> > `batter-upower-device` in runtime
> >
> > We might have `nil` values for the `battery-upower-device` and
> > `battery-upower-line-power-device` meaning "autodetect".  This will
> require
> > additional call to D-Bus (battery-upower-device-list) on every call to
> > `battery-upower`, probably this is OK.  If user want extra speed he just
> > set right values for that vars.  Sounds good?
>
> Right, we have to decouple the custom vars `battery-upower-device` and
> `battery-upower-line-power-device` from the actual list of devices.
> We can do this by recomputing the actual list (and checking its
> validity) every time time.
>
> Or if the performance impact matters, we can do this computation on the
> first call and then check every time that the custom vars haven't
> changed (and ask for D-Bus to calls us back when the list needs to be
> changed).
>

Not much impact actually.  Here is the updated battery.el against the
master this time.
Also added "%b" format-spec, so 'M-x display-battery-mode RET' shows "+" in
case AC is on-line

Thanks


-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 1940 bytes --]

[-- Attachment #2: 0001-Make-M-x-battery-RET-work-out-of-box-for-UPower-user.patch --]
[-- Type: text/x-patch, Size: 11125 bytes --]

From 9164b1da07b592585d84806a7f7afb8d4dba408e Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Thu, 6 Feb 2020 10:35:12 +0300
Subject: [PATCH] Make 'M-x battery RET' work out-of-box for UPower users.

* battery.el (battery-upower-prop): Removed in favor for
  'battery-upower-device-property'.
  (battery-upower-device): Can be nil, meaning autodetect the battery
  device.
  (battery-upower-line-power-device): New.  line-power device.  Can be
  nil, meaning autodetect line-power device.
  (battery-status-function): Check UPower service is available to use
  'battery-upower' as status function.
  (battery-upower): Speedup.  Request D-Bus only once, fetching all
  the properties at once.  Provide string for "%b" format spec.
  (battery-upower-device-list, battery-upower-device-all-properties,
  battery-upower-device-property): New functions to work with UPower
  devices.
  (battery-upower-dbus-service, battery-upower-dbus-interface,
  battery-upower-dbus-path, battery-upower-dbus-device-interface,
  battery-upower-dbus-device-path): New constants describing UPower
  D-Bus service.
---
 lisp/battery.el | 176 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 124 insertions(+), 52 deletions(-)

diff --git a/lisp/battery.el b/lisp/battery.el
index 1d3390070c..5368e1b0e9 100644
--- a/lisp/battery.el
+++ b/lisp/battery.el
@@ -23,14 +23,16 @@
 ;;; Commentary:
 
 ;; 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.
 
 ;;; Code:
 
 (require 'timer)
+(require 'dbus)
 (eval-when-compile (require 'cl-lib))
 \f
 (defgroup battery nil
@@ -38,12 +40,25 @@ battery
   :prefix "battery-"
   :group 'hardware)
 
-(defcustom battery-upower-device "battery_BAT1"
-  "Upower battery device name."
-  :version "26.1"
-  :type 'string
+(defcustom battery-upower-device nil
+  "UPower device of the `:battery' type.
+Use `battery-upower-device-list' to list all available UPower devices.
+If set to nil, then autodetect `:battery' device."
+  :version "28.1"
+  :type '(choice string (const :tag "Autodetect" nil))
   :group 'battery)
 
+(defcustom battery-upower-line-power-device nil
+  "UPower device of the `:line-power' type.
+Use `battery-upower-device-list' to list all available UPower devices.
+If set to nil, then autodetect `:battery' device."
+  :version "28.1"
+  :type '(choice string (const :tag "Autodetect" nil))
+  :group 'battery)
+
+(defconst battery-upower-dbus-service "org.freedesktop.UPower"
+  "Well-known UPower service name for the D-Bus system.")
+
 (defun battery--find-linux-sysfs-batteries ()
   (let ((dirs nil))
     (dolist (file (directory-files "/sys/class/power_supply/" t))
@@ -54,7 +69,9 @@ battery--find-linux-sysfs-batteries
     (nreverse dirs)))
 
 (defcustom battery-status-function
-  (cond ((and (eq system-type 'gnu/linux)
+  (cond ((dbus-ping :system battery-upower-dbus-service)
+         #'battery-upower)
+        ((and (eq system-type 'gnu/linux)
 	      (file-readable-p "/proc/apm"))
 	 #'battery-linux-proc-apm)
 	((and (eq system-type 'gnu/linux)
@@ -537,17 +554,68 @@ battery-linux-sysfs
                     (t "N/A"))))))
 
 \f
-(declare-function dbus-get-property "dbus.el"
-                  (bus service path interface property))
-
 ;;; `upowerd' interface.
-(defsubst battery-upower-prop (pname &optional device)
+(defconst battery-upower-dbus-interface "org.freedesktop.UPower"
+  "The interface to UPower.
+See URL `https://upower.freedesktop.org/docs/'.")
+
+(defconst battery-upower-dbus-path "/org/freedesktop/UPower"
+  "D-Bus path to talk to UPower service.")
+
+(defconst battery-upower-dbus-device-interface
+  (concat battery-upower-dbus-interface ".Device")
+  "The Device interface of the UPower.
+See URL `https://upower.freedesktop.org/docs/Device.html'.")
+
+(defconst battery-upower-dbus-device-path
+  (concat battery-upower-dbus-path "/devices")
+  "D-Bus path to talk to devices part of the UPower service.")
+
+(defconst battery-upower-types
+  '((0 . :unknown) (1 . :line-power) (2 . :battery)
+    (3 . :ups) (4 . :monitor) (5 . :mouse)
+    (6 . :keyboard) (7 . :pda) (8 . :phone))
+  "Type of the device.")
+
+(defconst battery-upower-states
+  '((0 . "unknown") (1 . "charging") (2 . "discharging")
+    (3 . "empty") (4 . "fully-charged") (5 . "pending-charge")
+    (6 . "pending-discharge"))
+  "Alist of battery power states.
+Only valid for `:battery' devices.")
+
+(defun battery-upower-device-property (device property)
+  "Get value of the single PROPERTY for the UPower DEVICE."
   (dbus-get-property
-   :system
-   "org.freedesktop.UPower"
-   (concat "/org/freedesktop/UPower/devices/" (or device battery-upower-device))
-   "org.freedesktop.UPower"
-   pname))
+   :system battery-upower-dbus-service
+   (expand-file-name device battery-upower-dbus-device-path)
+   battery-upower-dbus-device-interface
+   property))
+
+(defun battery-upower-device-all-properties (device)
+  "Return value for all available properties for the UPower DEVICE."
+  (dbus-get-all-properties
+   :system battery-upower-dbus-service
+   (expand-file-name device battery-upower-dbus-device-path)
+   battery-upower-dbus-device-interface))
+
+(defun battery-upower-device-list ()
+  "Return list of all available UPower devices.
+Each element is the cons cell in form: (DEVICE . DEVICE-TYPE)."
+  (mapcar (lambda (device-path)
+            (let* ((device (file-relative-name
+                            device-path battery-upower-dbus-device-path))
+                   (type-num (battery-upower-device-property device "Type")))
+              (cons device (or (cdr (assq type-num battery-upower-types))
+                               :unknown))))
+          (dbus-call-method :system battery-upower-dbus-service
+                            battery-upower-dbus-path
+                            battery-upower-dbus-interface
+                            "EnumerateDevices")))
+
+(defun battery-upower-device-autodetect (device-type)
+  "Return first matching UPower device of DEVICE-TYPE."
+  (car (rassq device-type (battery-upower-device-list))))
 
 (defun battery-upower ()
   "Get battery status from dbus Upower interface.
@@ -559,45 +627,49 @@ battery-upower
 %p Battery load percentage
 %r Current rate
 %B Battery status (verbose)
+%b Battery status: empty means high, `-' means low,
+   `!' means critical, and `+' means charging
 %L AC line status (verbose)
 %s Remaining time (to charge or discharge) in seconds
 %m Remaining time (to charge or discharge) in minutes
 %h Remaining time (to charge or discharge) in hours
 %t Remaining time (to charge or discharge) in the form `h:min'"
-  (let ((percents (battery-upower-prop "Percentage"))
-        (time-to-empty (battery-upower-prop "TimeToEmpty"))
-        (time-to-full (battery-upower-prop "TimeToFull"))
-        (state (battery-upower-prop "State"))
-        (online (battery-upower-prop "Online" "line_power_ACAD"))
-        (energy (battery-upower-prop "Energy"))
-        (energy-rate (battery-upower-prop "EnergyRate"))
-        (battery-states '((0 . "unknown") (1 . "charging")
-                          (2 . "discharging") (3 . "empty")
-                          (4 . "fully-charged") (5 . "pending-charge")
-                          (6 . "pending-discharge")))
-        seconds minutes hours remaining-time)
-    (cond ((and online time-to-full)
-           (setq seconds time-to-full))
-          ((and (not online) time-to-empty)
-           (setq seconds time-to-empty)))
-    (when seconds
-      (setq minutes (/ seconds 60)
-            hours (/ minutes 60)
-	    remaining-time (format "%d:%02d" hours (mod minutes 60))))
-    (list (cons ?c (or (and energy
-                            (number-to-string (round (* 1000 energy))))
-                       "N/A"))
-          (cons ?p (or (and percents (number-to-string (round percents)))
-                       "N/A"))
-          (cons ?r (or (and energy-rate
-                            (concat (number-to-string energy-rate) " W"))
-                       "N/A"))
-          (cons ?B (or (and state (cdr (assoc state battery-states)))
-                       "unknown"))
-          (cons ?L (or (and online "on-line") "off-line"))
-          (cons ?s (or (and seconds (number-to-string seconds)) "N/A"))
-          (cons ?m (or (and minutes (number-to-string minutes)) "N/A"))
-          (cons ?h (or (and hours (number-to-string hours)) "N/A"))
+  (let* ((bat-device (or battery-upower-device
+                         (battery-upower-device-autodetect :battery)))
+         (bat-props (when bat-device
+                      (battery-upower-device-all-properties bat-device)))
+         (percents (cdr (assoc "Percentage" bat-props)))
+         (time-to-empty (cdr (assoc "TimeToEmpty" bat-props)))
+         (time-to-full (cdr (assoc "TimeToFull" bat-props)))
+         (state (cdr (assoc "State" bat-props)))
+         (level (cdr (assoc "BatteryLevel" bat-props)))
+         (energy (cdr (assoc "Energy" bat-props)))
+         (energy-rate (cdr (assoc "EnergyRate" bat-props)))
+         (lp-device (or battery-upower-line-power-device
+                        (battery-upower-device-autodetect :line-power)))
+         (online-p (when lp-device
+                     (battery-upower-device-property lp-device "Online")))
+         (seconds (if online-p time-to-full time-to-empty))
+         (minutes (when seconds (/ seconds 60)))
+         (hours (when minutes (/ minutes 60)))
+         (remaining-time (when hours
+                           (format "%d:%02d" hours (mod minutes 60)))))
+    (list (cons ?c (if energy (number-to-string (round (* 1000 energy))) "N/A"))
+          (cons ?p (if percents (number-to-string (round percents)) "N/A"))
+          (cons ?r (if energy-rate
+                       (concat (number-to-string energy-rate) " W")
+                     "N/A"))
+          (cons ?B (if state
+                       (cdr (assq state battery-upower-states))
+                     "unknown"))
+          (cons ?b (cond ((= level 3) "-")
+                         ((= level 4) "!")
+                         (online-p "+")
+                         (t "")))
+          (cons ?L (if online-p "on-line" (if lp-device "off-line" "unknown")))
+          (cons ?s (if seconds (number-to-string seconds) "N/A"))
+          (cons ?m (if minutes (number-to-string minutes) "N/A"))
+          (cons ?h (if hours (number-to-string hours) "N/A"))
           (cons ?t (or remaining-time "N/A")))))
 
 \f
-- 
2.17.1

Re: [PATCH] battery.el, upower fixes

[Stefan Monnier]

> Not much impact actually.  Here is the updated battery.el against the
> master this time.  Also added "%b" format-spec, so 'M-x
> display-battery-mode RET' shows "+" in case AC is on-line

Great, thanks, pushed,


        Stefan

Re: [PATCH] battery.el, upower fixes

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Wed, 05 Feb 2020 19:55:31 -0500
> Cc: schwab@suse.de, eliz@gnu.org, lg.zevlg@gmail.com, emacs-devel@gnu.org
> 
> When these changes are instakked, would you please tell me?

They are installed now (on the master branch).

Re: [PATCH] battery.el, upower fixes

[Basil L. Contovounesios]

Michael Albinus <michael.albinus@gmx.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> I think there's a possibility that the set of devices changes over time
>> (e.g. on my machine (battery-upower-device-list) currently only shows
>> battery_BAT0, but I can insert a second battery and I'd expect the list
>> to then include a battery_BAT1 as well).  So maybe we should instead
>> allow a special value (e.g. nil) to mean "autodetect" and then do the
>> autodetection dynamically.  WDYT?
>
> Usually, D-Bus services send a signal when a device is added/removed. IIUC
> https://upower.freedesktop.org/docs/UPower.html,
> org.freedesktop.UPower.DeviceAdded and org.freedesktop.UPower.DeviceRemoved
> serve for that purpose. So you shall subscribe to those signals and
> handle them accordingly.

Now done in https://debbugs.gnu.org/39491#55.

-- 
Basil