Skip to content
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.

Sign up for GitHub

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

Jump to bottom

internal: Add libxcb and replace some Xlib functions #2614

Closed
wants to merge 3 commits into from
Closed

internal: Add libxcb and replace some Xlib functions #2614

wants to merge 3 commits into from

Conversation

notgull
Copy link
Member

@notgull notgull commented Jan 1, 2023 •
edited
Loading

  • Tested on all platforms changed.
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

This PR replaces portions of the X11 backend. Rather than using libX11, which is an outdated and buggy library, it uses libxcb, though the x11rb crate, to communicate with the X server. This PR significantly reduces the amount of unsafe code found in the backend, For some reason cargo geiger crashes on this codebase, so I can't accurately measure the change, but I can guarantee it's significantly lower. In fact, almost all of the unsafe code in the X11 backend has been removed.

I'm aware that prior work on this has been done in #1932 and #2088, but at this point I think it's best to start from scratch.

IME appeared to be broken after I started integration of x11rb (as if it wasn't broken anyways). I've deleted it completely for now. We may want to use xcb-imdkit or xim-rs to replace it.

Blocked on #2662 for now

@notgull notgull changed the title Add x11rb and replace some Xlib functions internal: Add x11rb and replace some Xlib functions Jan 1, 2023
@notgull notgull changed the title internal: Add x11rb and replace some Xlib functions internal: Add libxcb and replace some Xlib functions Jan 1, 2023
@ids1024
Copy link
Member

ids1024 commented Jan 1, 2023

Running in a NetBSD VM I see "failed to open library "libxcb.so.1".

Looks like the library is at /usr/X11R7/lib/libxcb.so.2.

@ids1024 ids1024 mentioned this pull request Jan 1, 2023
Closed
@notgull notgull marked this pull request as ready for review January 2, 2023 21:20
@madsmtm madsmtm added DS - x11 S - maintenance Repaying technical debt S - enhancement Wouldn't this be the coolest? labels Jan 3, 2023
@kchibisov
Copy link
Member

It would be nice to make the GLX/EGL work as it was before, and also ensure that it doesn't break other stuff like wgpu/vulkan. Since I think what we have is a mix of xlib and xcb.

What is the situation with error handling here as in which library delivers the errors(Xlib?)?

I'm not that familiar with X11 though.

@notgull
Copy link
Member Author

notgull commented Jan 3, 2023

It would be nice to make the GLX/EGL work as it was before, and also ensure that it doesn't break other stuff like wgpu/vulkan. Since I think what we have is a mix of xlib and xcb.

We should be able to just keep the Display pointer around and still pass that on through as_raw_display_handle, so nothing should break unless we want it to.

What is the situation with error handling here as in which library delivers the errors(Xlib?)?

I still keep the Xlib error handling infrastructure around. However, x11rb translates libxcb errors to higher-level errors (see ConnectionError). When I receive these errors, I usually panic on them, because that's what the backend did before.

@ids1024 ids1024 mentioned this pull request Jan 5, 2023
Closed
@notgull
Copy link
Member Author

notgull commented Jan 10, 2023 •
edited
Loading

Okay, I tested this code on:

  • Linux
  • OpenBSD
  • FreeBSD
  • DragonflyBSD

NetBSD appears to suffer from an x11-dl bug (AltF02/x11-rs#166), and Illumos fails to build (#2624).

The CI error is a spurious iOS network error, or it seems like it.

@notgull notgull force-pushed the x11-libxcb branch 3 times, most recently from 8af0265 to dbb2650 Compare January 16, 2023 00:57
@notgull notgull force-pushed the x11-libxcb branch 2 times, most recently from b95ccbc to 638a7a1 Compare January 19, 2023 22:16
@notgull notgull mentioned this pull request Jan 20, 2023
Closed
@lukaslihotzki lukaslihotzki mentioned this pull request Jan 25, 2023
Merged
5 tasks
@notgull
Copy link
Member Author

notgull commented Jan 27, 2023

What is the current status of this PR? Are there any blockers that I can resolve?

@kchibisov
Copy link
Member

I need to find time and read through it. Also it should have feature parity with the current master. Have you resolved the IME issues?

@notgull
Copy link
Member Author

notgull commented Jan 27, 2023

Not yet. I'll go ahead and add that the next time I have time.

@notgull notgull mentioned this pull request Jan 27, 2023
Closed
@notgull
Add x11rb and replace some Xlib functions
5401396 
Sub-commit messages:

Fix 32-bit builds

Fix clippy warnings

Clean up + better atom handling

Cleanup

fmt

x11rb should be optional

Make sure x11rb doesn't appear in public interface

Rebase on master

Tree shaking

fmt
@notgull notgull force-pushed the x11-libxcb branch 4 times, most recently from 06b511a to 19f04c9 Compare January 28, 2023 18:49
@notgull notgull force-pushed the x11-libxcb branch 2 times, most recently from 74251a3 to 6a72687 Compare January 30, 2023 18:55
@notgull notgull marked this pull request as draft January 30, 2023 18:56
@notgull
Copy link
Member Author

notgull commented Jan 30, 2023

Putting this back into the draft phase as this is now blocked on the next xim-rs release. Also I should probably implement XKB support.

@kchibisov
Copy link
Member

We already have XKB support on the keyboard v2 (or new-keyboard) branch. In general you can go without XKB and we can merge it later on, unless you really want xkb now for whatever reason. Some of the XKB stuff is present in wayland backend already for example.

@notgull notgull mentioned this pull request Jan 30, 2023
Closed
@notgull
Copy link
Member Author

notgull commented Jan 30, 2023

In general you can go without XKB and we can merge it later on, unless you really want xkb now for whatever reason.

Alright, if you say so. I'll replace it with a naïve algorithm for now.

@madsmtm
Copy link
Member

madsmtm commented Jan 31, 2023

FYI we just got #2662, which I think we should focus on first - so you might have to do redo some work once that lands @notgull

@notgull
Copy link
Member Author

notgull commented Jan 31, 2023

Don't worry, I've been continuously rebasing this branch off of master anyways. As long as it uses xkbcommon instead of Xlib's opaque solution it should be easy to integrate anyways.

@notgull
Copy link
Member Author

notgull commented Feb 1, 2023

Actually, I'd rather just wait for #2662 to be merged anyhow, since it seems this PR is blocked on that one.

@notgull notgull mentioned this pull request Feb 6, 2023
Closed
5 tasks
@kchibisov kchibisov mentioned this pull request Feb 15, 2023
Closed
2 tasks
This was referenced Feb 20, 2023
Merged
Closed
Closed
@madsmtm madsmtm linked an issue Feb 25, 2023 that may be closed by this pull request
Add support for XCB #5
Closed
@ids1024 ids1024 mentioned this pull request Apr 24, 2023
Open
@notgull notgull mentioned this pull request May 29, 2023
Merged
5 tasks
@notgull
Copy link
Member Author

notgull commented May 31, 2023

Supersedes by #2825

@notgull notgull closed this May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kchibisov kchibisov Awaiting requested review from kchibisov kchibisov is a code owner

Assignees
No one assigned
Labels
DS - x11 S - enhancement Wouldn't this be the coolest? S - maintenance Repaying technical debt
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for XCB
4 participants
@notgull @ids1024 @kchibisov @madsmtm