-
Notifications
You must be signed in to change notification settings - Fork 901
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
Use x11rb for event handling #3122
Conversation
f1c096a
to
7a585a5
Compare
Event handling functionality looks sound to me. I haven't tested IME yet. |
Tested IME on Fedora 38 |
@Riey Can you review this and make sure I'm using XIM right? |
@notgull Is there an example code to check preedit and position? |
I'm not sure what you're asking for, but here is the code responsible for handling pre-edits: winit/src/platform_impl/linux/x11/ime.rs Lines 648 to 767 in 46bb9e5
|
you can also, always try to do things with |
LGTM. |
This is broken until we start using x11rb for event lookups. Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
I don't know what this actually does, but it can't hurt, right? Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
display, | ||
x11_dl::xlib_xcb::XEventQueueOwner::XCBOwnsEventQueue, | ||
); | ||
} |
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.
Since the commit message for this one says
I don't know what this actually does, but it can't hurt, right?
"Normally", all events and errors go through Xlib. It does some gymnastics internally to ensure that before the reply for a request is handled, everything that happens before is done: Events are in its internal event queue and errors were passed to the error handler. This has the effect of removing all events/errors from XCB's queue.
Thus, normally you cannot use xcb_poll/wait_for_event()
when Xlib is involved, since Xlib might already have "eaten" an event and put it into its own queue.
The call you are doing here just tells Xlib "do not mess with the event queue, kthxbye". It still will use the flag XCB_REQUEST_CHECKED
flags when sending requests to that any errors caused "through" Xlib are reported to Xlib via xcb_wait_for_reply()
and do not end up in XCB's event queue, but it will no longer call xcb_poll_for_event()
or xcb_wait_for_event()
.
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.
Great, thanks for clarifying! Yeah I figured it was somehow telling Xlib to not interfere with libxcb. Glad to confirm it.
ime.set_ime_allowed(window_id, allowed); | ||
} | ||
X11Event::RandrNotify(_) => { | ||
self.process_dpi_change(&mut callback); |
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.
I came here for another reason, but then noticed this bit. I think this is wrong.
The old code does:
if event_type == self.randr_event_offset as c_int {
Imagine a + 0
in there and check that "event zero" from randr is RRScreenChangeNotify
: https://docs.rs/x11-dl/latest/x11_dl/xrandr/constant.RRScreenChangeNotify.html
RandrNotify is event 1: https://docs.rs/x11-dl/latest/x11_dl/xrandr/constant.RRNotify.html
Closing as per chat discussion |
CHANGELOG.md
if knowledge of this change could be valuable to usersSwitches out the previous event handling logic for x11rb-based logic. The last large hurdle before getting rid of Xlib. As part of the process, also gets rid of Xlib's IME and replaces it with the
xim
crate.Draft because I have not tested this in the slightest.