From 05b658114750e9b63717ec3d05b316a927d8378e Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Mon, 10 Jun 2024 14:08:16 +0200 Subject: [PATCH] linux/x11: handle XIM events sync to reduce lag (#12840) This helps with the problem of keyboard input feeling laggy when the event loop is under load. What would previously happen is: - N events from X11 arrive - N events get forwarded to XIM - N events are handled in N iterations of the event loop (sadly, yes: we only seem to be getting back one `ClientMessage` per poll from XCB connection) - Each event is pushed into the channel - N event loop iterations are needed to get the events off the channel and handle them With this change, we get rid of the last 2 steps: instead of pushing the event onto a channel, we store it on the XIM handler itself, and then work it off synchronously. Usually one shouldn't block the event loop, but I think in this case - user input! - it's better to handle the events directly instead of re-enqueuing them again in a channel, where they can accumulate and need multiple iterations of the loop to be worked off. This does *not* fix the problem of input feeling choppy/slower when the system is under load, but it makes the behavior now feel exactly the same as when XIM is disabled. I also think the code is easier to understand since it's more straightforward. Release Notes: - N/A --- crates/gpui/src/platform/linux/x11/client.rs | 54 ++++++++++--------- .../src/platform/linux/x11/xim_handler.rs | 37 +++++-------- 2 files changed, 42 insertions(+), 49 deletions(-) diff --git a/crates/gpui/src/platform/linux/x11/client.rs b/crates/gpui/src/platform/linux/x11/client.rs index 57295ee772..bd3d3e3e6b 100644 --- a/crates/gpui/src/platform/linux/x11/client.rs +++ b/crates/gpui/src/platform/linux/x11/client.rs @@ -5,7 +5,7 @@ use std::rc::{Rc, Weak}; use std::time::{Duration, Instant}; use calloop::generic::{FdWrapper, Generic}; -use calloop::{channel, EventLoop, LoopHandle, RegistrationToken}; +use calloop::{EventLoop, LoopHandle, RegistrationToken}; use collections::HashMap; use copypasta::x11_clipboard::{Clipboard, Primary, X11ClipboardContext}; @@ -282,11 +282,9 @@ impl X11Client { let xcb_connection = Rc::new(xcb_connection); - let (xim_tx, xim_rx) = channel::channel::(); - let ximc = X11rbClient::init(Rc::clone(&xcb_connection), x_root_index, None).ok(); let xim_handler = if ximc.is_some() { - Some(XimHandler::new(xim_tx)) + Some(XimHandler::new()) } else { None }; @@ -311,10 +309,12 @@ impl X11Client { client.handle_event(event); continue; } + let mut ximc = state.ximc.take().unwrap(); let mut xim_handler = state.xim_handler.take().unwrap(); let xim_connected = xim_handler.connected; drop(state); + let xim_filtered = match ximc.filter_event(&event, &mut xim_handler) { Ok(handled) => handled, Err(err) => { @@ -322,46 +322,34 @@ impl X11Client { false } }; + let xim_callback_event = xim_handler.last_callback_event.take(); + let mut state = client.0.borrow_mut(); state.ximc = Some(ximc); state.xim_handler = Some(xim_handler); drop(state); + + if let Some(event) = xim_callback_event { + client.handle_xim_callback_event(event); + } + if xim_filtered { continue; } + if xim_connected { client.xim_handle_event(event); } else { client.handle_event(event); } } + Ok(calloop::PostAction::Continue) } }, ) .expect("Failed to initialize x11 event source"); - handle - .insert_source(xim_rx, { - move |chan_event, _, client| match chan_event { - channel::Event::Msg(xim_event) => { - match xim_event { - XimCallbackEvent::XimXEvent(event) => { - client.handle_event(event); - } - XimCallbackEvent::XimCommitEvent(window, text) => { - client.xim_handle_commit(window, text); - } - XimCallbackEvent::XimPreeditEvent(window, text) => { - client.xim_handle_preedit(window, text); - } - }; - } - channel::Event::Closed => { - log::error!("XIM Event Sender dropped") - } - } - }) - .expect("Failed to initialize XIM event source"); + handle .insert_source(XDPEventSource::new(&common.background_executor), { move |event, _, client| match event { @@ -801,6 +789,20 @@ impl X11Client { Some(()) } + fn handle_xim_callback_event(&self, event: XimCallbackEvent) { + match event { + XimCallbackEvent::XimXEvent(event) => { + self.handle_event(event); + } + XimCallbackEvent::XimCommitEvent(window, text) => { + self.xim_handle_commit(window, text); + } + XimCallbackEvent::XimPreeditEvent(window, text) => { + self.xim_handle_preedit(window, text); + } + }; + } + fn xim_handle_event(&self, event: Event) -> Option<()> { match event { Event::KeyPress(event) | Event::KeyRelease(event) => { diff --git a/crates/gpui/src/platform/linux/x11/xim_handler.rs b/crates/gpui/src/platform/linux/x11/xim_handler.rs index 5900a19020..89b1ec3feb 100644 --- a/crates/gpui/src/platform/linux/x11/xim_handler.rs +++ b/crates/gpui/src/platform/linux/x11/xim_handler.rs @@ -1,7 +1,5 @@ use std::default::Default; -use calloop::channel; - use x11rb::protocol::{xproto, Event}; use xim::{AHashMap, AttributeName, Client, ClientError, ClientHandler, InputStyle}; @@ -14,19 +12,19 @@ pub enum XimCallbackEvent { pub struct XimHandler { pub im_id: u16, pub ic_id: u16, - pub xim_tx: channel::Sender, pub connected: bool, pub window: xproto::Window, + pub last_callback_event: Option, } impl XimHandler { - pub fn new(xim_tx: channel::Sender) -> Self { + pub fn new() -> Self { Self { im_id: Default::default(), ic_id: Default::default(), - xim_tx, connected: false, window: Default::default(), + last_callback_event: None, } } } @@ -80,12 +78,10 @@ impl> ClientHandler for XimHandler _input_context_id: u16, text: &str, ) -> Result<(), ClientError> { - self.xim_tx - .send(XimCallbackEvent::XimCommitEvent( - self.window, - String::from(text), - )) - .ok(); + self.last_callback_event = Some(XimCallbackEvent::XimCommitEvent( + self.window, + String::from(text), + )); Ok(()) } @@ -99,14 +95,11 @@ impl> ClientHandler for XimHandler ) -> Result<(), ClientError> { match xev.response_type { x11rb::protocol::xproto::KEY_PRESS_EVENT => { - self.xim_tx - .send(XimCallbackEvent::XimXEvent(Event::KeyPress(xev))) - .ok(); + self.last_callback_event = Some(XimCallbackEvent::XimXEvent(Event::KeyPress(xev))); } x11rb::protocol::xproto::KEY_RELEASE_EVENT => { - self.xim_tx - .send(XimCallbackEvent::XimXEvent(Event::KeyRelease(xev))) - .ok(); + self.last_callback_event = + Some(XimCallbackEvent::XimXEvent(Event::KeyRelease(xev))); } _ => {} } @@ -145,12 +138,10 @@ impl> ClientHandler for XimHandler // XIMPrimary, XIMHighlight, XIMSecondary, XIMTertiary are not specified, // but interchangeable as above // Currently there's no way to support these. - self.xim_tx - .send(XimCallbackEvent::XimPreeditEvent( - self.window, - String::from(preedit_string), - )) - .ok(); + self.last_callback_event = Some(XimCallbackEvent::XimPreeditEvent( + self.window, + String::from(preedit_string), + )); Ok(()) } }