[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] battery.el, upower fixes

From: Evgeny Zajcev
Subject: Re: [PATCH] battery.el, upower fixes
Date: Wed, 5 Feb 2020 03:22:14 +0300

вт, 4 февр. 2020 г. в 22:55, Stefan Monnier <address@hidden>:
> 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.


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]