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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sway support #72

Closed
wants to merge 1 commit into from
Closed

Sway support #72

wants to merge 1 commit into from

Conversation

JayceFayne
Copy link

Fixes #53

  • Use swaypic for IPC communication
  • Use wayland to draw the windows

@markstos could you verify that you can focus wayland applications with these changes? Seems to work on my machine 馃槃

@svenstaro
Copy link
Owner

I believe the i3 IPC should also work actually as they are the same API.

@JayceFayne
Copy link
Author

JayceFayne commented Nov 26, 2020

Yes it should work with i3ipc aswell. But the maintainer
for i3ipc-rs seems to be AWOL at the moment. This change should be backwards compatible since swayipc is a superset of i3ipc

@markstos
Copy link

I've tested this on on Arch Linux and found that it worked. In particular:

  • Built without error using the command in the PKGBUILD file:
    • cargo build --locked --release --features i3
  • wmfocus works to switch to both Wayland-native and X11 windows.
    • For Wayland, tested with Alacritty and Qutebrowser
    • For X11, tested with xeyes
  • Tested trying to launch Rofi overlay while wmfocus was active to see what would happen. wmfocus just exited (which was fine)

No problems found. Thanks for the work @JayceFayne !

@markstos
Copy link

@JayceFayne With some more testing, I did find some issues. Once after selecting a letter, I got this in the console:

~/.cargo/bin/wmfocus
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "Couldn\'t grab keyboard input within 1.000524225s"', src/main.rs:227:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I tried what I thought was the same steps again and could not reproduce this.

I ran into a second issue, which may not be specific to Wayland support so it could be split off into a separate issue:

My overlay labeled a window as df, so I tried to typed df, but nothing appeared to happen. It wasn't clear which window had focus. I tried again and ran into the same "bug".

Although Vimium and Qutebrowser commonly use multiple letter combinations to select a link, I realized that the hint wasn't trying tell me that I needed to type "df" to select the window, it was trying to tell me me to type "d" to select the parent container OR "f" to select the child window.

That could be clearer! Putting the letters in sequence certainly makes them look like they are to be typed in sequence. It would be clearer if in cases like this that the child containers letter was vertically offset to give a visual clue that it's not the second character in a sequence. Ex: drop the child container's letter 50% of the letter height.

@markstos
Copy link

markstos commented Nov 29, 2020

I found another problem, which I'm not sure is specific to Wayland or not. To reproduce:

  1. Start wmfocus
  2. Instead of typing a letter, focus a different window with the mouse.

At this point, wmfocus seems to be "stuck". It has lost keyboard focus. If you type the letters in the hints now, the keys will go to the focused app, not to wmfocus.
To get wmfocus unstuck, it seems you have to kill it-- even switching back to focus the terminal window that launched wmfocus is not enough to get the hints to work.

I think if wmfocus loses keyboard focus without a selection being made, it should just exit-- the user has expressed a different intent.

@JayceFayne
Copy link
Author

JayceFayne commented Nov 30, 2020

Thanks for testing and reporting back these issues @markstos!
Like mentioned in #53 I am no expert in writing UI code for X11/Wayland. The current code is still using X11 under sway to draw the windows, maybe these issues are related to that?

@svenstaro
Copy link
Owner

I'm definitely interested in merging this in case you can ensure that things still draw ok when using sway. Are you still pursuing this?

@markstos
Copy link

As the tester, I would still support merging this as-is even though I found some issues. I'm already using it daily. Here's some follow-ups on the three points of feedback I reported:

  • One of the things I gave feedback on was how the overlays work in general, that is not specific to Sway and I adjusted to how they work with more use.
  • The issue getting stuck when losing focus should be fixed if possible, but is an edge case, it doesn't come up during normal use. I recommend opening a new issue for it.
  • The "panic" I error I reported should also be investigated, but has not come up again during daily use. I recommend opening a new issue for it.

In short, I recommend merging this but opening some follow-up issues for specific items.

@markstos
Copy link

I still support merging this, but do run into the "stuck" state once every day or so. It's a minor annoyance fixed by killing wmfocus. A workaround which could be a nice-to-have feature anyway is the the option for a timeout. For example, 10 seconds. If the user doesn't make a selection by the timeout value, wmfocus exits.

@markstos
Copy link

@svenstaro Thanks for the new release. Despite some edge cases patch, this PR is still better than no Sway support, so I recommend merging it and it can continue to refined later. Maybe I can find some time to resolve this conflict, since I'd like to use this in combination with highlighting the current window.

@svenstaro
Copy link
Owner

Alright I'd be ok with that if you resolve the conflicts.


[dependencies]
cairo-sys-rs = "0.10"
css-color-parser = "0.1"
font-loader = "0.11"
i3ipc = { version = "0.10", optional = true }
swayipc = { git = "https://github.com/JayceFayne/swayipc-rs", optional = true }
Copy link

Choose a reason for hiding this comment

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

For reproducible builds, it would be better if a specific version of swayipc was depended on.

For example, it looks like swayipc is about to have a 2.0 release. If that contains breaking changes, it will break this build.

@markstos
Copy link

markstos commented Feb 7, 2022

This should be closed in favor of #122, which is a rebased version of this PR.

@svenstaro svenstaro closed this Feb 7, 2022
@JayceFayne JayceFayne deleted the sway branch August 19, 2022 19:20
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.

Sway support
3 participants