Some comments on the X11 platform

[psychon]

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.

---

#599 (comment)

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

• 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, 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).

---

#599 (comment)

Window redraw flashes: [...]

• 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.

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

---

#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...?

---

#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

• 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

• 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

• 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 fe13219#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

• 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

• 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.

---

• 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:

• 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.

[xStrom]

Thanks a lot for going through the trouble of giving us some feedback!

Some of the points I personally already know, while others points are news. In both cases it's great to have some of this written down. I'll also give some replies to a few points below.

Regarding the white flash and not setting the background white, that's probably related indeed. Windows had the same issue which was fixed in #915.

Regarding the draw scheduling, the master code is just a temporary hack. We do have a better solution in the works in #989 which will cut down on a lot of useless work and add buffering and v-sync.

Regarding MOTION_NOTIFY - I understand your concern but odds are we would like to receive more of these not less. Supporting high precision drawing can't work without receiving a lot of these and all of them. Imagine drawing a specific curve with your mouse. If profiling ends up showing that high frequency mouse events are too costly then we might switch it to an option, but that's a big if.

Regarding resource leaks during application quit - yes this is about cleanup in druid-shell Rust code, not X11 resources per se.

Regarding _NET_WM_NAME and _NET_WM_PID etc. I think it makes sense to support as many of these as we can. It's mostly just a case of the X11 backend being at a really early stage.

[jneem]

I did the atom_manager thing as part of the x11rb port.

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

I think we might not be, actually... (I opened up an issue on piet)

[crsaracco]

BTW, the initial commit of this (by me) was a super huge hack to see if we could get something else in the Rust ecosystem working. Code that I submitted is definitely not production-ready -- the expectation was that it would be improved over time -- but it's nice to have the list of stuff that's wrong! :)

[jneem]

#989 fixed a number of these, so I checked them off

[raphlinus]

I wanted to chime in here, to thank both Charles for getting the platform started, and psychon for giving constructive feedback. There's a lot of knowledge there, and it should serve as a useful guide for our corps of volunteers who are working on this. And of course to jneem for working to improve it :)

[jneem]

Thanks for getting it started, @crsaracco! I never would have written an x11 backend from scratch, but now that it's here I'm having a lot of fun (and learning a lot, too) tinkering with it.