Add a DBus object per output #9

Givralix · 2023-10-25T06:44:48Z

This pull request fixes #3 by adding the --output-name argument to specify output names for which DBus objects are created.

With wl-gammarelay-rs --output-name eDP-1 --output-name HDMI-A-1:

$ busctl --user tree rs.wl-gammarelay
└─ /Output
  ├─ /Output/HDMI_A_1
  └─ /Output/eDP_1

With wl-gammarelay-rs:

$ busctl --user tree rs.wl-gammarelay
Only root object discovered.

When specifying output names, the root object is not declared to avoid keeping two different states for each screen.

MaxVerevkin · 2023-10-25T07:51:52Z

Hey, thanks for implementing this!

I like the idea of creating an object per output, I didn't event think about this :) However I would prefer to keep the root object. Maybe it can represent an "averaged" output?

src/wayland.rs

Givralix · 2023-10-25T11:31:31Z

Hey, thanks for implementing this!

I like the idea of creating an object per output, I didn't event think about this :) However I would prefer to keep the root object. Maybe it can represent an "averaged" output?

I'd like to implement that, but I'm not sure about the details. I'll come back to it later.

MaxVerevkin · 2023-10-25T13:17:46Z

With wl-gammarelay-rs --output-name eDP-1 --output-name HDMI-A-1:
$ busctl --user tree rs.wl-gammarelay
└─ /Output
  ├─ /Output/HDMI_A_1
  └─ /Output/eDP_1
With wl-gammarelay-rs:
$ busctl --user tree rs.wl-gammarelay
Only root object discovered.

What do you think about creating objects for each output in all cases and using the CLI args as a filter?

src/dbus_server.rs

src/wayland.rs

Givralix · 2024-01-01T09:03:29Z

With wl-gammarelay-rs --output-name eDP-1 --output-name HDMI-A-1:
$ busctl --user tree rs.wl-gammarelay
└─ /Output
  ├─ /Output/HDMI_A_1
  └─ /Output/eDP_1
With wl-gammarelay-rs:
$ busctl --user tree rs.wl-gammarelay
Only root object discovered.
What do you think about creating objects for each output in all cases and using the CLI args as a filter?

I tried to do that, but I can't find a way to list the outputs with their names. It's impossible to send the names elsewhere from wl_output_cb using a channel because the callback isn't async, and creating a new object directly with zbus::ObjectServer::at is also async.

Do you know if there's another way to do this?

MaxVerevkin · 2024-01-01T09:59:08Z

It's impossible to send the names elsewhere from wl_output_cb using a channel because the callback isn't async

You can use tokio's unbounded channel, in which case the sender will be sync and never block.

MaxVerevkin · 2024-01-01T11:19:39Z

We need to decide on what happens when output settings deviate. What does the root node represent in this case?
The decision should be documented somewhere in the readme.
Hot-unplugging does not work at the moment (dbus objects are never removed).

MaxVerevkin · 2024-01-01T11:23:32Z

By the way, since callbacks have a mutable reference to the state, it is not necessary to use channels, a VecDeque is enough. You can check if it is not empty after conn.dispatch_events() call.

Givralix · 2024-01-01T11:28:19Z

We need to decide on what happens when output settings deviate. What does the root node represent in this case?

The decision should be documented somewhere in the readme.

Hot-unplugging does not work at the moment (dbus objects are never removed).

I'm working on a root node right now. It shows the average value of each setting for all the outputs (for inverted, it shows true only if all the monitors are inverted). When changing a value, it applies the modification to each output individually (except for ToggleInverted which sets inverted to false only if all the monitors are inverted). Let me know if I should do it another way.
I'll add that too.
I'll fix this.

By the way, since callbacks have a mutable reference to the state, it is not necessary to use channels, a VecDeque is enough. You can check if it is not empty after conn.dispatch_events() call.

Nice!

MaxVerevkin · 2024-01-01T11:41:12Z

I'm working on a root node right now. It shows the average value of each setting for all the outputs (for inverted, it shows true only if all the monitors are inverted). When changing a value, it applies the modification to each output individually (except for ToggleInverted which sets inverted to false only if all the monitors are inverted). Let me know if I should do it another way.

This is also how I imagined it at first.

Another option is to keep it simple and leave it as it is right now, with a small change to set the "global value" only when changing the root node:

diff --git a/src/wayland.rs b/src/wayland.rs
index 576ae2e..7a4842c 100644
--- a/src/wayland.rs
+++ b/src/wayland.rs
@@ -60,7 +60,9 @@ pub async fn run(
             }
             Some(request) = rx.recv() => {
                 let Request::SetColor { color, output_name } = request;
-                state.color = color;
+                if output_name.is_none() {
+                    state.color = color;
+                }
                 state
                     .outputs
                     .iter_mut()

Yet another option is to treat the root node as the "default" value. When an output property is set, this specific property of this specific output is marked as "custom", and is no longer affected by the "default" value. All new/hot-plugged outputs are treated as fully "default". Modifying the root node will update all "non-custom" properties of all outputs.

Givralix · 2024-01-02T08:01:03Z

I'm working on a root node right now. It shows the average value of each setting for all the outputs (for inverted, it shows true only if all the monitors are inverted). When changing a value, it applies the modification to each output individually (except for ToggleInverted which sets inverted to false only if all the monitors are inverted). Let me know if I should do it another way.

This is also how I imagined it at first.

Another option is to keep it simple and leave it as it is right now, with a small change to set the "global value" only when changing the root node:
diff --git a/src/wayland.rs b/src/wayland.rs
index 576ae2e..7a4842c 100644
--- a/src/wayland.rs
+++ b/src/wayland.rs
@@ -60,7 +60,9 @@ pub async fn run(
             }
             Some(request) = rx.recv() => {
                 let Request::SetColor { color, output_name } = request;
-                state.color = color;
+                if output_name.is_none() {
+                    state.color = color;
+                }
                 state
                     .outputs
                     .iter_mut()
Yet another option is to treat the root node as the "default" value. When an output property is set, this specific property of this specific output is marked as "custom", and is no longer affected by the "default" value. All new/hot-plugged outputs are treated as fully "default". Modifying the root node will update all "non-custom" properties of all outputs.

I implemented the root node the way I described earlier, added a description of how it works to the README and fixed hot unplugging.

MaxVerevkin · 2024-01-02T10:22:39Z

Updating an individual output should trigger "PropertiesChanged" signal to notify that the average has updated. Similarly, updating the root node should send notifications for each output.

Also its annoying that even with a single threaded executor we need all these Arc<Mutex<...>>es everywhere. I'll open an issue in zbus, maybe this can be improved.

Givralix · 2024-01-02T12:29:29Z

Updating an individual output should trigger "PropertiesChanged" signal to notify that the average has updated. Similarly, updating the root node should send notifications for each output.

I can't do that because the RootServer contains a list of dbus_server::Outputs, not Servers, and it can't store a list of Servers because zbus takes ownership of the interface when you define an object.

MaxVerevkin · 2024-01-02T15:35:10Z

Server can be made cheaply clone-able by deriving Clone, so you can store a list of Servers.

MaxVerevkin · 2024-01-02T15:37:46Z

And you can manually construct and store SignalContext in each Server.

MaxVerevkin · 2024-02-15T10:57:42Z

FYI: I'm planning to migrate to rustbus for dbus, this would remove the need for any interior mutability.

Givralix · 2024-03-03T11:00:47Z

FYI: I'm planning to migrate to rustbus for dbus, this would remove the need for any interior mutability.

I'm having trouble with interior mutability so I'll wait for the migration to redo this.

MaxVerevkin · 2024-03-03T20:26:10Z

Aaaand, done!

Givralix · 2024-03-17T07:44:23Z

I updated to use rustbus. I still need to handle two things:

Removing an object from DBus (is there a way to do this with rustbus-service?)
Signaling to all outputs when a property is changed on the root node.

MaxVerevkin · 2024-03-17T07:54:41Z

(is there a way to do this with rustbus-service?)

Nope, I didn't think about this :) Will add Object::remove_child(&mut self, path: &str) -> Option<Self>. Edit: done.

By the way, in theory there should be no need for Rcs and RefCells, since all callbacks receive a mutable reference to the state. Did you encounter any problems?

Givralix · 2024-03-17T08:21:49Z

(is there a way to do this with rustbus-service?)

Nope, I didn't think about this :) Will add Object::remove_child(&mut self, path: &str) -> Option<Self>. Edit: done.

Thanks!

By the way, in theory there should be no need for Rcs and RefCells, since all callbacks receive a mutable reference to the state. Did you encounter any problems?

In DbusServer::add_output, if I changed output to be a &mut Output, then the closures are FnMut instead of Fn and cannot be used in the interface.

For the DbusServer, I need access to it in wayland callbacks so I can tell it about new outputs, and at the same time in main() to poll it.

MaxVerevkin · 2024-03-17T08:34:08Z

In the callbacks, you can get &mut Output by searching through ctx.state.outputs, since you have ctx.object_path, similar to how wayland callbacks do it.

MaxVerevkin · 2024-03-17T08:47:37Z

Regarding DbusServer, I could come up with something like

diff --git a/src/main.rs b/src/main.rs
index 866cd8d..ada8f02 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -8,7 +8,6 @@ use std::os::fd::AsRawFd;
 use std::{io, rc::Rc};
 
 use clap::{Parser, Subcommand};
-use dbus_server::DbusServer;
 use wayrs_protocols::wlr_gamma_control_unstable_v1::ZwlrGammaControlManagerV1;
 
 use color::Color;
@@ -31,7 +30,6 @@ enum Command {
 struct State {
     outputs: Vec<Rc<RefCell<wayland::Output>>>,
     gamma_manager: ZwlrGammaControlManagerV1,
-    dbus_server: Rc<RefCell<DbusServer>>,
 }
 
 impl State {
@@ -161,22 +159,18 @@ fn main() -> anyhow::Result<()> {
 
     match command {
         Command::Run => {
-            if let Some(dbus_server) = dbus_server {
-                let dbus_server = Rc::new(RefCell::new(dbus_server));
-                let (mut wayland, mut state) = wayland::Wayland::new(dbus_server.clone())?;
+            if let Some(mut dbus_server) = dbus_server {
+                let (mut wayland, mut state) = wayland::Wayland::new()?;
 
-                let mut fds = {
-                    let dbus_server = dbus_server.borrow().as_raw_fd();
-                    [pollin(&dbus_server), pollin(&wayland)]
-                };
+                let mut fds = [pollin(&dbus_server), pollin(&wayland)];
 
                 loop {
                     poll(&mut fds)?;
                     if fds[0].revents != 0 {
-                        dbus_server.borrow_mut().poll(&mut state)?;
+                        dbus_server.poll(&mut state)?;
                     }
                     if fds[1].revents != 0 || state.color_changed() {
-                        wayland.poll(&mut state)?;
+                        (state, dbus_server) = wayland.poll(state, dbus_server)?;
                     }
                 }
             } else {
@@ -185,22 +179,18 @@ fn main() -> anyhow::Result<()> {
         }
         Command::Watch { format } => {
             let mut dbus_client = dbus_client::DbusClient::new(format, dbus_server.is_none())?;
-            if let Some(dbus_server) = dbus_server {
-                let dbus_server = Rc::new(RefCell::new(dbus_server));
-                let (mut wayland, mut state) = wayland::Wayland::new(dbus_server.clone())?;
+            if let Some(mut dbus_server) = dbus_server {
+                let (mut wayland, mut state) = wayland::Wayland::new()?;
 
-                let mut fds = {
-                    let dbus_server = dbus_server.borrow().as_raw_fd();
-                    [pollin(&dbus_server), pollin(&wayland), pollin(&dbus_client)]
-                };
+                let mut fds = [pollin(&dbus_server), pollin(&wayland), pollin(&dbus_client)];
 
                 loop {
                     poll(&mut fds)?;
                     if fds[0].revents != 0 {
-                        dbus_server.borrow_mut().poll(&mut state)?;
+                        dbus_server.poll(&mut state)?;
                     }
                     if fds[1].revents != 0 || state.color_changed() {
-                        wayland.poll(&mut state)?;
+                        (state, dbus_server) = wayland.poll(state, dbus_server)?;
                     }
                     if fds[2].revents != 0 {
                         dbus_client.run(false)?;
diff --git a/src/wayland.rs b/src/wayland.rs
index dc51335..aad70f7 100644
--- a/src/wayland.rs
+++ b/src/wayland.rs
@@ -19,7 +19,7 @@ use crate::dbus_server::DbusServer;
 use crate::State;
 
 pub struct Wayland {
-    conn: Connection<State>,
+    conn: Connection<(State, DbusServer)>,
 }
 
 impl AsRawFd for Wayland {
@@ -29,7 +29,7 @@ impl AsRawFd for Wayland {
 }
 
 impl Wayland {
-    pub fn new(dbus_server: Rc<RefCell<DbusServer>>) -> Result<(Self, State)> {
+    pub fn new() -> Result<(Self, State)> {
         let (mut conn, globals) = Connection::connect_and_collect_globals()?;
         conn.add_registry_cb(wl_registry_cb);
 
@@ -44,7 +44,6 @@ impl Wayland {
         let state = State {
             outputs,
             gamma_manager,
-            dbus_server,
         };
 
         conn.flush(IoMode::Blocking)?;
@@ -52,21 +51,22 @@ impl Wayland {
         Ok((Self { conn }, state))
     }
 
-    pub fn poll(&mut self, state: &mut State) -> Result<()> {
+    pub fn poll(&mut self, state: State, dbus_server: DbusServer) -> Result<(State, DbusServer)> {
+        let mut state = (state, dbus_server);
         match self.conn.recv_events(IoMode::NonBlocking) {
-            Ok(()) => self.conn.dispatch_events(state),
+            Ok(()) => self.conn.dispatch_events(&mut state),
             Err(e) if e.kind() == ErrorKind::WouldBlock => (),
             Err(e) => return Err(e.into()),
         }
 
-        for output in &state.outputs {
+        for output in &state.0.outputs {
             let mut output = output.borrow_mut();
             if output.color_changed {
                 output.update_displayed_color(&mut self.conn)?;
             }
         }
         self.conn.flush(IoMode::Blocking)?;
-        Ok(())
+        Ok((state.0, state.1))
     }
 }
 
@@ -83,7 +83,7 @@ pub struct Output {
 
 impl Output {
     fn bind(
-        conn: &mut Connection<State>,
+        conn: &mut Connection<(State, DbusServer)>,
         global: &Global,
         gamma_manager: ZwlrGammaControlManagerV1,
     ) -> Self {
@@ -100,7 +100,7 @@ impl Output {
         }
     }
 
-    fn destroy(self, conn: &mut Connection<State>) {
+    fn destroy(self, conn: &mut Connection<(State, DbusServer)>) {
         eprintln!("Output {} removed", self.reg_name);
         self.gamma_control.destroy(conn);
         if self.wl.version() >= 3 {
@@ -133,7 +133,7 @@ impl Output {
         self.color_changed = true;
     }
 
-    fn update_displayed_color(&mut self, conn: &mut Connection<State>) -> Result<()> {
+    fn update_displayed_color(&mut self, conn: &mut Connection<(State, DbusServer)>) -> Result<()> {
         if self.ramp_size == 0 {
             return Ok(());
         }
@@ -152,7 +152,12 @@ impl Output {
     }
 }
 
-fn wl_registry_cb(conn: &mut Connection<State>, state: &mut State, event: &wl_registry::Event) {
+fn wl_registry_cb(
+    conn: &mut Connection<(State, DbusServer)>,
+    state: &mut (State, DbusServer),
+    event: &wl_registry::Event,
+) {
+    let (state, dbus_server) = state;
     match event {
         wl_registry::Event::Global(global) if global.is::<WlOutput>() => {
             let mut output = Output::bind(conn, global, state.gamma_manager);
@@ -167,7 +172,7 @@ fn wl_registry_cb(conn: &mut Connection<State>, state: &mut State, event: &wl_re
                 .position(|o| o.borrow().reg_name == *name)
             {
                 if let Some(output_name) = &state.outputs[output_index].borrow().name {
-                    state.dbus_server.borrow_mut().remove_output(&output_name);
+                    dbus_server.remove_output(&output_name);
                 }
                 let output = state.outputs.swap_remove(output_index);
                 Rc::into_inner(output).unwrap().into_inner().destroy(conn);
@@ -177,31 +182,29 @@ fn wl_registry_cb(conn: &mut Connection<State>, state: &mut State, event: &wl_re
     }
 }
 
-fn gamma_control_cb(ctx: EventCtx<State, ZwlrGammaControlV1>) {
+fn gamma_control_cb(ctx: EventCtx<(State, DbusServer), ZwlrGammaControlV1>) {
     let output_index = ctx
         .state
+        .0
         .outputs
         .iter()
         .position(|o| o.borrow().gamma_control == ctx.proxy)
         .expect("Received event for unknown output");
     match ctx.event {
         zwlr_gamma_control_v1::Event::GammaSize(size) => {
-            let mut output = ctx.state.outputs[output_index].borrow_mut();
+            let mut output = ctx.state.0.outputs[output_index].borrow_mut();
             eprintln!("Output {}: ramp_size = {}", output.reg_name, size);
             output.ramp_size = size as usize;
             output.update_displayed_color(ctx.conn).unwrap();
         }
         zwlr_gamma_control_v1::Event::Failed => {
-            let output = ctx.state.outputs.swap_remove(output_index);
+            let output = ctx.state.0.outputs.swap_remove(output_index);
             eprintln!(
                 "Output {}: gamma_control::Event::Failed",
                 output.borrow().reg_name
             );
             if let Some(output_name) = &output.borrow().name {
-                ctx.state
-                    .dbus_server
-                    .borrow_mut()
-                    .remove_output(&output_name);
+                ctx.state.1.remove_output(&output_name);
             }
             Rc::into_inner(output)
                 .unwrap()
@@ -212,10 +215,11 @@ fn gamma_control_cb(ctx: EventCtx<State, ZwlrGammaControlV1>) {
     }
 }
 
-fn wl_output_cb(ctx: EventCtx<State, WlOutput>) {
+fn wl_output_cb(ctx: EventCtx<(State, DbusServer), WlOutput>) {
     if let wl_output::Event::Name(name) = ctx.event {
         let output = ctx
             .state
+            .0
             .outputs
             .iter()
             .find(|o| o.borrow().wl == ctx.proxy)
@@ -223,9 +227,6 @@ fn wl_output_cb(ctx: EventCtx<State, WlOutput>) {
         let name = String::from_utf8(name.into_bytes()).expect("invalid output name");
         eprintln!("Output {}: name = {name:?}", output.borrow().reg_name);
         output.borrow_mut().name = Some(name);
-        ctx.state
-            .dbus_server
-            .borrow_mut()
-            .add_output(output.clone());
+        ctx.state.1.add_output(output.clone());
     }
 }

Although (State, DbusServer) would be better represented as a struct.

Givralix · 2024-03-17T17:32:12Z

This looks ok to me but I'll test more tomorrow.

MaxVerevkin · 2024-03-18T06:07:59Z

Looks good to me too, thanks for working on this!

MaxVerevkin reviewed Oct 25, 2023

View reviewed changes

src/wayland.rs Outdated Show resolved Hide resolved

MaxVerevkin reviewed Oct 25, 2023

View reviewed changes

src/dbus_server.rs Outdated Show resolved Hide resolved

MaxVerevkin reviewed Oct 25, 2023

View reviewed changes

src/wayland.rs Outdated Show resolved Hide resolved

MaxVerevkin reviewed Oct 25, 2023

View reviewed changes

src/wayland.rs Outdated Show resolved Hide resolved

Givralix changed the title ~~Add CLI option for selecting a specific output~~ WIP: Add CLI option for selecting a specific output Jan 1, 2024

Givralix changed the title ~~WIP: Add CLI option for selecting a specific output~~ Add a DBus object per output Jan 1, 2024

Givralix added 7 commits January 1, 2024 22:06

add CLI option for selecting a specific output

6e6d4ce

cleanup

3a27059

change /Output/ into /outputs/ and cleanup

a319a53

automatically detect outputs

67768e8

put new output names in a Vec

a0456df

add a root server that averages the values of all outputs

58c2496

delete objects when outputs are disconnected (to test)

07a335b

Givralix force-pushed the main branch from de6fe07 to 07a335b Compare January 1, 2024 21:07

Givralix added 5 commits March 9, 2024 07:54

base for multi-output support

d7d2d3f

fix root node behavior

6567665

attempt

e6fc8ec

multi-output support

3932646

Merge branch 'rustbus'

07a236d

Givralix added 2 commits March 17, 2024 10:00

moved wayland::Output out of Rc

0c43771

moved DbusServer out of Rc

6c2b725

Givralix force-pushed the main branch from 5d3c347 to 6c2b725 Compare March 17, 2024 09:23

Givralix added 2 commits March 17, 2024 12:51

support removing an output and having no outputs

79b2637

signal relevant outputs when the root node is modified

608e02e

MaxVerevkin merged commit 18da442 into MaxVerevkin:main Mar 18, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a DBus object per output #9

Add a DBus object per output #9

Givralix commented Oct 25, 2023

MaxVerevkin commented Oct 25, 2023

Givralix commented Oct 25, 2023

MaxVerevkin commented Oct 25, 2023

Givralix commented Jan 1, 2024

MaxVerevkin commented Jan 1, 2024 •

edited

Loading

MaxVerevkin commented Jan 1, 2024

MaxVerevkin commented Jan 1, 2024

Givralix commented Jan 1, 2024

MaxVerevkin commented Jan 1, 2024

Givralix commented Jan 2, 2024

MaxVerevkin commented Jan 2, 2024

Givralix commented Jan 2, 2024

MaxVerevkin commented Jan 2, 2024

MaxVerevkin commented Jan 2, 2024

MaxVerevkin commented Feb 15, 2024

Givralix commented Mar 3, 2024

MaxVerevkin commented Mar 3, 2024

Givralix commented Mar 17, 2024 •

edited

Loading

MaxVerevkin commented Mar 17, 2024 •

edited

Loading

Givralix commented Mar 17, 2024

MaxVerevkin commented Mar 17, 2024

MaxVerevkin commented Mar 17, 2024

Givralix commented Mar 17, 2024

MaxVerevkin commented Mar 18, 2024

Add a DBus object per output #9

Add a DBus object per output #9

Conversation

Givralix commented Oct 25, 2023

MaxVerevkin commented Oct 25, 2023

Givralix commented Oct 25, 2023

MaxVerevkin commented Oct 25, 2023

Givralix commented Jan 1, 2024

MaxVerevkin commented Jan 1, 2024 • edited Loading

MaxVerevkin commented Jan 1, 2024

MaxVerevkin commented Jan 1, 2024

Givralix commented Jan 1, 2024

MaxVerevkin commented Jan 1, 2024

Givralix commented Jan 2, 2024

MaxVerevkin commented Jan 2, 2024

Givralix commented Jan 2, 2024

MaxVerevkin commented Jan 2, 2024

MaxVerevkin commented Jan 2, 2024

MaxVerevkin commented Feb 15, 2024

Givralix commented Mar 3, 2024

MaxVerevkin commented Mar 3, 2024

Givralix commented Mar 17, 2024 • edited Loading

MaxVerevkin commented Mar 17, 2024 • edited Loading

Givralix commented Mar 17, 2024

MaxVerevkin commented Mar 17, 2024

MaxVerevkin commented Mar 17, 2024

Givralix commented Mar 17, 2024

MaxVerevkin commented Mar 18, 2024

MaxVerevkin commented Jan 1, 2024 •

edited

Loading

Givralix commented Mar 17, 2024 •

edited

Loading

MaxVerevkin commented Mar 17, 2024 •

edited

Loading