rust-x-bindings / rust-xcb

Rust bindings and wrapper for XCB.
MIT License
165 stars 64 forks source link

Rustify connection to X Server #153

Closed cdecompilador closed 2 years ago

cdecompilador commented 2 years ago

In this case I think that a more idiomatic approach could be better, at the moment is just connect_* with many options, also there are some, from_*, my suggestion is to use the builder pattern and just maintain in the Connection struct the connect, and the simple from_* (Also remaining both ways to interact with the api would be ok)

My implementation

fn main() -> xcb::Result<()> {
    let (conn, _) = ConnBuilder::new()
        .with_optional_extensions(&[xcb::Extension::Glx])
        .with_auth("MIT-MAGIC-COOKIE-1", "...")
        .to_display("display name")
        .build()?;

     Ok(())
}

Currently

fn main() -> xcb::Result<()> {
     let (conn, _) = Connection::connect_to_display_with_auth_info_and_extensions(
         "display_name",
         &[],
         &[Extension::Glx],
         AuthInfo { name: "MIT-MAGIC-COOKIE-1", data: "..." });

    Ok(())
}

I've included a basic implementation, just in case you don't like the idea. The with_extensions(&[xcb::Extension::Glx]) could be easily replaced by with_extensions(xcb::Extension::Glx) for just 1 item with some trait magic, also there are some crates to auto generate this pattern but I'm not very into them, those are also an option.

rtbo commented 2 years ago

Hi, this is a good idea. I'm not really happy with the current design for building a new Connection.

There is one requirement I have to respect is that to safely implement Send and Sync, the extension data cache (Connection::ext_data field) must be immutable and entirely initialized in the Connection constructor.

But with an external builder, there is no such requirement. Here I can also reduce the number of constructors, because it is a little bit messy.

cdecompilador commented 2 years ago

Ok I will start working on it :)

cdecompilador commented 2 years ago

One question, the implementations of connect_* in Connect should remain or be removed? (If they have to be removed I will fix all the examples/tests don't worry)

rtbo commented 2 years ago

One question, the implementations of connect_* in Connect should remain or be removed?

Please leave them in. Quite a few people are already using the beta, I don't want to break anything at this point.