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.

Sign up for GitHub

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

Jump to bottom

Port X11 backend from xcb to x11rb. #1025

Merged
merged 5 commits into from
Jun 19, 2020
Merged

Port X11 backend from xcb to x11rb. #1025

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
869c639
Port X11 backend from xcb to x11rb.
jneem Jun 9, 2020
184312c
Add changelog entry.
jneem Jun 9, 2020
0e02b08
Review comments.
jneem Jun 10, 2020
38b6248
Update x11rb.
jneem Jun 19, 2020
b0d3afd
Fix broken link.
jneem Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion druid-shell/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ gtk = { version = "0.8.1", optional = true }
glib = { version = "0.9.3", optional = true }
glib-sys = { version = "0.9.1", optional = true }
gtk-sys = { version = "0.9.2", optional = true }
x11rb = { version = "0.5.0", features = ["allow-unsafe-code", "randr"], optional = true }
x11rb = { version = "0.6.0", features = ["allow-unsafe-code", "randr"], optional = true }

[target.'cfg(target_os="windows")'.dependencies]
wio = "0.2.2"
Expand Down
64 changes: 33 additions & 31 deletions druid-shell/src/platform/x11/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ struct State {

impl Application {
pub fn new() -> Result<Application, Error> {
// If we want to support OpenGL, we will need to open a connection with Xlib support (see
// https://xcb.freedesktop.org/opengl/ for background). There is some sample code for this
// in the `rust-xcb` crate (see `connect_with_xlib_display`), although it may be missing
// something: according to the link below, If you want to handle events through x11rb and
// libxcb, you should call XSetEventQueueOwner(dpy, XCBOwnsEventQueue). Otherwise, libX11
// might randomly eat your events / move them to its own event queue.
//
// https://github.com/xi-editor/druid/pull/1025/files/76b923417183bd103f61e56b56a56474b7417cec#r442777892
Copy link
Collaborator

Choose a reason for hiding this comment

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

This link is broken for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed, and I'll merge when CI lets me. Definitely nice to be using a library that is both rusty and actively maintained to the point where the maintainer drops by and tells us what we should be doing better 😄

let (conn, screen_num) = XCBConnection::connect(None)?;
Copy link
Member

Choose a reason for hiding this comment

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

I haven't really investigated this stuff with either library, but is this an equivalent port? The connect_with_xlib_display docs say that going through XLib is needed for OpenGL support. I'm not sure if cairo uses OpenGL but if it does - does the new connection also provide that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To my knowledge Cairo does not use OpenGL by default. There is an experimental feature for it but otherwise it uses the platform library (X11 on Linux). So it should be fine if there is no OpenGL available, unless we want to use it ourselves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suspect that we will want OpenGL support eventually. If not for cairo, then for #891-related stuff. But it looks like it will need some work on the x11rb side.

Copy link
Contributor

@psychon psychon Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you will want to use the x11 crate for this. (The following "talks C", I haven't looked at the Rust bindings of any of this.)

  • create you Display* via x11.
  • Get the underlying XCB Connection via XGetXCBConnection. This gives you an xcb_connection_t*.
  • This pointer can be given to x11rb::XCBConnection::from_raw_xcb_connection(dpy, false).
  • If you want to handle events through x11rb and libxcb, you should call XSetEventQueueOwner(dpy, XCBOwnsEventQueue). Otherwise, libX11 might randomly eat your events / move them to its own event queue.
  • One you drop / XCloseDisplay the Display*, any further use of the XCBConnection might cause demons to fly through your nose, so be careful (by the way: Similar fun can result with cairo, but cairo makes it much easier to accidentally continue using the connection after it was closed / freed.).

Congratulations, you now have a single X11 connection which you can access through all of libX11, libxcb and x11rb.

API-wise, I do not see anything that could be added to x11rb. The two functions I mentioned above live in libX11.so and libX11-xcb.so and x11rb certainly has no business depending on them.

Edit: This is also what connect_with_xlib_display does (minus the setting of the event queue owner...?) https://github.com/rtbo/rust-xcb/blob/69a9d0126d40e7e3a9b354c07fbf31a81443b105/src/base.rs#L637-L640

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the detailed information! I'd prefer not to block this PR on OpenGL support, so I've just dropped in a comment for now.

let connection = Rc::new(conn);
let window_id = Application::create_event_window(&connection, screen_num as i32)?;
Expand All @@ -96,37 +104,31 @@ impl Application {
.ok_or_else(|| anyhow!("invalid screen num: {}", screen_num))?;

// Create the actual window
if let Some(err) = conn
.create_window(
// Window depth
x11rb::COPY_FROM_PARENT.try_into().unwrap(),
// The new window's ID
id,
// Parent window of this new window
screen.root,
// X-coordinate of the new window
0,
// Y-coordinate of the new window
0,
// Width of the new window
1,
// Height of the new window
1,
// Border width
0,
// Window class type
WindowClass::InputOnly,
// Visual ID
x11rb::COPY_FROM_PARENT,
// Window properties mask
&CreateWindowAux::new().event_mask(EventMask::StructureNotify),
)?
.check()?
{
// TODO: https://github.com/psychon/x11rb/pull/469 will make error handling easier with
// the next x11rb release.
return Err(x11rb::errors::ReplyError::X11Error(err).into());
}
conn.create_window(
// Window depth
x11rb::COPY_FROM_PARENT.try_into().unwrap(),
// The new window's ID
id,
// Parent window of this new window
screen.root,
// X-coordinate of the new window
0,
// Y-coordinate of the new window
0,
// Width of the new window
1,
// Height of the new window
1,
// Border width
0,
// Window class type
WindowClass::InputOnly,
// Visual ID
x11rb::COPY_FROM_PARENT,
// Window properties mask
&CreateWindowAux::new().event_mask(EventMask::StructureNotify),
)?
.check()?;

Ok(id)
}
Expand Down
137 changes: 62 additions & 75 deletions druid-shell/src/platform/x11/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ use std::rc::{Rc, Weak};

use anyhow::{anyhow, Context, Error};
use cairo::{XCBConnection, XCBDrawable, XCBSurface, XCBVisualType};
use x11rb::atom_manager;
use x11rb::connection::Connection;
use x11rb::protocol::xproto::{
self, Atom, AtomEnum, ConnectionExt, EventMask, ExposeEvent, PropMode, Visualtype, WindowClass,
self, AtomEnum, ConnectionExt, EventMask, ExposeEvent, PropMode, Visualtype, WindowClass,
};
use x11rb::wrapper::ConnectionExt as WrapperConnectionExt;

Expand Down Expand Up @@ -127,51 +128,23 @@ impl WindowBuilder {
/// Registers and returns all the atoms that the window will need.
fn atoms(&self, window_id: u32) -> Result<WindowAtoms, Error> {
let conn = self.app.connection();

// WM_PROTOCOLS
//
// List of atoms that identify the communications protocols between
// the client and window manager in which the client is willing to participate.
//
// https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#wm_protocols_property
let wm_protocols = conn
.intern_atom(false, b"WM_PROTOCOLS")
.context("failed to get WM_PROTOCOLS")?
.reply()
.context("failed to get WM_PROTOCOLS reply")?
.atom;

// WM_DELETE_WINDOW
//
// Including this atom in the WM_PROTOCOLS property on each window makes sure that
// if the window manager respects WM_DELETE_WINDOW it will send us the event.
//
// The WM_DELETE_WINDOW event is sent when there is a request to close the window.
// Registering for but ignoring this event means that the window will remain open.
//
// https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#window_deletion
let wm_delete_window = conn
.intern_atom(false, b"WM_DELETE_WINDOW")
.context("failed to get WM_DELETE_WINDOW")?
let atoms = WindowAtoms::new(conn.as_ref())
.context("failed to intern X11 atoms")?
.reply()
.context("failed to get WM_DELETE_WINDOW reply")?
.atom;
.context("failed to get back X11 atoms")?;

// Replace the window's WM_PROTOCOLS with the following.
let protocols = [wm_delete_window];
let protocols = [atoms.WM_DELETE_WINDOW];
// TODO(x11/errors): Check the response for errors?
conn.change_property32(
PropMode::Replace,
window_id,
wm_protocols,
atoms.WM_PROTOCOLS,
AtomEnum::ATOM,
&protocols,
)?;

Ok(WindowAtoms {
wm_protocols,
wm_delete_window,
})
Ok(atoms)
}

/// Create a new cairo `Context`.
Expand Down Expand Up @@ -229,40 +202,34 @@ impl WindowBuilder {
);

// Create the actual window
// TODO: https://github.com/psychon/x11rb/pull/469 will make error handling easier with the
// next x11rb release.
if let Some(err) = conn
.create_window(
// Window depth
x11rb::COPY_FROM_PARENT.try_into().unwrap(),
// The new window's ID
id,
// Parent window of this new window
// TODO(#468): either `screen.root()` (no parent window) or pass parent here to attach
screen.root,
// X-coordinate of the new window
0,
// Y-coordinate of the new window
0,
// Width of the new window
// TODO(x11/dpi_scaling): figure out DPI scaling
self.size.width as u16,
// Height of the new window
// TODO(x11/dpi_scaling): figure out DPI scaling
self.size.height as u16,
// Border width
0,
// Window class type
WindowClass::InputOutput,
// Visual ID
visual_id,
// Window properties mask
&cw_values,
)?
.check()?
{
return Err(x11rb::errors::ReplyError::X11Error(err).into());
}
conn.create_window(
// Window depth
x11rb::COPY_FROM_PARENT.try_into().unwrap(),
// The new window's ID
id,
// Parent window of this new window
// TODO(#468): either `screen.root()` (no parent window) or pass parent here to attach
screen.root,
// X-coordinate of the new window
0,
// Y-coordinate of the new window
0,
// Width of the new window
// TODO(x11/dpi_scaling): figure out DPI scaling
self.size.width as u16,
// Height of the new window
// TODO(x11/dpi_scaling): figure out DPI scaling
self.size.height as u16,
// Border width
0,
// Window class type
WindowClass::InputOutput,
// Visual ID
visual_id,
// Window properties mask
&cw_values,
)?
.check()?;

// TODO(x11/errors): Should do proper cleanup (window destruction etc) in case of error
let atoms = self.atoms(id)?;
Expand Down Expand Up @@ -303,10 +270,30 @@ pub(crate) struct Window {
state: RefCell<WindowState>,
}

/// All the Atom references the window needs.
struct WindowAtoms {
wm_protocols: Atom,
wm_delete_window: Atom,
// This creates a `struct WindowAtoms` containing the specified atoms as members (along with some
// convenience methods to intern and query those atoms). We use the following atoms:
//
// WM_PROTOCOLS
//
// List of atoms that identify the communications protocols between
// the client and window manager in which the client is willing to participate.
//
// https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#wm_protocols_property
//
// WM_DELETE_WINDOW
//
// Including this atom in the WM_PROTOCOLS property on each window makes sure that
// if the window manager respects WM_DELETE_WINDOW it will send us the event.
//
// The WM_DELETE_WINDOW event is sent when there is a request to close the window.
// Registering for but ignoring this event means that the window will remain open.
//
// https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#window_deletion
atom_manager! {
WindowAtoms: WindowAtomsCookie {
WM_PROTOCOLS,
WM_DELETE_WINDOW,
}
}

/// The mutable state of the window.
Expand Down Expand Up @@ -633,9 +620,9 @@ impl Window {
) -> Result<(), Error> {
// https://www.x.org/releases/X11R7.7/doc/libX11/libX11/libX11.html#id2745388
// https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#window_deletion
if client_message.type_ == self.atoms.wm_protocols && client_message.format == 32 {
if client_message.type_ == self.atoms.WM_PROTOCOLS && client_message.format == 32 {
let protocol = client_message.data.as_data32()[0];
if protocol == self.atoms.wm_delete_window {
if protocol == self.atoms.WM_DELETE_WINDOW {
self.close();
}
}
Expand Down