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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safe bindings #83

Closed
ghost opened this issue Jun 12, 2018 · 3 comments
Closed

Safe bindings #83

ghost opened this issue Jun 12, 2018 · 3 comments

Comments

@ghost
Copy link

ghost commented Jun 12, 2018

I'd like to deprecate the x11-dl crate in favor of safe bindings (keeping x11 available where it's a better fit). I'm undecided on whether low- or high-level safe bindings would be better. By low-level, I mean a safe library that doesn't do much more than speak the X11 protocol (similar to XCB), while a high-level binding would use RAII structs to represent resources. For example:

// Low-level, safe binding
let display = x11::Display::open_default().unwrap();
let window: u32 = display.create_window(..........);
display.map_window(window);
.......... // main loop
display.unmap_window(window);
display.destroy_window(window);

And...

// High-level, safe binding
let display = x11::Display::open_default().unwrap();
let window: x11::Window = display.create_window(..........);
window.map();
.......... // main loop
window.unmap();

I've played around with code that uses the low-level approach of providing a Display struct that speaks the X protocol alone. It uses XOpenDisplay to connect to the display but otherwise uses XCB anywhere possible. The prime example of where XCB can't used afaik is when working with GLX contexts, so libX11 can't be eliminated entirely. The bindings would attempt to be neutral to Xlib and XCB but prefer the XCB way of doing things when there is a conflict.

@tomaka The major projects using x11-dl that I'm aware of are glutin and winit, so what do you think? Do you prefer high-level, low-level, or keep x11-dl the way it is? I'm leaning toward low-level, as high-level would almost result in a winit-like API but not cross-platform. If I get around to making this new crate, I can update glutin and winit to utilize it. However, I'm mostly just brainstorming ideas and this would likely not be any time soon, as I'm busy with school these days.

@francesca64 (replying to your comment in #82)

EDIT: A third alternative that would keep x11-dl mostly as it is while somewhat preventing problems like #81 would be to either return library structs successfully even with missing symbols returned as null, or to wrap functions in Option. Both of these would be pretty cumbersome to work with and the bindings would still be unsafe, but this would take the least work to implement.

@tomaka
Copy link

tomaka commented Jun 12, 2018

I'm leaning toward low-level, as high-level would almost result in a winit-like API but not cross-platform. If I get around to making this new crate, I can update glutin and winit to utilize it.

By low-level, I mean a safe library that doesn't do much more than speak the X11 protocol (similar to XCB)

OpenGL needs the exact pointers returned by the xlib available on the user's machine.
I don't think we can get away with anything else but raw bindings to xlib.

@francesca64
Copy link
Contributor

I'd like to deprecate the x11-dl crate in favor of safe bindings

I don't see what reason there could be for deprecating it. Though, dlib could probably be used to have both the static and dynamic bindings under one crate.

As far as APIs go, I've purposefully been keeping things straightforward, so that anyone reading the code would understand how it maps to Xlib. Here's some code from winit for getting frame extents:

let extents_atom = unsafe { self.get_atom_unchecked(b"_NET_FRAME_EXTENTS\0") };

if !hint_is_supported(extents_atom) {
    return None;
}

let extents: Option<Vec<c_ulong>> = self.get_property(
    window,
    extents_atom,
    ffi::XA_CARDINAL,
).ok();

(self is present because most of my util functions are methods on our XConnection struct, which is just all the dynamically loaded functions bundled with an X connection)

get_property returns Result<Vec<T>, GetPropertyError>, where T is constrained to c_char, c_short, and c_long (along with the unsigned counterparts). It handles all of the buffer stuff and memory safety for you, and just gives you all the data at once. I need to update it to return Result<Option<Vec<T>>, GetPropertyError> to better handle when the property is absent (since now you just get GetPropertyError::TypeMismatch(0) which kinda sucks), but I haven't gotten around to it yet.

There are still some design problems, i.e. get_atom_unchecked really irks me. The only way to make it safe while also eliminating pointless overhead seems to be to use a macro just like imgui-rs does.

One thing I've been thinking about is making more things more strongly typed, since while it's nice that it's statically verified that your return type is possible, it's not yet verified that it's correct. Since the vast majority of atoms people use have known meanings, it would be really sweet to be able to do something like this:

let extents = self.get_property::<NetFrameExtents>::(window);

Using a Property trait, which would use associated types and constants to abstract away the prop type and give you a return type that matches the prop type. This is something I've been planning on doing the next time I spruce up winit's X11 backend.

My API makes it obvious when a function is async, too. I've made a Flusher type, which is must_use (and prints an explanation of the Xlib output buffer if you don't use it), and provides you with the methods flush, sync, and queue to make all of this explicit. All async functions return this type. However, right now flush returns Result<(), XError>, which is wrong... we have no reason to believe we'll know the error status of the requests immediately after flushing the buffer. Really, the only reason I kept it there was because I wasn't ready to justify removing something like half of winit's error checks... I'd need to come up with a good time and place for error checking first.

In terms of what belongs in a standalone crate, one thing that's unresolved is whether or not it should include some higher-level utilities. hint_is_supported is one of these, but is the most trivial example. winit contains some pretty extensively tested heuristics for getting the correct frame extents on all WMs, accounting both for WMs that don't support the property and for ones that have various quirks. This is something that could be really useful for people to have, and that people are unlikely to implement correctly themselves.

@ghost
Copy link
Author

ghost commented Jul 22, 2018

Safe bindings specifically for X11 would be a whole lot more work than this project currently covers, and it wouldn't even work for many people's purposes. Closing for now.

@ghost ghost closed this as completed Jul 22, 2018
This issue was closed.
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

No branches or pull requests

2 participants