From ff92cb3ffde57df60d06bb67a0192ff8f11ed7bd Mon Sep 17 00:00:00 2001 From: jonathnwxh <258340642+jonathanwxh-cell@users.noreply.github.com> Date: Wed, 14 Oct 2026 22:57:13 +0800 Subject: [PATCH] Preserve editable clipboard contents when native reads fail --- apps/photocraft/src/services.rs | 83 +++++++++++++++--- crates/ui-egui/src/control.rs | 2 +- crates/ui-egui/src/lib.rs | 150 +++++++++++++++++++++++++++----- crates/ui-egui/src/menus.rs | 2 +- 4 files changed, 204 insertions(+), 33 deletions(-) diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index e6c882d198..8b4c7c045e 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -475,23 +475,35 @@ pub fn native(automation: Option) -> clipboard_get_image: Some({ let clip = clip.clone(); Box::new(move || { - let mut slot = clip.try_borrow_mut().ok()?; + let mut slot = clip.try_borrow_mut().map_err(|_| "clipboard is busy".to_string())?; let cb = match slot.as_mut() { Some(c) => c, - None => slot.insert(arboard::Clipboard::new().ok()?), + None => slot.insert(arboard::Clipboard::new().map_err(|e| e.to_string())?), }; // Copied files first: Finder also puts the file's icon on the clipboard as an // image, which would otherwise paste instead of the file. - if let Some(img) = cb.get().file_list().ok().and_then(|paths| image_from_files(&paths)) { - return Some(img); + match cb.get().file_list() { + Ok(paths) => { + if let Some(img) = image_from_files(&paths) { + return Ok(Some(img)); + } + } + Err(error @ arboard::Error::ClipboardOccupied) => return Err(error.to_string()), + Err(_) => {} } - if let Ok(img) = cb.get_image() { - return Some((img.width as u32, img.height as u32, img.bytes.into_owned())); + match cb.get_image() { + Ok(img) => Ok(Some((img.width as u32, img.height as u32, img.bytes.into_owned()))), + Err(error) => { + #[cfg(target_os = "linux")] + if let Some(image) = wayland_clipboard_image() { + return Ok(Some(image)); + } + match error { + arboard::Error::ContentNotAvailable => Ok(None), + _ => Err(error.to_string()), + } + } } - #[cfg(target_os = "linux")] - return wayland_clipboard_image(); - #[cfg(not(target_os = "linux"))] - None }) }), load_prefs: Some(Box::new(|| std::fs::read_to_string(prefs_file()?).ok())), @@ -597,6 +609,57 @@ mod tests { use photocraft_ui_egui::{PhotocraftApp, prefs_ui}; use serde_json::{Value, json}; + /// Regression using the real clipboard on an isolated Windows CI runner. + #[cfg(target_os = "windows")] + #[test] + #[ignore = "requires exclusive use of the Windows clipboard"] + fn busy_native_clipboard_preserves_copied_shape() { + for id in ["edit.paste", "edit.pasteSpecial.pasteInPlace"] { + let mut native = native(None); + let services = Services { + clipboard_set_image: native.clipboard_set_image.take(), + clipboard_get_image: native.clipboard_get_image.take(), + ..Default::default() + }; + let mut app = PhotocraftApp::new(Session::new(), services); + app.run("file.new", json!({"width": 80, "height": 60})).unwrap(); + app.run("shape.create", json!({"kind": "rect", "rect": [10, 10, 40, 20], "fill": "#ff0000"})).unwrap(); + app.run("edit.copy", json!({})).unwrap(); + assert!(app.session.clipboard.as_ref().unwrap().layers.is_some()); + assert!(app.services.clipboard_get_image.as_mut().unwrap()().unwrap().is_some(), "native export succeeded"); + app.run("file.new", json!({"width": 80, "height": 60})).unwrap(); + + // Another thread holds the OS clipboard open beyond arboard's bounded retry window. + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let holder = std::thread::spawn(move || { + // A non-null window distinguishes this owner from arboard's null-window opens. + let guard = winsafe::HWND::GetDesktopWindow().OpenClipboard().unwrap(); + ready_tx.send(()).unwrap(); + release_rx.recv_timeout(std::time::Duration::from_secs(10)).unwrap(); + drop(guard); + }); + ready_rx.recv_timeout(std::time::Duration::from_secs(10)).unwrap(); + let contention = arboard::Clipboard::new().unwrap().get_image().err(); + let first = app.run(id, json!({})); + release_tx.send(()).unwrap(); + holder.join().unwrap(); + assert!(matches!(contention, Some(arboard::Error::ClipboardOccupied)), "diagnostic must actually occupy the clipboard: {contention:?}"); + eprintln!("paste while clipboard occupied: {first:?}"); + assert!(first.is_err(), "report contention rather than a successful no-op"); + assert!( + app.session.clipboard.as_ref().is_some_and(|clip| clip.layers.is_some()), + "a temporary OS read failure must not discard the copied editable shape" + ); + app.run(id, json!({})).unwrap(); + let state = app.session.active().unwrap(); + assert!( + matches!(state.doc.layer(state.active_layer.unwrap()).unwrap().content, photocraft_doc::LayerContent::Shape(_)), + "retry keeps the shape editable" + ); + } + } + #[cfg(all(unix, not(target_os = "macos")))] #[test] fn open_filters_cover_uppercase_and_mixed_case_extensions() { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 209691b875..843e02dd76 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -1647,7 +1647,7 @@ mod tests { let image = clipboard.clone(); let mut app = PhotocraftApp::new( photocraft_engine::Session::new(), - crate::Services { clipboard_get_image: Some(Box::new(move || image.lock().ok()?.clone())), ..Default::default() }, + crate::Services { clipboard_get_image: Some(Box::new(move || Ok(image.lock().unwrap().clone()))), ..Default::default() }, ); let ctx = egui::Context::default(); let opened = call(&mut app, &ctx, "ui.dialog.open", json!({"kind":"newDocument"})); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 38118955c3..d75df23aa0 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -341,8 +341,9 @@ pub type OpenUrlFn = Box Result<(), String>>; pub type EncodePngFn = Box Result, String>>; /// Put an RGBA8 image (width, height, pixels) on the OS clipboard. pub type ClipboardSetFn = Box Result<(), String>>; -/// Read an RGBA8 image from the OS clipboard. -pub type ClipboardGetFn = Box Option<(u32, u32, Vec)>>; +/// Read an RGBA8 image from the OS clipboard. `Ok(None)` means no image is available; +/// `Err` means the contents could not be read and must not invalidate the internal copy. +pub type ClipboardGetFn = Box Result)>, String>>; /// Shared queue of (file name, bytes) delivered asynchronously. pub type Inbox = std::sync::Arc)>>>; /// Read the saved preferences text (`None` when there is none yet). @@ -1007,15 +1008,11 @@ impl PhotocraftApp { if let Some(r) = crate::crop_ui::intercept(self, id) { return r; } - let suppress_events = self.automation_input && self.session.prefs().script_events.enabled; - if suppress_events { - self.session.edit_prefs(|prefs| prefs.script_events.enabled = false); - } let t0 = gpu_canvas::now_ms(); // The OS clipboard is read only on an explicit paste, never in the background (privacy, CPU). if matches!(id, "edit.paste" | "edit.pasteSpecial.pasteInPlace" | "file.newFromClipboard") { if !clip_read { - self.import_os_clipboard(); + self.import_os_clipboard()?; } if self.session.clipboard.is_none() && self.services.clipboard_get_image.is_some() { // Enabled on the strength of the OS clipboard, which held no image: a quiet no-op. @@ -1025,6 +1022,10 @@ impl PhotocraftApp { return Ok(serde_json::json!({"pasted": false})); } } + let suppress_events = self.automation_input && self.session.prefs().script_events.enabled; + if suppress_events { + self.session.edit_prefs(|prefs| prefs.script_events.enabled = false); + } // Long commands become background jobs when enabled (`jobs_ui`); the rest run inline. let params = self.with_mask_target(id, params); let params = vector_ui::with_active_path(self, id, params); @@ -2446,7 +2447,9 @@ impl PhotocraftApp { } } } - self.import_os_clipboard(); + if self.import_os_clipboard().is_err() { + return f; + } if let Some(c) = self.session.clipboard.as_ref().filter(|c| !c.bounds.is_empty()) { crate::new_doc_ui::set_clipboard(&mut f, c.bounds.width(), c.bounds.height()); } @@ -2455,22 +2458,32 @@ impl PhotocraftApp { /// If the OS clipboard holds an image that isn't the one we put there, make it the session /// clipboard (so ⌘V pastes screenshots and images copied in other apps, like Photoshop). - /// Returns true when a new external image was imported. Once the OS clipboard no longer holds + /// Returns `Ok(true)` when a new external image was imported. Once the OS clipboard no longer holds /// the image we mirrored or imported (text or a file was copied since), that image is stale: /// it is dropped rather than pasted. - pub(crate) fn import_os_clipboard(&mut self) -> bool { - let Some(get) = self.services.clipboard_get_image.as_mut() else { return false }; - let image = get().filter(|(w, h, bytes)| *w > 0 && *h > 0 && bytes.len() == *w as usize * *h as usize * 4); + pub(crate) fn import_os_clipboard(&mut self) -> Result { + let Some(get) = self.services.clipboard_get_image.as_mut() else { return Ok(false) }; + // A read failure says nothing about the clipboard contents. Keep our editable copy and + // its signature, and abort this paste rather than treating the failure as an empty image. + let image = match get() { + Ok(image) => image, + Err(error) => { + self.ui.status = error.clone(); + self.ui.status_error = true; + return Err(error); + } + }; + let image = image.filter(|(w, h, bytes)| *w > 0 && *h > 0 && bytes.len() == *w as usize * *h as usize * 4); let Some((w, h, bytes)) = image else { if self.os_clip_sig.take().is_some() { self.session.clipboard = None; self.clip_external = false; } - return false; + return Ok(false); }; let sig = clip_signature(w, h, &bytes); if self.os_clip_sig == Some(sig) && self.session.clipboard.is_some() { - return false; + return Ok(false); } let r = photocraft_geom::Rect::new(0, 0, w as i32, h as i32); let mut surface = photocraft_raster::Surface::from_interleaved(photocraft_color::PixelFormat::RGBA8, r, &bytes); @@ -2478,7 +2491,7 @@ impl PhotocraftApp { self.session.clipboard = Some(photocraft_engine::edit_cmds::Clip { surface, bounds: r, layers: None, resolution: None }); self.os_clip_sig = Some(sig); self.clip_external = true; - true + Ok(true) } } @@ -2611,7 +2624,7 @@ mod clipboard_tests { *a.lock().unwrap() = Some((w, h, px.to_vec())); Ok(()) })), - clipboard_get_image: Some(Box::new(move || b.lock().unwrap().clone())), + clipboard_get_image: Some(Box::new(move || Ok(b.lock().unwrap().clone()))), ..Default::default() }; let mut app = PhotocraftApp::new(Session::new(), services); @@ -2622,10 +2635,10 @@ mod clipboard_tests { let (w, h, px) = os.lock().unwrap().clone().unwrap(); assert_eq!((w, h, px.len()), (8, 4, 8 * 4 * 4)); // Our own image comes back unchanged (no re-import, keeps the original position). - assert!(!app.import_os_clipboard()); + assert!(!app.import_os_clipboard().unwrap()); // Text copied elsewhere replaces our image: nothing to paste, not the stale image. let ours = os.lock().unwrap().take(); - assert!(!app.import_os_clipboard()); + assert!(!app.import_os_clipboard().unwrap()); assert!(app.session.clipboard.is_none()); *os.lock().unwrap() = ours; app.run("edit.copy", serde_json::json!({})).unwrap(); @@ -2659,12 +2672,107 @@ mod clipboard_tests { type OsClip = Arc)>>>; + #[test] + #[ignore = "renders clipboard error and retry evidence with a GPU adapter"] + fn clipboard_error_and_retry_visual_evidence() { + use std::sync::atomic::{AtomicBool, Ordering}; + let busy = Arc::new(AtomicBool::new(true)); + let unavailable = busy.clone(); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).with_max_steps(64).wgpu().build_eframe(move |cc| { + PhotocraftApp::setup_context(&cc.egui_ctx, Default::default()); + let services = Services { + clipboard_get_image: Some(Box::new(move || { + if unavailable.load(Ordering::SeqCst) { + Err("The native clipboard is not accessible due to being held by another party.".into()) + } else { + Ok(None) + } + })), + ..Default::default() + }; + let mut app = PhotocraftApp::new(Session::new(), services); + app.run("file.new", serde_json::json!({"width": 400, "height": 300, "background": "white"})).unwrap(); + app.run("shape.create", serde_json::json!({"kind": "rect", "rect": [80, 60, 200, 120], "fill": "#3070c0"})).unwrap(); + app.run("edit.copy", serde_json::json!({})).unwrap(); + app.run("file.new", serde_json::json!({"width": 400, "height": 300, "background": "white"})).unwrap(); + app + }); + let out = std::path::PathBuf::from(std::env::var_os("PHOTOCRAFT_CLIPBOARD_EVIDENCE").expect("evidence directory")); + std::fs::create_dir_all(&out).unwrap(); + h.run_steps(8); + let ctx = h.ctx.clone(); + assert!(crate::menus::invoke(h.state_mut(), &ctx, "edit.paste", serde_json::json!({})).is_err()); + h.run_steps(2); + assert!(h.state().ui.status_error); + h.render().unwrap().save(out.join("clipboard-busy.png")).unwrap(); + busy.store(false, Ordering::SeqCst); + crate::menus::invoke(h.state_mut(), &ctx, "edit.paste", serde_json::json!({})).unwrap(); + h.run_steps(3); + assert!(!h.state().ui.status_error); + assert!(h.state().ui.status.is_empty(), "retry clears the clipboard error: {}", h.state().ui.status); + // Direct harness mutations need a repaint after the command has cleared the error. + h.ctx.request_repaint(); + h.run_steps(2); + h.render().unwrap().save(out.join("clipboard-retry.png")).unwrap(); + } + + #[test] + fn failed_clipboard_reads_preserve_editable_contents_until_retry() { + use std::sync::atomic::{AtomicBool, Ordering}; + for id in ["edit.paste", "edit.pasteSpecial.pasteInPlace", "file.newFromClipboard"] { + let os = OsClip::default(); + let busy = Arc::new(AtomicBool::new(false)); + let (write, read, unavailable) = (os.clone(), os.clone(), busy.clone()); + let services = Services { + clipboard_set_image: Some(Box::new(move |w, h, bytes| { + *write.lock().unwrap() = Some((w, h, bytes.to_vec())); + Ok(()) + })), + clipboard_get_image: Some(Box::new(move || { + if unavailable.load(Ordering::SeqCst) { Err("clipboard is busy".into()) } else { Ok(read.lock().unwrap().clone()) } + })), + ..Default::default() + }; + let mut app = PhotocraftApp::new(Session::new(), services); + app.run("file.new", serde_json::json!({"width": 80, "height": 60})).unwrap(); + app.run("shape.create", serde_json::json!({"kind": "rect", "rect": [10, 10, 40, 20], "fill": "#ff0000"})).unwrap(); + app.run("edit.copy", serde_json::json!({})).unwrap(); + app.run("file.new", serde_json::json!({"width": 80, "height": 60})).unwrap(); + let signature = app.os_clip_sig; + assert!(signature.is_some()); + let before = app.session.active().unwrap().doc.walk().len(); + busy.store(true, Ordering::SeqCst); + let result = crate::menus::invoke(&mut app, &egui::Context::default(), id, serde_json::json!({})); + assert_eq!(result.unwrap_err(), "clipboard is busy", "{id}"); + assert!(app.ui.status_error); + assert_eq!(app.session.documents().len(), 2); + assert_eq!(app.session.active().unwrap().doc.walk().len(), before); + assert_eq!(app.os_clip_sig, signature); + assert!(app.session.clipboard.as_ref().unwrap().layers.is_some()); + // The New dialog also reads the clipboard; a failed preset probe must preserve it. + app.new_document_fields(); + assert_eq!(app.os_clip_sig, signature); + assert!(app.session.clipboard.as_ref().unwrap().layers.is_some()); + // An automation error must not leave script events disabled by the dispatch guard. + app.automation_input = true; + app.session.edit_prefs(|prefs| prefs.script_events.enabled = true); + assert!(app.run(id, serde_json::json!({})).is_err()); + assert!(app.session.prefs().script_events.enabled); + app.automation_input = false; + busy.store(false, Ordering::SeqCst); + app.run("edit.pasteSpecial.pasteInPlace", serde_json::json!({})).unwrap(); + let state = app.session.active().unwrap(); + assert!(matches!(state.doc.layer(state.active_layer.unwrap()).unwrap().content, photocraft_doc::LayerContent::Shape(_))); + assert!(!app.ui.status_error); + } + } + /// An app whose OS clipboard is `os`, counting every read in `reads`. fn app_with_os_clipboard(os: &OsClip, reads: &Arc) -> PhotocraftApp { let (b, n) = (os.clone(), reads.clone()); let get = move || { n.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - b.lock().unwrap().clone() + Ok(b.lock().unwrap().clone()) }; let mut app = PhotocraftApp::new(Session::new(), Services { clipboard_get_image: Some(Box::new(get)), ..Default::default() }); app.session.execute("file.new", serde_json::json!({"width": 64, "height": 64})).unwrap(); @@ -2700,7 +2808,7 @@ mod clipboard_tests { let (b, n) = (os.clone(), Arc::clone(&reads)); let get = move || { n.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - b.lock().unwrap().clone() + Ok(b.lock().unwrap().clone()) }; let mut app = PhotocraftApp::new(Session::new(), Services { clipboard_get_image: Some(Box::new(get)), ..Default::default() }); let ctx = egui::Context::default(); @@ -2750,7 +2858,7 @@ mod clipboard_tests { PhotocraftApp::setup_context(&cc.egui_ctx, Default::default()); let get = move || { n.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - b.lock().unwrap().clone() + Ok(b.lock().unwrap().clone()) }; let mut app = PhotocraftApp::new(Session::new(), Services { clipboard_get_image: Some(Box::new(get)), ..Default::default() }); app.session.execute("file.new", serde_json::json!({"width": 64, "height": 64})).unwrap(); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 403994ac10..2d4d312420 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -544,7 +544,7 @@ pub(crate) fn invoke_unguarded(app: &mut PhotocraftApp, ctx: &egui::Context, id: "edit.paste" if params.as_object().is_none_or(|o| o.is_empty()) => { // Photoshop: paste in place when the copied area is visible, else centred in the view; // images from other apps are always centred. - app.import_os_clipboard(); + app.import_os_clipboard()?; app.clip_read_for_paste = true; let external = app.clip_external; let visible = !external