From 7bc50901e499620e80b457224dc8c3109a6bc398 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Sun, 4 Oct 2026 11:49:45 +0700 Subject: [PATCH] =?UTF-8?q?M4.13:=20review=20fixes=20=E2=80=94=20template?= =?UTF-8?q?=20sublayers=20are=20left=20out=20of=20raster=20exports=20too?= =?UTF-8?q?=20(the=20check=20moves=20into=20draw=5Farc),=20Export=20Select?= =?UTF-8?q?ion=20skips=20art=20under=20a=20template=20layer=20at=20any=20d?= =?UTF-8?q?epth,=20Export=20for=20Screens=20drops=20@Nx=20suffixes=20on=20?= =?UTF-8?q?SVG/PDF=20rows,=20names=20files=20without=20case=20clashes=20an?= =?UTF-8?q?d=20calls=20an=20unnamed=20artboard=20Artboard-N,=20oversized?= =?UTF-8?q?=20raster=20exports=20are=20refused=20instead=20of=20writing=20?= =?UTF-8?q?empty=20files,=20a=20named=20MCP=20artboard/range=20replaces=20?= =?UTF-8?q?the=20option=20picks,=20and=20control=20app.export=20without=20?= =?UTF-8?q?a=20path=20returns=20dataBase64=20instead=20of=20opening=20a=20?= =?UTF-8?q?save=20dialog?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/fileio/encode.rs | 26 ++++++++++++++ crates/engine/src/cmd/fileio/export.rs | 34 +++++++++++++----- crates/engine/src/cmd/fileio/mod.rs | 4 +-- crates/engine/src/cmd/fileio/tests.rs | 50 ++++++++++++++++++++++++++ crates/mcp/src/tests_fileio.rs | 2 +- crates/mcp/src/tools.rs | 10 ++++-- crates/render/src/lib.rs | 6 ++-- crates/ui-egui/src/control.rs | 8 +++-- crates/ui-egui/src/io.rs | 12 +++++++ docs/control-protocol.md | 2 +- 10 files changed, 134 insertions(+), 20 deletions(-) diff --git a/crates/engine/src/cmd/fileio/encode.rs b/crates/engine/src/cmd/fileio/encode.rs index ff4f2a595..a2ee69471 100644 --- a/crates/engine/src/cmd/fileio/encode.rs +++ b/crates/engine/src/cmd/fileio/encode.rs @@ -12,6 +12,9 @@ use crate::EngineError; const C: &str = "document.export"; +/// The params that pick artboards (the fields of [`ArtboardPick`]). +pub const ARTBOARD_PARAMS: [&str; 3] = ["artboard", "artboards", "range"]; + /// Which artboards an export covers: `range` (`"1-3, 5"`, 1-based) wins over `artboards` /// (0-based), which wins over `artboard`. #[derive(Debug, Default, Deserialize)] @@ -80,6 +83,28 @@ fn options(f: &Format, p: &Value) -> Result { T::deserialize(p).map_err(|e| bad(C, format!("{} options: {e}", f.label))) } +/// Most pixels a raster export renders (1 GiB of RGBA). +const MAX_PIXELS: f64 = 268_435_456.0; + +/// Refuse a raster export the encoder can't write or memory can't hold (instead of writing an +/// empty file or aborting). +fn check_pixels(f: &Format, w: f64, h: f64) -> Result<()> { + // WebP stores sizes in 14 bits, JPEG in 16. + let side = if f.id == "webp" { 16383.0 } else { 65535.0 }; + if w.round() > side || h.round() > side || w * h > MAX_PIXELS { + return Err(bad( + C, + format!( + "{}×{} px is too large for {} (at most {side} px a side, {MAX_PIXELS} px in all): lower the scale", + w.round(), + h.round(), + f.label + ), + )); + } + Ok(()) +} + /// Encode `doc` as `format` (an id or extension from [`super::FORMATS`]) with that format's /// options from `p` (see `document.formats`). Raster formats leave template layers out. pub fn encode(doc: &Document, format: &str, p: &Value) -> Result> { @@ -102,6 +127,7 @@ pub fn encode(doc: &Document, format: &str, p: &Value) -> Result> { let o: RasterOptions = options(f, p)?; let region = doc.artboards[boards(o.boards.one(n))?].rect; let scale = o.scale.unwrap_or(1.0).clamp(0.01, 64.0); + check_pixels(f, region.width() * scale, region.height() * scale)?; let img = vectorcraft_render::Renderer::new().render_region(doc, region, scale, f.id == "jpg"); match f.id { "png" => img.to_png(), diff --git a/crates/engine/src/cmd/fileio/export.rs b/crates/engine/src/cmd/fileio/export.rs index 6c71c71ac..566dd4cf1 100644 --- a/crates/engine/src/cmd/fileio/export.rs +++ b/crates/engine/src/cmd/fileio/export.rs @@ -8,7 +8,7 @@ use serde_json::{Value, json}; use vectorcraft_doc::{Node, NodeKind}; use super::super::*; -use super::{ArtboardPick, Format, create_dir, encode, writable, write_file, write_or_return}; +use super::{ARTBOARD_PARAMS, ArtboardPick, Format, create_dir, encode, writable, write_file, write_or_return}; pub(super) fn serialize(s: &mut Session, p: &Value) -> Result { let f = writable("document.serialize", Some(str_param(p, "format").unwrap_or("vectorcraft")), None)?; @@ -30,7 +30,7 @@ pub(super) fn export(s: &mut Session, p: &Value) -> Result { fn without_artboards(p: &Value) -> Value { let mut q = p.clone(); if let Some(o) = q.as_object_mut() { - for k in ["artboard", "artboards", "range"] { + for k in ARTBOARD_PARAMS { o.remove(k); } } @@ -45,7 +45,9 @@ pub(super) fn export_selection(s: &mut Session, p: &Value) -> Result { let f = writable(C, str_param(p, "format"), path)?; let ids = edit::selected_roots(s)?; let st = s.doc()?; - let on_template = |id| st.doc.layer_of(id).and_then(|l| st.doc.node(l)).is_some_and(|l| matches!(l.kind, NodeKind::Layer { template: true, .. })); + let is_template = |id| st.doc.node(id).is_some_and(|l| matches!(l.kind, NodeKind::Layer { template: true, .. })); + // On a template layer or sublayer, at any depth. + let on_template = |id| st.doc.ancestry(id).is_some_and(|a| a.into_iter().any(is_template)); let nodes: Vec> = ids.into_iter().filter(|id| !on_template(*id)).filter_map(|id| st.doc.node(id).cloned().map(Arc::new)).collect(); let bounds = nodes.iter().filter_map(|n| n.visual_bounds()).reduce(|a, b| a.union(b)).ok_or_else(|| bad(C, "select something to export"))?; let mut d = (*st.doc).clone(); @@ -79,10 +81,15 @@ fn screen_format(row: &Value) -> Result { if format.id == "vectorcraft" { return Err(bad(C, "Export for Screens writes png, jpg, webp, svg or pdf")); } - // Vector formats have no pixel size: scale doesn't apply and adds no @Nx suffix. + // Vector formats have no pixel size: scale doesn't apply and adds no @Nx suffix (not even one + // left over from a raster row switched to SVG or PDF). let scale = if format.raster { f64_or(row, "scale", 1.0) } else { 1.0 }; - let suffix = - str_param(row, "suffix").map(str::to_string).unwrap_or_else(|| if (scale - 1.0).abs() < 1e-9 { String::new() } else { format!("@{scale}x") }); + let suffix = match str_param(row, "suffix") { + Some(s) if !format.raster && is_scale_suffix(s) => String::new(), + Some(s) => s.to_string(), + None if (scale - 1.0).abs() < 1e-9 => String::new(), + None => format!("@{scale}x"), + }; let mut options = without_artboards(row); if let Some(o) = options.as_object_mut() { o.insert("scale".into(), json!(scale)); @@ -90,6 +97,11 @@ fn screen_format(row: &Value) -> Result { Ok(ScreenFormat { format, options, suffix }) } +/// A pixel-density suffix such as `@2x` or `@0.5x`. +fn is_scale_suffix(s: &str) -> bool { + s.strip_prefix('@').and_then(|s| s.strip_suffix(['x', 'X'])).is_some_and(|n| n.parse::().is_ok()) +} + /// File → Export for Screens: every chosen artboard in every format, one file each (a PDF holds /// its artboard alone). Artboards that share a name get `-2`, `-3`… instead of overwriting. pub(super) fn export_for_screens(s: &mut Session, p: &Value) -> Result { @@ -107,21 +119,25 @@ pub(super) fn export_for_screens(s: &mut Session, p: &Value) -> Result { if let Some(dir) = folder { create_dir(dir)?; } + // Names are compared without case: `Icon` and `icon` are one file on most desktop file systems. let mut names = HashSet::new(); let mut written = HashSet::new(); let mut files = vec![]; for b in boards { - let base: String = doc.artboards[b].name.chars().map(|c| if c.is_alphanumeric() || c == '-' || c == '_' { c } else { '-' }).collect(); + let mut base: String = doc.artboards[b].name.chars().map(|c| if c.is_alphanumeric() || c == '-' || c == '_' { c } else { '-' }).collect(); + if base.is_empty() { + base = format!("Artboard-{}", b + 1); + } let mut name = base.clone(); for i in 2.. { - if names.insert(name.clone()) { + if names.insert(name.to_lowercase()) { break; } name = format!("{base}-{i}"); } for sf in &formats { let file = format!("{prefix}{name}{}.{}", sf.suffix, sf.format.extensions[0]); - if !written.insert(file.clone()) { + if !written.insert(file.to_lowercase()) { continue; // the same file from two identical rows } let mut options = sf.options.clone(); diff --git a/crates/engine/src/cmd/fileio/mod.rs b/crates/engine/src/cmd/fileio/mod.rs index 5aa307fcb..b8505e5cf 100644 --- a/crates/engine/src/cmd/fileio/mod.rs +++ b/crates/engine/src/cmd/fileio/mod.rs @@ -17,7 +17,7 @@ mod save; use serde_json::{Value, json}; -pub use encode::{ArtboardPick, encode}; +pub use encode::{ARTBOARD_PARAMS, ArtboardPick, encode}; pub use load::{Loaded, RasterImage, detect, load, open_bytes, raster_image}; use super::*; @@ -84,7 +84,7 @@ pub fn specs() -> Vec { "Export for Screens", ["File", "Export"], None, - "{folder?, artboards?: [index…] | range?: \"1-3\" (default all), formats?: [{format: png|jpg|webp|svg|pdf, scale?: 1 (raster only), suffix?: \"@2x\" (raster default: @{scale}x when scale ≠ 1)}], prefix?} one file per artboard and format (a PDF holds its artboard alone; artboards with the same name get -2, -3…) → {files: [path…]}; no folder → {files: [{name, dataBase64}]}", + "{folder?, artboards?: [index…] | range?: \"1-3\" (default all), formats?: [{format: png|jpg|webp|svg|pdf, scale?: 1 (raster only), suffix?: \"@2x\" (raster default: @{scale}x when scale ≠ 1; svg/pdf drop @Nx suffixes)}], prefix?} one file per artboard and format (a PDF holds its artboard alone; artboards with the same name, in any case, get -2, -3…; an unnamed one is Artboard-N) → {files: [path…]}; no folder → {files: [{name, dataBase64}]}", has_doc, export::export_for_screens ), diff --git a/crates/engine/src/cmd/fileio/tests.rs b/crates/engine/src/cmd/fileio/tests.rs index cf7e58020..ea31f52bc 100644 --- a/crates/engine/src/cmd/fileio/tests.rs +++ b/crates/engine/src/cmd/fileio/tests.rs @@ -278,3 +278,53 @@ fn export_for_screens_one_page_per_pdf_unique_names() { assert!(s.execute("document.exportForScreens", &json!({"formats": [{"format": "vectorcraft"}]})).is_err()); let _ = std::fs::remove_dir_all(dir); } + +#[test] +fn template_sublayers_are_left_out_too() { + let mut s = session(100.0, 100.0, 1); + let sub = s.execute("layer.newSublayer", &json!({})).unwrap()["id"].as_u64().unwrap(); + s.execute("layer.setCurrent", &json!({"id": sub})).unwrap(); + s.execute("paint.setFill", &json!({"color": "#ff0000"})).unwrap(); + let red = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 50, "height": 100})).unwrap()["id"].as_u64().unwrap(); + assert_eq!(s.doc().unwrap().doc.ancestry(vectorcraft_doc::NodeId(red)).unwrap().len(), 3, "the square is on a sublayer"); + assert!(has_red(&b64(&s.execute("document.export", &json!({"format": "png"})).unwrap()))); + s.execute("layer.setProps", &json!({"id": sub, "template": true})).unwrap(); + assert!(!has_red(&b64(&s.execute("document.export", &json!({"format": "png"})).unwrap())), "a template sublayer is left out"); + s.execute("select.set", &json!({"ids": [red]})).unwrap(); + assert!(s.execute("document.exportSelection", &json!({"format": "png"})).is_err(), "nothing but template art is selected"); +} + +#[test] +fn export_for_screens_vector_rows_drop_scale_suffixes_and_names_ignore_case() { + let mut s = session(100.0, 80.0, 3); + s.execute("artboard.setProps", &json!({"index": 0, "name": "Icon"})).unwrap(); + s.execute("artboard.setProps", &json!({"index": 1, "name": "icon"})).unwrap(); + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().doc).artboards[2].name.clear(); + // The dialog's second row starts as PNG @2x; switched to PDF it keeps that suffix text. + let formats = + json!([{"format": "png", "scale": 1, "suffix": ""}, {"format": "pdf", "scale": 2, "suffix": "@2x"}, {"format": "svg", "suffix": "-web"}]); + let r = s.execute("document.exportForScreens", &json!({"formats": formats})).unwrap(); + let names: Vec<&str> = r["files"].as_array().unwrap().iter().map(|f| f["name"].as_str().unwrap()).collect(); + assert_eq!( + names, + [ + "Icon.png", + "Icon.pdf", + "Icon-web.svg", + "icon-2.png", + "icon-2.pdf", + "icon-2-web.svg", + "Artboard-3.png", + "Artboard-3.pdf", + "Artboard-3-web.svg" + ] + ); +} + +#[test] +fn oversized_raster_exports_are_refused() { + let mut s = session(2000.0, 2000.0, 1); + let e = s.execute("document.export", &json!({"format": "webp", "scale": 10})).unwrap_err().to_string(); + assert!(e.contains("too large"), "{e}"); + assert!(s.execute("document.export", &json!({"format": "png", "scale": 40})).is_err(), "80000 px a side"); +} diff --git a/crates/mcp/src/tests_fileio.rs b/crates/mcp/src/tests_fileio.rs index 9b30034df..8b35f91d0 100644 --- a/crates/mcp/src/tests_fileio.rs +++ b/crates/mcp/src/tests_fileio.rs @@ -83,7 +83,7 @@ fn recording_app() -> (String, std::thread::JoinHandle>) { #[test] fn export_is_the_same_call_in_the_app_and_headless() { - let args = json!({"format": "png", "artboard": 1, "options": {"scale": 2, "artboard": 0}}); + let args = json!({"format": "png", "artboard": 1, "options": {"scale": 2, "artboard": 0, "range": "1-2"}}); let (addr, app) = recording_app(); let mut remote = Remote::connect(&addr).unwrap(); assert!(!call_tool(&mut remote, "export", &args).is_error); diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 484253b34..6c369c873 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -2,7 +2,7 @@ //! [`Backend`]. use serde_json::{Map, Value, json}; -use vectorcraft_engine::cmd::fileio::{FORMATS, OPEN_EXTS}; +use vectorcraft_engine::cmd::fileio::{ARTBOARD_PARAMS, FORMATS, OPEN_EXTS}; use crate::backend::Backend; @@ -736,8 +736,14 @@ fn dispatch(b: &mut dyn Backend, name: &str, a: &Args) -> Result j(b.call("app.open", json!({"path": req_str(a, "path")?}))?), "save_file" => j(b.call("app.save", json!({"path": a.get("path").and_then(Value::as_str)}))?), "export" => { - // One params object for every backend; the named arguments win over `options`. + // One params object for every backend; the named arguments win over `options` (a named + // artboard or range replaces every artboard choice in `options`). let mut params = a.get("options").and_then(Value::as_object).cloned().unwrap_or_default(); + if ["artboard", "range"].iter().any(|k| a.get(*k).is_some_and(|v| !v.is_null())) { + for k in ARTBOARD_PARAMS { + params.remove(k); + } + } for k in ["path", "format", "scale", "artboard", "range", "outlineText"] { if let Some(v) = a.get(k).filter(|v| !v.is_null()) { params.insert(k.into(), v.clone()); diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 3eaadc4cf..10cfadc64 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -298,8 +298,7 @@ impl Renderer { if !self.draw_pattern_edit(&mut ctx, &frame) { // While editing an opacity mask its art is seen only through the mask (Illustrator). let mask_layer = doc.mask_edit.map(|m| m.layer); - let skipped = |l: &Node| Some(l.id) == mask_layer || opts.skip_templates && matches!(l.kind, NodeKind::Layer { template: true, .. }); - for layer in doc.layers.iter().filter(|l| !skipped(l)) { + for layer in doc.layers.iter().filter(|l| Some(l.id) != mask_layer) { self.draw_arc(&mut ctx, &frame, layer); } } @@ -407,7 +406,8 @@ impl Renderer { } fn draw_arc(&mut self, ctx: &mut RenderContext, f: &Frame, a: &Arc) { - if !a.visible || f.opts.hidden.contains(&a.id) { + let skipped_template = f.opts.skip_templates && matches!(a.kind, NodeKind::Layer { template: true, .. }); + if !a.visible || skipped_template || f.opts.hidden.contains(&a.id) { return; } match self.bounds_of(a) { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 103312bc0..e4431f948 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -19,7 +19,7 @@ //! (`file.close`, `file.closeAll` and `app.quit` first open a `saveChanges` dialog for each //! modified document: `ui.dialog.confirm` saves, set `discard: true` then confirm to discard) //! - `app.export {path?, format?, …document.export options}`: encoded by the engine, written through -//! the host (a download on the web); the document keeps its path +//! the host; the document keeps its path. No path → `{dataBase64, format, bytes}` (as headless) use std::sync::mpsc::Sender; @@ -296,7 +296,11 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques } "app.open" => wrap(app.run("file.open", json!({"path": s("path")}))), "app.save" => wrap(app.run("file.save", json!({"path": s("path")}))), - "app.export" => wrap(crate::io::export(app, s("format"), s("path").map(str::to_string), p).map(|p| json!({"path": p}))), + "app.export" => match s("path") { + Some(path) => wrap(crate::io::export(app, s("format"), Some(path.to_string()), p).map(|p| json!({"path": p}))), + // No save dialog for an agent: the bytes come back, as in headless mode. + None => wrap(app.run("document.export", p.clone())), + }, "app.quit" => { ctx.send_viewport_cmd(egui::ViewportCommand::Close); ok(Value::Null) diff --git a/crates/ui-egui/src/io.rs b/crates/ui-egui/src/io.rs index 8f75fcfcf..8dde2d9f8 100644 --- a/crates/ui-egui/src/io.rs +++ b/crates/ui-egui/src/io.rs @@ -249,4 +249,16 @@ mod tests { app.run("file.exportForScreens", Value::Null).unwrap(); assert_eq!(app.ui.dialog.as_ref().map(|d| d.kind.as_str()), Some("exportForScreens")); } + + #[test] + fn control_export_without_path_returns_bytes() { + let (mut app, written) = app(); + app.session.execute("file.new", &json!({"width": 30, "height": 20})).unwrap(); + let (req, _) = crate::control::ControlRequest::new("app.export", json!({"format": "png", "scale": 2})); + let crate::control::Outcome::Done(r) = crate::control::handle(&mut app, &egui::Context::default(), &req) else { panic!("not done") }; + assert_eq!(r["ok"], true, "{r}"); + let png = vectorcraft_format::base64_decode(r["result"]["dataBase64"].as_str().unwrap()).unwrap(); + assert_eq!(&png[16..20], 60u32.to_be_bytes(), "30 pt at scale 2"); + assert!(written.borrow().is_empty(), "no file and no save dialog"); + } } diff --git a/docs/control-protocol.md b/docs/control-protocol.md index e4205ccc4..9f7d6c782 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -18,4 +18,4 @@ | `ui.screenshot` | `{path?}` | capture the window (PNG). Needs a presented frame: with the screen locked or the window minimized/covered it fails after ~8 s with an explanatory error | | `ui.render` | `{path?, scale?}` | render the artboard headlessly (PNG) | | `ui.resize` / `ui.focus` | | | -| `app.open` / `app.save` / `app.export` / `app.quit` | `{path}` / `{path?}` / `{path?, format?, artboard?, range?, scale?, …}` | `app.open` reads every format `document.open` reads (see `document.formats`). `app.export` encodes through the engine's `document.export` (same options; the document keeps its path) and writes through the host (a download on the web). `app.quit`, `file.close` and `file.closeAll` first open a `saveChanges` dialog for each modified document (they return `{"pending": "saveChanges"}`): `ui.dialog.confirm` saves, `ui.dialog.set {field: "discard", value: true}` then confirm discards, `ui.dialog.cancel` cancels the whole close or quit | +| `app.open` / `app.save` / `app.export` / `app.quit` | `{path}` / `{path?}` / `{path?, format?, artboard?, range?, scale?, …}` | `app.open` reads every format `document.open` reads (see `document.formats`). `app.export` encodes through the engine's `document.export` (same options; the document keeps its path) and writes `path` through the host; without `path` it returns `{dataBase64, format, bytes}`, as headless mode does. `app.quit`, `file.close` and `file.closeAll` first open a `saveChanges` dialog for each modified document (they return `{"pending": "saveChanges"}`): `ui.dialog.confirm` saves, `ui.dialog.set {field: "discard", value: true}` then confirm discards, `ui.dialog.cancel` cancels the whole close or quit |