implement upower module

[p00f]

There are a couple of issues:

1. I want to use the popup to show additional details like time to empty/full and battery levels of other connected devices like bluetooth headphones. I've hardcoded the function for now but nothing happens when I click the module

2. upower will tell me what icon to use depending on battery level and whether it is plugged in, how do I implement this? I did format: "icon:{icon} {percentage}%" in config.yaml but that doesn't seem to work

Broadly it seems to work fine

[p00f]

@JakeStanger pinging you because I don't think you get notified of draft prs

[JakeStanger]

I do get notified actually :)

1. You need a gtk::Button inside the widget which sends a toggle command on click. See how I'm doing it here:
   

   ironbar/src/modules/clock.rs

   Lines 68 to 74 in d4dd8c4

   	let orientation = info.bar_position.get_orientation();
   	button.connect_clicked(move |button| {
   	try_send!(
   	context.tx,
   	ModuleUpdateEvent::TogglePopup(Popup::button_pos(button, orientation))
   	);
   	});

2. It looks like upower just returns the name and there's aren't actually attached to icons anywhere, but rather left to the implementer (that's us!). I would suggest adding a map into the config option like music does

   ironbar/src/modules/music/config.rs

   Lines 79 to 81 in d4dd8c4

   	/// Player state icons
   	#[serde(default)]
   	pub(crate) icons: Icons,

   and then using an icon label

   ironbar/src/modules/music/mod.rs

   Line 454 in d4dd8c4

   let icon = new_icon_label(icon_input, icon_theme, 32);

   to let users choose any text/gtk icon/image they want. You can then pick some sensible defaults from nerd fonts. This implementation does mean you cannot add the icon to the formatting, and instead it will need to be hard-coded to the start/end, but I think that's fine.

I appreciate that's fairly involved, so give me a heads up if you get stuck - super appreciate you taking this on

[p00f]

The name of the icon is an actual icon in adwaita so I wanted to just pick that icon instead of making it configurable, this is what waybar does as well

[JakeStanger]

Ah okay, that's handy. I think most of the above still applies. You can use new_icon_label and pass it the icon name prefixed with icon: (so like icon:battery-good-charging-symbolic) and it will automatically resolve the image from the icon theme for you. That also means it could be configurable to override the theme default in the future.

[yavko]

Is there anything blocking the merge of this? If help is needed, I can help out. I would love to see this in ironbar!

[p00f]

@yavko I can't get icons to work

[JakeStanger]

Feel free to push your latest version up (might be wise to rebase onto master to get the latest changes) and I can have a look :)

[p00f]

The conflict is in Cargo.lock which I'm not sure how to handle, I haven't worked on this after the last push

[JakeStanger]

So long as the cargo.toml is fine, you can do whatever with the merge, then just delete cargo.lock and regen to ensure it generates a new valid lock.

I'm not sure if that's necessarily best practise but it's the easy way lol - I'll just double check it when we get to review stage.

[p00f]

@JakeStanger done

[p00f]

oh sorry it doesn't compile

[p00f]

fixed it

[JakeStanger]

Now I'm looking at it, using new_icon_label here is a bit more fiddly than I would have liked, so might not be the best path. Luckily it's fairly simple so we can replicate the needed behaviour in a few lines.

What you should able to do here instead is create an image widget and add it to your container:

let icon = gtk::Image::new();
// ...set up/add widget...

Then inside your callback where you get the icon name, you can pass it (still in the icon:name format) to ImageProvider::parse to load the new icon into your image widget. This will do all the work for you, and retains support for images from other sources should we want it in the future.

if let Err(err) = ImageProvider::parse(input, icon_theme, size)
            .and_then(|provider| provider.load_into_image(icon) {
    error!("{err:?}");
}

I'd advise caching the previous icon name to avoid re-loading the same image over and over, but we can get to bits like that when you're ready for a full review. Didn't see you published the PR, I'll get to this soon!

[JakeStanger]

Thanks for all the work so far, it's really appreciated. This is looking generally very good so far :)

Most of the bits I've raised are trivial pedantic bits, but there's a couple of more complex issues so do let me know if you need a hand with any of those.

The build is failing currently due to compiler errors when --no-default-features is set. One of the reasons is because regex is behind a feature flag, but it looks like that's not actually needed here anyway. The other is due to the incomplete icon code, so that should resolve itself in time.

[p00f]

the icon is wrong:

[JakeStanger]

I don't have a device with a battery that I can test this against, but on my desktop the correct battery-missing-symbolic icon is showing. It's probably worth trying with different icon themes and see if that helps.

[JakeStanger]

A few more little bits, but looking like it's nearly there! Let me know when you've got these, and the few changes leftover from yesterday sorted, thanks!

[p00f]

should i retry if connecting to the system bus (or anything in the dbus initialization portion) fails?

[p00f]

the icon was fine, it was just black on black.

[JakeStanger]

should i retry if connecting to the system bus (or anything in the dbus initialization portion) fails?

I don't think so. I think if DBus ever fails to connect, that's a much greater problem that retrying probably won't fix, so the best action is probably to just log the error and exit (expect is probably fine in that case)

[JakeStanger]

Please replace this unwrap

[JakeStanger]

Couple of tiny bits left, then I think it's just about there :)

[yavko]

I just wanted to comment, I see you are using the icon from upower, which makes sense, but honestly, I think the icon it gives is pretty misleading sometimes, I know this is extra code, but it provides a more accurate icon, in case you want to see how I did it in the past. Not sure if this icon is in all icon packs, but after checking seems it's in papirus and adwaita, and breeze too, it just has a leading zero on all the numbers, so actually maybe this isn't the best idea, as it'd have to be checked at runtime. (also, this was written with an older version of upower_dbus, so I don't know if BatteryState still exists, or works the same)

fn get_icon_name(status: BatteryState, capacity: u64) -> String {
    let suffix = if status == BatteryState::Charging {
        "-charging"
    } else {
        ""
    };

    let cap: u8 = match capacity {
        0..=9 => 0,
        10..=19 => 10,
        20..=29 => 20,
        30..=39 => 30,
        40..=49 => 40,
        50..=59 => 50,
        60..=69 => 60,
        70..=79 => 70,
        80..=89 => 80,
        90..=99 => 90,
        100 => 100,
        _ => 0,
    };

    format!("battery-level-{cap}{suffix}-symbolic")
}

[JakeStanger]

@p00f Thanks for all your work. I'm gonna complete the last few bits & add the docs, then get this merged.

@yavko That sounds good to me. I'll merge the current implementation for now, but if you want to follow up with a PR that adds your icon method, falling back to the upower icon if it's not present in the theme, that would be fantastic (if not that's cool too ofc, I'll add it to the todo list)