diff --git a/CHANGELOG.md b/CHANGELOG.md index 68f79bf1af..f55f872014 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -144,6 +144,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i - Refactored DPI scaling. ([#904] by [@xStrom]) - Added docs generation testing for all features. ([#942] by [@xStrom]) - Renamed `BaseState` to `WidgetState` ([#969] by [@cmyr]) +- X11: Reworked error handling ([#982] by [@jneem]) ### Outside News @@ -229,6 +230,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i [#969]: https://github.com/xi-editor/druid/pull/969 [#970]: https://github.com/xi-editor/druid/pull/970 [#980]: https://github.com/xi-editor/druid/pull/980 +[#982]: https://github.com/xi-editor/druid/pull/982 ## [0.5.0] - 2020-04-01 diff --git a/Cargo.lock b/Cargo.lock index 3b693c26f4..d36560144f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5,6 +5,11 @@ name = "adler32" version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "anyhow" +version = "1.0.31" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "arrayvec" version = "0.5.1" @@ -398,6 +403,7 @@ dependencies = [ name = "druid-shell" version = "0.6.0" dependencies = [ + "anyhow 1.0.31 (registry+https://github.com/rust-lang/crates.io-index)", "cairo-rs 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)", "cairo-sys-rs 0.9.2 (registry+https://github.com/rust-lang/crates.io-index)", "cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1870,6 +1876,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [metadata] "checksum adler32 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "5d2e7343e7fc9de883d1b0341e0b13970f764c14101234857d2ddafa1cb1cac2" +"checksum anyhow 1.0.31 (registry+https://github.com/rust-lang/crates.io-index)" = "85bb70cc08ec97ca5450e6eba421deeea5f172c0fc61f78b5357b2a8e8be195f" "checksum arrayvec 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "cff77d8686867eceff3105329d4698d96c2391c176d5d03adc90c7389162b5b8" "checksum atk 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "444daefa55f229af145ea58d77efd23725024ee1f6f3102743709aa6b18c663e" "checksum atk-sys 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)" = "e552c1776737a4c80110d06b36d099f47c727335f9aaa5d942a72b6863a8ec6f" diff --git a/druid-shell/Cargo.toml b/druid-shell/Cargo.toml index 53818f4493..23f9afc7b3 100644 --- a/druid-shell/Cargo.toml +++ b/druid-shell/Cargo.toml @@ -26,6 +26,7 @@ lazy_static = "1.0" time = "0.2.7" cfg-if = "0.1.10" instant = { version = "0.1", features = ["wasm-bindgen"] } +anyhow = "1.0.31" # Optional dependencies cairo-rs = { version = "0.8.1", default_features = false, optional = true } diff --git a/druid-shell/src/application.rs b/druid-shell/src/application.rs index 9358d235e8..c26c06320d 100644 --- a/druid-shell/src/application.rs +++ b/druid-shell/src/application.rs @@ -79,10 +79,7 @@ impl Application { return Err(Error::ApplicationAlreadyExists); } util::claim_main_thread(); - let platform_app = match platform::Application::new() { - Ok(app) => app, - Err(err) => return Err(Error::Platform(err)), - }; + let platform_app = platform::Application::new()?; let state = Rc::new(RefCell::new(State { running: false })); let app = Application { platform_app, diff --git a/druid-shell/src/error.rs b/druid-shell/src/error.rs index 2c0c03b020..cd6aafba84 100644 --- a/druid-shell/src/error.rs +++ b/druid-shell/src/error.rs @@ -15,6 +15,7 @@ //! Errors at the application shell level. use std::fmt; +use std::sync::Arc; use crate::platform::error as platform; @@ -25,12 +26,10 @@ pub enum Error { ApplicationAlreadyExists, /// The window has already been destroyed. WindowDropped, - /// Runtime borrow failure. - BorrowError(BorrowError), /// Platform specific error. Platform(platform::Error), /// Other miscellaneous error. - Other(&'static str), + Other(Arc), } impl fmt::Display for Error { @@ -39,9 +38,8 @@ impl fmt::Display for Error { Error::ApplicationAlreadyExists => { write!(f, "An application instance has already been created.") } - Error::WindowDropped => write!(f, "The window has already been destroyed."), - Error::BorrowError(err) => fmt::Display::fmt(err, f), Error::Platform(err) => fmt::Display::fmt(err, f), + Error::WindowDropped => write!(f, "The window has already been destroyed."), Error::Other(s) => write!(f, "{}", s), } } @@ -49,54 +47,14 @@ impl fmt::Display for Error { impl std::error::Error for Error {} -impl From for Error { - fn from(src: platform::Error) -> Error { - Error::Platform(src) +impl From for Error { + fn from(src: anyhow::Error) -> Error { + Error::Other(Arc::new(src)) } } -/// Runtime borrow failure. -#[derive(Debug, Clone)] -pub struct BorrowError { - location: &'static str, - target: &'static str, - mutable: bool, -} - -impl BorrowError { - pub fn new(location: &'static str, target: &'static str, mutable: bool) -> BorrowError { - BorrowError { - location, - target, - mutable, - } - } -} - -impl fmt::Display for BorrowError { - fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { - if self.mutable { - // Mutable borrow fails when any borrow exists - write!( - f, - "{} was already borrowed in {}", - self.target, self.location - ) - } else { - // Regular borrow fails when a mutable borrow exists - write!( - f, - "{} was already mutably borrowed in {}", - self.target, self.location - ) - } - } -} - -impl std::error::Error for BorrowError {} - -impl From for Error { - fn from(src: BorrowError) -> Error { - Error::BorrowError(src) +impl From for Error { + fn from(src: platform::Error) -> Error { + Error::Platform(src) } } diff --git a/druid-shell/src/lib.rs b/druid-shell/src/lib.rs index e205f69304..a1c80374ed 100644 --- a/druid-shell/src/lib.rs +++ b/druid-shell/src/lib.rs @@ -24,6 +24,9 @@ pub use kurbo; pub use piet_common as piet; +#[macro_use] +mod util; + mod application; mod clipboard; mod common_util; @@ -36,7 +39,6 @@ mod menu; mod mouse; mod platform; mod scale; -mod util; mod window; pub use application::{AppHandler, Application}; diff --git a/druid-shell/src/platform/gtk/dialog.rs b/druid-shell/src/platform/gtk/dialog.rs index bec0f4865b..bd6119c25e 100644 --- a/druid-shell/src/platform/gtk/dialog.rs +++ b/druid-shell/src/platform/gtk/dialog.rs @@ -16,6 +16,7 @@ use std::ffi::OsString; +use anyhow::anyhow; use gtk::{FileChooserAction, FileChooserExt, FileFilter, NativeDialogExt, ResponseType, Window}; use crate::dialog::{FileDialogOptions, FileDialogType, FileSpec}; @@ -89,12 +90,12 @@ pub(crate) fn get_file_dialog_path( let result = match result { ResponseType::Accept => match dialog.get_filename() { Some(path) => Ok(path.into_os_string()), - None => Err(Error::Other("No path received for filename")), + None => Err(anyhow!("No path received for filename")), }, - ResponseType::Cancel => Err(Error::Other("Dialog was deleted")), + ResponseType::Cancel => Err(anyhow!("Dialog was deleted")), _ => { log::warn!("Unhandled dialog result: {:?}", result); - Err(Error::Other("Unhandled dialog result")) + Err(anyhow!("Unhandled dialog result")) } }; @@ -102,5 +103,5 @@ pub(crate) fn get_file_dialog_path( dialog.destroy(); - result + Ok(result?) } diff --git a/druid-shell/src/platform/gtk/window.rs b/druid-shell/src/platform/gtk/window.rs index 95902a2837..218cb7ac2a 100644 --- a/druid-shell/src/platform/gtk/window.rs +++ b/druid-shell/src/platform/gtk/window.rs @@ -25,6 +25,7 @@ use std::slice; use std::sync::{Arc, Mutex, Weak}; use std::time::Instant; +use anyhow::anyhow; use gdk::{EventKey, EventMask, ModifierType, ScrollDirection, WindowExt}; use gio::ApplicationExt; use gtk::prelude::*; @@ -694,9 +695,7 @@ impl WindowHandle { if let Some(state) = self.state.upgrade() { dialog::get_file_dialog_path(state.window.upcast_ref(), ty, options) } else { - Err(ShellError::Other( - "Cannot upgrade state from weak pointer to arc", - )) + Err(anyhow!("Cannot upgrade state from weak pointer to arc").into()) } } } diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 7610c03713..dd9ac1a919 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -19,6 +19,7 @@ use std::collections::HashMap; use std::convert::TryInto; use std::rc::Rc; +use anyhow::{anyhow, Context, Error}; use xcb::{ ButtonPressEvent, ButtonReleaseEvent, ClientMessageEvent, Connection, DestroyNotifyEvent, ExposeEvent, KeyPressEvent, MotionNotifyEvent, BUTTON_PRESS, BUTTON_RELEASE, CLIENT_MESSAGE, @@ -29,7 +30,6 @@ use xcb::{ use crate::application::AppHandler; use super::clipboard::Clipboard; -use super::error::Error; use super::window::Window; #[derive(Clone)] @@ -74,10 +74,7 @@ struct State { impl Application { pub fn new() -> Result { - let (conn, screen_num) = match Connection::connect_with_xlib_display() { - Ok(conn) => conn, - Err(err) => return Err(Error::ConnectionError(err.to_string())), - }; + let (conn, screen_num) = Connection::connect_with_xlib_display()?; let connection = Rc::new(conn); let window_id = Application::create_event_window(&connection, screen_num)?; let state = Rc::new(RefCell::new(State { @@ -95,14 +92,15 @@ impl Application { fn create_event_window(conn: &Rc, screen_num: i32) -> Result { let id = conn.generate_id(); let setup = conn.get_setup(); - // TODO(x11/errors): Don't unwrap for screen? - let screen = setup.roots().nth(screen_num as usize).unwrap(); + let screen = setup + .roots() + .nth(screen_num as usize) + .ok_or_else(|| anyhow!("invalid screen num: {}", screen_num))?; let cw_values = [(CW_EVENT_MASK, EVENT_MASK_STRUCTURE_NOTIFY)]; // Create the actual window - // TODO(x11/errors): check that this actually succeeds? - xcb::create_window( + xcb::create_window_checked( // Connection to the X server conn, // Window depth @@ -127,50 +125,30 @@ impl Application { COPY_FROM_PARENT.try_into().unwrap(), // Window properties mask &cw_values, - ); + ) + .request_check()?; Ok(id) } pub(crate) fn add_window(&self, id: u32, window: Rc) -> Result<(), Error> { - match self.state.try_borrow_mut() { - Ok(mut state) => { - state.windows.insert(id, window); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Application::add_window state: {}", - err - ))), - } + borrow_mut!(self.state)?.windows.insert(id, window); + Ok(()) } /// Remove the specified window from the `Application` and return the number of windows left. fn remove_window(&self, id: u32) -> Result { - match self.state.try_borrow_mut() { - Ok(mut state) => { - state.windows.remove(&id); - Ok(state.windows.len()) - } - Err(err) => Err(Error::BorrowError(format!( - "Application::remove_window state: {}", - err - ))), - } + let mut state = borrow_mut!(self.state)?; + state.windows.remove(&id); + Ok(state.windows.len()) } fn window(&self, id: u32) -> Result, Error> { - match self.state.try_borrow() { - Ok(state) => state - .windows - .get(&id) - .cloned() - .ok_or_else(|| Error::Generic(format!("No window with id {}", id))), - Err(err) => Err(Error::BorrowError(format!( - "Application::window state: {}", - err - ))), - } + borrow!(self.state)? + .windows + .get(&id) + .cloned() + .ok_or_else(|| anyhow!("No window with id {}", id)) } #[inline] @@ -183,150 +161,125 @@ impl Application { self.screen_num } - #[allow(clippy::cognitive_complexity)] + /// Returns `Ok(true)` if we want to exit the main loop. + fn handle_event(&self, ev: &xcb::GenericEvent) -> Result { + let ev_type = ev.response_type() & !0x80; + // NOTE: When adding handling for any of the following events, + // there must be a check against self.window_id + // to know if the event must be ignored. + // Otherwise there will be a "failed to get window" error. + // + // CIRCULATE_NOTIFY, CONFIGURE_NOTIFY, GRAVITY_NOTIFY + // MAP_NOTIFY, REPARENT_NOTIFY, UNMAP_NOTIFY + match ev_type { + EXPOSE => { + let expose = unsafe { xcb::cast_event::(&ev) }; + let window_id = expose.window(); + let w = self + .window(window_id) + .context("EXPOSE - failed to get window")?; + w.handle_expose(expose) + .context("EXPOSE - failed to handle")?; + } + KEY_PRESS => { + let key_press = unsafe { xcb::cast_event::(&ev) }; + let window_id = key_press.event(); + let w = self + .window(window_id) + .context("KEY_PRESS - failed to get window")?; + w.handle_key_press(key_press) + .context("KEY_PRESS - failed to handle")?; + } + BUTTON_PRESS => { + let button_press = unsafe { xcb::cast_event::(&ev) }; + let window_id = button_press.event(); + let w = self + .window(window_id) + .context("BUTTON_PRESS - failed to get window")?; + + // X doesn't have dedicated scroll events: it uses mouse buttons instead. + // Buttons 4/5 are vertical; 6/7 are horizontal. + if button_press.detail() >= 4 && button_press.detail() <= 7 { + w.handle_wheel(button_press) + .context("BUTTON_PRESS - failed to handle wheel")?; + } else { + w.handle_button_press(button_press) + .context("BUTTON_PRESS - failed to handle")?; + } + } + BUTTON_RELEASE => { + let button_release = unsafe { xcb::cast_event::(&ev) }; + let window_id = button_release.event(); + let w = self + .window(window_id) + .context("BUTTON_RELEASE - failed to get window")?; + if button_release.detail() >= 4 && button_release.detail() <= 7 { + // This is the release event corresponding to a mouse wheel. + // Ignore it: we already handled the press event. + } else { + w.handle_button_release(button_release) + .context("BUTTON_RELEASE - failed to handle")?; + } + } + MOTION_NOTIFY => { + let motion_notify = unsafe { xcb::cast_event::(&ev) }; + let window_id = motion_notify.event(); + let w = self + .window(window_id) + .context("MOTION_NOTIFY - failed to get window")?; + w.handle_motion_notify(motion_notify) + .context("MOTION_NOTIFY - failed to handle")?; + } + CLIENT_MESSAGE => { + let client_message = unsafe { xcb::cast_event::(&ev) }; + let window_id = client_message.window(); + let w = self + .window(window_id) + .context("CLIENT_MESSAGE - failed to get window")?; + w.handle_client_message(client_message) + .context("CLIENT_MESSAGE - failed to handle")?; + } + DESTROY_NOTIFY => { + let destroy_notify = unsafe { xcb::cast_event::(&ev) }; + let window_id = destroy_notify.window(); + if window_id == self.window_id { + // The destruction of the Application window means that + // we need to quit the run loop. + return Ok(true); + } + + let w = self + .window(window_id) + .context("DESTROY_NOTIFY - failed to get window")?; + w.handle_destroy_notify(destroy_notify) + .context("DESTROY_NOTIFY - failed to handle")?; + + // Remove our reference to the Window and allow it to be dropped + let windows_left = self + .remove_window(window_id) + .context("DESTROY_NOTIFY - failed to remove window")?; + // Check if we need to finalize a quit request + if windows_left == 0 && borrow!(self.state)?.quitting { + self.finalize_quit(); + } + } + _ => {} + } + Ok(false) + } + pub fn run(self, _handler: Option>) { loop { if let Some(ev) = self.connection.wait_for_event() { - let ev_type = ev.response_type() & !0x80; - // NOTE: When adding handling for any of the following events, - // there must be a check against self.window_id - // to know if the event must be ignored. - // Otherwise there will be a "failed to get window" error. - // - // CIRCULATE_NOTIFY, CONFIGURE_NOTIFY, GRAVITY_NOTIFY - // MAP_NOTIFY, REPARENT_NOTIFY, UNMAP_NOTIFY - match ev_type { - EXPOSE => { - let expose = unsafe { xcb::cast_event::(&ev) }; - let window_id = expose.window(); - match self.window(window_id) { - Ok(w) => { - if let Err(err) = w.handle_expose(expose) { - log::error!("EXPOSE - failed to handle: {}", err); - } - } - Err(err) => log::error!("EXPOSE - failed to get window: {}", err), - } - } - KEY_PRESS => { - let key_press = unsafe { xcb::cast_event::(&ev) }; - let window_id = key_press.event(); - match self.window(window_id) { - Ok(w) => { - if let Err(err) = w.handle_key_press(key_press) { - log::error!("KEY_PRESS - failed to handle: {}", err); - } - } - Err(err) => log::error!("KEY_PRESS - failed to get window: {}", err), - } - } - BUTTON_PRESS => { - let button_press = unsafe { xcb::cast_event::(&ev) }; - let window_id = button_press.event(); - match self.window(window_id) { - Ok(w) => { - // X doesn't have dedicated scroll events: it uses mouse buttons instead. - // Buttons 4/5 are vertical; 6/7 are horizontal. - if button_press.detail() >= 4 && button_press.detail() <= 7 { - if let Err(err) = w.handle_wheel(button_press) { - log::error!( - "BUTTON_PRESS - failed to handle wheel: {}", - err - ); - } - } else if let Err(err) = w.handle_button_press(button_press) { - log::error!("BUTTON_PRESS - failed to handle: {}", err); - } - } - Err(err) => log::error!("BUTTON_PRESS - failed to get window: {}", err), - } - } - BUTTON_RELEASE => { - let button_release = unsafe { xcb::cast_event::(&ev) }; - let window_id = button_release.event(); - match self.window(window_id) { - Ok(w) => { - if button_release.detail() >= 4 && button_release.detail() <= 7 { - // This is the release event corresponding to a mouse wheel. - // Ignore it: we already handled the press event. - } else if let Err(err) = w.handle_button_release(button_release) { - log::error!("BUTTON_RELEASE - failed to handle: {}", err); - } - } - Err(err) => { - log::error!("BUTTON_RELEASE - failed to get window: {}", err) - } - } - } - MOTION_NOTIFY => { - let motion_notify = unsafe { xcb::cast_event::(&ev) }; - let window_id = motion_notify.event(); - match self.window(window_id) { - Ok(w) => { - if let Err(err) = w.handle_motion_notify(motion_notify) { - log::error!("MOTION_NOTIFY - failed to handle: {}", err); - } - } - Err(err) => { - log::error!("MOTION_NOTIFY - failed to get window: {}", err) - } - } - } - CLIENT_MESSAGE => { - let client_message = unsafe { xcb::cast_event::(&ev) }; - let window_id = client_message.window(); - match self.window(window_id) { - Ok(w) => { - if let Err(err) = w.handle_client_message(client_message) { - log::error!("CLIENT_MESSAGE - failed to handle: {}", err); - } - } - Err(err) => { - log::error!("CLIENT_MESSAGE - failed to get window: {}", err) - } - } - } - DESTROY_NOTIFY => { - let destroy_notify = unsafe { xcb::cast_event::(&ev) }; - let window_id = destroy_notify.window(); - if window_id == self.window_id { - // The destruction of the Application window means that - // we need to quit the run loop. + match self.handle_event(&ev) { + Ok(quit) => { + if quit { break; } - match self.window(window_id) { - Ok(w) => { - if let Err(err) = w.handle_destroy_notify(destroy_notify) { - log::error!("DESTROY_NOTIFY - failed to handle: {}", err); - } - } - Err(err) => { - log::error!("DESTROY_NOTIFY - failed to get window: {}", err) - } - } - - // Remove our reference to the Window and allow it to be dropped - match self.remove_window(window_id) { - Ok(windows_left) => { - if windows_left == 0 { - // Check if we need to finalize a quit request - if let Ok(state) = self.state.try_borrow() { - if state.quitting { - self.finalize_quit(); - } - } else { - log::error!( - "DESTROY_NOTIFY - failed to check for quit request" - ); - } - } - } - Err(err) => { - log::error!("DESTROY_NOTIFY - failed to remove window: {}", err) - } - } } - _ => {} + Err(e) => { + log::error!("Error handling event: {}", e); + } } } } diff --git a/druid-shell/src/platform/x11/error.rs b/druid-shell/src/platform/x11/error.rs index 43b86a5ed8..dce51664f8 100644 --- a/druid-shell/src/platform/x11/error.rs +++ b/druid-shell/src/platform/x11/error.rs @@ -16,23 +16,14 @@ use std::fmt; +/// The X11 backend doesn't currently define any platform-specific errors; +/// it uses the `crate::error::Other` variant instead. #[derive(Debug, Clone)] -pub enum Error { - // Generic error - Generic(String), - // TODO: Replace String with xcb::ConnError once that gets Clone support - ConnectionError(String), - // Runtime borrow failure - BorrowError(String), -} +pub enum Error {} impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { - match self { - Error::Generic(msg) => write!(f, "Error: {}", msg), - Error::ConnectionError(err) => write!(f, "Connection error: {}", err), - Error::BorrowError(msg) => write!(f, "Failed to borrow: {}", msg), - } + fn fmt(&self, _: &mut fmt::Formatter) -> Result<(), fmt::Error> { + Ok(()) } } diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index c02e5b4c0f..8b7d9d1a6e 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -19,6 +19,7 @@ use std::cell::RefCell; use std::convert::TryInto; use std::rc::{Rc, Weak}; +use anyhow::{anyhow, Context, Error}; use cairo::{XCBConnection, XCBDrawable, XCBSurface, XCBVisualType}; use xcb::{ Atom, Visualtype, ATOM_ATOM, ATOM_STRING, ATOM_WM_NAME, CONFIG_WINDOW_STACK_MODE, @@ -39,7 +40,6 @@ use crate::scale::Scale; use crate::window::{IdleToken, Text, TimerToken, WinHandler}; use super::application::Application; -use super::error::Error; use super::menu::Menu; use super::util; @@ -101,15 +101,10 @@ impl WindowBuilder { // the client and window manager in which the client is willing to participate. // // https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#wm_protocols_property - let wm_protocols = match xcb::intern_atom(conn, false, "WM_PROTOCOLS").get_reply() { - Ok(reply) => reply.atom(), - Err(err) => { - return Err(Error::Generic(format!( - "failed to get WM_PROTOCOLS: {}", - err - ))) - } - }; + let wm_protocols = xcb::intern_atom(conn, false, "WM_PROTOCOLS") + .get_reply() + .context("failed to get WM_PROTOCOLS")? + .atom(); // WM_DELETE_WINDOW // @@ -120,15 +115,10 @@ impl WindowBuilder { // Registering for but ignoring this event means that the window will remain open. // // https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#window_deletion - let wm_delete_window = match xcb::intern_atom(conn, false, "WM_DELETE_WINDOW").get_reply() { - Ok(reply) => reply.atom(), - Err(err) => { - return Err(Error::Generic(format!( - "failed to get WM_DELETE_WINDOW: {}", - err - ))) - } - }; + let wm_delete_window = xcb::intern_atom(conn, false, "WM_DELETE_WINDOW") + .get_reply() + .context("failed to get WM_DELETE_WINDOW")? + .atom(); // Replace the window's WM_PROTOCOLS with the following. let protocols = [wm_delete_window]; @@ -171,14 +161,10 @@ impl WindowBuilder { &cairo_visual_type, self.size.width as i32, self.size.height as i32, - ); - match cairo_surface { - Ok(cairo_surface) => Ok(RefCell::new(cairo::Context::new(&cairo_surface))), - Err(err) => Err(Error::Generic(format!( - "Failed to create cairo surface: {}", - err - ))), - } + ) + .map_err(|status| anyhow!("Failed to create cairo surface: {}", status))?; + + Ok(RefCell::new(cairo::Context::new(&cairo_surface))) } // TODO(x11/menus): make menus if requested @@ -207,8 +193,7 @@ impl WindowBuilder { ]; // Create the actual window - // TODO(x11/errors): check that this actually succeeds? - xcb::create_window( + xcb::create_window_checked( // Connection to the X server conn, // Window depth @@ -236,7 +221,8 @@ impl WindowBuilder { visual_id, // Window properties mask &cw_values, - ); + ) + .request_check()?; // TODO(x11/errors): Should do proper cleanup (window destruction etc) in case of error let atoms = self.atoms(id)?; @@ -290,19 +276,12 @@ struct WindowState { impl Window { fn connect(&self, handle: WindowHandle) -> Result<(), Error> { - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - let size = self.size()?; - handler.connect(&handle.into()); - handler.scale(Scale::default()); - handler.size(size); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::connect handler: {}", - err - ))), - } + let mut handler = borrow_mut!(self.handler)?; + let size = self.size()?; + handler.connect(&handle.into()); + handler.scale(Scale::default()); + handler.size(size); + Ok(()) } /// Start the destruction of the window. @@ -315,63 +294,38 @@ impl Window { } fn cairo_surface(&self) -> Result { - match self.cairo_context.try_borrow() { - Ok(ctx) => match ctx.get_target().try_into() { - Ok(surface) => Ok(surface), - Err(err) => Err(Error::Generic(format!( - "Window::cairo_surface ctx.try_into: {}", - err - ))), - }, - Err(err) => Err(Error::BorrowError(format!( - "Window::cairo_surface cairo_context: {}", - err - ))), - } + borrow!(self.cairo_context)? + .get_target() + .try_into() + .map_err(|_| anyhow!("Window::cairo_surface try_into")) } fn size(&self) -> Result { - match self.state.try_borrow() { - Ok(state) => Ok(state.size), - Err(err) => Err(Error::BorrowError(format!("Window::size state: {}", err))), - } + Ok(borrow!(self.state)?.size) } fn set_size(&self, size: Size) -> Result<(), Error> { // TODO(x11/dpi_scaling): detect DPI and scale size - let new_size = match self.state.try_borrow_mut() { - Ok(mut state) => { - if state.size != size { - state.size = size; - Some(size) - } else { - None - } - } - Err(err) => { - return Err(Error::BorrowError(format!( - "Window::set_size state: {}", - err - ))) + let new_size = { + let mut state = borrow_mut!(self.state)?; + if size != state.size { + state.size = size; + Some(size) + } else { + None } }; if let Some(size) = new_size { - let cairo_surface = self.cairo_surface()?; - if let Err(err) = cairo_surface.set_size(size.width as i32, size.height as i32) { - return Err(Error::Generic(format!( - "Failed to update cairo surface size to {:?}: {}", - size, err - ))); - } - match self.handler.try_borrow_mut() { - Ok(mut handler) => handler.size(size), - Err(err) => { - return Err(Error::BorrowError(format!( - "Window::set_size handler: {}", - err - ))) - } - } + self.cairo_surface()? + .set_size(size.width as i32, size.height as i32) + .map_err(|status| { + anyhow!( + "Failed to update cairo surface size to {:?}: {}", + size, + status + ) + })?; + borrow_mut!(self.handler)?.size(size); } Ok(()) } @@ -401,30 +355,40 @@ impl Window { } fn render(&self, invalid_rect: Rect) -> Result<(), Error> { - // TODO(x11/errors): this function should return a an error - // instead of panicking or logging if the error isn't recoverable. - // Figure out the window's current size let geometry_cookie = xcb::get_geometry(self.app.connection(), self.id); - let reply = geometry_cookie.get_reply().unwrap(); + let reply = geometry_cookie.get_reply()?; let size = Size::new(reply.width() as f64, reply.height() as f64); self.set_size(size)?; let mut anim = false; - if let Ok(mut cairo_ctx) = self.cairo_context.try_borrow_mut() { + { + let mut cairo_ctx = borrow_mut!(self.cairo_context)?; let mut piet_ctx = Piet::new(&mut cairo_ctx); piet_ctx.clip(invalid_rect); - if let Ok(mut handler) = self.handler.try_borrow_mut() { - anim = handler.paint(&mut piet_ctx, invalid_rect); - } else { - log::error!("Window::render - handler already borrowed"); - } - if let Err(e) = piet_ctx.finish() { - log::error!("Window::render - piet finish failed: {}", e); - } + + // We need to be careful with earlier returns here, because piet_ctx + // can panic if it isn't finish()ed. Also, we want to reset cairo's clip + // even on error. + let err; + match borrow_mut!(self.handler) { + Ok(mut handler) => { + anim = handler.paint(&mut piet_ctx, invalid_rect); + err = piet_ctx + .finish() + .map_err(|e| anyhow!("Window::render - piet finish failed: {}", e)); + } + Err(e) => { + err = Err(e); + if let Err(e) = piet_ctx.finish() { + // We can't return both errors, so just log this one. + log::error!("Window::render - piet finish failed in error branch: {}", e); + } + } + }; cairo_ctx.reset_clip(); - } else { - log::error!("Window::render - cairo context already borrowed"); + + err?; } if anim && self.refresh_rate.is_some() { @@ -522,7 +486,7 @@ impl Window { (expose.x() as f64, expose.y() as f64), (expose.width() as f64, expose.height() as f64), ); - self.render(rect) + Ok(self.render(rect)?) } pub fn handle_key_press(&self, key_press: &xcb::KeyPressEvent) -> Result<(), Error> { @@ -536,16 +500,8 @@ impl Window { key_code, key_code, ); - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - handler.key_down(key_event); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::handle_key_press handle: {}", - err - ))), - } + borrow_mut!(self.handler)?.key_down(key_event); + Ok(()) } pub fn handle_button_press(&self, button_press: &xcb::ButtonPressEvent) -> Result<(), Error> { @@ -562,16 +518,8 @@ impl Window { button, wheel_delta: Vec2::ZERO, }; - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - handler.mouse_down(&mouse_event); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::handle_button_press handle: {}", - err - ))), - } + borrow_mut!(self.handler)?.mouse_down(&mouse_event); + Ok(()) } pub fn handle_button_release( @@ -593,16 +541,8 @@ impl Window { button, wheel_delta: Vec2::ZERO, }; - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - handler.mouse_up(&mouse_event); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::handle_button_release handle: {}", - err - ))), - } + borrow_mut!(self.handler)?.mouse_up(&mouse_event); + Ok(()) } pub fn handle_wheel(&self, event: &xcb::ButtonPressEvent) -> Result<(), Error> { @@ -617,12 +557,7 @@ impl Window { 5 => (0.0, 120.0), 6 => (-120.0, 0.0), 7 => (120.0, 0.0), - _ => { - return Err(Error::Generic(format!( - "unexpected mouse wheel button: {}", - button - ))) - } + _ => return Err(anyhow!("unexpected mouse wheel button: {}", button)), }; let mouse_event = MouseEvent { pos: Point::new(event.event_x() as f64, event.event_y() as f64), @@ -634,16 +569,8 @@ impl Window { wheel_delta: delta.into(), }; - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - handler.wheel(&mouse_event); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::handle_wheel handle: {}", - err - ))), - } + borrow_mut!(self.handler)?.wheel(&mouse_event); + Ok(()) } pub fn handle_motion_notify( @@ -662,16 +589,8 @@ impl Window { button: MouseButton::None, wheel_delta: Vec2::ZERO, }; - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - handler.mouse_move(&mouse_event); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::handle_motion_notify handle: {}", - err - ))), - } + borrow_mut!(self.handler)?.mouse_move(&mouse_event); + Ok(()) } pub fn handle_client_message( @@ -693,16 +612,8 @@ impl Window { &self, _destroy_notify: &xcb::DestroyNotifyEvent, ) -> Result<(), Error> { - match self.handler.try_borrow_mut() { - Ok(mut handler) => { - handler.destroy(); - Ok(()) - } - Err(err) => Err(Error::BorrowError(format!( - "Window::handle_destroy_notify handle: {}", - err - ))), - } + borrow_mut!(self.handler)?.destroy(); + Ok(()) } } @@ -906,7 +817,7 @@ impl WindowHandle { pub fn get_scale(&self) -> Result { if let Some(w) = self.window.upgrade() { - w.get_scale().map_err(ShellError::Platform) + Ok(w.get_scale()?) } else { log::error!("Window {} has already been dropped", self.id); Ok(Scale::from_dpi(96.0, 96.0)) diff --git a/druid-shell/src/util.rs b/druid-shell/src/util.rs index b58aefdf1f..5ffa211602 100644 --- a/druid-shell/src/util.rs +++ b/druid-shell/src/util.rs @@ -80,3 +80,37 @@ pub fn release_main_thread() { ); } } + +/// Wrapper around `RefCell::borrow` that provides error context. +// These are currently only used in the X11 backend, so suppress the unused warning in other +// backends. +#[allow(unused_macros)] +macro_rules! borrow { + ($val:expr) => {{ + use anyhow::Context; + $val.try_borrow().with_context(|| { + format!( + "[{}:{}] {}", + std::file!(), + std::line!(), + std::stringify!($val) + ) + }) + }}; +} + +/// Wrapper around `RefCell::borrow_mut` that provides error context. +#[allow(unused_macros)] +macro_rules! borrow_mut { + ($val:expr) => {{ + use anyhow::Context; + $val.try_borrow_mut().with_context(|| { + format!( + "[{}:{}] {}", + std::file!(), + std::line!(), + std::stringify!($val) + ) + }) + }}; +}