From 5db200d392c877ac877dbcb4eb35a9cf97c2ca32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oscar=20Sebasti=C3=A1n=20Mellado=20Bustos?= Date: Thu, 8 Oct 2026 10:05:09 -0300 Subject: [PATCH] Linux: check the system clipboard for Paste off the UI thread `has()` on an X11 clipboard owner that never answers can block for seconds; the app called it every 0.25 s on the UI thread to decide whether Paste is enabled, so a stuck owner froze the frame loop (the window stopped drawing and could not be closed). `clipboard_probe` owns a second clipboard handle on a worker thread, publishes its verdict over a channel and wakes the UI only when it changes; the frame reads the latest verdict without ever blocking. Without a factory the check stays in line, throttled, exactly as before (the web shows SVG text only; hosts that don't install one keep the old path). The factory is installed on Linux only for now (the environment the bug reproduces in); other platforms keep the in-line check, so this cannot change their behaviour. Tests: a fake clipboard whose `has()` blocks on a gate, a probe factory sharing a recorder, and a headless frame whose elapsed time is measured. They assert the frame does not wait on the clipboard and that the check runs on the probe thread, that the probe's verdict reaches `system_paste` and the Paste menu, that dropping the app stops the probe, and that the in-line check (no factory: the web, older hosts) still works. --- apps/vectorcraft/src/main.rs | 16 ++ crates/ui-egui/src/clipboard_probe.rs | 68 +++++++ crates/ui-egui/src/lib.rs | 47 ++++- crates/ui-egui/src/sysclip.rs | 6 + crates/ui-egui/src/tests_sysclip_probe.rs | 205 ++++++++++++++++++++++ 5 files changed, 335 insertions(+), 7 deletions(-) create mode 100644 crates/ui-egui/src/clipboard_probe.rs create mode 100644 crates/ui-egui/src/tests_sysclip_probe.rs 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")); +}