Some comments on the X11 platform

Nobody has claimed this yet.

Hi,

I cam across Druid via https://www.reddit.com/r/rust/comments/h9z66n/changelog_for_druid_06_updated_from_05_two_weeks/. The changelog then links to a couple of things which then lead to more issues. I feel like I provide some useful input and hence I am opening this issue. I will try to add things to the already existing issues where applicable.

----

https://github.com/xi-editor/druid/pull/599#issuecomment-609376693

> Resizing the window on the left site makes it blink temporarily towards the
direction I drag on the right.

- [x] Random guess:

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L182

You are telling the X11 server to fill any exposed area with white before sending an expose event. I'd recommend just removing this line. The [default background is None](https://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html#requests:CreateWindow), which means that the server just sends an Expose event without drawing anything beforehand.

> This goes completely crazy when sizing it < 0 width, the window just flies
> away to the right joy. Does not happen when resizing on the right side

This makes no sense to me. X11 windows must have at least a size of 1x1. Sounds like a weird interaction with your WM, but I am not really sure what is going on.

> Application does not exit when closing the window

You seem to have figured out `WM_PROTOCOLS` already. Anyway, I recommend reading EWMH and scrolling through the table of contents of ICCCM (I have never read all of ICCCM myself, I think).

----

https://github.com/xi-editor/druid/pull/599#issuecomment-609459142

> Window redraw flashes: [...]

- [x] How are you redrawing? I see that cairo is involved. Are you trying directly to the cairo surface that represents your window? In that case, redrawing is "live" and intermediate steps are already visible (welcome to X11).

I recommend looking at double-buffering. With cairo, this is as simple as `cairo_surface_create_similar` (this is the C function) on the surface. Internally, this creates an X11 pixmap (if called on an X11 surface) and sets this up as a cairo surface. You can then draw to this surface and only copy your finished drawing to the X11 window.

If you are already doing double-buffering: Sorry for missing that part of the code.

- [x] By the way, you are calling `cairo_surface_flush()` where necessary, right? (Necessary = the drawing should actually be sent to the X11 server now.)

----

https://github.com/xi-editor/druid/issues/475

> Currently figure out the screen's refresh rate and sleep (!) for however long
> based on the refresh rate, which is probably not what we want to do. Better
> way to sync draws?

I have no clue about druid, but please tell me that you are only doing this if there is an active animation. I saw some screenshot of a calculator somewhere and that does not look like something that needs to be redrawn many times per second...?

----

https://github.com/xi-editor/druid/issues/475

> - Timer support implemented (WindowHandle::request_timer, etc)
> - Idle handle support (WindowHandle::get_idle_handle, etc)

You basically need to write your own main loop. The usual low-level X11 libraries that I have seen do not provide one for you. x11rb falls into this category.

Actually, you already wrote a very simple main loop, but so far it can only handle X11 events and nothing else:

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/application.rs#L271-L286

As an example on how a very simple version of this might look at, I recommend looking at (the main ingredient is `poll_with_timeout`): https://github.com/psychon/x11rb/blob/a6bd1453fd8e931394b9b1f2185fad48b7cca5fe/examples/xclock_utc.rs.

Basically:

- Call `poll_for_event()` to handle all the events that were read from the socket to the X11 server.
- Now there is nothing left to do. You might want to call this "idle" and run your idle-something-code.
- Wait for the X11 socket to become readable. Welcome to low-level unix programming and raw file descriptors.
- This waiting can be done with a timeout. Here, you'd need to keep a list of timers around and calculate when the next one expires.
- Repeat from the beginning.

This related to #934 and #935. I haven't written this to either of them since it does not completely fit with either of them.

In case you have the luxury of ignoring thread, the above works. In this case, you can also add a single call to `xcb_flush()` (well, `connection.flush()`) at the end of the main loop iteration. That way, you do not have to flush anywhere else (assuming your forbid functions that take a long time to return, but that would completely block your main loop and so I guess you do).

If you have to support threads: Don't.

If you still have to support threads: The Qt guys have not found a correct way to handle events in the main thread while other threads might also use the X11 connection. Their solution is to spawn a thread which always calls `xcb_wait_for_event()` in a loop and sends the result to the thread that runs the main loop (...via some Qt-internal mechanism that I know nothing about).

-----

I also took a look at the code in https://github.com/xi-editor/druid/tree/master/druid-shell/src/platform/x11.

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/application.rs#L224

I recommend only handling the last MotionNotify event in a series of events. If the X11 server generated these events faster than you can handle them, your application may lag behind more and more.

E.g. imagine a program that shows a cross at the current mouse position while it is inside of its window (just for the sake of argument). This program sleeps for a second after drawing its content (also just for the sake of argument). When the mouse cursor moves across the window, one can now see the old mouse positions to be drawn with a lot of delay. I hope this description makes sense.

A simple way to avoid this is to keep a "pending" `MotionNotify` event around while processing events. If a newer `MotionNotify` event comes in, the pending one is simply discarded. Some things, e.g. "mouse left the window" depend on the order of events and in this case the pending event would be handled.

Perhaps some code can explain this better than my weird explanation above:

https://github.com/awesomeWM/awesome/blob/e7113d7191e37c5b0faedccff6caf82d8e0bd77c/awesome.c#L392-L425

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/application.rs#L296-L297

> We need to queue up the destruction of all our windows. Failure to do so
> will lead to resource leaks.

Huh? Can someone tell me more about this? The X11 server automatically destroys all your resources when you close the connection (unless you use the `SetCloseDownMode` request and specify something else to happen).

Or is this about something like an `Rc` cycle, i.e. a leak inside of the Rust program?

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/keycodes.rs

This looks so simple. You should keep it at that version of the code. :-)

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L94-L140

- [x] After your switch to x11rb, I recommend taking a look at https://docs.rs/x11rb/0.5.0/x11rb/macro.atom_manager.html

----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L460-L471

- [x] Please, please set the `_NET_WM_NAME` property. It has type `UTF8_STRING`. I actually do not know how to decode `STRING` properties correctly, but just the fact that X11 is older than Unicode makes this scary.

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L293

- [ ] You might want to `.finish()` your cairo surface before destroying the underlying window. Most likely, it makes no difference, but in the cases where it makes a difference, it saves you some X11 errors.

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L260

- [x] Why are you storing a cairo context? I think storing a cairo surface makes more sense. Contexts are very cheap to create.

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L333-L354

- [ ] The X11 server does not care about your `SendEvent` request. It just forwards its content. Why do you need this to go through the X11 server instead of handling this internally?

Edit: Found an explanation at https://github.com/xi-editor/druid/pull/989/commits/fe132195b4d53f63a9f791c13f89136eed584c55#diff-31f2e83ba4096702ec15dea2f88c18edR336-R339
```
/// 3) Someone calls `invalidate` or `invalidate_rect` on us. We send ourselves an expose event
/// and end up in state 2. This is better than rendering straight away, because for example
/// they might have called `invalidate` from their paint callback, and then we'd end up
/// painting re-entrantively.
```
In this case I would say: Why not use your "soon existing" idle infrastructure instead? That would still save a round-trip to the server.

- [ ] Speaking of which, I would kind of expect that you keep around a dirty region. This contains all the pixels which need to be redrawn. Instead, you seem to redraw immediately when you receive an Expose event:

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/application.rs#L175-L182

- [ ] Well, expose events only contain a rectangle. For non-rectangular exposes, the server generates a series of Expose events. All but the last one have a non-zero `count` member. `count == 0` indicates that this is the last expose event for now.

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L358-L360

- [x] Uhm. Just keep that around in some variable. You get `ConfigureNotify` events when the size changes. That would avoid one round-trip to the X11 server.

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L433

- [x] With a reparenting WM, this does not do anything. There is some section in ICCCM somewhere that explains how to do this. I guess it involves sending an event to the root window.

Edit: Actually, I was wrong. ICCCM says that what you are doing here is fine.

-----

https://github.com/xi-editor/druid/blob/d810d30b95da52b3ca89efe63bd3b88b15b5a42b/druid-shell/src/platform/x11/window.rs#L443

- [ ] ICCCM forbids `SetInputFocus` with `CurrentTime`. Never ever may anything do that. Welcome to "X11 is mechanism, not policy".

-----

- [ ] Apropos violating ICCCM: There are lots of properties that you have to set before mapping your window, e.g. `WM_CLASS`. Also, `WM_CLIENT_MACHINE` and `WM_COMMAND` would be "nice to have". Oh and `_NET_WM_PID` (from EWMH, not ICCCM) is also a nice-to-have.

----

- [x] Edit: For cursor support (i.e.`WindowHandle::set_cursor`), I recommend taking a look at https://docs.rs/x11rb/0.6.0/x11rb/cursor/index.html

-----

Feel free to ping me as @psychon with questions in the future on other issues. There is certainly a lot that I do not know, but perhaps there are also bits and pieces that I can help with.

Edit:

- [x] I added some checkboxes to this in the hope that this simplifies tracking things in this big dump. Feel free to edit this post to check the boxes where applicable.

Start by triaging the separate X11 concerns in druid-shell/src/platform/x11/application.rs, window.rs, and keycodes.rs, along with the linked pull requests and issues. The issue has no single completion condition; split the actionable recommendations into focused issues, each with a defined file, test or reproduction, and done condition.

Written by the indexing model from the issue text.