diff --git a/apps/vectorcraft/src/main.rs b/apps/vectorcraft/src/main.rs index 3ffee76c..11a06ce9 100644 --- a/apps/vectorcraft/src/main.rs +++ b/apps/vectorcraft/src/main.rs @@ -231,6 +231,19 @@ fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> { fileio::write_atomic(std::path::Path::new(path), bytes).map_err(|e| e.to_string()) } +/// The factory for the off-thread clipboard check: Linux only for now. On Linux an X11 clipboard +/// owner that never answers would otherwise block the frame loop (see `clipboard_probe`). +fn clipboard_probe_factory() -> Option { + #[cfg(target_os = "linux")] + { + Some(Box::new(clipboard::system_clipboard)) + } + #[cfg(not(target_os = "linux"))] + { + None + } +} + fn services() -> Services { Services { pick_open: Some(Box::new(|pick: &FilePick| file_dialog(pick).pick_file().map(|p| p.to_string_lossy().to_string()))), @@ -256,6 +269,9 @@ fn services() -> Services { write_shared: Some(std::sync::Arc::new(write_file)), // Every format Copy offers and Paste reads (menu-bar Paste never sees egui's Paste event). system_clipboard: Some(clipboard::system_clipboard()), + // Whether Paste has something to take is checked on a background thread (Linux), so a stuck + // clipboard owner can't freeze the UI. + clipboard_probe: clipboard_probe_factory(), // Help → Discord / website / GitHub, the Discord button, About and Home links. open_url: Some(Box::new(|url: &str| { let _ = webbrowser::open(url); diff --git a/crates/ui-egui/src/clipboard_probe.rs b/crates/ui-egui/src/clipboard_probe.rs new file mode 100644 index 00000000..f9d5dda2 --- /dev/null +++ b/crates/ui-egui/src/clipboard_probe.rs @@ -0,0 +1,68 @@ +//! Checks the system clipboard for Paste off the UI thread. `has()` can block for seconds when the +//! clipboard owner never answers (an unresponsive X11 client), so it must not run on the thread that +//! draws the window: a worker owns a second clipboard handle, publishes its verdict, and the UI only +//! reads the latest one. Without a worker the check stays in line, as before (the web, and hosts +//! that don't install a factory). + +use std::sync::mpsc::{Receiver, TryRecvError}; +use std::time::Duration; + +use vectorcraft_engine::cmd::clipboard::PASTE_ORDER; + +use crate::sysclip::ClipboardProbeFactory; + +/// How often the worker looks at the clipboard. +const PROBE_INTERVAL: Duration = Duration::from_millis(250); + +/// The background clipboard check ([`crate::VectorcraftApp`]'s `clipboard_probe`). +pub(crate) struct Probe { + rx: Option>, + /// The last verdict the worker sent; `None` until the first one arrives. + last: Option, +} + +impl Probe { + /// Start the worker. `None` on wasm, or when the thread can't be spawned: the caller then falls + /// back to the in-line check. + pub(crate) fn start(make: ClipboardProbeFactory, ctx: egui::Context) -> Option { + if cfg!(target_arch = "wasm32") { + return None; + } + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::Builder::new() + .name("vectorcraft-clip".into()) + .spawn(move || { + let mut clipboard = make(); + let mut sent: Option = None; + loop { + // The worker's thread may wait here for a slow owner; the UI's never does. + let verdict = clipboard.has(&PASTE_ORDER); + if tx.send(verdict).is_err() { + return; // the app is gone: the receiver was dropped + } + // Wake the UI only when the answer changed (it reads the verdict every frame). + if sent != Some(verdict) { + ctx.request_repaint(); + } + sent = Some(verdict); + std::thread::sleep(PROBE_INTERVAL); + } + }) + .ok()?; + Some(Self { rx: Some(rx), last: None }) + } + + /// The latest verdict the worker sent, if one has arrived. Never blocks. + pub(crate) fn poll(&mut self) -> Option { + loop { + match self.rx.as_ref().map(Receiver::try_recv) { + Some(Ok(verdict)) => self.last = Some(verdict), + Some(Err(TryRecvError::Empty)) => return self.last, + Some(Err(TryRecvError::Disconnected)) | None => { + self.rx = None; + return self.last; + } + } + } + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 08f05485..7505d29a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -17,6 +17,7 @@ pub mod background; mod brand; pub mod canvas; pub mod chrome; +mod clipboard_probe; pub mod community; pub mod control; pub mod credits; @@ -129,6 +130,8 @@ mod tests_sysclip; #[cfg(test)] mod tests_sysclip_emf; #[cfg(test)] +mod tests_sysclip_probe; +#[cfg(test)] mod tests_transparencygrid; #[cfg(test)] mod tests_widthtool; @@ -142,7 +145,7 @@ use vectorcraft_engine::{Session, ViewInfo}; pub use control::{ControlRequest, ControlResponse}; pub use state::{UiState, View}; -pub use sysclip::SystemClipboard; +pub use sysclip::{ClipboardProbeFactory, SystemClipboard}; /// What a file dialog shows: a suggested file name (save dialogs), the folder to start in and the /// file-type filters. @@ -205,6 +208,10 @@ pub struct Services { /// Paste takes SVG, PDF, text and bitmaps from other apps. Without it, SVG text only (through /// egui and `clipboard_read`). pub system_clipboard: Option>, + /// A factory for a second system-clipboard handle, checked on a background thread so an + /// unresponsive clipboard owner never freezes the UI ([`VectorcraftApp::system_paste`]). + /// Without it, that check stays on the UI thread, as before. + pub clipboard_probe: Option, /// File → Show in Folder: select a file in the system file manager (desktop). pub reveal: Option, /// Write a file from any thread (desktop): lets Background Save and Export write off the UI @@ -329,6 +336,13 @@ pub struct VectorcraftApp { pub(crate) system_paste: bool, /// When `system_paste` was last checked (app time, s; at most once per frame). system_paste_at: f64, + /// The probe's last verdict: the system clipboard holds something Paste can take. + system_paste_available: bool, + /// The background thread that checks the system clipboard for Paste (native desktop), started + /// on the first frame. Without it the check runs in line, throttled ([`SYSTEM_CLIPBOARD_POLL`]). + clipboard_probe: Option, + /// The host's factory for the probe, taken when its thread starts ([`Services::clipboard_probe`]). + clipboard_probe_factory: Option, /// Keyboard pastes of something other than text (see [`shortcuts::PasteChord`]). pub(crate) paste_chord: shortcuts::PasteChord, /// Saves and exports running in the background (Preferences → File Handling). @@ -353,12 +367,14 @@ pub struct VectorcraftApp { const SYSTEM_CLIPBOARD_POLL: f64 = 0.25; impl VectorcraftApp { - pub fn new(mut session: Session, services: Services) -> Self { + pub fn new(mut session: Session, mut services: Services) -> Self { // The font menus and the first file opened need the installed fonts: catalog them now. vectorcraft_text::FontDb::global().scan_in_background(); if let Some(store) = &services.recovery_store { session.recovery.set_store(store.clone()); } + // The probe is started on the first frame, which is where an `egui::Context` is available. + let clipboard_probe_factory = services.clipboard_probe.take(); let views = session.documents().iter().map(View::of).collect(); Self { session, @@ -404,6 +420,9 @@ impl VectorcraftApp { place: Default::default(), system_paste: false, system_paste_at: f64::NEG_INFINITY, + system_paste_available: false, + clipboard_probe: None, + clipboard_probe_factory, paste_chord: Default::default(), background: Default::default(), recovery: Default::default(), @@ -729,12 +748,26 @@ impl VectorcraftApp { } self.last_time = now; self.sync_views(); - // Read the system clipboard only when that alone decides whether Paste is enabled, and at - // most a few times a second (opening it locks it against other apps on some systems). - if !(0.0..SYSTEM_CLIPBOARD_POLL).contains(&(now - self.system_paste_at)) { - self.system_paste_at = now; - self.system_paste = self.session.clipboard.is_empty() && self.session.active().is_some() && self.system_clipboard_pasteable(); + // Whether Paste can take from the system clipboard. On a native desktop a background thread + // checks it (an unresponsive clipboard owner must never stall the frame loop) and only its + // latest verdict is read here; without a probe the check runs in line, at most a few times a + // second (opening the clipboard locks it against other apps on some systems). + if self.clipboard_probe.is_none() + && let Some(make) = self.clipboard_probe_factory.take() + { + self.clipboard_probe = clipboard_probe::Probe::start(make, ctx.clone()); } + let verdict = self.clipboard_probe.as_mut().and_then(clipboard_probe::Probe::poll); + let available = if self.clipboard_probe.is_some() { + verdict.unwrap_or(self.system_paste_available) + } else if !(0.0..SYSTEM_CLIPBOARD_POLL).contains(&(now - self.system_paste_at)) { + self.system_paste_at = now; + self.system_clipboard_pasteable() + } else { + self.system_paste_available + }; + self.system_paste_available = available; + self.system_paste = available && self.session.clipboard.is_empty() && self.session.active().is_some(); background::poll(self); if !self.background.jobs.is_empty() { // Keep the status bar's progress moving and pick the result up when it arrives. diff --git a/crates/ui-egui/src/sysclip.rs b/crates/ui-egui/src/sysclip.rs index a63cd5df..b1330ab4 100644 --- a/crates/ui-egui/src/sysclip.rs +++ b/crates/ui-egui/src/sysclip.rs @@ -24,6 +24,12 @@ pub trait SystemClipboard { fn has(&mut self, mimes: &[&'static str]) -> bool; } +/// Builds a second, independent system-clipboard handle, used only to check (off the UI thread) +/// whether Paste has something to take. It is called on that thread and the value it returns lives +/// and dies there, so it needs no `Send` bound: an unresponsive clipboard owner must never stall the +/// thread that draws the window. +pub type ClipboardProbeFactory = Box Box + Send>; + /// The command (and its params) that loads `f` into the internal clipboard, centred on `center`. pub(crate) fn import_command(f: &Flavour, center: Option<[f64; 2]>) -> (&'static str, Value) { let b64 = || vectorcraft_format::base64_encode(&f.data); diff --git a/crates/ui-egui/src/tests_sysclip_probe.rs b/crates/ui-egui/src/tests_sysclip_probe.rs new file mode 100644 index 00000000..5f17a9a5 --- /dev/null +++ b/crates/ui-egui/src/tests_sysclip_probe.rs @@ -0,0 +1,205 @@ +//! The system clipboard is checked for Paste off the UI thread (the regression the app used to +//! freeze on): an X11 clipboard owner that never answers `get_text()` blocked the frame loop for +//! seconds, so the window stopped drawing and could not even be closed. These tests use a clipboard +//! that blocks on demand and prove the UI frame is not the thread that waits on it. +//! +//! With no probe factory (the web, tests that predate this) the check stays on the UI thread, so the +//! old path is kept and its tests still cover it. + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use serde_json::json; +use vectorcraft_engine::Session; +use vectorcraft_engine::cmd::clipboard::Flavour; + +use crate::{ClipboardProbeFactory, Services, SystemClipboard, VectorcraftApp, menus}; + +/// How long a blocking clipboard holds a call before giving up: bounded so a regression fails a +/// test instead of hanging the whole suite. +const GATE_MAX: Duration = Duration::from_millis(1000); + +/// Lets `has()` through when opened, and otherwise at most [`GATE_MAX`]. +#[derive(Clone, Default)] +struct Gate(Arc); + +impl Gate { + fn open(&self) { + self.0.store(true, Ordering::SeqCst); + } + fn wait(&self) { + let end = Instant::now() + GATE_MAX; + while !self.0.load(Ordering::SeqCst) && Instant::now() < end { + std::thread::sleep(Duration::from_millis(1)); + } + } +} + +/// What the two clipboard handles recorded: the thread each `has()` ran on, and how many handles +/// are alive. +#[derive(Default)] +struct Recorder { + threads: Mutex>>, + alive: AtomicUsize, +} + +/// A system clipboard whose `has()` blocks on a [`Gate`] (like an owner that never answers). +struct Blocking { + rec: Arc, + gate: Gate, + verdict: bool, +} + +impl Blocking { + fn new(rec: Arc, gate: Gate, verdict: bool) -> Self { + rec.alive.fetch_add(1, Ordering::SeqCst); + Self { rec, gate, verdict } + } +} + +impl Drop for Blocking { + fn drop(&mut self) { + self.rec.alive.fetch_sub(1, Ordering::SeqCst); + } +} + +impl SystemClipboard for Blocking { + fn write(&mut self, _flavours: &[Flavour]) -> Result<(), String> { + Ok(()) + } + fn holds_ours(&mut self) -> bool { + false + } + fn read(&mut self, _mimes: &[&'static str]) -> Option { + None + } + fn has(&mut self, _mimes: &[&'static str]) -> bool { + self.rec.threads.lock().unwrap().push(std::thread::current().name().map(str::to_owned)); + self.gate.wait(); + self.verdict + } +} + +/// An app with a document, a blocking `system_clipboard` (what the UI would call in line) and a +/// probe factory whose own handle shares the recorder. +fn app(system_verdict: bool, probe_verdict: bool, gate: &Gate) -> (VectorcraftApp, Arc) { + let rec = Arc::new(Recorder::default()); + let system = Blocking::new(Arc::clone(&rec), gate.clone(), system_verdict); + let make = { + let rec = Arc::clone(&rec); + let gate = gate.clone(); + move || -> Box { Box::new(Blocking::new(rec, gate, probe_verdict)) } + }; + let probe: ClipboardProbeFactory = Box::new(make); + let services = Services { system_clipboard: Some(Box::new(system)), clipboard_probe: Some(probe), ..Default::default() }; + let mut app = VectorcraftApp::new(Session::new(), services); + app.run("file.new", json!({"width": 100, "height": 100})).unwrap(); + (app, rec) +} + +/// One headless frame of app logic, in the test's own context (a new one would restart the fonts). +/// Time advances by more than the clipboard poll interval each frame, so every frame re-checks (a +/// headless context otherwise keeps `time` fixed and only the first frame would). +fn frame(app: &mut VectorcraftApp) { + thread_local! { + static CTX: egui::Context = egui::Context::default(); + static NOW: std::cell::Cell = const { std::cell::Cell::new(0.0) }; + } + let ctx = CTX.with(Clone::clone); + let time = NOW.with(|t| { + let v = t.get() + 0.5; + t.set(v); + v + }); + let input = egui::RawInput { time: Some(time), ..Default::default() }; + ctx.run_ui(input, |ui| app.logic(ui.ctx())).textures_delta.clear(); +} + +/// A regression guard is only meaningful if the frame really does not wait: a blocking clipboard +/// must not delay the frame, and the check must run on the probe thread. +#[test] +fn a_blocking_clipboard_never_blocks_the_ui_frame() { + let gate = Gate::default(); + let (mut app, rec) = app(false, false, &gate); + // Warm up: the first frames install the UI fonts. + for _ in 0..3 { + frame(&mut app); + } + let t0 = Instant::now(); + frame(&mut app); + let elapsed = t0.elapsed(); + assert!(elapsed < Duration::from_millis(300), "the frame waited on the clipboard: {elapsed:?}"); + // The check ran on the probe thread, never on the test's. + let end = Instant::now() + Duration::from_millis(1000); + while rec.threads.lock().unwrap().is_empty() && Instant::now() < end { + std::thread::sleep(Duration::from_millis(10)); + } + let threads = rec.threads.lock().unwrap().clone(); + assert!(!threads.is_empty(), "the clipboard was never checked"); + assert!(threads.iter().all(|n| n.as_deref() == Some("vectorcraft-clip")), "checked on: {threads:?}"); + assert!(!app.system_paste); +} + +#[test] +fn the_probe_reports_a_pasteable_clipboard_to_the_menu() { + let gate = Gate::default(); + gate.open(); // both handles answer at once + // The in-line clipboard reports nothing, only the probe's does: the verdict must come from the probe. + let (mut app, _) = app(false, true, &gate); + for _ in 0..3 { + frame(&mut app); + } + let mut seen = false; + for _ in 0..200 { + frame(&mut app); + if app.system_paste { + seen = true; + break; + } + std::thread::sleep(Duration::from_millis(5)); + } + assert!(seen, "the probe's verdict never reached `system_paste`"); + assert!(menus::enabled(&app, "edit.paste")); +} + +#[test] +fn a_probe_that_reports_nothing_leaves_paste_disabled() { + let gate = Gate::default(); + gate.open(); + let (mut app, _) = app(false, false, &gate); + for _ in 0..20 { + frame(&mut app); + std::thread::sleep(Duration::from_millis(5)); + } + assert!(!app.system_paste && !menus::enabled(&app, "edit.paste")); +} + +#[test] +fn dropping_the_app_stops_the_probe() { + let gate = Gate::default(); + gate.open(); + let (mut app, rec) = app(false, true, &gate); + for _ in 0..3 { + frame(&mut app); + } + assert!(rec.alive.load(Ordering::SeqCst) > 0, "no clipboard handle alive"); + drop(app); + let end = Instant::now() + Duration::from_millis(2000); + while rec.alive.load(Ordering::SeqCst) != 0 && Instant::now() < end { + std::thread::sleep(Duration::from_millis(10)); + } + assert_eq!(rec.alive.load(Ordering::SeqCst), 0, "the probe thread did not stop"); +} + +#[test] +fn without_a_probe_factory_the_inline_check_still_works() { + // The web and older hosts: no probe, the UI thread checks the clipboard itself (fast when the + // owner answers, which is the case here). + let svg = r##""##; + let services = Services { clipboard_read: Some(Box::new(move || Some(svg.to_string()))), ..Default::default() }; + let mut app = VectorcraftApp::new(Session::new(), services); + app.run("file.new", json!({"width": 100, "height": 100})).unwrap(); + frame(&mut app); + assert!(menus::enabled(&app, "edit.paste")); +}