From 5c1aea75330540a99ffa37c4c97a26461420770d Mon Sep 17 00:00:00 2001 From: KnottyDyes <47539659+KnottyDyes@users.noreply.github.com> Date: Sat, 10 Oct 2026 09:17:30 -0600 Subject: [PATCH] Place: an explicit x, y, spread or width puts the image in a new frame Explicit image placement leaves selected frames untouched; an explicit frame still wins. Null placement parameters behave like omitted values. Reject nonnumeric, nonfinite and nonpositive image widths before editing. Cover rejection without document changes, valid sizing and null selection behavior with regression tests. --- crates/engine/src/cmd/file.rs | 291 +++++++++++++++++++++++++++++++++- 1 file changed, 288 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index f63f0d6e9..f18f1d7b9 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -103,7 +103,7 @@ pub fn specs() -> Vec { "Place…", ["File"], Some("Cmd+D"), - "{path?|base64?, name?, frame?: id (place into), spread?, x?, y?, width?, pdfPage?: n (1-based, Image Import Options), pdfCrop?: crop|trim|bleed|art|media, loadCursor?: with no frame to fill and no x/y, an image loads into the place cursor (File › Place), layoutPage?: n (IDML / .designcraft: that page's objects as a group)} — places an image (into the selected empty frame if any); text files (.txt, .docx, .rtf, .md) and Excel workbooks (.xlsx, as a table) go into the insertion point, the selected frame or a new frame on `page`/`rect` — {autoflow?: adds pages with threaded frames until the text fits, removeStyles?, styleMap?: {imported name: document style}, styleConflicts?: useExisting|redefine|autoRename, link?: keep the file linked (Links panel; default: the linkTextFiles preference)}", + "{path?|base64?, name?, frame?: id (place into), spread?, x?, y?, width?: finite number > 0 (null: absent), pdfPage?: n (1-based, Image Import Options), pdfCrop?: crop|trim|bleed|art|media, loadCursor?: with no frame to fill and no x/y, an image loads into the place cursor (File › Place), layoutPage?: n (IDML / .designcraft: that page's objects as a group)} — places an image into `frame` when given; otherwise any explicit x/y/spread/width creates a new frame; otherwise uses the selected empty or graphic frame, or creates a new frame; text files (.txt, .docx, .rtf, .md) and Excel workbooks (.xlsx, as a table) go into the insertion point, the selected frame or a new frame on `page`/`rect` — {autoflow?: adds pages with threaded frames until the text fits, removeStyles?, styleMap?: {imported name: document style}, styleConflicts?: useExisting|redefine|autoRename, link?: keep the file linked (Links panel; default: the linkTextFiles preference)}", has_doc, file_place ), @@ -327,6 +327,16 @@ fn file_place(s: &mut Session, p: &Value) -> Result { if designcraft_textimport::is_text_file(&name) { return super::place_text::place_text(s, p, &name, &bytes, link, before); } + let want_w = p + .get("width") + .filter(|value| !value.is_null()) + .map(|value| { + value + .as_f64() + .filter(|width| width.is_finite() && *width > 0.0) + .ok_or_else(|| bad("file.place", "`width` must be a finite number greater than zero")) + }) + .transpose()?; // Image Import Options: which page of a PDF (1-based). let pdf_page = match p.get("pdfPage").and_then(Value::as_u64) { Some(n) if designcraft_render::is_pdf(&bytes) => { @@ -362,14 +372,20 @@ fn file_place(s: &mut Session, p: &Value) -> Result { None => None, }; let target_frame = super::id_param(p, "frame").or_else(|| { + if ["x", "y", "spread", "width"].iter().any(|key| p.get(*key).is_some_and(|value| !value.is_null())) { + return None; + } let st = s.active()?; st.selection.items.iter().copied().find(|i| st.doc.item(*i).is_some_and(|it| matches!(it.content, Content::Unassigned | Content::Graphic(_)))) }); - let spread = super::spread_param(p, "file.place")?; + // `"spread": null` counts as omitted, like the other placement parameters. + let spread = match p.get("spread") { + Some(Value::Null) => designcraft_doc::SpreadRef::Doc(0), + _ => super::spread_param(p, "file.place")?, + }; let lid = s.doc()?.active_layer; let px = p.get("x").and_then(Value::as_f64); let py = p.get("y").and_then(Value::as_f64); - let want_w = p.get("width").and_then(Value::as_f64); if target_frame.is_none() && px.is_none() && py.is_none() && p.get("loadCursor").and_then(Value::as_bool) == Some(true) { let mime = designcraft_render::image_mime(&bytes).to_string(); let asset = Asset { page: pdf_page, id: AssetId(0), name, mime, link, data: Arc::new(bytes), pixels: Some((pw, ph)) }; @@ -734,6 +750,275 @@ fn file_revert(s: &mut Session, _: &Value) -> Result { } } +#[cfg(all(test, not(target_arch = "wasm32")))] +mod image_place_tests { + use designcraft_doc::{Content, ItemId}; + use designcraft_geom::Rect; + use serde_json::{Value, json}; + + use crate::Session; + + fn image_file() -> std::io::Result { + image_file_with_color(30) + } + + fn image_file_with_color(red: u8) -> std::io::Result { + let file = tempfile::Builder::new().suffix(".png").tempfile()?; + let png = designcraft_render::Rendered { width: 40, height: 20, pixels: [red, 90, 180, 255].repeat(800) }.to_png(); + std::fs::write(file.path(), png)?; + Ok(file) + } + + #[test] + fn consecutive_path_placements_create_distinct_graphics() { + let images = [image_file_with_color(30).unwrap(), image_file_with_color(31).unwrap(), image_file_with_color(32).unwrap()]; + let mut session = Session::new(); + session.execute("file.new", &json!({"pages": 3, "facingPages": false})).unwrap(); + let mut placements = Vec::new(); + let mut originals = Vec::new(); + + for ((spread, x, y), image) in [(0, 0.0, 0.0), (1, 70.0, 90.0), (2, 150.0, 210.0)].into_iter().zip(&images) { + let placed = session.execute("file.place", &json!({"path": image.path(), "spread": spread, "x": x, "y": y})).unwrap(); + let id = ItemId(placed["id"].as_u64().unwrap()); + assert!( + placements.iter().all(|previous: &Value| previous["id"] != placed["id"]), + "each explicit placement needs a distinct frame: {placed}" + ); + let document = &session.doc().unwrap().doc; + let item = document.item(id).unwrap(); + assert_eq!(item.bounds(), Rect::new(x, y, x + 40.0, y + 20.0)); + assert!(document.spreads[spread].items.iter().any(|item| item.id == id)); + originals.push(serde_json::to_value(item).unwrap()); + placements.push(placed); + } + + let document = &session.doc().unwrap().doc; + assert_eq!(document.spreads.iter().flat_map(|spread| &spread.items).filter(|item| matches!(item.content, Content::Graphic(_))).count(), 3); + assert_eq!(document.assets.len(), 3); + for (placed, original) in placements.iter().zip(originals) { + assert_eq!(serde_json::to_value(document.item(ItemId(placed["id"].as_u64().unwrap())).unwrap()).unwrap(), original); + } + let links = session.execute("links.list", &json!({})).unwrap(); + assert_eq!(links.as_array().unwrap().len(), 3); + for (placed, image) in placements.into_iter().zip(&images) { + let row = links.as_array().unwrap().iter().find(|row| row["asset"] == placed["asset"]).unwrap(); + assert_eq!(row["path"], json!(image.path())); + assert_eq!(row["uses"].as_array().unwrap().len(), 1); + assert_eq!(row["uses"][0]["id"], placed["id"]); + } + } + + #[test] + fn individual_placement_parameters_bypass_selected_frames() { + let image = image_file().unwrap(); + for graphic in [false, true] { + for (key, value, expected) in [ + ("x", 0, Rect::new(0.0, 36.0, 40.0, 56.0)), + ("y", 0, Rect::new(36.0, 0.0, 76.0, 20.0)), + ("spread", 0, Rect::new(36.0, 36.0, 76.0, 56.0)), + ("width", 0, Rect::ZERO), + ("width", 80, Rect::new(36.0, 36.0, 116.0, 76.0)), + ] { + let mut session = Session::new(); + session.execute("file.new", &json!({})).unwrap(); + let selected = session.execute("frame.create", &json!({"rect": [100, 120, 200, 220]})).unwrap(); + if graphic { + session.execute("file.place", &json!({"path": image.path()})).unwrap(); + } + let id = ItemId(selected["id"].as_u64().unwrap()); + let original = serde_json::to_value(session.doc().unwrap().doc.item(id).unwrap()).unwrap(); + let mut params = json!({"path": image.path()}); + params[key] = json!(value); + + let result = session.execute("file.place", ¶ms); + if key == "width" && value == 0 { + assert!(result.is_err(), "zero width must be rejected, graphic={graphic}"); + continue; + } + let placed = result.unwrap(); + + assert_ne!(placed["id"], selected["id"], "{key}={value}, graphic={graphic}"); + let document = &session.doc().unwrap().doc; + assert_eq!(serde_json::to_value(document.item(id).unwrap()).unwrap(), original); + let item = document.item(ItemId(placed["id"].as_u64().unwrap())).unwrap(); + assert_eq!(item.bounds(), expected); + let Content::Graphic(content) = &item.content else { panic!("placed frame must be graphic") }; + let [scale_x, _, _, scale_y, _, _] = content.xf.as_coeffs(); + assert!(scale_x > 0.0 && scale_y > 0.0); + } + } + } + + #[test] + fn explicit_frame_wins_over_placement_and_selection() { + let image = image_file().unwrap(); + let mut session = Session::new(); + session.execute("file.new", &json!({})).unwrap(); + let target = session.execute("frame.create", &json!({"rect": [100, 120, 200, 220]})).unwrap(); + let selected = session.execute("frame.create", &json!({"rect": [300, 320, 400, 420]})).unwrap(); + + let placed = + session.execute("file.place", &json!({"path": image.path(), "frame": target["id"], "x": 0, "y": 0, "spread": 99, "width": 80})).unwrap(); + + assert_eq!(placed["id"], target["id"]); + let document = &session.doc().unwrap().doc; + let item = document.item(ItemId(target["id"].as_u64().unwrap())).unwrap(); + assert_eq!(item.bounds(), Rect::new(100.0, 120.0, 200.0, 220.0)); + assert!(matches!(item.content, Content::Graphic(_))); + assert!(matches!(document.item(ItemId(selected["id"].as_u64().unwrap())).unwrap().content, Content::Unassigned)); + assert_eq!(document.spreads[0].items.len(), 2); + } + + fn assert_width_rejected(width: Value) -> std::result::Result<(), Box> { + let image = image_file()?; + for graphic in [false, true] { + for explicit_frame in [false, true] { + let mut session = Session::new(); + session.execute("file.new", &json!({}))?; + let target = session.execute("frame.create", &json!({"rect": [100, 120, 200, 220]}))?; + if graphic { + session.execute("file.place", &json!({"path": image.path()}))?; + } + let mut params = json!({"path": image.path(), "width": width}); + if explicit_frame { + params["frame"] = target["id"].clone(); + session.execute("frame.create", &json!({"rect": [300, 320, 400, 420]}))?; + } + let before = session.doc()?; + let document = before.doc.clone(); + let selection = before.selection.clone(); + let revision = before.revision; + let assets = document.assets.len(); + + let result = session.execute("file.place", ¶ms); + + assert!( + matches!(&result, Err(crate::EngineError::BadParams { .. })), + "width={width}, graphic={graphic}, explicit_frame={explicit_frame}: expected BadParams, got {result:?}" + ); + let error = result.err().ok_or_else(|| crate::EngineError::Other("expected placement error".into()))?; + let crate::EngineError::BadParams { cmd, msg } = error else { return Err(error.into()) }; + assert_eq!(cmd, "file.place"); + assert!(msg.contains("width") && msg.contains("finite") && msg.contains("greater than zero"), "{msg}"); + let after = session.doc()?; + assert!(std::sync::Arc::ptr_eq(&document, &after.doc)); + assert_eq!(after.selection, selection); + assert_eq!(after.revision, revision); + assert_eq!(after.doc.assets.len(), assets); + } + } + Ok(()) + } + + #[test] + fn width_rejected_when_zero() { + assert_width_rejected(json!(0)).unwrap(); + } + + #[test] + fn width_rejected_when_negative() { + assert_width_rejected(json!(-80)).unwrap(); + } + + #[test] + fn width_rejected_when_non_numeric() { + for width in [json!("80"), json!(true), json!([80]), json!({"value": 80})] { + assert_width_rejected(width).unwrap(); + } + } + + #[test] + fn numeric_overflow_rejected_when_parsing_json() { + let result = serde_json::from_str::(r#"{"width":1e400}"#); + let error = result.unwrap_err(); + assert!(error.is_data() || error.is_syntax(), "{error}"); + eprintln!("overflow parser rejected width=1e400: {error}"); + } + + fn assert_null_placement_uses_selection(keys: &[&str]) -> std::result::Result<(), Box> { + let image = image_file()?; + let previous_image = image_file_with_color(31)?; + for graphic in [false, true] { + let mut session = Session::new(); + session.execute("file.new", &json!({}))?; + let selected = session.execute("frame.create", &json!({"rect": [100, 120, 200, 220]}))?; + let previous = graphic.then(|| session.execute("file.place", &json!({"path": previous_image.path()}))).transpose()?; + let mut params = json!({"path": image.path()}); + for key in keys { + params[*key] = Value::Null; + } + + let placed = session.execute("file.place", ¶ms)?; + + assert_eq!(placed["id"], selected["id"], "keys={keys:?}, graphic={graphic}"); + if let Some(previous) = previous { + assert_ne!(placed["asset"], previous["asset"]); + } + let document = &session.doc()?.doc; + let id = ItemId(selected["id"].as_u64().ok_or_else(|| crate::EngineError::Other("frame.create returned no numeric id".into()))?); + let item = document.item(id).ok_or(designcraft_doc::DocError::NoItem(id))?; + assert_eq!(item.bounds(), Rect::new(100.0, 120.0, 200.0, 220.0)); + assert!(matches!(&item.content, Content::Graphic(_)), "placed frame must be graphic"); + let Content::Graphic(content) = &item.content else { + return Err(crate::EngineError::Other("placed frame must be graphic".into()).into()); + }; + assert_eq!(json!(content.asset.0), placed["asset"]); + assert_eq!(document.spreads[0].items.len(), 1); + } + Ok(()) + } + + #[test] + fn selection_used_when_x_null() { + assert_null_placement_uses_selection(&["x"]).unwrap(); + } + + #[test] + fn selection_used_when_y_null() { + assert_null_placement_uses_selection(&["y"]).unwrap(); + } + + #[test] + fn selection_used_when_spread_null() { + assert_null_placement_uses_selection(&["spread"]).unwrap(); + } + + #[test] + fn selection_used_when_width_null() { + assert_null_placement_uses_selection(&["width"]).unwrap(); + } + + #[test] + fn selection_used_when_all_placement_parameters_null() { + assert_null_placement_uses_selection(&["x", "y", "spread", "width"]).unwrap(); + } + + #[test] + fn implicit_placement_fills_empty_and_replaces_graphic_selection() { + let image = image_file().unwrap(); + let previous_image = image_file_with_color(31).unwrap(); + for graphic in [false, true] { + let mut session = Session::new(); + session.execute("file.new", &json!({})).unwrap(); + let selected = session.execute("frame.create", &json!({"rect": [100, 120, 200, 220]})).unwrap(); + let previous = graphic.then(|| session.execute("file.place", &json!({"path": previous_image.path()})).unwrap()); + + let placed = session.execute("file.place", &json!({"path": image.path()})).unwrap(); + + assert_eq!(placed["id"], selected["id"]); + if let Some(previous) = previous { + assert_ne!(placed["asset"], previous["asset"]); + } + let document = &session.doc().unwrap().doc; + let item = document.item(ItemId(placed["id"].as_u64().unwrap())).unwrap(); + assert_eq!(item.bounds(), Rect::new(100.0, 120.0, 200.0, 220.0)); + let Content::Graphic(content) = &item.content else { panic!("placed frame must be graphic") }; + assert_eq!(json!(content.asset.0), placed["asset"]); + assert_eq!(document.spreads[0].items.len(), 1); + } + } +} + #[cfg(test)] mod pdf_crop_tests { use serde_json::json;