greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.85k stars 470 forks source link

Conflict in the dbus signal reading process in kdeconnect block for battery and network refresh #1872

Closed Atreyagaurav closed 1 year ago

Atreyagaurav commented 1 year ago

There is a "refreshed" signal emitted from kdeconnect plugin for cell network, as well as for battery strength that kdeconnect block is using.

This signal is the one currently being used by the kdeconnect block to refresh the value for battery status:

dbus-monitor "type='signal',sender='org.kde.kdeconnect',interface='org.kde.kdeconnect.device.battery'"

The signal is emitted here:

https://invent.kde.org/network/kdeconnect-kde/-/blob/master/plugins/battery/batteryplugin.cpp#L124

There is a signal with the same name with a different interface, if you turn on the connectivity plugin you can get the signal:

dbus-monitor "type='signal',sender='org.kde.kdeconnect',interface='org.kde.kdeconnect.device.connectivity_report'"

This is emitted here:

https://invent.kde.org/network/kdeconnect-kde/-/blob/master/plugins/connectivity-report/connectivity_reportplugin.cpp#L54

So I wanted to add the connectivity status on the kdeconnect block of the i3status-rs as well. Then I ran into the problem due to the macro used to create the functions as it makes the function names and other things based on the signal name which makes these two have the same name.

+#[dbus_proxy(
+    interface = "org.kde.kdeconnect.device.connectivity_report",
+    default_service = "org.kde.kdeconnect"
+)]
+trait ConnectivityDbus {
+    // this here doesn't work as the macro uses the "refreshed" name for objects and ends up with duplicate names
+    #[dbus_proxy(signal, name = "refreshed")]
+    fn refreshed(&self, network_type: String, network_strength: i32) -> zbus::Result<()>;
+
+    #[dbus_proxy(property, name = "cellularNetworkStrength")]
+    fn cellular_network_strength(&self) -> zbus::Result<i32>;
+
+    #[dbus_proxy(property, name = "cellularNetworkType")]
+    fn cellular_network_type(&self) -> zbus::Result<String>;
+}

Full commit: https://github.com/greshake/i3status-rust/commit/d3b25de840dbd735a47ae14cf5dfd97cc36f5524

image

The image shows one error, there are a ton of error, all due to the conflicting name.

This commit here compiles but doesn't behave the way I'd want: https://github.com/greshake/i3status-rust/commit/98bcff42455cbeee8bf78155067d9425969bfa00

The second commit just changes the name, and it compiles, but obviously it misses the signal it's supposed to get. Furthermore, The network signal gets mixed up with the battery one, and the kdeconnect blocks shows really low battery (as the network strength value is 0..=4).

Originally, I was planning to make a pull request with the new feature, but now, I'm here to discuss how we should approach this. Should we make the functionality to catch the signal without depending on the macro? Should we request zbus to fix this conflict? Or something else? I'm very new to rust, so I thought I could just copy the functionality with battery and change it for network, but it's way complicated for me as it is.

bim9262 commented 1 year ago

You could create a new module in a new kdeconnect directoy:

$ tree src/blocks/kdeconnect 
src/blocks/kdeconnect
├── battery.rs
└── connectivity_report.rs

and then use the modules like this:

diff --git a/src/blocks/kdeconnect.rs b/src/blocks/kdeconnect.rs
index ee14688d..bd450bcf 100644
--- a/src/blocks/kdeconnect.rs
+++ b/src/blocks/kdeconnect.rs
@@ -50,6 +50,11 @@ use zbus::dbus_proxy;

 use super::prelude::*;

+mod battery;
+mod connectivity_report;
+use battery::BatteryDbusProxy;
+use connectivity_report::ConnectivityDbusProxy;
+
 #[derive(Deserialize, Debug, SmartDefault)]
 #[serde(default)]
 pub struct Config {
Atreyagaurav commented 1 year ago

@bim9262 Thank you so much. It did the job. I made a pull request.

With this:

format = " $icon {$bat_icon $bat_charge|}{ $notif_icon|} {$network_icon $network|}"

I get this block:

image

And when the network plugin is disabled in kdeconnect, it doesn't show the network blocks.