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. |
| await_sync(async { | ||
| if let Ok(mut conn) = Connection::connect().await { | ||
| let command = Request::Action(Action::FocusWorkspace { | ||
| reference: WorkspaceReferenceArg::from_str(name.as_str()).unwrap(), |
There was a problem hiding this comment.
Please can this unwrap be changed. By the looks of it this is infallible so just need to say it should always be valid in an expect.
| for event in events { | ||
| send!(tx, event); | ||
| } |
There was a problem hiding this comment.
Is there a risk of filling up the channel buffer here by sending too many events at once? Some stress testing may be required to check. If it is a potential problem, a 'batch' event might solve it.
|
Hi there! Thanks for the work, this PR is exactly what I need. Is there anything I can do to help move it forward? |
|
Hey, the following are outstanding:
If you're able to offer a hand with any of the above at all, that'd be great. The plan is to ship 0.16.1 this weekend, assuming the tray doesn't find new ways to break before then. Once that's out, I'll merge this as soon as it's ready. |
|
There is an issue. If I remove a workspace from the middle, somehow when I move to another workspace, the focused workspace on the bar does not change. I am unable to pin point why this is happening. I also do not have multiple monitors, so it would be helpful if anyone could test for that. |
|
I'd love to tackle these issues, but I don't know the best way to do it on a PR that comes from a fork that I don't own. And for the comment about stress testing, I'm also not sure how to go about this. |
|
You may be able to open a PR on the fork, targeting the branch used for this PR. If that gets merged, it'll update here. If not you can just send a patch here. Spamming the Niri IPC (aka a while true loop) with workspace changes and ensuring Ironbar can keep up is sufficient for stress testing. |
Hi, please let me know if I've missed something. Resolves #650.