Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sway support #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sway support #122

wants to merge 1 commit into from

Conversation

markstos
Copy link

@markstos markstos commented Jan 5, 2022

Adds sway support.

It uses the "swayipc" library, which supports both Sway and i3.

@markstos markstos marked this pull request as draft January 5, 2022 02:44
Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'd really like to support sway. How to we get stuff drawn on wayland though? In my understanding, this is only the control logic and not the drawing logic.

Cargo.toml Outdated Show resolved Hide resolved
src/wm_i3.rs Show resolved Hide resolved
@markstos
Copy link
Author

markstos commented Jan 8, 2022

Instead of using X-specific code, use winit, a cross-platform Rust library which supports both X and Wayland by default.

https://github.com/rust-windowing/winit

@svenstaro
Copy link
Owner

svenstaro commented Jan 10, 2022

@markstos When I initially implemented this, I tried using winit first but I concluded it didn't have exactly the capabilities required. If you like, you can try to take a stab at this in order to make it work using winit which is definitely preferable. Maybe the situation has changed by now.

@markstos
Copy link
Author

@svenstaro Although I thought I could read Rust well enough to do this rebase, I haven't started doing Rust development yet and wouldn't be able to get to this soon, but maybe @JayceFayne could take a look. If you are able to recall which features it was missing at the time, perhaps we can check the Changelog to see if they have been added since then.

@svenstaro
Copy link
Owner

I honestly don't remember, it was a long time ago. I guess the most efficient way is to just attempt it and see how far you can get. I suppose this is actually a fairly nice way to learn Rust as the project is rather small and it's also a thing you'd supposedly use yourself. I'm happy to do reviews.

@markstos
Copy link
Author

I'm interested to learn Rust, but this my first day back from a work vacation so I expect to stay busy catching up with things for a while. I'll keep this in mind as a first Rust project!

swayipc which supports both Sway and i3
@markstos
Copy link
Author

@svenstaro I have successfully rebased the Sway support off of the main branch, resolved all the conflicts and tested that it works with Sway. 🎉

I can say this about the if node.window.is_some() { line: The Sway branch seems to work fine without the conditional. However, I tried adding it back and on the Sway branch, and wmfocus does not work at all with that conditional included. It hangs.

If you could re-test with the Sway support didn't break the i3 support, I think this is ready for release.

Note, this branch also now depends on the swayipc 3.0.0 package instead of the Git repo.

Yes, the windows are still being drawn with X11, but 99% of of Wayland users are also using XWayland and won't notice or care about the difference. This is still a good step towards impoved Sway and Wayland support. Releasing this milestone may help attract the next Rust+Wayland developer to take on the "winit" refactor or otherwise add native Wayland drawing support.

@markstos markstos marked this pull request as ready for review January 15, 2022 03:18
@markstos markstos mentioned this pull request Jan 19, 2022
@svenstaro
Copy link
Owner

svenstaro commented Feb 6, 2022

Running this, I'm getting thread 'main' panicked at 'Problem communicating with IPC: SerdeJson(Error("missing field layout", line: 1, column: 153))', src/wm_i3.rs:101:10 on i3.

Also sorry for the inactivity, I'd really like to get this merged soon! If you have some time to work on it, I'll provide quick feedback from now on.

@markstos
Copy link
Author

markstos commented Feb 7, 2022

@svenstaro Are you running it with Sway/Wayland or i3/X11 when you get that error? I don't get it with Sway.

Seems like the data structure returned by the two APIs must differ.

If so, the place to fix that might be in the swayipc-rs package: https://github.com/JayceFayne/swayipc-rs

@svenstaro
Copy link
Owner

I'm using straight i3.

@markstos
Copy link
Author

markstos commented Feb 7, 2022

I think I understand the root case, but so far I've only learned enough Rust to get this code that @JayceFayne wrote into the current state. 😂 Here's my guess about what's happening:

One difference between the Sway and i3 protocols is the format of the call to get_workspaces. The documentation what i3 returns is here:

https://i3wm.org/docs/ipc.html#_workspaces_reply

Sway documents their structure in man sway-ipc, Here's an example of what I see Sway actually returning:

❯ swaymsg -t get_workspaces
Workspace 4
  Output: eDP-1
  Layout: splith
  Representation: H[FirstApp]

Workspace 5 (off-screen)
  Output: eDP-1
  Layout: splith
  Representation: H[SecondApp]

I'm noticing that a "Layout" value is returned, which is the same field reported as "missing" in i3.

There's definitely a "layout" property for both i3 and Sway for i3 calls to "get_tree".

I suspect the i3 code is iterating over the results of a call to get_workspaces() (which doesn't contain a "layout" field) when it should be iterating over a call to get_windows() instead.

It would be helpful to confirm: is the IPC connection actually working with i3 and returning JSON, or is the error message accurate and the problem has to do with the presence or absence of the layout field? I checked the bug queue for the swayipc-rs package. Both i3 and Sway useres are definitely using the package, but no one is his reported this interoperability problem.

@markstos markstos mentioned this pull request Feb 7, 2022
2 tasks
@markstos
Copy link
Author

markstos commented Feb 7, 2022

There is another way to resolve this, which is to rename wm_i3.rs to wm_sway.rs, and use wm_i3.rs from the trunk. In this approach, sway would be considered a separate "feature" would no longer try to use the same IPC code as i3.

i3 would return to depending on i3-specific IPC library and Sway would use the swayipc library.

Yes, there would be some duplication of code, but the assumption that the Sway and i3 IPC approaches are compatible would be removed. Someone could then safely update the Sway code without worrying it would break i3 or vice versa.

Currently we have a developer collaboration challenge because the i3 devs don't use Sway and the Sway devs don't use i3 and no one is excited about installing and testing with both.

I think that may be the best way forward.

@markstos
Copy link
Author

@svenstaro Do you have feedback on the alternate approach I suggested above of keeping the i3 and Sway support separate? It's a bit of code duplication but I think it may be easier to maintain.

@svenstaro
Copy link
Owner

I'll revamp the plugin system in wmfocus in a bit. I think the current way is likely ill-designed in hindsight. Shouldn't be too bad. I might even set up a sway setup on a system to test this with. Stay tuned, sorry for the delay.

@markstos
Copy link
Author

@svenstaro Any luck with your Sway setup?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants