From 31790939856b59834a5500a71c4b156ece210f09 Mon Sep 17 00:00:00 2001 From: Armaan Sandhu <74664101+Ar-maan05@users.noreply.github.com> Date: Thu, 8 Oct 2026 14:30:43 -0400 Subject: [PATCH 1/7] Motion Blur: apply the samples as one merged kernel over whole rows (#902) Motion Blur took distance + 1 bilinear samples per pixel, each through Image::sample with per-tap edge checks, so distance 100 cost about 400 edge-checked reads per pixel and channel. The sample offsets are the same for every pixel, so merge their bilinear taps once into a fixed kernel (each tap an integer offset and a weight) and apply it tap by tap as a multiply-add over whole rows of a premultiplied window, which the compiler vectorizes. This is the same premultiplied mean as before, summed in a different order. --- crates/algo/src/blur.rs | 110 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 102 insertions(+), 8 deletions(-) diff --git a/crates/algo/src/blur.rs b/crates/algo/src/blur.rs index 9d2fe45001..2ca3bcc88b 100644 --- a/crates/algo/src/blur.rs +++ b/crates/algo/src/blur.rs @@ -348,19 +348,83 @@ fn average_samples(src: &Image, out: Rect, ctx: &Ctx, mut offsets: impl FnMut(f3 res } -pub(crate) fn motion(src: &Image, out: Rect, ctx: &Ctx, angle: f32, distance: f32) -> Vec { +/// Sample offsets of a motion blur of `distance` at `angle`, relative to the pixel centre. +fn motion_offsets(angle: f32, distance: f32) -> Vec<(f32, f32)> { let d = distance.abs(); - if d < 0.5 { - return src.crop(out); - } let (s, c) = angle.to_radians().sin_cos(); let steps = d.ceil() as i32; - average_samples(src, out, ctx, |x, y, pts| { - for i in 0..=steps { + (0..=steps) + .map(|i| { let t = i as f32 / steps as f32 - 0.5; - pts.push((x + c * d * t, y - s * d * t)); + (c * d * t, -s * d * t) + }) + .collect() +} + +/// Motion blur: the mean of bilinear samples along a line centred on the pixel. The offsets are the +/// same for every pixel, so their bilinear taps are merged once into a fixed kernel, which is +/// applied tap by tap to whole rows of a premultiplied window (the same premultiplied mean the +/// per-sample loop takes, summed in another order). +pub(crate) fn motion(src: &Image, out: Rect, ctx: &Ctx, angle: f32, distance: f32) -> Vec { + if distance.abs() < 0.5 { + return src.crop(out); + } + let offs = motion_offsets(angle, distance); + let k = 1.0 / offs.len() as f32; + // Merged taps (dx, dy, weight), in the order first seen. + let mut taps: Vec<(i32, i32, f32)> = Vec::new(); + let mut slot = std::collections::HashMap::new(); + for &(ox, oy) in &offs { + let (x0, y0) = (ox.floor(), oy.floor()); + let (ax, ay) = (ox - x0, oy - y0); + let (x0, y0) = (x0 as i32, y0 as i32); + for (dx, dy, w) in [(0, 0, (1.0 - ax) * (1.0 - ay)), (1, 0, ax * (1.0 - ay)), (0, 1, (1.0 - ax) * ay), (1, 1, ax * ay)] { + if w <= 0.0 { + continue; + } + let i = *slot.entry((x0 + dx, y0 + dy)).or_insert_with(|| { + taps.push((x0 + dx, y0 + dy, 0.0)); + taps.len() - 1 + }); + taps[i].2 += w * k; } - }) + } + let reach = taps.iter().map(|t| t.0.abs().max(t.1.abs())).max().unwrap_or(0); + let n = src.ch; + let win = out.inflate(reach); + let ww = win.width() as usize; + // Premultiplied window; outside the source reads as transparent, as the sampler does. + let mut pw = vec![0.0f32; ww * win.height() as usize * n]; + for y in win.y0..win.y1 { + for x in win.x0..win.x1 { + let i = ((y - win.y0) as usize * ww + (x - win.x0) as usize) * n; + let a = if ctx.alpha { src.get(x, y, n - 1) } else { 1.0 }; + for c in 0..n { + let v = src.get(x, y, c); + pw[i + c] = if ctx.alpha && c < n - 1 { v * a } else { v }; + } + } + } + let (ow, oh) = (out.width() as usize, out.height() as usize); + let mut res = vec![0.0f32; ow * oh * n]; + let r = reach as usize; + for (oy, row) in res.chunks_exact_mut(ow * n).enumerate() { + for &(dx, dy, w) in &taps { + let start = ((oy + r).wrapping_add_signed(dy as isize) * ww + r.wrapping_add_signed(dx as isize)) * n; + for (o, v) in row.iter_mut().zip(&pw[start..start + ow * n]) { + *o += w * v; + } + } + if ctx.alpha { + for px in row.chunks_exact_mut(n) { + let a = px[n - 1]; + for v in px.iter_mut().take(n - 1) { + *v = if a > 1e-7 { *v / a } else { 0.0 }; + } + } + } + } + res } pub(crate) fn radial(src: &Image, out: Rect, ctx: &Ctx, amount: f32, method: RadialMethod, center: (f32, f32)) -> Vec { @@ -702,4 +766,34 @@ mod tests { img.data[i] = 0.5 / 255.0; assert!(surface_8bit(&img, out, &ctx, 4, 0.1, 26).is_none()); } + + #[test] + fn motion_kernel_matches_the_per_pixel_samples() { + // Noisy RGBA with some transparent pixels; the output reaches the source edge. + let src_rect = Rect::new(-30, -30, 50, 40); + let mut img = Image::new(src_rect, 4); + let mut s = 0x1b87_3593u32; + for px in img.data.as_chunks_mut::<4>().0 { + for v in px.iter_mut() { + s ^= s << 13; + s ^= s >> 17; + s ^= s << 5; + *v = (s % 1000) as f32 / 999.0; + } + if s.is_multiple_of(5) { + px[3] = 0.0; + } + } + let out = Rect::new(-12, -6, 30, 22); + for alpha in [true, false] { + let ctx = Ctx { bounds: src_rect, mode: crate::ColorMode::Rgb, alpha }; + for (angle, distance) in [(0.0, 1.0), (0.0, 9.0), (30.0, 7.0), (90.0, 12.0), (-45.0, 20.0), (137.0, 33.5), (200.0, 40.0)] { + let fast = motion(&img, out, &ctx, angle, distance); + let offs = motion_offsets(angle, distance); + let slow = average_samples(&img, out, &ctx, |x, y, pts| pts.extend(offs.iter().map(|&(dx, dy)| (x + dx, y + dy)))); + let err = slow.iter().zip(&fast).map(|(a, b)| (a - b).abs()).fold(0.0f32, f32::max); + assert!(err < 1e-5, "alpha {alpha} angle {angle} distance {distance}: max error {err}"); + } + } + } } From 8f5a0817e4ad3ffd7893f97eb8149674bb923752 Mon Sep 17 00:00:00 2001 From: Ian Swope Date: Thu, 8 Oct 2026 13:30:50 -0500 Subject: [PATCH 2/7] Applying a text data set re-renders the type layer instead of blanking it (#990) (#1025) apply_to_doc replaced a text-replacement variable's text and set the layer's render cache to None, but nothing rebuilt it. The compositor draws a type layer from that cache, so the text vanished from the canvas after image.applyDataSet and from every file written by file.export.dataSetsAsFiles. The layer is now re-rendered with type_cmds::refresh, as type.edit does. --- crates/engine/src/variables_cmds.rs | 5 ++++- crates/engine/src/variables_cmds/tests.rs | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/variables_cmds.rs b/crates/engine/src/variables_cmds.rs index 2cf30e7dd4..2500797f4d 100644 --- a/crates/engine/src/variables_cmds.rs +++ b/crates/engine/src/variables_cmds.rs @@ -164,13 +164,16 @@ fn apply_to_doc(doc: &mut Document, vars: &Variables, set: &DataSet) -> Result<( } } (VarKind::TextReplacement, VarValue::Text(text)) => { + let snapshot = doc.clone(); if let Some(l) = doc.layer_mut(def.layer) && let LayerContent::Text(t) = &mut l.content { t.text = text.clone(); - t.cache = None; // force re-render t.runs.clear(); // re-flow as one run from the summary style t.paragraphs.clear(); + // The compositor draws a type layer from its cache: clearing it left the + // layer blank until some other edit re-rendered it (#990). + crate::type_cmds::refresh(&snapshot, t); } } (VarKind::PixelReplacement { method, align, clip }, VarValue::Pixels(path)) => { diff --git a/crates/engine/src/variables_cmds/tests.rs b/crates/engine/src/variables_cmds/tests.rs index 55ff20540d..69301add02 100644 --- a/crates/engine/src/variables_cmds/tests.rs +++ b/crates/engine/src/variables_cmds/tests.rs @@ -190,3 +190,25 @@ fn pixel_replacement_changes_the_layer() { assert!(b > r, "replacement is blue-ish: {rgba:?}"); } } + +#[test] +fn applied_text_is_rendered_in_the_document_and_in_exports() { + // Applying a text value cleared the type layer's render cache and nothing rebuilt it, so the + // text vanished from the canvas and from every exported file (#990). + let mut s = Session::new(); + s.execute("file.new", json!({"width": 200, "height": 60, "background": "white"})).unwrap(); + let id = s.execute("type.create", json!({"x": 4, "y": 40, "text": "HELLO", "size": 32, "color": "#000000"})).unwrap()["layer"].clone(); + let dark = |d: &Document| photocraft_compose::render(d, d.bounds()).px.iter().filter(|p| p[0] < 0.5).count(); + assert!(dark(doc(&s)) > 0); + s.execute("image.variables.define", json!({"defs": [{"name": "headline", "layer": id, "type": "textReplacement"}]})).unwrap(); + s.execute("image.variables.dataSets", json!({"dataSets": [{"name": "row1", "values": [{"variable": "headline", "kind": "text", "value": "BYE"}]}]})) + .unwrap(); + let dir = tmp("text-export"); + let out = s.execute("file.export.dataSetsAsFiles", json!({"dir": dir, "format": "png"})).unwrap(); + let path = out["files"][0].as_str().unwrap(); + let exported = photocraft_io::import(path, &std::fs::read(path).unwrap()).unwrap().document; + assert!(dark(&exported) > 0, "the exported row keeps its text"); + s.execute("image.applyDataSet", json!({"name": "row1"})).unwrap(); + assert!(dark(doc(&s)) > 0, "the applied text is drawn"); + let _ = std::fs::remove_dir_all(&dir); +} From 317d946f45550d98bde2bf3f765f80246d044aa8 Mon Sep 17 00:00:00 2001 From: wstock Date: Thu, 8 Oct 2026 19:30:56 +0100 Subject: [PATCH 3/7] PSD: a spec-order mask record is not misread as Photoshop's order when that reading is implausible (#949) (#1129) real_before_parameters picked Photoshop's order (real mask first) whenever the record length fit the parameter-flags byte at offset 36, even when that byte was just part of the real rectangle. A density-only record in the spec's order then read its density as the real mask's background and lost the parameter. When both orders fit, Photoshop's reading now has to be plausible (background 0 or 255, known parameter flag bits) or the spec's reading with a real mask wins. Co-authored-by: wstock <1394687+wstock@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 --- crates/psd/src/layer.rs | 63 +++++++++++++++++++++++++++++++++-- crates/psd/tests/roundtrip.rs | 27 +++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/crates/psd/src/layer.rs b/crates/psd/src/layer.rs index a0c6bd5ffb..2ecf0df6f3 100644 --- a/crates/psd/src/layer.rs +++ b/crates/psd/src/layer.rs @@ -214,11 +214,31 @@ impl LayerMask { /// Whether `d` (a whole mask record with mask parameters) stores the real-mask fields before /// the parameters, as Photoshop does. The spec's order wins when it reads as parameters - /// followed by padding alone (no real mask), the one layout the two orders can share. + /// followed by padding alone (no real mask), the one layout the two orders can share. When + /// both orders fit the record's length, Photoshop's is taken unless its reading is + /// implausible (a real-mask background that is neither 0 nor 255, or parameter flag bits + /// the spec does not define) while the spec's reading with a real mask is sound. pub(crate) fn real_before_parameters(d: &[u8]) -> bool { + // Parameters at `pf_at`, then 0..=3 bytes of padding, and nothing else. let fits = |pf_at: usize| d.get(pf_at).is_some_and(|&pf| (pf_at + 1 + mask_parameter_bytes(pf)..=pf_at + 4 + mask_parameter_bytes(pf)).contains(&d.len())); - !fits(18) && fits(36) + if fits(18) { + return false; + } + let photoshop = fits(36); + if !photoshop { + return false; + } + let known_flags = |pf: u8| pf & 0xF0 == 0; + let real_background_ok = |at: usize| d.get(at + 1).is_some_and(|&bg| bg == 0 || bg == 255); + // Photoshop's reading: real-mask fields at 18, parameters at 36. + let photoshop_plausible = real_background_ok(18) && d.get(36).is_some_and(|&pf| known_flags(pf)); + // The spec's reading: parameters at 18, then the 18 real-mask bytes, then padding. + let spec_plausible = d.get(18).is_some_and(|&pf| { + let real_at = 19 + mask_parameter_bytes(pf); + known_flags(pf) && (real_at + 18..=real_at + 21).contains(&d.len()) && real_background_ok(real_at) + }); + photoshop_plausible || !spec_plausible } fn parse(d: &[u8]) -> Result { @@ -845,6 +865,45 @@ mod tests { assert_eq!(out, d); } + /// A density-only record in the spec's order (#949): the parameter flags byte at 18 says one + /// value byte, so the spec's reading excludes the 38-byte length only if the real mask is + /// ignored, while byte 36 (inside the real rectangle's `right`) is 0 and makes Photoshop's + /// reading fit too. Photoshop's reading puts the density (64) where a 0-or-255 background + /// belongs, so the spec's order wins and the density survives. + #[test] + fn density_only_spec_layout_is_not_read_as_photoshop_order() { + for (density, right) in [(64u8, 1u32), (200, 1), (64, 300), (1, 7)] { + let mut d = Vec::new(); + Rect::from_xywh(0, 0, 1, 1).write(&mut d); + d.extend([255, LayerMask::FLAG_PARAMETERS]); + d.extend([0b0001, density]); + d.extend([0, 0]); + Rect::from_xywh(0, 0, right, 1).write(&mut d); + let m = LayerMask::parse(&d).unwrap(); + assert!(!m.real_first, "density {density}, right {right}: {m:?}"); + assert_eq!(m.parameters.unwrap().user_density, Some(density)); + assert_eq!(m.real, Some(RealMask { flags: 0, background: 0, rect: Rect::from_xywh(0, 0, right, 1) })); + assert!(m.trailing.is_empty()); + let mut out = Vec::new(); + m.write(&mut out); + assert_eq!(out, d); + } + // Photoshop's order with the same lengths still reads as Photoshop's order. + let mut d = Vec::new(); + Rect::from_xywh(0, 0, 1, 1).write(&mut d); + d.extend([255, LayerMask::FLAG_PARAMETERS]); + d.extend([0, 255]); + Rect::from_xywh(0, 0, 1, 1).write(&mut d); + d.extend([0b0001, 64]); + let m = LayerMask::parse(&d).unwrap(); + assert!(m.real_first, "{m:?}"); + assert_eq!(m.parameters.unwrap().user_density, Some(64)); + assert_eq!(m.real.map(|r| r.background), Some(255)); + let mut out = Vec::new(); + m.write(&mut out); + assert_eq!(out, d); + } + /// The spec's order (parameters, then real fields) still parses and stays byte-stable. #[test] fn real_mask_after_parameters_spec_layout() { diff --git a/crates/psd/tests/roundtrip.rs b/crates/psd/tests/roundtrip.rs index 8666ba3cc4..1f23ee6284 100644 --- a/crates/psd/tests/roundtrip.rs +++ b/crates/psd/tests/roundtrip.rs @@ -285,3 +285,30 @@ fn save_and_open() { assert_eq!(g, f); let _ = std::fs::remove_dir_all(&dir); } + +/// A density-only mask record written in the spec's order (parameters, then the real-mask +/// fields) used to be read back in Photoshop's order, which moved the density into the real +/// mask's background and dropped it from the parameters (#949). The model must survive the +/// round trip, not only the bytes. +#[test] +fn spec_order_density_only_mask_round_trips_as_a_model() { + let mut f = testgen::small(Version::Psd, Compression::Raw); + let layer = &mut f.layers_mut()[0]; + layer.mask = MaskData::Mask(LayerMask { + rect: Rect::from_xywh(0, 0, 1, 1), + default_color: 255, + flags: LayerMask::FLAG_PARAMETERS, + parameters: Some(MaskParameters { flags: 1, user_density: Some(64), ..Default::default() }), + real: Some(RealMask { flags: 0, background: 0, rect: Rect::from_xywh(0, 0, 1, 1) }), + trailing: vec![], + real_first: false, + }); + // The generated 2 x 2 mask channel is replaced by one that matches the 1 x 1 rectangle. + let mask_channel = layer.channels.iter_mut().find(|c| c.id == -2).expect("the generated layer has a mask channel"); + *mask_channel = ChannelData::encode(-2, Compression::Raw, &[64], 1, 1, 8, Version::Psd).unwrap(); + let b = f.to_bytes().unwrap(); + let g = PsdFile::from_bytes(&b).unwrap(); + assert_eq!(g, f, "model round trip"); + assert_eq!(g.layers()[0].decode_channel(-2, 8, Version::Psd).unwrap(), vec![64]); + assert_eq!(g.to_bytes().unwrap(), b, "byte stability"); +} From f5080f9774571c0698a8ea8c07b986a6fc356d68 Mon Sep 17 00:00:00 2001 From: Elvis33LE <67311580+Elvis33LE@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:31:03 +0200 Subject: [PATCH 4/7] ui.window.open: an invalid document index is an error, not a side effect (#1133) The document parameter's set_active result was discarded, so an out-of-range index still switched nothing, changed nothing, and the window opened for whatever document happened to be active - a caller was told success for a document that does not exist. Now the call errors with the index and leaves the active document alone. --- crates/ui-egui/src/control.rs | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index cf99a88276..96e8693bf6 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -397,8 +397,10 @@ fn dispatch(app: &mut PhotocraftApp, ctx: &egui::Context, req: &ControlRequest) None => err("no such dialog"), }, "ui.window.open" => { - if let Some(d) = u("document") { - app.session.set_active(d as usize); + if let Some(d) = u("document") + && !app.session.set_active(d as usize) + { + return err(format!("no document {d}")); } wrap(crate::menus::invoke(app, ctx, "window.newWindowForDocument", json!({}))) } @@ -805,6 +807,22 @@ mod tests { } } + #[test] + fn ui_window_open_rejects_an_unknown_document_index() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + let ctx = egui::Context::default(); + app.run("file.new", json!({"width": 8, "height": 8})).unwrap(); + // An out-of-range index must error instead of silently switching the active document + // and opening the window for whatever is active now. + let before = app.session.active_index(); + let r = call(&mut app, &ctx, "ui.window.open", json!({"document": 99})); + assert_eq!(r["ok"], false, "{r}"); + assert!(r["error"].as_str().unwrap().contains("no document 99"), "{r}"); + assert_eq!(app.session.active_index(), before, "the active document is untouched"); + // A valid index still opens the window. + assert_eq!(call(&mut app, &ctx, "ui.window.open", json!({"document": 0}))["ok"], true); + } + #[test] fn ui_set_brush_size_dispatches_a_journaled_brush_command() { let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); From f9ccdda1e25251a36326fc4787b7640a4af07316 Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:31:10 +0700 Subject: [PATCH 5/7] fix(ui): remember previously applied New Guide Layout values (#1261) * Remember last applied New Guide Layout parameters in UI state * Save guide layout preferences only after successful application * Cover successful, cancelled and failed guide layout dialog state * Avoid overlapping borrows when reopening guide layout dialog --- crates/ui-egui/src/dialogs.rs | 6 +++- crates/ui-egui/src/view_cmds.rs | 10 ++++++- crates/ui-egui/src/view_cmds/tests.rs | 40 +++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 5e2dfa2898..88727983cf 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -324,7 +324,11 @@ pub fn confirm(app: &mut PhotocraftApp, id: u64) -> Result { app.filter_preview = None; let cmd = d.fields.get("__command").and_then(|v| v.as_str().map(str::to_string)).ok_or("dialog has no command")?; let (cmd, params) = crate::smart_ui::confirm_command(&d.fields, cmd, crate::filter_dialog::params_of(&d.fields)); - app.run(&cmd, params) + let result = app.run(&cmd, params.clone()); + if result.is_ok() && cmd == "view.newGuideLayout" { + app.ui.view.guide_layout = params; + } + result } DialogKind::LayerStyle => crate::layer_style::confirm(app, &d.fields), DialogKind::About | DialogKind::Error => Ok(Value::Null), diff --git a/crates/ui-egui/src/view_cmds.rs b/crates/ui-egui/src/view_cmds.rs index 1184ea0910..1072b3eccf 100644 --- a/crates/ui-egui/src/view_cmds.rs +++ b/crates/ui-egui/src/view_cmds.rs @@ -94,6 +94,12 @@ pub struct ViewOptions { /// and the Middle Eastern & South Asian composer. pub language_features: String, pub middle_eastern_composer: bool, + /// Parameters from the last successfully applied New Guide Layout dialog. + pub guide_layout: Value, +} + +fn default_guide_layout() -> Value { + json!({"columns": 8, "gutter": 20, "rows": 0, "rowGutter": 0, "margin": 0, "centerColumns": false, "clearExisting": false}) } impl Default for ViewOptions { @@ -112,6 +118,7 @@ impl Default for ViewOptions { font_preview_size: "medium".into(), language_features: "defaultFeatures".into(), middle_eastern_composer: false, + guide_layout: default_guide_layout(), } } } @@ -846,7 +853,8 @@ fn front(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option { - dialog(app, json!({"columns": 8, "gutter": 20, "rows": 0, "rowGutter": 0, "margin": 0, "centerColumns": false, "clearExisting": false}), json!({})) + let fields = app.ui.view.guide_layout.clone(); + dialog(app, fields, json!({})) } "type.warpText" => { let styles: Vec<&str> = std::iter::once("none").chain(photocraft_text::warp::STYLES.iter().map(|(_, s)| *s)).collect(); diff --git a/crates/ui-egui/src/view_cmds/tests.rs b/crates/ui-egui/src/view_cmds/tests.rs index eaabdddd66..f5519b806c 100644 --- a/crates/ui-egui/src/view_cmds/tests.rs +++ b/crates/ui-egui/src/view_cmds/tests.rs @@ -172,3 +172,43 @@ fn engine_commands_open_their_dialogs() { crate::menus::invoke_unguarded(&mut app, &ctx, "file.closeAll", json!({})).unwrap(); assert!(app.ui.views.is_empty() && app.session.documents().is_empty()); } + +#[test] +fn new_guide_layout_remembers_successful_values_not_cancelled_edits() { + let (mut app, ctx) = app_with(1); + let first = menu(&mut app, &ctx, "view.newGuideLayout", json!({})).unwrap()["dialog"].as_u64().unwrap(); + assert_eq!(app.ui.dialog_mut(first).unwrap().fields["columns"], 8); + assert_eq!(app.ui.dialog_mut(first).unwrap().fields["gutter"], 20); + for (key, value) in [("columns", json!(2)), ("gutter", json!(0)), ("rows", json!(2)), ("rowGutter", json!(0))] { + app.ui.dialog_mut(first).unwrap().fields.insert(key.into(), value); + } + crate::dialogs::confirm(&mut app, first).unwrap(); + + let second = menu(&mut app, &ctx, "view.newGuideLayout", json!({})).unwrap()["dialog"].as_u64().unwrap(); + let fields = &app.ui.dialog_mut(second).unwrap().fields; + assert_eq!(fields["columns"], 2); + assert_eq!(fields["gutter"], 0); + assert_eq!(fields["rows"], 2); + assert_eq!(fields["rowGutter"], 0); + assert_eq!(fields["clearExisting"], false); + // A cancelled edit must not replace the last successfully used values. + app.ui.dialog_mut(second).unwrap().fields.insert("columns".into(), json!(9)); + app.ui.close_dialog(second); + let third = menu(&mut app, &ctx, "view.newGuideLayout", json!({})).unwrap()["dialog"].as_u64().unwrap(); + assert_eq!(app.ui.dialog_mut(third).unwrap().fields["columns"], 2); + + // The remembered state is part of the serializable UI state. + let json_state = serde_json::to_value(&app.ui).unwrap(); + assert_eq!(json_state["view"]["guide_layout"]["columns"], 2); + let restored: crate::state::UiState = serde_json::from_value(json_state).unwrap(); + assert_eq!(restored.view.guide_layout["rows"], 2); +} + +#[test] +fn new_guide_layout_does_not_remember_a_failed_command() { + let (mut app, _) = app_with(0); + let id = front(&mut app, "view.newGuideLayout", &json!({})).unwrap().unwrap()["dialog"].as_u64().unwrap(); + app.ui.dialog_mut(id).unwrap().fields.insert("columns".into(), json!(3)); + assert!(crate::dialogs::confirm(&mut app, id).is_err()); + assert_eq!(app.ui.view.guide_layout["columns"], 8); +} From 5054308d0b887a0456a5e1ba238383fbb4e43ef0 Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:31:17 +0700 Subject: [PATCH 6/7] fix(ui): allow removing the last polygonal lasso point (#1262) * Allow removing last polygonal lasso vertex without editing layers * Route lasso Backspace ahead of destructive edit shortcuts * Use right-click to retract polygonal lasso points instead of opening menu * Regress lasso vertex removal via keyboard and actual canvas right click * Apply rustfmt layout reported by Linux CI * Apply rustfmt layout reported by Linux CI --- crates/ui-egui/src/canvas.rs | 3 ++ crates/ui-egui/src/lasso_ui.rs | 23 +++++++++++++ crates/ui-egui/src/lasso_ui/tests.rs | 51 ++++++++++++++++++++++++++++ crates/ui-egui/src/shortcuts.rs | 9 +++++ 4 files changed, 86 insertions(+) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 7e93f02b26..259db97037 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1817,12 +1817,14 @@ pub fn canvas_view(app: &mut PhotocraftApp, ui: &mut egui::Ui, idx: usize, rect: { crate::canvas_tool_menu::open_transform(app, [p.x, p.y]); } + let lasso_retracted = tool == Tool::Lasso && response.secondary_clicked() && crate::lasso_ui::undo_last_vertex(app); if tool == Tool::Lasso { crate::lasso_ui::canvas_input(app, &ctx, &xf, &response); (buttons.started, buttons.dragged, buttons.stopped, buttons.clicked) = (false, false, false, false); } // Right-click with the Move tool, or ⌘/Ctrl+right-click: the layers under the pointer. if response.secondary_clicked() + && !lasso_retracted && !transforming && crate::layer_pick_ui::is_gesture(tool, mods) && let Some(p) = response.interact_pointer_pos() @@ -1832,6 +1834,7 @@ pub fn canvas_view(app: &mut PhotocraftApp, ui: &mut egui::Ui, idx: usize, rect: crate::layer_pick_ui::open(app, [p.x, p.y], d[0], d[1]); } if response.secondary_clicked() + && !lasso_retracted && !transforming && !crate::layer_pick_ui::is_gesture(tool, mods) && let Some(p) = response.interact_pointer_pos() diff --git a/crates/ui-egui/src/lasso_ui.rs b/crates/ui-egui/src/lasso_ui.rs index 8897148184..2328f748bd 100644 --- a/crates/ui-egui/src/lasso_ui.rs +++ b/crates/ui-egui/src/lasso_ui.rs @@ -20,6 +20,29 @@ pub fn active(app: &PhotocraftApp) -> bool { app.drag.as_ref().is_some_and(|d| d.tool == Tool::Lasso && d.lasso.is_some()) } +/// Between polygonal clicks, Backspace/right-click retracts the last fixed vertex. +/// Do not affect a freehand stroke whose primary button is still held. +pub fn waiting_for_vertex(app: &PhotocraftApp) -> bool { + app.ui.tool == Tool::Lasso + && app + .drag + .as_ref() + .is_some_and(|d| d.tool == Tool::Lasso && d.lasso.as_ref().is_some_and(|l| !l.down && l.document == app.session.active().map(|st| st.doc.id))) +} + +pub fn undo_last_vertex(app: &mut PhotocraftApp) -> bool { + cancel_stale(app); + if !waiting_for_vertex(app) { + return false; + } + if app.drag.as_ref().is_some_and(|d| d.points.len() <= 1) { + app.drag = None; + } else if let Some(d) = app.drag.as_mut() { + d.points.pop(); + } + true +} + /// A lasso drag that moves the selection (started inside it, `canvas::selection_drag_kind`): /// the canvas's own selection-move path handles it, this module only feeds it the events. fn moving_selection(app: &PhotocraftApp) -> bool { diff --git a/crates/ui-egui/src/lasso_ui/tests.rs b/crates/ui-egui/src/lasso_ui/tests.rs index 7639170ed8..97480f6b3f 100644 --- a/crates/ui-egui/src/lasso_ui/tests.rs +++ b/crates/ui-egui/src/lasso_ui/tests.rs @@ -199,3 +199,54 @@ fn a_drag_inside_the_selection_moves_it() { assert!(h.state().drag.is_none()); assert_eq!(bounds(h.state()), Some(photocraft_geom::Rect::new(110, 105, 160, 145))); } + +#[test] +fn retracting_polygonal_vertices_keeps_the_document_unchanged() { + let mut app = app(); + begin(&mut app, Modifiers::NONE); + event(&mut app, "up", 120.0, 50.0, Modifiers::ALT); + assert!(waiting_for_vertex(&app)); + let layer = app.session.active().unwrap().active_layer.unwrap(); + assert_eq!(app.drag.as_ref().unwrap().points.len(), 3); + assert!(undo_last_vertex(&mut app)); + assert_eq!(app.drag.as_ref().unwrap().points.len(), 2); + assert!(undo_last_vertex(&mut app)); + assert_eq!(app.drag.as_ref().unwrap().points.len(), 1); + assert!(undo_last_vertex(&mut app)); + assert!(!active(&app)); + assert!(!undo_last_vertex(&mut app)); + let st = app.session.active().unwrap(); + assert!(st.doc.layer(layer).is_some()); + assert!(st.doc.selection.is_none()); +} + +#[test] +fn real_canvas_backspace_and_right_click_retract_polygonal_points() { + let mut h = harness(); + mouse(&mut h, "down", 50.0, 50.0, Modifiers::NONE); + mouse(&mut h, "move", 120.0, 50.0, Modifiers::NONE); + mouse(&mut h, "up", 120.0, 50.0, Modifiers::ALT); + mouse(&mut h, "down", 180.0, 100.0, Modifiers::ALT); + mouse(&mut h, "up", 180.0, 100.0, Modifiers::ALT); + assert_eq!(h.state().drag.as_ref().unwrap().points.len(), 3); + let layer = h.state().session.active().unwrap().active_layer.unwrap(); + + h.event(Event::Key { key: egui::Key::Backspace, physical_key: None, pressed: true, repeat: false, modifiers: Modifiers::ALT }); + h.run_steps(2); + assert_eq!(h.state().drag.as_ref().unwrap().points.len(), 2); + assert!(h.state().session.active().unwrap().doc.layer(layer).is_some()); + + let state = h.state(); + let view = &state.ui.views[0]; + let xf = ViewXform { rect: crate::rulers::content_rect(state, state.last_canvas_rect), zoom: view.zoom, center: view.center, flip: false }; + let pos = xf.to_screen(155.0, 100.0); + h.event(Event::PointerMoved(pos)); + h.event(Event::PointerButton { pos, button: PointerButton::Secondary, pressed: true, modifiers: Modifiers::ALT }); + h.run_steps(1); + h.event(Event::PointerButton { pos, button: PointerButton::Secondary, pressed: false, modifiers: Modifiers::ALT }); + h.run_steps(2); + assert_eq!(h.state().drag.as_ref().unwrap().points.len(), 1); + assert!(h.state().ui.canvas_tool_menu.is_none()); + assert!(h.state().session.active().unwrap().doc.layer(layer).is_some()); + assert!(h.state().session.active().unwrap().doc.selection.is_none()); +} diff --git a/crates/ui-egui/src/shortcuts.rs b/crates/ui-egui/src/shortcuts.rs index 078c9f3b7f..b413c44ea8 100644 --- a/crates/ui-egui/src/shortcuts.rs +++ b/crates/ui-egui/src/shortcuts.rs @@ -296,6 +296,15 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context) { if crate::magnetic_lasso_ui::keys(app, ctx) { return; } + // While placing polygonal lasso vertices, Backspace/Delete retracts the last point; + // it must win over the regular Edit › Clear shortcut, which edits document pixels. + if crate::lasso_ui::waiting_for_vertex(app) { + let mods = ctx.input(|i| i.modifiers); + if !mods.command && !mods.ctrl && !mods.shift && ctx.input_mut(|i| i.consume_key(mods, Key::Backspace) || i.consume_key(mods, Key::Delete)) { + crate::lasso_ui::undo_last_vertex(app); + return; + } + } // Inline type editing eats text and navigation keys; ⌘-shortcuts still reach the menus. let editing = crate::type_tool::handle_keys(app, ctx); // Registry, UI and menu-catalogue shortcuts (see [`crate::shortcut_dispatch::bindings`]). From c833294e34124eb8025de91627eb5a2d3a50f422 Mon Sep 17 00:00:00 2001 From: DWAA1660 Date: Thu, 8 Oct 2026 12:31:24 -0600 Subject: [PATCH 7/7] PSD export: write empty layer and mask channels as raw (#1290) --- crates/io/src/psd_export.rs | 6 +++- crates/io/tests/psd_empty_channels.rs | 44 +++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 crates/io/tests/psd_empty_channels.rs diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index d9f9cc09e8..f8f0337339 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -244,7 +244,11 @@ impl Ex { fn encode(&self, id: i16, plane: &[u8], w: usize, h: usize) -> ChannelData { let depth = psd_depth(self.fmt.sample); - ChannelData::encode(id, self.compression(), plane, w, h, depth, self.version) + // Photoshop writes empty layer/mask channels as just the raw compression code. + // A ZIP stream for a zero-sized channel makes readers try to decompress an + // invalid image extent (notably psd-tools for 32-bit adjustment layers). + let compression = if w == 0 || h == 0 { Compression::Raw } else { self.compression() }; + ChannelData::encode(id, compression, plane, w, h, depth, self.version) .or_else(|_| ChannelData::encode(id, Compression::Raw, plane, w, h, depth, self.version)) .unwrap_or(ChannelData { id, compression: Some(Compression::Raw), data: plane.to_vec() }) } diff --git a/crates/io/tests/psd_empty_channels.rs b/crates/io/tests/psd_empty_channels.rs new file mode 100644 index 0000000000..285f03cfcd --- /dev/null +++ b/crates/io/tests/psd_empty_channels.rs @@ -0,0 +1,44 @@ +mod common; +use common::Features; +use photocraft_color::{ColorMode, SampleType}; +use photocraft_io::{ExportOptions, export}; +use photocraft_psd::{Compression, PsdFile}; + +#[test] +fn empty_layer_and_mask_channels_use_raw_without_a_payload() { + for mode in [ColorMode::Rgb, ColorMode::Grayscale] { + for depth in [SampleType::U8, SampleType::U16, SampleType::F32] { + for ext in ["out.psd", "out.psb"] { + let mut doc = common::gen_doc(mode, depth, Features::ALL); + let mut masked = common::raster("Uniform mask", doc.pixel_format(), photocraft_geom::Rect::new(0, 0, 3, 3), 1, true); + masked.mask = Some(photocraft_doc::LayerMask { + surface: photocraft_raster::Surface::new(photocraft_color::PixelFormat::new(ColorMode::Grayscale, depth, false)), + enabled: true, + linked: true, + density: 1.0, + feather: 0.0, + }); + doc.layers.push(masked); + let bytes = export(&doc, ext, &ExportOptions::default()).unwrap().bytes; + let file = PsdFile::from_bytes(&bytes).unwrap(); + let mut empty = 0; + let mut empty_masks = 0; + for layer in file.layers() { + for channel in &layer.channels { + let rect = layer.channel_rect(channel.id); + if rect.width() == 0 || rect.height() == 0 { + empty += 1; + if channel.id == -2 { + empty_masks += 1; + } + assert_eq!(channel.compression, Some(Compression::Raw), "{mode:?} {depth:?} {ext}: {} channel {}", layer.name(), channel.id); + assert!(channel.data.is_empty()); + } + } + } + assert!(empty > 0); + assert!(empty_masks > 0); + } + } + } +}