From 6f46fcfc4ae5d5dad7c28342a3519abfb79bf8f9 Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Wed, 14 Oct 2026 23:17:59 +0700 Subject: [PATCH 01/36] fix(svg): budget rasterised fallback layers across imports --- crates/io/src/svg.rs | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/crates/io/src/svg.rs b/crates/io/src/svg.rs index 04fc4bee3a..cc858a0694 100644 --- a/crates/io/src/svg.rs +++ b/crates/io/src/svg.rs @@ -28,6 +28,9 @@ use crate::{ImportResult, IoError}; pub const MAX_SIDE: u32 = 16_384; /// A rasterised fallback layer or a smart-object render never exceeds this many pixels. const MAX_RASTER_PIXELS: u64 = 24_000_000; +/// Aggregate decoded raster memory allowed when opening one drawing. Per-element limits +/// alone do not stop thousands of masked groups from exhausting memory. +const MAX_PIXEL_BYTES: u64 = 1 << 30; /// Past this many drawable elements the whole drawing opens as one pixel layer. const MAX_NODES: usize = 2_000; /// Nesting past which a subtree is rasterised instead of walked (bounded recursion). @@ -212,6 +215,10 @@ pub fn import_svg_with_group_depth(name: &str, bytes: &[u8], max_group_depth: us } fn import_parsed(name: &str, parsed: Parsed, max_group_depth: usize) -> Result { + import_parsed_with_budget(name, parsed, max_group_depth, MAX_PIXEL_BYTES) +} + +fn import_parsed_with_budget(name: &str, parsed: Parsed, max_group_depth: usize, pixel_budget: u64) -> Result { let Parsed { tree, fontless_text } = parsed; let mut warnings = Vec::new(); if fontless_text { @@ -222,7 +229,7 @@ fn import_parsed(name: &str, parsed: Parsed, max_group_depth: usize) -> Result MAX_NODES { conv.warnings.push(format!("SVG: more than {MAX_NODES} elements; opened as one pixel layer instead of shapes")); let (w, h) = (cv.size.width, cv.size.height); @@ -311,6 +318,8 @@ struct Conv { noted: Vec<&'static str>, /// Unnamed shapes, groups, images and texts so far, for "Shape 3"-style names. counts: [usize; 4], + /// Remaining budget for rasterised fallback layers, measured in f32 sample bytes. + pixel_budget: u64, } impl Conv { @@ -321,6 +330,20 @@ impl Conv { } } + fn charge_pixels(&mut self, w: usize, h: usize) -> bool { + let bytes = (w as u64).saturating_mul(h as u64).saturating_mul(u64::from(self.fmt.channels()) * 4); + match self.pixel_budget.checked_sub(bytes) { + Some(remaining) => { + self.pixel_budget = remaining; + true + } + None => { + self.note("pixel-budget", "SVG: rasterised elements exceed the import's pixel memory budget; remaining elements were dropped".into()); + false + } + } + } + /// The element's `id`, or a numbered name by kind (0 shape, 1 group, 2 image, 3 text). fn name(&mut self, id: &str, kind: usize) -> String { if !id.is_empty() { @@ -548,7 +571,7 @@ impl Conv { let y0 = ((bb.y() * k).floor() as i32).max(self.canvas.y0); let x1 = ((bb.right() * k).ceil() as i32).min(self.canvas.x1); let y1 = ((bb.bottom() * k).ceil() as i32).min(self.canvas.y1); - if x1 <= x0 || y1 <= y0 { + if x1 <= x0 || y1 <= y0 || !self.charge_pixels((x1 - x0) as usize, (y1 - y0) as usize) { return None; } let Some(mut pixmap) = pixmap((x1 - x0) as u32, (y1 - y0) as u32) else { @@ -768,6 +791,22 @@ mod tests { assert!(r.warnings.iter().any(|w| w.contains("clip-path")), "{:?}", r.warnings); } + + #[test] + fn rasterized_groups_stop_at_the_aggregate_pixel_budget() { + let mut svg = String::from(""); + svg.push_str(""); + for i in 0..8 { + svg.push_str(&format!("")); + } + svg.push_str(""); + // Enough for one 256×256 RGBA-f32 surface, but not eight of them. + let budget = 256u64 * 256 * 4 * 4; + let result = import_parsed_with_budget("budget", parse(svg.as_bytes()).unwrap(), MAX_GROUP_DEPTH, budget).unwrap(); + assert_eq!(result.document.layers.len(), 1, "later raster layers are omitted"); + assert!(result.warnings.iter().any(|warning| warning.contains("pixel memory")), "{:?}", result.warnings); + } + #[test] fn huge_drawings_scale_to_the_side_limit() { let tree = parse(b"").unwrap().tree; From 92647dd6ffcac9f00857b07afff28e652dead97f Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Wed, 14 Oct 2026 23:18:25 +0700 Subject: [PATCH 02/36] fix(svg): cast channel count for pixel budget arithmetic --- crates/io/src/svg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/io/src/svg.rs b/crates/io/src/svg.rs index cc858a0694..11d80ed92b 100644 --- a/crates/io/src/svg.rs +++ b/crates/io/src/svg.rs @@ -331,7 +331,7 @@ impl Conv { } fn charge_pixels(&mut self, w: usize, h: usize) -> bool { - let bytes = (w as u64).saturating_mul(h as u64).saturating_mul(u64::from(self.fmt.channels()) * 4); + let bytes = (w as u64).saturating_mul(h as u64).saturating_mul(self.fmt.channels() as u64 * 4); match self.pixel_budget.checked_sub(bytes) { Some(remaining) => { self.pixel_budget = remaining; From 205c9c173721fadceeeb3695794387eceabe8950 Mon Sep 17 00:00:00 2001 From: Muhammad Saad Date: Thu, 15 Oct 2026 09:39:25 +0400 Subject: [PATCH 03/36] History Brush: bound the stroke before its dense buffers (#4961) The History Brush renders its coverage and reads its source region densely over the stroke's bounds. Coverage is accumulated in tiles, so a thin stroke holds only the tiles it touches, but dense_coverage() materialises the whole bounding box and Region::read allocates that box again for every channel. Neither is admitted first, and both go through the infallible vec!, which aborts the process instead of returning an error. Pattern Stamp already refuses a stroke it cannot render: reject_huge_stroke bounds it from the points and the brush size before anything is rendered, reject_bounds checks the rendered box, and MAX_STROKE_PIXELS (8192 squared) leaves room for a stroke that runs corner to corner across a 24-36 MP canvas. The three were private to that module. Move them to the shared retouch_cmds module and apply them in the History Brush: reject_huge_stroke before the source surface is cloned or any coverage is rendered, reject_bounds before the dense source read. The budget is unchanged, so every stroke that worked before still works. Clone Stamp, Healing Brush, Spot Healing and Remove reach the same conversion and are left for a follow-up. Test: a stroke whose box dwarfs the canvas is refused with the size error and leaves the pixels as they were, while one that runs corner to corner across the canvas still paints, at 8, 16 and 32 bits. --- crates/engine/src/retouch_cmds.rs | 51 +++++++++++++++++++ .../engine/src/retouch_cmds/pattern_stamp.rs | 46 +---------------- crates/engine/src/retouch_cmds/tests.rs | 21 ++++++++ 3 files changed, 73 insertions(+), 45 deletions(-) diff --git a/crates/engine/src/retouch_cmds.rs b/crates/engine/src/retouch_cmds.rs index 9ea7fd5975..9726172062 100644 --- a/crates/engine/src/retouch_cmds.rs +++ b/crates/engine/src/retouch_cmds.rs @@ -35,6 +35,52 @@ fn bad(cmd: &str, msg: impl Into) -> EngineError { EngineError::BadParams { cmd: cmd.into(), msg: msg.into() } } +/// A single stroke's accumulated coverage must stay at or under this many pixels (8192², so a +/// stroke corner to corner across a 24–36 MP canvas fits). Larger footprints return an error +/// instead of allocating. +const MAX_STROKE_PIXELS: u64 = 67_108_864; + +/// Refuse a stroke whose footprint would not fit the coverage budget, from the points and brush +/// size alone, before the renderer allocates anything for it. +fn reject_huge_stroke(cmd: &str, stroke: &Stroke) -> Result<()> { + let pad = f64::from(stroke.brush.size.max(1.0)).mul_add(0.5, 4.0); + let mut min_x = f64::INFINITY; + let mut min_y = f64::INFINITY; + let mut max_x = f64::NEG_INFINITY; + let mut max_y = f64::NEG_INFINITY; + for q in &stroke.points { + min_x = min_x.min(q.x); + min_y = min_y.min(q.y); + max_x = max_x.max(q.x); + max_y = max_y.max(q.y); + } + if !min_x.is_finite() || !min_y.is_finite() || !max_x.is_finite() || !max_y.is_finite() { + return Err(bad(cmd, "point coordinates must be finite")); + } + let w = (max_x - min_x + pad * 2.0).ceil().max(1.0); + let h = (max_y - min_y + pad * 2.0).ceil().max(1.0); + if !w.is_finite() || !h.is_finite() || w > 1.0e9 || h > 1.0e9 { + return Err(bad(cmd, format!("stroke too large (at most {MAX_STROKE_PIXELS} px)"))); + } + let area = (w as u64).saturating_mul(h as u64); + if area > MAX_STROKE_PIXELS { + return Err(bad(cmd, format!("stroke too large ({w:.0}×{h:.0} px, at most {MAX_STROKE_PIXELS})"))); + } + Ok(()) +} + +fn reject_bounds(cmd: &str, bounds: Rect) -> Result<()> { + if bounds.is_empty() { + return Ok(()); + } + let (w, h) = (u64::from(bounds.width()), u64::from(bounds.height())); + let area = w.saturating_mul(h); + if area > MAX_STROKE_PIXELS { + return Err(bad(cmd, format!("stroke too large ({w}×{h} px, at most {MAX_STROKE_PIXELS})"))); + } + Ok(()) +} + /// A pixel layer, a layer with a mask, or a targeted alpha channel / Quick Mask (as the Brush). fn has_pixel_layer(s: &Session) -> std::result::Result<(), String> { if crate::channel_cmds::edits_channel(s) { @@ -631,6 +677,10 @@ fn history_brush(s: &mut Session, p: &Value) -> Result { const CMD: &str = "paint.historyBrush"; let (stroke, id) = parse_brush(s, p, CMD)?; let mode = blend_param(p, CMD)?; + // The coverage and the source region below are both dense over the stroke's bounds, so a + // thin stroke across a large canvas would still ask for the whole box. Refuse it from the + // points and brush size, before the source surface is cloned or anything is rendered (#4961). + reject_huge_stroke(CMD, &stroke)?; let live = s.live_commit.take().filter(|l| l.matches(s, CMD, p, &stroke.points)); let from_live = live.is_some(); let src_surface = history_source(s, id, p, CMD)?; @@ -642,6 +692,7 @@ fn history_brush(s: &mut Session, p: &Value) -> Result { let fmt = surf.format(); let src = if src_surface.format() == fmt { src_surface } else { src_surface.convert(fmt) }; let (bounds, cov) = stroke_coverage(&stroke); + reject_bounds(CMD, bounds)?; let paint = Region::read(&src, bounds); Ok(apply_coverage(surf, bounds, &cov, stroke.brush.opacity, sel, lock, &paint, mode)) })?; diff --git a/crates/engine/src/retouch_cmds/pattern_stamp.rs b/crates/engine/src/retouch_cmds/pattern_stamp.rs index 0dc9f102ac..3c90c6d2a4 100644 --- a/crates/engine/src/retouch_cmds/pattern_stamp.rs +++ b/crates/engine/src/retouch_cmds/pattern_stamp.rs @@ -13,14 +13,9 @@ use photocraft_paint::{Stroke, StrokePoint}; use photocraft_raster::{Surface, from_rgba_into, to_rgba}; use serde_json::{Value, json}; -use super::{bad, blend_param, damage_json, flag, has_pixel_layer, parse_brush}; +use super::{bad, blend_param, damage_json, flag, has_pixel_layer, parse_brush, reject_bounds, reject_huge_stroke}; use crate::{Result, Session}; -/// A single stroke's accumulated coverage must stay at or under this many pixels (8192², so a -/// stroke corner to corner across a 24–36 MP canvas fits). Larger footprints return an error -/// instead of allocating. -const MAX_STROKE_PIXELS: u64 = 67_108_864; - const STREAM_PHASE_X: u64 = 101; const STREAM_PHASE_Y: u64 = 102; @@ -123,45 +118,6 @@ fn stamp_phase(p: &Value, cmd: &str, aligned: bool, first: &StrokePoint) -> Resu } } -fn reject_huge_stroke(cmd: &str, stroke: &Stroke) -> Result<()> { - let pad = f64::from(stroke.brush.size.max(1.0)).mul_add(0.5, 4.0); - let mut min_x = f64::INFINITY; - let mut min_y = f64::INFINITY; - let mut max_x = f64::NEG_INFINITY; - let mut max_y = f64::NEG_INFINITY; - for q in &stroke.points { - min_x = min_x.min(q.x); - min_y = min_y.min(q.y); - max_x = max_x.max(q.x); - max_y = max_y.max(q.y); - } - if !min_x.is_finite() || !min_y.is_finite() || !max_x.is_finite() || !max_y.is_finite() { - return Err(bad(cmd, "point coordinates must be finite")); - } - let w = (max_x - min_x + pad * 2.0).ceil().max(1.0); - let h = (max_y - min_y + pad * 2.0).ceil().max(1.0); - if !w.is_finite() || !h.is_finite() || w > 1.0e9 || h > 1.0e9 { - return Err(bad(cmd, format!("stroke too large (at most {MAX_STROKE_PIXELS} px)"))); - } - let area = (w as u64).saturating_mul(h as u64); - if area > MAX_STROKE_PIXELS { - return Err(bad(cmd, format!("stroke too large ({w:.0}×{h:.0} px, at most {MAX_STROKE_PIXELS})"))); - } - Ok(()) -} - -fn reject_bounds(cmd: &str, bounds: Rect) -> Result<()> { - if bounds.is_empty() { - return Ok(()); - } - let (w, h) = (u64::from(bounds.width()), u64::from(bounds.height())); - let area = w.saturating_mul(h); - if area > MAX_STROKE_PIXELS { - return Err(bad(cmd, format!("stroke too large ({w}×{h} px, at most {MAX_STROKE_PIXELS})"))); - } - Ok(()) -} - fn placement(phase: (f32, f32), scale: f32, angle: f32) -> Placement { Placement::new(Rect::EMPTY, false, phase, scale, angle) } diff --git a/crates/engine/src/retouch_cmds/tests.rs b/crates/engine/src/retouch_cmds/tests.rs index e20706857a..d72924c430 100644 --- a/crates/engine/src/retouch_cmds/tests.rs +++ b/crates/engine/src/retouch_cmds/tests.rs @@ -544,6 +544,27 @@ fn history_brush_restores_earlier_pixels() { } } +/// #4961: the History Brush's coverage and its source region are both dense over the stroke's +/// bounds, so a stroke whose box dwarfs the canvas is refused from the points and brush size +/// alone, before the source surface is cloned or any coverage is rendered. The document is left +/// as it was, and ordinary strokes still paint at every depth. +#[test] +fn history_brush_refuses_a_stroke_too_large_to_render() { + for depth in DEPTHS { + let mut s = session(40, 20, depth, "rgb"); + s.execute("paint.stroke", json!({"points": [[0, 10], [40, 10]], "size": 12, "hardness": 1.0, "color": "#ff0000"})).unwrap(); + let before = rgba(&s, 20, 10); + + let err = s.execute("paint.historyBrush", json!({"points": [[0, 0], [60000, 60000]], "size": 6, "hardness": 100})).unwrap_err(); + assert!(err.to_string().contains("stroke too large"), "depth {depth}: {err}"); + assert_eq!(rgba(&s, 20, 10), before, "depth {depth}: a refused stroke leaves the pixels alone"); + + // The guard is sized for real work: a stroke corner to corner across the canvas still runs. + s.execute("paint.historyBrush", json!({"points": [[0, 0], [40, 20]], "size": 6, "hardness": 100})).unwrap(); + assert!(rgba(&s, 20, 10).iter().all(|v| *v > 0.99), "depth {depth}: the Open snapshot is white"); + } +} + #[test] fn history_brush_reads_a_source_when_its_pixels_were_locked() { // Given a white layer locked in history state 1, then unlocked and filled red. From 5b39b2b3af04c84311fe715775acdfa6ef49eaab Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:18:19 +0200 Subject: [PATCH 04/36] =?UTF-8?q?Help=20=E2=80=BA=20Search:=20typed=20lett?= =?UTF-8?q?ers=20go=20to=20the=20field,=20not=20to=20menu=20mnemonics=20(#?= =?UTF-8?q?5996)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In an open menu a plain letter runs the row with that mnemonic. Typing into Help › Search did too: "S" ran System Info and "A" opened the ArtCraft website, so only "De" of "Deselect" could be typed. Mnemonics now wait while a text field in the menu has the keyboard. --- crates/ui-egui/src/menu_keys.rs | 6 ++++-- crates/ui-egui/src/menus.rs | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/menu_keys.rs b/crates/ui-egui/src/menu_keys.rs index 5762b2a945..7fe6c5e388 100644 --- a/crates/ui-egui/src/menu_keys.rs +++ b/crates/ui-egui/src/menu_keys.rs @@ -207,8 +207,10 @@ impl Keys { return; } if open { - // A letter runs the row with that mnemonic in the level the arrows are in. - let letter = events.iter().find_map(|e| match e { + // A letter runs the row with that mnemonic in the level the arrows are in, unless it is + // typed into a field in the menu: Help › Search took "S" for System Info (#5996). + let typing = ctx.text_edit_focused(); + let letter = events.iter().filter(|_| !typing).find_map(|e| match e { Event::Key { key, pressed: true, modifiers, .. } if !modifiers.command && !modifiers.ctrl && !modifiers.alt => { key_char(*key).map(|c| (*key, c)) } diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 7f47e6ea64..4db0d61855 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1956,6 +1956,44 @@ mod tests { } } + /// #5996: a real key press comes as a key and its text. A letter typed into Help › Search is + /// text for the field, not a mnemonic: "S" ran System Info and "A" the ArtCraft website. + #[test] + fn help_search_letters_are_not_mnemonics() { + use egui::{Event, Key, Modifiers, PointerButton}; + use egui_kittest::{Harness, kittest::Queryable}; + let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).with_max_steps(64).build_eframe(|cc| { + PhotocraftApp::setup_context(&cc.egui_ctx, crate::theme::ThemeKind::ALL[0]); + PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()) + }); + h.run_steps(4); + let help = h.get_by_label("Help").rect().center(); + h.event(Event::PointerMoved(help)); + h.run_steps(1); + for pressed in [true, false] { + h.event(Event::PointerButton { pos: help, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }); + h.run_steps(1); + } + h.run_steps(3); + assert!(h.query_by_label_contains("About PhotoCraft").is_some(), "Help opened"); + for c in "DeSelect all".chars() { + let key = Key::from_name(&c.to_ascii_uppercase().to_string()); + if let Some(key) = key { + h.event(Event::Key { key, physical_key: Some(key), pressed: true, repeat: false, modifiers: Modifiers::NONE }); + } + h.event(Event::Text(c.to_string())); + h.run_steps(1); + if let Some(key) = key { + h.event(Event::Key { key, physical_key: Some(key), pressed: false, repeat: false, modifiers: Modifiers::NONE }); + } + } + h.run_steps(3); + assert!(h.query_by_label_contains("About PhotoCraft").is_some(), "typing keeps Help open"); + let query: String = h.ctx.data(|d| d.get_temp(egui::Id::new("help-menu-search"))).unwrap_or_default(); + assert_eq!(query, "DeSelect all", "every letter reached the field"); + assert!(h.state().ui.dialogs.is_empty(), "no menu command ran: {:?}", h.state().ui.dialogs); + } + #[test] fn dropdown_items_have_room() { use egui_kittest::{Harness, kittest::Queryable}; From 494308a038d6c4c31b4c05d444f67fa9ac06978c Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:28:55 +0200 Subject: [PATCH 05/36] Image Size: Indexed Color resamples with Nearest Neighbor only (#6019) An Indexed Color document holds only its colour table's colours, and a save quantises to them. Interpolating methods showed blended colours on screen that the saved file then lost. image.imageSize now resamples an Indexed document with Nearest Neighbor whatever method it is given, and the dialog offers only Automatic and Nearest Neighbor for one. --- crates/engine/src/image_cmds.rs | 27 ++++++++++++++++++++++ crates/ui-egui/src/sizing.rs | 40 ++++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index 3e870aa686..5f37a2eaa8 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -171,6 +171,12 @@ fn image_size(s: &mut Session, p: &Value) -> Result { let d = s.active().ok_or(EngineError::NoDocument)?; let (ow, oh) = (d.doc.size.width, d.doc.size.height); let resample = parse_resample(p.get("resample").and_then(Value::as_str).unwrap_or("bicubic")); + // An Indexed Color document holds only its colour table's colours, and a save quantises to + // them: interpolation showed blends that the saved file didn't have (#6019). + let resample = match resample { + Method::Filter(_) | Method::Automatic if d.doc.mode == ColorMode::Indexed => Method::Filter(Resample::Nearest), + m => m, + }; let mut pw = p.get("width").and_then(Value::as_f64); let mut ph = p.get("height").and_then(Value::as_f64); // `percent` scales both sides (what a Photoshop action records as "200%"); an explicit side wins. @@ -943,6 +949,27 @@ mod tests { /// #1544: 64 x 48 to 300000 x 300000 (90 billion pixels) started allocating resampled bands /// without a budget. It is refused before any allocation and the document is untouched; a /// change of resolution only, and an ordinary resize, still work. + #[test] + fn image_size_resamples_indexed_color_with_nearest_neighbor() { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 2, "height": 1})).unwrap(); + s.execute("edit.fill", json!({"color": "#ff0000"})).unwrap(); + s.execute("select.rect", json!({"x": 1, "y": 0, "width": 1, "height": 1})).unwrap(); + s.execute("edit.fill", json!({"color": "#0000ff"})).unwrap(); + s.execute("select.deselect", json!({})).unwrap(); + s.execute("image.mode.indexedColor", json!({})).unwrap(); + for resample in ["bicubic", "automatic", "bilinear", "lanczos", "preserveDetails"] { + s.execute("image.imageSize", json!({"width": 8, "resample": resample})).unwrap(); + let st = s.active().unwrap(); + let surf = st.doc.layers[0].surface().unwrap(); + for x in 0..8 { + let px = surf.pixel(x, 0); + assert!(px[..3] == [1.0, 0.0, 0.0] || px[..3] == [0.0, 0.0, 1.0], "{resample}: no blended colour at x {x}: {px:?}"); + } + s.execute("history.undo", json!({})).unwrap(); + } + } + #[test] fn image_size_refuses_a_raster_past_the_budget() { let mut s = Session::new(); diff --git a/crates/ui-egui/src/sizing.rs b/crates/ui-egui/src/sizing.rs index 3c48e041f4..c3ec7a199b 100644 --- a/crates/ui-egui/src/sizing.rs +++ b/crates/ui-egui/src/sizing.rs @@ -70,7 +70,7 @@ pub fn reset(f: &mut Map) { pub fn open(app: &mut PhotocraftApp, command: &str) -> Option { let unit = pref_unit(app.session.prefs().units_and_rulers.rulers); - let resample = pref_resample(app.session.prefs().general.image_interpolation); + let mut resample = pref_resample(app.session.prefs().general.image_interpolation); let pt_per_in = app.session.prefs().units_and_rulers.point_size.per_inch(); let d = &app.session.active()?.doc; let (w, h, res) = (d.size.width as f64, d.size.height as f64, d.resolution_dpi as f64); @@ -88,6 +88,12 @@ pub fn open(app: &mut PhotocraftApp, command: &str) -> Option { f.insert("width".into(), json!(w)); f.insert("height".into(), json!(h)); if command == "image.imageSize" { + // Indexed Color resamples with Nearest Neighbor only, as the command does (#6019). + let indexed = d.mode == photocraft_color::ColorMode::Indexed; + if indexed && resample != "automatic" { + resample = "nearest"; + } + f.insert("__indexed".into(), json!(indexed)); f.insert("__constrain".into(), json!(true)); f.insert("resolution".into(), json!(res)); f.insert("resample".into(), json!(resample)); @@ -321,6 +327,10 @@ fn image_size(ui: &mut egui::Ui, f: &mut Map) { ("bilinear".to_string(), tl!("Bilinear")), ("lanczos".to_string(), tl!("Lanczos (sharp)")), ]; + // Indexed Color: every other method blends colours the colour table doesn't have, + // which the command doesn't use and a save quantises away (#6019). + let indexed = f.get("__indexed").and_then(Value::as_bool) == Some(true); + let opts: Vec<_> = opts.into_iter().filter(|(k, _)| !indexed || k == "automatic" || k == "nearest").collect(); if crate::widgets::dropdown(ui, "is-resample", &mut cur, &opts, 200.0) { f.insert("resample".into(), json!(cur)); } @@ -517,6 +527,34 @@ mod tests { /// The dialog starts on Preferences › General › Image Interpolation: Bicubic Automatic opens /// as Automatic and the others as themselves (Bicubic Sharper used to open as Lanczos). + #[test] + fn indexed_color_resamples_with_nearest_neighbor_only() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.session.execute("file.new", json!({"width": 200, "height": 100})).unwrap(); + app.session.execute("image.mode.indexedColor", json!({})).unwrap(); + for (pref, method) in [("bicubic", "nearest"), ("bicubicAutomatic", "automatic"), ("nearestNeighbor", "nearest")] { + app.session.execute("prefs.set", json!({"path": "general.imageInterpolation", "value": pref})).unwrap(); + let id = open(&mut app, "image.imageSize").unwrap(); + let f = &app.ui.dialog_mut(id).unwrap().fields; + assert_eq!(f.get("resample"), Some(&json!(method)), "{pref}"); + assert_eq!(f.get("__indexed"), Some(&json!(true))); + app.ui.close_dialog(id); + } + // The open list offers only Automatic and Nearest Neighbor. + use egui_kittest::{Harness, kittest::Queryable}; + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_ui_state(|ui, app| crate::dialogs::show(app, ui.ctx()), app); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ALL[0]); + open(h.state_mut(), "image.imageSize").unwrap(); + h.run_steps(3); + let combo = h.query_all_by_role(egui::accesskit::Role::ComboBox).find(|n| n.value().as_deref() == Some("Nearest Neighbor (hard edges)")); + combo.expect("the Resample list shows Nearest Neighbor").click(); + h.run_steps(3); + assert!(h.query_by_label_contains("Automatic").is_some(), "Automatic is offered"); + for gone in ["Bicubic", "Bilinear", "Lanczos", "Preserve Details"] { + assert!(h.query_by_label_contains(gone).is_none(), "{gone} is not offered for Indexed Color"); + } + } + #[test] fn resample_follows_the_interpolation_preference() { let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); From 510636b19cea3c0e3bcac860c92dee7808eec7d1 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:34:09 +0200 Subject: [PATCH 06/36] Type: Korean paragraph text wraps between words, not syllables (#5959) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unicode's line-breaking rules allow a break between any two Hangul syllables, so Korean box text split words (설 / 정이나). Korean is set with breaks at the spaces between words, as Photoshop does: runs of Hangul now lay out with WordBreak::KeepAll. Chinese and Japanese keep breaking between characters. --- crates/text/src/layout.rs | 22 ++++++++++++++++++++++ crates/text/src/tests.rs | 17 +++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/crates/text/src/layout.rs b/crates/text/src/layout.rs index e2bd223b3b..a09e323ab5 100644 --- a/crates/text/src/layout.rs +++ b/crates/text/src/layout.rs @@ -629,6 +629,11 @@ impl Layouter { for (range, size) in &synthetic_small_caps { b.push(StyleProperty::FontSize(*size), range.clone()); } + // Korean breaks lines at spaces, between words, as Photoshop sets it; Unicode's + // default rules break between any two Hangul syllables, mid-word (#5959). + for range in hangul_runs(&ptext) { + b.push(StyleProperty::WordBreak(parley::WordBreak::KeepAll), range); + } for (&at, &spacing) in tabs.iter().zip(tab_spacing) { b.push(StyleProperty::LetterSpacing(spacing), at..at + 1); } @@ -1375,6 +1380,23 @@ fn first_ascent(line: &parley::Line<'_, RunBrush>) -> Option { best } +/// Hangul (syllables and jamo). +fn is_hangul(c: char) -> bool { + matches!(c, '\u{1100}'..='\u{11FF}' | '\u{3130}'..='\u{318F}' | '\u{A960}'..='\u{A97F}' | '\u{AC00}'..='\u{D7AF}' | '\u{D7B0}'..='\u{D7FF}') +} + +/// The byte ranges of `text` made of Hangul, each as long as it runs. +pub(crate) fn hangul_runs(text: &str) -> Vec> { + let mut runs: Vec> = Vec::new(); + for (i, c) in text.char_indices().filter(|&(_, c)| is_hangul(c)) { + match runs.last_mut() { + Some(r) if r.end == i => r.end = i + c.len_utf8(), + _ => runs.push(i..i + c.len_utf8()), + } + } + runs +} + #[cfg(test)] mod tests { use super::{SmallCapsMode, feature_list, small_caps_mode, variation_list}; diff --git a/crates/text/src/tests.rs b/crates/text/src/tests.rs index 13de665ccd..83d195b517 100644 --- a/crates/text/src/tests.rs +++ b/crates/text/src/tests.rs @@ -1452,6 +1452,23 @@ fn japanese_box_text_preserves_wrap_boundaries() { assert_eq!(end, text.text.len()); } +/// #5959: Korean box text broke between Hangul syllables, mid-word ("설 / 정이나"). Lines break at +/// the spaces between words, as Photoshop sets Korean; a word longer than the box still breaks. +#[test] +fn korean_box_text_wraps_between_words() { + let mut engine = TextEngine::new(); + let mut text = point("여기서는 각종 설정이나 캐릭터 제작을 진행합니다.", 12.0); + text.shape = TextShape::Box { x: 0.0, y: 0.0, width: 60.0, height: 1000.0 }; + let layout = engine.layout(&text, 72.0); + assert!(layout.lines.len() > 2, "{}", layout.lines.len()); + let words: Vec<&str> = text.text.split(' ').collect(); + for line in &layout.lines { + let content = text.text.get(line.range.clone()).expect("UTF-8 line boundaries").trim(); + assert!(content.split(' ').all(|w| words.contains(&w)), "a line holds whole words only: {content:?}"); + } + assert_eq!(super::layout::hangul_runs("a 한국어 b"), vec![2..11]); +} + /// Which craft-fonts family drew the glyphs of `l` (matched by the embedded bytes). fn craft_family_of(l: &crate::TextLayout) -> Vec<&'static str> { let mut v = Vec::new(); From 116a60769712c7624e94b90b1553abc139807f16 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Thu, 15 Oct 2026 09:41:16 +0200 Subject: [PATCH 07/36] =?UTF-8?q?Window=20=E2=80=BA=20Arrange:=20tiles=20k?= =?UTF-8?q?eep=20their=20documents,=20and=20every=20tile=20has=20rulers=20?= =?UTF-8?q?(#6017)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With documents tiled side by side, a click into the other one moved it to the first tile, swapping the two, and only the active tile showed rulers. Each document now keeps its tile (with more documents than tiles, the shown run slides just enough to include the active one), and inactive tiles and floating windows draw their own document's rulers. Guides and the zero point are still dragged in the active view. --- crates/ui-egui/src/canvas.rs | 14 +++++-- crates/ui-egui/src/rulers.rs | 42 ++++++++++++++++---- crates/ui-egui/src/view_cmds/tests.rs | 57 +++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 391ad464fd..6e9a65b072 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2020,9 +2020,11 @@ pub(crate) fn arranged_cells(app: &PhotocraftApp, rect: Rect) -> Option = (0..n).map(|k| (idx + k) % n).zip(cells).collect(); - shown.sort_by_key(|(d, _)| *d); - Some(shown) + // Each document keeps its tile: a click into another one activates it where it is (#6017; it + // used to move to the first tile). With more documents than tiles, the shown run slides + // only as far as it must to include the active one. + let start = (idx + 1).saturating_sub(cells.len()); + Some((start..n).zip(cells).collect()) } fn documents(app: &mut PhotocraftApp, ui: &mut egui::Ui) { @@ -2904,7 +2906,9 @@ pub fn canvas_view(app: &mut PhotocraftApp, ui: &mut egui::Ui, idx: usize, rect: crate::monitor_status::check_window(app, &ctx); } let full = rect; - let rect = if primary { crate::rulers::content_rect(app, rect) } else { rect }; + // Every document view has its rulers, the tiles of Window › Arrange and floating windows + // too, as in Photoshop (#6017). + let rect = crate::rulers::content_rect(app, rect); let doc = app.session.documents()[idx].doc.clone(); let size = [doc.size.width, doc.size.height]; if view.doc_size != size { @@ -4087,6 +4091,8 @@ pub fn canvas_view(app: &mut PhotocraftApp, ui: &mut egui::Ui, idx: usize, rect: } if primary && app.ui.extras.rulers { crate::rulers::draw_rulers(app, ui, full, &xf); + } else if app.ui.extras.rulers { + crate::rulers::paint_rulers(app, ui, full, &xf, &doc); } crate::rotate_view::overlay(app, &ctx, &painter, &xf, &mut view, &response); if primary { diff --git a/crates/ui-egui/src/rulers.rs b/crates/ui-egui/src/rulers.rs index 61ea25d96f..004b4cb321 100644 --- a/crates/ui-egui/src/rulers.rs +++ b/crates/ui-egui/src/rulers.rs @@ -329,10 +329,14 @@ fn ruler_pos(xf: &ViewXform, vertical: bool, u: f64, px_per_unit: f64, zero: [f6 /// (`y_axis` false) or the left one, from Preferences › Units & Rulers and the zoom. Minor ticks /// sit at tenths of the step. fn ruler_scale(app: &PhotocraftApp, xf: &ViewXform, y_axis: bool) -> (f64, f64) { + ruler_scale_of(app, app.session.active().map(|d| &*d.doc), xf, y_axis) +} + +/// [`ruler_scale`] for the view of `doc`. +fn ruler_scale_of(app: &PhotocraftApp, doc: Option<&photocraft_doc::Document>, xf: &ViewXform, y_axis: bool) -> (f64, f64) { let ur = &app.session.prefs().units_and_rulers; let (unit, ppi) = (ur.rulers, ur.point_size.per_inch()); - let (dpi, size) = - app.session.active().map_or((72.0, [1.0, 1.0]), |d| (d.doc.resolution_dpi.max(1.0) as f64, [d.doc.size.width as f64, d.doc.size.height as f64])); + let (dpi, size) = doc.map_or((72.0, [1.0, 1.0]), |d| (d.resolution_dpi.max(1.0) as f64, [d.size.width as f64, d.size.height as f64])); let px_per_unit = unit.to_px(1.0, dpi, if y_axis { size[1] } else { size[0] }, ppi).max(1e-9); // The top ruler runs along document x, which pixel aspect correction stretches. let zoom = if y_axis { xf.zoom } else { xf.zoom_x() }; @@ -356,9 +360,16 @@ fn ruler_label(v: f64, step: f64, whole: bool) -> String { if step >= 1.0 || whole { format!("{}", v.round() as i64) } else { crate::widgets::fmt_num2(v) } } -/// Rulers along the top and left of `full` (the canvas rect before `content_rect`), with the -/// pointer position marked; dragging out of a ruler creates a guide. -pub fn draw_rulers(app: &mut PhotocraftApp, ui: &mut egui::Ui, full: Rect, xf: &ViewXform) { +/// The rulers of `doc`'s view in `full`: background, ticks and labels from its zero point. +/// Returns the top, left and corner rects, the painter, the zero point and the minor tick sizes. +#[allow(clippy::type_complexity)] +fn paint( + app: &PhotocraftApp, + ui: &egui::Ui, + full: Rect, + xf: &ViewXform, + doc: &photocraft_doc::Document, +) -> (Rect, Rect, Rect, egui::Painter, [f64; 2], [f64; 2]) { let t = Tokens::get(ui.ctx()); let top = Rect::from_min_max(pos2(full.left() + RULER, full.top()), pos2(full.right(), full.top() + RULER)); let left = Rect::from_min_max(pos2(full.left(), full.top() + RULER), pos2(full.left() + RULER, full.bottom())); @@ -376,11 +387,11 @@ pub fn draw_rulers(app: &mut PhotocraftApp, ui: &mut egui::Ui, full: Rect, xf: & let unit = app.session.prefs().units_and_rulers.rulers; let whole = unit == Unit::Pixels; // The zero point (#2955): rulers read 0 there and count the distance from it. - let origin = app.session.active().map_or([0.0, 0.0], |d| d.doc.ruler_origin); + let origin = doc.ruler_origin; let mut ticks = [1.0; 2]; for vertical in [false, true] { let p = p.with_clip_rect(if vertical { left } else { top }); - let (px_per_unit, step) = ruler_scale(app, xf, vertical); + let (px_per_unit, step) = ruler_scale_of(app, Some(doc), xf, vertical); ticks[usize::from(vertical)] = step * px_per_unit / 10.0; let (u0, u1) = if vertical { ruler_range(xf, left.left_top(), left.left_bottom(), 1, px_per_unit, origin) @@ -413,6 +424,23 @@ pub fn draw_rulers(app: &mut PhotocraftApp, ui: &mut egui::Ui, full: Rect, xf: & v += step; } } + (top, left, corner, p, origin, ticks) +} + +/// Rulers for a document view that isn't the active one (another tile of Window › Arrange, a +/// floating window): shown as the active view's are, but guides and the zero point are dragged +/// in the active view (#6017). +pub fn paint_rulers(app: &PhotocraftApp, ui: &egui::Ui, full: Rect, xf: &ViewXform, doc: &photocraft_doc::Document) { + paint(app, ui, full, xf, doc); +} + +/// Rulers along the top and left of `full` (the canvas rect before `content_rect`), with the +/// pointer position marked; dragging out of a ruler creates a guide. +pub fn draw_rulers(app: &mut PhotocraftApp, ui: &mut egui::Ui, full: Rect, xf: &ViewXform) { + let Some(doc) = app.session.active().map(|d| d.doc.clone()) else { return }; + let (top, left, corner, p, origin, ticks) = paint(app, ui, full, xf, &doc); + let t = Tokens::get(ui.ctx()); + let unit = app.session.prefs().units_and_rulers.rulers; // Pointer position markers. if let Some(h) = ui.ctx().pointer_hover_pos().filter(|h| full.contains(*h)) { let m = Stroke::new(1.0, t.text); diff --git a/crates/ui-egui/src/view_cmds/tests.rs b/crates/ui-egui/src/view_cmds/tests.rs index 2205b6e157..ba1b61aa29 100644 --- a/crates/ui-egui/src/view_cmds/tests.rs +++ b/crates/ui-egui/src/view_cmds/tests.rs @@ -506,3 +506,60 @@ fn esc_leaves_full_screen_modes_unless_something_else_owns_it() { assert_eq!(app.ui.view.screen_mode, "standard"); assert!(app.session.active().unwrap().doc.selection.is_some()); } + +/// #6017: a click into another tile activated that document and moved it to the first tile, so +/// the two side-by-side documents swapped places. Each document keeps its tile; with more +/// documents than tiles, the shown run slides just enough to include the active one. +#[test] +fn arranged_documents_keep_their_tiles() { + let rect = egui::Rect::from_min_max(egui::pos2(0.0, 0.0), egui::pos2(900.0, 600.0)); + let (mut app, _) = app_with(3); + app.ui.view.arrange = "tileAllVertically".into(); + for active in 0..3 { + app.session.set_active(active); + let shown = crate::canvas::arranged_cells(&app, rect).unwrap(); + let order: Vec = shown.iter().map(|(d, _)| *d).collect(); + assert_eq!(order, vec![0, 1, 2], "active {active}"); + assert!(shown.windows(2).all(|w| w[0].1.left() < w[1].1.left()), "document 0 on the left"); + } + app.ui.view.arrange = "twoUpVertical".into(); + let docs = |app: &mut PhotocraftApp, active: usize| { + app.session.set_active(active); + crate::canvas::arranged_cells(app, rect).unwrap().iter().map(|(d, _)| *d).collect::>() + }; + assert_eq!(docs(&mut app, 0), vec![0, 1]); + assert_eq!(docs(&mut app, 1), vec![0, 1], "the second tile's document stays on the right"); + assert_eq!(docs(&mut app, 2), vec![1, 2], "the third document slides in next to the second"); +} + +/// #6017: rulers showed in the active tile only; the other tile had none and no room for them. +#[test] +fn every_arranged_tile_has_rulers() { + use egui_kittest::Harness; + let (mut app, _) = app_with(2); + app.ui.view.arrange = "tileAllVertically".into(); + app.ui.extras.rulers = true; + let mut h = Harness::builder().with_size(egui::vec2(1000.0, 600.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + if !ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) { + return; + } + egui::CentralPanel::default().show(ui, |ui| crate::canvas::document_area(app, ui)); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ALL[0]); + h.run_steps(4); + let right = h.state().last_canvas_rect.center().x; + // Ruler labels are 9 pt text: some lie in each half of the document area. + let mut labels = [0, 0]; + for s in &h.output().shapes { + if let egui::Shape::Text(t) = &s.shape + && t.galley.job.sections.first().is_some_and(|sec| sec.format.font_id.size == 9.0) + && t.galley.text().chars().all(|c| c.is_ascii_digit()) + { + labels[usize::from(t.pos.x > right)] += 1; + } + } + assert!(labels[0] > 0 && labels[1] > 0, "ruler labels per tile: {labels:?}"); +} From f116c3230a27fff30bc31b79b72c2cfb0f0ba074 Mon Sep 17 00:00:00 2001 From: Muhammad Saad Date: Thu, 15 Oct 2026 15:47:38 +0400 Subject: [PATCH 08/36] Retouch: guard every stroke command, not only Pattern Stamp (#6110) Pattern Stamp was the only retouch command that refused a stroke it could not render. Clone Stamp, Healing Brush, Spot Healing and Remove all reach the same dense conversion: coverage is accumulated in tiles, but dense_coverage() materialises the whole bounding box, and the retouch paths read a Region over that box for every channel. The box comes from the stroke's own points, which are validated against MAX_COORD rather than the document, so it is not bounded by the canvas. Both allocations go through the infallible vec!, which aborts instead of returning an error. With the guards now shared in retouch_cmds, apply them to the rest: reject_huge_stroke before anything is rendered, and reject_bounds before the dense region read where the command can return one. spot_heal_surface returns a Rect, so Spot Healing charges the stroke in the command; Remove builds a list, so each stroke is charged as it is parsed. The allowance is unchanged, so no stroke that worked before starts failing. Test: every retouch stroke refuses the oversized box, leaves the pixels alone, and still accepts a stroke across the whole canvas. --- crates/engine/src/retouch_cmds.rs | 5 +++ crates/engine/src/retouch_cmds/remove.rs | 2 + crates/engine/src/retouch_cmds/tests.rs | 47 ++++++++++++++++++++++-- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/retouch_cmds.rs b/crates/engine/src/retouch_cmds.rs index 9726172062..b462d37519 100644 --- a/crates/engine/src/retouch_cmds.rs +++ b/crates/engine/src/retouch_cmds.rs @@ -358,6 +358,7 @@ fn clamp_samples(fmt: &PixelFormat, data: &mut [f32]) { fn clone_stamp(s: &mut Session, p: &Value) -> Result { const CMD: &str = "paint.cloneStamp"; let (stroke, id) = parse_brush(s, p, CMD)?; + reject_huge_stroke(CMD, &stroke)?; let which = sample_layers(p, CMD)?; let mode = blend_param(p, CMD)?; let aligned = flag(p, "aligned", true); @@ -383,6 +384,7 @@ fn clone_stamp(s: &mut Session, p: &Value) -> Result { .draw(bounds, &stroke.brush, &stroke.points, |r| clone_sample(pre, id, &before, which, r, &map, true, CMD)); } let (bounds, cov) = stroke_coverage(&stroke); + reject_bounds(CMD, bounds)?; // Samples come from the pre-stroke state, so pixels painted earlier in the stroke are never re-cloned. let paint = clone_sample(pre, id, surf, which, bounds, &map, flag(p, "wrap", false), CMD)?; Ok(apply_coverage(surf, bounds, &cov, stroke.brush.opacity, sel, lock, &paint, mode)) @@ -638,6 +640,7 @@ fn heal_region(fmt: &PixelFormat, src: &Region, dst: &Region, mask: &[bool]) -> fn healing_brush(s: &mut Session, p: &Value) -> Result { const CMD: &str = "paint.healingBrush"; let (stroke, id) = parse_brush(s, p, CMD)?; + reject_huge_stroke(CMD, &stroke)?; let which = sample_layers(p, CMD)?; let mode = blend_param(p, CMD)?; let aligned = flag(p, "aligned", true); @@ -645,6 +648,7 @@ fn healing_brush(s: &mut Session, p: &Value) -> Result { let off = map.offset(); let dmg = run_stroke(s, "Healing Brush", id, p, |pre, surf, sel, lock| { let (bounds, cov) = stroke_coverage(&stroke); + reject_bounds(CMD, bounds)?; let (g, cov) = pad_coverage(bounds, &cov, 2); let fmt = surf.format(); let src = clone_sample(pre, id, surf, which, g, &map, false, CMD)?; @@ -821,6 +825,7 @@ fn spot_heal_surface(surf: &mut Surface, pre: &Document, stroke: &Stroke, kind: fn spot_healing(s: &mut Session, p: &Value) -> Result { const CMD: &str = "paint.spotHealing"; let (stroke, id) = parse_brush(s, p, CMD)?; + reject_huge_stroke(CMD, &stroke)?; let kind = match string(p, "type", "contentAware") { "contentAware" => SpotType::ContentAware, "createTexture" => SpotType::CreateTexture, diff --git a/crates/engine/src/retouch_cmds/remove.rs b/crates/engine/src/retouch_cmds/remove.rs index 4e256fb7fc..55c53d3af7 100644 --- a/crates/engine/src/retouch_cmds/remove.rs +++ b/crates/engine/src/retouch_cmds/remove.rs @@ -71,6 +71,7 @@ fn stroke_bounds(stroke: &Stroke) -> Rect { fn parse_strokes(s: &Session, p: &Value) -> Result<(Vec, Option)> { let Some(list) = p.get("strokes") else { let (stroke, id) = parse_brush(s, p, CMD)?; + reject_huge_stroke(CMD, &stroke)?; return Ok((vec![stroke], id)); }; let list = list.as_array().ok_or_else(|| bad(CMD, "`strokes` is a list of point lists"))?; @@ -86,6 +87,7 @@ fn parse_strokes(s: &Session, p: &Value) -> Result<(Vec, Option o.insert("points".into(), points.clone()); } let (stroke, layer) = parse_brush(s, &one, CMD)?; + reject_huge_stroke(CMD, &stroke)?; strokes.push(stroke); id = layer; } diff --git a/crates/engine/src/retouch_cmds/tests.rs b/crates/engine/src/retouch_cmds/tests.rs index d72924c430..35f6e81efd 100644 --- a/crates/engine/src/retouch_cmds/tests.rs +++ b/crates/engine/src/retouch_cmds/tests.rs @@ -544,10 +544,10 @@ fn history_brush_restores_earlier_pixels() { } } -/// #4961: the History Brush's coverage and its source region are both dense over the stroke's -/// bounds, so a stroke whose box dwarfs the canvas is refused from the points and brush size -/// alone, before the source surface is cloned or any coverage is rendered. The document is left -/// as it was, and ordinary strokes still paint at every depth. +/// #4961 and #6110: every retouch stroke renders its coverage densely over the stroke's bounds +/// and reads a source region over the same box, so a stroke whose box dwarfs the canvas is +/// refused from the points and brush size alone, before anything is cloned or rendered. The +/// document is left as it was, and ordinary strokes still paint at every depth. #[test] fn history_brush_refuses_a_stroke_too_large_to_render() { for depth in DEPTHS { @@ -565,6 +565,45 @@ fn history_brush_refuses_a_stroke_too_large_to_render() { } } +/// #6110: Pattern Stamp already refused a stroke it could not render; every other retouch stroke +/// reached the same dense conversion unguarded. Each command refuses the oversized stroke and +/// leaves the pixels alone, and a stroke across the whole canvas still works. +#[test] +fn every_retouch_stroke_refuses_a_box_it_cannot_render() { + let huge = json!([[0, 0], [900_000, 900_000]]); + let ok = json!([[0, 0], [40, 20]]); + let cases: [(&str, Value); 5] = [ + ("paint.cloneStamp", json!({"offset": [4, 0], "size": 8, "hardness": 100})), + ("paint.healingBrush", json!({"offset": [4, 0], "size": 8, "hardness": 100})), + ("paint.spotHealing", json!({"size": 8, "hardness": 100})), + ("paint.historyBrush", json!({"size": 8, "hardness": 100})), + ("paint.patternStamp", json!({"size": 8, "hardness": 100})), + ]; + for (cmd, base) in cases { + let mut s = session(40, 20, 8, "rgb"); + s.execute("paint.stroke", json!({"points": [[0, 10], [40, 10]], "size": 12, "hardness": 1.0, "color": "#ff0000"})).unwrap(); + let before = rgba(&s, 20, 10); + let with = |points: &Value| { + let mut v = base.clone(); + if let Some(o) = v.as_object_mut() { + o.insert("points".into(), points.clone()); + } + v + }; + let err = s.execute(cmd, with(&huge)).unwrap_err().to_string(); + assert!(err.contains("stroke too large"), "{cmd}: {err}"); + assert_eq!(rgba(&s, 20, 10), before, "{cmd}: a refused stroke leaves the pixels alone"); + // The allowance still covers a stroke across the whole canvas. + let across = s.execute(cmd, with(&ok)); + assert!(across.is_ok() || !format!("{across:?}").contains("stroke too large"), "{cmd}: {across:?}"); + } + + // Remove takes a list of strokes; each one is charged as it is parsed. + let mut s = session(40, 20, 8, "rgb"); + let err = s.execute("paint.remove", json!({"strokes": [[[0, 0], [900_000, 900_000]]], "size": 8})).unwrap_err().to_string(); + assert!(err.contains("stroke too large"), "paint.remove: {err}"); +} + #[test] fn history_brush_reads_a_source_when_its_pixels_were_locked() { // Given a white layer locked in history state 1, then unlocked and filled red. From 9a25a8a3908ba986a68e0db0596b9ce3726b3d23 Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:14:38 +0700 Subject: [PATCH 09/36] style(svg): format expanded conversion state initializer --- crates/io/src/svg.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/io/src/svg.rs b/crates/io/src/svg.rs index a4384c8e17..6537aff28e 100644 --- a/crates/io/src/svg.rs +++ b/crates/io/src/svg.rs @@ -230,7 +230,17 @@ fn import_parsed_with_budget(name: &str, parsed: Parsed, max_group_depth: usize, let fmt = doc.pixel_format(); let global = tiny_skia::Transform::from_scale(cv.scale, cv.scale); let max_group_depth = (max_group_depth != 0).then_some(max_group_depth.clamp(1, MAX_GROUP_DEPTH)); - let mut conv = Conv { fmt, canvas: doc.bounds(), global, max_group_depth, warnings, noted: Vec::new(), counts: [0; 4], pixel_budget, retained_pixel_bytes: 0 }; + let mut conv = Conv { + fmt, + canvas: doc.bounds(), + global, + max_group_depth, + warnings, + noted: Vec::new(), + counts: [0; 4], + pixel_budget, + retained_pixel_bytes: 0, + }; if count_nodes(tree.root()) > MAX_NODES { conv.warnings.push(format!("SVG: more than {MAX_NODES} elements; opened as one pixel layer instead of shapes")); let (w, h) = (cv.size.width, cv.size.height); From a4cdc63d88eebf3debf73da572830244f8ad8f3e Mon Sep 17 00:00:00 2001 From: Muhammad Saad Date: Thu, 15 Oct 2026 23:50:55 +0400 Subject: [PATCH 10/36] Retouch: admit the rendered stroke extent, not an estimate (#4961, #6110) Review of f116c32 was right that the guard did not establish the bound it advertised. reject_bounds ran on the value stroke_coverage() returned, and that function calls dense_coverage() internally, so the dense Vec was already allocated by the time it was checked. reject_huge_stroke, the only check that ran first, estimated from the raw points and half the tip, which brush dynamics exceed: Scattering offsets a dab centre by its amount times the tip size. Split rendering from dense extraction. stroke_coverage_within renders the stroke into the renderer's tiles, which hold only what the dabs touch, reads the extent once every dab is placed and refuses before allocating the width x height buffer. retouch_cmds::coverage wraps it at MAX_STROKE_PIXELS and every stroke command goes through it, so the bound now holds whatever the dynamics do. reject_bounds is removed; it could not do the job it was named for. spot_heal_surface returned a bare Rect and so had nowhere to refuse; it returns Result now. reject_huge_stroke stays as the cheap early-out and pads by the scatter reach and the dual-brush radius so it no longer falls short. The allowance is unchanged, so no stroke that worked before starts failing. Tests: coverage_within takes the budget as a parameter, and a scattering brush proves the rendered extent exceeds what the points alone suggest, is refused one pixel under it and fits at it. Every command's refusal now asserts the pixels and the history step count are untouched, and the ordinary stroke uses a valid fixture and must succeed rather than merely avoid the size error. --- crates/engine/src/retouch_cmds.rs | 57 +++++++++++-------- .../engine/src/retouch_cmds/pattern_stamp.rs | 13 ++--- crates/engine/src/retouch_cmds/remove.rs | 8 +-- crates/engine/src/retouch_cmds/tests.rs | 46 ++++++++++++--- crates/paint/src/retouch.rs | 18 ++++++ 5 files changed, 99 insertions(+), 43 deletions(-) diff --git a/crates/engine/src/retouch_cmds.rs b/crates/engine/src/retouch_cmds.rs index b462d37519..8ef8722c16 100644 --- a/crates/engine/src/retouch_cmds.rs +++ b/crates/engine/src/retouch_cmds.rs @@ -15,7 +15,7 @@ use photocraft_algo::retouch::{ToneRange, dodge_burn, local_blur, local_sharpen, use photocraft_color::{BlendMode, PixelFormat, SampleType}; use photocraft_doc::{Document, LayerContent, LayerId}; use photocraft_geom::Rect; -use photocraft_paint::retouch::{Footprint, Region, Smudge, alpha_index, apply_coverage, apply_dab_stroke, stroke_coverage}; +use photocraft_paint::retouch::{Footprint, Region, Smudge, alpha_index, apply_coverage, apply_dab_stroke}; use photocraft_paint::{BrushSettings, Stroke, StrokePoint}; use photocraft_raster::{Surface, from_rgba_into, to_rgba}; use serde_json::{Value, json}; @@ -43,7 +43,16 @@ const MAX_STROKE_PIXELS: u64 = 67_108_864; /// Refuse a stroke whose footprint would not fit the coverage budget, from the points and brush /// size alone, before the renderer allocates anything for it. fn reject_huge_stroke(cmd: &str, stroke: &Stroke) -> Result<()> { - let pad = f64::from(stroke.brush.size.max(1.0)).mul_add(0.5, 4.0); + // Dynamics place dabs beyond the input points, so half the tip is not the whole reach: + // Scattering offsets a centre by up to its amount times the tip size, and a Dual Brush dab + // reaches its own radius further. Pad by what those can add so this estimate does not fall + // short of what the renderer will cover. It is only the cheap early-out; the rendered extent + // is admitted for real in `coverage` below. + let b = &stroke.brush; + let size = f64::from(b.size.max(1.0)); + let scatter = if b.scattering.enabled { size * f64::from(b.scattering.scatter.jitter.max(0.0)) } else { 0.0 }; + let dual = if b.dual_brush.enabled { f64::from(b.dual_brush.size.max(0.0)) * 0.5 } else { 0.0 }; + let pad = size.mul_add(0.5, 4.0) + scatter + dual; let mut min_x = f64::INFINITY; let mut min_y = f64::INFINITY; let mut max_x = f64::NEG_INFINITY; @@ -69,16 +78,19 @@ fn reject_huge_stroke(cmd: &str, stroke: &Stroke) -> Result<()> { Ok(()) } -fn reject_bounds(cmd: &str, bounds: Rect) -> Result<()> { - if bounds.is_empty() { - return Ok(()); - } - let (w, h) = (u64::from(bounds.width()), u64::from(bounds.height())); - let area = w.saturating_mul(h); - if area > MAX_STROKE_PIXELS { - return Err(bad(cmd, format!("stroke too large ({w}×{h} px, at most {MAX_STROKE_PIXELS})"))); - } - Ok(()) +/// The stroke's coverage, with the **rendered** extent admitted before the dense buffer exists. +/// +/// `reject_huge_stroke` estimates from the input points; this is the real bound. Rendering is +/// tiled, so it holds only the tiles the dabs touch; the extent is read from those tiles once +/// every dab is placed (tips, dynamics and scattering included) and checked against `max`, and +/// only a stroke that fits is made dense. `max` is a parameter so tests can inject a small budget. +fn coverage_within(cmd: &str, stroke: &Stroke, max: u64) -> Result<(Rect, Vec)> { + photocraft_paint::retouch::stroke_coverage_within(stroke, max).ok_or_else(|| bad(cmd, format!("stroke too large (at most {max} px)"))) +} + +/// [`coverage_within`] at the shared allowance. +fn coverage(cmd: &str, stroke: &Stroke) -> Result<(Rect, Vec)> { + coverage_within(cmd, stroke, MAX_STROKE_PIXELS) } /// A pixel layer, a layer with a mask, or a targeted alpha channel / Quick Mask (as the Brush). @@ -383,8 +395,7 @@ fn clone_stamp(s: &mut Session, p: &Value) -> Result { return periodic::Composite { target: surf, before: &before, tile: pre.bounds(), opacity: stroke.brush.opacity, selection: sel, lock, mode } .draw(bounds, &stroke.brush, &stroke.points, |r| clone_sample(pre, id, &before, which, r, &map, true, CMD)); } - let (bounds, cov) = stroke_coverage(&stroke); - reject_bounds(CMD, bounds)?; + let (bounds, cov) = coverage(CMD, &stroke)?; // Samples come from the pre-stroke state, so pixels painted earlier in the stroke are never re-cloned. let paint = clone_sample(pre, id, surf, which, bounds, &map, flag(p, "wrap", false), CMD)?; Ok(apply_coverage(surf, bounds, &cov, stroke.brush.opacity, sel, lock, &paint, mode)) @@ -647,8 +658,7 @@ fn healing_brush(s: &mut Session, p: &Value) -> Result { let map = clone_mapping(s, p, &stroke, CMD)?; let off = map.offset(); let dmg = run_stroke(s, "Healing Brush", id, p, |pre, surf, sel, lock| { - let (bounds, cov) = stroke_coverage(&stroke); - reject_bounds(CMD, bounds)?; + let (bounds, cov) = coverage(CMD, &stroke)?; let (g, cov) = pad_coverage(bounds, &cov, 2); let fmt = surf.format(); let src = clone_sample(pre, id, surf, which, g, &map, false, CMD)?; @@ -695,8 +705,7 @@ fn history_brush(s: &mut Session, p: &Value) -> Result { } let fmt = surf.format(); let src = if src_surface.format() == fmt { src_surface } else { src_surface.convert(fmt) }; - let (bounds, cov) = stroke_coverage(&stroke); - reject_bounds(CMD, bounds)?; + let (bounds, cov) = coverage(CMD, &stroke)?; let paint = Region::read(&src, bounds); Ok(apply_coverage(surf, bounds, &cov, stroke.brush.opacity, sel, lock, &paint, mode)) })?; @@ -773,12 +782,12 @@ enum SpotType { /// Photoshop 25.4 (measured): a dab changes exactly the brush's footprint, and Content-Aware takes /// its texture from around it, as Edit › Fill does from its window /// ([`crate::fill_cmds::sampling_window`] of the stroke's bounds), which is what this samples. -fn spot_heal_surface(surf: &mut Surface, pre: &Document, stroke: &Stroke, kind: SpotType, all: bool, sel: Option<&Surface>, lock: bool) -> Rect { - let (bounds, cov) = stroke_coverage(stroke); +fn spot_heal_surface(surf: &mut Surface, pre: &Document, stroke: &Stroke, kind: SpotType, all: bool, sel: Option<&Surface>, lock: bool) -> Result { + let (bounds, cov) = coverage("paint.spotHealing", stroke)?; let size = stroke.brush.size; let region_rect = crate::fill_cmds::sampling_window(bounds, pre.bounds()); if region_rect.is_empty() { - return Rect::EMPTY; + return Ok(Rect::EMPTY); } // Proximity Match looks for its source anywhere in that window. let margin = (region_rect.width().max(region_rect.height()) / 2) as i32; @@ -795,7 +804,7 @@ fn spot_heal_surface(surf: &mut Surface, pre: &Document, stroke: &Stroke, kind: } let hole: Vec = stroke_cov.iter().map(|c| *c > 0.0).collect(); if !hole.iter().any(|h| *h) { - return Rect::EMPTY; + return Ok(Rect::EMPTY); } // Synthesise over a slightly larger domain than the Poisson region, so the solve's boundary pixels // carry synthesised values that differ from the untouched surroundings: that mismatch is what the @@ -819,7 +828,7 @@ fn spot_heal_surface(surf: &mut Surface, pre: &Document, stroke: &Stroke, kind: let healed = heal_region(&fmt, &src, &img, &heal_mask); let dmg = bounds.intersect(®ion_rect); let dmg_cov: Vec = (dmg.y0..dmg.y1).flat_map(|y| (dmg.x0..dmg.x1).map(move |x| (x, y))).map(|(x, y)| stroke_cov[img.index(x, y)]).collect(); - apply_coverage(surf, dmg, &dmg_cov, stroke.brush.opacity, sel, lock, &healed, BlendMode::Normal) + Ok(apply_coverage(surf, dmg, &dmg_cov, stroke.brush.opacity, sel, lock, &healed, BlendMode::Normal)) } fn spot_healing(s: &mut Session, p: &Value) -> Result { @@ -833,7 +842,7 @@ fn spot_healing(s: &mut Session, p: &Value) -> Result { o => return Err(bad(CMD, format!("unknown type `{o}` (contentAware|createTexture|proximityMatch)"))), }; let all = sample_all_layers(p); - let dmg = run_stroke(s, "Spot Healing Brush", id, p, |pre, surf, sel, lock| Ok(spot_heal_surface(surf, pre, &stroke, kind, all, sel, lock)))?; + let dmg = run_stroke(s, "Spot Healing Brush", id, p, |pre, surf, sel, lock| spot_heal_surface(surf, pre, &stroke, kind, all, sel, lock))?; Ok(json!({ "damage": damage_json(dmg) })) } diff --git a/crates/engine/src/retouch_cmds/pattern_stamp.rs b/crates/engine/src/retouch_cmds/pattern_stamp.rs index 3c90c6d2a4..b4b71745ae 100644 --- a/crates/engine/src/retouch_cmds/pattern_stamp.rs +++ b/crates/engine/src/retouch_cmds/pattern_stamp.rs @@ -8,12 +8,12 @@ use photocraft_color::{BlendMode, PixelFormat}; use photocraft_compose::pattern::{Placement, Tile, render}; use photocraft_doc::Pattern; use photocraft_geom::Rect; -use photocraft_paint::retouch::{Footprint, Region, apply_coverage, apply_dab_stroke, over_native, stroke_coverage}; +use photocraft_paint::retouch::{Footprint, Region, apply_coverage, apply_dab_stroke, over_native}; use photocraft_paint::{Stroke, StrokePoint}; use photocraft_raster::{Surface, from_rgba_into, to_rgba}; use serde_json::{Value, json}; -use super::{bad, blend_param, damage_json, flag, has_pixel_layer, parse_brush, reject_bounds, reject_huge_stroke}; +use super::{bad, blend_param, damage_json, flag, has_pixel_layer, parse_brush, reject_huge_stroke}; use crate::{Result, Session}; const STREAM_PHASE_X: u64 = 101; @@ -150,8 +150,7 @@ fn paint_aligned( mode: BlendMode, cmd: &str, ) -> Result { - let (bounds, cov) = stroke_coverage(stroke); - reject_bounds(cmd, bounds)?; + let (bounds, cov) = super::coverage(cmd, stroke)?; if bounds.is_empty() { return Ok(Rect::EMPTY); } @@ -259,9 +258,9 @@ mod tests { #[test] fn a_diagonal_across_a_24_mp_canvas_is_not_too_large() { - assert!(reject_huge_stroke("t", &stroke(&[(0.0, 0.0), (6015.0, 3999.0)], 100.0)).is_ok()); - assert!(reject_bounds("t", Rect::from_xywh(-60, -60, 6136, 4120)).is_ok()); + let across = stroke(&[(0.0, 0.0), (6015.0, 3999.0)], 100.0); + assert!(reject_huge_stroke("t", &across).is_ok()); + assert!(super::super::coverage("t", &across).is_ok(), "the rendered extent fits too"); assert!(reject_huge_stroke("t", &stroke(&[(0.0, 0.0), (9000.0, 9000.0)], 100.0)).is_err()); - assert!(reject_bounds("t", Rect::from_xywh(0, 0, 9000, 9000)).is_err()); } } diff --git a/crates/engine/src/retouch_cmds/remove.rs b/crates/engine/src/retouch_cmds/remove.rs index 55c53d3af7..d298e8f02c 100644 --- a/crates/engine/src/retouch_cmds/remove.rs +++ b/crates/engine/src/retouch_cmds/remove.rs @@ -117,12 +117,12 @@ fn plan(s: &Session, p: &Value) -> Result { /// The hole over the plan's window: the strokes' footprints, their enclosed areas and a margin, /// limited to the selection. Empty when nothing would change. -fn hole(plan: &Plan, sel: Option<&Surface>) -> Vec { +fn hole(plan: &Plan, sel: Option<&Surface>) -> Result> { let win = plan.window; let (w, h) = (win.width() as usize, win.height() as usize); let mut m = vec![false; w * h]; for stroke in &plan.strokes { - let (bounds, cov) = stroke_coverage(stroke); + let (bounds, cov) = super::coverage(CMD, stroke)?; let bw = bounds.width() as usize; for (i, c) in cov.iter().enumerate() { let (x, y) = (bounds.x0 + (i % bw.max(1)) as i32, bounds.y0 + (i / bw.max(1)) as i32); @@ -143,13 +143,13 @@ fn hole(plan: &Plan, sel: Option<&Surface>) -> Vec { *v &= c > 0.0; } } - m + Ok(m) } /// The filled window and the hole it fills, computed on `doc` (a snapshot). `None` when the /// layer's own window is fully transparent: nothing to remove and nothing to fill from. fn run_remove(doc: &mut Document, plan: &Plan, p: &Value, ctx: &crate::jobs::JobCtx, label: &str) -> Result)>> { - let hole = hole(plan, doc.selection.as_ref()); + let hole = hole(plan, doc.selection.as_ref())?; if !hole.iter().any(|m| *m) { return Err(bad(CMD, "the stroke doesn't cover anything that can change (check the selection)")); } diff --git a/crates/engine/src/retouch_cmds/tests.rs b/crates/engine/src/retouch_cmds/tests.rs index 35f6e81efd..edbf33c317 100644 --- a/crates/engine/src/retouch_cmds/tests.rs +++ b/crates/engine/src/retouch_cmds/tests.rs @@ -566,23 +566,24 @@ fn history_brush_refuses_a_stroke_too_large_to_render() { } /// #6110: Pattern Stamp already refused a stroke it could not render; every other retouch stroke -/// reached the same dense conversion unguarded. Each command refuses the oversized stroke and -/// leaves the pixels alone, and a stroke across the whole canvas still works. +/// reached the same dense conversion unguarded. Each command refuses the oversized stroke without +/// touching the pixels or the history, and an ordinary stroke still succeeds. #[test] fn every_retouch_stroke_refuses_a_box_it_cannot_render() { let huge = json!([[0, 0], [900_000, 900_000]]); - let ok = json!([[0, 0], [40, 20]]); - let cases: [(&str, Value); 5] = [ + let ok = json!([[2, 2], [30, 16]]); + // Fixtures that are valid for a plain RGB document with one pixel layer. + let cases: [(&str, Value); 4] = [ ("paint.cloneStamp", json!({"offset": [4, 0], "size": 8, "hardness": 100})), ("paint.healingBrush", json!({"offset": [4, 0], "size": 8, "hardness": 100})), ("paint.spotHealing", json!({"size": 8, "hardness": 100})), ("paint.historyBrush", json!({"size": 8, "hardness": 100})), - ("paint.patternStamp", json!({"size": 8, "hardness": 100})), ]; for (cmd, base) in cases { let mut s = session(40, 20, 8, "rgb"); s.execute("paint.stroke", json!({"points": [[0, 10], [40, 10]], "size": 12, "hardness": 1.0, "color": "#ff0000"})).unwrap(); let before = rgba(&s, 20, 10); + let steps = s.active().unwrap().history.past_len(); let with = |points: &Value| { let mut v = base.clone(); if let Some(o) = v.as_object_mut() { @@ -593,9 +594,11 @@ fn every_retouch_stroke_refuses_a_box_it_cannot_render() { let err = s.execute(cmd, with(&huge)).unwrap_err().to_string(); assert!(err.contains("stroke too large"), "{cmd}: {err}"); assert_eq!(rgba(&s, 20, 10), before, "{cmd}: a refused stroke leaves the pixels alone"); - // The allowance still covers a stroke across the whole canvas. - let across = s.execute(cmd, with(&ok)); - assert!(across.is_ok() || !format!("{across:?}").contains("stroke too large"), "{cmd}: {across:?}"); + assert_eq!(s.active().unwrap().history.past_len(), steps, "{cmd}: and records no history step"); + + // The allowance still covers ordinary work: this must succeed, not merely avoid the error. + s.execute(cmd, with(&ok)).unwrap_or_else(|e| panic!("{cmd}: an ordinary stroke must work: {e}")); + assert_eq!(s.active().unwrap().history.past_len(), steps + 1, "{cmd}: and records one step"); } // Remove takes a list of strokes; each one is charged as it is parsed. @@ -604,6 +607,33 @@ fn every_retouch_stroke_refuses_a_box_it_cannot_render() { assert!(err.contains("stroke too large"), "paint.remove: {err}"); } +/// The review on #6110: the estimate from the input points is not the bound. Brush dynamics place +/// dabs beyond those points, so the budget is applied to the extent the renderer actually produced, +/// before the dense buffer is allocated. An injected budget makes that observable without asking +/// for a stroke big enough to exhaust anything. +#[test] +fn the_budget_is_applied_to_the_rendered_extent_not_the_input_points() { + use photocraft_paint::{BrushSettings, Stroke, StrokePoint}; + let mut brush = BrushSettings { size: 24.0, hardness: 1.0, ..Default::default() }; + brush.scattering.enabled = true; + brush.scattering.scatter.jitter = 6.0; + let stroke = Stroke { brush, points: vec![StrokePoint::new(10.0, 10.0, 1.0), StrokePoint::new(40.0, 10.0, 1.0)] }; + + // What the renderer really covered, once scattering has placed every dab. + let (bounds, cov) = super::coverage_within("t", &stroke, u64::MAX).unwrap(); + let rendered = u64::from(bounds.width()) * u64::from(bounds.height()); + assert_eq!(cov.len() as u64, rendered, "the dense buffer is exactly the rendered extent"); + + // Scattering reaches past the points plus half the tip, which is what the estimate alone sees. + let points_only = (40.0 - 10.0 + 24.0 + 8.0) * (24.0 + 8.0); + assert!(rendered as f64 > points_only, "scattering expanded the stroke: {rendered} vs {points_only}"); + + // A budget one pixel under what it rendered refuses it; exactly the rendered extent fits. + let err = super::coverage_within("t", &stroke, rendered - 1).unwrap_err().to_string(); + assert!(err.contains("stroke too large"), "{err}"); + assert!(super::coverage_within("t", &stroke, rendered).is_ok(), "the rendered extent itself fits"); +} + #[test] fn history_brush_reads_a_source_when_its_pixels_were_locked() { // Given a white layer locked in history state 1, then unlocked and filled red. diff --git a/crates/paint/src/retouch.rs b/crates/paint/src/retouch.rs index 9ef837c0c0..023e04b5ec 100644 --- a/crates/paint/src/retouch.rs +++ b/crates/paint/src/retouch.rs @@ -130,6 +130,24 @@ pub fn stroke_coverage(stroke: &Stroke) -> (Rect, Vec) { r.dense_coverage() } +/// [`stroke_coverage`] that admits the **rendered** extent before it is made dense: `None` when the +/// stroke covers more than `max_pixels`. +/// +/// Rendering itself is tiled, so it holds only the tiles the dabs touch. The bounds are read from +/// those tiles, after tips, dynamics and scattering have placed every dab, so this is the stroke's +/// real extent rather than an estimate from the input points. Only then is the `width × height` +/// buffer allocated, which is what callers need bounded. +pub fn stroke_coverage_within(stroke: &Stroke, max_pixels: u64) -> Option<(Rect, Vec)> { + let mut r = crate::StrokeRenderer::new(&stroke.brush, None, 1.0); + r.push(&stroke.points); + r.finish(); + let b = r.bounds(); + if u64::from(b.width()).saturating_mul(u64::from(b.height())) > max_pixels { + return None; + } + Some(r.dense_coverage()) +} + /// Index of the alpha channel, if the format has one. #[inline] pub fn alpha_index(fmt: &PixelFormat) -> Option { From 4258089716133d7776d02a618c5830c143555bde Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Thu, 15 Oct 2026 23:38:01 +0200 Subject: [PATCH 11/36] Replace Color: the sample eyedropper is armed when the dialog opens (#6279) Photoshop's first click on the image after opening Replace Color samples the colour. The dialog now opens (and resets) with its canvas eyedropper active; the pipette button still turns it off and on. --- crates/ui-egui/src/filter_dialog.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/filter_dialog.rs b/crates/ui-egui/src/filter_dialog.rs index fa7cb9524d..83944a1b03 100644 --- a/crates/ui-egui/src/filter_dialog.rs +++ b/crates/ui-egui/src/filter_dialog.rs @@ -388,6 +388,11 @@ fn apply_command_defaults(app: &PhotocraftApp, command: &str, fields: &mut Map Date: Thu, 15 Oct 2026 23:46:54 +0200 Subject: [PATCH 12/36] Smart filters: an edited filter's dialog previews again (#6324) Re-opening a smart filter dropped the dialog's Preview, since the live preview ran the filter command and would have stacked a second copy. The preview now runs layer.smartFilter.setParams on the proxy, as OK does, at full size and reduced afterwards (the smart object renders again from its source, which the proxy doesn't scale), so it shows the filter replaced in its stack. --- crates/ui-egui/src/canvas.rs | 10 ++++-- crates/ui-egui/src/filter_dialog.rs | 7 +++-- crates/ui-egui/src/smart_ui.rs | 48 +++++++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 391ad464fd..22568ba3f9 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1704,8 +1704,14 @@ pub(crate) fn ensure_filter_preview(app: &mut PhotocraftApp, idx: usize, ctx: &e return None; } let cmd = d.fields.get("__command")?.as_str()?.to_string(); - // Previews edit what the command will: a targeted layer mask included (#780). - let mut params = app.with_mask_target(&cmd, crate::filter_dialog::params_of(&d.fields)); + // Previews edit what the command will: a targeted layer mask included (#780), and a smart + // filter being edited is replaced in its stack rather than stacked again (#6324). + let (cmd, mut params) = if d.fields.contains_key("__smartFilter") { + crate::smart_ui::confirm_command(&d.fields, cmd, crate::filter_dialog::params_of(&d.fields)) + } else { + let params = app.with_mask_target(&cmd, crate::filter_dialog::params_of(&d.fields)); + (cmd, params) + }; // Select and Mask previews in its view mode; holding X shows the image without one. if crate::select_mask_view::owns(&d.fields) { if crate::select_mask_view::hides_view(&d.fields) { diff --git a/crates/ui-egui/src/filter_dialog.rs b/crates/ui-egui/src/filter_dialog.rs index fa7cb9524d..80bcd57d23 100644 --- a/crates/ui-egui/src/filter_dialog.rs +++ b/crates/ui-egui/src/filter_dialog.rs @@ -607,6 +607,8 @@ fn uses_angle_dial(command: &str, key: &str) -> bool { /// eyedropper like Photoshop's. While armed, the next click on the image writes the merged /// composite colour into the `color` field (the same sampler the Color Picker uses). const REPLACE_COLOR: &str = "image.adjustments.replaceColor"; +/// What OK on an edited smart filter runs (`smart_ui::confirm_command`); its preview too. +const SMART_FILTER_SET_PARAMS: &str = "layer.smartFilter.setParams"; const COLOR_EYEDROPPER: &str = "__colorEyedropper"; fn color_eyedropper_owns(f: &Map) -> bool { @@ -896,8 +898,9 @@ pub(crate) fn preview_document_for_target( return crate::select_mask_view::preview_document(doc, active, params, k, cancel); } // Warp Text renders the type again from the layer model (font size, position), which a - // reduced proxy doesn't scale: it runs at full size and the result is reduced (#218). - let (run_k, reduce_k) = if command == "type.warpText" { (1, k) } else { (k, 1) }; + // reduced proxy doesn't scale: it runs at full size and the result is reduced (#218). So does + // a smart filter being edited: the smart object renders again from its source (#6324). + let (run_k, reduce_k) = if command == "type.warpText" || command == SMART_FILTER_SET_PARAMS { (1, k) } else { (k, 1) }; let proxy = crate::proxy::proxy_document(doc, run_k); let mut s = photocraft_engine::Session::new(); s.set_inline_job_ctx(cancel.cloned()); diff --git a/crates/ui-egui/src/smart_ui.rs b/crates/ui-egui/src/smart_ui.rs index 7f49843c53..722e8fe910 100644 --- a/crates/ui-egui/src/smart_ui.rs +++ b/crates/ui-egui/src/smart_ui.rs @@ -314,8 +314,12 @@ pub fn open_editor(app: &mut PhotocraftApp, ctx: &egui::Context, layer: u64, sm: /// filter (`layer.smartFilter.setParams`, see [`confirm_command`]) instead of adding another. fn edit_filter(app: &mut PhotocraftApp, id: u64, layer: u64, index: usize, params: &Value) { if let Some(d) = app.ui.dialog_mut(id) { - // The live preview would stack a second copy of the filter on the smart object. - d.fields.remove("__preview"); + // A filter's live preview runs `layer.smartFilter.setParams` on the proxy, as OK does + // (`canvas::ensure_filter_preview`, #6324). An adjustment's previews on the layer and + // would stack a second copy of it on the smart object. + if d.fields.contains_key("__adjust") { + d.fields.remove("__preview"); + } d.fields.insert("__smartFilter".into(), json!({"layer": layer, "index": index})); if let Value::Object(p) = params { for (k, v) in p { @@ -452,4 +456,44 @@ mod tests { assert_eq!(sm.smart_filters[0].params.get("brightness").and_then(Value::as_f64), Some(40.0)); assert_eq!(sm.smart_filters[0].params.get("contrast").and_then(Value::as_f64), Some(-10.0)); } + + /// #6324: a smart filter opened for editing previews, and its preview replaces the filter in + /// the stack (as OK does) instead of stacking a second one. + #[test] + fn an_edited_smart_filter_previews_what_ok_gives() { + let mut s = photocraft_engine::Session::new(); + s.execute("file.new", json!({"width": 64, "height": 48, "background": "#ffffff"})).unwrap(); + s.execute("layer.new.layer", json!({})).unwrap(); + s.execute("select.all", json!({})).unwrap(); + s.execute("edit.stroke", json!({"width": 4, "color": "#000000", "location": "inside"})).unwrap(); + s.execute("select.deselect", json!({})).unwrap(); + let id = s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap()["layer"].as_u64().unwrap(); + s.execute("filter.blur.gaussianBlur", json!({"radius": 1.0})).unwrap(); + let mut app = PhotocraftApp::new(s, crate::Services::default()); + app.background_jobs = false; + let smart = |app: &PhotocraftApp, doc: &Document| match &doc.layer(LayerId(id)).unwrap().content { + LayerContent::Smart(sm) => sm.clone(), + other => panic!("expected a smart object, got {other:?} ({})", app.ui.status), + }; + let sm = smart(&app, &app.session.active().unwrap().doc); + open_editor(&mut app, &egui::Context::default(), id, &sm, 0); + let d = app.ui.dialogs.last_mut().expect("the filter's dialog opened"); + assert_eq!(d.fields.get("__preview"), Some(&json!(true)), "the dialog previews"); + d.fields.insert("radius".into(), json!(6.0)); + let dialog = d.id; + + crate::canvas::ensure_filter_preview(&mut app, 0, &egui::Context::default()).expect("a preview"); + let preview = app.filter_preview.as_ref().unwrap(); + assert_eq!(preview.key.command, "layer.smartFilter.setParams"); + let shown = preview.result.clone().expect("the preview's document"); + let filters = smart(&app, &shown).smart_filters; + assert_eq!(filters.len(), 1, "the edited filter is replaced, not stacked"); + assert_eq!(filters[0].params.get("radius").and_then(Value::as_f64), Some(6.0)); + assert_eq!(smart(&app, &app.session.active().unwrap().doc).smart_filters[0].params["radius"], json!(1.0), "the document is untouched"); + + crate::dialogs::confirm(&mut app, dialog).unwrap(); + let doc = &app.session.active().unwrap().doc; + assert_eq!(smart(&app, doc).smart_filters.len(), 1); + assert!(photocraft_compose::flatten(&shown).px == photocraft_compose::flatten(doc).px, "OK gives what the preview showed"); + } } From f71eb28f844f8d47c73edf88227137a499cc5088 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Thu, 15 Oct 2026 23:55:58 +0200 Subject: [PATCH 13/36] Replace Color: a Result swatch whose Color Picker sets the sliders (#6278) Photoshop's Replace Color shows the replacement colour under the Hue, Saturation and Lightness sliders; picking one in its Color Picker sets the sliders so the sampled colour becomes it. tone::replace_color_result gives the swatch (the sampled colour after the shift) and tone::replace_color_shift the slider values for a picked colour. --- crates/algo/src/tone.rs | 51 +++++++++++++++++ crates/ui-egui/src/filter_dialog.rs | 85 +++++++++++++++++++++++++++++ crates/ui-egui/src/i18n/ko.tsv | 1 + crates/ui-egui/src/i18n/zh-hans.tsv | 1 + crates/ui-egui/src/i18n/zh-hant.tsv | 1 + 5 files changed, 139 insertions(+) diff --git a/crates/algo/src/tone.rs b/crates/algo/src/tone.rs index 2632d4a29a..5a0f752fac 100644 --- a/crates/algo/src/tone.rs +++ b/crates/algo/src/tone.rs @@ -65,6 +65,35 @@ pub fn replace_color(px: &mut [[f32; 4]], color: [f32; 3], fuzziness: f32, hue: mask } +/// Replace Color's Result swatch: `color` after the hue (±180°), saturation and lightness (±100) +/// shift, as a fully matched pixel of that colour becomes. +pub fn replace_color_result(color: [f32; 3], hue: f32, saturation: f32, lightness: f32) -> [f32; 3] { + let mut px = [[color[0], color[1], color[2], 1.0]]; + replace_color_masked(&mut px, &[1.0], hue, saturation, lightness); + [px[0][0], px[0][1], px[0][2]] +} + +/// The hue (±180°), saturation and lightness (±100) shift that turns `from` into `to`, as +/// Photoshop's Result Color Picker sets the sliders. A shift the sliders can't reach (a grey has +/// no hue to turn, black or white no saturation to give) goes as far as they do. +pub fn replace_color_shift(from: [f32; 3], to: [f32; 3]) -> (f32, f32, f32) { + let [fh, fs, fl] = rgb_to_hsl(from); + let [th, ts, tl] = rgb_to_hsl(to); + // Hue turns the short way round; a grey target keeps the hue where it is. + let hue = if ts <= 0.0 { 0.0 } else { ((th - fh + 0.5).rem_euclid(1.0) - 0.5) * 360.0 }; + // Inverse of the shift in `replace_color_masked`: up moves toward 1, down toward 0. + let amount = |from: f32, to: f32| { + if to >= from { + if from >= 1.0 { 0.0 } else { (to - from) / (1.0 - from) * 100.0 } + } else if from <= 0.0 { + 0.0 + } else { + (to / from - 1.0) * 100.0 + } + }; + (hue.clamp(-180.0, 180.0), amount(fs, ts).clamp(-100.0, 100.0), amount(fl, tl).clamp(-100.0, 100.0)) +} + /// Replace Color's hue, saturation and lightness shift, applied to each pixel as far as `mask` /// (`0..=1`, one per pixel: the colour match, localized or not) covers it. pub fn replace_color_masked(px: &mut [[f32; 4]], mask: &[f32], hue: f32, saturation: f32, lightness: f32) { @@ -277,6 +306,28 @@ pub fn hdr_toning(px: &mut [[f32; 4]], w: usize, h: usize, p: &HdrToning) { mod tests { use super::*; + /// #6278: the Result swatch is the sampled colour after the shift, and a picked Result sets + /// the sliders that give it. + #[test] + fn replace_color_result_and_shift_round_trip() { + let from = [0.8, 0.2, 0.2]; + let near = |a: [f32; 3], b: [f32; 3]| a.iter().zip(b).all(|(x, y)| (x - y).abs() < 2e-3); + assert!(near(replace_color_result(from, 0.0, 0.0, 0.0), from), "no shift, no change"); + for to in [[0.2, 0.3, 0.9], [0.9, 0.85, 0.1], [0.4, 0.1, 0.1], [0.95, 0.6, 0.6], [0.1, 0.6, 0.3]] { + let (h, s, l) = replace_color_shift(from, to); + assert!((-180.0..=180.0).contains(&h) && (-100.0..=100.0).contains(&s) && (-100.0..=100.0).contains(&l)); + assert!(near(replace_color_result(from, h, s, l), to), "{to:?}: ({h}, {s}, {l}) gives {:?}", replace_color_result(from, h, s, l)); + } + // Red to blue turns the short way (−120°, not +240°). + assert!((replace_color_shift([1.0, 0.0, 0.0], [0.0, 0.0, 1.0]).0 + 120.0).abs() < 0.5); + // A grey target: saturation all the way down, the hue left alone. + let (h, s, _) = replace_color_shift(from, [0.5, 0.5, 0.5]); + assert_eq!((h, s), (0.0, -100.0)); + // Black has no saturation to raise: the slider stays put rather than dividing by zero. + let (h, s, l) = replace_color_shift([0.0, 0.0, 0.0], [0.2, 0.6, 0.3]); + assert!(h.is_finite() && s.is_finite() && l > 0.0); + } + fn ramp(w: usize, h: usize) -> Vec<[f32; 4]> { (0..w * h) .map(|i| { diff --git a/crates/ui-egui/src/filter_dialog.rs b/crates/ui-egui/src/filter_dialog.rs index fa7cb9524d..f2212efa6c 100644 --- a/crates/ui-egui/src/filter_dialog.rs +++ b/crates/ui-egui/src/filter_dialog.rs @@ -637,6 +637,49 @@ pub(crate) fn color_eyedropper_sample(app: &mut PhotocraftApp, x: f64, y: f64) - true } +/// The Color Picker of Replace Color's Result swatch writes here (#6278). +const RESULT_PICKED: &str = "__resultPicked"; +/// The Result the picker opened on: a picked colour that differs from it sets the sliders. +const RESULT_FROM: &str = "__resultFrom"; + +/// Replace Color's Result swatch (#6278), as in Photoshop: the sampled colour after the Hue, +/// Saturation and Lightness shift. Picking a Result in its Color Picker sets the three sliders so +/// the sampled colour becomes it. +fn replace_color_result(ui: &mut egui::Ui, f: &mut Map) { + use photocraft_algo::tone; + let t = Tokens::get(ui.ctx()); + let from = f.get("color").and_then(Value::as_str).and_then(crate::color_picker_ui::parse_hex).unwrap_or([0.0; 3]); + let picked = f.get(RESULT_PICKED).and_then(Value::as_str).and_then(crate::color_picker_ui::parse_hex); + if let Some(to) = picked + && f.get(RESULT_PICKED) != f.get(RESULT_FROM) + { + let (hue, saturation, lightness) = tone::replace_color_shift(from, to); + for (key, v) in [("hue", hue), ("saturation", saturation), ("lightness", lightness)] { + f.insert(key.into(), json!(v.round())); + } + f.remove(RESULT_PICKED); + f.remove(RESULT_FROM); + } + let num = |key: &str| f.get(key).and_then(Value::as_f64).unwrap_or(0.0) as f32; + let rgb = tone::replace_color_result(from, num("hue"), num("saturation"), num("lightness")); + let bytes = rgb.map(|v| (v.clamp(0.0, 1.0) * 255.0).round() as u8); + let name = tl!("Result"); + let mut clicked = false; + ui.horizontal(|ui| { + ui.label(egui::RichText::new(name).color(t.text_dim)); + ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { + clicked = crate::widgets::color_swatch_button(ui, egui::Color32::from_rgb(bytes[0], bytes[1], bytes[2]), name).clicked(); + ui.label(egui::RichText::new(crate::color_picker_ui::hex(rgb)).color(t.text_dim).monospace()); + }); + }); + if clicked { + let hex = json!(crate::color_picker_ui::hex(rgb)); + f.insert(RESULT_PICKED.into(), hex.clone()); + f.insert(RESULT_FROM.into(), hex); + crate::color_picker_ui::request_field(f, RESULT_PICKED, &crate::color_picker_ui::title_for(name)); + } +} + /// Dialog body for filter commands. pub fn body(ui: &mut egui::Ui, f: &mut Map) { let t = Tokens::get(ui.ctx()); @@ -832,6 +875,9 @@ pub fn body(ui: &mut egui::Ui, f: &mut Map) { } } } + if cmd == REPLACE_COLOR { + replace_color_result(ui, f); + } // Read-only context the dialog opener supplies (e.g. the monitor profile in use). if let Some(note) = f.get("__note").and_then(Value::as_str) { ui.add_space(4.0); @@ -2383,6 +2429,45 @@ mod tests { assert!(!color_eyedropper_armed(h.state())); } + /// #6278: the Result swatch shows the sampled colour after the shift, and a colour picked for + /// it sets Hue, Saturation and Lightness so the sampled colour becomes it. + #[test] + fn replace_color_result_swatch_sets_the_sliders() { + use egui_kittest::Harness; + use egui_kittest::kittest::Queryable; + + let mut s = photocraft_engine::Session::new(); + s.execute("file.new", json!({"width": 16, "height": 16})).unwrap(); + let mut app = PhotocraftApp::new(s, crate::Services::default()); + app.run("tools.setColors", json!({"foreground": "#cc3333"})).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(900.0, 900.0)).build_ui_state(|ui, app| crate::dialogs::show(app, ui.ctx()), &mut app); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ALL[0]); + let id = open(h.state_mut(), REPLACE_COLOR).unwrap(); + h.run_steps(3); + // No shift yet: the Result is the sampled colour. + assert_eq!(h.get_all_by_label("#cc3333").count(), 2, "the sampled colour and the Result both read #cc3333"); + h.get_by_role_and_label(egui::accesskit::Role::ColorWell, "Result").click(); + h.run_steps(3); + let picker = h.state().ui.dialogs.last().unwrap(); + assert_ne!(picker.id, id, "the Color Picker opened over the dialog"); + assert_eq!(picker.fields["color"], json!("#cc3333"), "on the current Result"); + let picker = picker.id; + h.state_mut().ui.dialog_mut(picker).unwrap().fields.insert("color".into(), json!("#3355cc")); + crate::dialogs::confirm(h.state_mut(), picker).unwrap(); + h.state_mut().ui.dialogs.retain(|d| d.id != picker); + h.run_steps(3); + let f = h.state_mut().ui.dialog_mut(id).unwrap().fields.clone(); + let n = |k: &str| f[k].as_f64().unwrap() as f32; + let got = photocraft_algo::tone::replace_color_result([0.8, 0.2, 0.2], n("hue"), n("saturation"), n("lightness")); + let want = crate::color_picker_ui::parse_hex("#3355cc").unwrap(); + assert!(got.iter().zip(want).all(|(a, b)| (a - b).abs() < 0.02), "the sliders turn the sampled colour into the pick: {got:?}"); + assert!(n("hue") != 0.0, "the hue moved"); + // Moving a slider afterwards is not mistaken for another pick. + h.state_mut().ui.dialog_mut(id).unwrap().fields.insert("lightness".into(), json!(10.0)); + h.run_steps(2); + assert_eq!(h.state_mut().ui.dialog_mut(id).unwrap().fields["lightness"], json!(10.0)); + } + /// #4457: Localized Color Clusters and its Range, as in Photoshop's dialog; the sampled point /// and both settings reach the command. #[test] diff --git a/crates/ui-egui/src/i18n/ko.tsv b/crates/ui-egui/src/i18n/ko.tsv index 2e12c33799..723d048568 100644 --- a/crates/ui-egui/src/i18n/ko.tsv +++ b/crates/ui-egui/src/i18n/ko.tsv @@ -3374,3 +3374,4 @@ curves Smooth 매끄럽게 Take the shortcut from the other command and select it 다른 명령에서 단축키를 빼고 그 명령 선택 Undo Changes 변경 사항 취소 {shortcut} is reserved by the system and can't be assigned. {shortcut}은(는) 시스템에서 예약되어 할당할 수 없습니다. + Result 결과 diff --git a/crates/ui-egui/src/i18n/zh-hans.tsv b/crates/ui-egui/src/i18n/zh-hans.tsv index cbf3c9d08f..f02dfc1ff7 100644 --- a/crates/ui-egui/src/i18n/zh-hans.tsv +++ b/crates/ui-egui/src/i18n/zh-hans.tsv @@ -2812,3 +2812,4 @@ curves Smooth 平滑 Take the shortcut from the other command and select it 从另一个命令移除该快捷键并选中它 Undo Changes 撤消更改 {shortcut} is reserved by the system and can't be assigned. {shortcut} 由系统保留，无法指定。 + Result 结果 diff --git a/crates/ui-egui/src/i18n/zh-hant.tsv b/crates/ui-egui/src/i18n/zh-hant.tsv index 30aa3f5a94..3d82e5b4d3 100644 --- a/crates/ui-egui/src/i18n/zh-hant.tsv +++ b/crates/ui-egui/src/i18n/zh-hant.tsv @@ -2797,3 +2797,4 @@ curves Smooth 平滑 Take the shortcut from the other command and select it 從另一個指令移除該快速鍵並選取它 Undo Changes 還原變更 {shortcut} is reserved by the system and can't be assigned. {shortcut} 由系統保留，無法指定。 + Result 結果 From 8369b4b55cfb1cffd04c02fc8150c19174115435 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:04:50 +0200 Subject: [PATCH 14/36] =?UTF-8?q?File=20=E2=80=BA=20Open:=20a=20file=20typ?= =?UTF-8?q?e=20filter=20per=20format=20(#6337)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After All Formats the Open dialog listed only PhotoCraft, OpenRaster, Krita, SVG and Paint.NET. Every format it reads now has its own filter (Photoshop, Affinity, HEIF, PNG, JPEG, TIFF, Portable Bitmap, OpenEXR, Radiance HDR, camera raw, …), and together they cover All Formats exactly. --- apps/photocraft/src/services.rs | 74 +++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 13 deletions(-) diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index 9de1c475ea..b65a4b69e1 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -36,16 +36,47 @@ fn open_filter_extensions(extensions: &[&str]) -> Vec { extensions.iter().map(|ext| if case_sensitive_globs { ext.chars().map(class).collect() } else { ext.to_string() }).collect() } -/// An explicit SVG filter keeps vector artwork discoverable even when a native picker defaults to -/// the PhotoCraft-only filter. SVG is also included in All Formats for normal multi-format opens. +/// File › Open's file types after All Formats, as Photoshop lists them: one per format, so a folder +/// of mixed files can be narrowed to one kind (#6337). Together they cover every extension of +/// [`OPEN_EXTS`]. An explicit SVG filter also keeps vector artwork discoverable when a native +/// picker defaults to another filter. +const OPEN_FILTERS: &[(&str, &[&str])] = &[ + ("PhotoCraft", &["pcraft"]), + ("Photoshop", &["psd", "psb", "psdt"]), + ("Affinity", &["af", "afdesign", "afphoto", "afpub"]), + ("Krita", &["kra", "krz"]), + ("OpenRaster", &["ora"]), + ("Paint.NET", &["pdn"]), + ("SVG", SVG_EXTS), + ("PNG", &["png"]), + ("JPEG", &["jpg", "jpeg"]), + ("HEIF", &["heic", "heif", "hif"]), + ("TIFF", &["tif", "tiff"]), + ("WebP", &["webp"]), + ("GIF", &["gif"]), + ("BMP", &["bmp"]), + ("Targa", &["tga"]), + ("ICO", &["ico"]), + ("QOI", &["qoi"]), + ("PCX", &["pcx"]), + ("DDS", &["dds"]), + ("XPM", &["xpm"]), + ("Portable Bitmap", &["pbm", "pgm", "ppm", "pam", "pfm"]), + ("OpenEXR", &["exr"]), + ("Radiance HDR", &["hdr"]), + ("FITS", &["fits", "fit", "fts"]), + ("DPX and Cineon", &["dpx", "cin"]), + ("Camera Raw", &["dng", "cr2", "cr3", "nef", "nrw", "arw", "pef", "orf", "rw2", "raf"]), + ("Brushes", &["abr"]), + ("Gradients", &["grd"]), + ("Swatches", &["aco", "ase"]), + ("Keyboard Shortcuts", &["kys"]), +]; + fn open_filters() -> Vec<(&'static str, Vec)> { - vec![ - ("All Formats", open_filter_extensions(OPEN_EXTS)), - ("PhotoCraft", open_filter_extensions(&["pcraft"])), - ("OpenRaster", open_filter_extensions(&["ora"])), - ("Krita", open_filter_extensions(&["kra", "krz"])), - ("SVG", open_filter_extensions(SVG_EXTS)), - ] + std::iter::once(("All Formats", open_filter_extensions(OPEN_EXTS))) + .chain(OPEN_FILTERS.iter().map(|(name, exts)| (*name, open_filter_extensions(exts)))) + .collect() } /// File › Save As formats: (filter name, extensions). The filter matching the suggested name's @@ -127,10 +158,7 @@ fn show_file_dialog(request: FileDialogRequest, parent: Option<&eframe::Frame>, let dialog = if let Some(exts) = extensions { dialog.add_filter("Supported Files", &open_filter_extensions(&exts.iter().map(String::as_str).collect::>())) } else { - open_filters() - .into_iter() - .fold(dialog, |dialog, (name, extensions)| dialog.add_filter(name, &extensions)) - .add_filter("Paint.NET", &open_filter_extensions(&["pdn"])) + open_filters().into_iter().fold(dialog, |dialog, (name, extensions)| dialog.add_filter(name, &extensions)) }; if multiple { let picked = dialog.pick_files(); @@ -619,6 +647,26 @@ mod tests { assert!(svg_filter.iter().any(|glob| matches(glob, "svg")) && svg_filter.iter().any(|glob| matches(glob, "svgz"))); } + /// #6337: after All Formats, every format File › Open reads has its own filter, and no filter + /// offers an extension All Formats doesn't. + #[test] + fn every_open_format_has_its_own_filter() { + let filters = open_filters(); + assert_eq!(filters[0], ("All Formats", open_filter_extensions(OPEN_EXTS)), "All Formats comes first"); + let mut named: Vec<&str> = OPEN_FILTERS.iter().flat_map(|(_, exts)| exts.iter().copied()).collect(); + let total = named.len(); + named.sort_unstable(); + named.dedup(); + assert_eq!(named.len(), total, "an extension is listed under two filters"); + let mut all = OPEN_EXTS.to_vec(); + all.sort_unstable(); + assert_eq!(named, all, "the named filters cover All Formats exactly"); + let names: Vec<&str> = filters.iter().map(|(name, _)| *name).collect(); + for name in ["Photoshop", "Affinity", "HEIF", "PNG", "JPEG", "TIFF", "Portable Bitmap", "OpenEXR", "Radiance HDR", "Paint.NET"] { + assert!(names.contains(&name), "{name}"); + } + } + #[cfg(not(all(unix, not(target_os = "macos"))))] #[test] fn open_filters_keep_literal_extensions_on_windows_and_macos() { From 8176b613ca2a95cb8afc6a535bd4ae8ff532c64f Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:15:50 +0200 Subject: [PATCH 15/36] Smart filters: the dialog's preview window shows the edited filter too (#6324) The filter dialog's own preview window matched the canvas preview by the filter's command, but an edited smart filter previews as layer.smartFilter.setParams, so the window kept the unfiltered image. smart_ui::preview_command names the command the preview runs. --- crates/ui-egui/src/filter_preview_ui.rs | 4 +++- crates/ui-egui/src/smart_ui.rs | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/filter_preview_ui.rs b/crates/ui-egui/src/filter_preview_ui.rs index 90eaf4f6fe..66499ab9f0 100644 --- a/crates/ui-egui/src/filter_preview_ui.rs +++ b/crates/ui-egui/src/filter_preview_ui.rs @@ -43,7 +43,9 @@ pub(crate) fn window(app: &PhotocraftApp, dialog: u64, ui: &mut egui::Ui, f: &mu // decides whether the filtered result (rather than the original) is shown. let preview_on = f.get("__preview").and_then(Value::as_bool) == Some(true); let filtered: Option<(FilterPreviewKey, std::sync::Arc)> = app.filter_preview.as_ref().and_then(|p| { - (preview_on && p.key.dialog == dialog && p.key.doc == doc_id && p.key.command == cmd).then(|| p.result.clone().map(|r| (p.key.clone(), r))).flatten() + // An edited smart filter previews as `layer.smartFilter.setParams` (#6324). + let previewed = crate::smart_ui::preview_command(f, &cmd); + (preview_on && p.key.dialog == dialog && p.key.doc == doc_id && p.key.command == previewed).then(|| p.result.clone().map(|r| (p.key.clone(), r))).flatten() }); // The proxy factor the preview runs at (the filtered key's when it exists, else the request's). let base_k = filtered.as_ref().map(|(k, _)| k.k).unwrap_or_else(|| { diff --git a/crates/ui-egui/src/smart_ui.rs b/crates/ui-egui/src/smart_ui.rs index 722e8fe910..2acc77ceaf 100644 --- a/crates/ui-egui/src/smart_ui.rs +++ b/crates/ui-egui/src/smart_ui.rs @@ -331,6 +331,12 @@ fn edit_filter(app: &mut PhotocraftApp, id: u64, layer: u64, index: usize, param } } +/// The command a filter dialog's live preview runs ([`confirm_command`]'s): `layer.smartFilter.setParams` +/// while it edits a smart filter (#6324), else `command`. +pub fn preview_command<'a>(fields: &serde_json::Map, command: &'a str) -> &'a str { + if fields.contains_key("__smartFilter") { "layer.smartFilter.setParams" } else { command } +} + /// The command a confirmed filter dialog runs: smart-filter edits update the recorded filter. pub fn confirm_command(fields: &serde_json::Map, command: String, params: Value) -> (String, Value) { match fields.get("__smartFilter") { @@ -485,6 +491,10 @@ mod tests { crate::canvas::ensure_filter_preview(&mut app, 0, &egui::Context::default()).expect("a preview"); let preview = app.filter_preview.as_ref().unwrap(); assert_eq!(preview.key.command, "layer.smartFilter.setParams"); + // The dialog's own preview window matches it to the dialog by that command. + let fields = &app.ui.dialogs.last().unwrap().fields; + assert_eq!(preview_command(fields, "filter.blur.gaussianBlur"), preview.key.command); + assert_eq!(preview_command(&serde_json::Map::new(), "filter.blur.gaussianBlur"), "filter.blur.gaussianBlur"); let shown = preview.result.clone().expect("the preview's document"); let filters = smart(&app, &shown).smart_filters; assert_eq!(filters.len(), 1, "the edited filter is replaced, not stacked"); From c04da11bb3986a8da42642dbbcf272473e0bdfbe Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:16:06 +0200 Subject: [PATCH 16/36] Smart filters: rustfmt --- crates/ui-egui/src/filter_preview_ui.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/filter_preview_ui.rs b/crates/ui-egui/src/filter_preview_ui.rs index 66499ab9f0..859a829568 100644 --- a/crates/ui-egui/src/filter_preview_ui.rs +++ b/crates/ui-egui/src/filter_preview_ui.rs @@ -45,7 +45,9 @@ pub(crate) fn window(app: &PhotocraftApp, dialog: u64, ui: &mut egui::Ui, f: &mu let filtered: Option<(FilterPreviewKey, std::sync::Arc)> = app.filter_preview.as_ref().and_then(|p| { // An edited smart filter previews as `layer.smartFilter.setParams` (#6324). let previewed = crate::smart_ui::preview_command(f, &cmd); - (preview_on && p.key.dialog == dialog && p.key.doc == doc_id && p.key.command == previewed).then(|| p.result.clone().map(|r| (p.key.clone(), r))).flatten() + (preview_on && p.key.dialog == dialog && p.key.doc == doc_id && p.key.command == previewed) + .then(|| p.result.clone().map(|r| (p.key.clone(), r))) + .flatten() }); // The proxy factor the preview runs at (the filtered key's when it exists, else the request's). let base_k = filtered.as_ref().map(|(k, _)| k.k).unwrap_or_else(|| { From 944d0cf722e504594bc0433454be6d73a304e675 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Thu, 15 Oct 2026 18:27:56 -0400 Subject: [PATCH 17/36] Path Selection: the canvas right-click opens the path menu, as the Pen's does (#1056) The Path Selection and Direct Selection tools had no canvas menu, so Make Selection, Fill and Stroke Path, Free Transform Path, Delete Path and the combine and style rows needed the Pen tool or the Paths panel. Like Photoshop, their right-click now opens the path menu for the selected path, with the Pen menu's rows and the same enablement. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas_tool_menu.rs | 44 ++++++++++++++++++++++---- docs/context-menu-parity.md | 4 +-- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/canvas_tool_menu.rs b/crates/ui-egui/src/canvas_tool_menu.rs index da835a786f..a02859c5cd 100644 --- a/crates/ui-egui/src/canvas_tool_menu.rs +++ b/crates/ui-egui/src/canvas_tool_menu.rs @@ -284,8 +284,14 @@ pub fn applies(tool: Tool) -> bool { | Tool::MagneticLasso | Tool::MagicWand | Tool::ObjectSelection - | Tool::Pen - ) + ) || path_menu(tool) +} + +/// Tools whose canvas right-click opens the path menu ([`PEN_MENU`]): the Pen, and the Path +/// Selection and Direct Selection tools, whose Photoshop menu offers the same whole-path actions +/// for the selected path (#1056). +pub fn path_menu(tool: Tool) -> bool { + matches!(tool, Tool::Pen | Tool::PathSelection | Tool::DirectSelection) } /// Tools whose plain canvas right-click offers the view menu. @@ -310,7 +316,7 @@ pub fn rows(menu: &CanvasToolMenu) -> &'static [Row] { VIEW_MENU } else if menu.text { TYPE_EDIT_MENU - } else if menu.tool == Tool::Pen { + } else if path_menu(menu.tool) { PEN_MENU } else { selection_rows(menu.has_selection) @@ -471,7 +477,7 @@ pub fn pasteboard_at(app: &PhotocraftApp, tool: Tool, doc: [f64; 2]) -> bool { let Some(st) = app.session.active() else { return false }; let (w, h) = (f64::from(st.doc.size.width), f64::from(st.doc.size.height)); let outside = doc.iter().all(|v| v.is_finite()) && (doc[0] < 0.0 || doc[1] < 0.0 || doc[0] >= w || doc[1] >= h); - outside && tool != Tool::Pen && !crate::paint_mouse::has_brush_picker(tool) && !crate::paint_mouse::right_erases(app, tool) && app.ui.text_edit.is_none() + outside && !path_menu(tool) && !crate::paint_mouse::has_brush_picker(tool) && !crate::paint_mouse::right_erases(app, tool) && app.ui.text_edit.is_none() } /// Right-click on the pasteboard: its colour menu at screen point `pos`. @@ -572,7 +578,7 @@ pub fn entry_enabled(app: &PhotocraftApp, menu: &CanvasToolMenu, command: &str) c => TYPE_EDIT_MENU.iter().flatten().any(|(_, id)| *id == c) && crate::menus::is_enabled(app, c), }; } - if menu.transform || menu.tool != Tool::Pen { + if menu.transform || !path_menu(menu.tool) { return crate::menus::is_enabled(app, command); } let Some(st) = app.session.active() else { return false }; @@ -699,7 +705,7 @@ pub fn choose(app: &mut PhotocraftApp, ctx: &Context, command: &str) { choose_spelling(app, &menu, command) } else if menu.text { choose_type(app, ctx, command) - } else if menu.tool == Tool::Pen && !menu.transform { + } else if path_menu(menu.tool) && !menu.transform { choose_pen(app, ctx, &menu, command) } else if command == "select.toWorkPath" { // Photoshop's Make Work Path… asks for the tolerance first. @@ -1492,6 +1498,32 @@ mod tests { assert!(app.session.active().unwrap().doc.selection.is_some()); } + /// #1056: the Path Selection and Direct Selection tools' right-click opens the path menu for + /// the selected path, as the Pen's does, instead of nothing. + #[test] + fn path_selection_tools_open_the_path_menu() { + for tool in [Tool::PathSelection, Tool::DirectSelection] { + let mut app = app(); + app.ui.tool = Tool::Pen; + for (x, y) in [(2.0, 2.0), (20.0, 2.0), (20.0, 20.0)] { + crate::vector_ui::pen_down(&mut app, x, y, false); + crate::vector_ui::pen_up(&mut app); + } + crate::vector_ui::pen_commit(&mut app, false); + assert!(app.session.active().unwrap().doc.work_path.is_some()); + app.ui.tool = tool; + assert!(applies(tool)); + assert!(open(&mut app, &Context::default(), tool, [10.0, 10.0]), "{tool:?}"); + let menu = app.ui.canvas_tool_menu.clone().unwrap(); + assert_eq!(rows(&menu), PEN_MENU, "{tool:?}"); + assert!(menu.has_path && entry_enabled(&app, &menu, "path.toSelection"), "{tool:?}"); + choose(&mut app, &Context::default(), "path.toSelection"); + let dialog = app.ui.dialogs.last().map(|d| d.id).unwrap(); + crate::dialogs::confirm(&mut app, dialog).unwrap(); + assert!(app.session.active().unwrap().doc.selection.is_some(), "{tool:?}: Make Selection made one"); + } + } + #[test] fn pen_menu_has_all_reference_rows_in_order_and_guards_unavailable_actions() { let labels: Vec<_> = PEN_MENU.iter().map(|row| row.map(|(label, _)| label)).collect(); diff --git a/docs/context-menu-parity.md b/docs/context-menu-parity.md index 76d6724b7e..1e8abf9a67 100644 --- a/docs/context-menu-parity.md +++ b/docs/context-menu-parity.md @@ -1,6 +1,6 @@ # Context-menu parity contract -> **Last reviewed:** 2026-10-13 · **Last updated:** 2026-10-13 · **Change:** minor (layer-mask thumbnail menu, #4575) · **Target:** Adobe Photoshop 2026 +> **Last reviewed:** 2026-10-13 · **Last updated:** 2026-10-15 · **Change:** minor (Path Selection canvas menu, #1056) · **Target:** Adobe Photoshop 2026 Status: implementation contract, updated 2026-10-09. Scope: canvas, document tabs, and panels. “Right-click” means a secondary pointer click; macOS Control-click should also open the same menu where the platform sends a context-menu gesture. Photoshop changes a context menu with the active tool, selection, and panel item. Adobe documents that principle but does not publish a complete current menu catalog. **Documented** means an Adobe source explicitly names the gesture or action. **Inferred** means a PhotoCraft design target or an item known from a main menu whose presence in a Photoshop context menu still needs a current-product capture. Do not present inferred rows as exact Photoshop menus. [Adobe: panels and menus](https://helpx.adobe.com/photoshop/using/panels-menus.html), [Adobe: shortcuts and context menus](https://helpx.adobe.com/sa_en/photoshop/using/customizing-keyboard-shortcuts.html). @@ -29,7 +29,7 @@ The “PhotoCraft at branch point” column records behavior before this PR. Pic | Gradient, Paint Bucket | Gradient preset/type/mode or fill source/tolerance **(inferred context placement)** | No tool menu | P1 | | Type | In text: cut/copy/paste, spelling suggestions, text formatting; outside text: type-layer action **(inferred context placement)** | While editing text (canvas_tool_menu.rs `TYPE_EDIT_MENU`): Undo, Cut, Copy, Paste, Select All (on the text), Check Spelling…, Find and Replace Text…, Faux Bold/Italic (checked), Rasterize Type, Create Work Path, Convert to Shape, Horizontal/Vertical and the anti-alias modes (checked), Convert to Paragraph/Point Text, Warp Text…; Esc closes only the menu. Over a misspelled word the menu starts with up to 3 spelling suggestions (replace the word) and Add to Dictionary (#4370); Ignore / Ignore All and the non-editing type-layer menu remain open | P1; separate editing and nonediting states; [Adobe type editing](https://helpx.adobe.com/photoshop/desktop/text-typography/get-started-with-text/edit-text.html) | | Hand, Zoom | Fit on Screen, 100%, 200%, Print Size; zoom step choices **(inferred context placement)** | View menu (#3242): Zoom In, Zoom Out, Fit on Screen, Fill Screen, 100%, 200%, Print Size. Zoom In/Out step about the right-clicked point (Preferences › Tools › Zoom Clicked Point to Center centres it). Outside the image the pasteboard menu still wins. | P1; view actions stay out of document history; row order still needs a dated Photoshop capture | -| Pen, Path Selection | Path/point actions, Make Selection, Fill/Stroke Path, Free Transform Path **(inferred context placement)** | Pen canvas right-click follows the user-provided 20-row Photoshop capture below. Paths rows offer Make Selection, Fill and Stroke. Path Selection's own canvas menu remains open. | P1; distinguish node, segment, path, and blank canvas | +| Pen, Path Selection | Path/point actions, Make Selection, Fill/Stroke Path, Free Transform Path **(inferred context placement)** | Pen canvas right-click follows the user-provided 20-row Photoshop capture below. Paths rows offer Make Selection, Fill and Stroke. The Path Selection and Direct Selection tools open the same path menu for the selected path (#1056, 2026-10-15); node- and segment-specific rows remain open. | P1; distinguish node, segment, path, and blank canvas | | Rectangle, Ellipse Shape, Triangle, Polygon, Line, Custom Shape | Shape/path operations, fill/stroke settings, transform **(inferred context placement)** | No tool menu | P2 | | Slice, Slice Select | Slice properties, duplicate/delete, divide, guides **(inferred context placement)** | No tool menu | P2 | | Any tool while Free Transform is active | Scale, Rotate, Skew, Distort, Perspective, Warp, rotate/flip variants, commit/cancel **(inferred exact context placement)** | Missing per docs/scorecard.md TOOL-214-5 | P0; transform operations [documented by Adobe](https://helpx.adobe.com/photoshop/desktop/crop-resize-transform/transform-manipulate-reshape/adjust-scale-rotation-and-perspective.html); type restrictions [documented here](https://helpx.adobe.com/photoshop/using/creating-type.html) | From a76d3295ba2b2d22d94f87fe2a2d3a5205cdc041 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:38:48 +0200 Subject: [PATCH 18/36] Type: the text caret shows on a white background (part of #6085) In the Photoshop themes the caret was a 1.5 pt white line with a 0.75 pt black hairline inside, so on a white background only a faint grey line showed. It is now a solid 1 pt black line inside a 3 pt white halo, readable on any background, in every theme. --- crates/ui-egui/src/type_tool.rs | 11 +++++-- crates/ui-egui/src/type_tool_tests.rs | 43 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/type_tool.rs b/crates/ui-egui/src/type_tool.rs index d73bb2e2c4..d85964ed62 100644 --- a/crates/ui-egui/src/type_tool.rs +++ b/crates/ui-egui/src/type_tool.rs @@ -1088,13 +1088,18 @@ pub fn draw_overlay(app: &mut PhotocraftApp, painter: &egui::Painter, xf: &ViewX if ed.dragging || (time * 1000.0 / 530.0) as i64 % 2 == 0 { let (x, top, bot) = l.caret(a); let (x, top, bot) = if l.lines.is_empty() { (0.0, -(12.0 * l.px_per_pt.max(1.0)), 3.0) } else { (x, top, bot) }; - let c = if crate::theme::Tokens::get(painter.ctx()).pro { Color32::WHITE } else { Color32::BLACK }; - painter.line_segment([scr(x, top), scr(x, bot)], Stroke::new(1.5, c)); - painter.line_segment([scr(x, top), scr(x, bot)], Stroke::new(0.75, Color32::BLACK)); + for stroke in CARET_STROKES { + painter.line_segment([scr(x, top), scr(x, bot)], stroke); + } } } } +/// The text caret, drawn in order: a white halo, then a solid black line inside it, so it reads on +/// any background (#6085). The black line used to be a 0.75 pt hairline inside a white one, which +/// on a white background left only a faint grey line. +const CARET_STROKES: [Stroke; 2] = [Stroke { width: 3.0, color: Color32::WHITE }, Stroke { width: 1.0, color: Color32::BLACK }]; + /// The Type Mask tools while typing: Quick Mask's colour over the canvas with the glyphs clear, /// as Photoshop shows the selection-to-be. The glyphs come from the hidden working layer's pixels. fn draw_mask_overlay(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform, id: LayerId) { diff --git a/crates/ui-egui/src/type_tool_tests.rs b/crates/ui-egui/src/type_tool_tests.rs index c7b54c8871..d27fdbcdac 100644 --- a/crates/ui-egui/src/type_tool_tests.rs +++ b/crates/ui-egui/src/type_tool_tests.rs @@ -2065,3 +2065,46 @@ fn type_mask_shift_adds_and_alt_subtracts_without_a_layer_row() { assert!(at(&app, stem.0, stem.1) < 0.01, "⌥ subtracts the glyphs"); assert!(at(&app, 1050, 150) > 0.99, "and keeps the rest"); } + +/// #6085: the text caret is a solid black line inside a white halo, so it shows on a white +/// background (the black used to be a 0.75 pt hairline inside a white line) and on a dark one. +#[test] +fn the_caret_reads_on_white_and_on_black() { + use crate::canvas::{ToolEvent, tool_event}; + let mut app = new_app(); + app.ui.tool = crate::state::Tool::Type; + tool_event(&mut app, ToolEvent::Down { x: 100.0, y: 100.0, pressure: 1.0 }, Modifiers::NONE); + tool_event(&mut app, ToolEvent::Up { x: 100.0, y: 100.0 }, Modifiers::NONE); + // A caret, not the placeholder's selection; solid, whatever the blink (as while dragging). + let ed = app.ui.text_edit.as_mut().expect("editing"); + ed.anchor = ed.caret; + ed.dragging = true; + let xf = ViewXform { + rect: egui::Rect::from_min_size(Pos2::ZERO, vec2(400.0, 300.0)), + zoom: 1.0, + center: [100.0, 100.0], + flip: false, + rotation: 0.0, + aspect: 1.0, + }; + let mut h = + Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state(move |ui, app: &mut PhotocraftApp| super::draw_overlay(app, ui.painter(), &xf), app); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ProMedium); + h.run(); + // Vertical line segments are the caret (the frame and underline run sideways). + let caret: Vec<(f32, egui::Color32)> = h + .output() + .shapes + .iter() + .filter_map(|s| match &s.shape { + egui::Shape::LineSegment { points: [a, b], stroke } if (a.x - b.x).abs() < 0.01 && (a.y - b.y).abs() > 1.0 => Some((stroke.width, stroke.color)), + _ => None, + }) + .collect(); + assert_eq!(caret.len(), 2, "a halo and a line: {caret:?}"); + let ((halo_w, halo), (line_w, line)) = (caret[0], caret[1]); + assert_eq!(line, egui::Color32::BLACK); + assert!(line_w >= 1.0, "a solid black line, not a hairline: {line_w}"); + assert_eq!(halo, egui::Color32::WHITE); + assert!(halo_w >= line_w + 2.0, "at least 1 pt of white on each side: {halo_w}"); +} From dedf89feae2df0636a1cad6c06e6761ed46f3442 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Fri, 16 Oct 2026 01:33:24 +0200 Subject: [PATCH 19/36] Type: move the caret test next to its helper, away from the file's end --- crates/ui-egui/src/type_tool_tests.rs | 86 +++++++++++++-------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/crates/ui-egui/src/type_tool_tests.rs b/crates/ui-egui/src/type_tool_tests.rs index d27fdbcdac..1202ba28fa 100644 --- a/crates/ui-egui/src/type_tool_tests.rs +++ b/crates/ui-egui/src/type_tool_tests.rs @@ -115,6 +115,49 @@ fn new_app() -> PhotocraftApp { app } +/// #6085: the text caret is a solid black line inside a white halo, so it shows on a white +/// background (the black used to be a 0.75 pt hairline inside a white line) and on a dark one. +#[test] +fn the_caret_reads_on_white_and_on_black() { + use crate::canvas::{ToolEvent, tool_event}; + let mut app = new_app(); + app.ui.tool = crate::state::Tool::Type; + tool_event(&mut app, ToolEvent::Down { x: 100.0, y: 100.0, pressure: 1.0 }, Modifiers::NONE); + tool_event(&mut app, ToolEvent::Up { x: 100.0, y: 100.0 }, Modifiers::NONE); + // A caret, not the placeholder's selection; solid, whatever the blink (as while dragging). + let ed = app.ui.text_edit.as_mut().expect("editing"); + ed.anchor = ed.caret; + ed.dragging = true; + let xf = ViewXform { + rect: egui::Rect::from_min_size(Pos2::ZERO, vec2(400.0, 300.0)), + zoom: 1.0, + center: [100.0, 100.0], + flip: false, + rotation: 0.0, + aspect: 1.0, + }; + let mut h = + Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state(move |ui, app: &mut PhotocraftApp| super::draw_overlay(app, ui.painter(), &xf), app); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ProMedium); + h.run(); + // Vertical line segments are the caret (the frame and underline run sideways). + let caret: Vec<(f32, egui::Color32)> = h + .output() + .shapes + .iter() + .filter_map(|s| match &s.shape { + egui::Shape::LineSegment { points: [a, b], stroke } if (a.x - b.x).abs() < 0.01 && (a.y - b.y).abs() > 1.0 => Some((stroke.width, stroke.color)), + _ => None, + }) + .collect(); + assert_eq!(caret.len(), 2, "a halo and a line: {caret:?}"); + let ((halo_w, halo), (line_w, line)) = (caret[0], caret[1]); + assert_eq!(line, egui::Color32::BLACK); + assert!(line_w >= 1.0, "a solid black line, not a hairline: {line_w}"); + assert_eq!(halo, egui::Color32::WHITE); + assert!(halo_w >= line_w + 2.0, "at least 1 pt of white on each side: {halo_w}"); +} + fn xf(app: &PhotocraftApp) -> ViewXform { let v = &app.ui.views[0]; ViewXform { @@ -2065,46 +2108,3 @@ fn type_mask_shift_adds_and_alt_subtracts_without_a_layer_row() { assert!(at(&app, stem.0, stem.1) < 0.01, "⌥ subtracts the glyphs"); assert!(at(&app, 1050, 150) > 0.99, "and keeps the rest"); } - -/// #6085: the text caret is a solid black line inside a white halo, so it shows on a white -/// background (the black used to be a 0.75 pt hairline inside a white line) and on a dark one. -#[test] -fn the_caret_reads_on_white_and_on_black() { - use crate::canvas::{ToolEvent, tool_event}; - let mut app = new_app(); - app.ui.tool = crate::state::Tool::Type; - tool_event(&mut app, ToolEvent::Down { x: 100.0, y: 100.0, pressure: 1.0 }, Modifiers::NONE); - tool_event(&mut app, ToolEvent::Up { x: 100.0, y: 100.0 }, Modifiers::NONE); - // A caret, not the placeholder's selection; solid, whatever the blink (as while dragging). - let ed = app.ui.text_edit.as_mut().expect("editing"); - ed.anchor = ed.caret; - ed.dragging = true; - let xf = ViewXform { - rect: egui::Rect::from_min_size(Pos2::ZERO, vec2(400.0, 300.0)), - zoom: 1.0, - center: [100.0, 100.0], - flip: false, - rotation: 0.0, - aspect: 1.0, - }; - let mut h = - Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state(move |ui, app: &mut PhotocraftApp| super::draw_overlay(app, ui.painter(), &xf), app); - PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ProMedium); - h.run(); - // Vertical line segments are the caret (the frame and underline run sideways). - let caret: Vec<(f32, egui::Color32)> = h - .output() - .shapes - .iter() - .filter_map(|s| match &s.shape { - egui::Shape::LineSegment { points: [a, b], stroke } if (a.x - b.x).abs() < 0.01 && (a.y - b.y).abs() > 1.0 => Some((stroke.width, stroke.color)), - _ => None, - }) - .collect(); - assert_eq!(caret.len(), 2, "a halo and a line: {caret:?}"); - let ((halo_w, halo), (line_w, line)) = (caret[0], caret[1]); - assert_eq!(line, egui::Color32::BLACK); - assert!(line_w >= 1.0, "a solid black line, not a hairline: {line_w}"); - assert_eq!(halo, egui::Color32::WHITE); - assert!(halo_w >= line_w + 2.0, "at least 1 pt of white on each side: {halo_w}"); -} From 0ce7e00fee45c2db22ede12184bb6a1fd3203777 Mon Sep 17 00:00:00 2001 From: David Mann Date: Fri, 16 Oct 2026 03:06:07 +0200 Subject: [PATCH 20/36] Brush: the Dual Brush tip keeps its proportion at tiny main sizes (#4809) Photoshop scales the Dual Brush diameter by the main size ratio, unrounded, with a 5000 px cap and no lower floor: main 100 / dual 20 resized to 1 px gives a 0.2 px dual tip, and back at 100 px it is 20 px again. PhotoCraft already scaled the dual tip (#4856) but floored it at 0.5 px, so a trip to 1 px and back left a 50 px dual tip. Drop the floor (rendering bounds the diameter itself), keep the cap, and leave a non-positive or NaN result alone. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/engine/src/brush_cmds/tests.rs | 51 +++++++++++++++++++++++++++ crates/paint/src/brush.rs | 12 ++++++- crates/paint/src/brush_tests.rs | 48 +++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/brush_cmds/tests.rs b/crates/engine/src/brush_cmds/tests.rs index eaf69281bc..0e34997bb4 100644 --- a/crates/engine/src/brush_cmds/tests.rs +++ b/crates/engine/src/brush_cmds/tests.rs @@ -371,6 +371,57 @@ fn resizing_a_brush_resizes_its_enabled_secondary_tip() { assert_eq!((s.tools.brush.size, s.tools.brush.dual_brush.size), (40.0, 9.0)); } +/// Every way of changing the main size scales the Dual Brush tip as Photoshop does (#4809): `[` / +/// `]`, the options bar / Brushes panel / Alt+right-drag (`tools.setBrush` with only a size), down +/// to 1 px and back without losing the proportion to a floor. +#[test] +fn every_size_change_scales_the_dual_tip_and_keeps_its_proportion() { + let mut s = session(32, 32); + s.execute("tools.setBrush", json!({"brush": {"size": 100, "dualBrush": {"enabled": true, "size": 20}}})).unwrap(); + let sizes = |s: &Session| (s.tools.brush.size, s.tools.brush.dual_brush.size); + s.execute("tools.decreaseBrushSize", json!({})).unwrap(); + assert_eq!(sizes(&s), (80.0, 16.0)); + s.execute("tools.increaseBrushSize", json!({})).unwrap(); + assert_eq!(sizes(&s), (100.0, 20.0)); + // A coalesced drag (options bar slider, Alt+right-drag) is a run of size-only patches. + for size in [60, 10, 1] { + s.execute("tools.setBrush", json!({"brush": {"size": size}, "coalesce": "drag"})).unwrap(); + } + assert_eq!(s.tools.brush.size, 1.0); + assert!((s.tools.brush.dual_brush.size - 0.2).abs() < 1e-5, "no 0.5 px floor: {}", s.tools.brush.dual_brush.size); + s.execute("tools.setBrush", json!({"brush": {"size": 100}})).unwrap(); + assert_eq!(s.tools.brush.size, 100.0); + assert!((s.tools.brush.dual_brush.size - 20.0).abs() < 1e-3, "back to 20 px: {}", s.tools.brush.dual_brush.size); + // Painting with the sub-pixel dual tip works (the renderer bounds it). + s.execute("tools.setBrush", json!({"brush": {"size": 1}})).unwrap(); + s.execute("paint.stroke", json!({"points": [[4, 16], [28, 16]]})).unwrap(); +} + +/// Picking a preset that captured its size sets both diameters from the preset: the dual tip is +/// not scaled on top of that. +#[test] +fn a_captured_preset_size_is_not_scaled_twice() { + let mut s = session(32, 32); + s.execute("tools.setBrush", json!({"size": 40, "dualBrush": {"enabled": true, "size": 10}})).unwrap(); + s.execute("brush.presets.save", json!({"name": "Dual40"})).unwrap(); + s.execute("tools.setBrush", json!({"size": 200})).unwrap(); + assert_eq!(s.tools.brush.dual_brush.size, 50.0); + s.execute("tools.setBrush", json!({"preset": "Dual40"})).unwrap(); + assert_eq!((s.tools.brush.size, s.tools.brush.dual_brush.size), (40.0, 10.0)); +} + +#[test] +fn bad_sizes_fail_and_leave_the_dual_tip_alone() { + let mut s = session(32, 32); + s.execute("tools.setBrush", json!({"brush": {"size": 100, "dualBrush": {"enabled": true, "size": 20}}})).unwrap(); + for bad in [json!(1e30), json!("big"), json!([1]), json!({"x": 1}), json!(null)] { + let _ = s.execute("tools.setBrush", json!({"brush": {"size": bad}})); + assert_eq!(s.tools.brush.dual_brush.size, 20.0, "{bad}"); + assert_eq!(s.tools.brush.size, 100.0, "{bad}"); + } + assert!(s.execute("tools.setBrush", json!({"brush": {"size": 1e30}})).is_err()); +} + #[test] fn an_uncaptured_preset_size_keeps_dual_brush_proportions() { let mut s = session(32, 32); diff --git a/crates/paint/src/brush.rs b/crates/paint/src/brush.rs index d60412a58d..3943073855 100644 --- a/crates/paint/src/brush.rs +++ b/crates/paint/src/brush.rs @@ -589,9 +589,19 @@ impl Default for BrushSettings { impl BrushSettings { /// Resize the primary tip and keep an enabled Dual Brush tip at the same relative size. /// An explicit Dual Brush size edit should bypass this and retain the requested diameter. + /// + /// As in Photoshop, the dual diameter is multiplied by the size ratio unrounded and only capped + /// at the top ([`MAX_BRUSH_SIZE`]): a 100 px brush with a 20 px dual tip shrunk to 1 px has a + /// 0.2 px dual tip and grows back to exactly 20 px. A sub-pixel floor here would lose the + /// proportion on the way back up (#4809); rendering bounds the diameter itself + /// ([`Self::bounded_for_render`]). pub fn resize_with_dual_brush(&mut self, new_size: f32) { if self.dual_brush.enabled && self.size.is_finite() && self.size > 0.0 && new_size.is_finite() && new_size > 0.0 { - self.dual_brush.size = (self.dual_brush.size * (new_size / self.size)).clamp(0.5, MAX_BRUSH_SIZE); + let scaled = self.dual_brush.size * (new_size / self.size); + // NaN fails the comparison; infinity is capped. + if scaled > 0.0 { + self.dual_brush.size = scaled.min(MAX_BRUSH_SIZE); + } } self.size = new_size; } diff --git a/crates/paint/src/brush_tests.rs b/crates/paint/src/brush_tests.rs index d0d7de4110..051b3652f8 100644 --- a/crates/paint/src/brush_tests.rs +++ b/crates/paint/src/brush_tests.rs @@ -1493,3 +1493,51 @@ fn an_aliased_stroke_at_partial_opacity_is_one_flat_colour() { assert_eq!(s.rgba(x, 5), first, "pixel {x} of the Pencil line"); } } + +/// Photoshop (measured): a 100 px brush with a 20 px Dual Brush tip resized to 50 / 2 / 1 px has a +/// 10 / 0.4 / 0.2 px dual tip, unrounded and with no floor, and back at 100 px it is 20 px again; past +/// 5000 px the dual tip is capped there and the proportion is lost (#4809). +#[test] +fn resizing_scales_the_dual_tip_like_photoshop() { + let mut b = BrushSettings { size: 100.0, ..brush() }; + b.dual_brush.enabled = true; + b.dual_brush.size = 20.0; + for (main, dual) in [(50.0, 10.0), (2.0, 0.4), (1.0, 0.2), (100.0, 20.0), (240.0, 48.0), (100.0, 20.0)] { + b.resize_with_dual_brush(main); + assert_eq!(b.size, main); + assert!((b.dual_brush.size - dual).abs() < 1e-4, "main {main}: dual {} != {dual}", b.dual_brush.size); + } + // The cap: 300 px at 100 px would be 15000 px at 5000 px; Photoshop stores 5000 and scales from it. + b.dual_brush.size = 300.0; + b.resize_with_dual_brush(MAX_BRUSH_SIZE); + assert_eq!(b.dual_brush.size, MAX_BRUSH_SIZE); + b.resize_with_dual_brush(100.0); + assert_eq!(b.dual_brush.size, 100.0); + // A sub-pixel dual tip renders at the renderer's smallest diameter. + b.dual_brush.size = 20.0; + b.resize_with_dual_brush(1.0); + assert_eq!(b.bounded_for_render().dual_brush.size, 0.5); +} + +#[test] +fn resizing_with_bad_sizes_leaves_the_dual_tip_alone() { + let mut b = BrushSettings { size: 100.0, ..brush() }; + b.dual_brush.enabled = true; + b.dual_brush.size = 20.0; + for bad in [f32::NAN, f32::INFINITY, f32::NEG_INFINITY, 0.0, -5.0] { + b.size = 100.0; + b.resize_with_dual_brush(bad); + assert_eq!(b.dual_brush.size, 20.0, "{bad}"); + } + for bad_dual in [f32::NAN, 0.0, -3.0] { + b.size = 100.0; + b.dual_brush.size = bad_dual; + b.resize_with_dual_brush(50.0); + assert_eq!(b.dual_brush.size.to_bits(), bad_dual.to_bits(), "a bad dual size {bad_dual} is left as it was"); + } + // A disabled dual tip keeps its size. + b.dual_brush = DualBrush { enabled: false, size: 20.0, ..Default::default() }; + b.size = 100.0; + b.resize_with_dual_brush(50.0); + assert_eq!(b.dual_brush.size, 20.0); +} From 7bc556ec02aae434f0a83ff2efc16d48c3311e24 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Thu, 15 Oct 2026 21:54:09 -0400 Subject: [PATCH 21/36] Web: a requested download no longer marks the document saved (#4962) The web writer only triggers an anchor download, which the user can cancel and which reports nothing back, yet Save treated it as a confirmed write: the document was marked clean and the leave-page guard disarmed. Services gains `write_unconfirmed` (set by the web shell); a save through such a writer names the document but keeps its changes unsaved. Co-Authored-By: Claude Opus 5.5 --- apps/photocraft-web/src/web.rs | 2 ++ crates/ui-egui/src/lib.rs | 14 +++++++++++++- crates/ui-egui/src/save_identity_tests.rs | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/apps/photocraft-web/src/web.rs b/apps/photocraft-web/src/web.rs index eb350d17eb..7084396cbe 100644 --- a/apps/photocraft-web/src/web.rs +++ b/apps/photocraft-web/src/web.rs @@ -379,6 +379,8 @@ fn services(inbox: Inbox) -> Services { } })), write: Some(Box::new(|path: &str, bytes: &[u8]| download(path, bytes))), + // A download may be cancelled and never reports back: it doesn't save the document. + write_unconfirmed: true, encode_png: Some(Box::new(|w, h, rgba| { let img = Image::from_u8(w, h, ChannelLayout::Rgba, rgba.to_vec()).map_err(|e| e.to_string())?; photocraft_codecs::encode(&img, photocraft_codecs::Format::Png, &EncodeOptions::default()).map_err(|e| e.to_string()) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index bde51ff27a..4802fbe91a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -354,6 +354,13 @@ pub type WriteFn = Box Result<(), String>>; /// [`jobs_ui::SaveCtl::commit`] right before replacing the file, and write nothing if that fails. pub type SaveFileFn = std::sync::Arc Result, String> + Send + Sync>; +/// The revision a document counts as saved at after `written` went to its writer: that one when +/// the writer confirms the bytes were stored, else still `previous`, so the document stays +/// unsaved. A browser download is only a request the user can cancel (#4962). +pub(crate) fn saved_revision_after_write(confirmed: bool, written: u64, previous: u64) -> u64 { + if confirmed { written } else { previous } +} + /// Who a finished save serves. #[derive(Clone, Copy)] pub(crate) enum SaveKind { @@ -446,6 +453,10 @@ pub struct Services { pub file_dialog: Option, /// Write bytes to a path (native) or trigger a download (web). pub write: Option, + /// `write` only asks for the bytes to be kept (a browser download, which the user can + /// cancel and which reports nothing back), so a save through it names the document but + /// leaves its changes unsaved and the leave-page guard armed (#4962). + pub write_unconfirmed: bool, /// Encode and write in one step on a worker thread, so a large save doesn't freeze the /// window (#2017). With background jobs on, saves use it instead of `export` and `write`. pub save_file: Option, @@ -1592,11 +1603,12 @@ impl PhotocraftApp { return jobs_ui::start_save(self, path, settings.clone(), copy, save, false); } let st = self.session.active().ok_or("no document")?; - let (doc, revision) = (st.doc.id, st.revision); + let (doc, written, previous) = (st.doc.id, st.revision, st.saved_revision); let export = self.services.export.as_ref().ok_or("no exporter configured")?; let (bytes, warnings) = export(&st.doc, &path, settings)?; let write = self.services.write.as_mut().ok_or("no writer configured")?; write(&path, &bytes)?; + let revision = saved_revision_after_write(!self.services.write_unconfirmed, written, previous); self.saved(doc, revision, &path, &warnings, copy); Ok(Some((path, warnings))) } diff --git a/crates/ui-egui/src/save_identity_tests.rs b/crates/ui-egui/src/save_identity_tests.rs index c947afc9b6..3396dfcdd5 100644 --- a/crates/ui-egui/src/save_identity_tests.rs +++ b/crates/ui-egui/src/save_identity_tests.rs @@ -254,3 +254,21 @@ fn save_settings_start_at_the_opened_files_encoding() { app.open_bytes("art.webp", &lossless).unwrap(); assert!(app.save_settings().webp_lossless); } + +/// A browser download is only requested: the document takes the name but keeps its unsaved +/// changes, so the leave-page guard stays armed (#4962). A confirmed write still saves it. +#[test] +fn unconfirmed_download_names_the_document_but_keeps_it_unsaved() { + assert_eq!(crate::saved_revision_after_write(true, 7, 3), 7); + assert_eq!(crate::saved_revision_after_write(false, 7, 3), 3); + let mut app = app(); + app.services.write_unconfirmed = true; + let (_, _, revision, saved) = identity(&app); + app.save_as(Some("download.psd".into())).unwrap(); + assert_eq!(identity(&app), ("download.psd".into(), Some("download.psd".into()), revision, saved)); + assert!(app.session.active().unwrap().is_dirty()); + assert!(app.has_unsaved_work()); + app.services.write_unconfirmed = false; + app.save_as(Some("download.psd".into())).unwrap(); + assert!(!app.has_unsaved_work()); +} From 76f29f67d55f3554c25d47125145cefeb0e9a030 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Thu, 15 Oct 2026 22:16:03 -0400 Subject: [PATCH 22/36] rustfmt: format after merging main (CI Format step) Co-Authored-By: Claude Opus 5.5 --- crates/automation/tests/smart_filter_authorization.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/automation/tests/smart_filter_authorization.rs b/crates/automation/tests/smart_filter_authorization.rs index a5c1681a52..30f17b795b 100644 --- a/crates/automation/tests/smart_filter_authorization.rs +++ b/crates/automation/tests/smart_filter_authorization.rs @@ -230,11 +230,9 @@ fn a_refresh_outside_command_execution_surfaces_the_denial_instead_of_rendering_ let before = h.session.active().unwrap().doc.clone(); // Session::edit is the outside-execute mutation path (an edit closure the UI or a // linked-file change runs); the stored filter must be denied there too. - assert_denied(h.session.edit("Re-render", |d, _| { - match smart_cmds::refresh(d, layer) { - Ok(_) => Ok(()), - Err(e) => Err(e), - } + assert_denied(h.session.edit("Re-render", |d, _| match smart_cmds::refresh(d, layer) { + Ok(_) => Ok(()), + Err(e) => Err(e), })); assert!(Arc::ptr_eq(&before, &h.session.active().unwrap().doc)); // A trusted session (no gate) still renders the same document. From 5c0e40784a1056a6dfcda902e44c3e0242bf0464 Mon Sep 17 00:00:00 2001 From: Higgs32584 Date: Thu, 15 Oct 2026 22:29:42 -0400 Subject: [PATCH 23/36] rustfmt; merge main (CI Format step) Co-Authored-By: Claude Opus 5.5 --- crates/automation/tests/smart_filter_authorization.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/automation/tests/smart_filter_authorization.rs b/crates/automation/tests/smart_filter_authorization.rs index a5c1681a52..30f17b795b 100644 --- a/crates/automation/tests/smart_filter_authorization.rs +++ b/crates/automation/tests/smart_filter_authorization.rs @@ -230,11 +230,9 @@ fn a_refresh_outside_command_execution_surfaces_the_denial_instead_of_rendering_ let before = h.session.active().unwrap().doc.clone(); // Session::edit is the outside-execute mutation path (an edit closure the UI or a // linked-file change runs); the stored filter must be denied there too. - assert_denied(h.session.edit("Re-render", |d, _| { - match smart_cmds::refresh(d, layer) { - Ok(_) => Ok(()), - Err(e) => Err(e), - } + assert_denied(h.session.edit("Re-render", |d, _| match smart_cmds::refresh(d, layer) { + Ok(_) => Ok(()), + Err(e) => Err(e), })); assert!(Arc::ptr_eq(&before, &h.session.active().unwrap().doc)); // A trusted session (no gate) still renders the same document. From d8a23d3ec38aa6666c434265c7416aa71fcd3470 Mon Sep 17 00:00:00 2001 From: Gert Pellin Date: Fri, 16 Oct 2026 07:30:48 +0200 Subject: [PATCH 24/36] Automation: a control token authenticates in either letter case (#6425) validate_token accepts uppercase hex and server_token keeps the token lowercased, but authentication_reply passed the client's token to token_matches unchanged, which compared bytes exactly. A client sending the token exactly as configured (--control-token or a token file in uppercase) was refused; only the bundled bridge, which lowercases, got in. token_matches now folds ASCII letters on both sides, still without an early exit. Only A-Z fold, so a non-hex byte never matches a hex digit. --- crates/automation/src/security.rs | 45 ++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/crates/automation/src/security.rs b/crates/automation/src/security.rs index 7928420b12..3f62c191b5 100644 --- a/crates/automation/src/security.rs +++ b/crates/automation/src/security.rs @@ -97,11 +97,15 @@ pub fn validate_token(token: &str) -> Result<(), AutomationError> { } /// Compare fixed-width tokens without an early exit on a mismatching byte. +/// +/// Hex letters match in either case: [`validate_token`] accepts uppercase, and the server keeps +/// the token lowercased, so a client sending the token exactly as it was configured must still +/// authenticate (#6425). Only `A`–`Z` fold, so a non-hex byte never matches a hex digit. pub fn token_matches(expected: &str, supplied: &str) -> bool { if expected.len() != TOKEN_HEX_LEN || supplied.len() != TOKEN_HEX_LEN { return false; } - expected.bytes().zip(supplied.bytes()).fold(0u8, |different, (a, b)| different | (a ^ b)) == 0 + expected.bytes().zip(supplied.bytes()).fold(0u8, |different, (a, b)| different | (a.to_ascii_lowercase() ^ b.to_ascii_lowercase())) == 0 } /// Validate the first TCP frame without exposing any control method before authentication. @@ -348,6 +352,45 @@ mod tests { assert_eq!(reply["result"]["authenticated"], true); } + fn auth_line(token: &str) -> String { + json!({"id": 1, "method": AUTH_METHOD, "params": {"token": token}}).to_string() + } + + /// #6425: `validate_token` accepts uppercase hex and the server keeps the token lowercased, so a + /// client sending the token exactly as it was configured was refused. + #[test] + fn a_token_authenticates_in_any_letter_case() { + let configured = "0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF"; + let mixed = "0123456789AbCdEf0123456789aBcDeF0123456789ABCDEF0123456789abcdef"; + let expected = server_token(Some(configured), None).unwrap(); + for supplied in [configured, &configured.to_ascii_lowercase(), mixed] { + let (reply, authenticated) = authentication_reply(&auth_line(supplied), &expected); + assert!(authenticated, "{supplied}: {reply}"); + assert_eq!(reply["result"]["authenticated"], true); + } + // The comparison is case-insensitive in both directions. + assert!(token_matches(configured, &configured.to_ascii_lowercase())); + + // A different token, or a non-hex character that only folds to a hex one, still fails. + let other = "1123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF"; + let (_, authenticated) = authentication_reply(&auth_line(other), &expected); + assert!(!authenticated); + let not_hex = "0123456789ABCDEG0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF"; + assert!(!token_matches(&expected, not_hex)); + } + + /// #6425: the same for a token file whose contents are uppercase. + #[test] + fn an_uppercase_token_file_authenticates_its_exact_contents() { + let path = std::env::temp_dir().join(format!("photocraft-control-token-upper-{}-{}.txt", std::process::id(), generate_token().unwrap())); + let contents = generate_token().unwrap().to_ascii_uppercase(); + std::fs::write(&path, format!("{contents}\n")).unwrap(); + let expected = server_token(None, Some(&path)); + std::fs::remove_file(&path).unwrap(); + let (reply, authenticated) = authentication_reply(&auth_line(&contents), &expected.unwrap()); + assert!(authenticated, "{reply}"); + } + #[test] fn repeat_auth_on_an_authenticated_connection_says_so() { let line = json!({"id": 3, "method": AUTH_METHOD, "params": {"token": "anything"}}).to_string(); From a08992c9d93676affdabf97981547a4bce6974bc Mon Sep 17 00:00:00 2001 From: highperfocused Date: Fri, 16 Oct 2026 09:14:24 +0200 Subject: [PATCH 25/36] Desktop: a control/automation flag without its value is a usage error (#6429) --- apps/photocraft/src/main.rs | 288 +++++++++++++++++++++++++----------- 1 file changed, 203 insertions(+), 85 deletions(-) diff --git a/apps/photocraft/src/main.rs b/apps/photocraft/src/main.rs index c0f93bd319..337399d99a 100644 --- a/apps/photocraft/src/main.rs +++ b/apps/photocraft/src/main.rs @@ -149,12 +149,115 @@ fn parse_control_port(value: &str, source: &str) -> Result { value.trim().parse().map_err(|_| format!("{source}: `{value}` is not a valid port (expected a number from 0 to 65535)")) } -/// Process exit status for the collected `--control` / `PHOTOCRAFT_CONTROL_PORT` errors: `None` -/// when there are none, else 2 (a command-line usage error), never 0. +/// Process exit status for the collected control/automation argument errors: `None` when there +/// are none, else 2 (a command-line usage error), never 0. fn control_args_exit_code(errors: &[String]) -> Option { if errors.is_empty() { None } else { Some(2) } } +/// Everything the desktop launcher takes from the command line, on top of the environment +/// defaults handed in to `parse_launch_args`. +#[derive(Debug, Default, PartialEq, Eq)] +struct LaunchArgs { + control_port: Option, + control_token: Option, + control_token_file: Option, + automation_read_root: Option, + automation_write_root: Option, + files: Vec, + /// Arguments that are not valid Unicode (issue #1108): a file manager can hand over a Latin-1 + /// file name on Linux, and `std::env::args()` would panic before any window opened. Paths are + /// reported in a notice once the app is up; a control token is a usage error like a bad port. + unreadable_paths: Vec, + safe_gpu: bool, + new_instance: bool, + /// Options that apply only to the process they're given to; a launch with any keeps its own. + launch_options: Vec<&'static str>, + present_mode_arg: Option, + frame_latency_arg: Option, + stroke_trace_arg: Option, + in_window_menus: bool, + version: bool, + /// Usage errors in the control/automation flags; a non-empty list fails the launch (issue #701). + errors: Vec, +} + +/// Parse the desktop launch arguments over the environment defaults in `out`. A control or +/// automation flag with no value is a usage error naming the flag (issue #6429), never a silent +/// fall back to a generated token or to no root: like `--control` with a bad port, it exits 2. +fn parse_launch_args(args: impl Iterator, mut out: LaunchArgs) -> LaunchArgs { + let mut args = args; + while let Some(a) = args.next() { + match a.to_str() { + Some("--control") => match args.next() { + Some(value) => match parse_control_port(&value.to_string_lossy(), "--control") { + Ok(port) => out.control_port = Some(port), + Err(error) => out.errors.push(error), + }, + None => out.errors.push("--control: missing port value (expected `--control `)".to_string()), + }, + Some("--control-token") => match args.next().map(std::ffi::OsString::into_string) { + Some(Ok(token)) => out.control_token = Some(token), + Some(Err(raw)) => out.errors.push(format!("--control-token: value is not valid Unicode (`{}`)", raw.to_string_lossy())), + None => out.errors.push("--control-token: missing token value (expected `--control-token `)".to_string()), + }, + Some("--control-token-file") => match args.next() { + Some(value) => out.control_token_file = Some(std::path::PathBuf::from(value)), + None => out.errors.push("--control-token-file: missing file path value (expected `--control-token-file `)".to_string()), + }, + Some("--automation-read-root") => { + out.launch_options.push("--automation-read-root"); + match args.next() { + Some(value) => out.automation_read_root = Some(std::path::PathBuf::from(value)), + None => out.errors.push("--automation-read-root: missing directory value (expected `--automation-read-root `)".to_string()), + } + } + Some("--automation-write-root") => { + out.launch_options.push("--automation-write-root"); + match args.next() { + Some(value) => out.automation_write_root = Some(std::path::PathBuf::from(value)), + None => out.errors.push("--automation-write-root: missing directory value (expected `--automation-write-root `)".to_string()), + } + } + Some("--safe-gpu") => { + out.launch_options.push("--safe-gpu"); + out.safe_gpu = true; + } + Some(flag @ ("--present-mode" | "--frame-latency" | "--stroke-trace")) => { + out.launch_options.push(match flag { + "--present-mode" => "--present-mode", + "--frame-latency" => "--frame-latency", + _ => "--stroke-trace", + }); + let flag = flag.to_string(); + match args.next().map(std::ffi::OsString::into_string) { + Some(Ok(v)) => match flag.as_str() { + "--present-mode" => out.present_mode_arg = Some(v), + "--frame-latency" => out.frame_latency_arg = Some(v), + _ => out.stroke_trace_arg = Some(v), + }, + // A bad value is reported and ignored; it never fails the launch. + _ => eprintln!("photocraft: {flag}: missing or unreadable value; ignored"), + } + } + Some("--in-window-menus") => { + out.launch_options.push("--in-window-menus"); + out.in_window_menus = true; + } + Some("--new-instance") => out.new_instance = true, + Some("--version") => { + out.version = true; + break; + } + // Old macOS passes a process serial number when launched from Finder. + Some(s) if s.starts_with("-psn_") => {} + Some(s) => out.files.push(s.to_owned()), + None => out.unreadable_paths.push(a.to_string_lossy().into_owned()), + } + } + out +} + #[cfg(test)] mod control_port_tests { use super::{control_args_exit_code, parse_control_port}; @@ -184,105 +287,120 @@ mod control_port_tests { } } +#[cfg(test)] +mod launch_args_tests { + use super::{control_args_exit_code, parse_launch_args, LaunchArgs}; + use std::ffi::OsString; + + fn parse(values: &[&str]) -> LaunchArgs { + parse_launch_args(values.iter().map(OsString::from), LaunchArgs::default()) + } + + #[test] + fn flags_without_a_value_are_usage_errors() { + // Issue #6429: a missing value must fail the launch, not silently fall back to a + // generated token or to no root. + for flag in ["--control", "--control-token", "--control-token-file", "--automation-read-root", "--automation-write-root"] { + let parsed = parse(&[flag]); + assert_eq!(parsed.errors.len(), 1, "{flag}: {:?}", parsed.errors); + assert!(parsed.errors[0].starts_with(flag), "{}", parsed.errors[0]); + assert!(parsed.errors[0].contains("missing"), "{}", parsed.errors[0]); + assert_eq!(control_args_exit_code(&parsed.errors), Some(2)); + } + } + + #[test] + fn missing_value_does_not_clear_an_environment_default() { + // `--automation-read-root` with no value must not drop a root set through the env var. + let defaults = LaunchArgs { + automation_read_root: Some("/env/read".into()), + automation_write_root: Some("/env/write".into()), + ..Default::default() + }; + let parsed = parse_launch_args(["--automation-read-root"].iter().map(OsString::from), defaults); + assert_eq!(parsed.automation_read_root.as_deref(), Some(std::path::Path::new("/env/read"))); + assert_eq!(parsed.automation_write_root.as_deref(), Some(std::path::Path::new("/env/write"))); + assert_eq!(parsed.errors.len(), 1, "{:?}", parsed.errors); + } + + #[test] + fn values_are_parsed_and_no_errors_remain() { + let parsed = parse(&["--control", "50494", "--control-token", "abcd", "--control-token-file", "/tmp/token", "--automation-read-root", "/r", "--automation-write-root", "/w", "--safe-gpu", "--in-window-menus", "--new-instance", "--present-mode", "mailbox", "photo.psd"]); + assert!(parsed.errors.is_empty(), "{:?}", parsed.errors); + assert_eq!(parsed.control_port, Some(50494)); + assert_eq!(parsed.control_token.as_deref(), Some("abcd")); + assert_eq!(parsed.control_token_file.as_deref(), Some(std::path::Path::new("/tmp/token"))); + assert_eq!(parsed.automation_read_root.as_deref(), Some(std::path::Path::new("/r"))); + assert_eq!(parsed.automation_write_root.as_deref(), Some(std::path::Path::new("/w"))); + assert!(parsed.safe_gpu); + assert!(parsed.in_window_menus); + assert!(parsed.new_instance); + assert_eq!(parsed.present_mode_arg.as_deref(), Some("mailbox")); + assert_eq!(parsed.launch_options, vec!["--automation-read-root", "--automation-write-root", "--safe-gpu", "--in-window-menus", "--present-mode"]); + assert_eq!(parsed.files, vec!["photo.psd".to_string()]); + } + + #[test] + fn version_stops_parsing() { + let parsed = parse(&["--version", "--control"]); + assert!(parsed.version); + assert!(parsed.errors.is_empty(), "{:?}", parsed.errors); + } +} + fn main() -> eframe::Result { // First, so the panic hook and every start-up warning are recorded (`logging`). let logger = logging::install(); crash_guard::install_hook(); // The system font scan can take seconds (minutes on some Macs): run it off the UI thread. photocraft_ui_egui::type_tool::warm_fonts(); - let mut control_port: Option = None; - let mut control_arg_errors: Vec = Vec::new(); + let mut launch = LaunchArgs { + automation_read_root: std::env::var_os("PHOTOCRAFT_AUTOMATION_READ_ROOT").map(std::path::PathBuf::from), + automation_write_root: std::env::var_os("PHOTOCRAFT_AUTOMATION_WRITE_ROOT").map(std::path::PathBuf::from), + in_window_menus: std::env::var_os("PHOTOCRAFT_IN_WINDOW_MENUS").is_some_and(|v| !v.is_empty() && v != "0"), + ..Default::default() + }; if let Some(value) = std::env::var("PHOTOCRAFT_CONTROL_PORT").ok().filter(|v| !v.trim().is_empty()) { match parse_control_port(&value, "PHOTOCRAFT_CONTROL_PORT") { - Ok(port) => control_port = Some(port), - Err(error) => control_arg_errors.push(error), + Ok(port) => launch.control_port = Some(port), + Err(error) => launch.errors.push(error), } } - let mut control_token = None; - let mut control_token_file = None; - let mut automation_read_root = std::env::var_os("PHOTOCRAFT_AUTOMATION_READ_ROOT").map(std::path::PathBuf::from); - let mut automation_write_root = std::env::var_os("PHOTOCRAFT_AUTOMATION_WRITE_ROOT").map(std::path::PathBuf::from); - let mut files = Vec::new(); - // Arguments that are not valid Unicode (issue #1108): a file manager can hand over a Latin-1 - // file name on Linux, and `std::env::args()` would panic before any window opened. Paths are - // reported in a notice once the app is up; a control token is a usage error like a bad port. - let mut unreadable_paths: Vec = Vec::new(); - let mut safe_gpu = false; - let mut new_instance = false; - // Options that apply only to the process they're given to; a launch with any keeps its own. - let mut launch_options: Vec<&'static str> = Vec::new(); - let (mut present_mode_arg, mut frame_latency_arg, mut stroke_trace_arg): (Option, Option, Option) = (None, None, None); - let mut in_window_menus = std::env::var_os("PHOTOCRAFT_IN_WINDOW_MENUS").is_some_and(|v| !v.is_empty() && v != "0"); - let mut args = std::env::args_os().skip(1); - while let Some(a) = args.next() { - match a.to_str() { - Some("--control") => match args.next() { - Some(value) => match parse_control_port(&value.to_string_lossy(), "--control") { - Ok(port) => control_port = Some(port), - Err(error) => control_arg_errors.push(error), - }, - None => control_arg_errors.push("--control: missing port value (expected `--control `)".to_string()), - }, - Some("--control-token") => match args.next().map(std::ffi::OsString::into_string) { - Some(Ok(token)) => control_token = Some(token), - Some(Err(raw)) => control_arg_errors.push(format!("--control-token: value is not valid Unicode (`{}`)", raw.to_string_lossy())), - None => control_token = None, - }, - Some("--control-token-file") => control_token_file = args.next().map(std::path::PathBuf::from), - Some("--automation-read-root") => { - launch_options.push("--automation-read-root"); - automation_read_root = args.next().map(std::path::PathBuf::from); - } - Some("--automation-write-root") => { - launch_options.push("--automation-write-root"); - automation_write_root = args.next().map(std::path::PathBuf::from); - } - Some("--safe-gpu") => { - launch_options.push("--safe-gpu"); - safe_gpu = true; - } - Some(flag @ ("--present-mode" | "--frame-latency" | "--stroke-trace")) => { - launch_options.push(match flag { - "--present-mode" => "--present-mode", - "--frame-latency" => "--frame-latency", - _ => "--stroke-trace", - }); - let flag = flag.to_string(); - match args.next().map(std::ffi::OsString::into_string) { - Some(Ok(v)) => match flag.as_str() { - "--present-mode" => present_mode_arg = Some(v), - "--frame-latency" => frame_latency_arg = Some(v), - _ => stroke_trace_arg = Some(v), - }, - // A bad value is reported and ignored; it never fails the launch. - _ => eprintln!("photocraft: {flag}: missing or unreadable value; ignored"), - } - } - Some("--in-window-menus") => { - launch_options.push("--in-window-menus"); - in_window_menus = true; - } - Some("--new-instance") => new_instance = true, - Some("--version") => { - println!("photocraft {}", photocraft_engine::build_info::long_version()); - return Ok(()); - } - // Old macOS passes a process serial number when launched from Finder. - Some(s) if s.starts_with("-psn_") => {} - Some(s) => files.push(s.to_owned()), - None => unreadable_paths.push(a.to_string_lossy().into_owned()), - } + launch = parse_launch_args(std::env::args_os().skip(1), launch); + if launch.version { + println!("photocraft {}", photocraft_engine::build_info::long_version()); + return Ok(()); } - // A malformed control port must not silently drop the control server (issue #701): name the - // bad value and fail the launch, like `photocraft-cli serve --port` does for the same typo. - // Exit status 2 is the usual command-line usage error, so a launcher sees the failure. - if let Some(code) = control_args_exit_code(&control_arg_errors) { - for error in &control_arg_errors { + // A malformed control port must not silently drop the control server (issue #701), and a + // control or automation flag without its value must not silently fall back to a generated + // token or to no root (issue #6429): name the flag and fail the launch, like + // `photocraft-cli serve --port` does for the same typo. Exit status 2 is the usual + // command-line usage error, so a launcher sees the failure. + if let Some(code) = control_args_exit_code(&launch.errors) { + for error in &launch.errors { eprintln!("photocraft: {error}"); } std::process::exit(code); } + let LaunchArgs { + control_port, + control_token, + control_token_file, + automation_read_root, + automation_write_root, + files, + unreadable_paths, + safe_gpu, + new_instance, + launch_options, + present_mode_arg, + frame_latency_arg, + stroke_trace_arg, + in_window_menus, + version: _, + errors: _, + } = launch; // The log file lives under the settings directory; opened after the arguments, so `--version` // and usage errors leave no file behind. Records logged until now are written to it first. From 3f216a0cd1b9549a3ec81db75f5d160badf14d89 Mon Sep 17 00:00:00 2001 From: PerchAnton Date: Fri, 16 Oct 2026 10:28:57 +0300 Subject: [PATCH 26/36] fix(engine): reject numeric overflow before document edits --- crates/engine/src/adjust_cmds.rs | 2 +- crates/engine/src/analysis_cmds.rs | 14 +- crates/engine/src/artboard_cmds.rs | 10 +- crates/engine/src/channel_cmds.rs | 16 +- crates/engine/src/color_to_alpha_cmds.rs | 9 +- crates/engine/src/commands.rs | 60 ++++++-- crates/engine/src/extra_cmds.rs | 2 +- crates/engine/src/fill_cmds.rs | 2 +- crates/engine/src/layer_style.rs | 31 +++- crates/engine/src/lens_cmds.rs | 2 +- crates/engine/src/notes_cmds.rs | 14 +- crates/engine/src/type_cmds.rs | 5 +- crates/engine/tests/finite_command_params.rs | 154 +++++++++++++++++++ 13 files changed, 270 insertions(+), 51 deletions(-) create mode 100644 crates/engine/tests/finite_command_params.rs diff --git a/crates/engine/src/adjust_cmds.rs b/crates/engine/src/adjust_cmds.rs index c1393bb94c..e465b0ddc1 100644 --- a/crates/engine/src/adjust_cmds.rs +++ b/crates/engine/src/adjust_cmds.rs @@ -335,7 +335,7 @@ fn shadows_highlights_params(p: &Value) -> ShadowsHighlights { fn replace_color(s: &mut Session, p: &Value) -> Result { const CMD: &str = "image.adjustments.replaceColor"; - let c = crate::commands::color_param(p, "color", s.tools.foreground); + let c = crate::commands::color_param(CMD, p, "color", s.tools.foreground)?; let fuzz = num(p, "fuzziness", 40.0).clamp(0.0, 200.0); let (h, sat, l) = (num(p, "hue", 0.0).clamp(-180.0, 180.0), num(p, "saturation", 0.0).clamp(-100.0, 100.0), num(p, "lightness", 0.0).clamp(-100.0, 100.0)); // Localized Color Clusters, as in Select › Color Range: the colour only matches within Range diff --git a/crates/engine/src/analysis_cmds.rs b/crates/engine/src/analysis_cmds.rs index 85a6be4513..e6e285993e 100644 --- a/crates/engine/src/analysis_cmds.rs +++ b/crates/engine/src/analysis_cmds.rs @@ -722,10 +722,12 @@ fn group_index(d: &Document, p: &Value) -> Option { (i < d.measurement.count_groups.len()).then_some(i) } -fn color_of(p: &Value, key: &str) -> Option { - p.get(key)?; - let c = crate::commands::color_param(p, key, [1.0, 0.0, 0.0, 1.0]); - Some(Color::rgb(c[0], c[1], c[2])) +fn color_of(cmd: &str, p: &Value, key: &str) -> Result> { + if p.get(key).is_none() { + return Ok(None); + } + let c = crate::commands::color_param(cmd, p, key, [1.0, 0.0, 0.0, 1.0])?; + Ok(Some(Color::rgb(c[0], c[1], c[2]))) } /// Photoshop's colours for new count groups, in order. @@ -840,7 +842,7 @@ fn count_clear(s: &mut Session, p: &Value) -> Result { fn count_new_group(s: &mut Session, p: &Value) -> Result { let name = p.get("name").and_then(Value::as_str).map(str::to_string); - let color = color_of(p, "color"); + let color = color_of("count.newGroup", p, "color")?; s.edit("New Count Group", |d, _| { let g = new_group(d, name, color); d.measurement.count_groups.push(g); @@ -887,7 +889,7 @@ fn count_set_group(s: &mut Session, p: &Value) -> Result { if let Some(n) = p.get("name").and_then(Value::as_str) { grp.name = n.to_string(); } - if let Some(c) = color_of(p, "color") { + if let Some(c) = color_of(CMD, p, "color")? { grp.color = c; } if let Some(v) = whole_size(CMD, p, "markerSize", 1, 10)? { diff --git a/crates/engine/src/artboard_cmds.rs b/crates/engine/src/artboard_cmds.rs index 5cebe86aa9..4f551b2d77 100644 --- a/crates/engine/src/artboard_cmds.rs +++ b/crates/engine/src/artboard_cmds.rs @@ -92,10 +92,12 @@ fn preset_size(name: &str) -> Option<(u32, u32)> { } fn background_param(p: &Value, cmd: &str) -> Result> { - let custom = p.get("color").map(|_| { - let c = color_param(p, "color", [1.0; 4]); - ArtboardBackground::Custom(Color::rgba(c[0], c[1], c[2], c[3])) - }); + let custom = if p.get("color").is_some() { + let c = color_param(cmd, p, "color", [1.0; 4])?; + Some(ArtboardBackground::Custom(Color::rgba(c[0], c[1], c[2], c[3]))) + } else { + None + }; Ok(match p.get("background").and_then(Value::as_str) { None => custom, Some("white") => Some(ArtboardBackground::White), diff --git a/crates/engine/src/channel_cmds.rs b/crates/engine/src/channel_cmds.rs index 468db2698b..991f40dc0b 100644 --- a/crates/engine/src/channel_cmds.rs +++ b/crates/engine/src/channel_cmds.rs @@ -1086,17 +1086,19 @@ fn indicates_param(v: Option<&Value>) -> Option { } } -fn color_value(p: &Value, key: &str) -> Option { - p.get(key)?; - let c = crate::commands::color_param(p, key, [1.0, 0.0, 0.0, 1.0]); - Some(Color::rgb(c[0], c[1], c[2])) +fn color_value(cmd: &str, p: &Value, key: &str) -> Result> { + if p.get(key).is_none() { + return Ok(None); + } + let c = crate::commands::color_param(cmd, p, key, [1.0, 0.0, 0.0, 1.0])?; + Ok(Some(Color::rgb(c[0], c[1], c[2]))) } fn new_channel(s: &mut Session, p: &Value) -> Result { let fill = p.get("fill").and_then(Value::as_str).unwrap_or("black").to_string(); let name = p.get("name").and_then(Value::as_str).map(str::to_string); let indicates = indicates_param(p.get("indicates")); - let color = color_value(p, "color"); + let color = color_value("channel.new", p, "color")?; let opacity = p.get("opacity").and_then(Value::as_f64).map(|v| (v as f32 / 100.0).clamp(0.0, 1.0)); let i = s.edit("New Channel", |doc, _| { let area = doc.bounds(); @@ -1125,7 +1127,7 @@ fn new_channel(s: &mut Session, p: &Value) -> Result { } fn new_spot(s: &mut Session, p: &Value) -> Result { - let color = color_value(p, "color").unwrap_or(Color::rgb(1.0, 0.0, 0.0)); + let color = color_value("channel.newSpot", p, "color")?.unwrap_or(Color::rgb(1.0, 0.0, 0.0)); let solidity = p.get("solidity").and_then(Value::as_f64).unwrap_or(0.0).clamp(0.0, 100.0) as f32 / 100.0; let name = p.get("name").and_then(Value::as_str).map(str::to_string); let from_sel = p.get("fromSelection").and_then(Value::as_bool).unwrap_or(true); @@ -1421,7 +1423,7 @@ fn options(s: &mut Session, p: &Value) -> Result { let doc = &s.active().ok_or(EngineError::NoDocument)?.doc; let r = p.get("channel").and_then(|v| parse_ref(v, doc)).ok_or_else(|| bad(cmd, "missing or unknown `channel`"))?; let name = p.get("name").and_then(Value::as_str).map(str::trim).filter(|n| !n.is_empty()).map(str::to_string); - let color = color_value(p, "color"); + let color = color_value(cmd, p, "color")?; let opacity = p.get("opacity").and_then(Value::as_f64).map(|v| (v as f32 / 100.0).clamp(0.0, 1.0)); let solidity = p.get("solidity").and_then(Value::as_f64).map(|v| (v as f32 / 100.0).clamp(0.0, 1.0)); let kind = p.get("indicates").and_then(Value::as_str).map(str::to_string); diff --git a/crates/engine/src/color_to_alpha_cmds.rs b/crates/engine/src/color_to_alpha_cmds.rs index a6208e8246..81a60640fa 100644 --- a/crates/engine/src/color_to_alpha_cmds.rs +++ b/crates/engine/src/color_to_alpha_cmds.rs @@ -34,7 +34,7 @@ fn unit(v: f32, default: f32) -> f32 { /// Algorithm parameters (lenient: out-of-range values are clamped, unreadable ones fall back to /// the defaults). [`run`] validates strictly before it gets here. pub(crate) fn params(p: &Value) -> FilterParams { - let c = crate::commands::color_param(p, "color", WHITE); + let c = crate::commands::color_param(ID, p, "color", WHITE).unwrap_or(WHITE); let num = |k: &str, d: f32| p.get(k).and_then(Value::as_f64).map_or(d, |v| unit(v as f32, d)); FilterParams::ColorToAlpha { color: [unit(c[0], 1.0), unit(c[1], 1.0), unit(c[2], 1.0), 1.0], @@ -65,9 +65,10 @@ fn validate(p: &Value) -> Result<()> { crate::commands::parse_hex(s).ok_or_else(|| bad(format!("`color` must be \"#rrggbb\", not {s:?}")))?; } Some(Value::Array(a)) if (3..=4).contains(&a.len()) => { - if !a.iter().all(|v| v.as_f64().is_some_and(|x| x.is_finite() && (0.0..=1.0).contains(&x))) { - return Err(bad("`color` as an array must hold 3 or 4 numbers in 0..1")); - } + a.iter() + .all(|v| v.as_f64().is_some_and(|x| x.is_finite() && (0.0..=1.0).contains(&x))) + .then_some(()) + .ok_or_else(|| bad("`color` as an array must hold 3 or 4 numbers in 0..1"))?; } Some(_) => return Err(bad("`color` must be \"#rrggbb\" or [r, g, b] in 0..1")), } diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index 2f04187128..83b0717528 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -232,15 +232,53 @@ pub(crate) fn u32_id_param(cmd: &str, p: &Value, key: &str) -> Result f32 { p.get(key).and_then(Value::as_f64).map(|v| v as f32).unwrap_or(default) } -pub(crate) fn color_param(p: &Value, key: &str, default: [f32; 4]) -> [f32; 4] { - match p.get(key) { +/// JSON numbers may be finite f64 values but overflow when stored in a document's f32 field. +pub(crate) fn finite_f32(cmd: &str, key: &str, value: f64) -> Result { + let narrowed = value as f32; + if narrowed.is_finite() { Ok(narrowed) } else { Err(bad(cmd, format!("`{key}` must fit a finite 32-bit number"))) } +} + +/// Effect/type parameters contain nested colour arrays and snapshots as well as scalars. +/// Validate before parsing or editing, including numbers hidden in an effect snapshot. +pub(crate) fn check_f32_values(cmd: &str, value: &Value) -> Result<()> { + // Bound the call stack, without allocating a work list proportional to an input array. + fn check(cmd: &str, value: &Value, depth: usize) -> Result<()> { + if depth > 64 { + return Err(bad(cmd, "parameters nested deeper than 64 levels")); + } + match value { + Value::Number(n) => { + finite_f32(cmd, "number", n.as_f64().ok_or_else(|| bad(cmd, "invalid number"))?)?; + } + Value::Array(a) => { + for v in a { + check(cmd, v, depth + 1)?; + } + } + Value::Object(o) => { + for v in o.values() { + check(cmd, v, depth + 1)?; + } + } + _ => {} + } + Ok(()) + } + check(cmd, value, 0) +} + +pub(crate) fn color_param(cmd: &str, p: &Value, key: &str, default: [f32; 4]) -> Result<[f32; 4]> { + Ok(match p.get(key) { Some(Value::Array(a)) if a.len() >= 3 => { - let v: Vec = a.iter().map(|x| x.as_f64().unwrap_or(0.0) as f32).collect(); - [v[0], v[1], v[2], v.get(3).copied().unwrap_or(1.0)] + let mut out = [0.0, 0.0, 0.0, 1.0]; + for (slot, value) in out.iter_mut().zip(a) { + *slot = finite_f32(cmd, key, value.as_f64().unwrap_or(0.0))?; + } + out } Some(Value::String(s)) => parse_hex(s).unwrap_or(default), _ => default, - } + }) } pub(crate) fn parse_hex(s: &str) -> Option<[f32; 4]> { let s = s.trim_start_matches('#'); @@ -968,7 +1006,7 @@ fn build() -> Vec { r##"{"color":"#rrggbb"=foreground,"path":"work"|saved path name|{…path}? (the active path: becomes the vector mask, making a shape; else the selection is the layer mask)}"##, has_doc, |s, p| { - let c = color_param(p, "color", s.tools.foreground); + let c = color_param("layer.newFillLayer.solidColor", p, "color", s.tools.foreground)?; let id = s.edit("New Color Fill Layer", |doc, active| { let fill = Fill::Solid(Color::rgba(c[0], c[1], c[2], c[3])).in_mode(doc.mode); let mut l = Layer::new(doc.next_layer_name("Color Fill"), LayerContent::Fill(fill)); @@ -988,8 +1026,8 @@ fn build() -> Vec { r##"{"from":"#rrggbb","to":"#rrggbb","angle":deg=90,"style":"linear|radial|angle|reflected|diamond","reverse":bool,"path":"work"|saved path name|{…path}? (the active path: becomes the vector mask; else the selection is the layer mask)}"##, has_doc, |s, p| { - let a = color_param(p, "from", s.tools.foreground); - let b = color_param(p, "to", s.tools.background); + let a = color_param("layer.newFillLayer.gradient", p, "from", s.tools.foreground)?; + let b = color_param("layer.newFillLayer.gradient", p, "to", s.tools.background)?; let angle = f32_or(p, "angle", 90.0); let style = crate::layer_style::gradient_style(p.get("style").and_then(Value::as_str).unwrap_or("linear")); let reverse = p.get("reverse").and_then(Value::as_bool).unwrap_or(false); @@ -1120,8 +1158,10 @@ fn build() -> Vec { } }; let (fg_cmyk, bg_cmyk) = (cmyk("foregroundCmyk")?, cmyk("backgroundCmyk")?); - s.tools.foreground = color_param(p, "foreground", s.tools.foreground); - s.tools.background = color_param(p, "background", s.tools.background); + let foreground = color_param("tools.setColors", p, "foreground", s.tools.foreground)?; + let background = color_param("tools.setColors", p, "background", s.tools.background)?; + s.tools.foreground = foreground; + s.tools.background = background; if let Some(k) = fg_cmyk { let [r, g, b, _] = s.tools.foreground; s.tools.remember_cmyk([r, g, b], k); diff --git a/crates/engine/src/extra_cmds.rs b/crates/engine/src/extra_cmds.rs index c291862167..abd739b842 100644 --- a/crates/engine/src/extra_cmds.rs +++ b/crates/engine/src/extra_cmds.rs @@ -135,7 +135,7 @@ fn map_pixels(s: &mut Session, label: &str, p: &Value, f: impl FnOnce(Rect, &mut fn stroke(s: &mut Session, p: &Value) -> Result { // Whole pixels, rounded up, as in Photoshop (0.5 px strokes 1 px). let width = p.get("width").and_then(Value::as_f64).filter(|w| w.is_finite()).unwrap_or(1.0).clamp(1.0, 250.0).ceil() as usize; - let color = color_param(p, "color", s.tools.foreground); + let color = color_param("edit.stroke", p, "color", s.tools.foreground)?; // A colour picked as CMYK strokes a CMYK document with the picked inks (#4196). let picked = s.tools.picked_cmyk([color[0], color[1], color[2]]); let mode = match p.get("mode") { diff --git a/crates/engine/src/fill_cmds.rs b/crates/engine/src/fill_cmds.rs index 2aa0a02bdc..b8e6033cb3 100644 --- a/crates/engine/src/fill_cmds.rs +++ b/crates/engine/src/fill_cmds.rs @@ -106,7 +106,7 @@ pub fn fill(s: &mut Session, p: &Value) -> Result { let source = match contents { "foreground" => color_source(s, s.tools.foreground), "background" => color_source(s, s.tools.background), - "color" => color_source(s, color_param(p, "color", s.tools.foreground)), + "color" => color_source(s, color_param(CMD, p, "color", s.tools.foreground)?), "black" => Source::Color([0.0, 0.0, 0.0, 1.0]), "gray" => Source::Color([128.0 / 255.0, 128.0 / 255.0, 128.0 / 255.0, 1.0]), "white" => Source::Color([1.0; 4]), diff --git a/crates/engine/src/layer_style.rs b/crates/engine/src/layer_style.rs index 95b5476850..8933f838b9 100644 --- a/crates/engine/src/layer_style.rs +++ b/crates/engine/src/layer_style.rs @@ -594,6 +594,9 @@ fn overlay_gradient(g: &mut Gradient, p: &Value) { /// a dialog round trip never touches what it doesn't model: an imported /// gradient stroke keeps its gradient while its opacity is edited. pub fn overlay_effect(fx: &mut Effect, p: &Value) { + if crate::commands::check_f32_values("layer.layerStyle", p).is_err() { + return; + } fn set_color(p: &Value, k: &str, c: &mut Color) { if let Some(x) = color_of(p, k) { *c = x; @@ -821,6 +824,7 @@ pub fn overlay_effect(fx: &mut Effect, p: &Value) { /// Builds an effect of `kind` from JSON params (Photoshop units: opacity, /// spread, range in percent; sizes in px; angles in degrees). pub fn effect_from_params(kind: &str, p: &Value) -> Option { + crate::commands::check_f32_values("layer.layerStyle", p).ok()?; let mut fx = fresh_effect(kind)?; overlay_effect(&mut fx, p); Some(fx) @@ -856,6 +860,7 @@ const MAX_EFFECTS: usize = 256; /// mismatches and malformed snapshots are errors, never panics. fn replace_effects(s: &mut Session, p: &Value) -> Result { const CMD: &str = "layer.layerStyle.replace"; + crate::commands::check_f32_values(CMD, p)?; let id = crate::commands::layer_param(s, p)?; let entries = p.get("effects").and_then(Value::as_array).ok_or_else(|| bad(CMD, "need `effects`: [{kind, params, fx?}]"))?; if entries.len() > MAX_EFFECTS { @@ -905,6 +910,7 @@ fn replace_effects(s: &mut Session, p: &Value) -> Result { } fn set_effect(s: &mut Session, p: &Value, kind: &str) -> Result { + crate::commands::check_f32_values(&format!("layer.layerStyle.{kind}"), p)?; let fx = effect_from_params(kind, p).ok_or_else(|| EngineError::Other(format!("unknown effect {kind}")))?; let (fx, pattern) = crate::pattern_cmds::resolve_effect(s, fx)?; let label = format!("Layer Style: {}", fx.label()); @@ -936,6 +942,7 @@ fn make_default(s: &mut Session, p: &Value) -> Result { Some(v) if v.is_object() => v.clone(), _ => return Err(bad(CMD, "`params` must be an object")), }; + crate::commands::check_f32_values(CMD, ¶ms)?; s.presets.layer_defaults.insert(kind.to_string(), params); s.presets_changed(); Ok(json!({"kind": kind})) @@ -1152,17 +1159,25 @@ mod tests { /// blur kernel of 2^62 taps ("capacity overflow"), or ran for minutes at 1e6. The blur width /// is capped at the effect reach, so the document still renders, quickly. #[test] - fn oversized_satin_and_bevel_sizes_render() { - for (cmd, p) in [ - ("satin", json!({"size": 1e308})), - ("satin", json!({"size": 1_000_000})), - ("bevelEmboss", json!({"size": 1e308})), - ("bevelEmboss", json!({"soften": 1e308})), - ("bevelEmboss", json!({"size": 1_000_000, "soften": 1_000_000})), + fn oversized_satin_and_bevel_sizes_stay_saveable_and_render() { + for (cmd, p, accepted) in [ + ("satin", json!({"size": 1e308}), false), + ("satin", json!({"size": 1e30}), true), + ("satin", json!({"size": 1_000_000}), true), + ("bevelEmboss", json!({"size": 1e308}), false), + ("bevelEmboss", json!({"soften": 1e308}), false), + ("bevelEmboss", json!({"size": 1e30, "soften": 1e30}), true), + ("bevelEmboss", json!({"size": 1_000_000, "soften": 1_000_000}), true), ] { let mut s = session(); s.execute("edit.fill", json!({"color": "#ff0000"})).unwrap(); - s.execute(&format!("layer.layerStyle.{cmd}"), p.clone()).unwrap(); + let result = s.execute(&format!("layer.layerStyle.{cmd}"), p.clone()); + if accepted { + result.unwrap(); + } else { + assert!(matches!(result, Err(EngineError::BadParams { .. }))); + } + photocraft_format::save_to_bytes(&s.active().unwrap().doc, &Default::default()).unwrap(); let start = std::time::Instant::now(); s.execute("document.pixel", json!({"x": 0, "y": 0})).unwrap_or_else(|e| panic!("{cmd} {p}: {e}")); assert!(start.elapsed().as_secs() < 20, "{cmd} {p}: {:?}", start.elapsed()); diff --git a/crates/engine/src/lens_cmds.rs b/crates/engine/src/lens_cmds.rs index 379ca2402a..ff72fbb664 100644 --- a/crates/engine/src/lens_cmds.rs +++ b/crates/engine/src/lens_cmds.rs @@ -62,7 +62,7 @@ pub fn lens_params(cmd: &str, p: &Value, frame: Rect, info: Option<&exif::Camera "edgeExtension" | "extension" => EdgeMode::Extension, "black" => EdgeMode::Color([0.0, 0.0, 0.0, 1.0]), "white" => EdgeMode::Color([1.0, 1.0, 1.0, 1.0]), - "background" | "color" => EdgeMode::Color(crate::commands::color_param(p, "edgeColor", [0.0, 0.0, 0.0, 1.0])), + "background" | "color" => EdgeMode::Color(crate::commands::color_param(cmd, p, "edgeColor", [0.0, 0.0, 0.0, 1.0])?), other => return Err(bad(cmd, format!("unknown edge `{other}` (transparency|edgeExtension|black|white|color)"))), }; let focal = num(p, "focalLength", 0.0); diff --git a/crates/engine/src/notes_cmds.rs b/crates/engine/src/notes_cmds.rs index 54bbd60b7a..57116e4c8c 100644 --- a/crates/engine/src/notes_cmds.rs +++ b/crates/engine/src/notes_cmds.rs @@ -40,10 +40,12 @@ fn list_json(d: &Document) -> Value { json!({"notes": d.notes.iter().enumerate().map(|(i, n)| note_json(i, n)).collect::>()}) } -fn color_of(p: &Value) -> Option { - p.get("color")?; - let c = crate::commands::color_param(p, "color", [1.0, 1.0, 0.51, 1.0]); - Some(Color::rgb(c[0], c[1], c[2])) +fn color_of(cmd: &str, p: &Value) -> Result> { + if p.get("color").is_none() { + return Ok(None); + } + let c = crate::commands::color_param(cmd, p, "color", [1.0, 1.0, 0.51, 1.0])?; + Ok(Some(Color::rgb(c[0], c[1], c[2]))) } fn index_of(s: &Session, p: &Value, cmd: &str) -> Result { @@ -68,7 +70,7 @@ fn add(s: &mut Session, p: &Value) -> Result { modified: pdf_date(), ..Default::default() }; - if let Some(c) = color_of(p) { + if let Some(c) = color_of(CMD, p)? { n.color = c; } let i = s.edit("New Note", |d, _| { @@ -90,7 +92,7 @@ fn set(s: &mut Session, p: &Value) -> Result { if let Some(a) = p.get("author").and_then(Value::as_str) { n.author = a.to_string(); } - if let Some(c) = color_of(p) { + if let Some(c) = color_of(CMD, p)? { n.color = c; } if let Some(o) = p.get("open").and_then(Value::as_bool) { diff --git a/crates/engine/src/type_cmds.rs b/crates/engine/src/type_cmds.rs index d6b92e840c..8216b5d2c9 100644 --- a/crates/engine/src/type_cmds.rs +++ b/crates/engine/src/type_cmds.rs @@ -34,14 +34,14 @@ fn bad(cmd: &str, msg: impl Into) -> EngineError { } fn f32p(p: &Value, k: &str) -> Option { - p.get(k).and_then(Value::as_f64).map(|v| v as f32) + p.get(k).and_then(Value::as_f64).map(|v| v as f32).filter(|v| v.is_finite()) } fn color(v: &Value) -> Option { match v { Value::Array(a) if a.len() >= 3 => { let c: Vec = a.iter().map(|x| x.as_f64().unwrap_or(0.0) as f32).collect(); - Some(Color::rgba(c[0], c[1], c[2], c.get(3).copied().unwrap_or(1.0))) + c.iter().take(4).all(|v| v.is_finite()).then(|| Color::rgba(c[0], c[1], c[2], c.get(3).copied().unwrap_or(1.0))) } Value::String(s) => { let s = s.trim_start_matches('#'); @@ -743,6 +743,7 @@ fn info(s: &Session, p: &Value) -> Result { /// paragraph `box`, `orientation`, character and paragraph keys, starting from the default type /// styles in the foreground colour. Shared with the Type Mask tools' `type.createSelection`. pub(crate) fn new_text_layer(s: &Session, p: &Value, cmd: &str) -> Result { + crate::commands::check_f32_values(cmd, p)?; let orientation = match p.get("orientation") { None => Orientation::Horizontal, Some(Value::String(value)) if value == "horizontal" => Orientation::Horizontal, diff --git a/crates/engine/tests/finite_command_params.rs b/crates/engine/tests/finite_command_params.rs new file mode 100644 index 0000000000..0350d74493 --- /dev/null +++ b/crates/engine/tests/finite_command_params.rs @@ -0,0 +1,154 @@ +//! #6424: rejecting an overflowing f32 parameter must leave a saveable document and history. +use photocraft_engine::{EngineError, Session}; +use serde_json::{Value, json}; + +fn fresh(mode: &str, depth: u32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 16, "height": 12, "mode": mode, "depth": depth})).unwrap(); + s.execute("layer.new.layer", json!({})).unwrap(); + s +} + +fn saved(s: &Session) -> Vec { + photocraft_format::save_to_bytes(&s.active().unwrap().doc, &Default::default()).unwrap() +} + +fn rejected(cases: Vec<(&str, Value)>) { + let mut failures = Vec::new(); + for (id, p) in cases { + let mut s = fresh("rgb", 8); + let before = saved(&s); + let undo = s.active().unwrap().history.undo_label().map(str::to_owned); + let result = s.execute(id, p.clone()); + let save = photocraft_format::save_to_bytes(&s.active().unwrap().doc, &Default::default()); + if !matches!(result, Err(EngineError::BadParams { .. })) { + failures.push(format!("{id} {p}: result={result:?}, save error={:?}", save.as_ref().err())); + continue; + } + assert_eq!(save.unwrap(), before, "rejected {id} must not change the document"); + assert_eq!(s.active().unwrap().history.undo_label(), undo.as_deref(), "rejected {id} must not add history"); + } + assert!(failures.is_empty(), "{}", failures.join("\n")); +} + +#[test] +fn overflowing_layer_styles_are_rejected_before_mutation() { + let mut cases = Vec::new(); + for value in [1e39, -1e39] { + for (kind, key) in [ + ("dropShadow", "angle"), + ("innerShadow", "angle"), + ("outerGlow", "size"), + ("innerGlow", "size"), + ("stroke", "size"), + ("satin", "angle"), + ("bevelEmboss", "depth"), + ("gradientOverlay", "angle"), + ("patternOverlay", "phaseX"), + ] { + let id = match kind { + "dropShadow" => "layer.layerStyle.dropShadow", + "innerShadow" => "layer.layerStyle.innerShadow", + "outerGlow" => "layer.layerStyle.outerGlow", + "innerGlow" => "layer.layerStyle.innerGlow", + "stroke" => "layer.layerStyle.stroke", + "satin" => "layer.layerStyle.satin", + "bevelEmboss" => "layer.layerStyle.bevelEmboss", + "gradientOverlay" => "layer.layerStyle.gradientOverlay", + _ => "layer.layerStyle.patternOverlay", + }; + let params = json!({key: value}); + cases.push((id, params.clone())); + cases.push(("layer.layerStyle.replace", json!({"effects": [{"kind": kind, "params": params}]}))); + } + cases.push(("layer.layerStyle.colorOverlay", json!({"color": [value, 0, 0]}))); + } + rejected(cases); +} + +#[test] +fn overflowing_document_colors_are_rejected_before_mutation() { + let mut cases = Vec::new(); + for id in ["layer.newFillLayer.solidColor", "channel.new", "channel.newSpot", "layer.new.artboard", "count.newGroup"] { + for value in [1e39, -1e39] { + for component in 0..4 { + let mut color = [0.5; 4]; + color[component] = value; + cases.push((id, json!({"color": color}))); + } + } + } + rejected(cases); +} + +#[test] +fn overflowing_type_positions_and_colors_are_rejected_before_mutation() { + rejected(vec![ + ("type.create", json!({"text": "hello", "x": 1e39})), + ("type.create", json!({"text": "hello", "y": -1e39})), + ("type.create", json!({"text": "hello", "color": [1e39, 0, 0]})), + ]); +} + +#[test] +fn valid_color_and_style_edits_still_save_and_undo_at_each_depth() { + for mode in ["rgb", "gray", "cmyk", "lab"] { + for depth in [8, 16, 32] { + let mut s = fresh(mode, depth); + let before = saved(&s); + s.execute("layer.layerStyle.dropShadow", json!({"angle": -45, "color": [0.1, 0.3, 0.8]})).unwrap(); + let edited = saved(&s); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(saved(&s), before); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(saved(&s), edited); + s.execute("layer.newFillLayer.solidColor", json!({"color": "#4080c0"})).unwrap(); + let bytes = saved(&s); + photocraft_format::load_from_bytes(&bytes).unwrap(); + } + } +} + +#[test] +fn effect_snapshots_cannot_bypass_numeric_validation() { + let mut fx = serde_json::to_value(photocraft_engine::layer_style::effect_from_params("dropShadow", &json!({})).unwrap()).unwrap(); + fx["DropShadow"]["angle"] = json!(1e39); + rejected(vec![( + "layer.layerStyle.replace", + json!({"effects": [ + {"kind": "colorOverlay", "params": {"color": "#00ff00"}}, + {"kind": "dropShadow", "fx": fx, "params": {}} + ]}), + )]); +} + +#[test] +fn direct_effect_helpers_do_not_introduce_infinity() { + let original = photocraft_engine::layer_style::effect_from_params("dropShadow", &json!({})).unwrap(); + for p in [json!({"angle": 1e39}), json!({"color": [0, -1e39, 0]})] { + assert!(photocraft_engine::layer_style::effect_from_params("dropShadow", &p).is_none()); + let mut edited = original.clone(); + photocraft_engine::layer_style::overlay_effect(&mut edited, &p); + assert_eq!(edited, original); + } +} + +#[test] +fn bad_background_color_does_not_partially_change_tool_colors() { + let mut s = fresh("rgb", 32); + let before = (s.tools.foreground, s.tools.background); + let result = s.execute("tools.setColors", json!({"foreground": "#40a0c0", "background": [0, 1e39, 0]})); + assert!(matches!(result, Err(EngineError::BadParams { .. }))); + assert_eq!((s.tools.foreground, s.tools.background), before); +} + +#[test] +fn finite_hdr_colors_are_preserved_without_unit_clamping() { + let mut s = fresh("rgb", 32); + let id = s.execute("layer.newFillLayer.solidColor", json!({"color": [4.0, -0.5, 0.25]})).unwrap()["layer"].as_u64().unwrap(); + let bytes = saved(&s); + let reopened = photocraft_format::load_from_bytes(&bytes).unwrap(); + let layer = reopened.layer(photocraft_doc::LayerId(id)).unwrap(); + let photocraft_doc::LayerContent::Fill(photocraft_doc::Fill::Solid(c)) = &layer.content else { panic!("expected a solid fill") }; + assert_eq!(c.to_rgb(), [4.0, -0.5, 0.25]); +} From f59b75b058b954e55d6df67b7d8521536e66ec7b Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 16 Oct 2026 09:35:06 +0200 Subject: [PATCH 27/36] Character and Paragraph panels: size, leading, baseline shift and indents follow the Type units preference (fixes #6491) --- crates/ui-egui/src/type_tool.rs | 36 ++++++++++++++------------- crates/ui-egui/src/type_tool_tests.rs | 21 ++++++++++++++++ 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/crates/ui-egui/src/type_tool.rs b/crates/ui-egui/src/type_tool.rs index d73bb2e2c4..f95b1bdf1b 100644 --- a/crates/ui-egui/src/type_tool.rs +++ b/crates/ui-egui/src/type_tool.rs @@ -2130,6 +2130,8 @@ fn type_sections(app: &mut PhotocraftApp, ui: &mut egui::Ui, character: bool, pa use crate::props_layout::{COL_GAP, LABEL_GAP, LABEL_W, field_width, section}; use crate::theme::ROW_GAP; let Some((c, para)) = styles_at(app) else { return }; + // Preferences ▸ Units & Rulers ▸ Type, as in the options bar: the model keeps points. + let (u, unit) = type_unit(app); let t = crate::theme::Tokens::get(ui.ctx()); // Row helper: `ui.horizontal` with the column gap, then the shared row gap. let row = |ui: &mut egui::Ui, add: &mut dyn FnMut(&mut egui::Ui)| { @@ -2167,13 +2169,13 @@ fn type_sections(app: &mut PhotocraftApp, ui: &mut egui::Ui, character: bool, pa let k = shown_scale(app); // Hover tips stay English until wired through `tl!` in a multi-locale change // (`every_tl_literal_is_translated` requires every complete catalog). - if let Some(v) = num_field(ui, "tT", "Font size", c.size_pt * k, 0.1..=1296.0, "pt", w) { - app.ui.tool_options.type_size = v; - apply(app, ui.ctx(), json!({"size": v, "metricsAsShown": true})); + if let Some(v) = num_field(ui, "tT", "Font size", c.size_pt * k * u, 0.1 * u..=1296.0 * u, unit, w) { + app.ui.tool_options.type_size = v / u; + apply(app, ui.ctx(), json!({"size": v / u, "metricsAsShown": true})); } let lead = c.leading_pt.unwrap_or(c.size_pt * para.auto_leading.max(0.01)) * k; - if let Some(v) = num_field(ui, "A↕", "Leading (set to the font size × auto-leading when Auto)", lead, 0.1..=5000.0, "pt", w) { - apply(app, ui.ctx(), json!({"leading": v, "metricsAsShown": true})); + if let Some(v) = num_field(ui, "A↕", "Leading (set to the font size × auto-leading when Auto)", lead * u, 0.1 * u..=5000.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"leading": v / u, "metricsAsShown": true})); } }); row(ui, &mut |ui| { @@ -2198,8 +2200,8 @@ fn type_sections(app: &mut PhotocraftApp, ui: &mut egui::Ui, character: bool, pa } }); row(ui, &mut |ui| { - if let Some(v) = num_field(ui, "Aª", "Baseline shift", c.baseline_shift_pt, -1296.0..=1296.0, "pt", w) { - apply(app, ui.ctx(), json!({"baselineShift": v})); + if let Some(v) = num_field(ui, "Aª", "Baseline shift", c.baseline_shift_pt * u, -1296.0 * u..=1296.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"baselineShift": v / u})); } // Colour: label + a swatch filling the rest of the second column. ui.horizontal(|ui| { @@ -2306,24 +2308,24 @@ fn type_sections(app: &mut PhotocraftApp, ui: &mut egui::Ui, character: bool, pa }); ui.add_space(ROW_GAP); row(ui, &mut |ui| { - if let Some(v) = num_field(ui, "→|", "Indent left margin", para.start_indent_pt, -1296.0..=1296.0, "pt", w) { - apply(app, ui.ctx(), json!({"startIndent": v})); + if let Some(v) = num_field(ui, "→|", "Indent left margin", para.start_indent_pt * u, -1296.0 * u..=1296.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"startIndent": v / u})); } - if let Some(v) = num_field(ui, "|←", "Indent right margin", para.end_indent_pt, -1296.0..=1296.0, "pt", w) { - apply(app, ui.ctx(), json!({"endIndent": v})); + if let Some(v) = num_field(ui, "|←", "Indent right margin", para.end_indent_pt * u, -1296.0 * u..=1296.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"endIndent": v / u})); } }); row(ui, &mut |ui| { - if let Some(v) = num_field(ui, "¶→", "Indent first line", para.first_line_indent_pt, -1296.0..=1296.0, "pt", w) { - apply(app, ui.ctx(), json!({"firstLineIndent": v})); + if let Some(v) = num_field(ui, "¶→", "Indent first line", para.first_line_indent_pt * u, -1296.0 * u..=1296.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"firstLineIndent": v / u})); } }); row(ui, &mut |ui| { - if let Some(v) = num_field(ui, "↑¶", "Add space before paragraph", para.space_before_pt, 0.0..=1296.0, "pt", w) { - apply(app, ui.ctx(), json!({"spaceBefore": v})); + if let Some(v) = num_field(ui, "↑¶", "Add space before paragraph", para.space_before_pt * u, 0.0 * u..=1296.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"spaceBefore": v / u})); } - if let Some(v) = num_field(ui, "¶↓", "Add space after paragraph", para.space_after_pt, 0.0..=1296.0, "pt", w) { - apply(app, ui.ctx(), json!({"spaceAfter": v})); + if let Some(v) = num_field(ui, "¶↓", "Add space after paragraph", para.space_after_pt * u, 0.0 * u..=1296.0 * u, unit, w) { + apply(app, ui.ctx(), json!({"spaceAfter": v / u})); } }); let mut hy = para.hyphenate; diff --git a/crates/ui-egui/src/type_tool_tests.rs b/crates/ui-egui/src/type_tool_tests.rs index 7a23c9ccc3..c2931fc463 100644 --- a/crates/ui-egui/src/type_tool_tests.rs +++ b/crates/ui-egui/src/type_tool_tests.rs @@ -1549,6 +1549,27 @@ mod multi_layer_formatting { assert_eq!(text(h.state(), ids[1]).char_runs(), peer); } + /// #6491: with Units & Rulers ▸ Type set to pixels, the Character and Paragraph fields show + /// and take pixels, like the options bar. 144 ppi so a pixel is not a point. + #[test] + fn character_and_paragraph_fields_follow_the_type_units_preference() { + use egui_kittest::kittest::NodeT; + let (mut app, ids) = fixture(); + app.run("prefs.set", json!({"path": "unitsAndRulers.typeUnits", "value": "pixels"})).unwrap(); + app.run("image.imageSize", json!({"resolution": 144, "resample": "none"})).unwrap(); + selected(&mut app, &[ids[0]]); + let mut h = controls(app, true); + let shown = h.get_by_role_and_label(Role::SpinButton, "Font size").accesskit_node().numeric_value(); + assert_eq!(shown, Some(24.0), "12 pt at 144 ppi"); + number(&mut h, "Font size", "60"); + number(&mut h, "Leading (set to the font size × auto-leading when Auto)", "84"); + number(&mut h, "Baseline shift", "10"); + number(&mut h, "Indent left margin", "9"); + let c = style(h.state(), ids[0], 0); + assert_eq!((c.size_pt, c.leading_pt, c.baseline_shift_pt), (30.0, Some(42.0), 5.0)); + assert_eq!(text(h.state(), ids[0]).paragraph_runs()[0].style.start_indent_pt, 4.5); + } + /// #5717: a layer scaled so its shown size or leading is past a field's range (2400 pt, or /// 0.08 pt) keeps it while the Character panel and the options bar only show it. #[test] From 9c914a9fd29bf27d8e1ecd72203578cc52412c8e Mon Sep 17 00:00:00 2001 From: jonathanwxh-cell Date: Fri, 16 Oct 2026 15:58:41 +0800 Subject: [PATCH 28/36] Refuse Affinity spot colours nested more than 4 levels deep paint::color followed a spot colour's RGB equivalent and base colour with no depth limit. Colours are shared objects, so a spot colour that links to itself overflowed the stack and aborted the import, and a chain whose levels link to the next through both took exponential time. Stop after 4 levels of spot colours defined through spot colours, with a warning. Co-Authored-By: Claude Opus 5.5 --- crates/affinity/README.md | 2 +- crates/affinity/src/paint.rs | 17 +++- crates/affinity/src/tests.rs | 171 +++++++++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 2 deletions(-) diff --git a/crates/affinity/README.md b/crates/affinity/README.md index 3e4e066156..e881352c8b 100644 --- a/crates/affinity/README.md +++ b/crates/affinity/README.md @@ -139,7 +139,7 @@ zstd window, 4096 saved revisions, array lengths no longer than the bytes left, 16 777 216 decoded values per document stream (fields and array elements together, counted before anything is allocated: a value takes about 40 bytes in memory however few it took in the file; the largest of those 189 public documents uses 3.3 million), 384 levels of object nesting, 128 levels of layers, 500 000 layers, four million curve -nodes per curve, 64 megapixels per pixel layer (cropped to its content first) and 1024 gradient +nodes per curve, 4 levels of spot colours defined through spot colours, 64 megapixels per pixel layer (cropped to its content first) and 1024 gradient stops; `photocraft-io` adds 300 000 pixels a side per document and 64 megapixels per image on the canvas. Every archive entry's CRC-32 and size must match. Malformed input returns an `Error`; the crate has no panics outside tests. diff --git a/crates/affinity/src/paint.rs b/crates/affinity/src/paint.rs index c4ebff8446..343c276b60 100644 --- a/crates/affinity/src/paint.rs +++ b/crates/affinity/src/paint.rs @@ -134,8 +134,19 @@ fn floats(v: &Value) -> Option> { } } +/// Spot colours defined through spot colours. A spot colour (`Pant`) is read through its RGB +/// equivalent, else its base colour, which in real files hold plain colours. Colours are shared +/// objects, so one can link to itself, and with both links followed at every level a colour read +/// costs up to 2^(depth+1) lookups. +const MAX_SPOT_DEPTH: usize = 4; + /// A colour object (`RGBA`, `HSLA`, `CMYK`, `GRAY`, `LABA`, `Pant`). pub(crate) fn color(r: &mut Reader, id: ObjId) -> Option { + color_at(r, id, 0) +} + +/// [`color`] of an object `depth` spot colours down. +fn color_at(r: &mut Reader, id: ObjId, depth: usize) -> Option { let s = r.s; let class = s.class(id)?; let col = || -> Option> { @@ -166,8 +177,12 @@ pub(crate) fn color(r: &mut Reader, id: ObjId) -> Option { Color::Lab { l: 100.0 * u(0)?, a: 255.0 * u(1)? - 128.0, b: 255.0 * u(2)? - 128.0, alpha: u(3)? } } b"Pant" => { + if depth >= MAX_SPOT_DEPTH { + r.warn("spot colours nested deeper than 4 levels (left empty)"); + return None; + } r.warn("spot colours (imported as their RGB equivalent)"); - return s.obj(id, b"srgb").and_then(|c| color(r, c)).or_else(|| s.obj(id, b"base").and_then(|c| color(r, c))); + return s.obj(id, b"srgb").and_then(|c| color_at(r, c, depth + 1)).or_else(|| s.obj(id, b"base").and_then(|c| color_at(r, c, depth + 1))); } b"RegC" => Color::Cmyk { c: 1.0, m: 1.0, y: 1.0, k: 1.0, a: 1.0 }, _ => { diff --git a/crates/affinity/src/tests.rs b/crates/affinity/src/tests.rs index 59ae99044a..2c68a064a3 100644 --- a/crates/affinity/src/tests.rs +++ b/crates/affinity/src/tests.rs @@ -538,3 +538,174 @@ mod pixels { ); } } + +/// Spot colours that link to themselves or to each other. Each document is read on a 1 MiB +/// stack, the smallest main-thread stack PhotoCraft runs on (Windows), first in a child process +/// that must finish within 20 s. A stack overflow aborts the process it happens in, so unbounded +/// recursion fails the one test that caused it instead of taking the other tests in the binary +/// down with it. +mod spot_colours { + use std::io::{Read, Write}; + use std::process::{Command, Stdio}; + use std::time::{Duration, Instant}; + + use crate::container::Limits; + use crate::model::{Document, Kind}; + use crate::paint::{Color, Paint}; + use crate::stream::Tag; + use crate::synth::{self, F, Method, tag}; + + /// A one-page document whose page holds `top`, read in a child process first and then here. + fn read(top: Vec) -> Document { + let spread = F::Def(2, vec![tag(b"Sprd")], vec![(tag(b"SprB"), F::F64s(vec![0.0, 0.0, 400.0, 300.0])), (tag(b"Chld"), F::Shared(top))]); + let data = synth::stream(&[(tag(b"DocR"), F::Def(1, vec![tag(b"DocN")], vec![(tag(b"Chld"), F::Shared(vec![spread]))]))]); + let bytes = synth::container(&[("doc.dat", &data, Method::Zlib)], None); + let test = std::thread::current().name().unwrap_or("reader").to_owned(); + read_in_child(&bytes, &test); + read_on_small_stack(bytes, &test).unwrap() + } + + /// Reads on a 1 MiB thread named after the test, so a stack overflow message names the test. + fn read_on_small_stack(bytes: Vec, test: &str) -> Result { + let reader = std::thread::Builder::new().name(test.to_owned()).stack_size(1 << 20).spawn(move || crate::read(&bytes, Limits::default())).unwrap(); + reader.join().unwrap_or_else(|panic| std::panic::resume_unwind(panic)) + } + + /// Set in the child process to the name of the test it reads for. + const CHILD: &str = "PHOTOCRAFT_AFFINITY_READ_CHILD"; + + /// Runs `read_child` in a new process of this test binary on `bytes` and fails the test if + /// that process crashes, panics or takes over 20 s, or never read the document. + fn read_in_child(bytes: &[u8], test: &str) { + let mut child = Command::new(std::env::current_exe().unwrap()) + .args(["tests::spot_colours::read_child", "--exact", "--ignored", "--nocapture", "--test-threads=1"]) + .env(CHILD, test) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let drain = |mut pipe: Box| { + std::thread::spawn(move || { + let mut text = String::new(); + let _ = pipe.read_to_string(&mut text); + text + }) + }; + let stdout = drain(Box::new(child.stdout.take().unwrap())); + let stderr = drain(Box::new(child.stderr.take().unwrap())); + // Fed from a thread, so a child that never reads cannot block the deadline below; a child + // that dies first breaks the pipe and is reported by its exit status. + let (mut stdin, input) = (child.stdin.take().unwrap(), bytes.to_vec()); + let feed = std::thread::spawn(move || { + let _ = stdin.write_all(&input); + }); + let deadline = Instant::now() + Duration::from_secs(20); + let status = loop { + if let Some(status) = child.try_wait().unwrap() { + break Some(status); + } + if Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + break None; + } + std::thread::sleep(Duration::from_millis(10)); + }; + feed.join().unwrap(); + let output = format!("{}{}", stdout.join().unwrap(), stderr.join().unwrap()); + match status { + None => panic!("reading the document did not finish within 20 s:\n{output}"), + Some(status) if !status.success() => panic!("reading the document failed in a child process ({status}):\n{output}"), + // A filter that matched no test would also exit successfully, having read nothing. + Some(_) => assert!(output.contains(&format!("read {} bytes", bytes.len())), "the child process did not read the document:\n{output}"), + } + } + + /// Not a test of its own: `read_in_child` runs it in a child process to read the document on + /// its stdin. Without the variable, as when run with `--ignored`, it does nothing. + #[test] + #[ignore = "run by read_in_child in a child process"] + fn read_child() { + let Some(test) = std::env::var_os(CHILD) else { return }; + let mut bytes = Vec::new(); + std::io::stdin().read_to_end(&mut bytes).unwrap(); + let len = bytes.len(); + let _ = read_on_small_stack(bytes, &test.to_string_lossy()); + println!("read {len} bytes"); + } + + /// An ellipse filled with `colour`. + fn filled(id: u32, colour: F) -> F { + F::Def( + id, + vec![tag(b"ShpN")], + vec![ + (tag(b"ShpB"), F::F64s(vec![0.0, 0.0, 1.0, 1.0])), + (tag(b"Shpe"), F::Def(id + 1_000_000, vec![tag(b"ShpE")], vec![])), + (tag(b"BFFl"), F::Shared(vec![F::Def(id + 2_000_000, vec![tag(b"FilS")], vec![(tag(b"Colr"), colour)])])), + ], + ) + } + + fn spot(id: u32, fields: Vec<(Tag, F)>) -> F { + F::Def(id, vec![tag(b"Pant")], fields) + } + + fn fills(doc: &Document) -> &[Paint] { + match &doc.spreads[0].nodes[0].kind { + Kind::Shape { fills, .. } => fills, + other => panic!("expected a shape, got {other:?}"), + } + } + + fn warned(doc: &Document, what: &str) -> bool { + doc.warnings.iter().any(|w| w.contains(what)) + } + + #[test] + fn a_spot_colour_that_is_its_own_rgb_equivalent_is_refused() { + let doc = read(vec![filled(10, spot(20, vec![(tag(b"srgb"), F::Ref(20))]))]); + assert!(fills(&doc).is_empty(), "{:?}", fills(&doc)); + assert!(warned(&doc, "spot colours nested deeper than 4 levels"), "{:?}", doc.warnings); + } + + #[test] + fn spot_colours_that_are_each_others_base_and_rgb_equivalent_are_refused() { + let doc = read(vec![filled(10, spot(20, vec![(tag(b"base"), spot(21, vec![(tag(b"srgb"), F::Ref(20))]))]))]); + assert!(fills(&doc).is_empty(), "{:?}", fills(&doc)); + assert!(warned(&doc, "spot colours nested deeper than 4 levels"), "{:?}", doc.warnings); + } + + #[test] + fn spot_colours_of_spot_colours_do_not_take_exponential_time() { + // Each level's RGB equivalent and base are the next level, and the last is in an unknown + // model, so both are followed everywhere: 2^41 colour reads before the fix. + fn level(id: u32, left: u32) -> F { + if left == 0 { + return F::Def(id, vec![tag(b"XXXX")], vec![]); + } + spot(id, vec![(tag(b"srgb"), level(id + 1, left - 1)), (tag(b"base"), F::Ref(id + 1))]) + } + let doc = read(vec![filled(10, level(100, 40))]); + assert!(fills(&doc).is_empty(), "{:?}", fills(&doc)); + } + + /// `spots` spot colours, each defined through the next, around an opaque RGB colour. + fn spots(id: u32, spots: u32) -> F { + if spots == 0 { + let rgba: Vec = [1.0f32, 0.5, 0.25, 1.0].iter().flat_map(|c| c.to_le_bytes()).collect(); + return F::Def(id, vec![tag(b"RGBA")], vec![(tag(b"_col"), F::Struct(rgba))]); + } + spot(id, vec![(tag(b"srgb"), self::spots(id + 1, spots - 1))]) + } + + #[test] + fn spot_colours_nest_four_levels_and_no_deeper() { + let doc = read(vec![filled(10, spots(100, 4))]); + assert_eq!(fills(&doc), [Paint::Solid(Color::Rgb { r: 1.0, g: 0.5, b: 0.25, a: 1.0 })]); + let doc = read(vec![filled(10, spots(100, 5))]); + assert!(fills(&doc).is_empty(), "{:?}", fills(&doc)); + assert!(warned(&doc, "spot colours nested deeper than 4 levels"), "{:?}", doc.warnings); + } +} From 4d6f3f4d1e2d87056898d1ae589d0f298afb03d4 Mon Sep 17 00:00:00 2001 From: highperfocused Date: Fri, 16 Oct 2026 11:31:52 +0200 Subject: [PATCH 29/36] style: cargo fmt (review on #6499) --- apps/photocraft/src/main.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/apps/photocraft/src/main.rs b/apps/photocraft/src/main.rs index 337399d99a..085adff597 100644 --- a/apps/photocraft/src/main.rs +++ b/apps/photocraft/src/main.rs @@ -289,7 +289,7 @@ mod control_port_tests { #[cfg(test)] mod launch_args_tests { - use super::{control_args_exit_code, parse_launch_args, LaunchArgs}; + use super::{LaunchArgs, control_args_exit_code, parse_launch_args}; use std::ffi::OsString; fn parse(values: &[&str]) -> LaunchArgs { @@ -312,11 +312,7 @@ mod launch_args_tests { #[test] fn missing_value_does_not_clear_an_environment_default() { // `--automation-read-root` with no value must not drop a root set through the env var. - let defaults = LaunchArgs { - automation_read_root: Some("/env/read".into()), - automation_write_root: Some("/env/write".into()), - ..Default::default() - }; + let defaults = LaunchArgs { automation_read_root: Some("/env/read".into()), automation_write_root: Some("/env/write".into()), ..Default::default() }; let parsed = parse_launch_args(["--automation-read-root"].iter().map(OsString::from), defaults); assert_eq!(parsed.automation_read_root.as_deref(), Some(std::path::Path::new("/env/read"))); assert_eq!(parsed.automation_write_root.as_deref(), Some(std::path::Path::new("/env/write"))); @@ -325,7 +321,24 @@ mod launch_args_tests { #[test] fn values_are_parsed_and_no_errors_remain() { - let parsed = parse(&["--control", "50494", "--control-token", "abcd", "--control-token-file", "/tmp/token", "--automation-read-root", "/r", "--automation-write-root", "/w", "--safe-gpu", "--in-window-menus", "--new-instance", "--present-mode", "mailbox", "photo.psd"]); + let parsed = parse(&[ + "--control", + "50494", + "--control-token", + "abcd", + "--control-token-file", + "/tmp/token", + "--automation-read-root", + "/r", + "--automation-write-root", + "/w", + "--safe-gpu", + "--in-window-menus", + "--new-instance", + "--present-mode", + "mailbox", + "photo.psd", + ]); assert!(parsed.errors.is_empty(), "{:?}", parsed.errors); assert_eq!(parsed.control_port, Some(50494)); assert_eq!(parsed.control_token.as_deref(), Some("abcd")); From 1302e08791731d88506f64634308d3f9035eeb8e Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:04:34 +0200 Subject: [PATCH 30/36] Arrange: the shown tiles stay put when a visible document is activated (#6017) With more documents than tiles, the run of shown documents was worked out from the active index alone, so after activating document 2 in Two-up, a click into the visible document 1 moved the tiles back to [0, 1]. The run is now kept from frame to frame and slides only when a hidden document becomes active. --- crates/ui-egui/src/canvas.rs | 8 +++++--- crates/ui-egui/src/lib.rs | 4 ++++ crates/ui-egui/src/view_cmds/tests.rs | 8 +++++++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 05ee6ba855..cd6d3739c3 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2058,9 +2058,10 @@ pub(crate) fn arranged_cells(app: &PhotocraftApp, rect: Rect) -> Option>() + let shown: Vec = crate::canvas::arranged_cells(app, rect).unwrap().iter().map(|(d, _)| *d).collect(); + app.arrange_start = shown[0]; + shown }; assert_eq!(docs(&mut app, 0), vec![0, 1]); assert_eq!(docs(&mut app, 1), vec![0, 1], "the second tile's document stays on the right"); assert_eq!(docs(&mut app, 2), vec![1, 2], "the third document slides in next to the second"); + assert_eq!(docs(&mut app, 1), vec![1, 2], "a click into the visible document 1 leaves the tiles as they are"); + assert_eq!(docs(&mut app, 0), vec![0, 1], "a hidden document slides in from the left"); + assert_eq!(docs(&mut app, 1), vec![0, 1]); } /// #6017: rulers showed in the active tile only; the other tile had none and no room for them. From 158dcc673197132bfd0404d0c2c5c7b05cc30111 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Sat, 17 Oct 2026 04:30:14 +0000 Subject: [PATCH 31/36] Fix engine compile error: load export presets from presets JSON Fixes #6911 Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 (cherry picked from commit bb7a5f23091fb4d23c02d253ea78b20f10f142fb) --- crates/engine/src/presets/mod.rs | 1 + crates/engine/src/presets/tests.rs | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/crates/engine/src/presets/mod.rs b/crates/engine/src/presets/mod.rs index 03696291dd..81382b418a 100644 --- a/crates/engine/src/presets/mod.rs +++ b/crates/engine/src/presets/mod.rs @@ -186,6 +186,7 @@ impl Session { tool_presets: take(&mut m, "tool_presets"), documents: take(&mut m, "documents"), recent_documents: take(&mut m, "recent_documents"), + export_presets: take(&mut m, "export_presets"), custom_shapes: take(&mut m, "custom_shapes"), layer_defaults: take(&mut m, "layer_defaults"), swatches: take(&mut m, "swatches"), diff --git a/crates/engine/src/presets/tests.rs b/crates/engine/src/presets/tests.rs index b7c0a3ede2..1cde39abe5 100644 --- a/crates/engine/src/presets/tests.rs +++ b/crates/engine/src/presets/tests.rs @@ -774,3 +774,21 @@ fn a_damaged_preset_set_keeps_the_other_sets() { assert_eq!(next.presets.gradients.len(), Session::new().presets.gradients.len()); assert_eq!(next.presets.documents, s.presets.documents); } + +// ------------------------------------------------------------------ export presets + +#[test] +fn export_presets_survive_a_presets_json_round_trip() { + let mut s = Session::new(); + s.execute("file.export.presets.save", json!({"name": "Web", "settings": {"format": "jpg", "quality": 70}})).unwrap(); + assert_eq!(s.presets.export_presets.len(), 1); + + let saved = s.presets.to_json(&s); + assert_eq!(saved["export_presets"].as_array().map(Vec::len), Some(1)); + + let mut restored = Session::new(); + restored.load_presets_json(saved); + assert_eq!(restored.presets.export_presets.len(), 1); + assert_eq!(restored.presets.export_presets[0].name, "Web"); + assert_eq!(restored.presets.export_presets[0].settings.format, "jpg"); +} From 3bca0123b04f46f3092b348005b34dac2e9274a7 Mon Sep 17 00:00:00 2001 From: itsF4LCON <107556180+itsF4LCON@users.noreply.github.com> Date: Sat, 17 Oct 2026 18:12:29 +0200 Subject: [PATCH 32/36] Type: the blink-phase test finds the caret by its strokes arrow_keys_reset_caret_blink_phase (#5512) looked for a 1.5-wide line, the old caret. The caret is now CARET_STROKES (a 3 pt white halo and a 1 pt black line), so the test matches those. --- crates/ui-egui/src/type_tool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ui-egui/src/type_tool.rs b/crates/ui-egui/src/type_tool.rs index 88119f917f..44c0c9b033 100644 --- a/crates/ui-egui/src/type_tool.rs +++ b/crates/ui-egui/src/type_tool.rs @@ -2732,7 +2732,7 @@ mod tests { draw_overlay(app, &painter, &xf); }); out.textures_delta.clear(); - out.shapes.iter().any(|s| matches!(&s.shape, egui::Shape::LineSegment { stroke, .. } if (stroke.width - 1.5).abs() < 1e-4)) + out.shapes.iter().any(|s| matches!(&s.shape, egui::Shape::LineSegment { stroke, .. } if CARET_STROKES.contains(stroke))) }; // Before resetting, the caret is in the off phase at t = 10.5 s (10500 / 530 = 19, odd phase) From 0c36fbc49d094a7ea017becc30f1eb564a8d8376 Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 16 Oct 2026 14:04:13 +0000 Subject: [PATCH 33/36] Integration: #6035 move the new tests above existing tests' doc comments --- crates/engine/src/image_cmds.rs | 11 ++++++----- crates/ui-egui/src/sizing.rs | 5 +++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index 5f37a2eaa8..69fb1b17c1 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -944,11 +944,7 @@ mod tests { } } - /// The canvas border stays opaque after Image Size (it used to fade into transparency, so a - /// Background or a 200 % export got a translucent frame), and a full selection stays full. - /// #1544: 64 x 48 to 300000 x 300000 (90 billion pixels) started allocating resampled bands - /// without a budget. It is refused before any allocation and the document is untouched; a - /// change of resolution only, and an ordinary resize, still work. + /// #6019: Image Size on an Indexed Color document makes no colours the colour table doesn't have. #[test] fn image_size_resamples_indexed_color_with_nearest_neighbor() { let mut s = Session::new(); @@ -970,6 +966,11 @@ mod tests { } } + /// The canvas border stays opaque after Image Size (it used to fade into transparency, so a + /// Background or a 200 % export got a translucent frame), and a full selection stays full. + /// #1544: 64 x 48 to 300000 x 300000 (90 billion pixels) started allocating resampled bands + /// without a budget. It is refused before any allocation and the document is untouched; a + /// change of resolution only, and an ordinary resize, still work. #[test] fn image_size_refuses_a_raster_past_the_budget() { let mut s = Session::new(); diff --git a/crates/ui-egui/src/sizing.rs b/crates/ui-egui/src/sizing.rs index c3ec7a199b..d0e0cfdec4 100644 --- a/crates/ui-egui/src/sizing.rs +++ b/crates/ui-egui/src/sizing.rs @@ -525,8 +525,7 @@ mod tests { assert!(app.ui.dialogs.iter().any(|x| x.id == id), "reset leaves the dialog open"); } - /// The dialog starts on Preferences › General › Image Interpolation: Bicubic Automatic opens - /// as Automatic and the others as themselves (Bicubic Sharper used to open as Lanczos). + /// #6019: on an Indexed Color document the dialog opens on, and offers only, Automatic and Nearest Neighbor. #[test] fn indexed_color_resamples_with_nearest_neighbor_only() { let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); @@ -555,6 +554,8 @@ mod tests { } } + /// The dialog starts on Preferences › General › Image Interpolation: Bicubic Automatic opens + /// as Automatic and the others as themselves (Bicubic Sharper used to open as Lanczos). #[test] fn resample_follows_the_interpolation_preference() { let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); From fac74f83e8dd35b084e47d39c55d2dfb8db388d9 Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 16 Oct 2026 14:05:02 +0000 Subject: [PATCH 34/36] Integration: #6414 an unconfirmed save releases the unsaved-changes prompt --- crates/ui-egui/src/discard_ui.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/discard_ui.rs b/crates/ui-egui/src/discard_ui.rs index 3dbf33c905..fe75cadc3e 100644 --- a/crates/ui-egui/src/discard_ui.rs +++ b/crates/ui-egui/src/discard_ui.rs @@ -214,7 +214,8 @@ fn save(app: &mut PhotocraftApp, ctx: &egui::Context, doc: DocId) -> bool { } match saved { // A Save Document script may have edited the document after it was written: still dirty. - Ok(_) => !app.save_options_open() && !app.saving() && index_of(app, doc).is_some_and(|i| !app.session.documents()[i].is_dirty()), + // A write the platform can't confirm (a web download) leaves it dirty too, and counts. + Ok(_) => !app.save_options_open() && !app.saving() && index_of(app, doc).is_some_and(|i| app.services.write_unconfirmed || !app.session.documents()[i].is_dirty()), // Backing out of the file dialog is the user's choice, not an error. Err(e) if e == crate::file_dialog::CANCELLED => false, Err(e) => { @@ -227,12 +228,12 @@ fn save(app: &mut PhotocraftApp, ctx: &egui::Context, doc: DocId) -> bool { /// A completed save may release the close prompt. Choosing a path only starts a layered TIFF /// save; TIFF Options calls this after the write, and `jobs_ui` when a background save ends. -/// Copies and failed writes leave the doc dirty. +/// Copies and failed writes leave the doc dirty; so does any write when it is unconfirmed. pub(crate) fn saved_document(app: &mut PhotocraftApp, ctx: &egui::Context, doc: DocId) { // Still the prompt that asked (quitting may have replaced it meanwhile). if !app.save_options_open() && app.discard.as_ref().and_then(|p| p.docs.first()) == Some(&doc) - && index_of(app, doc).is_some_and(|i| !app.session.documents()[i].is_dirty()) + && index_of(app, doc).is_some_and(|i| app.services.write_unconfirmed || !app.session.documents()[i].is_dirty()) { advance(app, ctx); } @@ -924,6 +925,30 @@ mod tests { assert!(h.state().session.documents().is_empty(), "closed once saved"); } + /// #6414: on the web a save stays dirty (the download isn't confirmed), and answering Save + /// still closes the tab after one write. + #[test] + fn saving_from_the_prompt_closes_after_an_unconfirmed_write() { + use std::{cell::RefCell, rc::Rc}; + let written: Rc>> = Rc::default(); + let w = written.clone(); + let mut app = app_with_docs(1); + make_dirty(&mut app, 0); + app.session.active_mut().unwrap().path = Some("/pics/kept.psd".into()); + app.services.write_unconfirmed = true; + app.services.export = Some(Box::new(|_: &photocraft_doc::Document, _: &str, _: &crate::ExportSettings| Ok((b"out".to_vec(), Vec::new())))); + app.services.write = Some(Box::new(move |p: &str, _: &[u8]| { + w.borrow_mut().push(p.to_string()); + Ok(()) + })); + let mut h = prompt_for(app, "file.close", egui::os::OperatingSystem::Windows); + h.key_press(Key::Y); + h.run_steps(3); + assert_eq!(*written.borrow(), ["/pics/kept.psd"]); + assert!(h.state().discard.is_none()); + assert!(h.state().session.documents().is_empty(), "closed after one write"); + } + #[path = "tiff_tests.rs"] mod tiff_tests; From 0b167ca2599957b794baadec9b947146a6a114e4 Mon Sep 17 00:00:00 2001 From: aiongg Date: Sat, 17 Oct 2026 12:16:37 +0000 Subject: [PATCH 35/36] Integration: #6476's uppercase token test writes its file owner-only (#5459 on main) --- crates/automation/src/security.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/automation/src/security.rs b/crates/automation/src/security.rs index 14aadbca53..89a760b32a 100644 --- a/crates/automation/src/security.rs +++ b/crates/automation/src/security.rs @@ -404,7 +404,7 @@ mod tests { fn an_uppercase_token_file_authenticates_its_exact_contents() { let path = std::env::temp_dir().join(format!("photocraft-control-token-upper-{}-{}.txt", std::process::id(), generate_token().unwrap())); let contents = generate_token().unwrap().to_ascii_uppercase(); - std::fs::write(&path, format!("{contents}\n")).unwrap(); + create_token_file(&path, &contents).unwrap(); let expected = server_token(None, Some(&path)); std::fs::remove_file(&path).unwrap(); let (reply, authenticated) = authentication_reply(&auth_line(&contents), &expected.unwrap()); From 5a01650652c2d24ee49810272a03d9c2a05db786 Mon Sep 17 00:00:00 2001 From: aiongg Date: Sun, 18 Oct 2026 13:21:01 +0000 Subject: [PATCH 36/36] Integration: cargo fmt --- crates/io/src/svg.rs | 13 ++----------- crates/ui-egui/src/discard_ui.rs | 6 +++++- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/io/src/svg.rs b/crates/io/src/svg.rs index 3d66f45e94..8afefb733d 100644 --- a/crates/io/src/svg.rs +++ b/crates/io/src/svg.rs @@ -230,17 +230,8 @@ fn import_parsed_with_budget(name: &str, parsed: Parsed, max_group_depth: usize, let fmt = doc.pixel_format(); let global = tiny_skia::Transform::from_scale(cv.scale, cv.scale); let max_group_depth = (max_group_depth != 0).then_some(max_group_depth.clamp(1, MAX_GROUP_DEPTH)); - let mut conv = Conv { - fmt, - canvas: doc.bounds(), - global, - max_group_depth, - warnings, - noted: Vec::new(), - counts: [0; 4], - pixel_budget, - retained_pixel_bytes: 0, - }; + let mut conv = + Conv { fmt, canvas: doc.bounds(), global, max_group_depth, warnings, noted: Vec::new(), counts: [0; 4], pixel_budget, retained_pixel_bytes: 0 }; if count_nodes(tree.root()) > MAX_NODES { conv.warnings.push(format!("SVG: more than {MAX_NODES} elements; opened as one pixel layer instead of shapes")); let (w, h) = (cv.size.width, cv.size.height); diff --git a/crates/ui-egui/src/discard_ui.rs b/crates/ui-egui/src/discard_ui.rs index fe75cadc3e..82e4497d92 100644 --- a/crates/ui-egui/src/discard_ui.rs +++ b/crates/ui-egui/src/discard_ui.rs @@ -215,7 +215,11 @@ fn save(app: &mut PhotocraftApp, ctx: &egui::Context, doc: DocId) -> bool { match saved { // A Save Document script may have edited the document after it was written: still dirty. // A write the platform can't confirm (a web download) leaves it dirty too, and counts. - Ok(_) => !app.save_options_open() && !app.saving() && index_of(app, doc).is_some_and(|i| app.services.write_unconfirmed || !app.session.documents()[i].is_dirty()), + Ok(_) => { + !app.save_options_open() + && !app.saving() + && index_of(app, doc).is_some_and(|i| app.services.write_unconfirmed || !app.session.documents()[i].is_dirty()) + } // Backing out of the file dialog is the user's choice, not an error. Err(e) if e == crate::file_dialog::CANCELLED => false, Err(e) => {