From: Evgeny Zajcev <lg.zevlg@gmail.com>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: Andreas Schwab <schwab@suse.de>, Eli Zaretskii <eliz@gnu.org>,
emacs-devel <emacs-devel@gnu.org>
Subject: Re: [PATCH] battery.el, upower fixes
Date: Wed, 5 Feb 2020 03:22:14 +0300 [thread overview]
Message-ID: <CAO=W_ZrQT5Pn93cC_CEf5c41sbyACyAXHpBjiiuOodv--awcbQ@mail.gmail.com> (raw)
In-Reply-To: <jwvr1zaupjy.fsf-monnier+emacs@gnu.org>
[-- 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 --]
next prev parent reply other threads:[~2020-02-05 0:22 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-26 23:04 [PATCH] battery.el, upower fixes Evgeny Zajcev
2020-01-27 14:43 ` Stefan Monnier
2020-01-27 18:23 ` Eli Zaretskii
2020-01-29 7:05 ` Evgeny Zajcev
2020-01-29 8:37 ` Andreas Schwab
2020-01-30 10:22 ` Evgeny Zajcev
2020-02-04 19:55 ` Stefan Monnier
2020-02-04 20:16 ` Michael Albinus
2020-02-04 23:10 ` Stefan Monnier
2020-06-11 16:12 ` Basil L. Contovounesios
2020-02-05 0:22 ` Evgeny Zajcev [this message]
2020-02-05 1:27 ` Stefan Monnier
2020-02-05 2:24 ` Evgeny Zajcev
2020-02-05 3:03 ` Stefan Monnier
2020-02-06 0:55 ` Richard Stallman
2020-02-06 18:27 ` Eli Zaretskii
2020-02-06 7:48 ` Evgeny Zajcev
2020-02-06 14:18 ` Stefan Monnier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAO=W_ZrQT5Pn93cC_CEf5c41sbyACyAXHpBjiiuOodv--awcbQ@mail.gmail.com' \
--to=lg.zevlg@gmail.com \
--cc=eliz@gnu.org \
--cc=emacs-devel@gnu.org \
--cc=monnier@iro.umontreal.ca \
--cc=schwab@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.