collective
Browse / github.com/storytold/photocraft / patch
fix(ui): preserve transform state and prevent ghosting when switching documents
bug: Layers/content disappear from active document when creating a new file ### Description When a user creates a new document (e.g., A4), inserts an image/layer into canvas, and then opens/creates a second new document, switching back to the previous document tab causes the inserted content/layers to completely disappear from the canvas (or rendering resets). ### Steps to Reproduce * Open PhotoCraft and create a new document (e.g., A4 preset). * Import/drag-and-drop or paste an image onto the canvas. * Go to Arquivo > Novo... (File > New) and create a second document. * Switch back to the first document tab (Untitled-1 / Sem Título-1). * Notice that the i
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- alexandrubunea (imported from pull request #9759, open)
- Base
938542c78b5cdd6600ef70c3eb72a536a5d6c797- Size
- +143 −6 in 6 files
- Review
- pending
- Shared
- 2026-10-28
- Id
be5e2ccff438075859bcdfed
How it went
No other operator has reported using it yet.
Diff
raw · crates/ui-egui/src/canvas.rs crates/ui-egui/src/dialogs.rs crates/ui-egui/src/file_open.rs crates/ui-egui/src/lib.rs crates/ui-egui/src/menus.rs crates/ui-egui/src/transform_tool.rs
@@ -2655,7 +2655,8 @@ fn documents(app: &mut PhotocraftApp, ui: &mut egui::Ui) { if d != idx { app.ui.views[d] = v; if ui.input(|i| i.pointer.primary_pressed() && i.pointer.interact_pos().is_some_and(|p| cell.contains(p))) {- app.session.set_active(d);+ // Commit any transform tool change before switching documents (#9735)+ app.activate_document(d); } } let stroke = if d == idx { Stroke::new(1.5, t.accent) } else { Stroke::new(1.0, Color32::from_black_alpha(160)) };@@ -2982,8 +2983,11 @@ fn open_tab_clicks( cancel_open: Option<photocraft_engine::jobs::JobId>, ) { if let Some(i) = activate {- app.session.set_active(i);- app.jobs.focus = None;+ // Commit any transform tool change before switching documents (#9735)+ app.activate_document(i);+ if app.session.active_index() == Some(i) {+ app.jobs.focus = None;+ } } if let Some(job) = focus_open { app.jobs.focus = Some(job);@@ -6909,6 +6913,44 @@ mod tabs_tests; #[cfg(test)] mod tests {+ /// #9735: Switching documents must commit any open transform session to prevent state loss.+ #[test]+ fn transform_commits_on_tab_click() {+ use egui_kittest::kittest::Queryable;+ use serde_json::json;++ let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());+ for name in ["first", "second"] {+ app.run("file.new", json!({"width": 64, "height": 64, "name": name})).unwrap();+ }+ app.sync_views();+ app.activate_document(0);++ app.run("select.all", json!({})).unwrap();+ let ctx = egui::Context::default();+ crate::menus::invoke(&mut app, &ctx, "select.transformSelection", json!({})).unwrap();+ assert!(app.ui.transform.is_some(), "transform must be active");++ let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(700.0, 40.0)).build_ui_state(+ |ui, (app, ready): &mut (PhotocraftApp, bool)| {+ if *ready {+ tabs(app, ui);+ }+ },+ (app, false),+ );++ PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Studio);+ h.state_mut().1 = true;+ h.run_steps(2);++ h.get_by_label_contains("second").click_button(egui::PointerButton::Primary);+ h.run_steps(2);++ assert!(h.state().0.ui.transform.is_none(), "transform must clear on tab click");+ assert_eq!(h.state().0.session.active_index(), Some(1), "active document must change");+ }+ /// #5472: a ⇧ square pressed between pixels snapped to 101 × 102; the short side grows to /// match, on the side the drag points to. #[test]@@ -826,6 +826,16 @@ pub fn confirm(app: &mut PhotocraftApp, id: u64) -> Result<Value, String> { let d = app.ui.close_dialog(id).ok_or_else(|| format!("no dialog {id}"))?; match d.kind { DialogKind::NewDocument => {+ // #9735: Before creating a document, we commit the current transform, if there is any+ if app.ui.transform.is_some() {+ crate::transform_tool::commit(app);++ // Do not proceed if the transform is not commited+ if app.ui.transform.is_some() {+ return Err("Cannot create file while transform is active".into());+ }+ }+ let r = app.run("file.new", crate::new_doc_ui::command_params(&d.fields)); if let Some(i) = app.session.active_index() { app.ui.views[i].fit_pending = true;@@ -137,10 +137,22 @@ impl PhotocraftApp { // Documents opening in the background are read on the worker too (a 2 GB PSB read // would block the window). Preset files (brushes, gradients) go the usual way. let ext = std::path::Path::new(path).extension().map(|e| e.to_string_lossy().to_ascii_lowercase()).unwrap_or_default();++ // #9735: Before opening a document, we commit the current transform, if there is any+ if !crate::preset_files_ui::PRESET_EXTS.contains(&ext.as_str()) && self.ui.transform.is_some() {+ crate::transform_tool::commit(self);++ // If the transform commit fails, do not change the document+ if self.ui.transform.is_some() {+ return Err("Cannot open file while transform is active".into());+ }+ }+ if self.background_jobs && !crate::preset_files_ui::PRESET_EXTS.contains(&ext.as_str()) { crate::jobs_ui::start_open(self, &display_name(path), Some(path.to_string()), photocraft_engine::jobs::OpenSource::Path(path.to_string()))?; return Ok(Vec::new()); }+ let bytes = photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| format!("{path}: {e}"))?; self.open_file(path, &bytes) }@@ -190,7 +202,11 @@ impl PhotocraftApp { let canon = canonical(path); let same = |p: &str| same_file(p, path, canon.as_deref()); if let Some(i) = self.session.documents().iter().position(|d| d.path.as_deref().is_some_and(same)) {- self.session.set_active(i);+ self.activate_document(i); // Commit any transform tool change before switching documents (#9735)+ // If the activate_document fails, then do not proceed.+ if self.session.active_index() != Some(i) {+ return false;+ } self.jobs.focus = None; } else if let Some(o) = self.jobs.opens.iter().find(|o| o.path.as_deref().is_some_and(|p| !is_template(p) && same(p))) { // A template still loading opens as another Untitled, as it does once loaded.@@ -1620,6 +1620,15 @@ impl PhotocraftApp { if let Some(r) = preset_files_ui::open(self, name, bytes) { return r.map(|()| Vec::new()); }++ // #9735: Before opening a document from bytes, commit the current transform if any+ if self.ui.transform.is_some() {+ crate::transform_tool::commit(self);+ if self.ui.transform.is_some() {+ return Err("Cannot open file while transform is active".into());+ }+ }+ let name = &self.open_name(name); // Decoded on a worker: a tab with progress appears now, the document when it's ready // (warnings are shown then).@@ -2894,6 +2903,23 @@ impl PhotocraftApp { } } +impl PhotocraftApp {+ /// This is a helper function created in order to fix #9735.+ /// Safely switches the active document index, automatically committing any+ /// active Free Transform or Warp session if the target document differs.+ pub(crate) fn activate_document(&mut self, index: usize) {+ if index < self.session.documents().len() && self.session.active_index() != Some(index) && self.ui.transform.is_some() {+ crate::transform_tool::commit(self);++ // If the transform commit fails, do not change the document+ if self.ui.transform.is_some() {+ return;+ }+ }+ self.session.set_active(index);+ }+}+ impl PhotocraftApp { /// Cached `Surface::content_bounds` keyed by an id and the surface's tile identity. pub fn cached_bounds(&mut self, key: u64, surface: &photocraft_raster::Surface) -> photocraft_geom::Rect {@@ -201,6 +201,7 @@ pub(crate) fn invoke_unguarded(app: &mut PhotocraftApp, ctx: &egui::Context, id: apply_workspace(app); return Ok(json!({"workspace": app.ui.workspace})); }+ match id { // ⌘↩ / Ctrl+Enter (#306): load the path selected in the Paths panel (or the one being // drawn) as a selection.@@ -634,14 +635,22 @@ pub(crate) fn invoke_unguarded(app: &mut PhotocraftApp, ctx: &egui::Context, id: let n = app.session.documents().len(); let at = app.session.active_index().ok_or("no document open")?; let to = if id == "window.nextDocument" { (at + 1) % n } else { (at + n - 1) % n };- app.session.set_active(to);+ app.activate_document(to); // #9735: We need to commit the transform changes before changing the document+ // If the activate_document fails, then do not proceed.+ if app.session.active_index() != Some(to) {+ return Err("could not activate document".into());+ } app.jobs.focus = None; Ok(json!({"document": to})) } id if let Some(index) = id.strip_prefix(DOCUMENT_ITEM) => { let n = app.session.documents().len(); let to = index.parse::<usize>().ok().filter(|i| *i < n).ok_or("no such document")?;- app.session.set_active(to);+ app.activate_document(to); // #9735: We need to commit the transform changes before changing the document+ // If the activate_document fails, then do not proceed.+ if app.session.active_index() != Some(to) {+ return Err("could not activate document".into());+ } app.jobs.focus = None; Ok(json!({"document": to})) }@@ -2367,6 +2376,26 @@ mod window_documents_tests { window[at..].iter().map(|i| (i.id.clone(), i.label.clone(), i.checked)).collect() } + /// #9735: Switching documents must commit any open transform session to prevent state loss.+ #[test]+ fn transform_commits_on_window_next_document() {+ use serde_json::json;+ let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());+ app.run("file.new", json!({"width": 64, "height": 64})).unwrap();+ app.run("file.new", json!({"width": 64, "height": 64})).unwrap();+ app.session.set_active(0);++ app.run("select.all", json!({})).unwrap();+ let ctx = egui::Context::default();+ crate::menus::invoke(&mut app, &ctx, "select.transformSelection", json!({})).unwrap();+ assert!(app.ui.transform.is_some(), "transform must be active on the first document");++ crate::menus::invoke(&mut app, &ctx, "window.nextDocument", json!({})).unwrap();++ assert!(app.ui.transform.is_none(), "transform must clear when switching documents via command");+ assert_eq!(app.session.active_index(), Some(1), "focus must move to the second document");+ }+ /// #2991: Window ends with the open documents in tab order, the active one checked, as in /// Photoshop; choosing one brings it to the front. #[test]@@ -2212,6 +2212,13 @@ fn front_vertices(h: &Homography, n: usize) -> Vec<bool> { /// Photoshop. The box, handles and reference point are [`draw_overlay`], drawn over the guides. pub fn draw_content(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform) { let (Some(t), Some(pv)) = (&app.ui.transform, &app.transform_preview) else { return };++ // Fix for ghosting after #9735: The async background commit leaves the transform preview populated.+ // We must ensure the preview is only drawn if its document matches the currently active document.+ if app.session.active().map(|st| st.doc.id) != Some(pv.doc.id) {+ return;+ }+ let output = crate::monitor_status::view_display(app, painter.ctx()); let (display, _) = crate::canvas::canvas_display(app, &pv.doc, output); // Selection tint and lone mask/channel values are UI diagnostics, not document colors.@@ -2272,6 +2279,13 @@ pub fn draw_content(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform /// drew; the warp grid and its split guides for a warp. pub fn draw_overlay(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform) { let (Some(t), Some(pv)) = (&app.ui.transform, &app.transform_preview) else { return };++ // Fix for ghosting after #9735: The async background commit leaves the transform preview populated.+ // We must ensure the preview is only drawn if its document matches the currently active document.+ if app.session.active().map(|st| st.doc.id) != Some(pv.doc.id) {+ return;+ }+ let scr = |q: [f64; 2]| xf.to_screen(q[0] as f32, q[1] as f32); if let Some(w) = &t.warp { let over_canvas = painter.ctx().input(|i| i.pointer.hover_pos()).filter(|p| xf.rect.contains(*p)).map(|p| xf.to_doc(p));