Merge pull request #534 from storytold/pr/linux-clipboard-probe

Linux: check the system clipboard for Paste off the UI thread (#510)

apps/vectorcraft/src/main.rs

@@ -29,7 +29,7 @@ mod window;
 use vectorcraft_engine::Session;
 use vectorcraft_engine::cmd::fileio;
 use vectorcraft_ui_egui::graphics::GraphicsLoss;
-use vectorcraft_ui_egui::{FilePick, Services, VectorcraftApp};
+use vectorcraft_ui_egui::{ClipboardProbeFactory, FilePick, Services, VectorcraftApp};
 
 struct App {
     app: VectorcraftApp,
@@ -256,6 +256,10 @@ 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()),
+        // Linux checks whether Paste has something to take on a background thread: an X11 clipboard
+        // owner that never answers holds a read for up to 4 s. Windows only asks which formats the
+        // clipboard holds and macOS asks the pasteboard server, so they check in line.
+        clipboard_probe: cfg!(target_os = "linux").then(|| Box::new(clipboard::system_clipboard) as ClipboardProbeFactory),
         // Help → Discord / website / GitHub, the Discord button, About and Home links.
         open_url: Some(Box::new(|url: &str| {
             let _ = webbrowser::open(url);

crates/ui-egui/src/clipboard_probe.rs

@@ -0,0 +1,99 @@
+//! 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, sends its verdict when it changes, 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, RecvTimeoutError, Sender, TryRecvError};
+use std::time::{Duration, Instant};
+
+use vectorcraft_engine::cmd::clipboard::PASTE_ORDER;
+
+use crate::sysclip::ClipboardProbeFactory;
+
+/// How often the worker looks at the clipboard while the UI wants to know.
+const PROBE_INTERVAL: Duration = Duration::from_millis(250);
+
+/// How long dropping the probe waits for the worker to let go of its clipboard handle.
+const STOP_WAIT: Duration = Duration::from_millis(250);
+
+/// The background clipboard check ([`crate::VectorcraftApp`]'s `clipboard_probe`).
+pub(crate) struct Probe {
+    /// Tells the worker whether Paste depends on the system clipboard; dropped to stop it.
+    wants: Option<Sender<bool>>,
+    wanted: bool,
+    verdicts: Receiver<bool>,
+    /// The worker's last verdict.
+    last: bool,
+}
+
+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<Self> {
+        if cfg!(target_arch = "wasm32") {
+            return None;
+        }
+        let (wants, wanted_rx) = std::sync::mpsc::channel();
+        let (tx, verdicts) = std::sync::mpsc::channel();
+        std::thread::Builder::new()
+            .name("vectorcraft-clip".into())
+            .spawn(move || {
+                let mut clipboard = make();
+                let (mut wanted, mut sent) = (false, None);
+                loop {
+                    if wanted {
+                        // This thread may wait here for a slow owner; the UI's never does.
+                        let verdict = clipboard.has(&PASTE_ORDER);
+                        if sent != Some(verdict) {
+                            if tx.send(verdict).is_err() {
+                                break;
+                            }
+                            sent = Some(verdict);
+                            ctx.request_repaint();
+                        }
+                    }
+                    // Sleep until the next look (no looks while unwanted) or the UI's next word.
+                    let next =
+                        if wanted { wanted_rx.recv_timeout(PROBE_INTERVAL) } else { wanted_rx.recv().map_err(|_| RecvTimeoutError::Disconnected) };
+                    match next {
+                        Ok(w) => wanted = w,
+                        Err(RecvTimeoutError::Timeout) => {}
+                        Err(RecvTimeoutError::Disconnected) => break,
+                    }
+                }
+                // The handle goes before the app stops waiting (`Drop`): on X11 the last one alive
+                // hands what the app copied to the clipboard manager, which must finish before exit.
+                drop(clipboard);
+                drop(tx);
+            })
+            .ok()?;
+        Some(Self { wants: Some(wants), wanted: false, verdicts, last: false })
+    }
+
+    /// Paste can take from the system clipboard: `wanted` (nothing copied in the app, a document
+    /// open) and the worker's latest verdict says so. Never blocks. `None` once the worker is gone.
+    pub(crate) fn pasteable(&mut self, wanted: bool) -> Option<bool> {
+        if wanted != self.wanted {
+            self.wants.as_ref()?.send(wanted).ok()?;
+            self.wanted = wanted;
+        }
+        loop {
+            match self.verdicts.try_recv() {
+                Ok(verdict) => self.last = verdict,
+                Err(TryRecvError::Empty) => return Some(wanted && self.last),
+                Err(TryRecvError::Disconnected) => return None,
+            }
+        }
+    }
+}
+
+impl Drop for Probe {
+    /// Wake the worker to stop and wait (a moment at most: it may be stuck on an owner that never
+    /// answers) until it has let go of its clipboard handle.
+    fn drop(&mut self) {
+        self.wants = None;
+        let end = Instant::now() + STOP_WAIT;
+        while self.verdicts.recv_timeout(end.saturating_duration_since(Instant::now())).is_ok() {}
+    }
+}

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<Box<dyn SystemClipboard>>,
+    /// A second system-clipboard handle, for checking whether Paste has something to take on a
+    /// background thread (Linux: an X11 clipboard owner that never answers would freeze the UI).
+    /// Taken on the first frame. Without it the check runs on the UI thread.
+    pub clipboard_probe: Option<ClipboardProbeFactory>,
     /// File → Show in Folder: select a file in the system file manager (desktop).
     pub reveal: Option<RevealFn>,
     /// Write a file from any thread (desktop): lets Background Save and Export write off the UI
@@ -327,8 +334,11 @@ pub struct VectorcraftApp {
     /// (SVG without a `system_clipboard`) while the internal clipboard is empty. It enables the
     /// Paste menu items.
     pub(crate) system_paste: bool,
-    /// When `system_paste` was last checked (app time, s; at most once per frame).
+    /// When `system_paste` was last checked in line (app time, s; at most once per frame).
     system_paste_at: f64,
+    /// The thread that checks the system clipboard for `system_paste` instead
+    /// ([`Services::clipboard_probe`]).
+    clipboard_probe: Option<clipboard_probe::Probe>,
     /// 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).
@@ -404,6 +414,7 @@ impl VectorcraftApp {
             place: Default::default(),
             system_paste: false,
             system_paste_at: f64::NEG_INFINITY,
+            clipboard_probe: None,
             paste_chord: Default::default(),
             background: Default::default(),
             recovery: Default::default(),
@@ -729,11 +740,20 @@ 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)) {
+        // Read the system clipboard only when that alone decides whether Paste is enabled. A
+        // background thread reads it where the host installs one (an unresponsive owner must never
+        // stall the frame loop); otherwise, or once that thread is gone, it is read here, at most a
+        // few times a second (opening it locks it against other apps on some systems).
+        let wanted = self.session.clipboard.is_empty() && self.session.active().is_some();
+        if let Some(make) = self.services.clipboard_probe.take() {
+            self.clipboard_probe = clipboard_probe::Probe::start(make, ctx.clone());
+        }
+        if let Some(pasteable) = self.clipboard_probe.as_mut().and_then(|p| p.pasteable(wanted)) {
+            self.system_paste = pasteable;
+        } else if !(0.0..SYSTEM_CLIPBOARD_POLL).contains(&(now - self.system_paste_at)) {
+            self.clipboard_probe = None;
             self.system_paste_at = now;
-            self.system_paste = self.session.clipboard.is_empty() && self.session.active().is_some() && self.system_clipboard_pasteable();
+            self.system_paste = wanted && self.system_clipboard_pasteable();
         }
         background::poll(self);
         if !self.background.jobs.is_empty() {

crates/ui-egui/src/sysclip.rs

@@ -24,6 +24,11 @@ pub trait SystemClipboard {
     fn has(&mut self, mimes: &[&'static str]) -> bool;
 }
 
+/// Makes a second system-clipboard handle, to check off the UI thread whether Paste has something
+/// to take ([`Services::clipboard_probe`](crate::Services::clipboard_probe)). It is called on that
+/// thread and the handle lives and dies there, so the handle needn't be `Send`.
+pub type ClipboardProbeFactory = Box<dyn FnOnce() -> Box<dyn SystemClipboard> + 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);

crates/ui-egui/src/tests_sysclip.rs

@@ -66,7 +66,7 @@ pub(super) fn run(app: &mut VectorcraftApp, id: &str, p: Value) -> Value {
     app.run(id, p).unwrap_or_else(|e| panic!("{id}: {e}"))
 }
 
-fn copy_rect(app: &mut VectorcraftApp) {
+pub(super) fn copy_rect(app: &mut VectorcraftApp) {
     let id = run(app, "shape.rectangle", json!({"x": 10, "y": 10, "width": 40, "height": 20}))["id"].clone();
     run(app, "paint.setFill", json!({"color": "#ff0000"}));
     run(app, "select.set", json!({"ids": [id]}));

crates/ui-egui/src/tests_sysclip_probe.rs

@@ -0,0 +1,156 @@
+//! The system clipboard is checked for Paste off the UI thread where the host installs a probe
+//! (Linux): an X11 clipboard owner that never answers used to block the frame loop for seconds, so
+//! the window stopped drawing and could not even be closed. The in-line check (no probe: the web,
+//! Windows, macOS) is covered by `tests_clipboard` and `tests_sysclip`.
+
+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::tests_sysclip::{copy_rect, run};
+use crate::{Services, SystemClipboard, VectorcraftApp, menus};
+
+/// What the clipboard handles saw: the thread each `has()` ran on, and how many handles are alive.
+#[derive(Default)]
+struct Recorder {
+    threads: Mutex<Vec<Option<String>>>,
+    alive: AtomicUsize,
+}
+
+impl Recorder {
+    fn looks(&self) -> usize {
+        self.threads.lock().unwrap().len()
+    }
+}
+
+/// A system clipboard whose `has()` takes `delay` (an owner slow to answer) and answers `verdict`,
+/// or panics without one (a bug in the platform code).
+struct Slow {
+    rec: Arc<Recorder>,
+    delay: Duration,
+    verdict: Option<bool>,
+}
+
+impl Slow {
+    fn boxed(rec: &Arc<Recorder>, delay: Duration, verdict: Option<bool>) -> Box<dyn SystemClipboard> {
+        rec.alive.fetch_add(1, Ordering::SeqCst);
+        Box::new(Self { rec: Arc::clone(rec), delay, verdict })
+    }
+}
+
+impl Drop for Slow {
+    fn drop(&mut self) {
+        self.rec.alive.fetch_sub(1, Ordering::SeqCst);
+    }
+}
+
+impl SystemClipboard for Slow {
+    fn write(&mut self, _flavours: &[Flavour]) -> Result<(), String> {
+        Ok(())
+    }
+    fn holds_ours(&mut self) -> bool {
+        false
+    }
+    fn read(&mut self, _mimes: &[&'static str]) -> Option<Flavour> {
+        None
+    }
+    fn has(&mut self, _mimes: &[&'static str]) -> bool {
+        self.rec.threads.lock().unwrap().push(std::thread::current().name().map(str::to_owned));
+        std::thread::sleep(self.delay);
+        self.verdict.expect("the clipboard failed")
+    }
+}
+
+/// An app whose two clipboard handles record into `rec`: the UI's says `inline`, the probe's
+/// `probe`, each after `delay`.
+fn app(rec: &Arc<Recorder>, delay: Duration, inline: bool, probe: Option<bool>) -> VectorcraftApp {
+    let make = {
+        let rec = Arc::clone(rec);
+        move || Slow::boxed(&rec, delay, probe)
+    };
+    let services =
+        Services { system_clipboard: Some(Slow::boxed(rec, delay, Some(inline))), clipboard_probe: Some(Box::new(make)), ..Default::default() };
+    VectorcraftApp::new(Session::new(), services)
+}
+
+/// One headless frame of app logic in the test's one context (a new one would restart the fonts),
+/// half a second after the last: past the in-line check's throttle, so it would run every frame.
+fn frame(app: &mut VectorcraftApp) {
+    thread_local! {
+        static CTX: egui::Context = egui::Context::default();
+        static NOW: std::cell::Cell<f64> = const { std::cell::Cell::new(0.0) };
+    }
+    let time = NOW.with(|t| {
+        t.set(t.get() + 0.5);
+        t.get()
+    });
+    let input = egui::RawInput { time: Some(time), ..Default::default() };
+    CTX.with(|ctx| ctx.run_ui(input, |ui| app.logic(ui.ctx())).textures_delta.clear());
+}
+
+/// Frames for `wait`, or until `done` holds; whether it does.
+fn frames_until(app: &mut VectorcraftApp, wait: Duration, done: impl Fn(&VectorcraftApp) -> bool) -> bool {
+    let end = Instant::now() + wait;
+    loop {
+        frame(app);
+        if done(app) || Instant::now() > end {
+            return done(app);
+        }
+        std::thread::sleep(Duration::from_millis(5));
+    }
+}
+
+#[test]
+fn a_slow_clipboard_is_read_off_the_ui_thread() {
+    let rec = Arc::new(Recorder::default());
+    let mut app = app(&rec, Duration::from_secs(1), false, Some(true));
+    // The first frames install the UI fonts. Without a document Paste is off whatever the clipboard
+    // holds: nobody looks.
+    assert!(!frames_until(&mut app, Duration::from_millis(100), |_| rec.looks() > 0));
+    run(&mut app, "file.new", json!({"width": 100, "height": 100}));
+    frame(&mut app);
+    let t0 = Instant::now();
+    frame(&mut app);
+    assert!(t0.elapsed() < Duration::from_millis(300), "the frame waited on the clipboard: {:?}", t0.elapsed());
+    // The probe's verdict (the UI's own handle would say no) enables Paste once it arrives.
+    assert!(frames_until(&mut app, Duration::from_secs(3), |app| app.system_paste), "the probe's verdict never arrived");
+    assert!(menus::enabled(&app, "edit.paste"));
+    let threads = rec.threads.lock().unwrap().clone();
+    assert!(threads.iter().all(|n| n.as_deref() == Some("vectorcraft-clip")), "checked on: {threads:?}");
+}
+
+#[test]
+fn the_probe_rests_after_a_copy_and_stops_with_the_app() {
+    let rec = Arc::new(Recorder::default());
+    let mut app = app(&rec, Duration::ZERO, false, Some(true));
+    run(&mut app, "file.new", json!({"width": 100, "height": 100}));
+    assert!(frames_until(&mut app, Duration::from_secs(2), |app| app.system_paste));
+    // Something copied in the app enables Paste alone: the clipboard isn't read any more.
+    copy_rect(&mut app);
+    frames_until(&mut app, Duration::from_millis(50), |_| false);
+    let looks = rec.looks();
+    frames_until(&mut app, Duration::from_millis(600), |_| false);
+    assert_eq!(rec.looks(), looks);
+    assert!(!app.system_paste && menus::enabled(&app, "edit.paste"));
+    // Quitting waits until the probe's thread has let go of its handle: on X11 the last handle
+    // alive hands what the app copied to the clipboard manager.
+    assert_eq!(rec.alive.load(Ordering::SeqCst), 2);
+    drop(app);
+    assert_eq!(rec.alive.load(Ordering::SeqCst), 0, "the probe's thread kept its clipboard");
+}
+
+#[test]
+fn without_its_thread_the_check_runs_in_line_again() {
+    let rec = Arc::new(Recorder::default());
+    // The probe's clipboard panics: its thread ends.
+    let mut app = app(&rec, Duration::ZERO, true, None);
+    run(&mut app, "file.new", json!({"width": 100, "height": 100}));
+    assert!(frames_until(&mut app, Duration::from_secs(2), |app| app.system_paste), "no fallback");
+    assert!(app.clipboard_probe.is_none());
+    let threads = rec.threads.lock().unwrap().clone();
+    assert_eq!(threads.first().cloned().flatten().as_deref(), Some("vectorcraft-clip"), "{threads:?}");
+}

docs/development.md

@@ -74,6 +74,7 @@ By default VectorCraft's own crates log at `info` and everything else at `warn`.
 The window runs natively on Wayland (eframe's `wayland` feature) and on X11. On machines with two GPUs it renders on the one the desktop runs on unless Preferences say otherwise; see [Desktop graphics processor](#desktop-graphics-processor) (#502). The system clipboard (`apps/vectorcraft/src/clipboard.rs`) is arboard with its `wayland-data-control` feature:
 
 - **Clipboard:** under Wayland arboard talks to the compositor through the data-control protocol (wlroots compositors such as Sway and Hyprland, KDE Plasma), so bitmaps (`image/png`), SVG markup and text copied in any app paste (#398). Where the compositor lacks the protocol (GNOME's Mutter, [arboard#223](https://github.com/1Password/arboard/issues/223)), arboard falls back to the X11 clipboard through XWayland, which only holds what X11 apps copied. egui's own text paste into fields goes through smithay-clipboard and works either way.
+- **Clipboard owners that never answer (#510):** reading the clipboard waits on the app that owns it: arboard gives up on an X11 owner after 4 s, and a Wayland read waits for the owner to send its data. So on Linux the check that enables the Paste menu items runs on a background thread with a second clipboard handle (`crates/ui-egui/src/clipboard_probe.rs`, installed as `Services::clipboard_probe`), only while nothing is copied in the app and a document is open, and the window keeps drawing. A paste itself still reads on the UI thread, so pasting from such an owner waits for it once. Windows and macOS keep the check on the UI thread: Windows only asks which formats the clipboard holds, macOS asks the pasteboard server.
 - **Copied files:** files copied in a file manager (`text/uri-list`; `CF_HDROP` on Windows, file URLs on macOS) paste as the first one that is art (SVG, PDF, EMF/WMF or a bitmap), before the clipboard's other formats (which include the files' paths as text).
 - **Dropping files on the window does not work under Wayland:** winit 0.30, which eframe 0.36 runs on, has no Wayland drag and drop ([winit#1881](https://github.com/rust-windowing/winit/issues/1881), added in winit 0.31; [egui#1563](https://github.com/emilk/egui/issues/1563)), so no `dropped_files` arrive. Until eframe moves to winit 0.31: copy the files in the file manager and paste them, use File › Place or Open, or run the app under XWayland, where drops work (`WAYLAND_DISPLAY= vectorcraft`).
 - **Pens and pen displays under Wayland (#491):** winit 0.30 doesn't bind the Wayland tablet protocol (`zwp_tablet_v2`; tablet input arrives in winit 0.31, [winit#4318](https://github.com/rust-windowing/winit/pull/4318), [egui#7731](https://github.com/emilk/egui/pull/7731)). Since Plasma 6.3, KWin no longer turns tablet input into pointer input for such apps ([kwin!6336](https://invent.kde.org/plasma/kwin/-/merge_requests/6336)): the pen moves the compositor's cursor over the window, but the app gets no motion or presses and shows none of its own cursors. Until eframe moves to winit 0.31, either run the app under XWayland, which supports the tablet protocol and hands the pen to X11 apps as a pointer (`WAYLAND_DISPLAY= vectorcraft`, or `env -u WAYLAND_DISPLAY vectorcraft`; for a desktop launcher, put `env WAYLAND_DISPLAY=` before the command in its `Exec=` line), or turn KWin's deprecated emulation back on for the whole session by setting `KWIN_WAYLAND_EMULATE_TABLET=1` in KWin's environment (for example in a file under `~/.config/environment.d/` or in `/etc/environment`) and logging in again. The pen then works as a mouse, without pressure: winit 0.30 reads pen pressure on Windows only (Windows Ink, which reaches the Liquify tools' Use Pressure Pen). Other compositors that don't emulate a pointer for tablets behave the same way.