Add builder patterns to build requests

[notgull]

The Why

In order to create a window in current x11rb, you do this:

x11rb/x11rb/examples/tutorial.rs

Lines 405 to 418 in e83d647

	// Create the window
	conn.create_window(
	COPY_DEPTH_FROM_PARENT, // depth (same as root)
	win, // window Id
	screen.root, // parent window
	0, // x
	0, // y
	150, // width
	150, // height
	10, // border width
	WindowClass::INPUT_OUTPUT, // class
	screen.root_visual, // visual
	&Default::default(),
	)?; // masks, not used yet

This function takes 11 arguments, not including the ones bundled into the Aux structure. In the above case, most of the arguments are numbers. In the past, I've found it difficult to use these functions. It's tough to remember which argument is which ("say, is the width the 4th or 5th one?"), and this has led to hard-to-diagnose buggy behavior in the past. Although this

The What

My solution would be to leverage Rust's "builder pattern", which would go something like this:

CreateWindowRequestBuilder::new()
    .depth(COPY_DEPTH_FROM_PARENT)
    .window(win)
    .x(0)
    .y(0)
    .width(150)
    .height(150)
    .border_width(10)
    .class(WindowClass::INPUT_OUTPUT)
    .visual(screen.root_visual)
    .send(&mut conn)?;

This ensures that:

• All parameters are explicitly labeled.
• Parameters can be provided in any order if needed.
• A CreateWindowRequest could be built up over a period of time.

The How

There are two ways of creating builders in Rust. You could have a struct where every field is its own generic, and then have a few pipeline methods that replace those generic fields successively. But I personally like the classical method better.

Take this request:

struct FooRequest {
    foo: u32,
    bar: u16,
    aux: FooRequestAux,
}

#[derive(Default)]
struct FooRequestAux {
    baz: Option<Baz>
}

We would have the generator also emit a new "builder" struct. The "classical" way of creating builder patterns is like this:

#[derive(Default)]
struct FooRequestBuilder {
    foo: Option<u32>,
    bar: Option<u16>,
    aux: FooRequestAux,
}

impl FooRequestBuilder {
    fn foo(&mut self, foo: u32) -> &mut Self {
        self.foo = Some(foo);
        self
    }

    // same thing for bar
    
    fn baz(&mut self, baz: Baz) -> &mut Self {
        self.aux.baz = Some(baz);
        self
    }

    fn build(&mut self) -> FooRequest {
        let foo = self.foo.take().unwrap();
        let bar = self.bar.take().unwrap();
        let aux = mem::take(&mut self.aux);

        FooRequest { foo, bar, aux }
    }
}

(Although, probably without the unwraping. build() could just be a fallible function.)

This stores the fields in Option<T> (except for auxiliary fields, which are already stored in Option<T> so I think this is irrelevant) and then takes them to be used in the eventual FooRequest. As for a potential send() method... we could always put this on the protocol side:

trait Builder {
    type Req: Request;

    fn build(&mut self) -> Self::Req;
}

...and then have an extension trait on the x11rb end:

trait BuilderExt : Builder {
    fn send(&mut self, c: &mut impl RequestConnection) -> Result<Cookie> { /* ... */ }
}

impl<B: Builder> BuilderExt for B {}

This is simpler and can be used from an &mut position, but there is a small amount of overhead in the form of the Options. What are your thoughts on this, @psychon?

[psychon]

What are your thoughts on this, @psychon?

Since I have to leave again soonish, I didn't read all of this (yet), but still, here is a quick first reaction: We do not yet have a builder pattern, but we have something "more structured" than the existing functions.

conn.send_trait_request_without_reply(CreateWindowRequest{
  depth: COPY_DEPTH_FROM_PARENT,
  wid: window,
  parent: screen.root,
  x: 0,
  y: 0,
  width: 150,
  height: 150,
  border_width: 10,
  class: WindowClass::INPUT_OUTPUT,
  visual: screen.root_visual,
  value_list: CreateWindowAux::default().into(), // dunno if this line works like this, this has to construct a Cow
})?;

Does this do what you want it to do? Is there some downside compared to the builder? On the plus side: All the Option::take() calls are gone and can no longer fail. ;-)

[notgull]

The main advantage of the Builder pattern (note: older page) is that it allows the user to construct large values in a piecemeal way, which the struct notation doesn’t allow for. In addition, I think it would make dealing with the auxiliary structures easier; I think there’s at least one issue in breadx over someone being confused with how to construct a value due to this.

Overall, it opens the door for usage in more styles of coding.

[psychon]

The main advantage of the Builder pattern (note: older page)

That page says:

The builder constructor should take as parameters only the data required to to make a T.

I think we already (pretty much) do that, except for the *Aux parameter being required. Dunno if adding a builder with all values wrapped in Optional helps much.

@eduardosm What do you think? (And what does everyone else think?)

[eduardosm]

I think we already (pretty much) do that, except for the *Aux parameter being required. Dunno if adding a builder with all values wrapped in Optional helps much.

I agree, the builder pattern is useful when you have few required parameters and many optionals. Using Options as proposed here means that a missing field is caught at runtime (i.e., panic) instead of compile-time.

[notgull]

The other option, which outsources the checks to compile time, would be to do this:

struct FooBuilder<Foo, Bar> {
    foo: Foo,
    bar: Bar,
    aux: FooRequestAux,
}

Then, we initialize it like this:

impl FooBuilder<(), ()> {
    fn new() -> Self {
        Self { foo: (), bar: (), aux: Default::default() }
    }
}

Then, for each field, you do this:

impl<Bar> FooBuilder<(), Bar> {
    fn foo(self, value: u32) -> FooBuilder<u32, Bar> {
        let FooBuilder { foo: (), bar, aux } = self;
        FooBuilder { foo: value, bar, aux }
    }
}

Finally, once every field has been set, you can do this to build it:

impl FooBuilder<u32, u16> {
    fn build(self) -> FooRequest {
        let Self { foo, bar, aux } = self;
        FooRequest { foo, bar, aux }
    }
}

The advantage of this design is that is uses compile-time guarantees to ensure that there is never a case where a value can’t be constructed at runtime. The disadvantage is that it’s less usable. You have to use it from a self position, and with the generics it’s difficult to pass into a function.

[notgull]

Closing as I do not plan to pursue this further in the wake of #883