Like how window Attributes use a struct with Option<...> fields and a bitmask to represent a set of window attributes, similar needs to be done for ConfigureWindow values.

Should migrate the useful parts of their docs, if any, first.

Relevant section in the X11 protocol: Encoding::Connection_Setup

Examples

Information sent by the client at connection setup:

define! {
    pub struct ConnectionSetup {
        pub endianness: Endianness,
        [(); 1], // 1 unused byte
        #authorization_protocol_name: u16, // length of string
        #authorization_protocol_data: u16, // length of string
        [(); 2], // 2 unused bytes
        pub authorization_protocol_name: String8,
        [(); {authorization_protocol_name}], // padding bytes for that string
        pub authorization_protocol_data: String8,
        [(); {authorization_protocol_data}], // padding bytes for the second string
    }
}

This will involve letting you use the content syntax for structs and enums in general:

• Unused bytes
  • () -- 1 unused byte.
  • [(); 20] -- 20 unused bytes.
  • [(); {fieldname}] -- Calculate the padding for a field ((4 - (byte_size % 4)) % 4).
• Field length
  • #fieldname: u16 -- Will be replaced with self.fieldname.len() when (de)serialized.

Example

define! {
    pub struct Host {
        pub family: HostFamily,
        (),
        #address: u16,
        pub address: Vec<u8>,
        [(); {address}],
    }
}

Examples

#[derive(new, unwrap)]
pub struct Atom(pub(crate) u32);

could derive:

#[automatically_derived]
impl Atom {
    pub const fn new(field_0: u32) -> Self {
        Self(field_0)
    }
}

#[automatically_derived]
impl Atom {
    pub const fn unwrap(self) -> (u32) {
        let Self(field_0) = self;

        (field_0)
    }
}

These need to be made public.

A syntax needs to be implemented for the messages macro in xrb-proc-macros that can represent metadata associated with event messages. This shouldn't be too complicated™, because all of the body can reuse the code for requests and replies.

The one thing to wonder about I guess is: how do you distinguish between events and requests syntax-wise? Two ways I initially thought of:

Square brackets instead of normal brackets

pub struct MyEvent[4];

I don't think this makes it clear enough, and square brackets already have associations in Rust that are nothing to do with events.

! before brackets

pub struct MyEvent!(4);

I think this is better, but it still isn't really clear. It probably doesn't matter though, because events will be defined in a separate module. Unless I think of a better idea, I'll probably implement this.

When splitting syn::Generics, for some reason there is a where clause component. This is strange because the generics shouldn't have a where clause - we parse that where clause separately, as we should, because it isn't always in the same place as the generics. Need to see why that component is split like that and how to do it properly.

The title says it all: implement cornflakes::Readable, cornflakes::Writable, and cornflakes::DataSize for bitmasks. This is one of the main things holding back a lot of X11 types from being implemented. These masks are implemented using the bitflags crate.

In the example for Sources, the macro name used is define!. This has since been updated to derive_xrb!. Since I haven't found a way to include xrb and cornflakes in the test environment, this wasn't picked up by running doctests.

This is a simple fix: replace define! with derive_xrb! in the xrbk_macro::source::Source documentation.

In the example, the source function does not take its argument by reference. It should, and its expression needs to then dereference them: *width * *height (or maybe the pattern can do that part...?).

Examples

/// Gets the closest ideal size to the given `width` and `height`.
///
/// For [`Cursor`], this is the largest size that can be fully displayed
/// within `width` and `height`. For [`Tile`], this is the size that can be
/// tiled fastest. For [`Stipple`], this is the size that can be stippled
/// fastest.
///
/// # Errors
/// - [`Drawable`]
/// - [`Match`] -- Generated if an [`InputOnly`] [window] is used with the
///   [`Tile`] or [`Stipple`] classes.
/// - [`Value`]
///
/// # Reply
/// This request generates a [`QueryBestSizeReply`].
///
/// [`Cursor`]: query_best_size::Class::Cursor
/// [`Tile`]: query_best_size::Class::Tile
/// [`Stipple`]: query_best_size::Class::Stipple
/// [`Drawable`]: crate::x11::errors::Drawable
/// [`Match`]: crate::x11::errors::Match
/// [`Value`]: crate::x11::errors::Value
/// [window]: Window
/// [`InputOnly`]: WindowClass::InputOnly
pub struct QueryBestSize<'a>(97) -> QueryBestSizeReply {
    /// The 'type' of 'best size' being queried.
    pub $class: QueryBestSizeClass,
    /// Indicates the desired screen.
    ///
    /// For [`Tile`] and [`Stipple`], the `drawable` indicates the screen
    /// and also possibly the window class and depth.
    ///
    /// An [`InputOnly`] [`Window`] cannot be used as the drawable for
    /// [`Tile`] or [`Stipple`], else a [`Match`] error occurs.
    ///
    /// [`Tile`]: query_best_size::Class::Tile
    /// [`Stipple`]: query_best_size::Class::Stipple
    /// [`InputOnly`]: query_best_size::Class::InputOnly
    pub drawable: &'a dyn Drawable,
    /// The given width to find an ideal size for.
    pub width: u16,
    /// The given height to find an ideal size for.
    pub height: u16,
}

When serialization and deserialization happen, numbers need to be written and read as either big endian (most significant to least significant byte order) or little endian (least significant to most significant byte order). This needs to be properly taken into account. One of the following options needs to be chosen and implemented:

• Allow the preferred byte order to be specified when serializing and deserializing (I imagine this would work by either passing through a ByteOrder or w/e enum, or by requiring three functions to be implemented, not just one, for every type - big endian, little endian, and native endian [native endian meaning matching the endianess of the computer XRB is running on])
• Force the use of the endianness of the system (native endian)
• Force the use of big endian (the standard byte order in networking - technically how it's implemented now). Only "problem" (I suspect it makes next to no difference) is that most architectures actually use little endian, so the bytes would have to always be reversed (done automatically, but just thinking about performance)

This issue probably fits more in cornflakes, but it's easy to keep track of here, and applies the the code generated by the define! macro too.

Document common::set::*.

Suggestion: #[read(...)] attribute to change the way a particular element is read, #[write(...)] attribute to change the way a particular element is written.

I'm not sure if this can work or not, but I wonder if it would be possible to do this kinda thing:

pub enum ReadError<Discrim = u32, Other = Infallible> {
    UnrecognisedDiscriminant(Discrim),
    Other(Other),
}

My concern would be how to define in the e.g. Readable trait - should work with associated types I suppose, main unfortunate part is that associated types can't have defaults.

Examples

#[derive(X11Size, ReadableWithContext)]
#[read_with(len: &usize)]
pub struct MyVec<T> {
    #[context(len => *len)]
    vec: Vec<T>,
}

Which could derive the following:

#[automatically_derived]
impl<T: Readable + X11Size> ReadableWithContext for MyVec<T> {
    type Context = (usize);

    fn read_with(buf: &mut impl Buf, (param_len): &(usize)) -> Result<Self, ReadError> {
        fn field_vec(len: &usize) -> <Vec<T>>::Context {
            *len
        }

        let field_vec = <Vec<T>>::read_with(buf, field_vec(param_len));

        Ok(Self {
            vec: field_vec,
        })
    }
}

For example, if you add a documentation comment on Window, the documentation can't be shown by the IDE. I'm guessing this might have something to do with the identifier being used in different places - maybe when implementing traits for the type, for example, it needs to use a different Span, maybe Span::call_site() or something?

After working on everything else, the base data types and structures have 'fallen to the side'. With some of the new tools that I have developed (i.e. the macro and some cornflakes stuff), as well as some more knowledge, I should be able to reimplement everything better.

These traits need to be implemented for appropriate Rust types, such as primitives, Option<T>, etc., and X11 protocol types in XRB.

Event messages in X11 need to be implemented in XRB. This requires the event macro syntax from Add event metadata syntax parsing to the macro. #1.

The serialization has been rewritten in cornflakes (our serialization library). There are many errors from code written with the old version: they just need to be fixed/rewritten, it should hopefully be easy to see how.

A.k.a. serialize and deserialize traits. Might want to do #3 first?

Currently, the derive_xrb! macro derives X11Size, Readable, and Writable for all structs and enums. This should be changed so that one can opt into deriving these traits by matching the derive syntax - i.e. derive attributes are parsed, XRBK derives are stripped from the attributes, and the macro derives for the traits that were present in the derive attributes.

Perhaps better syntax highlighting for the other attributes could be done by instead leaving the derives on, but putting another attribute (perhaps reusing the #[hide] attribute but on the whole item) so that the derive macros know not to actually derive as usual. I think this is the only way to do syntax highlighting (other option being a dummy derive macro), because the other way for us to define attributes is attribute macros, but they are active: they get removed from use anyway (and generate weird errors when I tried).

Examples

derive_xrb! {
    #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, X11Size, Writable, Readable)]
    pub struct Point {
        pub x: i32,
        pub y: i32,
    }
}

This example would be the same as if you weren't to use the derive_xrb! macro.

derive_xrb! {
    #[derive(Clone, Eq, PartialEq, Hash, Debug, X11Size, Readable, Writable)]
    pub struct Host {
        pub family: HostFamily,
        _,

        #[allow(clippy::cast_possible_truncation)]
        let address_len: u16 = address => address.len() as u16,

        #[context(address_len => *address_len as usize)]
        pub address: Vec<u8>,
        [_; ..],
    }
}

This example uses the derive_xrb! syntax, but enables deriving the XRBK traits only by including them in the #[derive(...)] attribute.

There needs to be a semicolon after a unit or tuple struct (Items::Unit or Items::Unnamed), but not after an enum or a 'normal struct' (Enum or Items::Named).

e.g.

pub struct UnitStruct; /* semicolon */

pub struct TupleStruct(u32, u32); /* semicolon */

pub struct NormalStruct {
    field1: u32,
    field1: u32,
} /* no semicolon */

pub enum Enum {
    UnitVariant, /* no semicolon */
    TupleVariant(u32, u32), /* no semicolon */
    StructVariant {
        field1: u32,
        field1: u32,
    }, /* no semicolon */
} /* no semicolon */

One reason for this is #59, but the other is that in the Request example, there is a name field where the context attribute has been forgotten.

Fix for the name field:

#[context(name_len => *name_len as usize)]
pub name: String8,

This issue is causing the CI to fail, so it is important to fix quickly to make development of other features easier.

A lot of types have been renamed in XRB for clarity, but people who are familiar with the existing names may find them hard to find. #[doc(alias = ...)] attributes can be added with the names people are familiar with.

For example (pseudocode)

// ... x11::event ...

derive_xrb! {
    #[doc(alias = "FocusOut")]
    pub struct Unfocus: Event(...) {
        ...
    }
}

E.g. the CreateWindow request has the field visual, where 0 means CopyFromParent, and any other value means a VisualId. This will need to therefore be something like:

pub enum Inheritable<T> {
    CopyFromParent,
    Uninherited(T),
}

where Uninherited(T) writes no discriminant, and CopyFromParent, in the case of VisualIds, is written as a u32 value (as are VisualIds).

Thoughts

May be able to take advantage of the fact that VisualIds actually wrap u32 values. Traits could be implemented for Inheritable<T> where T wraps a u32 value (what bound that could be, I'm not sure; T: From<u32> wouldn't work because of the fact that it would be possible to implement From<u16>, etc., for T as well).

Requests define the OtherErrors that they generate (i.e. other than those potentially generated for all requests - see xrb::message::RequestError for more information). There needs to be syntax to define these errors.

Potential ideas

• derive_xrb! {
      pub struct MyRequest: Request(5) -> Result<(), MyRequestErrors> {}
  }

  Issues with this idea are that it's particularly structured for how much it 'resembles' the Result type. Really, you'd want that to mean it's not defining two associated types, only one. That could cause difficulty with ensuring the associated type is a Result though. Don't think this is gonna work, but who knows, maybe.

  derive_xrb! {
      pub struct MyInfallibleRequest(6) -> Result<(), Infallible> {}
  }

• derive_xrb! {
      pub struct MyRequest: Request(5, MyRequestErrors) {}
  }

  Issues with this idea is that errors are also 'returned' by the request, so there is a bit of a disconnect. Though, that said, whether we can treat them as such I'm not sure - I don't think you get a message that says "there are no more errors to follow for that request"? If you don't, then I don't think we can return the errors along with the replies.

  I think this is better than the previous one.

  derive_xrb! {
      pub struct MyInfallibleRequest: Request(6) {}
  }

For example, all fields measured in pixels should use Px<Num> instead of Num. This makes these fields type safe: you can't accidentally use pixels instead of millimeters.

They need to start with the size of their masks, not with 0.

Added in Enable the macro syntax to be applied to non-message structs and enums too. #10, xrb_proc_macros now supports define!; a macro to generate serialization and deserialization with xrb_proc_macro's custom syntax for normal structs and enums, rather than just messages.

With the addition of enums, this means that xrb_proc_macros::content::Content is no longer quite correct (it was modelled after structs only), so it needs to be refactored to reflect a better syntax. Perhaps the concept of shorthand/longhand content definitions may need to be handled differently, I don't know yet.

Currently, the implementations of X11 data structures/types/etc. all reside in the x11 folder:

xrb/src:

• lib.rs
• x11
  • etc.

But the idea is that XRB will provide implementations of various X extensions/protocols too, and these should be in separate folders. Really, the x11 folder should contain only the messages in the core protocol (and any types specifically used for those messages only). For example, the Atom type, the Request trait, the EventMask - these would not be in the x11 folder, because they are general-use X11 types. ChangeSaveSetMode would, however, be in the x11 folder, along with the ChangeSaveSet request, because they are specifically for messages defined by the core protocol.

A lot of the functions in the sets are not const because they use closures to destructure internal representations. It should be possible to achieve the destructuring with the use of const fns though, with no closure, perhaps associated const fns of the internal representation types.

I don't believe it is possible for the builders to work in const contexts yet, unless mutable references are allowed in const fns (link to tracking issue). I feel like in theory they should be able to work - all of the mutations can occur during const evaluation only, and all of the mutable references go out of scope if the build() function is called. The thing that wouldn't work is a const mutable reference at the end of const evaluation (i.e. build() is not called), which feels like it could be something that an error is generated for only if a mutable reference is left over at the end of const evaluation. That's just my intuitive point of view, though, not sure how it works in practice.

Currently, sequence fields in replies and events are implicit. I think that making these implicit was the wrong decision, because:

• It complicates the expansion code for replies and events.
• It hides a field that will end up being part of the output code, and makes it impossible to give that field documentation.

I think it should be a field explicitly written out. We will have to see whether it needs special treatment in terms of its positioning - I think it might be located in a specific location in the header, if I remember correctly (big if there). If so, perhaps a #[sequence] attribute would be good.

The length() trait methods in Reply and Request can have a default implementation, since these traits have a trait bound of X11Size:

// ... snippet ...

// for Request
fn length(&self) -> u16 {
    if self.x11_size() % 4 != 0 {
        panic!("a Request's size must be a multiple of 4 bytes");
    }

    (self.x11_size() / 4) as u16
}

// ... snippet ...

// ... snippet ...

// for Reply
fn length(&self) -> u32 {
    let size = self.x11_size();

    if size < 32 {
        panic!("a Reply's size must be at least 32 bytes");
    } else if size % 4 != 0 {
        panic!("a Reply's size must be a multiple of 4 bytes");
    }

    ((self.x11_size() - 32) / 4) as u32
}

// ... snippet ...

The procedural macro that generates X messages in X11 needs to implement the length() method on its messages. This method needs to count up the byte size all the items in the message definition (fields, unused bytes, and field lengths).

I think as a general code style rule, it is clearer to write:

#[derive(Debug, Hash, PartialEq, Eq, X11Size, Readable, Writable)]
pub struct SetFontPath: Request(51, error::Value) {
    #[allow(clippy::cast_possible_truncation)]
    let path_len: u16 = path => path.len() as u16,
    [_; 2],

    #[context(path_len => usize::from(*path_len))]
    pub path: Vec<LengthString8>,
    [_; path => pad(path)],
}

rather than:

#[derive(Debug, Hash, PartialEq, Eq, X11Size, Readable, Writable)]
pub struct SetFontPath: Request(51, error::Value) {
    #[allow(clippy::cast_possible_truncation)]
    let path_len: u16 = path => path.len() as u16,
    [_; 2],

    #[context(path_len => usize::from(*path_len))]
    pub path: Vec<LengthString8>,
    [_; ..],
}

I asked for some feedback for this from friends, and the first was strongly preferred for clarity and readability.

Unresolved questions

Should unused bytes for filling up to the minimum message limits be explicit, or stay as inferred? I'm leaning towards inferred, but feel free to discuss.

Inferred

pub struct GetSelectionOwner: Reply for request::GetSelectionOwner {
	#[sequence]
	#[derivative(Hash = "ignore", PartialEq = "ignore")]
	pub sequence: u16,

	pub owner: Option<Window>,
	[_; ..],
}

Explicit

pub struct GetSelectionOwner: Reply for request::GetSelectionOwner {
	#[sequence]
	#[derivative(Hash = "ignore", PartialEq = "ignore")]
	pub sequence: u16,

	pub owner: Option<Window>,
	[_; 20],
}

xrbk_macro should add bounds to generics in a type when implementing XRBK traits for it. For example, every element that doesn't have a #[hide] attribute would have to implement X11Size before X11Size could be derived for the struct or enum.

Color ID is a more appropriate name. Not sure if it's 100% accurate, but it definitely gets the point across that it refers to a color, whilst not being that color itself.

There is certain macro syntax which should be parsed or not parsed depending on which XRBK traits one derives.

For example, a let element would be parsed if any of these traits are derived:

• X11Size
• ConstantX11Size
• Readable

...but without the source used to write it:

let name_len: u16

Only if Writable is derived would it have a source:

let name_len: u16 = name => name.len() as u16

Also, if the macro syntax is parsed conditionally depending on which traits are derived, then all structs should be parsed custom, not just those with at least one XRBK trait. This is so that requests, replies, events, and errors may still be implemented without deriving their XRBK traits (you really should, but for consistency, and therefore each of learning, it wouldn't be required). It also means better errors can be given.