Skip to content
This repository was archived by the owner on Nov 12, 2023. It is now read-only.

Add Big Requests Extension#100

Open
Syudagye wants to merge 9 commits intomainfrom
big-requests
Open

Add Big Requests Extension#100
Syudagye wants to merge 9 commits intomainfrom
big-requests

Conversation

@Syudagye
Copy link
Collaborator

This adds support for the big request extension.

@Syudagye
Copy link
Collaborator Author

Syudagye commented Feb 19, 2023

Affected by #101.

@Syudagye Syudagye linked an issue Feb 19, 2023 that may be closed by this pull request
Copy link
Collaborator

@Antikyth Antikyth left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to look through this properly (i.e. when not super tired) yet - seems good to me overall, obviously implements the extension well. Pretty much just code style for readability is my suggestions right now, as well as a question regarding the dynamic major opcodes you discovered (tysm for that, that was a big oversight on my part! It was never mentioned in the core protocol I don't think).

@Antikyth
Copy link
Collaborator

(I removed the link to the issue there because, much to my surprise when I did the same thing in the past, that actually means that this PR will close that issue - it is really poor UX from GitHub, that is definitely not clear)

Syudagye and others added 2 commits February 26, 2023 19:47
Co-authored-by: Antikyth <104020300+Antikyth@users.noreply.github.com>
derive_xrb! {
#[derive(Derivative, Debug, X11Size, Readable, Writable)]
#[derivative(Hash, PartialEq, Eq)]
pub struct EnableBigRequests: Request(0 /* TODO: extensions use dynamic major opcodes */) -> reply::EnableBigRequests {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not using the todo macro here, because it prevents the crate from compiling

@Syudagye
Copy link
Collaborator Author

I'll document this a bit later, and if i remember to do so

@Antikyth
Copy link
Collaborator

Any idea what's causing the check failures? Looks like the macro isn't working? I can't remember where we left off with the macro and such.

@Syudagye
Copy link
Collaborator Author

I missed this apparently !
It was a legitimate clippy error

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants