-
Notifications
You must be signed in to change notification settings - Fork 5
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
Parse the rest of the connection setup buffer to get the screens, depths, and visual types #8
base: master
Are you sure you want to change the base?
Conversation
x.zig
Outdated
}; | ||
|
||
var TEST_RECEIVED_CONNECT_SETUP_BUFFER align(4) = [_]u8{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some dummy data of 2x screens that have depths at 1, 4, 24, 32 and some visual types for each depth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment to the diff ⏩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool a unit test for a connection. Let's put this in a separate file though. Maybe we could add a test
directory and put it in there? Or we could just add a test.zig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the tests to a test/
directory and things seem to run as expected when using zig build test --summary all
. Also had to rename the standalone test
shell script to test.sh
so it doesn't conflict with the directory.
Does that look about right?
const format_list_offset = ConnectSetup.getFormatListOffset(self.fixed().vendor_len); | ||
const format_list_limit = ConnectSetup.getFormatListLimit(format_list_offset, self.fixed().format_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signatures of these functions are little bit cumbersome to use everywhere.
It seems like we could update them to just reference everything from self
and handle the complexity of getting the right fields in the functions. Is there a benefit to the current way?
ex.
pub fn getFormatListOffset(self: @This()) u32 {
return VendorOffset + std.mem.alignForward(u32, self.fixed().vendor_len, 4);
}
pub fn getFormatListLimit(self: @This()) u32 {
return self.getFormatListOffset() + (@sizeOf(Format) * self.fixed().format_count);
}
Then when someone wants to get the format list, they can simply just call...
Before:
const format_list_offset = x.ConnectSetup.getFormatListOffset(fixed.vendor_len);
const format_list_limit = x.ConnectSetup.getFormatListLimit(format_list_offset, fixed.format_count);
const formats = try conn.setup.getFormatList(format_list_offset, format_list_limit);
After:
const formats = try conn.setup.getFormatList();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is something for a future PR in any case)
@@ -1,3 +1,4 @@ | |||
zig-cache/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this back because it's nice to be able to switch Zig versions without these files appearing in staging.
// This exposes a `test` step to the `zig build --help` menu, providing a way for | ||
// the user to request running the unit tests. | ||
const test_step = b.step("test", "Run unit tests"); | ||
test_step.dependOn(&run_unit_tests.step); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marler8997 Friendly poke to review/merge (it's been updated to support Zig 0.13.0). I'd like to start getting some of these PR's merged so they don't keep stacking up with lots of juggling.
Parse the rest of the connection setup buffer to get the screens, depths, and visual types
This is one step on the path towards making the window transparent (support 32-bit color depth). Even though you can use
0xAARRGGBB
colors, the alpha channel has no effect when using the 24-bit color depth that gets inherited from the root window by default.If you naively, only set
create_window.Args.depth = 32
, you get aErrorCode.match
error when creating the window.So when creating the window, I think we need to also set the matching visual type ID that has the correct
depth
and.true_color
and use it when creating the window. Albeit, you still get aErrorCode.match
error at this point.Judging from the conversation on this StackOverflow question, I think another piece is figuring out what colormap to set on
window.Options.colormap
but I can tackle this in another follow-up PR if necessary -> #9Tested with Zig 0.11.0 on Manjaro Linux (arch-based) with XFCE.
Update: Now tested with Zig 0.13.0
Feel free to nit whatever 🙇. I'm new to Zig and X11 development so there is bound to be better patterns to accomplish some of these things and would love to learn the better ways.
Testing
Dev notes
(source https://www.x.org/releases/current/doc/xproto/x11protocol.html#Encoding::Connection_Setup)
Connection setup buffer format
The nested flexible array members (variable length array) of screens, depths, and visual types make the pointer arithmetic a bit more hairy than desired.