From aa1b2d74ee03395a449f5210e034ec145798be15 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 17 Jun 2025 19:40:17 -0600 Subject: [PATCH] x11: Improve error handling (#32913) Continuing this work from a while back in #21079, now greatly aided by agent + sonnet 4. With this change, there are now only a few spots that explicitly panic, though errors during initialization will panic. Motivation was this recent user panic in `handle_event`, figured fixing all this use of unwrap was a great use of the agent. > called `Result::unwrap()` on an `Err` value: X11 GetProperty for _NET_WM_STATE failed. Release Notes: - N/A --- crates/gpui/src/platform.rs | 9 +- crates/gpui/src/platform/linux/x11/client.rs | 572 +++++++++++-------- crates/gpui/src/platform/linux/x11/window.rs | 130 +++-- 3 files changed, 407 insertions(+), 304 deletions(-) diff --git a/crates/gpui/src/platform.rs b/crates/gpui/src/platform.rs index 66068db560e22f0f6fcaf3ba4c9dffa100975138..ac21c5dea12c783790d3877735a7074f7dbd9c95 100644 --- a/crates/gpui/src/platform.rs +++ b/crates/gpui/src/platform.rs @@ -93,6 +93,9 @@ pub(crate) fn current_platform(headless: bool) -> Rc { #[cfg(any(target_os = "linux", target_os = "freebsd"))] pub(crate) fn current_platform(headless: bool) -> Rc { + #[cfg(feature = "x11")] + use anyhow::Context as _; + if headless { return Rc::new(HeadlessClient::new()); } @@ -102,7 +105,11 @@ pub(crate) fn current_platform(headless: bool) -> Rc { "Wayland" => Rc::new(WaylandClient::new()), #[cfg(feature = "x11")] - "X11" => Rc::new(X11Client::new()), + "X11" => Rc::new( + X11Client::new() + .context("Failed to initialize X11 client.") + .unwrap(), + ), "Headless" => Rc::new(HeadlessClient::new()), _ => unreachable!(), diff --git a/crates/gpui/src/platform/linux/x11/client.rs b/crates/gpui/src/platform/linux/x11/client.rs index d2884f37c7cc9b6c3b3e19c16147f507bb7b5fb9..dddff8566102be44b63680999be7bed30439e7a5 100644 --- a/crates/gpui/src/platform/linux/x11/client.rs +++ b/crates/gpui/src/platform/linux/x11/client.rs @@ -17,6 +17,7 @@ use calloop::{ use collections::HashMap; use futures::channel::oneshot; use http_client::Url; +use log::Level; use smallvec::SmallVec; use util::ResultExt; @@ -41,12 +42,12 @@ use xkbc::x11::ffi::{XKB_X11_MIN_MAJOR_XKB_VERSION, XKB_X11_MIN_MINOR_XKB_VERSIO use xkbcommon::xkb::{self as xkbc, LayoutIndex, ModMask, STATE_LAYOUT_EFFECTIVE}; use super::{ - ButtonOrScroll, ScrollDirection, button_or_scroll_from_event_detail, + ButtonOrScroll, ScrollDirection, X11Display, X11WindowStatePtr, XcbAtoms, XimCallbackEvent, + XimHandler, button_or_scroll_from_event_detail, check_reply, clipboard::{self, Clipboard}, - get_valuator_axis_index, modifiers_from_state, pressed_button_from_mask, + get_reply, get_valuator_axis_index, handle_connection_error, modifiers_from_state, + pressed_button_from_mask, }; -use super::{X11Display, X11WindowStatePtr, XcbAtoms}; -use super::{XimCallbackEvent, XimHandler}; use crate::platform::{ LinuxCommon, PlatformWindow, @@ -230,12 +231,14 @@ pub struct X11ClientState { pub struct X11ClientStatePtr(pub Weak>); impl X11ClientStatePtr { - fn get_client(&self) -> X11Client { - X11Client(self.0.upgrade().expect("client already dropped")) + fn get_client(&self) -> Option { + self.0.upgrade().map(X11Client) } pub fn drop_window(&self, x_window: u32) { - let client = self.get_client(); + let Some(client) = self.get_client() else { + return; + }; let mut state = client.0.borrow_mut(); if let Some(window_ref) = state.windows.remove(&x_window) { @@ -262,14 +265,23 @@ impl X11ClientStatePtr { } pub fn update_ime_position(&self, bounds: Bounds) { - let client = self.get_client(); + let Some(client) = self.get_client() else { + return; + }; let mut state = client.0.borrow_mut(); if state.composing || state.ximc.is_none() { return; } - let mut ximc = state.ximc.take().unwrap(); - let xim_handler = state.xim_handler.take().unwrap(); + let Some(mut ximc) = state.ximc.take() else { + log::error!("bug: xim connection not set"); + return; + }; + let Some(xim_handler) = state.xim_handler.take() else { + log::error!("bug: xim handler not set"); + state.ximc = Some(ximc); + return; + }; let ic_attributes = ximc .build_ic_attributes() .push( @@ -300,8 +312,8 @@ impl X11ClientStatePtr { pub(crate) struct X11Client(Rc>); impl X11Client { - pub(crate) fn new() -> Self { - let event_loop = EventLoop::try_new().unwrap(); + pub(crate) fn new() -> anyhow::Result { + let event_loop = EventLoop::try_new()?; let (common, main_receiver) = LinuxCommon::new(event_loop.get_signal()); @@ -321,39 +333,34 @@ impl X11Client { } } }) - .unwrap(); - - let (xcb_connection, x_root_index) = XCBConnection::connect(None).unwrap(); - xcb_connection - .prefetch_extension_information(xkb::X11_EXTENSION_NAME) - .unwrap(); - xcb_connection - .prefetch_extension_information(randr::X11_EXTENSION_NAME) - .unwrap(); - xcb_connection - .prefetch_extension_information(render::X11_EXTENSION_NAME) - .unwrap(); - xcb_connection - .prefetch_extension_information(xinput::X11_EXTENSION_NAME) - .unwrap(); + .map_err(|err| { + anyhow!("Failed to initialize event loop handling of foreground tasks: {err:?}") + })?; + + let (xcb_connection, x_root_index) = XCBConnection::connect(None)?; + xcb_connection.prefetch_extension_information(xkb::X11_EXTENSION_NAME)?; + xcb_connection.prefetch_extension_information(randr::X11_EXTENSION_NAME)?; + xcb_connection.prefetch_extension_information(render::X11_EXTENSION_NAME)?; + xcb_connection.prefetch_extension_information(xinput::X11_EXTENSION_NAME)?; // Announce to X server that XInput up to 2.1 is supported. To increase this to 2.2 and // beyond, support for touch events would need to be added. - let xinput_version = xcb_connection - .xinput_xi_query_version(2, 1) - .unwrap() - .reply() - .unwrap(); - // XInput 1.x is not supported. + let xinput_version = get_reply( + || "XInput XiQueryVersion failed", + xcb_connection.xinput_xi_query_version(2, 1), + )?; assert!( xinput_version.major_version >= 2, "XInput version >= 2 required." ); let pointer_device_states = - get_new_pointer_device_states(&xcb_connection, &BTreeMap::new()); + current_pointer_device_states(&xcb_connection, &BTreeMap::new()).unwrap_or_default(); - let atoms = XcbAtoms::new(&xcb_connection).unwrap().reply().unwrap(); + let atoms = XcbAtoms::new(&xcb_connection) + .context("Failed to get XCB atoms")? + .reply() + .context("Failed to get XCB atoms")?; let root = xcb_connection.setup().roots[0].root; let compositor_present = check_compositor_present(&xcb_connection, root); @@ -366,11 +373,11 @@ impl X11Client { gtk_frame_extents_supported ); - let xkb = xcb_connection - .xkb_use_extension(XKB_X11_MIN_MAJOR_XKB_VERSION, XKB_X11_MIN_MINOR_XKB_VERSION) - .unwrap() - .reply() - .unwrap(); + let xkb = get_reply( + || "Failed to initialize XKB extension", + xcb_connection + .xkb_use_extension(XKB_X11_MIN_MAJOR_XKB_VERSION, XKB_X11_MIN_MINOR_XKB_VERSION), + )?; let events = xkb::EventType::STATE_NOTIFY | xkb::EventType::MAP_NOTIFY @@ -383,16 +390,17 @@ impl X11Client { | xkb::MapPart::KEY_BEHAVIORS | xkb::MapPart::VIRTUAL_MODS | xkb::MapPart::VIRTUAL_MOD_MAP; - xcb_connection - .xkb_select_events( + check_reply( + || "Failed to select XKB events", + xcb_connection.xkb_select_events( xkb::ID::USE_CORE_KBD.into(), 0u8.into(), events, map_notify_parts, map_notify_parts, &xkb::SelectEventsAux::new(), - ) - .unwrap(); + ), + )?; assert!(xkb.supported); let xkb_context = xkbc::Context::new(xkbc::CONTEXT_NO_FLAGS); @@ -414,9 +422,10 @@ impl X11Client { .to_string(); let keyboard_layout = LinuxKeyboardLayout::new(layout_name.into()); - let gpu_context = BladeContext::new().expect("Unable to init GPU context"); + let gpu_context = BladeContext::new().context("Unable to init GPU context")?; - let resource_database = x11rb::resource_manager::new_from_default(&xcb_connection).unwrap(); + let resource_database = x11rb::resource_manager::new_from_default(&xcb_connection) + .context("Failed to create resource database")?; let scale_factor = resource_database .get_value("Xft.dpi", "Xft.dpi") .ok() @@ -424,11 +433,11 @@ impl X11Client { .map(|dpi: f32| dpi / 96.0) .unwrap_or(1.0); let cursor_handle = cursor::Handle::new(&xcb_connection, x_root_index, &resource_database) - .unwrap() + .context("Failed to initialize cursor theme handler")? .reply() - .unwrap(); + .context("Failed to initialize cursor theme handler")?; - let clipboard = Clipboard::new().unwrap(); + let clipboard = Clipboard::new().context("Failed to initialize clipboard")?; let xcb_connection = Rc::new(xcb_connection); @@ -457,7 +466,7 @@ impl X11Client { } }, ) - .expect("Failed to initialize x11 event source"); + .map_err(|err| anyhow!("Failed to initialize X11 event source: {err:?}"))?; handle .insert_source(XDPEventSource::new(&common.background_executor), { @@ -473,9 +482,9 @@ impl X11Client { } } }) - .unwrap(); + .map_err(|err| anyhow!("Failed to initialize XDP event source: {err:?}"))?; - X11Client(Rc::new(RefCell::new(X11ClientState { + Ok(X11Client(Rc::new(RefCell::new(X11ClientState { modifiers: Modifiers::default(), capslock: Capslock::default(), last_modifiers_changed_event: Modifiers::default(), @@ -520,7 +529,7 @@ impl X11Client { clipboard, clipboard_item: None, xdnd_state: Xdnd::default(), - }))) + })))) } pub fn process_x11_events( @@ -606,8 +615,9 @@ impl X11Client { Ok(None) => { break; } - Err(e) => { - log::warn!("error polling for X11 events: {e:?}"); + Err(err) => { + let err = handle_connection_error(err); + log::warn!("error while polling for X11 events: {err:?}"); break; } } @@ -633,14 +643,15 @@ impl X11Client { for event in events.into_iter() { let mut state = self.0.borrow_mut(); - if state.ximc.is_none() || state.xim_handler.is_none() { + if !state.has_xim() { drop(state); self.handle_event(event); continue; } - let mut ximc = state.ximc.take().unwrap(); - let mut xim_handler = state.xim_handler.take().unwrap(); + let Some((mut ximc, mut xim_handler)) = state.take_xim() else { + continue; + }; let xim_connected = xim_handler.connected; drop(state); @@ -654,8 +665,7 @@ impl X11Client { let xim_callback_event = xim_handler.last_callback_event.take(); let mut state = self.0.borrow_mut(); - state.ximc = Some(ximc); - state.xim_handler = Some(xim_handler); + state.restore_xim(ximc, xim_handler); drop(state); if let Some(event) = xim_callback_event { @@ -678,12 +688,13 @@ impl X11Client { pub fn enable_ime(&self) { let mut state = self.0.borrow_mut(); - if state.ximc.is_none() { + if !state.has_xim() { return; } - let mut ximc = state.ximc.take().unwrap(); - let mut xim_handler = state.xim_handler.take().unwrap(); + let Some((mut ximc, mut xim_handler)) = state.take_xim() else { + return; + }; let mut ic_attributes = ximc .build_ic_attributes() .push(AttributeName::InputStyle, InputStyle::PREEDIT_CALLBACKS) @@ -693,7 +704,13 @@ impl X11Client { let window_id = state.keyboard_focused_window; drop(state); if let Some(window_id) = window_id { - let window = self.get_window(window_id).unwrap(); + let Some(window) = self.get_window(window_id) else { + log::error!("Failed to get window for IME positioning"); + let mut state = self.0.borrow_mut(); + state.ximc = Some(ximc); + state.xim_handler = Some(xim_handler); + return; + }; if let Some(area) = window.get_ime_area() { ic_attributes = ic_attributes.nested_list(xim::AttributeName::PreeditAttributes, |b| { @@ -709,17 +726,19 @@ impl X11Client { } ximc.create_ic(xim_handler.im_id, ic_attributes.build()) .ok(); - state = self.0.borrow_mut(); - state.xim_handler = Some(xim_handler); - state.ximc = Some(ximc); + let mut state = self.0.borrow_mut(); + state.restore_xim(ximc, xim_handler); } pub fn reset_ime(&self) { let mut state = self.0.borrow_mut(); state.composing = false; if let Some(mut ximc) = state.ximc.take() { - let xim_handler = state.xim_handler.as_ref().unwrap(); - ximc.reset_ic(xim_handler.im_id, xim_handler.ic_id).ok(); + if let Some(xim_handler) = state.xim_handler.as_ref() { + ximc.reset_ic(xim_handler.im_id, xim_handler.ic_id).ok(); + } else { + log::error!("bug: xim handler not set in reset_ime"); + } state.ximc = Some(ximc); } } @@ -799,26 +818,25 @@ impl X11Client { window.handle_input(PlatformInput::FileDrop(FileDropEvent::Exited {})); self.0.borrow_mut().xdnd_state = Xdnd::default(); } else if event.type_ == state.atoms.XdndPosition { - if let Ok(pos) = state - .xcb_connection - .query_pointer(event.window) - .unwrap() - .reply() - { + if let Ok(pos) = get_reply( + || "Failed to query pointer position", + state.xcb_connection.query_pointer(event.window), + ) { state.xdnd_state.position = Point::new(Pixels(pos.win_x as f32), Pixels(pos.win_y as f32)); } if !state.xdnd_state.retrieved { - state - .xcb_connection - .convert_selection( + check_reply( + || "Failed to convert selection for drag and drop", + state.xcb_connection.convert_selection( event.window, state.atoms.XdndSelection, state.xdnd_state.drag_type, state.atoms.XDND_DATA, arg3, - ) - .unwrap(); + ), + ) + .log_err(); } xdnd_send_status( &state.xcb_connection, @@ -848,35 +866,37 @@ impl X11Client { Event::SelectionNotify(event) => { let window = self.get_window(event.requestor)?; let mut state = self.0.borrow_mut(); - let property = state.xcb_connection.get_property( - false, - event.requestor, - state.atoms.XDND_DATA, - AtomEnum::ANY, - 0, - 1024, - ); - if property.as_ref().log_err().is_none() { + let reply = get_reply( + || "Failed to get XDND_DATA", + state.xcb_connection.get_property( + false, + event.requestor, + state.atoms.XDND_DATA, + AtomEnum::ANY, + 0, + 1024, + ), + ) + .log_err(); + let Some(reply) = reply else { return Some(()); - } - if let Ok(reply) = property.unwrap().reply() { - match str::from_utf8(&reply.value) { - Ok(file_list) => { - let paths: SmallVec<[_; 2]> = file_list - .lines() - .filter_map(|path| Url::parse(path).log_err()) - .filter_map(|url| url.to_file_path().log_err()) - .collect(); - let input = PlatformInput::FileDrop(FileDropEvent::Entered { - position: state.xdnd_state.position, - paths: crate::ExternalPaths(paths), - }); - drop(state); - window.handle_input(input); - self.0.borrow_mut().xdnd_state.retrieved = true; - } - Err(_) => {} + }; + match str::from_utf8(&reply.value) { + Ok(file_list) => { + let paths: SmallVec<[_; 2]> = file_list + .lines() + .filter_map(|path| Url::parse(path).log_err()) + .filter_map(|url| url.to_file_path().log_err()) + .collect(); + let input = PlatformInput::FileDrop(FileDropEvent::Entered { + position: state.xdnd_state.position, + paths: crate::ExternalPaths(paths), + }); + drop(state); + window.handle_input(input); + self.0.borrow_mut().xdnd_state.retrieved = true; } + Err(_) => {} } } Event::ConfigureNotify(event) => { @@ -891,11 +911,17 @@ impl X11Client { }, }; let window = self.get_window(event.window)?; - window.configure(bounds).unwrap(); + window + .set_bounds(bounds) + .context("X11: Failed to set window bounds") + .log_err(); } Event::PropertyNotify(event) => { let window = self.get_window(event.window)?; - window.property_notify(event).unwrap(); + window + .property_notify(event) + .context("X11: Failed to handle property notify") + .log_err(); } Event::FocusIn(event) => { let window = self.get_window(event.event)?; @@ -1262,10 +1288,12 @@ impl X11Client { state.pointer_device_states.remove(&info.deviceid); } } - state.pointer_device_states = get_new_pointer_device_states( + if let Some(pointer_device_states) = current_pointer_device_states( &state.xcb_connection, &state.pointer_device_states, - ); + ) { + state.pointer_device_states = pointer_device_states; + } } Event::XinputDeviceChanged(event) => { let mut state = self.0.borrow_mut(); @@ -1302,8 +1330,7 @@ impl X11Client { state.modifiers, event.detail.into(), )); - let mut ximc = state.ximc.take().unwrap(); - let mut xim_handler = state.xim_handler.take().unwrap(); + let (mut ximc, mut xim_handler) = state.take_xim()?; drop(state); xim_handler.window = event.event; ximc.forward_event( @@ -1312,10 +1339,10 @@ impl X11Client { xim::ForwardEventFlag::empty(), &event, ) - .unwrap(); + .context("X11: Failed to forward XIM event") + .log_err(); let mut state = self.0.borrow_mut(); - state.ximc = Some(ximc); - state.xim_handler = Some(xim_handler); + state.restore_xim(ximc, xim_handler); drop(state); } event => { @@ -1326,7 +1353,10 @@ impl X11Client { } fn xim_handle_commit(&self, window: xproto::Window, text: String) -> Option<()> { - let window = self.get_window(window).unwrap(); + let Some(window) = self.get_window(window) else { + log::error!("bug: Failed to get window for XIM commit"); + return None; + }; let mut state = self.0.borrow_mut(); let keystroke = state.pre_key_char_down.take(); state.composing = false; @@ -1343,11 +1373,13 @@ impl X11Client { } fn xim_handle_preedit(&self, window: xproto::Window, text: String) -> Option<()> { - let window = self.get_window(window).unwrap(); + let Some(window) = self.get_window(window) else { + log::error!("bug: Failed to get window for XIM preedit"); + return None; + }; let mut state = self.0.borrow_mut(); - let mut ximc = state.ximc.take().unwrap(); - let mut xim_handler = state.xim_handler.take().unwrap(); + let (mut ximc, mut xim_handler) = state.take_xim()?; state.composing = !text.is_empty(); drop(state); window.handle_ime_preedit(text); @@ -1375,8 +1407,7 @@ impl X11Client { .ok(); } let mut state = self.0.borrow_mut(); - state.ximc = Some(ximc); - state.xim_handler = Some(xim_handler); + state.restore_xim(ximc, xim_handler); drop(state); Some(()) } @@ -1429,15 +1460,13 @@ impl LinuxClient for X11Client { fn primary_display(&self) -> Option> { let state = self.0.borrow(); - - Some(Rc::new( - X11Display::new( - &state.xcb_connection, - state.scale_factor, - state.x_root_index, - ) - .expect("There should always be a root index"), - )) + X11Display::new( + &state.xcb_connection, + state.scale_factor, + state.x_root_index, + ) + .log_err() + .map(|display| Rc::new(display) as Rc) } fn display(&self, id: DisplayId) -> Option> { @@ -1464,7 +1493,10 @@ impl LinuxClient for X11Client { params: WindowParams, ) -> anyhow::Result> { let mut state = self.0.borrow_mut(); - let x_window = state.xcb_connection.generate_id().unwrap(); + let x_window = state + .xcb_connection + .generate_id() + .context("X11: Failed to generate window ID")?; let window = X11Window::new( handle, @@ -1480,16 +1512,17 @@ impl LinuxClient for X11Client { state.scale_factor, state.common.appearance, )?; - state - .xcb_connection - .change_property32( + check_reply( + || "Failed to set XdndAware property", + state.xcb_connection.change_property32( xproto::PropMode::REPLACE, x_window, state.atoms.XdndAware, state.atoms.XA_ATOM, &[5], - ) - .unwrap(); + ), + ) + .log_err(); let window_ref = WindowRef { window: window.0.clone(), @@ -1532,7 +1565,7 @@ impl LinuxClient for X11Client { ) .anyhow() .and_then(|cookie| cookie.check().anyhow()) - .context("setting cursor style") + .context("X11: Failed to set cursor style") .log_err(); } @@ -1555,7 +1588,7 @@ impl LinuxClient for X11Client { clipboard::ClipboardKind::Primary, clipboard::WaitConfig::None, ) - .context("Failed to write to clipboard (primary)") + .context("X11 Failed to write to clipboard (primary)") .log_with_level(log::Level::Debug); } @@ -1568,7 +1601,7 @@ impl LinuxClient for X11Client { clipboard::ClipboardKind::Clipboard, clipboard::WaitConfig::None, ) - .context("Failed to write to clipboard (clipboard)") + .context("X11: Failed to write to clipboard (clipboard)") .log_with_level(log::Level::Debug); state.clipboard_item.replace(item); } @@ -1578,7 +1611,7 @@ impl LinuxClient for X11Client { return state .clipboard .get_any(clipboard::ClipboardKind::Primary) - .context("Failed to read from clipboard (primary)") + .context("X11: Failed to read from clipboard (primary)") .log_with_level(log::Level::Debug); } @@ -1595,17 +1628,21 @@ impl LinuxClient for X11Client { return state .clipboard .get_any(clipboard::ClipboardKind::Clipboard) - .context("Failed to read from clipboard (clipboard)") + .context("X11: Failed to read from clipboard (clipboard)") .log_with_level(log::Level::Debug); } fn run(&self) { - let mut event_loop = self + let Some(mut event_loop) = self .0 .borrow_mut() .event_loop .take() - .expect("App is already running"); + .context("X11Client::run called but it's already running") + .log_err() + else { + return; + }; event_loop.run(None, &mut self.clone(), |_| {}).log_err(); } @@ -1641,7 +1678,7 @@ impl LinuxClient for X11Client { let window_ids = reply .value .chunks_exact(4) - .map(|chunk| u32::from_ne_bytes(chunk.try_into().unwrap())) + .filter_map(|chunk| chunk.try_into().ok().map(u32::from_ne_bytes)) .collect::>(); let mut handles = Vec::new(); @@ -1664,6 +1701,30 @@ impl LinuxClient for X11Client { } impl X11ClientState { + fn has_xim(&self) -> bool { + self.ximc.is_some() && self.xim_handler.is_some() + } + + fn take_xim(&mut self) -> Option<(X11rbClient>, XimHandler)> { + let ximc = self + .ximc + .take() + .ok_or(anyhow!("bug: XIM connection not set")) + .log_err()?; + if let Some(xim_handler) = self.xim_handler.take() { + Some((ximc, xim_handler)) + } else { + self.ximc = Some(ximc); + log::error!("bug: XIM handler not set"); + None + } + } + + fn restore_xim(&mut self, ximc: X11rbClient>, xim_handler: XimHandler) { + self.ximc = Some(ximc); + self.xim_handler = Some(xim_handler); + } + fn update_refresh_loop(&mut self, x_window: xproto::Window) { let Some(window_ref) = self.windows.get_mut(&x_window) else { return; @@ -1697,35 +1758,41 @@ impl X11ClientState { }); } (true, None) => { - let screen_resources = self - .xcb_connection - .randr_get_screen_resources_current(x_window) - .unwrap() - .reply() - .expect("Could not find available screens"); + let Some(screen_resources) = get_reply( + || "Failed to get screen resources", + self.xcb_connection + .randr_get_screen_resources_current(x_window), + ) + .log_err() else { + return; + }; // Ideally this would be re-queried when the window changes screens, but there // doesn't seem to be an efficient / straightforward way to do this. Should also be // updated when screen configurations change. - let refresh_rate = mode_refresh_rate( + let mode_info = screen_resources.crtcs.iter().find_map(|crtc| { + let crtc_info = self + .xcb_connection + .randr_get_crtc_info(*crtc, x11rb::CURRENT_TIME) + .ok()? + .reply() + .ok()?; + screen_resources - .crtcs + .modes .iter() - .find_map(|crtc| { - let crtc_info = self - .xcb_connection - .randr_get_crtc_info(*crtc, x11rb::CURRENT_TIME) - .ok()? - .reply() - .ok()?; - - screen_resources - .modes - .iter() - .find(|m| m.id == crtc_info.mode) - }) - .expect("Unable to find screen refresh rate"), - ); + .find(|m| m.id == crtc_info.mode) + }); + let refresh_rate = match mode_info { + Some(mode_info) => mode_refresh_rate(mode_info), + None => { + log::error!( + "Failed to get screen mode info from xrandr, \ + defaulting to 60hz refresh rate." + ); + Duration::from_micros(1_000_000 / 60) + } + }; let event_loop_token = self.start_refresh_loop(x_window, refresh_rate); let Some(window_ref) = self.windows.get_mut(&x_window) else { @@ -1772,7 +1839,7 @@ impl X11ClientState { calloop::timer::TimeoutAction::ToInstant(instant) } }) - .expect("Failed to initialize refresh timer") + .expect("Failed to initialize window refresh timer") } fn get_cursor_icon(&mut self, style: CursorStyle) -> Option { @@ -1784,7 +1851,7 @@ impl X11ClientState { match style { CursorStyle::None => match create_invisible_cursor(&self.xcb_connection) { Ok(loaded_cursor) => result = Ok(loaded_cursor), - Err(err) => result = Err(err.context("error while creating invisible cursor")), + Err(err) => result = Err(err.context("X11: error while creating invisible cursor")), }, _ => 'outer: { let mut errors = String::new(); @@ -1827,14 +1894,14 @@ impl X11ClientState { { Ok(default) => { log_cursor_icon_warning(err.context(format!( - "x11: error loading cursor icon, falling back on default icon '{}'", + "X11: error loading cursor icon, falling back on default icon '{}'", DEFAULT_CURSOR_ICON_NAME ))); Some(default) } Err(default_err) => { log_cursor_icon_warning(err.context(default_err).context(format!( - "x11: error loading default cursor fallback '{}'", + "X11: error loading default cursor fallback '{}'", DEFAULT_CURSOR_ICON_NAME ))); None @@ -1857,7 +1924,7 @@ pub fn mode_refresh_rate(mode: &randr::ModeInfo) -> Duration { let millihertz = mode.dot_clock as u64 * 1_000 / (mode.htotal as u64 * mode.vtotal as u64); let micros = 1_000_000_000 / millihertz; - log::info!("Refreshing at {} micros", micros); + log::info!("Refreshing every {}ms", micros / 1_000); Duration::from_micros(micros) } @@ -1868,66 +1935,63 @@ fn fp3232_to_f32(value: xinput::Fp3232) -> f32 { fn check_compositor_present(xcb_connection: &XCBConnection, root: u32) -> bool { // Method 1: Check for _NET_WM_CM_S{root} let atom_name = format!("_NET_WM_CM_S{}", root); - let atom = xcb_connection - .intern_atom(false, atom_name.as_bytes()) - .unwrap() - .reply() - .map(|reply| reply.atom) - .unwrap_or(0); - - let method1 = if atom != 0 { - xcb_connection - .get_selection_owner(atom) - .unwrap() - .reply() + let atom1 = get_reply( + || format!("Failed to intern {atom_name}"), + xcb_connection.intern_atom(false, atom_name.as_bytes()), + ); + let method1 = match atom1.log_with_level(Level::Debug) { + Some(reply) if reply.atom != x11rb::NONE => { + let atom = reply.atom; + get_reply( + || format!("Failed to get {atom_name} owner"), + xcb_connection.get_selection_owner(atom), + ) .map(|reply| reply.owner != 0) + .log_with_level(Level::Debug) .unwrap_or(false) - } else { - false + } + _ => false, }; // Method 2: Check for _NET_WM_CM_OWNER let atom_name = "_NET_WM_CM_OWNER"; - let atom = xcb_connection - .intern_atom(false, atom_name.as_bytes()) - .unwrap() - .reply() - .map(|reply| reply.atom) - .unwrap_or(0); - - let method2 = if atom != 0 { - xcb_connection - .get_property(false, root, atom, xproto::AtomEnum::WINDOW, 0, 1) - .unwrap() - .reply() + let atom2 = get_reply( + || format!("Failed to intern {atom_name}"), + xcb_connection.intern_atom(false, atom_name.as_bytes()), + ); + let method2 = match atom2.log_with_level(Level::Debug) { + Some(reply) if reply.atom != x11rb::NONE => { + let atom = reply.atom; + get_reply( + || format!("Failed to get {atom_name}"), + xcb_connection.get_property(false, root, atom, xproto::AtomEnum::WINDOW, 0, 1), + ) .map(|reply| reply.value_len > 0) .unwrap_or(false) - } else { - false + } + _ => return false, }; // Method 3: Check for _NET_SUPPORTING_WM_CHECK let atom_name = "_NET_SUPPORTING_WM_CHECK"; - let atom = xcb_connection - .intern_atom(false, atom_name.as_bytes()) - .unwrap() - .reply() - .map(|reply| reply.atom) - .unwrap_or(0); - - let method3 = if atom != 0 { - xcb_connection - .get_property(false, root, atom, xproto::AtomEnum::WINDOW, 0, 1) - .unwrap() - .reply() + let atom3 = get_reply( + || format!("Failed to intern {atom_name}"), + xcb_connection.intern_atom(false, atom_name.as_bytes()), + ); + let method3 = match atom3.log_with_level(Level::Debug) { + Some(reply) if reply.atom != x11rb::NONE => { + let atom = reply.atom; + get_reply( + || format!("Failed to get {atom_name}"), + xcb_connection.get_property(false, root, atom, xproto::AtomEnum::WINDOW, 0, 1), + ) .map(|reply| reply.value_len > 0) .unwrap_or(false) - } else { - false + } + _ => return false, }; - // TODO: Remove this - log::info!( + log::debug!( "Compositor detection: _NET_WM_CM_S?={}, _NET_WM_CM_OWNER={}, _NET_SUPPORTING_WM_CHECK={}", method1, method2, @@ -1942,28 +2006,28 @@ fn check_gtk_frame_extents_supported( atoms: &XcbAtoms, root: xproto::Window, ) -> bool { - let supported_atoms = xcb_connection - .get_property( + let Some(supported_atoms) = get_reply( + || "Failed to get _NET_SUPPORTED", + xcb_connection.get_property( false, root, atoms._NET_SUPPORTED, xproto::AtomEnum::ATOM, 0, 1024, - ) - .unwrap() - .reply() - .map(|reply| { - // Convert Vec to Vec - reply - .value - .chunks_exact(4) - .map(|chunk| u32::from_ne_bytes([chunk[0], chunk[1], chunk[2], chunk[3]])) - .collect::>() - }) - .unwrap_or_default(); + ), + ) + .log_with_level(Level::Debug) else { + return false; + }; + + let supported_atom_ids: Vec = supported_atoms + .value + .chunks_exact(4) + .filter_map(|chunk| chunk.try_into().ok().map(u32::from_ne_bytes)) + .collect(); - supported_atoms.contains(&atoms._GTK_FRAME_EXTENTS) + supported_atom_ids.contains(&atoms._GTK_FRAME_EXTENTS) } fn xdnd_is_atom_supported(atom: u32, atoms: &XcbAtoms) -> bool { @@ -1980,17 +2044,19 @@ fn xdnd_get_supported_atom( supported_atoms: &XcbAtoms, target: xproto::Window, ) -> u32 { - let property = xcb_connection - .get_property( + if let Some(reply) = get_reply( + || "Failed to get XDnD supported atoms", + xcb_connection.get_property( false, target, supported_atoms.XdndTypeList, AtomEnum::ANY, 0, 1024, - ) - .unwrap(); - if let Ok(reply) = property.reply() { + ), + ) + .log_with_level(Level::Warn) + { if let Some(atoms) = reply.value32() { for atom in atoms { if xdnd_is_atom_supported(atom, &supported_atoms) { @@ -2016,9 +2082,11 @@ fn xdnd_send_finished( sequence: 0, response_type: xproto::CLIENT_MESSAGE_EVENT, }; - xcb_connection - .send_event(false, target, EventMask::default(), message) - .unwrap(); + check_reply( + || "Failed to send XDnD finished event", + xcb_connection.send_event(false, target, EventMask::default(), message), + ) + .log_err(); } fn xdnd_send_status( @@ -2036,22 +2104,24 @@ fn xdnd_send_status( sequence: 0, response_type: xproto::CLIENT_MESSAGE_EVENT, }; - xcb_connection - .send_event(false, target, EventMask::default(), message) - .unwrap(); + check_reply( + || "Failed to send XDnD status event", + xcb_connection.send_event(false, target, EventMask::default(), message), + ) + .log_err(); } /// Recomputes `pointer_device_states` by querying all pointer devices. /// When a device is present in `scroll_values_to_preserve`, its value for `ScrollAxisState.scroll_value` is used. -fn get_new_pointer_device_states( +fn current_pointer_device_states( xcb_connection: &XCBConnection, scroll_values_to_preserve: &BTreeMap, -) -> BTreeMap { - let devices_query_result = xcb_connection - .xinput_xi_query_device(XINPUT_ALL_DEVICES) - .unwrap() - .reply() - .unwrap(); +) -> Option> { + let devices_query_result = get_reply( + || "Failed to query XInput devices", + xcb_connection.xinput_xi_query_device(XINPUT_ALL_DEVICES), + ) + .log_err()?; let mut pointer_device_states = BTreeMap::new(); pointer_device_states.extend( @@ -2094,7 +2164,7 @@ fn get_new_pointer_device_states( if pointer_device_states.is_empty() { log::error!("Found no xinput mouse pointers."); } - return pointer_device_states; + return Some(pointer_device_states); } /// Returns true if the device is a pointer device. Does not include pointer device groups. diff --git a/crates/gpui/src/platform/linux/x11/window.rs b/crates/gpui/src/platform/linux/x11/window.rs index 7eedf5d4e688d355b0645148163770791db0925b..2b6028f1ccc2e258f80e33f533fcf5bbf69f881a 100644 --- a/crates/gpui/src/platform/linux/x11/window.rs +++ b/crates/gpui/src/platform/linux/x11/window.rs @@ -1,4 +1,5 @@ use anyhow::{Context as _, anyhow}; +use x11rb::connection::RequestConnection; use crate::platform::blade::{BladeContext, BladeRenderer, BladeSurfaceConfig}; use crate::{ @@ -32,6 +33,7 @@ use std::{ }; use super::{X11Display, XINPUT_ALL_DEVICE_GROUPS, XINPUT_ALL_DEVICES}; + x11rb::atom_manager! { pub XcbAtoms: AtomsCookie { XA_ATOM, @@ -318,43 +320,57 @@ impl rwh::HasDisplayHandle for X11Window { } } -fn check_reply( +pub(crate) fn check_reply( failure_context: F, - result: Result>, ConnectionError>, + result: Result, ConnectionError>, ) -> anyhow::Result<()> where - C: Display + Send + Sync + 'static, - F: FnOnce() -> C, + E: Display + Send + Sync + 'static, + F: FnOnce() -> E, + C: RequestConnection, { result - .map_err(|connection_error| anyhow!(connection_error)) - .and_then(|response| { - response - .check() - .map_err(|error_response| anyhow!(error_response)) - }) + .map_err(handle_connection_error) + .and_then(|response| response.check().map_err(|reply_error| anyhow!(reply_error))) .with_context(failure_context) } -fn get_reply( +pub(crate) fn get_reply( failure_context: F, - result: Result, O>, ConnectionError>, + result: Result, ConnectionError>, ) -> anyhow::Result where - C: Display + Send + Sync + 'static, - F: FnOnce() -> C, + E: Display + Send + Sync + 'static, + F: FnOnce() -> E, + C: RequestConnection, O: x11rb::x11_utils::TryParse, { result - .map_err(|connection_error| anyhow!(connection_error)) - .and_then(|response| { - response - .reply() - .map_err(|error_response| anyhow!(error_response)) - }) + .map_err(handle_connection_error) + .and_then(|response| response.reply().map_err(|reply_error| anyhow!(reply_error))) .with_context(failure_context) } +/// Convert X11 connection errors to `anyhow::Error` and panic for unrecoverable errors. +pub(crate) fn handle_connection_error(err: ConnectionError) -> anyhow::Error { + match err { + ConnectionError::UnknownError => anyhow!("X11 connection: Unknown error"), + ConnectionError::UnsupportedExtension => anyhow!("X11 connection: Unsupported extension"), + ConnectionError::MaximumRequestLengthExceeded => { + anyhow!("X11 connection: Maximum request length exceeded") + } + ConnectionError::FdPassingFailed => { + panic!("X11 connection: File descriptor passing failed") + } + ConnectionError::ParseError(parse_error) => { + anyhow!(parse_error).context("Parse error in X11 response") + } + ConnectionError::InsufficientMemory => panic!("X11 connection: Insufficient memory"), + ConnectionError::IoError(err) => anyhow!(err).context("X11 connection: IOError"), + _ => anyhow!(err), + } +} + impl X11WindowState { pub fn new( handle: AnyWindowHandle, @@ -581,7 +597,7 @@ impl X11WindowState { ), )?; - xcb.flush().with_context(|| "X11 Flush failed.")?; + xcb.flush()?; let renderer = { let raw_window = RawWindow { @@ -641,8 +657,7 @@ impl X11WindowState { || "X11 DestroyWindow failed while cleaning it up after setup failure.", xcb.destroy_window(x_window), )?; - xcb.flush() - .with_context(|| "X11 Flush failed while cleaning it up after setup failure.")?; + xcb.flush()?; } setup_result @@ -670,9 +685,7 @@ impl Drop for X11WindowHandle { || "X11 DestroyWindow failed while dropping X11WindowHandle.", self.xcb.destroy_window(self.id), )?; - self.xcb - .flush() - .with_context(|| "X11 Flush failed while dropping X11WindowHandle.")?; + self.xcb.flush()?; anyhow::Ok(()) }) .log_err(); @@ -691,10 +704,7 @@ impl Drop for X11Window { || "X11 DestroyWindow failure.", self.0.xcb.destroy_window(self.0.x_window), )?; - self.0 - .xcb - .flush() - .with_context(|| "X11 Flush failed after calling DestroyWindow.")?; + self.0.xcb.flush()?; anyhow::Ok(()) }) @@ -812,7 +822,7 @@ impl X11Window { let state = self.0.state.borrow(); check_reply( - || "X11 UngrabPointer before move/resize of window ailed.", + || "X11 UngrabPointer before move/resize of window failed.", self.0.xcb.ungrab_pointer(x11rb::CURRENT_TIME), )?; @@ -846,7 +856,11 @@ impl X11Window { } fn flush(&self) -> anyhow::Result<()> { - self.0.xcb.flush().with_context(|| "X11 Flush failed.") + self.0 + .xcb + .flush() + .map_err(handle_connection_error) + .context("X11 flush failed") } } @@ -889,9 +903,13 @@ impl X11WindowStatePtr { )?; if reply.value_len != 0 { - let atom = u32::from_ne_bytes(reply.value[0..4].try_into().unwrap()); - let edge_constraints = EdgeConstraints::from_atom(atom); - state.edge_constraints.replace(edge_constraints); + if let Ok(bytes) = reply.value[0..4].try_into() { + let atom = u32::from_ne_bytes(bytes); + let edge_constraints = EdgeConstraints::from_atom(atom); + state.edge_constraints.replace(edge_constraints); + } else { + log::error!("Failed to parse GTK_EDGE_CONSTRAINTS"); + } } Ok(()) @@ -1030,7 +1048,7 @@ impl X11WindowStatePtr { bounds } - pub fn configure(&self, bounds: Bounds) -> anyhow::Result<()> { + pub fn set_bounds(&self, bounds: Bounds) -> anyhow::Result<()> { let mut resize_args = None; let is_resize; { @@ -1196,12 +1214,14 @@ impl PlatformWindow for X11Window { } fn mouse_position(&self) -> Point { - let reply = get_reply( + get_reply( || "X11 QueryPointer failed.", self.0.xcb.query_pointer(self.0.x_window), ) - .unwrap(); - Point::new((reply.root_x as u32).into(), (reply.root_y as u32).into()) + .log_err() + .map_or(Point::new(Pixels(0.0), Pixels(0.0)), |reply| { + Point::new((reply.root_x as u32).into(), (reply.root_y as u32).into()) + }) } fn modifiers(&self) -> Modifiers { @@ -1269,7 +1289,7 @@ impl PlatformWindow for X11Window { xproto::Time::CURRENT_TIME, ) .log_err(); - self.flush().unwrap(); + self.flush().log_err(); } fn is_active(&self) -> bool { @@ -1323,7 +1343,7 @@ impl PlatformWindow for X11Window { &data, ), ) - .unwrap(); + .log_err(); } fn map_window(&mut self) -> anyhow::Result<()> { @@ -1359,7 +1379,7 @@ impl PlatformWindow for X11Window { message, ), ) - .unwrap(); + .log_err(); } fn zoom(&self) { @@ -1370,7 +1390,7 @@ impl PlatformWindow for X11Window { state.atoms._NET_WM_STATE_MAXIMIZED_VERT, state.atoms._NET_WM_STATE_MAXIMIZED_HORZ, ) - .unwrap(); + .log_err(); } fn toggle_fullscreen(&self) { @@ -1381,7 +1401,7 @@ impl PlatformWindow for X11Window { state.atoms._NET_WM_STATE_FULLSCREEN, xproto::AtomEnum::NONE.into(), ) - .unwrap(); + .log_err(); } fn is_fullscreen(&self) -> bool { @@ -1444,9 +1464,11 @@ impl PlatformWindow for X11Window { || "X11 UngrabPointer failed.", self.0.xcb.ungrab_pointer(x11rb::CURRENT_TIME), ) - .unwrap(); + .log_err(); - let coords = self.get_root_position(position).unwrap(); + let Some(coords) = self.get_root_position(position).log_err() else { + return; + }; let message = ClientMessageEvent::new( 32, self.0.x_window, @@ -1468,16 +1490,16 @@ impl PlatformWindow for X11Window { message, ), ) - .unwrap(); + .log_err(); } fn start_window_move(&self) { const MOVERESIZE_MOVE: u32 = 8; - self.send_moveresize(MOVERESIZE_MOVE).unwrap(); + self.send_moveresize(MOVERESIZE_MOVE).log_err(); } fn start_window_resize(&self, edge: ResizeEdge) { - self.send_moveresize(edge.to_moveresize()).unwrap(); + self.send_moveresize(edge.to_moveresize()).log_err(); } fn window_decorations(&self) -> crate::Decorations { @@ -1552,7 +1574,7 @@ impl PlatformWindow for X11Window { bytemuck::cast_slice::(&insets), ), ) - .unwrap(); + .log_err(); } } @@ -1574,7 +1596,7 @@ impl PlatformWindow for X11Window { WindowDecorations::Client => [1 << 1, 0, 0, 0, 0], }; - check_reply( + let success = check_reply( || "X11 ChangeProperty for _MOTIF_WM_HINTS failed.", self.0.xcb.change_property( xproto::PropMode::REPLACE, @@ -1586,7 +1608,11 @@ impl PlatformWindow for X11Window { bytemuck::cast_slice::(&hints_data), ), ) - .unwrap(); + .log_err(); + + let Some(()) = success else { + return; + }; match decorations { WindowDecorations::Server => {