Support For Niri Workspaces #726
Conversation
src/clients/compositor/niri.rs
Outdated
| let mut conn = Connection::connect().await.unwrap(); | ||
|
|
||
| let command = Request::Action(Action::FocusWorkspace { | ||
| reference: WorkspaceReferenceArg::from_str(name.as_str()).unwrap(), |
There was a problem hiding this comment.
Since most workspaces don't have names, this should instead focus by Id (not Idx). There's WorkspaceReferenceArg::Id(u64).
There was a problem hiding this comment.
The focus method in the WorkspaceClient trait sends a name. So I would need some way to get the id from name. When converting from Niri Workspace struct to Ironbar Workspace struct, if the workspace has no name I set the id as name. Should I change the FromStr for WorkspaceReferenceArg to parse id instead of idx?
There was a problem hiding this comment.
It depends on what ironbar gives you, which I'm not familiar with. Index will work only if the correct monitor is focused when you send this request.
There was a problem hiding this comment.
I think it uses name because that was necessary for one of Sway or Hyprland. Using ID is definitely preferable. If necessary, I'd be okay splitting into a FocusById and FocusByName and letting each compositor impl pick the best case.
|
Appreciate the hard work here, big thanks for taking this on. Cheers YaLTeR for reviewing too, means a lot to see the actual dev provide input. I'm a little busy at the moment so responses from me won't be as fast as I like, but ping me as soon as you need anything from me and I'll look as soon as I can. I'll give it a full review as soon as you're ready. |
Cargo.toml
Outdated
| "workspaces+sway" = ["workspaces", "sway"] | ||
| "workspaces+hyprland" = ["workspaces", "hyprland"] | ||
| "workspaces+niri" = ["workspaces", "niri"] | ||
|
|
||
| niri = [] |
There was a problem hiding this comment.
Niri isn't required as its own feature here. Sway has its own because of the sway-mode module, and hyprland is the name of the Hyprland client crate.
src/clients/compositor/niri.rs
Outdated
| impl WorkspaceClient for Client { | ||
| fn focus(&self, name: String) -> Result<()> { | ||
| await_sync(async { | ||
| let mut conn = Connection::connect().await.unwrap(); |
There was a problem hiding this comment.
Can all unwraps be replaced by better error handling please. Generally, errors should be cleanly handled by either returning an Err or logging an error and cleanly returning. In rare cases where you absolutely do not expect it to fail, use expect and give a message of why it is expected to never fail.
src/clients/compositor/niri.rs
Outdated
| fn subscribe_workspace_change( | ||
| &self, | ||
| ) -> tokio::sync::broadcast::Receiver<super::WorkspaceUpdate> { | ||
| let (tx, rx) = channel(32); |
There was a problem hiding this comment.
I prefer to include part of the namespace when creating channels so it's immediatley obvious what kind:
| let (tx, rx) = channel(32); | |
| let (tx, rx) = broadcast::channel(32); |
src/clients/compositor/niri.rs
Outdated
| let event_niri = event_listener().unwrap(); | ||
| let events = match event_niri { | ||
| Event::WorkspacesChanged { workspaces } => { | ||
| // Niri only has a WorkspacesChanged Event and Ironbar has 4 events which have to be handled: Add, Remove, Rename and Move. The way I am handling this here is by keeping a previous state of workspaces and comparing with the new state for changes. To do this efficiently, first I sort the new workspace state based on id. Then I do a linear scan on the states(old and new) togethor. At the end, I over write the old workspace state with the new one. Because of this, on the next event, the old workspace state is already sorted. |
There was a problem hiding this comment.
Appreciate the comment but this is a very long line - can it be split across a few lines pls
src/clients/compositor/niri.rs
Outdated
| for event in events { | ||
| send!(tx, event); | ||
| } | ||
| std::thread::sleep(Duration::from_millis(30)); |
There was a problem hiding this comment.
As you are inside an async block, you should use Tokio's sleep to avoid blocking the thread.
src/clients/niri.rs
Outdated
There was a problem hiding this comment.
As this is all compositor-focused I would prefer if all this code was under the compositor module for now. Happy to have a niri module with a couple of files in there.
src/clients/niri.rs
Outdated
| request: Request, | ||
| ) -> io::Result<(Reply, impl FnMut() -> io::Result<Event> + '_)> { | ||
| let Self(stream) = self; | ||
| let mut buf = serde_json::to_string(&request).unwrap(); |
There was a problem hiding this comment.
In cases like this, if you return a color_eyre::Result you should be able to automatically use the ? operator despite mixing error types.
Hi, please let me know if I've missed something. Resolves #650.