From 7e447dfd40c9a159f0a9c0f6d92ece8fbb9f7fd7 Mon Sep 17 00:00:00 2001 From: jonathanwxh-cell Date: Thu, 15 Oct 2026 01:28:42 +0800 Subject: [PATCH] Start Free Transform after placing an image from the file picker (#5390) * test(ui): reproduce missing transform after menu placement * fix(ui): start placement transform after choosing a file * test(ui): exercise native linked placement file paths * test(ui): pass context to existing menu wrapper tests --------- Co-authored-by: jonathnwxh <258340642+jonathanwxh-cell@users.noreply.github.com> --- crates/ui-egui/src/file_dialog/tests.rs | 110 ++++++++++++++++++++++++ crates/ui-egui/src/view_cmds.rs | 15 +++- crates/ui-egui/src/view_cmds/tests.rs | 10 +-- 3 files changed, 127 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/file_dialog/tests.rs b/crates/ui-egui/src/file_dialog/tests.rs index 5f7dfc5c98..3418adb257 100644 --- a/crates/ui-egui/src/file_dialog/tests.rs +++ b/crates/ui-egui/src/file_dialog/tests.rs @@ -161,6 +161,116 @@ fn answer(open: &Open, answer: Option) { std::thread::spawn(move || reply.send(answer)).join().unwrap(); } +#[test] +fn menu_place_starts_transform_for_each_chosen_image() { + for command in ["file.placeEmbedded", "file.placeLinked"] { + let (mut app, open, _) = app(); + app.run("edit.fill", json!({"color": "#ff0000"})).unwrap(); + let bytes = photocraft_io::export(&app.session.active().unwrap().doc, "source.png", &Default::default()).unwrap().bytes; + let ctx = egui::Context::default(); + let initial_layers = app.session.active().unwrap().doc.layer_count(); + let initial_history = app.session.active().unwrap().history.past_len(); + let linked_dir = (command == "file.placeLinked").then(|| { + let unique = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos(); + let dir = std::env::temp_dir().join(format!("photocraft-menu-place-{}-{unique}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + dir + }); + for index in 0..2 { + menus::invoke(&mut app, &ctx, command, json!({})).unwrap(); + app.poll_file_dialog(&ctx, None); + let picked = if let Some(dir) = &linked_dir { + let path = dir.join(format!("drawing-{index}.png")); + std::fs::write(&path, &bytes).unwrap(); + FileDialogAnswer::Paths(vec![path.to_string_lossy().into_owned()]) + } else { + FileDialogAnswer::Contents(format!("drawing-{index}.png"), bytes.clone()) + }; + answer(&open, Some(picked)); + app.poll_file_dialog(&ctx, None); + let st = app.session.active().unwrap(); + assert_eq!(st.doc.layer_count(), initial_layers + index + 1); + assert_eq!(st.doc.layer(st.active_layer.unwrap()).unwrap().name, format!("drawing-{index}")); + let transform = app.ui.transform.as_ref().expect("File > Place must offer transform handles after the picker completes"); + assert_eq!(transform.made, Some(crate::state::MadeLayer::Place)); + if index == 0 { + let layer = st.active_layer.unwrap(); + let photocraft_doc::LayerContent::Smart(smart) = &st.doc.layer(layer).unwrap().content else { panic!("placed Smart Object") }; + let original_scale = smart.transform.m[0]; + let transform = app.ui.transform.as_mut().unwrap(); + let left = transform.quad[0][0]; + for corner in &mut transform.quad { + corner[0] = left + (corner[0] - left) * 2.0; + } + crate::transform_tool::commit(&mut app); + assert!(app.ui.transform.is_none(), "resizing the placed image must commit"); + let st = app.session.active().unwrap(); + let photocraft_doc::LayerContent::Smart(smart) = &st.doc.layer(layer).unwrap().content else { panic!("placed Smart Object") }; + assert!((smart.transform.m[0] - original_scale * 2.0).abs() < 1e-8, "placement width must double"); + assert_eq!(app.session.active().unwrap().history.past_len(), initial_history + 1); + } else { + crate::transform_tool::cancel(&mut app); + assert_eq!(app.session.active().unwrap().doc.layer_count(), initial_layers + 1); + assert_eq!(app.session.active().unwrap().history.past_len(), initial_history + 1); + } + } + if let Some(dir) = linked_dir { + for index in 0..2 { + std::fs::remove_file(dir.join(format!("drawing-{index}.png"))).unwrap(); + } + std::fs::remove_dir(dir).unwrap(); + } + } +} + +#[test] +fn cancelled_or_invalid_menu_place_does_not_start_transform() { + for command in ["file.placeEmbedded", "file.placeLinked"] { + for reply in [None, Some(FileDialogAnswer::Contents("broken.png".into(), b"not an image".to_vec()))] { + let (mut app, open, _) = app(); + let ctx = egui::Context::default(); + let before = app.session.active().unwrap().doc.clone(); + let history = app.session.active().unwrap().history.past_len(); + menus::invoke(&mut app, &ctx, command, json!({})).unwrap(); + app.poll_file_dialog(&ctx, None); + answer(&open, reply); + app.poll_file_dialog(&ctx, None); + assert!(app.ui.transform.is_none()); + assert_eq!(app.session.active().unwrap().doc, before); + assert_eq!(app.session.active().unwrap().history.past_len(), history); + } + } +} + +#[test] +fn menu_place_preserves_an_unfinished_transform() { + for start_before_picker in [true, false] { + let (mut app, open, _) = app(); + app.run("edit.fill", json!({"color": "#ff0000"})).unwrap(); + app.run("layer.new.layerFromBackground", json!({})).unwrap(); + let bytes = photocraft_io::export(&app.session.active().unwrap().doc, "source.png", &Default::default()).unwrap().bytes; + let ctx = egui::Context::default(); + if !start_before_picker { + menus::invoke(&mut app, &ctx, "file.placeEmbedded", json!({})).unwrap(); + app.poll_file_dialog(&ctx, None); + } + crate::transform_tool::begin(&mut app, &ctx).unwrap(); + let transform = app.ui.transform.clone(); + let doc = app.session.active().unwrap().doc.clone(); + let history = app.session.active().unwrap().history.past_len(); + if start_before_picker { + assert!(menus::invoke(&mut app, &ctx, "file.placeEmbedded", json!({})).is_err()); + assert!(!app.file_dialog_open()); + } else { + answer(&open, Some(FileDialogAnswer::Contents("source.png".into(), bytes))); + app.poll_file_dialog(&ctx, None); + } + assert_eq!(app.ui.transform, transform); + assert_eq!(app.session.active().unwrap().doc, doc); + assert_eq!(app.session.active().unwrap().history.past_len(), history); + } +} + #[test] fn open_starts_in_the_last_used_folder() { let (mut app, open, _) = app(); diff --git a/crates/ui-egui/src/view_cmds.rs b/crates/ui-egui/src/view_cmds.rs index 793257be8b..e5f9116ebb 100644 --- a/crates/ui-egui/src/view_cmds.rs +++ b/crates/ui-egui/src/view_cmds.rs @@ -612,7 +612,7 @@ fn no_params(p: &Value) -> bool { /// `id` isn't ours. pub fn invoke(app: &mut PhotocraftApp, ctx: &egui::Context, id: &str, params: &Value) -> Option> { if wraps(id) { - return front(app, id, params); + return front(app, ctx, id, params); } if !handles(id) { return None; @@ -1154,7 +1154,7 @@ fn default_dir(app: &PhotocraftApp) -> String { app.session.active().and_then(|d| d.path.as_deref()).and_then(|p| p.rfind(['/', '\\']).map(|i| p[..i].to_string())).unwrap_or_else(|| ".".into()) } -fn front(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option> { +fn front(app: &mut PhotocraftApp, ctx: &egui::Context, id: &str, params: &Value) -> Option> { if id == "file.closeAll" || id == "file.closeOthers" { // Keep the remaining document's view (views are index-aligned with documents). let keep = app.session.active_index().and_then(|i| app.ui.views.get(i).cloned()); @@ -1192,15 +1192,24 @@ fn front(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option open_exr_parts_dialog(app), "file.saveACopy" => Some(save_a_copy(app)), "file.placeEmbedded" | "file.placeLinked" => { + if app.ui.transform.is_some() { + return Some(Err("Commit or cancel the transform first".into())); + } let doc = match app.active_doc_id() { Ok(doc) => doc, Err(e) => return Some(Err(e)), }; let linked = id == "file.placeLinked"; + let ctx = ctx.clone(); Some(app.pick_file_bytes(move |app, name, bytes| { + if app.ui.transform.is_some() { + return Err("Commit or cancel the transform first".into()); + } app.refocus(doc)?; let linked = linked.then(|| name.clone()); - app.place_bytes(&name, bytes, linked) + let result = app.place_bytes(&name, bytes, linked)?; + crate::transform_tool::begin_placed(app, &ctx)?; + Ok(result) })) } "layer.smartObjects.replaceContents" => { diff --git a/crates/ui-egui/src/view_cmds/tests.rs b/crates/ui-egui/src/view_cmds/tests.rs index 7ef3100d9e..22fa0711d9 100644 --- a/crates/ui-egui/src/view_cmds/tests.rs +++ b/crates/ui-egui/src/view_cmds/tests.rs @@ -327,8 +327,8 @@ fn new_guide_layout_remembers_successful_values_not_cancelled_edits() { #[test] fn new_guide_layout_does_not_remember_a_failed_command() { - let (mut app, _) = app_with(0); - let id = front(&mut app, "view.newGuideLayout", &json!({})).unwrap().unwrap()["dialog"].as_u64().unwrap(); + let (mut app, ctx) = app_with(0); + let id = front(&mut app, &ctx, "view.newGuideLayout", &json!({})).unwrap().unwrap()["dialog"].as_u64().unwrap(); app.ui.dialog_mut(id).unwrap().fields.insert("columns".into(), json!(3)); assert!(crate::dialogs::confirm(&mut app, id).is_err()); assert_eq!(app.ui.view.guide_layout["columns"], 8); @@ -336,10 +336,10 @@ fn new_guide_layout_does_not_remember_a_failed_command() { #[test] fn lookup_export_dialog_uses_the_current_adjustment_selection() { - let (mut app, _ctx) = app_with(1); + let (mut app, ctx) = app_with(1); app.run("layer.newAdjustmentLayer.invert", json!({})).unwrap(); let current = app.session.active().unwrap().active_layer.unwrap(); - let opened = front(&mut app, "file.export.colorLookupTables", &json!({})).unwrap().unwrap(); + let opened = front(&mut app, &ctx, "file.export.colorLookupTables", &json!({})).unwrap().unwrap(); let id = opened["dialog"].as_u64().unwrap(); let dialog = &app.ui.dialogs.last().unwrap().fields; assert_eq!(dialog["scope"], "selected"); @@ -348,7 +348,7 @@ fn lookup_export_dialog_uses_the_current_adjustment_selection() { app.ui.close_dialog(id); app.run("layer.new.layer", json!({"name": "Unrelated pixels"})).unwrap(); - let opened = front(&mut app, "file.export.colorLookupTables", &json!({})).unwrap().unwrap(); + let opened = front(&mut app, &ctx, "file.export.colorLookupTables", &json!({})).unwrap().unwrap(); let id = opened["dialog"].as_u64().unwrap(); assert_eq!(app.ui.dialogs.last().unwrap().fields["scope"], "all"); app.ui.close_dialog(id);