Introduce a power-profiles-daemon module #2971

picnoir · 2024-02-25T14:01:00Z

We introduce a module in charge to display and toggle on click the power profiles via power-profiles-daemon.

https://gitlab.freedesktop.org/upower/power-profiles-daemon

This daemon is pretty widespread. It's the component used by Gnome and KDE to manage the power profiles. The power management daemon is a pretty important software component for laptops and other battery-powered devices.

We're using the daemon DBus interface to:

Fetch the available power profiles.
Track the active power profile.
Change the active power profile.

The original author recently gave up maintenance on the project. The Upower group took over the maintenance burden… …and created a new DBus name for the project. The old name is still advertised for now. We use the old name for compatibility sake: most distributions did not release 0.20, which introduces this new DBus name. We'll likely revisit
this in the future and point to the new bus name. See the inline comment for more details.

Given how widespread this daemon is, I activated the module in the default configuration. I'm not 100% sure about myself on that one though :)

Demo video: https://social.alternativebit.fr/media/514a598a09c6c9047e61af405d188aa7b99a5730015c95ad787f7c023ea1f461.mp4

picnoir · 2024-02-25T14:12:55Z

Fixes #1539

Alexays · 2024-02-25T22:44:47Z

src/modules/power_profiles_daemon.cpp

+  activeProfile_ = std::find_if(availableProfiles_.begin(), availableProfiles_.end(), pred);
+  if (activeProfile_ == availableProfiles_.end()) {
+    spdlog::error("PowerProfilesDaemon: FATAL, can't find the active profile in the available profiles list");
+    exit(-1);


why an exit?

Erf! Two reasons 😅

When I wrote this, I did not know the best way to crash the module to prevent it to do some dangerous dereferences. I made a mental note about that and put this exit in the meantime.

My memory is not so good, I tend to forget my mental notes very fast 😀

Anyway, after looking the current codebase, it seems like throwing a runtime error does the trick. Don't hesitate to point me to another direction if this is not the best way to go.

picnoir · 2024-02-26T08:46:37Z

Rebased, there was a conflict on the config file.

Documentation-wise, I could use some help.

I'm not sure what to do for now. Is the wiki the canonical source for it? How are you dealing with it when adding a new feature like this?

There's not much to document for now, the label ain't configurable: I personally don't really need that. I planned to implement that iteratively in a subsequent PR. Unless you'd prefer this to be done all at the same time in this PR.

~~Edit: CI is red apparently, I don't have time to fix it RN. I'll do that tonight.~~ Fixed

We introduce a module in charge to display and toggle on click the power profiles via power-profiles-daemon. https://gitlab.freedesktop.org/upower/power-profiles-daemon This daemon is pretty widespread. It's the component used by Gnome and KDE to manage the power profiles. The power management daemon is a pretty important software component for laptops and other battery-powered devices. We're using the daemon DBus interface to: - Fetch the available power profiles. - Track the active power profile. - Change the active power profile. The original author recently gave up maintenance on the project. The Upower group took over the maintenance burden… …and created a new DBus name for the project. The old name is still advertised for now. We use the old name for compatibility sake: most distributions did not release 0.20, which introduces this new DBus name. We'll likely revisit this in the future and point to the new bus name. See the inline comment for more details. Given how widespread this daemon is, I activated the module in the default configuration.

Alexays · 2024-02-27T15:39:49Z

When adding new feature like this, a new entry in the man and wiki with module settings like others is ok :)

picnoir · 2024-02-27T22:16:31Z

Thanks for the directions. I'll write the manpage section by the end of the week. (and fix the clang-tidy issues)

I'm a bit puzzled by the freebsd CI failure. Is it expected?

lheckemann · 2024-02-29T13:59:22Z

Thanks for implementing this, I've wanted it for a while!

It would be nice to have the format string be configurable; the full profile names are a bit wide to fit neatly in my already crowded status bar, so I'd like to be able to use just 🌱 / ⚖️ / ⚡ for example.

alebastr · 2024-02-29T17:45:53Z

I'm a bit puzzled by the freebsd CI failure. Is it expected?

FreeBSD CI is flaky. It works via qemu vm in a macOS virtual machine, and there are random connectivity problems or timeouts. The actual status in the log is fine, so please ignore the failure.

It would be nice to have the format string be configurable; the full profile names are a bit wide to fit neatly in my already crowded status bar, so I'd like to be able to use just 🌱 / ⚖️ / ⚡ for example.

I'd agree with that and even suggest to use the icons by default. For a module enabled in the default config, it takes too much screen space.
Although fontawesome icons would be a better choice due to an ability to set the color with CSS.

alebastr · 2024-03-01T02:43:14Z

It appears that this PR crashes instantly if you don't have p-p-d installed.

While investigating, I found a wonderful fact: g_dbus_proxy_new_* family of APIs does not consider missing service an error. I.e. successful return from the constructor doesn't mean that it's safe to touch cached properties (profilesVariant contains nullptr with obvious consequences) or call remote methods. And every public method in the module should check that the initialization was completeld (update() crashes on the first line while attempting to dereference an empty iterator).
Also, create_for_bus_sync or really anything *_sync can block for 25 seconds, but that's not as exciting compared to the above.

So the module init should look like

create_for_bus (async version)
call("org.freedesktop.DBus.Properties.GetAll") to force peer status check
if the chain of async operations succeeded and the necessary properties are available, set the readiness flag and show the module

...I think I need to schedule some time to review existing DBus modules.

We move to a single icon label format to save space on the bar. We still display the profile name and the driver in the tooltip.

2 changes to address the review feedback: 1. Aleksei pointed out in this comment (Alexays#2971 (comment)) that there's no way to tell if a proxy is alive other than trying to call a method on it. We perform a little dance to check whether or not power-profiles-daemon is available on the system by calling properties.GetAll. If something responds, we assume power-profiles-daemon is installed, it's then safe to draw the widget and attach the callback to the active profile. 2. We replaced all the synchronous DBus operations by their async counterparts.

picnoir · 2024-03-01T11:48:21Z

Thanks for the detailed feedback @alebastr, it was super useful!

From a (very) occasional C++ developper and Glib newbie perspective, this DBus API is pretty confusing and painful to use. :/

I adressed @lheckemann and @alebastr feedback wrt. the label formatting in 61fed6a. We're now only displaying a small icon in the default config. Everything, can be customized.

I addressed #2971 (comment) in 901314a.

I'll write the man page this EU afternoon.

2 changes to address the review feedback: 1. Aleksei pointed out in this comment (Alexays#2971 (comment)) that there's no way to tell if a proxy is alive other than trying to call a method on it. We perform a little dance to check whether or not power-profiles-daemon is available on the system by calling properties.GetAll. If something responds, we assume power-profiles-daemon is installed, it's then safe to draw the widget and attach the callback to the active profile. 2. We replaced all the synchronous DBus operations by their async counterparts.

picnoir · 2024-03-01T14:21:56Z

The manpage is ready.

PTAL

alebastr

Thanks for addressing the DBus safety comments! Looks correct and works for me both with and without daemon.

I left a few more comments, mostly style nits and redundant code.

alebastr · 2024-03-01T13:50:13Z

src/modules/power_profiles_daemon.cpp

+// In the 80000 version of fmt library authors decided to optimize imports
+// and moved declarations required for fmt::dynamic_format_arg_store in new
+// header fmt/args.h
+#if (FMT_VERSION >= 80000)


Waybar requires fmt >= 8.1.1, this check is not necessary in a new code.

alebastr · 2024-03-01T13:55:59Z

src/modules/power_profiles_daemon.cpp

+  if (config_["format"].isString()) {
+    format_ = config_["format"].asString();
+  } else {
+    format_ = "{icon}";
+  }


ALabel::ALabel will do that for you if you pass the correct default format string to the constructor.

alebastr · 2024-03-01T13:56:47Z

include/modules/power_profiles_daemon.hpp

+  // Current CSS class applied to the label
+  std::string currentStyle_;
+  // Format strings
+  std::string labelFormat_;


labelFormat_ is unused

alebastr · 2024-03-01T14:01:46Z

src/modules/power_profiles_daemon.cpp

+  if (powerProfileChangeSignal_.connected()) {
+    powerProfileChangeSignal_.disconnect();
+  }


For any connections made with sigc::mem_fun where the object inherits sigc::trackable (base class for the modules does), this will happen automatically. You don't need to track or disconnect the connection.

alebastr · 2024-03-01T14:04:05Z

src/modules/power_profiles_daemon.cpp

+    powerProfileChangeSignal_.disconnect();
+  }
+  if (powerProfilesProxy_) {
+    powerProfilesProxy_.reset();


RefPtr already does that during destruction.

alebastr · 2024-03-01T14:27:25Z

src/modules/power_profiles_daemon.cpp

+}
+
+void PowerProfilesDaemon::setPropCb(Glib::RefPtr<Gio::AsyncResult>& r) {
+  auto _ = powerProfilesProxy_->call_finish(r);


Please, check for exceptions here.

alebastr · 2024-03-01T14:32:04Z

src/modules/power_profiles_daemon.cpp

+
+bool PowerProfilesDaemon::handleToggle(GdkEventButton* const& e) {
+  if (connected_) {
+    if (e->type == GdkEventType::GDK_BUTTON_PRESS && powerProfilesProxy_) {


The check for powerProfilesProxy_ could be redundant if you already check connected_

alebastr · 2024-03-01T14:37:01Z

src/modules/power_profiles_daemon.cpp

+  auto pred = [str](Profile const& p) { return p.name == str; };
+  this->activeProfile_ = std::find_if(availableProfiles_.begin(), availableProfiles_.end(), pred);
+  if (activeProfile_ == availableProfiles_.end()) {
+    throw std::runtime_error("FATAL, can't find the active profile in the available profiles list");


I'm not sure throwing exception is necessary here. It will be caught by Glibmm exception handler anyways, with no consequences.
You can just log the error and hide the module on the next update.

alebastr · 2024-03-01T14:38:26Z

src/modules/power_profiles_daemon.cpp

+      VarStr activeProfileVariant = VarStr::create(activeProfile_->name);
+      auto callArgs = SetPowerProfileVar::create(
+          std::make_tuple("net.hadess.PowerProfiles", "ActiveProfile", activeProfileVariant));
+      auto container = Glib::VariantBase::cast_dynamic<Glib::VariantContainerBase>(callArgs);


The manual cast might be unnecessary

alebastr · 2024-03-01T14:45:15Z

src/modules/power_profiles_daemon.cpp

+    profile = {name, driver};
+    availableProfiles_.push_back(profile);


availableProfiles_.emplace_back(name, driver); would avoid one temporary object.

And if you move name/driver declaration into the loop (which you might want to do anyways to avoid accidentally reusing values from previous iterations), it'll be safe to write if (!name.empty()) { availableProfiles_.emplace_back(std::move(name), std::move(driver)); }

picnoir · 2024-03-01T18:41:21Z

Thanks for having taken the time to write these detailed explanations. Learning a lot here :)

Addressed these "nits" in the above commit.

Don't hesitate to squash all these if you feel like it.

alebastr

Looks good. Thanks for implementing this feature!

include/modules/power_profiles_daemon.hpp

src/modules/power_profiles_daemon.cpp

picnoir · 2024-03-01T20:18:26Z

Loosely related note: there's a quite a few synchronous dbus calls throughout the codebase:

~/code-root/github.com/Alexays/Waybar (pic/power-profiles-daemon) » ag call_sync
src/util/portal.cpp
50:    response = call_sync(std::string(PORTAL_INTERFACE) + ".Read", params);

src/util/backlight_backend.cpp
223:  login_proxy_->call_sync("SetBrightness", call_args);

src/modules/systemd_failed_units.cpp
77:      Glib::VariantContainerBase data = proxy->call_sync("Get", parameters);

src/modules/gamemode.cpp
130:      Glib::VariantContainerBase data = gamemode_proxy->call_sync("Get", parameters);

~/code-root/github.com/Alexays/Waybar (pic/power-profiles-daemon) » ag bus_sync
src/util/backlight_backend.cpp
105:  login_proxy_ = Gio::DBus::Proxy::create_for_bus_sync(

src/modules/systemd_failed_units.cpp
31:    system_proxy = Gio::DBus::Proxy::create_for_bus_sync(
40:    user_proxy = Gio::DBus::Proxy::create_for_bus_sync(

src/modules/bluetooth.cpp
22:  GDBusObjectManager* manager = g_dbus_object_manager_client_new_for_bus_sync(
28:    spdlog::error("g_dbus_object_manager_client_new_for_bus_sync() failed: {}", error->message);

src/modules/gamemode.cpp
90:  gamemode_proxy = Gio::DBus::Proxy::create_for_bus_sync(Gio::DBus::BusType::BUS_TYPE_SESSION,

There was no way to display the default value of format-icons without breaking the table :(

picnoir · 2024-03-02T08:49:03Z

I just pushed a small visual fix. The fontawesome icons were not really centered in the box in the default config. I feel like there's something wrong with the calculation of the icon width in the typesetter code.

Added a bit of padding-left to center everything.

alebastr · 2024-03-02T16:34:40Z

You also accidentally reverted the last set of the review fixes :)

Would it make sense to use power-profiles-daemon (kebab-case) in the configuration? It's a bit awkward that the CSS element name differs from the config name.

alebastr · 2024-03-02T16:48:12Z

Loosely related note: there's a quite a few synchronous dbus calls throughout the codebase:

Yep, I did review all of those.

Systemd interfaces aren't affected by DBus activation issues and will not block.
Portal interface would break GTK if the DBus service is not available (all those "Waybar takes 25 seconds to start" issues in the tracker). So we already require a correct configuration for this one.
bluetooth and gamemode are off by default.

But here's a snippet that simulates a problem with gamemode service activation, blocks startup of waybar for the duration of a DBus timeout and then causes crash on invalid memory access. Fun.

% mkdir -p ~/.local/share/dbus-1/services
% cat >~/.local/share/dbus-1/services/com.feralinteractive.GameMode.service <<EOF
[D-BUS Service]
Name=com.feralinteractive.GameMode
Exec=/usr/bin/sleep 30
EOF

Adding : - A missing try/catch - Glib::Error catch - Remove the useless destructor - Populate the profiles vector more efficiently - Numerous nits

The icon is not really centered in the box. This is likely coming from a bogus glyph width calculation. It's not a big deal, but that's not really pleasant aesthetically-wise. Adding a bit of right padding makes it much more pleasant to watch. It does not really disrupt a wider display form, like one that explicitely writes the active profile.

picnoir · 2024-03-02T17:44:12Z

You also accidentally reverted the last set of the review fixes :)

🤦 fixed

Would it make sense to use power-profiles-daemon (kebab-case) in the configuration? It's a bit awkward that the CSS element name differs from the config name.

Yes, I also prefer that. The daemon itself is spelled using kebab cas.

I originally used snake-case seeing it used for idle_inhibitor, thinking it was the local style. But we actually already have some kebab-case names for some other modules (keyboard-state, systemd-failed-units).

power_profiles_daemon => power-profiles-daemon

Alexays · 2024-03-04T13:25:14Z

LGTM, can you also update the github wiki? :)
Thanks for this nice module.

picnoir · 2024-03-04T14:06:39Z

🎉

I created the module wiki page there: https://github.com/Alexays/Waybar/wiki/Module:-PowerProfilesDaemon

leiserfg · 2024-03-18T13:33:57Z

@picnoir thanks a lot for this feature! Would it make sense to make it so scrolling up and down cycles up and down? Or right-click cycles down (to keep the symmetry with the current behavior)?

picnoir · 2024-03-18T19:09:43Z

Hey! There's only 3 profiles available ATM. For my personal use case, toggling through them is good enough. If you feel like implementing this, you'll likely have to replace the availableProfiles_ vector with something that can be iterated in both directions (std::list?) See: https://github.com/Alexays/Waybar/blob/2c927de4c624d0c560f73ad78b4abc85fb703562/include/modules/power_profiles_daemon.hpp#L35 Once that done, you'll need to retrieve which button has been pressed in the handleToggle (https://github.com/Alexays/Waybar/blob/2c927de4c624d0c560f73ad78b4abc85fb703562/src/modules/power_profiles_daemon.cpp#L177) function. See the GdkEventButton documentation for more details https://api.gtkd.org/gdk.c.types.GdkEventButton.html Don't hesitate to ping me on Matrix/IRC or here if you're stuck and need a helping hand!

leiserfg · 2024-03-18T20:47:15Z

Changing the container is not required, std::vector has a bidirectional iterator as well, I will make a pr for it.

leiserfg · 2024-03-18T20:51:36Z

#3032

picnoir force-pushed the pic/power-profiles-daemon branch 2 times, most recently from f999681 to 9502940 Compare February 25, 2024 14:12

Alexays reviewed Feb 25, 2024

View reviewed changes

picnoir force-pushed the pic/power-profiles-daemon branch 2 times, most recently from 6cc362b to 7405011 Compare February 26, 2024 08:39

picnoir added 2 commits February 26, 2024 14:44

modules/power-profiles-daemon: run clang format

968f469

picnoir force-pushed the pic/power-profiles-daemon branch 3 times, most recently from 225f20f to f99f08b Compare February 26, 2024 14:23

modules/power-profiles-daemon: apply clang-tidy suggestions

162b41c

picnoir force-pushed the pic/power-profiles-daemon branch from f99f08b to 162b41c Compare February 29, 2024 20:47

modules/power_profiles_daemon: add custom format from config

61fed6a

We move to a single icon label format to save space on the bar. We still display the profile name and the driver in the tooltip.

picnoir force-pushed the pic/power-profiles-daemon branch 2 times, most recently from 29d6eaa to b620eb9 Compare March 1, 2024 14:11

alebastr reviewed Mar 1, 2024

View reviewed changes

alebastr approved these changes Mar 1, 2024

View reviewed changes

include/modules/power_profiles_daemon.hpp Outdated Show resolved Hide resolved

src/modules/power_profiles_daemon.cpp Show resolved Hide resolved

picnoir force-pushed the pic/power-profiles-daemon branch from 94b61c2 to ee197e4 Compare March 1, 2024 20:09

modules/power-profiles-daemon: add man page

bddc870

There was no way to display the default value of format-icons without breaking the table :(

picnoir force-pushed the pic/power-profiles-daemon branch from ee197e4 to 42f442e Compare March 2, 2024 08:47

picnoir added 2 commits March 2, 2024 18:20

Power profiles daemon: address review comments

cc759a8

Adding : - A missing try/catch - Glib::Error catch - Remove the useless destructor - Populate the profiles vector more efficiently - Numerous nits

picnoir force-pushed the pic/power-profiles-daemon branch from 42f442e to 0153031 Compare March 2, 2024 17:38

modules/power-profiles-daemon: kebab case name in config

5578c12

power_profiles_daemon => power-profiles-daemon

picnoir force-pushed the pic/power-profiles-daemon branch from 0153031 to 5578c12 Compare March 2, 2024 17:44

Alexays merged commit 3806075 into Alexays:master Mar 4, 2024

picnoir deleted the pic/power-profiles-daemon branch March 4, 2024 13:38

picnoir mentioned this pull request Mar 4, 2024

[Feature] Add module to configure PowerProfiles #1539

Closed

		profile = {name, driver};
		availableProfiles_.push_back(profile);

Introduce a power-profiles-daemon module #2971

Introduce a power-profiles-daemon module #2971

Conversation

picnoir commented Feb 25, 2024 • edited Loading

picnoir commented Feb 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnoir commented Feb 26, 2024 • edited Loading

Alexays commented Feb 27, 2024

picnoir commented Feb 27, 2024

lheckemann commented Feb 29, 2024 • edited Loading

alebastr commented Feb 29, 2024

alebastr commented Mar 1, 2024

picnoir commented Mar 1, 2024 • edited Loading

picnoir commented Mar 1, 2024

alebastr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnoir commented Mar 1, 2024 • edited Loading

alebastr left a comment

Choose a reason for hiding this comment

picnoir commented Mar 1, 2024

picnoir commented Mar 2, 2024 • edited Loading

alebastr commented Mar 2, 2024

alebastr commented Mar 2, 2024

picnoir commented Mar 2, 2024

Alexays commented Mar 4, 2024

picnoir commented Mar 4, 2024

leiserfg commented Mar 18, 2024

picnoir commented Mar 18, 2024 via email

leiserfg commented Mar 18, 2024

leiserfg commented Mar 18, 2024

picnoir commented Feb 25, 2024 •

edited

Loading

picnoir commented Feb 26, 2024 •

edited

Loading

lheckemann commented Feb 29, 2024 •

edited

Loading

picnoir commented Mar 1, 2024 •

edited

Loading

picnoir commented Mar 1, 2024 •

edited

Loading

picnoir commented Mar 2, 2024 •

edited

Loading