Basic Xwayland support for anvil #254
Conversation
psychon
force-pushed
the
xwayland2
branch
2 times, most recently
from
295706f to
3570e8e
Compare
There was a problem hiding this comment.
Just some small nitpicks, overall this is a great PR. I am happy someone is finally tackling XWayland support.
- One issue is about the design of the current
XWindowManagertrait (is that even an appropriate name?) and about proper cleanup of the server. (I rather iron this part out now then later.) - And the other is a small suggestion to make the
unpaired_surfaceworkaround a little simpler. - I am also not super happy with the
commit_hook. Is there potentially a better place for this? Could we rather do this on WlSurface creation, if smithay would provide a callback here? Otherwise on first commit is probably fine. - Currently I am all for experimenting with this in anvil, but in the future large parts of this should likely be moved into smithay itself. I personally do not mind the usage of x11rb, but I am also not aware of the differences between the different x11-crates. Are there any limitations we should be aware of? Smithay also aims to provide support for multiple libraries, where possible, or at least allow the user to use the same building blocks, if they want to use a different one. If we would move this into smithay, I assume someone could still just re-implement this with a different library in their own compositor, if they wish to, right?
| .unwrap(); | ||
| } | ||
|
|
||
| fn xwayland_exited(&mut self) {} |
There was a problem hiding this comment.
@vberger In case e.g. XWayland crashes, do we need to remove the source again? Or is a closed fd automatically removed?
Also does XWindowManager need some kind of Id for an xwayland client? How would you identify, which xwayland client has exited? Running multiple xwayland clients might not be a common use-case, but is in theory possible, is it?
Note: This is not something that needs to be addressed in this PR. Anvil can very much assume, that it just uses one XWayland server, but this might be a general limitation of the XWindowManager trait.
There was a problem hiding this comment.
The XWindowManager trait and the general XWayland api in Smithay has several issues (see #245 ), so it'll need to be rethought and the question of how to handle restarting XWayland is a core subject. It will not be possible to handle this properly without this redesign.
There was a problem hiding this comment.
Or is a closed fd automatically removed?
I'm not even sure if the FD is closed when the X11 connection breaks. The UnixConnection is owned by x11rb::rust_connection::RustConnection, which lives in an Rc that is inside the x11rb_event_source::X11Source which is given to calloop. The same Rc is also referenced via Client::data_map() for the X11 client. The Client in turn is referenced by the callback used for handling X11 events. It is very well possible that this ends up being a cyclic reference where everything keeps everything alive and the UnixConnection is not dropped.
Well, actually, when the X11 connection is closed, the UnixConnection becomes readable and thus the X11Source wakes up. This does conn.poll_for_event(), gets some kind of error (x11rb::ConnectionError::IOError?), wraps this error in an IOError and returns it from X11Source::process_events. I do not know what calloop then does with this error...
Looks like calloop returns the error from event_loop.dispatch(). The code in anvil/src/winit.rs and udev.rs just checks for is_err() and then silently exits without any helpful message.
Since smithay's XWayland code does None.unwrap() when Xwayland dies, I haven't done much in this direction expect thinking "let's hope this somehow works". The above is more than I ever did before.
| } | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
I would rather like to request this info in new_window instead and removing the need to store the location in unpaired_surfaces. That should be possible as well, right?
There was a problem hiding this comment.
Yes and no. To avoid a deadlock with the X11 server, whenever you write to the X11 socket, you also have to read data. x11rb does that. Thus, even just sending a request can end up reading from the X11 socket. That's bad, since it could now happen that an X11 event was read from the socket and is buffered in x11rb, but the socket is no longer readable. Thus, the calloop integration that I add in this PR would not wake up and the event would sit in the buffer until the next event comes.
For this reason, some more magic is necessary before anything can interact with the X11 connection outside of the event handling (but such magic will definitely be necessary in the future). My best idea on how to tackle this so far is something like calloop's make_ping(): Whenever we do anything with the X11 connection, we also send a ping. This then allows us to later check if some more X11 events are pending.
Also, your suggestion would make new_window() fallible / have to return a Result, which means I have to think even more about error handling. Currently I managed to hide everything behind few unwrap()s. ;-)
Sure, what can I do? However, I never used calloop before and I fail to see "the big picture" in some of this. I do not think I can propose a new
This refers to "only get the window geometry when it is needed and not immediately"? If so, I think I answered that outside of this text.
wlroots does it on WlSurface creation. Dunno what other projects do. I got the suggestion to hijack the commit like this from @vberger on IRC since apparently there is no way to get a callback on WlSurface creation currently.
First: Of course I am biased towards x11rb since I am its author. Then: I do not think it really matters / makes much of a difference which X11 library is used. All of these should be possible to use here and I think all of these would have similar "friction points". Here is a bit of text about all the X11 rust crates that I could find: https://github.com/psychon/x11rb/blob/master/doc/comparison.md |
|
Here is a patch that I hacked together today: https://gist.github.com/psychon/871644d18d3c61853d0b5cd3e876290d Welcome to callback hell! In the end, I had to hack this up and use unsafe raw pointers to handle a pointer to the X11 connection. A |
|
Rebased ontop of #255 since that PR will likely be merged first. Before this rebase, the following commits were part of this PR:
The first commit mainly used to deal with passing around a |
|
Slightly related: At swaywm/wlroots#2404, the wlroots people are trying to figure out what makes shaped X11 windows work in weston (so that wlroots can do the same). |
This commit adds an xwayland feature to anvil. Right now, this feature doesn't do much. anvil uses the smithay code to start XWayland, but does not do anything with it once it is running. Signed-off-by: Uli Schlachter <psychon@znc.in>
This commits adds the necessary magic incantations to become the X11 WM after Xwayland starts. This uses the pure-Rust implementation from x11rb, but any other X11 crate could be used as well. Signed-off-by: Uli Schlachter <psychon@znc.in>
This commit creates an event source for calloop that receives X11 events from the X11 server. Signed-off-by: Uli Schlachter <psychon@znc.in>
Xwayland gives us a mapping between X11 window and WlSurface IDs via special WL_SURFACE_ID messages. This commit uses these messages to find the corresponding WlSurface. For this, the new client.get_resource API from wayland-server is needed. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
|
Rebased ontop of master |
There was a problem hiding this comment.
I think given the current circumstances, this PR is a good as it gets. 👍
My two issues with it are still:
- Xwayland API is fundamentally broken #245, which can be done later.
- The commit hook, which should be fixed by providing a surface-create callback in the future. (We should probably open an issue here as well to track this.)
As both issues are likely not solved over night, I am in favor of merging this.
Nice work.
I could make a long list of things that do not work. Instead, I will only report that glium is sad every time it has to render an X11 surface ("Debug message with high or medium severity:
GL_INVALID_OPERATION in glTexImage2D(format = GL_RGBA, type = GL_UNSIGNED_BYTE, internalformat = GL_RGB8).") and show a screenshot with xeyes, gvim and a menu (menus are the reason for the last commit in this PR since placing them at(0, 0)looked bad):I tried to do this in lots of small commits instead of one large code drop. I hope I succeeded in making this understandable.
I used x11rb for the X11 interactions since this crate is what I know best, but this should be relatively easily portable to other X11 libraries.
Interaction with the X11 server is done blockingly ((roughly) every
.reply()needs a round-trip). wlroots seem to do the same. The only alternative that I currently know of is breadX' async support.Currently, it is not safely possible to interact with the X11 server outside of reacting to X11 events ("safe" not in the sense of "Rust's
unsafe", but in the sense of "you might miss wakeups"):Right now a
calloop::Genericis used to wait for theUnixStreamconnecting us to the X11 server to become readable. Then, events are read as long as some are available. However, when doing anything with the X11 connection outside of this (even just sending an X11 request to the server), it could happen that x11rb reads an event from the server and buffers it in an internalVec. In this situation, it could happen that there is a pending event, but the FD is not readable.Dealing with this problem is left as an exercise to whoever adds the first code that needs to interact with the X11 server outside of events (which might very well be me).