From 427fb34042876593cc780667612e617da11711d0 Mon Sep 17 00:00:00 2001 From: Gert Pellin - GPE Date: Sat, 24 Oct 2026 00:34:02 +0200 Subject: [PATCH 01/16] Curves: the graph grows with the Properties panel and a resizable dialog (#8984) (#8998) * Curves: the graph grows with the Properties panel and a resizable dialog (#8984) The graph was fixed at 300 points. It now takes the width it is given, no taller than the window leaves room for beside the other rows. The Image > Adjustments > Curves dialog gets a resize grip and reopens at the size it was left at. * Curves: the dialog stops widening where the graph stops; a squeezed size isn't remembered (#8984) The dialog's resize grip goes no wider than the graph can grow in this window, so no blank column. A size at the window's limit no longer replaces the size the user left the dialog at. * test(ui): keep the Issue #43 doc comment on its own test Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/adjust_dialog.rs | 78 ++++++++++++++++++++++ crates/ui-egui/src/adjust_editors.rs | 23 ++++++- crates/ui-egui/src/adjust_editors_tests.rs | 21 ++++++ crates/ui-egui/src/dialogs.rs | 71 ++++++++++++++++---- 4 files changed, 178 insertions(+), 15 deletions(-) diff --git a/crates/ui-egui/src/adjust_dialog.rs b/crates/ui-egui/src/adjust_dialog.rs index acb56b46aa..1c45da190b 100644 --- a/crates/ui-egui/src/adjust_dialog.rs +++ b/crates/ui-egui/src/adjust_dialog.rs @@ -629,6 +629,84 @@ mod tests { } } + /// #8984: the Curves dialog has a resize grip; its graph grows with the dialog, the controls + /// stay inside it, and it reopens at the size it was left at. + #[test] + fn curves_dialog_resizes_with_its_graph_and_reopens_at_that_size() { + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1100.0)).build_ui_state(|ui, app| crate::dialogs::show(app, ui.ctx()), app_with_image()); + PhotocraftApp::setup_context(&h.ctx, Default::default()); + let graph = |h: &Harness<'_, PhotocraftApp>| h.ctx.data(|d| d.get_temp::(dialog_mem("curves").with("curves-graph"))).unwrap(); + let window = |h: &Harness<'_, PhotocraftApp>, id: u64| h.ctx.memory(|m| m.area_rect(egui::Id::new(("dialog", id)))).unwrap(); + let id = open(h.state_mut(), "image.adjustments.curves").unwrap(); + h.run_steps(4); + let (opened, small) = (window(&h, id), graph(&h)); + assert!(small.width() > 270.0 && small.width() < 300.0, "opens at the 300-point graph: {small:?}"); + // Drag the grip (bottom-right corner) 300 points right and 200 down. + let corner = opened.max - egui::vec2(12.0, 12.0); + let by = egui::vec2(300.0, 200.0); + h.event(egui::Event::PointerMoved(corner)); + h.step(); + h.event(egui::Event::PointerButton { pos: corner, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.step(); + for i in 1..=4 { + h.event(egui::Event::PointerMoved(corner + by * (i as f32 / 4.0))); + h.step(); + } + h.event(egui::Event::PointerButton { pos: corner + by, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + h.run_steps(6); + let (wide, big) = (window(&h, id), graph(&h)); + assert!(big.width() > small.width() + 250.0, "the graph follows the dialog: {small:?} → {big:?}"); + assert!((big.width() - big.height()).abs() < 1.0, "still square: {big:?}"); + assert!(wide.contains_rect(big) && wide.right() - big.right() < 40.0, "no blank column: window {wide:?}, graph {big:?}"); + for label in ["Channel:", "Input:", "Output:", "Preview", "OK", "Cancel"] { + assert!(wide.contains_rect(h.get_by_label(crate::i18n::t(label)).rect()), "{label} inside the window"); + } + // Cancel, open again: the size it was left at. + h.get_by_label("Cancel").click(); + h.run_steps(3); + assert!(h.state().ui.dialogs.is_empty()); + let again = open(h.state_mut(), "image.adjustments.curves").unwrap(); + h.run_steps(4); + assert!((graph(&h).width() - big.width()).abs() < 2.0, "reopened at {:?}, left at {big:?}", graph(&h)); + assert!((window(&h, again).width() - wide.width()).abs() < 2.0); + } + + /// #8984: however tall the graph's width would make it, the dialog still fits the window. + #[test] + fn curves_graph_never_pushes_the_dialog_off_a_short_window() { + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 700.0)).build_ui_state(|ui, app| crate::dialogs::show(app, ui.ctx()), app_with_image()); + PhotocraftApp::setup_context(&h.ctx, Default::default()); + h.ctx.data_mut(|m| m.insert_persisted(egui::Id::new(("dialog-place", "image.adjustments.curves")).with("size"), egui::vec2(1200.0, 1200.0))); + let id = open(h.state_mut(), "image.adjustments.curves").unwrap(); + h.run_steps(5); + let graph = h.ctx.data(|d| d.get_temp::(dialog_mem("curves").with("curves-graph"))).unwrap(); + let window = h.ctx.memory(|m| m.area_rect(egui::Id::new(("dialog", id)))).unwrap(); + assert!(graph.height() <= 700.0 - 360.0, "{graph:?}"); + assert!(window.height() <= 700.0, "{window:?}"); + assert!(window.contains_rect(h.get_by_label("OK").rect())); + // The size the window squeezed it to is not remembered: a larger window gets 1200 again. + let kept = h.ctx.data_mut(|m| m.get_persisted::(egui::Id::new(("dialog-place", "image.adjustments.curves")).with("size"))); + assert_eq!(kept, Some(egui::vec2(1200.0, 1200.0))); + // Dragged far wider, the dialog stops where the graph stops: no blank column. + let corner = window.max - egui::vec2(12.0, 12.0); + h.event(egui::Event::PointerMoved(corner)); + h.step(); + h.event(egui::Event::PointerButton { pos: corner, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.step(); + h.event(egui::Event::PointerMoved(corner + egui::vec2(600.0, 0.0))); + h.step(); + h.event(egui::Event::PointerButton { + pos: corner + egui::vec2(600.0, 0.0), + button: egui::PointerButton::Primary, + pressed: false, + modifiers: egui::Modifiers::NONE, + }); + h.run_steps(5); + let graph = h.ctx.data(|d| d.get_temp::(dialog_mem("curves").with("curves-graph"))).unwrap(); + let window = h.ctx.memory(|m| m.area_rect(egui::Id::new(("dialog", id)))).unwrap(); + assert!(window.right() - graph.right() < 40.0, "no blank column: window {window:?}, graph {graph:?}"); + } + #[test] fn cancel_leaves_the_document_untouched() { let mut harness = diff --git a/crates/ui-egui/src/adjust_editors.rs b/crates/ui-egui/src/adjust_editors.rs index 877525ffb8..f646ddd452 100644 --- a/crates/ui-egui/src/adjust_editors.rs +++ b/crates/ui-egui/src/adjust_editors.rs @@ -357,6 +357,27 @@ fn gradient_bar(p: &egui::Painter, r: Rect, from: Color32, to: Color32, vertical const CURVE_HANDLE_SIZE: f32 = 7.0; const CURVE_GRAPH_PADDING: f32 = 5.0; +/// The room left on screen for the Curves dialog's other rows (title, channel and mode, Input and +/// Output, eyedroppers, buttons) when its graph is as tall as it can be. +const CURVES_CHROME: f32 = 360.0; +/// The smallest and the default largest side of the Curves graph: 300 points was the fixed +/// size until #8984. +const CURVE_SIDE: (f32, f32) = (120.0, 300.0); + +/// The Curves graph's side (#8984): the width it is given, in the Properties panel and in the +/// resizable dialog, and no taller than the window leaves room for beside the other rows. +fn curve_side(ui: &egui::Ui) -> f32 { + let tallest = curve_side_max(ui.ctx()); + let width = ui.available_width(); + if width.is_finite() { width.clamp(CURVE_SIDE.0, tallest) } else { CURVE_SIDE.1 } +} + +/// The largest the Curves graph gets in this window: as tall as the window leaves room for beside +/// the dialog's other rows, and never under the old 300 points. The Curves dialog is no wider. +pub(crate) fn curve_side_max(ctx: &egui::Context) -> f32 { + (ctx.content_rect().height() - CURVES_CHROME).max(CURVE_SIDE.1) +} + fn curve_graph(full: Rect, side: f32) -> Rect { let base = Rect::from_min_size(full.min + vec2(14.0, 0.0), vec2(side - 14.0, side - 14.0)); Rect::from_min_max(base.min + vec2(CURVE_GRAPH_PADDING, CURVE_GRAPH_PADDING), base.max - vec2(CURVE_GRAPH_PADDING, CURVE_GRAPH_PADDING)) @@ -461,7 +482,7 @@ fn curves(ui: &mut egui::Ui, v: &mut Value, cx: &EditorCx) -> Edit { let pencil = v.get("pencil").and_then(Value::as_bool).unwrap_or(false); let mut pts = read_curve(v, key); let mut map = read_map(v, key); - let side = ui.available_width().clamp(120.0, 300.0); + let side = curve_side(ui); ui.add_space(4.0); let (full, resp) = ui.allocate_exact_size(vec2(side, side + 14.0), Sense::click_and_drag()); let graph = curve_graph(full, side); diff --git a/crates/ui-egui/src/adjust_editors_tests.rs b/crates/ui-egui/src/adjust_editors_tests.rs index d8c6e7953a..7f669186bb 100644 --- a/crates/ui-egui/src/adjust_editors_tests.rs +++ b/crates/ui-egui/src/adjust_editors_tests.rs @@ -206,6 +206,27 @@ fn curve_of(a: &Adjustment) -> Vec<[f32; 2]> { } } +/// #8984: the Curves graph in Properties grows with the panel instead of stopping at 300 +/// points, stays square, and edits where it is clicked at its new size. +#[test] +fn curves_graph_grows_with_the_properties_panel() { + let mut h = app_harness("curves", "rgb", 8); + let (id, _) = layer(&h); + let narrow = curves_graph(&h, id); + assert!(narrow.width() < 300.0, "{narrow:?}"); + crate::panels::request_dock_width(&h.ctx, 760.0); + h.run_steps(8); + let wide = curves_graph(&h, id); + // 760 points of panel, but the 900-point window leaves room for 540 beside the other rows. + assert!(wide.width() > narrow.width() + 250.0 && wide.height() <= 900.0 - 360.0, "{narrow:?} → {wide:?}"); + assert!((wide.width() - wide.height()).abs() < 1.0, "square: {wide:?}"); + // A point added at the graph's centre lands at 128, 128 of the curve. + click(&mut h, wide.center()); + let c = curve_of(&layer(&h).1); + assert_eq!(c.len(), 3, "{c:?}"); + assert!((c[1][0] - 127.5).abs() < 2.0 && (c[1][1] - 127.5).abs() < 2.0, "{c:?}"); +} + /// Issue #43: pressing on a point selects it (no new point), Delete/Backspace and ⌘/Ctrl-click /// remove points, dragging off the graph removes one, endpoints stay. #[test] diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 3191d64392..0ff73e135c 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -181,16 +181,47 @@ fn resizable_dialog(fields: &serde_json::Map) -> Option) -> bool { + fields.get("__adjust").and_then(Value::as_str) == Some("curves") +} + +/// The size the Curves dialog first opens at: the 300-point graph it had before it could be +/// resized (#5113); it grows to fit its controls below the graph. +const CURVES_SIZE: egui::Vec2 = egui::vec2(300.0, 420.0); +/// The smallest a resizable dialog is dragged to: Curves keeps a usable graph (#8984), the others +/// their two-column layouts. +fn resizable_min(fields: &serde_json::Map) -> egui::Vec2 { + if is_curves(fields) { egui::vec2(300.0, 420.0) } else { egui::vec2(560.0, 400.0) } +} + +/// Where the size the user last gave a resizable dialog is kept (#8984), beside its place. +fn size_id(place: egui::Id) -> egui::Id { + place.with("size") +} + +/// The largest a resizable dialog of `fields` with smallest size `min` is dragged to: the window +/// less a margin, and for Curves no wider than its graph can grow (#8984), so no blank column. +fn resizable_max(ctx: &egui::Context, fields: &serde_json::Map, min: egui::Vec2) -> egui::Vec2 { + let mut max = (ctx.content_rect().size() - egui::vec2(40.0, 40.0)).max(min); + if is_curves(fields) { + max.x = max.x.min(crate::adjust_editors::curve_side_max(ctx)).max(min.x); + } + max +} + fn show_modal( modal: egui::Modal, ctx: &egui::Context, id: egui::Id, - resizable: Option, + resizable: Option<(egui::Vec2, egui::Vec2, egui::Vec2)>, interactive: bool, contents: impl FnOnce(&mut egui::Ui) -> T, ) -> egui::ModalResponse { @@ -199,17 +230,10 @@ fn show_modal( if !interactive { ui.disable(); } - if let Some(size) = resizable { - let max = (ctx.content_rect().size() - egui::vec2(40.0, 40.0)).max(egui::vec2(560.0, 400.0)); + if let Some((size, min, max)) = resizable { // No stroke: Resize outlines its contents 2 points out, an inner box inside the // dialog's frame that left the title and buttons flush against it (#8199). - egui::Resize::default() - .id(id.with("resize")) - .with_stroke(false) - .default_size(size.min(max)) - .min_size(egui::vec2(560.0, 400.0)) - .max_size(max) - .show(ui, contents) + egui::Resize::default().id(id.with("resize")).with_stroke(false).default_size(size.min(max)).min_size(min).max_size(max).show(ui, contents) } else { contents(ui) } @@ -289,14 +313,22 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { let web = fields.contains_key("__web"); let web_popup_open = web && egui::Popup::is_any_open(ctx); let resizable = resizable_dialog(&fields); - let modal = show_modal(modal, ctx, id, resizable, interactive, |ui| { + // Curves reopens at the size it was left at (#8984). + let remembered = if is_curves(&fields) { ctx.data_mut(|m| m.get_persisted::(size_id(place))) } else { None }; + let resize = resizable.map(|size| { + let min = resizable_min(&fields); + (remembered.unwrap_or(size), min, resizable_max(ctx, &fields, min)) + }); + let mut given = None; + let modal = show_modal(modal, ctx, id, resize, interactive, |ui| { sizing = ui.is_sizing_pass(); let available = ui.available_size(); + given = Some(available); // A dialog of a few short fields (`__compact`, Fit Image) is narrower (#4673): the title // rule and the button row fill the width they are given, so it needs its own maximum too. let compact = d.fields.contains_key("__compact"); - // The curve editor caps its graph allocation at 300 points (#5113). - let curves = fields.get("__adjust").and_then(Value::as_str) == Some("curves"); + // The curve editor's graph is as wide as the dialog lets it be (#5113, #8984). + let curves = is_curves(&fields); if !compact { ui.set_min_width(if curves { 300.0 } else { 380.0 }); } @@ -323,7 +355,7 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { } else if compact { 260.0 } else if curves { - 300.0 + available.x.max(300.0) } else { 440.0 })); @@ -579,6 +611,17 @@ pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { }); // The picker has a larger frame margin; include that whole frame in the canvas hit guard. shown.push(if color_picker { modal.response.rect } else { modal.inner }); + // The size Curves was given (dragged or grown to fit) is the one it reopens at (#8984). + // A size at the window's limit is the window's, not the user's: a large remembered size + // squeezed into a small window is kept for a larger one. + if let (false, true, Some(size), Some((opened, _, max))) = (sizing, is_curves(&fields), given.filter(|g| g.is_finite()), resize) + && size != opened.min(max) + && size.x < max.x - 0.5 + && size.y < max.y - 0.5 + && remembered != Some(size) + { + ctx.data_mut(|m| m.insert_persisted(size_id(place), size)); + } // Pin once laid out at its real size (the first frame is an invisible sizing pass). if !sizing && (web || pinned.is_none() || drag != egui::Vec2::ZERO) { let screen = ctx.content_rect(); From f719f0bfac8bb53a17aae12f7f9e95468fbbabe5 Mon Sep 17 00:00:00 2001 From: David Mann Date: Sat, 24 Oct 2026 00:34:16 +0200 Subject: [PATCH 02/16] Perspective Crop: the swap button swaps W and H (#7059) (#7060) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Perspective Crop: the swap button swaps W and H (#7059) The W/H swap button called crop_size::swap, which only acts in the Crop tool's W x H x Resolution mode; Perspective Crop never sets that mode, so the button did nothing there. The button now always swaps the fields; X keeps the guard. Co-Authored-By: Claude Opus 5.5 * Address review: the test clicks Perspective Crop's ⇄ button The test now renders the real options bar with the Perspective Crop tool, clicks ⇄ through the egui_kittest harness and checks that W and H swapped, instead of calling `swap_fields` directly. It fails if the button goes back to calling `swap`. Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/ui-egui/src/crop_size.rs | 12 ++++++--- crates/ui-egui/src/perspective_crop_ui.rs | 30 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/crop_size.rs b/crates/ui-egui/src/crop_size.rs index dc91ec681b..71fc57f495 100644 --- a/crates/ui-egui/src/crop_size.rs +++ b/crates/ui-egui/src/crop_size.rs @@ -226,15 +226,21 @@ pub fn clear(o: &mut ToolOptions) { o.crop_resolution.clear(); } -/// Swaps W and H (the ⇄ button, X). False outside W x H x Resolution mode. +/// Swaps W and H for the Crop tool's X. False outside W x H x Resolution mode. pub fn swap(o: &mut ToolOptions) -> bool { if o.crop_ratio != WHR { return false; } - std::mem::swap(&mut o.crop_width, &mut o.crop_height); + swap_fields(o); true } +/// The ⇄ button between the W and H fields: always swaps, as Perspective Crop shows the fields +/// without the Crop tool's W x H x Resolution mode. +fn swap_fields(o: &mut ToolOptions) { + std::mem::swap(&mut o.crop_width, &mut o.crop_height); +} + /// After the preset dropdown chose `crop_ratio`: a size preset fills the fields and Front Image /// reads the document, both leaving W x H x Resolution mode on. pub fn chosen(app: &mut PhotocraftApp) { @@ -298,7 +304,7 @@ fn text_field(ui: &mut egui::Ui, id: &str, value: &mut String, hint: &str, width pub fn fields(o: &mut ToolOptions, ui: &mut egui::Ui) { text_field(ui, "crop-width", &mut o.crop_width, tl!("Width"), 64.0, normalize_length); if crate::icons::button(ui, "arrow-left-right", 22.0, false, tl!("Swaps height and width")).clicked() { - swap(o); + swap_fields(o); } text_field(ui, "crop-height", &mut o.crop_height, tl!("Height"), 64.0, normalize_length); text_field(ui, "crop-resolution", &mut o.crop_resolution, tl!("Resolution"), 64.0, normalize_resolution); diff --git a/crates/ui-egui/src/perspective_crop_ui.rs b/crates/ui-egui/src/perspective_crop_ui.rs index 396fb87997..2a696bd5ed 100644 --- a/crates/ui-egui/src/perspective_crop_ui.rs +++ b/crates/ui-egui/src/perspective_crop_ui.rs @@ -352,6 +352,36 @@ mod tests { assert_eq!(ordered([[10.0, 90.0], [100.0, 5.0], [0.0, 0.0], [90.0, 100.0]]), [[0.0, 0.0], [100.0, 5.0], [90.0, 100.0], [10.0, 90.0]]); } + /// Clicking ⇄ in the rendered options bar swaps W and H, although Perspective Crop never sets + /// the Crop tool's W x H x Resolution mode (#7059). + #[test] + fn the_options_bar_swap_button_swaps_w_and_h() { + use egui_kittest::kittest::Queryable; + let doc = Document::with_background("p", Size::new(200, 150), ColorMode::Rgb, SampleType::U8, Color::WHITE); + let mut s = photocraft_engine::Session::new(); + s.add_document(doc, None); + let mut app = PhotocraftApp::new(s, crate::Services::default()); + app.ui.tool = Tool::PerspectiveCrop; + app.ui.tool_options.crop_width = "4 in".into(); + app.ui.tool_options.crop_height = "6 in".into(); + assert_ne!(app.ui.tool_options.crop_ratio, crate::crop_size::WHR); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1400.0, 60.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + if !ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) { + return; + } + crate::panels::options_bar(app, ui); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ProMedium); + h.run_steps(4); + h.get_by_label("Swaps height and width").click(); + h.run_steps(2); + let o = &h.state().ui.tool_options; + assert_eq!((o.crop_width.as_str(), o.crop_height.as_str()), ("6 in", "4 in")); + } + /// The pending quad belongs to its document (#1918): switching documents drops it, so it is /// never drawn on the other one; on its own document it stays. #[test] From 74ada29c1224581408d0e96ad70edbf7f4c5ac05 Mon Sep 17 00:00:00 2001 From: Vikram Bhaskaran Date: Sat, 24 Oct 2026 09:34:29 +1100 Subject: [PATCH 03/16] Keep shape fill and stroke alpha through PSD round-trip (#6180) (#6868) * Keep shape fill and stroke alpha through PSD round-trip (#6180). Photoshop colour descriptors have no alpha, so fill opacity was dropped on save. Write a PhotoCraft-only `pcAl` key for fill colour, and fold stroke colour alpha into `strokeStyleOpacity` so Appearance still matches. * Write pcAl only on solid shape fills, not every colour (#6180). Gradient stops already store alpha in Trns. Putting pcAl on color_to_desc made Ramp sample that alpha twice. Generic colour descriptors stay opaque; fill_to_desc adds pcAl only for SoCo. * Return color_to_desc's match directly so clippy stays clean (#6180). The leftover let-and-return after dropping the pcAl branch failed clippy -D warnings on Windows integration. * Restore rectangle fill and stroke appearance alphas after PSD save (#6180). The ticket is Appearance after load, then a redraw. Fill already used pcAl. Stroke folded colour alpha into strokeStyleOpacity and dropped the colour alpha, so Appearance still showed a maxed stroke colour. Write pcAl on the original stroke colour, keep the folded opacity for Photoshop, and unfold on read. Cover it with a rectangle document round-trip and redraw. * fix(io): ignore a non-finite pcAl alpha from a file Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/io/src/blocks.rs | 69 +++++++++++++++++++++++++++++++++++- crates/io/src/vector_map.rs | 48 +++++++++++++++++++++++-- crates/io/tests/it/vector.rs | 31 ++++++++++++++++ 3 files changed, 144 insertions(+), 4 deletions(-) diff --git a/crates/io/src/blocks.rs b/crates/io/src/blocks.rs index 1ae5e1fffa..3e804a7ede 100644 --- a/crates/io/src/blocks.rs +++ b/crates/io/src/blocks.rs @@ -182,6 +182,17 @@ pub fn color_from_desc(d: &Descriptor) -> Option { Color::rgb(g("Rd ")? / 255.0, g("Grn ")? / 255.0, g("Bl ")? / 255.0) } }) + .map(|c| with_pc_alpha(c, d)) +} + +/// PSD colour descriptors have no alpha. PhotoCraft writes `pcAl` (0..1) so shape fill +/// opacity round-trips (#6180). Photoshop ignores the key. +fn with_pc_alpha(mut c: Color, d: &Descriptor) -> Color { + // A hostile file's NaN or infinity would survive the clamp: keep the colour opaque instead. + if let Some(a) = num(d.get("pcAl")).filter(|a| a.is_finite()) { + c.alpha = (a as f32).clamp(0.0, 1.0); + } + c } /// [`Color`] → descriptor in its own model. @@ -204,6 +215,13 @@ pub fn color_to_desc(c: &Color) -> Descriptor { } } +/// Solid shape fills have no other alpha slot. Write `pcAl` only here so gradient stops +/// keep using `Trns` alone (#6180). +fn color_to_desc_with_pc_alpha(c: &Color) -> Descriptor { + let desc = color_to_desc(c); + if c.alpha.is_finite() && (c.alpha - 1.0).abs() > 1e-6 { desc.with("pcAl", Value::Double(f64::from(c.alpha.clamp(0.0, 1.0)))) } else { desc } +} + pub(crate) fn get_desc<'a>(d: &'a Descriptor, key: &str) -> Option<&'a Descriptor> { match d.get(key)? { Value::Descriptor(x) | Value::GlobalObject(x) => Some(x), @@ -499,7 +517,7 @@ pub fn write_fill(f: &Fill) -> ([u8; 4], Vec) { /// A fill as (block key, descriptor with class `null`). pub fn fill_to_desc(f: &Fill) -> ([u8; 4], Descriptor) { match f { - Fill::Solid(c) => (*b"SoCo", Descriptor::new("null").with("Clr ", Value::Descriptor(color_to_desc(c)))), + Fill::Solid(c) => (*b"SoCo", Descriptor::new("null").with("Clr ", Value::Descriptor(color_to_desc_with_pc_alpha(c)))), Fill::Gradient { stops, angle, scale, style, reverse, opacity_stops, midpoints, offset, dither, align } => { let prc = |v: f32| Value::UnitFloat { unit: *b"#Prc", value: f64::from(v * 100.0) }; let mut d = Descriptor::new("null") @@ -991,6 +1009,55 @@ mod tests { assert!(super::color_from_desc(&Descriptor::new("HSBC").with("Strt", Value::Double(50.0))).is_none()); } + #[test] + fn color_descriptor_keeps_photocraft_alpha() { + use photocraft_color::Color; + use photocraft_doc::Fill; + let c = Color::rgba(0.2, 0.4, 0.6, 0.25); + let (k, d) = super::fill_to_desc(&Fill::Solid(c)); + let Fill::Solid(back) = super::fill_from_desc(&k, &d).unwrap() else { panic!("solid") }; + assert!((back.alpha - 0.25).abs() < 1e-5); + assert!((back.to_rgb()[0] - 0.2).abs() < 1e-3); + let (k, d) = super::fill_to_desc(&Fill::Solid(Color::rgb(1.0, 0.0, 0.0))); + let Fill::Solid(opaque) = super::fill_from_desc(&k, &d).unwrap() else { panic!("solid") }; + assert!((opaque.alpha - 1.0).abs() < 1e-6); + assert!(super::color_to_desc(&c).get("pcAl").is_none(), "generic colour descriptors have no pcAl"); + } + + #[test] + fn non_finite_photocraft_alpha_is_ignored() { + use photocraft_color::Color; + for bad in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + let d = super::color_to_desc(&Color::rgb(0.2, 0.4, 0.6)).with("pcAl", Value::Double(bad)); + let c = super::color_from_desc(&d).unwrap(); + assert_eq!(c.alpha, 1.0, "pcAl {bad}"); + } + let d = super::color_to_desc(&Color::rgb(0.2, 0.4, 0.6)).with("pcAl", Value::Double(0.5)); + assert!((super::color_from_desc(&d).unwrap().alpha - 0.5).abs() < 1e-6); + } + + #[test] + fn gradient_stop_alpha_round_trips_once() { + use photocraft_color::Color; + use photocraft_doc::Fill; + let fill = Fill::Gradient { + stops: vec![(0.0, Color::rgba(1.0, 0.0, 0.0, 0.5)), (1.0, Color::WHITE)], + angle: 90.0, + scale: 1.0, + style: photocraft_doc::GradientStyle::Linear, + reverse: false, + opacity_stops: vec![], + midpoints: vec![], + offset: (0.0, 0.0), + dither: false, + align: true, + }; + let (k, d) = super::fill_to_desc(&fill); + let Fill::Gradient { stops, opacity_stops, .. } = super::fill_from_desc(&k, &d).unwrap() else { panic!("grad") }; + assert!((stops[0].1.alpha - 1.0).abs() < 1e-4, "colour alpha lives in Trns, not the stop colour"); + assert!(opacity_stops.iter().any(|(_, a)| (*a - 0.5).abs() < 1e-3), "{opacity_stops:?}"); + } + #[test] fn blending_ranges_map_to_blend_if() { use photocraft_doc::{BlendIf, BlendRange}; diff --git a/crates/io/src/vector_map.rs b/crates/io/src/vector_map.rs index bb830dbd68..621edf47b8 100644 --- a/crates/io/src/vector_map.rs +++ b/crates/io/src/vector_map.rs @@ -258,10 +258,20 @@ pub fn parse_vstk(data: &[u8], dpi: f32) -> Option { _ => Vec::new(), }; let paint = get_desc(&d, "strokeStyleContent").and_then(content_fill).unwrap_or(Fill::Solid(photocraft_color::Color::BLACK)); + let mut opacity = num(d.get("strokeStyleOpacity")).map_or(1.0, |v| v as f32 / 100.0); + // Write folds colour alpha into strokeStyleOpacity for Photoshop. Restore both Appearance + // fields when pcAl is present so PhotoCraft does not show a maxed colour alpha (#6180). + if let Fill::Solid(c) = &paint + && c.alpha.is_finite() + && c.alpha > 1e-6 + && (c.alpha - 1.0).abs() > 1e-6 + { + opacity = (opacity / c.alpha).clamp(0.0, 1.0); + } let stroke = ShapeStroke { width: unit_px(d.get("strokeStyleLineWidth"), dpi).unwrap_or(1.0) as f32, paint, - opacity: num(d.get("strokeStyleOpacity")).map_or(1.0, |v| v as f32 / 100.0), + opacity, align, cap, join, @@ -276,10 +286,28 @@ fn en(ty: &str, v: &str) -> Value { Value::Enumerated { type_id: Id::new(ty), value: Id::new(v) } } +/// Photoshop's stroke colour has no alpha. Fold the colour's alpha into `strokeStyleOpacity` +/// so Appearance still matches after a PSD round-trip (#6180). +fn fold_stroke_color_alpha(s: &ShapeStroke) -> ShapeStroke { + let mut out = s.clone(); + if let Fill::Solid(c) = &mut out.paint + && c.alpha.is_finite() + && (c.alpha - 1.0).abs() > 1e-6 + { + out.opacity = (out.opacity * c.alpha).clamp(0.0, 1.0); + c.alpha = 1.0; + } + out +} + pub fn vstk_bytes(stroke: Option<&ShapeStroke>, fill_enabled: bool, dpi: f32) -> Vec { let default = ShapeStroke::default(); - let s = stroke.unwrap_or(&default); - let (_, content) = fill_to_desc(&s.paint); + let folded = stroke.map(fold_stroke_color_alpha); + let s = folded.as_ref().unwrap_or(&default); + // Keep pcAl on the original colour so Appearance can restore fill/stroke alphas separately + // from the folded Photoshop opacity (#6180). + let paint = stroke.map(|st| &st.paint).unwrap_or(&s.paint); + let (_, content) = fill_to_desc(paint); let class = match s.paint { Fill::Solid(_) => "solidColorLayer", Fill::Gradient { .. } => "gradientLayer", @@ -660,6 +688,20 @@ mod tests { assert!(off.stroke.is_none() && off.fill_enabled); } + #[test] + fn vstk_folds_color_alpha_into_stroke_opacity() { + let s = ShapeStroke { width: 2.0, paint: Fill::Solid(Color::rgba(0.0, 0.0, 1.0, 0.4)), opacity: 0.5, ..ShapeStroke::default() }; + let bytes = vstk_bytes(Some(&s), true, 72.0); + let raw = crate::blocks::parse_prefix_versioned(&bytes).unwrap(); + let file_opacity = crate::blocks::num(raw.get("strokeStyleOpacity")).unwrap() as f32 / 100.0; + assert!((file_opacity - 0.2).abs() < 1e-4, "Photoshop sees the product in strokeStyleOpacity"); + let got = parse_vstk(&bytes, 72.0).unwrap().stroke.unwrap(); + assert!((got.opacity - 0.5).abs() < 1e-4); + let Fill::Solid(c) = got.paint else { panic!("solid") }; + assert!((c.alpha - 0.4).abs() < 1e-5); + assert!((c.to_rgb()[2] - 1.0).abs() < 1e-3); + } + #[test] fn vogk_roundtrip() { for s in [ diff --git a/crates/io/tests/it/vector.rs b/crates/io/tests/it/vector.rs index e32cbed3fa..71bf66823c 100644 --- a/crates/io/tests/it/vector.rs +++ b/crates/io/tests/it/vector.rs @@ -135,6 +135,37 @@ fn shape_layer_typed_roundtrip() { } } +/// #6180: Appearance fill and stroke alphas survive PSD save/load, and a redraw +/// (move/crop) uses those values instead of snapping to opaque. +#[test] +fn rectangle_appearance_alphas_survive_psd_and_redraw() { + let mut d = doc(); + let sh = ShapeLayer { + path: photocraft_vector::shapes::rect(16.0, 16.0, 128.0, 96.0), + fill: Some(Fill::Solid(Color::rgba(1.0, 0.0, 0.0, 0.5))), + stroke: Some(ShapeStroke { width: 4.0, paint: Fill::Solid(Color::rgba(0.0, 0.0, 1.0, 0.4)), opacity: 0.5, ..ShapeStroke::default() }), + live: Some(LiveShape::Rect { rect: [16.0, 16.0, 128.0, 96.0], radii: [0.0; 4] }), + ..Default::default() + }; + d.layers.push(shape_layer(sh)); + let b = roundtrip(&d); + let LayerContent::Shape(s) = &b.layers[1].content else { panic!("not a shape") }; + let Fill::Solid(fill) = s.fill.as_ref().unwrap() else { panic!("solid fill") }; + assert!((fill.alpha - 0.5).abs() < 1e-4, "fill appearance alpha {}", fill.alpha); + let st = s.stroke.as_ref().unwrap(); + let Fill::Solid(stroke_c) = &st.paint else { panic!("solid stroke") }; + assert!((stroke_c.alpha - 0.4).abs() < 1e-4, "stroke colour alpha {}", stroke_c.alpha); + assert!((st.opacity - 0.5).abs() < 1e-4, "stroke opacity {}", st.opacity); + let mut redrawn = s.clone(); + redrawn.cache = Some(photocraft_vector::render_shape(&redrawn, photocraft_color::PixelFormat::RGBA8, Rect::new(0, 0, 256, 256))); + let Fill::Solid(fill2) = redrawn.fill.as_ref().unwrap() else { panic!("solid fill") }; + assert!((fill2.alpha - 0.5).abs() < 1e-4); + let st2 = redrawn.stroke.as_ref().unwrap(); + let Fill::Solid(stroke2) = &st2.paint else { panic!("solid stroke") }; + assert!((stroke2.alpha - 0.4).abs() < 1e-4); + assert!((st2.opacity - 0.5).abs() < 1e-4); +} + #[test] fn custom_stroke_styles_roundtrip_through_psd_vstk() { for align in [StrokeAlign::Inside, StrokeAlign::Center, StrokeAlign::Outside] { From d878e383331a52fbb7375d0be63c9c8d42afd485 Mon Sep 17 00:00:00 2001 From: Hodd Date: Sat, 24 Oct 2026 00:34:41 +0200 Subject: [PATCH 04/16] Color pickers: pick on press and follow small drags (#7793) (#7822) * Color pickers: pick on press and follow small drags (#7793) The Color Picker dialog's field and slider, and the Color panel's field and hue strip, only applied the colour on dragged()/clicked(). egui holds dragged() back until the pointer moves past its click threshold (~6 pt) or is held a while, so a press picked nothing at first and small adjustments jumped. Add widgets::pick_pos, which reads the pointer from the frame the button goes down (is_pointer_button_down_on), and use it on all four surfaces. The Color panel now restyles selected type only when the picked colour changes. Tests: color_picker_ui::field_and_slider_pick_on_press_and_follow_small_moves, panels::color_tests::color_panel_field_and_hue_strip_pick_on_press. * #1 Reverted accidental scoreboard change when regenerating it #2 Fixed color picker not checking for pointer press, but any mouse click * Update crates/ui-egui/src/widgets.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Color pickers: rustfmt pick_pos * test(ui): use main's rgb_to_hsb in the colour picker tests Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/color_picker_ui.rs | 93 ++++++++++++++++++++++++++- crates/ui-egui/src/panels.rs | 89 ++++++++++++++++++++++--- crates/ui-egui/src/widgets.rs | 10 +++ 3 files changed, 182 insertions(+), 10 deletions(-) diff --git a/crates/ui-egui/src/color_picker_ui.rs b/crates/ui-egui/src/color_picker_ui.rs index 27177ceb57..3effb27c4b 100644 --- a/crates/ui-egui/src/color_picker_ui.rs +++ b/crates/ui-egui/src/color_picker_ui.rs @@ -484,7 +484,7 @@ fn color_field(ui: &mut egui::Ui, f: &mut Map) { // Photoshop updates the colour on the press itself, not only when the button is released // (#6937): `is_pointer_button_down_on` is true from the press frame onwards (it covers // dragging too), while `clicked` fires on release and `dragged` only after a move. - if let Some(p) = resp.interact_pointer_pos().filter(|_| resp.is_pointer_button_down_on() || resp.clicked()) { + if let Some(p) = widgets::pick_pos(&resp) { let (x, y) = (((p.x - field.left()) / field.width()).clamp(0.0, 1.0), ((p.y - field.top()) / field.height()).clamp(0.0, 1.0)); let c = field_color(&mode, fz, x, y); let keep = match mode.as_str() { @@ -503,7 +503,7 @@ fn color_field(ui: &mut egui::Ui, f: &mut Map) { let tri = vec![pos2(x, sy), pos2(x - 6.0 * dir, sy - 4.0), pos2(x - 6.0 * dir, sy + 4.0)]; ui.painter().add(egui::Shape::convex_polygon(tri, t.text, Stroke::NONE)); } - if let Some(p) = sresp.interact_pointer_pos().filter(|_| sresp.is_pointer_button_down_on() || sresp.clicked()) { + if let Some(p) = widgets::pick_pos(&sresp) { let z = 1.0 - ((p.y - strip.top()) / strip.height()).clamp(0.0, 1.0); let h = match mode.as_str() { "h" => Some([z * 360.0, hsv[1], hsv[2]]), @@ -864,6 +864,95 @@ mod tests { h.state().get("color").and_then(Value::as_str).unwrap().to_string() } + /// Press at `at` and hold, with no release and no time passing, then run one frame. + fn press(h: &mut Harness<'static, S>, at: egui::Pos2) { + press_button(h, at, egui::PointerButton::Primary, true); + } + + fn press_button(h: &mut Harness<'static, S>, at: egui::Pos2, button: egui::PointerButton, pressed: bool) { + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button, pressed, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + } + + /// #7793: the field and the slider pick on the frame the button goes down, not after the + /// pointer has travelled past egui's click threshold or been held for a while, and follow + /// moves of a few points. + #[test] + fn field_and_slider_pick_on_press_and_follow_small_moves() { + let mut f = Map::new(); + f.insert("color".into(), json!("#336699")); + let mut h = Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state( + |ui, f: &mut Map| { + ui.horizontal(|ui| { + let o = ui.cursor().min; + let gap = ui.spacing().item_spacing.x; + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), (o, gap))); + color_field(ui, f); + }); + }, + f, + ); + h.run_steps(2); + let (o, gap) = h.ctx.data(|d| d.get_temp::<(egui::Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + // Field: saturation across, brightness down (hue mode). + let at = o + vec2(FIELD * 0.5, FIELD * 0.5); + press(&mut h, at); + let picked = color(&h); + assert_ne!(picked, "#336699", "the press itself picks"); + let hsv = rgb_to_hsb(parse_hex(&picked).unwrap()); + assert!((hsv[1] - 0.5).abs() < 0.02 && (hsv[2] - 0.5).abs() < 0.02, "{picked} {hsv:?}"); + // A 3-point nudge (under the 6-point click threshold) moves the pick. + h.event(egui::Event::PointerMoved(at + vec2(3.0, 0.0))); + h.run_steps(1); + assert_ne!(color(&h), picked, "a small drag adjusts the pick"); + h.event(egui::Event::PointerButton { + pos: at + vec2(3.0, 0.0), + button: egui::PointerButton::Primary, + pressed: false, + modifiers: egui::Modifiers::NONE, + }); + h.run_steps(2); + // Hue slider: a press near the bottom picks a hue near 0 at once. + let before = color(&h); + press(&mut h, o + vec2(FIELD + gap + 9.0, FIELD * 0.95)); + assert_ne!(color(&h), before, "the slider picks on press"); + let hue = rgb_to_hsb(parse_hex(&color(&h)).unwrap())[0]; + assert!(hue < 30.0 || hue > 330.0, "hue {hue}"); + } + + /// Only the primary button picks: a right or middle press (and its release) on the field or + /// the slider leaves the colour alone. + #[test] + fn field_and_slider_ignore_secondary_and_middle_presses() { + let mut f = Map::new(); + f.insert("color".into(), json!("#336699")); + let mut h = Harness::builder().with_size(vec2(400.0, 300.0)).build_ui_state( + |ui, f: &mut Map| { + ui.horizontal(|ui| { + let o = ui.cursor().min; + let gap = ui.spacing().item_spacing.x; + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), (o, gap))); + color_field(ui, f); + }); + }, + f, + ); + h.run_steps(2); + let (o, gap) = h.ctx.data(|d| d.get_temp::<(egui::Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + for at in [o + vec2(FIELD * 0.5, FIELD * 0.5), o + vec2(FIELD + gap + 9.0, FIELD * 0.95)] { + for button in [egui::PointerButton::Secondary, egui::PointerButton::Middle] { + press_button(&mut h, at, button, true); + assert_eq!(color(&h), "#336699", "{button:?} press at {at:?}"); + h.event(egui::Event::PointerMoved(at + vec2(3.0, 0.0))); + h.run_steps(1); + press_button(&mut h, at + vec2(3.0, 0.0), button, false); + h.run_steps(1); + assert_eq!(color(&h), "#336699", "{button:?} drag and release at {at:?}"); + } + } + } + #[test] fn hex_field_shows_uppercase_without_rewriting_the_selected_color() { let h = picker("#12abef"); diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index 04b6f1ac59..162cf0911a 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -4345,9 +4345,8 @@ fn color_field(app: &mut PhotocraftApp, ui: &mut egui::Ui) { } ui.painter().add(mesh); ui.painter().rect_stroke(field, 0.0, Stroke::new(1.0, t.separator), StrokeKind::Outside); - if (fresp.dragged() || fresp.clicked()) - && let Some(p) = fresp.interact_pointer_pos() - { + let field_pick = widgets::pick_pos(&fresp); + if let Some(p) = field_pick { hsva.s = ((p.x - field.left()) / field.width()).clamp(0.0, 1.0); hsva.v = 1.0 - ((p.y - field.top()) / field.height()).clamp(0.0, 1.0); } @@ -4370,18 +4369,18 @@ fn color_field(app: &mut PhotocraftApp, ui: &mut egui::Ui) { m2.add_triangle(a, a + 3, a + 2); } ui.painter().add(m2); - if (sresp.dragged() || sresp.clicked()) - && let Some(p) = sresp.interact_pointer_pos() - { + let strip_pick = widgets::pick_pos(&sresp); + if let Some(p) = strip_pick { hsva.h = 1.0 - ((p.y - strip.top()) / strip.height()).clamp(0.0, 0.9999); } let y = strip.top() + (1.0 - hsva.h) * strip.height(); let tri = vec![pos2(strip.right() + 1.0, y), pos2(strip.right() + 6.0, y - 4.0), pos2(strip.right() + 6.0, y + 4.0)]; ui.painter().add(egui::Shape::convex_polygon(tri, t.text, Stroke::NONE)); - if fresp.dragged() || fresp.clicked() || sresp.dragged() || sresp.clicked() { + if field_pick.is_some() || strip_pick.is_some() { if bg_active { app.session.tools.background = hsva_srgb(hsva); - } else { + } else if app.session.tools.foreground != hsva_srgb(hsva) { + // A held, unmoved press repeats the pick: restyle selected type only on a change. app.session.tools.foreground = hsva_srgb(hsva); crate::type_tool::foreground_changed(app); crate::shape_fill_ui::foreground_changed(app, crate::shape_fill_ui::gesture(ui)); @@ -5000,6 +4999,80 @@ mod color_tests { } } + /// #7793: the Color panel's field and hue strip pick on the frame the button goes down (no + /// release, no time passing) and follow moves smaller than egui's 6-point click threshold. + #[test] + fn color_panel_field_and_hue_strip_pick_on_press() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.session.tools.foreground = [0.2, 0.4, 0.6, 1.0]; + let mut h = egui_kittest::Harness::builder().with_size(vec2(300.0, 300.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + let at = (ui.cursor().min, ui.available_width()); + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), at)); + color_field(app, ui); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Studio); + h.run_steps(2); + let (o, w) = h.ctx.data(|d| d.get_temp::<(Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + let field_w = w - 38.0 - 14.0 - 16.0; + let press = |h: &mut egui_kittest::Harness<'static, PhotocraftApp>, at: Pos2, pressed: bool| { + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + }; + let at = o + vec2(38.0 + 8.0 + field_w * 0.5, 60.0); + press(&mut h, at, true); + let picked = h.state().session.tools.foreground; + assert_ne!(picked, [0.2, 0.4, 0.6, 1.0], "the press itself picks"); + let hsva = srgb_hsva(picked); + assert!((hsva.s - 0.5).abs() < 0.03 && (hsva.v - 0.5).abs() < 0.03, "{hsva:?}"); + h.event(egui::Event::PointerMoved(at + vec2(3.0, 0.0))); + h.run_steps(1); + assert_ne!(h.state().session.tools.foreground, picked, "a small drag adjusts the pick"); + press(&mut h, at + vec2(3.0, 0.0), false); + h.run_steps(1); + // Hue strip: a press a third of the way down is a hue of about 240° at once. + let before = h.state().session.tools.foreground; + press(&mut h, o + vec2(38.0 + 8.0 + field_w + 8.0 + 7.0, 40.0), true); + let now = h.state().session.tools.foreground; + assert_ne!(now, before, "the strip picks on press"); + assert!((srgb_hsva(now).h * 360.0 - 240.0).abs() < 8.0, "{:?}", srgb_hsva(now)); + } + + /// Only the primary button picks: a right or middle press (and its release) on the field or + /// the hue strip leaves the foreground colour alone. + #[test] + fn color_panel_ignores_secondary_and_middle_presses() { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.session.tools.foreground = [0.2, 0.4, 0.6, 1.0]; + let mut h = egui_kittest::Harness::builder().with_size(vec2(300.0, 300.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + let at = (ui.cursor().min, ui.available_width()); + ui.data_mut(|d| d.insert_temp(egui::Id::new("test-origin"), at)); + color_field(app, ui); + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Studio); + h.run_steps(2); + let (o, w) = h.ctx.data(|d| d.get_temp::<(Pos2, f32)>(egui::Id::new("test-origin"))).unwrap(); + let field_w = w - 38.0 - 14.0 - 16.0; + let field = o + vec2(38.0 + 8.0 + field_w * 0.5, 60.0); + let strip = o + vec2(38.0 + 8.0 + field_w + 8.0 + 7.0, 40.0); + for at in [field, strip] { + for button in [egui::PointerButton::Secondary, egui::PointerButton::Middle] { + for pressed in [true, false] { + h.event(egui::Event::PointerMoved(at)); + h.event(egui::Event::PointerButton { pos: at, button, pressed, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + assert_eq!(h.state().session.tools.foreground, [0.2, 0.4, 0.6, 1.0], "{button:?} pressed={pressed} at {at:?}"); + } + } + } + } + /// Click the hex field, select its text and type `text` one key per frame. fn type_hex(h: &mut egui_kittest::Harness<'static, PhotocraftApp>, text: &str) { use egui_kittest::kittest::Queryable; diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index ad5cba2105..1210c6c804 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -3001,6 +3001,16 @@ pub(crate) fn color_swatch(ui: &mut Ui, color: Color32) -> Response { ui.painter().rect_stroke(rect, t.radius_sm, Stroke::new(1.0, t.field_border), StrokeKind::Inside); response } +/// Where a press or drag on a picking surface (a colour field, a hue strip) points, from the +/// frame the button goes down and on every move after it. `dragged()` waits for egui's click +/// threshold (a few points of travel, or a long hold), so a press picked nothing until the +/// pointer had moved away and fine adjustments jumped (#7793). Only the primary button picks: +/// `is_pointer_button_down_on` is true for any button, so a right or middle press is ignored. +pub fn pick_pos(resp: &Response) -> Option { + let held = resp.is_pointer_button_down_on() + && resp.ctx.input(|i| i.pointer.primary_down() && !i.pointer.button_down(egui::PointerButton::Secondary) && !i.pointer.middle_down()); + if held || resp.clicked() { resp.interact_pointer_pos() } else { None } +} /// A colour swatch that only reports clicks (no popup): for colours edited in PhotoCraft's own /// Color Picker dialog. `label` names it for hover text and accessibility. pub fn color_swatch_button(ui: &mut Ui, color: Color32, label: &str) -> Response { From f3941972df3b954e20ce3d20997d729f94aec9cf Mon Sep 17 00:00:00 2001 From: Perchenko Anton <101743951+PerchAnton@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:34:54 +0300 Subject: [PATCH 05/16] fix(actions): preserve scalar Selective Color settings in ATN export (#7681) Co-authored-by: Brandon Thomas --- crates/engine/src/atn_cmds.rs | 56 +++---- crates/engine/tests/it/atn_selective_color.rs | 147 ++++++++++++++++++ crates/engine/tests/it/main.rs | 1 + 3 files changed, 169 insertions(+), 35 deletions(-) create mode 100644 crates/engine/tests/it/atn_selective_color.rs diff --git a/crates/engine/src/atn_cmds.rs b/crates/engine/src/atn_cmds.rs index cedfa5aeed..c863331523 100644 --- a/crates/engine/src/atn_cmds.rs +++ b/crates/engine/src/atn_cmds.rs @@ -764,46 +764,32 @@ fn selective_from(d: &Descriptor) -> Option { } fn selective_to(p: &Value) -> Option { - // Each range, as the command reads it (`adjust_cmds::selective_from_params`): the array form - // `"reds": [c, m, y, k]` first, then the scalar form, which names one range in `"colors"` and - // gives the channels it changes. Writing only the arrays dropped the scalar form's values, and - // the step exported as an adjustment that changes nothing (#7672). - let mut rows: Vec> = vec![None; SELECTIVE_RANGES.len()]; - for (row, (_, name)) in rows.iter_mut().zip(SELECTIVE_RANGES) { - if let Some(a) = p.get(*name).and_then(Value::as_array) { - let v = |i: usize| a.get(i).and_then(Value::as_f64).unwrap_or(0.0); - *row = Some([v(0), v(1), v(2), v(3)]); - } - } - if let Some(named) = jstr(p, "colors") { - let i = SELECTIVE_RANGES.iter().position(|(_, name)| *name == named)?; - let row = rows.get_mut(i)?; - let mut v = row.unwrap_or([0.0; 4]); - for (slot, key) in v.iter_mut().zip(["cyan", "magenta", "yellow", "black"]) { - // A channel that is given but is not a number is an error in the command - // (`adjust_cmds::selective_from_params`), so a step carrying one cannot be written - // either: ignoring it would export a different adjustment than the one recorded. - // `null` counts as absent there, and so here. - match p.get(key) { - None | Some(Value::Null) => {} - Some(_) => *slot = jnum(p, key)?, - } - } - *row = Some(v); - } + // Match command execution: scalar components override only the selected range, `relative` + // takes precedence over `method`, and invalid values must not become a different edit. + let photocraft_doc::Adjustment::SelectiveColor { relative, adjustments } = crate::adjust_cmds::selective_from_params(p, None).ok()? else { + return None; + }; let mut list = Vec::new(); - for (row, (ps, _)) in rows.iter().zip(SELECTIVE_RANGES) { - if let Some(v) = row { + for (name, row) in crate::adjust_cmds::RANGES.into_iter().zip(adjustments) { + if p.get(name).is_some_and(|v| !v.is_null()) || jstr(p, "colors") == Some(name) { + let ps = lookup_back(SELECTIVE_RANGES, name)?; + let [cyan, magenta, yellow, black] = row.map(f64::from); list.push(D::Descriptor( - desc("ClrC").with("Clrs", enm("Clrs", ps)).with("Cyn ", pct(v[0])).with("Mgnt", pct(v[1])).with("Yllw", pct(v[2])).with("Blck", pct(v[3])), + desc("ClrC") + .with("Clrs", enm("Clrs", ps)) + .with("Cyn ", pct(cyan)) + .with("Mgnt", pct(magenta)) + .with("Yllw", pct(yellow)) + .with("Blck", pct(black)), )); } } - let mut d = desc("SlcC").with("presetKind", enm("presetKindType", "presetKindCustom")); - if let Some(m) = jstr(p, "method") { - d = d.with("Mthd", enm("CrcM", if m == "absolute" { "Absl" } else { "Rltv" })); - } - Some(d.with("ClrC", D::List(list))) + Some( + desc("SlcC") + .with("presetKind", enm("presetKindType", "presetKindCustom")) + .with("Mthd", enm("CrcM", if relative { "Rltv" } else { "Absl" })) + .with("ClrC", D::List(list)), + ) } /// Gradient Map: the gradient's colour stops at 0..4096 → `stops: [[0..1, "#rrggbb"], …]`. diff --git a/crates/engine/tests/it/atn_selective_color.rs b/crates/engine/tests/it/atn_selective_color.rs new file mode 100644 index 0000000000..454846f59e --- /dev/null +++ b/crates/engine/tests/it/atn_selective_color.rs @@ -0,0 +1,147 @@ +//! #7672: strict ATN export must preserve both accepted Selective Color parameter forms. +use photocraft_engine::actions_cmds::Action; +use photocraft_engine::{Session, atn_cmds}; +use serde_json::{Value, json}; + +const RANGES: [&str; 9] = ["reds", "yellows", "greens", "cyans", "blues", "magentas", "whites", "neutrals", "blacks"]; +const COMMANDS: [&str; 2] = ["image.adjustments.selectiveColor", "layer.newAdjustmentLayer.selectiveColor"]; + +fn fixture(depth: u32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 10, "height": 2, "depth": depth})).unwrap(); + let palette = [ + [0.85, 0.10, 0.05, 1.0], + [0.85, 0.80, 0.05, 1.0], + [0.05, 0.85, 0.10, 1.0], + [0.05, 0.80, 0.85, 1.0], + [0.10, 0.05, 0.85, 1.0], + [0.85, 0.05, 0.80, 1.0], + [0.90, 0.93, 0.95, 1.0], + [0.45, 0.50, 0.55, 1.0], + [0.05, 0.08, 0.10, 1.0], + [0.65, 0.20, 0.70, 0.4], + ]; + s.edit("synthetic colours", |doc, active| { + let surface = doc.layer_mut(active.unwrap()).unwrap().surface_mut().unwrap(); + let format = surface.format(); + let bytes: Vec<_> = palette.iter().cycle().take(20).flat_map(|rgba| photocraft_raster::from_rgba(&format, *rgba)).collect(); + surface.write_region(doc_bounds(), &bytes); + Ok(()) + }) + .unwrap(); + s +} + +fn doc_bounds() -> photocraft_doc::Rect { + photocraft_doc::Rect::from_xywh(0, 0, 10, 2) +} + +fn pixels(s: &Session) -> Vec<[f32; 4]> { + photocraft_compose::flatten(&s.active().unwrap().doc).px +} + +fn check_cases(id: &str, cases: &[Value], require_effect: bool) { + let mut failures = Vec::new(); + for depth in [8, 16, 32] { + for params in cases { + let action = Action::new("Selective Color", vec![(id.into(), params.clone())]); + let mut direct = fixture(depth); + let before = pixels(&direct); + direct.execute(id, params.clone()).unwrap(); + let expected = pixels(&direct); + if require_effect { + assert_ne!(expected, before, "ineffective fixture: {id}, depth {depth}, {params}"); + } + + let mut native = fixture(depth); + native.actions.list.push(action.clone()); + native.execute("actions.play", json!({"action": action.name})).unwrap(); + assert_eq!(pixels(&native), expected, "native replay: {id}, depth {depth}, {params}"); + + let (bytes, skipped) = atn_cmds::export("Regression", &[&action], &[], false).unwrap(); + assert!(skipped.is_empty()); + let imported = atn_cmds::import(&bytes).unwrap(); + assert!(imported.unsupported.is_empty()); + assert!(imported.warnings.is_empty()); + assert_eq!(imported.actions.len(), 1); + assert_eq!(imported.actions[0].steps.len(), 1); + assert_eq!(imported.actions[0].steps[0].0, id); + + let mut replay = fixture(depth); + replay.actions.list = imported.actions; + replay.execute("actions.play", json!({"action": action.name})).unwrap(); + if pixels(&replay) != expected { + failures.push(format!("{id}, depth {depth}, {params}")); + } + } + } + assert!(failures.is_empty(), "ATN playback changed the result:\n{}", failures.join("\n")); +} + +fn scalar_cases() -> Vec { + RANGES + .iter() + .flat_map(|range| { + ["relative", "absolute"].map(|method| json!({"method": method, "colors": range, "cyan": 35, "magenta": -12.5, "yellow": 17.75, "black": 9})) + }) + .collect() +} + +#[test] +fn scalar_selective_color_actions_preserve_pixels_at_every_depth() { + let mut cases = scalar_cases(); + cases.push(json!({"method": "absolute", "colors": "reds", "cyan": 35})); + check_cases(COMMANDS[0], &cases, true); +} + +#[test] +fn scalar_selective_color_adjustment_layers_preserve_pixels_at_every_depth() { + check_cases(COMMANDS[1], &scalar_cases(), true); +} + +#[test] +fn mixed_forms_method_precedence_nulls_and_clamping_keep_their_effect() { + let cases = [ + json!({"method": "absolute", "reds": [10, 20, 30, 40], "blues": [-10, 5, 0, 4], "colors": "reds", "cyan": 0, "yellow": -17.5}), + json!({"method": "relative", "relative": false, "colors": "reds", "cyan": 35}), + json!({"method": "absolute", "relative": true, "colors": "neutrals", "black": 15}), + json!({"colors": "blues", "magenta": -25}), + json!({"method": null, "relative": null, "reds": null, "colors": "reds", "cyan": 35, "black": null}), + json!({"method": "absolute", "colors": "reds", "cyan": 175, "magenta": -150}), + json!({"method": "absolute", "reds": [175, -150, 0, 0]}), + json!({"method": "absolute", "reds": [10, 20, 30, 40], "colors": "reds", "cyan": null}), + json!({}), + json!({"colors": "reds"}), + ]; + for id in COMMANDS { + check_cases(id, &cases, false); + } +} + +#[test] +fn per_range_array_controls_still_preserve_pixels() { + let cases: Vec<_> = RANGES.iter().map(|range| json!({"method": "absolute", *range: [35, -12.5, 17.75, 9]})).collect(); + for id in COMMANDS { + check_cases(id, &cases, true); + } +} + +#[test] +fn malformed_selective_color_params_cannot_be_silently_exported() { + for params in [ + json!({"colors": "purples", "cyan": 35}), + json!({"colors": 3}), + json!({"colors": "reds", "cyan": "35"}), + json!({"method": "sideways"}), + json!({"relative": "yes"}), + json!({"reds": [10, 20]}), + json!({"reds": [10, "20", 0, 0]}), + ] { + for id in COMMANDS { + let action = Action::new("Invalid", vec![(id.into(), params.clone())]); + assert!(atn_cmds::export("Regression", &[&action], &[], false).is_err(), "strict export accepted {id} {params}"); + let (_, skipped) = atn_cmds::export("Regression", &[&action], &[], true).unwrap(); + assert_eq!(skipped.len(), 1, "skipUnsupported must report {id} {params}"); + } + } +} diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index 1e31899a35..959992e019 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -4,6 +4,7 @@ mod affinity; mod artboard_export_names; mod atn_crystallize_cell_size; mod atn_ripple_amount; +mod atn_selective_color; mod atn_stroke_width; mod background_and_mask_depth; mod background_layer_consistency; From 6e5264150838372bf0594a6e8ca82b0a279fadc9 Mon Sep 17 00:00:00 2001 From: David Mann Date: Sat, 24 Oct 2026 00:35:08 +0200 Subject: [PATCH 06/16] Auto-Align: a locked layer becomes the reference instead of being moved (#7080) (#7081) * Auto-Align: a locked layer becomes the reference instead of being moved (#7080) Co-Authored-By: Claude Opus 5.5 * Address review: Auto-Align moves the Background as a normal layer With the Background selected and a Cylindrical or Spherical result (every layer is reprojected), or another layer as the reference, the Background's own position lock made Auto-Align refuse. Like Free Transform and Photoshop, the Background's built-in position lock no longer pins it: it becomes a normal layer ("Layer 0") when it has to move. Lock All, Lock Pixels and a user's Lock Position on any other layer still keep it as the reference. Co-Authored-By: Claude Opus 5.5 (1M context) * Address CodeRabbit review: the Auto-Align params doc names the locked layer as the default reference Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/engine/src/align_cmds.rs | 110 ++++++++++++++++++++++++++++++-- 1 file changed, 105 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/align_cmds.rs b/crates/engine/src/align_cmds.rs index d4cc56b2bd..99e3c8a590 100644 --- a/crates/engine/src/align_cmds.rs +++ b/crates/engine/src/align_cmds.rs @@ -44,6 +44,14 @@ fn selected_pixel_layers(s: &Session) -> Vec { st.selected_layers().into_iter().filter(|id| st.doc.layer(*id).is_some_and(|l| matches!(l.content, LayerContent::Raster(_)))).collect() } +/// Whether a lock keeps the layer `id` from being moved or warped: lock all, position or pixels. +/// The Background's own position lock doesn't count: like Free Transform, Auto-Align turns the +/// Background into a normal layer when it has to move it (Photoshop). +fn pinned(doc: &Document, id: LayerId) -> bool { + let l = doc.effective_locks(id); + l.all || l.pixels || (l.position && !doc.is_background(id)) +} + fn two_layers(s: &Session) -> std::result::Result<(), String> { s.active().ok_or("no document open")?; if selected_pixel_layers(s).len() < 2 { Err("select two or more pixel layers in the Layers panel".into()) } else { Ok(()) } @@ -63,7 +71,8 @@ fn auto_align(s: &mut Session, p: &Value) -> Result { let reference = match p.get("reference").and_then(Value::as_u64) { Some(r) if ids.contains(&LayerId(r)) => LayerId(r), Some(r) => return Err(bad(cmd, format!("reference {r} is not a selected pixel layer"))), - None => ids[0], + // As in Photoshop, a locked layer stays put: it becomes the reference (#7080). + None => ids.iter().copied().find(|id| pinned(&doc, *id)).unwrap_or(ids[0]), }; let ref_idx = ids.iter().position(|i| *i == reference).unwrap_or(0); let area = doc.bounds(); @@ -100,6 +109,9 @@ fn auto_align(s: &mut Session, p: &Value) -> Result { if moves.is_empty() { return Err(EngineError::Other("Auto-Align couldn't find enough matching detail between the layers".into())); } + if let Some(l) = moves.iter().find(|&&i| pinned(&doc, ids[i])).and_then(|&i| doc.layer(ids[i])) { + return Err(EngineError::Other(format!("Auto-Align can't move the locked layer \"{}\"", l.name))); + } let warped: Vec<(LayerId, Surface, Option)> = moves .iter() .map(|&i| { @@ -122,6 +134,8 @@ fn auto_align(s: &mut Session, p: &Value) -> Result { .collect::>()?; s.edit("Auto-Align Layers", |doc, _| { for (id, px, mask) in &warped { + // Photoshop turns the Background into a normal layer before moving it. + crate::extra_cmds::background_to_layer(doc, *id); let l = doc.layer_mut(*id).ok_or(EngineError::NoLayer(*id))?; if let Some(surf) = l.surface_mut() { *surf = px.clone(); @@ -331,7 +345,7 @@ pub fn specs() -> Vec { spec!( "edit.autoAlignLayers", "Auto-Align Layers…", - r##"{"projection":"auto|perspective|cylindrical|spherical|collage|reposition","reference":layer id?=bottom selected layer,"geometricCorrection":bool=false,"interpolation":"bicubic|bilinear|nearest","registration":"auto|features|intensity"}"##, + r##"{"projection":"auto|perspective|cylindrical|spherical|collage|reposition","reference":layer id?=the locked selected layer, else the bottom selected layer,"geometricCorrection":bool=false,"interpolation":"bicubic|bilinear|nearest","registration":"auto|features|intensity"}"##, auto_align ), spec!( @@ -350,8 +364,15 @@ mod tests { /// Deterministic texture written into a layer at offset (dx, dy). fn texture_layer(s: &mut Session, name: &str, dx: i32, dy: i32) -> LayerId { s.execute("layer.new.layer", json!({"name": name})).unwrap(); - s.edit("tex", |doc, active| { - let surf = doc.layer_mut(active.unwrap()).unwrap().surface_mut().unwrap(); + let id = s.active().unwrap().active_layer.unwrap(); + paint_texture(s, id, dx, dy); + id + } + + /// Writes the deterministic texture into layer `id` at offset (dx, dy). + fn paint_texture(s: &mut Session, id: LayerId, dx: i32, dy: i32) { + s.edit("tex", |doc, _| { + let surf = doc.layer_mut(id).unwrap().surface_mut().unwrap(); let mut seed = 99u64; let mut rnd = || { seed = seed.wrapping_mul(6364136223846793005).wrapping_add(1442695040888963407); @@ -367,7 +388,6 @@ mod tests { Ok(()) }) .unwrap(); - s.active().unwrap().active_layer.unwrap() } fn select(s: &mut Session, ids: &[LayerId]) { @@ -411,6 +431,86 @@ mod tests { assert!(s.execute("edit.autoAlignLayers", json!({"reference": u64::MAX})).is_err()); } + /// As in Photoshop, a locked layer stays where it is: it becomes the reference and the + /// others move onto it (#7080). + #[test] + fn auto_align_keeps_a_locked_layer_as_the_reference() { + for lock in ["all", "position", "pixels"] { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 300, "height": 240, "background": "transparent"})).unwrap(); + let a = texture_layer(&mut s, "A", 20, 20); + let b = texture_layer(&mut s, "B", 31, 13); + s.edit("lock", |doc, _| { + doc.layer_mut(b).unwrap().locks.set(lock, true); + Ok(()) + }) + .unwrap(); + select(&mut s, &[a, b]); + let r = s.execute("edit.autoAlignLayers", json!({"projection": "reposition"})).unwrap(); + assert_eq!(r["reference"], b.0, "{lock}: {r}"); + let doc = &s.active().unwrap().doc; + let bb = doc.layer(b).unwrap().surface().unwrap().content_bounds(); + assert_eq!((bb.x0, bb.y0), (31, 13), "{lock}: the locked layer stays put"); + let ab = doc.layer(a).unwrap().surface().unwrap().content_bounds(); + assert!((ab.x0 - 31).abs() <= 1 && (ab.y0 - 13).abs() <= 1, "{lock}: {ab:?} {r}"); + // An explicit reference that would move the locked layer is refused. + s.undo(); + assert!(s.execute("edit.autoAlignLayers", json!({"projection": "reposition", "reference": a.0})).is_err()); + // Two locked layers: nothing may move. + s.edit("lock", |doc, _| { + doc.layer_mut(a).unwrap().locks.set(lock, true); + Ok(()) + }) + .unwrap(); + assert!(s.execute("edit.autoAlignLayers", json!({"projection": "reposition"})).is_err()); + let bb = s.active().unwrap().doc.layer(a).unwrap().surface().unwrap().content_bounds(); + assert_eq!((bb.x0, bb.y0), (20, 20)); + } + } + + /// The Background doesn't block Auto-Align: when a projection reprojects every layer + /// (Cylindrical, Spherical, or Auto choosing one), or another layer is the reference, it + /// becomes a normal layer ("Layer 0") and moves, as in Photoshop and as Free Transform does. + #[test] + fn auto_align_moves_the_background_as_a_normal_layer() { + let setup = || { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 300, "height": 240, "background": "white"})).unwrap(); + let bg = s.active().unwrap().doc.background().unwrap().id; + paint_texture(&mut s, bg, 20, 20); + let b = texture_layer(&mut s, "B", 31, 13); + select(&mut s, &[bg, b]); + (s, bg, b) + }; + for projection in ["cylindrical", "spherical", "auto"] { + let (mut s, bg, b) = setup(); + let r = s.execute("edit.autoAlignLayers", json!({"projection": projection})).unwrap_or_else(|e| panic!("{projection}: {e}")); + let doc = &s.active().unwrap().doc; + let moved = r["aligned"].as_array().unwrap().iter().any(|a| a["layer"] == bg.0); + assert_eq!(moved, projection != "auto" || r["layout"] != "perspective", "{projection}: {r}"); + assert_eq!(doc.is_background(bg), !moved, "{projection}: a moved Background is a normal layer: {r}"); + if moved { + assert_eq!(doc.layer(bg).unwrap().name, "Layer 0", "{projection}"); + } + assert!(doc.layer(b).is_some(), "{projection}"); + s.undo(); + assert!(s.active().unwrap().doc.is_background(bg), "{projection}: undo restores the Background"); + } + // Planar, with the other layer as the reference: the Background moves onto it. + let (mut s, bg, b) = setup(); + s.execute("edit.autoAlignLayers", json!({"projection": "reposition", "reference": b.0})).unwrap(); + let doc = &s.active().unwrap().doc; + assert!(!doc.is_background(bg)); + // Default reference: the Background (bottom) stays put and stays the Background. + let (mut s, bg, b) = setup(); + let r = s.execute("edit.autoAlignLayers", json!({"projection": "reposition"})).unwrap(); + assert_eq!(r["reference"], bg.0, "{r}"); + let doc = &s.active().unwrap().doc; + assert!(doc.is_background(bg)); + let bb = doc.layer(b).unwrap().surface().unwrap().content_bounds(); + assert!((bb.x0 - 20).abs() <= 1 && (bb.y0 - 20).abs() <= 1, "{bb:?} {r}"); + } + /// The moving layer's mask is warped as a mask: one grey channel whose outside keeps its /// default (reveal-all stays revealed, hide-all stays hidden), for planar and mesh warps. #[test] From 15db2d856057ef06465ede54cb7188baee1f774a Mon Sep 17 00:00:00 2001 From: Lucas Rodrigues Date: Fri, 23 Oct 2026 19:35:21 -0300 Subject: [PATCH 07/16] Paste: keep the pasted image on the canvas (#6759) (#6798) * Paste: keep the pasted image on the canvas (#6759) Paste centred the clipboard image on the view centre with no clamp, so an image that fit the canvas could land partly off it, and one larger than the canvas could leave blank edges. Now, per axis, an image that fits stays wholly inside the canvas, as close to the view centre as it can, and one larger than the canvas starts at the canvas's top-left edge, as in Photoshop. Paste Into / Outside still centre on the selection ("clampToCanvas": false). Centres are computed edge by edge in f64, so extreme coordinates no longer overflow. * Paste: doc comments for Paste Into and the paste placement tests --------- Co-authored-by: Brandon Thomas --- crates/engine/src/edit_cmds.rs | 100 ++++++++++++++++++++++++++++++-- crates/engine/src/extra_cmds.rs | 6 +- crates/ui-egui/src/lib.rs | 8 ++- 3 files changed, 106 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/edit_cmds.rs b/crates/engine/src/edit_cmds.rs index 1ae19fcdad..1a6c3a4072 100644 --- a/crates/engine/src/edit_cmds.rs +++ b/crates/engine/src/edit_cmds.rs @@ -450,15 +450,35 @@ fn paste_layers(s: &mut Session, layers: &Document, dx: i32, dy: i32) -> Result< /// How far a paste moves the clipboard pixels: not at all in place (or when they lie on the /// canvas and no `center` is given), else onto `center` (the view centre from the UI) or the -/// canvas centre. +/// canvas centre, then kept on the canvas as Photoshop keeps it: pixels that fit stay wholly +/// inside, larger ones start at the canvas's top-left corner (#6759). `"clampToCanvas": false` +/// skips that, for Paste Into / Outside, which centre on the selection. pub(crate) fn paste_offset(clip: &Clip, canvas: Rect, p: &Value, in_place: bool) -> (i32, i32) { if in_place || (clip.bounds.intersect(&canvas) == clip.bounds && p.get("center").is_none()) { return (0, 0); } let c = p.get("center").and_then(Value::as_array).filter(|a| a.len() >= 2).map(|a| (a[0].as_f64().unwrap_or(0.0), a[1].as_f64().unwrap_or(0.0))); - let (cx, cy) = c.unwrap_or(((f64::from(canvas.x0) + f64::from(canvas.x1)) / 2.0, (f64::from(canvas.y0) + f64::from(canvas.y1)) / 2.0)); + let (cx, cy) = c.unwrap_or((mid(canvas.x0, canvas.x1), mid(canvas.y0, canvas.y1))); let b = clip.bounds; - ((cx - (f64::from(b.x0) + f64::from(b.x1)) / 2.0).round() as i32, (cy - (f64::from(b.y0) + f64::from(b.y1)) / 2.0).round() as i32) + let (dx, dy) = ((cx - mid(b.x0, b.x1)).round(), (cy - mid(b.y0, b.y1)).round()); + if p.get("clampToCanvas").and_then(Value::as_bool) == Some(false) || b.is_empty() { + return (dx as i32, dy as i32); + } + (clamp_shift(dx, b.x0, b.x1, canvas.x0, canvas.x1), clamp_shift(dy, b.y0, b.y1, canvas.y0, canvas.y1)) +} + +/// The midpoint of `a..b`, each edge converted before adding so extreme coordinates can't overflow. +fn mid(a: i32, b: i32) -> f64 { + (f64::from(a) + f64::from(b)) / 2.0 +} + +/// Shift `d` of the span `b0..b1` along one axis, changed as little as needed to keep the span +/// inside `c0..c1` when it fits; a span longer than the canvas starts at `c0`. +fn clamp_shift(d: f64, b0: i32, b1: i32, c0: i32, c1: i32) -> i32 { + let (b0, b1, c0, c1) = (i64::from(b0), i64::from(b1), i64::from(c0), i64::from(c1)); + // `as` saturates a NaN or out-of-range shift, then the clamp brings it into range. + let start = if b1 - b0 <= c1 - c0 { b0.saturating_add(d as i64).clamp(c0, c1 - (b1 - b0)) } else { c0 }; + i32::try_from(start - b0).unwrap_or(if start > b0 { i32::MAX } else { i32::MIN }) } pub(crate) fn shifted(surface: &Surface, dx: i32, dy: i32) -> Surface { @@ -994,7 +1014,7 @@ pub fn specs() -> Vec { &["Edit"], Some("Cmd+V"), concat!( - r#"{"center":[x,y]? (view centre; default keeps the position when it overlaps the canvas),"#, + r#"{"center":[x,y]? (view centre; default keeps the position when it overlaps the canvas),"clampToCanvas":bool=true (keep the paste on the canvas, or at its top-left when larger),"#, paste_target!(), "} (with no document open: a new document from the clipboard)" ), @@ -1288,6 +1308,78 @@ mod tests { assert_eq!(s.active().unwrap().doc.layers.len(), 4); } + /// A `w`×`h` document with an opaque `cw`×`ch` image from another app on the clipboard. + fn external_clip_session(w: i32, h: i32, cw: i32, ch: i32) -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": w, "height": h})).unwrap(); + let mut surface = Surface::new(PixelFormat::RGBA8); + surface.fill_rect(Rect::new(0, 0, cw, ch), &[1.0, 0.0, 0.0, 1.0]); + s.clipboard = Some(Clip { surface, bounds: Rect::new(0, 0, cw, ch), layers: None, resolution: None }); + s + } + + /// #6759: wherever the view is centred, a paste that fits the canvas stays wholly on it. + #[test] + fn paste_that_fits_stays_on_the_canvas() { + for (w, h, center, want) in [ + (60, 100, [40.0, 45.0], Rect::new(0, 0, 60, 100)), + (60, 100, [-500.0, 900.0], Rect::new(0, 0, 60, 100)), + (100, 100, [10.0, 80.0], Rect::new(0, 0, 60, 100)), + (100, 100, [95.0, 50.0], Rect::new(40, 0, 100, 100)), + (100, 120, [50.0, 60.0], Rect::new(20, 10, 80, 110)), + ] { + let mut s = external_clip_session(w, h, 60, 100); + s.execute("edit.paste", json!({ "center": center })).unwrap(); + assert_eq!(active_bounds(&s), want, "{w}x{h} canvas, view centre {center:?}"); + } + } + + /// #6759: a paste larger than the canvas starts at its top-left corner, wherever the view is. + #[test] + fn paste_larger_than_the_canvas_starts_at_its_top_left() { + for center in [[0.0, 0.0], [25.0, 20.0], [1000.0, 1000.0], [-1000.0, 30.0]] { + let mut s = external_clip_session(50, 40, 60, 100); + s.execute("edit.paste", json!({ "center": center })).unwrap(); + assert_eq!(active_bounds(&s), Rect::new(0, 0, 60, 100), "view centre {center:?}"); + } + // Wider but not taller: at the left edge, and wholly inside vertically. + for (center, want) in + [([500.0, 500.0], Rect::new(0, 100, 60, 200)), ([-500.0, -500.0], Rect::new(0, 0, 60, 100)), ([25.0, 120.0], Rect::new(0, 70, 60, 170))] + { + let mut s = external_clip_session(50, 200, 60, 100); + s.execute("edit.paste", json!({ "center": center })).unwrap(); + assert_eq!(active_bounds(&s), want, "view centre {center:?}"); + } + } + + /// Paste Into centres on the selection even where that runs off the canvas. + #[test] + fn paste_into_is_not_clamped_to_the_canvas() { + let mut s = session(); + s.execute("select.rect", json!({"x": 10, "y": 10, "width": 40, "height": 20})).unwrap(); + s.execute("edit.copy", json!({})).unwrap(); + s.execute("select.rect", json!({"x": 90, "y": 90, "width": 10, "height": 10})).unwrap(); + s.execute("edit.pasteSpecial.pasteInto", json!({})).unwrap(); + assert_eq!(active_bounds(&s), Rect::new(75, 85, 115, 105)); + } + + /// The clamp never overflows or panics: NaN, infinite and `i32`-limit shifts saturate (Rule 9). + #[test] + fn paste_offset_clamp_survives_extreme_coordinates() { + assert_eq!(clamp_shift(f64::NAN, 0, 10, 0, 100), 0); + assert_eq!(clamp_shift(f64::INFINITY, 0, 10, 0, 100), 90); + assert_eq!(clamp_shift(f64::NEG_INFINITY, i32::MIN, i32::MAX, 0, 1), i32::MAX); + assert_eq!(clamp_shift(1e30, i32::MIN, i32::MIN + 1, i32::MAX - 1, i32::MAX), i32::MAX); + let clip = Clip { + surface: Surface::new(PixelFormat::RGBA8), + bounds: Rect::new(i32::MAX - 2, i32::MAX - 2, i32::MAX, i32::MAX), + layers: None, + resolution: None, + }; + let (dx, dy) = paste_offset(&clip, Rect::new(i32::MIN, i32::MIN, i32::MIN + 4, i32::MIN + 4), &json!({}), false); + assert_eq!((dx, dy), (i32::MIN, i32::MIN), "an unrepresentable shift saturates instead of wrapping"); + } + #[test] fn paste_with_malformed_center_does_not_panic() { // A short/empty `center` array must not panic (Rule 9) — it falls back to the canvas centre. diff --git a/crates/engine/src/extra_cmds.rs b/crates/engine/src/extra_cmds.rs index 4e2a265ab9..c7dc306d1a 100644 --- a/crates/engine/src/extra_cmds.rs +++ b/crates/engine/src/extra_cmds.rs @@ -336,6 +336,9 @@ fn can_reselect(s: &Session) -> std::result::Result<(), String> { reselect_target(s).map(|_| ()).ok_or_else(|| "there is no selection to restore".into()) } +/// Edit › Paste Special › Paste Into (or Outside): the clipboard centred on the selection, not +/// clamped to the canvas, as a new layer masked to the selection (or its inverse), or into the +/// targeted mask or channel. One history step. fn paste_into(s: &mut Session, p: &Value, outside: bool) -> Result { let d = s.active().ok_or(EngineError::NoDocument)?; let selection = d.doc.selection.clone().ok_or(EngineError::Other("Paste Into needs a selection".into()))?; @@ -353,7 +356,8 @@ fn paste_into(s: &mut Session, p: &Value, outside: bool) -> Result { selection }; let label = if outside { "Paste Outside" } else { "Paste Into" }; - let mut params = json!({"center": [(b.x0 + b.x1) as f64 / 2.0, (b.y0 + b.y1) as f64 / 2.0]}); + // Centred on the selection, even where that runs off the canvas (not clamped like a paste). + let mut params = json!({"center": [(b.x0 + b.x1) as f64 / 2.0, (b.y0 + b.y1) as f64 / 2.0], "clampToCanvas": false}); if let Some(c) = p.get("center") { params["center"] = c.clone(); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index cd9216893a..886fffecaf 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -3606,10 +3606,11 @@ mod clipboard_tests { app } + /// Paste is enabled from the clipboard service alone (no read); the explicit paste imports + /// the OS image, once, and centres it in the view like Photoshop does for foreign clipboard + /// content. #[test] fn external_clipboard_enables_and_centres_paste() { - // Paste is enabled from the clipboard service alone (no read); the explicit paste imports - // the OS image, once, and centres it like Photoshop does for foreign clipboard content. let (os, reads) = (OsClip::default(), Arc::default()); let mut app = app_with_os_clipboard(&os, &reads); *os.lock().unwrap() = Some((4, 4, [255u8, 0, 0, 255].repeat(16))); @@ -3617,8 +3618,9 @@ mod clipboard_tests { assert!(crate::menus::is_enabled(&app, "edit.paste")); assert!(crate::menus::is_enabled(&app, "edit.pasteSpecial.pasteInPlace")); assert_eq!(reads.load(std::sync::atomic::Ordering::SeqCst), 0, "enablement never reads the clipboard"); + app.ui.views[0].center = [40.0, 20.0]; let r = crate::menus::invoke(&mut app, &egui::Context::default(), "edit.paste", serde_json::json!({})).unwrap(); - assert_ne!(r["offset"], serde_json::json!([0, 0]), "external image is centred: {r}"); + assert_eq!(r["offset"], serde_json::json!([38, 18]), "external image is centred in the view: {r}"); assert_eq!(reads.load(std::sync::atomic::Ordering::SeqCst), 1, "one read per explicit paste"); // Without a clipboard service Paste greys until something is copied in the app. let mut plain = PhotocraftApp::new(Session::new(), Services::default()); From fe7f46c3f6105c1c0734de6e5a71a6ab38062a3d Mon Sep 17 00:00:00 2001 From: Michael Higgins <55243596+Higgs32584@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:35:34 -0400 Subject: [PATCH 08/16] PSD smart filters: Sharpen, Dust & Scratches and Hue/Saturation stay editable (#6267) (#6401) * PSD smart filters: Sharpen, Dust & Scratches and Hue/Saturation stay editable (#6267) They came in as psd.unsupportedFilter and were dropped when the smart object re-rendered (Replace Contents, Relink). They now map to filter.sharpen.sharpen (no Fltr, keyed by filterID), filter.noise. dustAndScratches (Rds/Thsh) and image.adjustments.hueSaturation (Clrz, Hst2 entries with LclR ranges), and are written back the same way. Co-Authored-By: Claude Opus 5.5 * rustfmt; merge main (CI Format step) Co-Authored-By: Claude Opus 5.5 * Apply suggestion from @coderabbitai[bot] Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * PSD smart filters: keep every Hue/Saturation range on import and export - A repeated `LclR` range (or a second master entry) in `Adjs` would overwrite the first on import; such a filter now stays verbatim as an unsupported filter instead. - Export rejects a colour range given as a non-object (`"reds": 5`), as the engine's Hue/Saturation params do, instead of dropping it. - Colorize's 25% saturation default applies to the master entry only; a colour range's saturation defaults to 0, as in the engine. Co-Authored-By: Claude Opus 5.5 * Remove DstS and Shrp filter handling from smart_map --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Brandon Thomas --- crates/io/src/hue_sat_filter_map.rs | 4 +- crates/io/src/smart_map.rs | 69 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/crates/io/src/hue_sat_filter_map.rs b/crates/io/src/hue_sat_filter_map.rs index d7ce04938d..05af178c27 100644 --- a/crates/io/src/hue_sat_filter_map.rs +++ b/crates/io/src/hue_sat_filter_map.rs @@ -173,7 +173,8 @@ pub(super) fn export_item(f: &SmartFilter) -> Result { Descriptor::new("Hst2").with("H ", master.0).with("Strt", master.1).with("Lght", setting(p, "lightness", 0.0, -100.0, 100.0)?), )]; for (i, key) in RANGES.iter().enumerate() { - if let Some(o) = p.get(*key) { + // A `null` range is an untouched one, like a missing key. + if let Some(o) = p.get(*key).filter(|o| !o.is_null()) { adjs.extend(range_entry(i, o)?); } } @@ -303,6 +304,7 @@ mod tests { assert_eq!(export(json!({"hue": 12.4})), json!({"colorize": false, "hue": 12, "saturation": 0, "lightness": 0})); assert_eq!(export(json!({"hue": -400, "lightness": 101, "layer": 3})), json!({"colorize": false, "hue": -180, "saturation": 0, "lightness": 100})); assert_eq!(export(J::Null), json!({"colorize": false, "hue": 0, "saturation": 0, "lightness": 0})); + assert_eq!(export(json!({"reds": null})), json!({"colorize": false, "hue": 0, "saturation": 0, "lightness": 0})); // Colorize starts at 25 % saturation and wraps the hue into 0..360. assert_eq!(export(json!({"colorize": true, "hue": -30})), json!({"colorize": true, "hue": 330, "saturation": 25, "lightness": 0})); // The full set (`to_params`): neutral ranges at their default extent are left out, reds diff --git a/crates/io/src/smart_map.rs b/crates/io/src/smart_map.rs index 78b2d79201..2322ec7bff 100644 --- a/crates/io/src/smart_map.rs +++ b/crates/io/src/smart_map.rs @@ -1731,6 +1731,13 @@ mod tests { sf("image.adjustments.levels", json!({"inBlack": 20, "inWhite": 220, "gamma": 1.4, "red": {"inBlack": 0, "inWhite": 255, "gamma": 0.8}})), sf("image.adjustments.curves", json!({"points": [[0.0, 0.0], [64.0, 40.0], [255.0, 255.0]], "blue": [[0.0, 20.0], [255.0, 235.0]]})), sf("image.adjustments.shadowsHighlights", json!({})), + sf("filter.sharpen.sharpen", json!({})), + // Photoshop stores Dust & Scratches' radius in whole pixels (a `long`). + sf("filter.noise.dustAndScratches", json!({"radius": 4, "threshold": 12})), + sf( + "image.adjustments.hueSaturation", + json!({"colorize": false, "hue": 10, "saturation": -20, "lightness": 5, "reds": {"hue": 5, "saturation": 0, "lightness": -3, "range": [315, 345, 15, 45]}}), + ), // A pencil-drawn curve: 256 output levels (`Mpng`). sf("image.adjustments.curves", json!({"pencil": true, "points": (0..256).rev().collect::>()})), ]; @@ -2580,6 +2587,68 @@ mod tests { assert_eq!(filter_from_item(&bare).command, UNSUPPORTED_FILTER); } + #[test] + fn photoshop_sharpen_without_fltr_imports_as_sharpen() { + // #6267: Photoshop's Sharpen item holds only its name, blending and `filterID`. + let item = Descriptor::new("filterFX") + .with("Nm ", Value::Text(UnicodeString::new_nul("Sharpen"))) + .with("blendOptions", Value::Descriptor(Descriptor::new("blendOptions").with("Opct", unit(b"#Prc", 100.0)).with("Md ", enumv("BlnM", "normal")))) + .with("enab", Value::Boolean(true)) + .with("hasoptions", Value::Boolean(false)) + .with("filterID", Value::Integer(1_399_353_968)); + let f = filter_from_item(&item); + assert_eq!((f.command.as_str(), &f.params), ("filter.sharpen.sharpen", &json!({}))); + // Hue/Saturation with an unnumbered range and Dust & Scratches with a missing setting fail + // cleanly instead of panicking. + let bad = SmartFilter { + command: "image.adjustments.hueSaturation".into(), + params: json!({"reds": {"range": [1, "x", 3, 4]}}), + blend: BlendMode::Normal, + opacity: 1.0, + visible: true, + }; + assert!(item_for_filter(&bad).is_err()); + let dust = Descriptor::new("filterFX") + .with("Fltr", Value::Descriptor(Descriptor::new("DstS").with("Rds ", Value::Integer(3)))) + .with("filterID", Value::Integer(code(b"DstS"))); + assert_eq!(filter_from_item(&dust).command, UNSUPPORTED_FILTER); + } + + #[test] + fn hue_saturation_ranges_are_not_lost() { + let hue_sat = |params: J| item_for_filter(&sf("image.adjustments.hueSaturation", params)); + // A range that is not an object is an error, not a silently dropped setting. + assert!(hue_sat(json!({"reds": 5})).is_err()); + assert!(hue_sat(json!({"reds": null})).is_ok()); + // Colorize's 25% default is the master saturation's only: a range's stays 0. + let d = hue_sat(json!({"colorize": true, "reds": {"hue": 5}})).unwrap(); + let adjs = adjustment_list(get_desc(&d, "Fltr").unwrap()).unwrap(); + let saturations: Vec<_> = adjs.iter().map(|a| int(a, "Strt")).collect(); + assert_eq!(saturations, [Some(25), Some(0)]); + // A repeated range (or master) would overwrite the first: the filter is kept verbatim. + let entry = |range: Option| { + let e = Descriptor::new("Hst2").with("H ", Value::Integer(0)).with("Strt", Value::Integer(0)).with("Lght", Value::Integer(0)); + let Some(r) = range else { return Value::Descriptor(e) }; + let e = ["BgnR", "BgnS", "EndS", "EndR"].iter().fold(e.with("LclR", Value::Integer(r)), |e, k| e.with(k, Value::Integer(10))); + Value::Descriptor(e) + }; + for adjs in [vec![entry(None), entry(Some(1)), entry(Some(1))], vec![entry(None), entry(None)]] { + let fltr = Descriptor::new("HStr").with("Clrz", Value::Boolean(false)).with("Adjs", Value::List(adjs)); + let item = Descriptor::new("filterFX") + .with("Nm ", Value::Text(UnicodeString::new_nul("Hue/Saturation..."))) + .with( + "blendOptions", + Value::Descriptor(Descriptor::new("blendOptions").with("Opct", unit(b"#Prc", 100.0)).with("Md ", enumv("BlnM", "normal"))), + ) + .with("enab", Value::Boolean(true)) + .with("Fltr", Value::Descriptor(fltr)) + .with("filterID", Value::Integer(code(b"HStr"))); + let f = filter_from_item(&item); + assert_eq!(f.command, UNSUPPORTED_FILTER); + assert_eq!(item_for_filter(&f).unwrap(), item); + } + } + #[test] fn unknown_filters_keep_their_descriptor() { let item = Descriptor::new("filterFX") From 117faadb7911d31f5f5c359307baf03db3e44496 Mon Sep 17 00:00:00 2001 From: Vikram Bhaskaran Date: Sat, 24 Oct 2026 09:35:47 +1100 Subject: [PATCH 09/16] Isolate live distortion tools per document (#5115) (#5447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Drop live distortion tools when switching or closing documents. Puppet Warp, Liquify, and the other modal distort tools kept session UI on a global prompt, so undo, delete, and pixels leaked across tabs. Cancel the live tool with the document that owns it. * Fix Liquify isolation test: new documents have nothing to undo. file.new leaves an empty history, so the second document needs a layer step before edit.undo can prove Ctrl+Z is not swallowed. * Stop expecting Puppet Warp's preview to follow a new document. display_doc is keyed to the session's document (#5115), so a File › New tab no longer shows the old warp. drop_foreign still closes the session. --------- Co-authored-by: Brandon Thomas --- crates/ui-egui/src/canvas.rs | 1 + crates/ui-egui/src/distort_ui.rs | 155 +++++++++++++++++++++++++-- crates/ui-egui/src/gallery_ui.rs | 6 +- crates/ui-egui/src/lib.rs | 1 + crates/ui-egui/src/liquify_ui.rs | 6 +- crates/ui-egui/src/perspective_ui.rs | 7 +- crates/ui-egui/src/puppet_ui.rs | 13 ++- crates/ui-egui/src/shortcuts.rs | 1 + crates/ui-egui/src/vp_ui.rs | 6 +- 9 files changed, 178 insertions(+), 18 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index d2543a54c2..4a885afcff 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1385,6 +1385,7 @@ pub(crate) fn display_doc(app: &mut PhotocraftApp, idx: usize) -> (std::sync::Ar } if let (Some(t), Some(pv)) = (&app.ui.transform, &app.transform_preview) && app.session.active_index() == Some(idx) + && pv.doc.id == st.doc.id && st.doc.layer(photocraft_doc::LayerId(t.layer)).is_some() { return (pv.doc.clone(), (1 << 40) + pv.session); diff --git a/crates/ui-egui/src/distort_ui.rs b/crates/ui-egui/src/distort_ui.rs index 5441a971ee..be1c0ddbc8 100644 --- a/crates/ui-egui/src/distort_ui.rs +++ b/crates/ui-egui/src/distort_ui.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use egui::{Color32, ColorImage}; -use photocraft_doc::{Document, LayerContent, LayerId}; +use photocraft_doc::{DocId, Document, LayerContent, LayerId}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -91,15 +91,50 @@ pub(crate) fn surface_image(surf: &Surface, r: Rect, max_side: usize) -> ColorIm ColorImage::new([tw, th], px) } +fn active_doc(app: &PhotocraftApp) -> Option { + app.session.active().map(|st| st.doc.id) +} + +fn on_doc(app: &PhotocraftApp, document: DocId) -> bool { + active_doc(app) == Some(document) +} + +/// Drops Liquify / Puppet / Perspective Warp / Vanishing Point / Filter Gallery when the +/// document they were opened on is no longer active, so they cannot steal undo, block layer +/// delete, or draw another document's pixels (#5115). +pub fn cancel_stale(app: &mut PhotocraftApp) { + let active = active_doc(app); + if app.distort.liquify.as_ref().is_some_and(|d| Some(d.document) != active) { + crate::liquify_ui::cancel(app); + } + if app.distort.puppet.as_ref().is_some_and(|p| Some(p.document) != active) { + app.distort.puppet = None; + } + if app.distort.perspective.as_ref().is_some_and(|p| Some(p.document) != active) { + app.distort.perspective = None; + } + if app.distort.vanishing.as_ref().is_some_and(|v| Some(v.document) != active) { + app.distort.vanishing = None; + } + if app.distort.gallery.as_ref().is_some_and(|g| Some(g.document) != active) { + app.distort.gallery = None; + } +} + /// The document shown on the canvas while Puppet or Perspective Warp previews a layer. pub fn display_doc(app: &PhotocraftApp, idx: usize) -> Option<(Arc, u64)> { if app.session.active_index() != Some(idx) { return None; } - if let Some(p) = &app.distort.puppet { + let id = app.session.documents().get(idx).map(|st| st.doc.id)?; + if let Some(p) = &app.distort.puppet + && p.document == id + { return Some((p.preview_doc.clone(), (1 << 42) + p.key)); } - if let Some(p) = &app.distort.perspective { + if let Some(p) = &app.distort.perspective + && p.document == id + { return Some((p.preview_doc.clone(), (1 << 42) + p.key)); } None @@ -137,23 +172,24 @@ pub fn menu(app: &mut PhotocraftApp, ctx: &egui::Context, id: &str, params: &Val /// Pointer events (document coordinates) while a mode is active. True when consumed. pub fn pointer(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modifiers) -> bool { + cancel_stale(app); if app.distort.caf.is_some() { crate::caf_ui::pointer(app, ev, mods); return true; } - if app.distort.liquify.is_some() { + if app.distort.liquify.as_ref().is_some_and(|d| on_doc(app, d.document)) { crate::liquify_ui::pointer(app, ev, mods); return true; } - if app.distort.puppet.is_some() { + if app.distort.puppet.as_ref().is_some_and(|p| on_doc(app, p.document)) { crate::puppet_ui::pointer(app, ev, mods); return true; } - if app.distort.perspective.is_some() { + if app.distort.perspective.as_ref().is_some_and(|p| on_doc(app, p.document)) { crate::perspective_ui::pointer(app, ev, mods); return true; } - if app.distort.vanishing.is_some() { + if app.distort.vanishing.as_ref().is_some_and(|v| on_doc(app, v.document)) { crate::vp_ui::pointer(app, ev, mods); return true; } @@ -164,6 +200,7 @@ pub fn pointer(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modifiers) -> /// of the rest of the app should be skipped. pub fn keys(app: &mut PhotocraftApp, ctx: &egui::Context) -> bool { use egui::{Key, Modifiers}; + cancel_stale(app); let enter = |ctx: &egui::Context| ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Enter)); let esc = |ctx: &egui::Context| ctx.input_mut(|i| i.consume_key(Modifiers::NONE, Key::Escape)); if app.distort.gallery.is_some() { @@ -245,19 +282,26 @@ pub fn draw_content(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform /// Canvas overlay for the on-canvas modes: pins, planes and meshes over the guides. pub fn draw_overlay(app: &PhotocraftApp, painter: &egui::Painter, xf: &ViewXform) { - if let Some(p) = &app.distort.puppet { + if let Some(p) = &app.distort.puppet + && on_doc(app, p.document) + { crate::puppet_ui::draw(p, painter, xf); } - if let Some(p) = &app.distort.perspective { + if let Some(p) = &app.distort.perspective + && on_doc(app, p.document) + { crate::perspective_ui::draw(p, painter, xf); } - if let Some(v) = &app.distort.vanishing { + if let Some(v) = &app.distort.vanishing + && on_doc(app, v.document) + { crate::vp_ui::draw(v, painter, xf); } } /// Options bar for the on-canvas modes. True when drawn. pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui) -> bool { + cancel_stale(app); if app.distort.puppet.is_some() { crate::puppet_ui::options_bar(app, ui); return true; @@ -295,6 +339,7 @@ pub fn drop_foreign(app: &mut PhotocraftApp) { /// Full-window dialogs (Liquify, Content-Aware Fill, Filter Gallery). pub fn show(app: &mut PhotocraftApp, ctx: &egui::Context) { + cancel_stale(app); drop_foreign(app); if app.distort.caf.is_some() { crate::caf_ui::show(app, ctx); @@ -312,3 +357,93 @@ pub(crate) fn tolerance(app: &PhotocraftApp) -> f64 { let z = app.point_zoom().max(0.01); 8.0 / f64::from(z) } + +#[cfg(test)] +mod tests { + use super::*; + use photocraft_geom::Rect; + use serde_json::json; + + fn paint_bar(app: &mut PhotocraftApp) { + app.session + .edit("paint", |doc, active| { + doc.layer_mut(active.unwrap()).unwrap().surface_mut().unwrap().fill_rect(Rect::new(20, 30, 80, 50), &[1.0, 0.0, 0.0, 1.0]); + Ok(()) + }) + .unwrap(); + } + + /// #5115: Puppet Warp is per-document. Switching tabs drops the session so Ctrl+Z and + /// layer delete apply to the new document, and the warp mesh does not leak onto it. + #[test] + fn switching_documents_drops_puppet_and_restores_undo_and_delete() { + let ctx = egui::Context::default(); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.execute("file.new", json!({"width": 100, "height": 80, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + paint_bar(&mut app); + app.sync_views(); + menu(&mut app, &ctx, "edit.puppetWarp", &json!({})).unwrap().unwrap(); + pointer(&mut app, ToolEvent::Down { x: 50.0, y: 40.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Up { x: 50.0, y: 40.0 }, egui::Modifiers::NONE); + assert!(app.distort.puppet.is_some()); + let first = app.session.active().unwrap().doc.id; + app.session.execute("file.new", json!({"width": 60, "height": 40, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + app.sync_views(); + assert!(app.distort.puppet.is_none(), "puppet does not follow the new document"); + assert!(display_doc(&app, app.session.active_index().unwrap()).is_none()); + let layers_before = app.session.active().unwrap().doc.layers.len(); + app.session.execute("layer.delete", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().doc.layers.len(), layers_before - 1); + assert!(app.session.set_active(0)); + app.sync_views(); + assert_eq!(app.session.active().unwrap().doc.id, first); + let past = app.session.active().unwrap().history.past_len(); + app.session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().history.past_len(), past - 1); + } + + #[test] + fn committed_puppet_is_one_undo_step_and_the_layer_stays_deletable() { + let ctx = egui::Context::default(); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.execute("file.new", json!({"width": 100, "height": 80, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + paint_bar(&mut app); + app.sync_views(); + menu(&mut app, &ctx, "edit.puppetWarp", &json!({})).unwrap().unwrap(); + pointer(&mut app, ToolEvent::Down { x: 25.0, y: 40.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Up { x: 25.0, y: 40.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Down { x: 75.0, y: 40.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Move { x: 75.0, y: 30.0, pressure: 1.0 }, egui::Modifiers::NONE); + pointer(&mut app, ToolEvent::Up { x: 75.0, y: 20.0 }, egui::Modifiers::NONE); + let before = app.session.active().unwrap().history.past_len(); + crate::puppet_ui::commit(&mut app); + assert!(app.distort.puppet.is_none()); + assert_eq!(app.session.active().unwrap().history.past_len(), before + 1); + app.session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().history.past_len(), before); + let layers = app.session.active().unwrap().doc.layers.len(); + app.session.execute("layer.delete", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().doc.layers.len(), layers - 1); + } + + #[test] + fn switching_documents_drops_liquify() { + let ctx = egui::Context::default(); + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); + app.session.execute("file.new", json!({"width": 100, "height": 80, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + paint_bar(&mut app); + crate::liquify_ui::open(&mut app, &ctx).unwrap(); + assert!(app.distort.liquify.is_some()); + app.session.execute("file.new", json!({"width": 40, "height": 40, "depth": 8})).unwrap(); + app.session.execute("layer.new.layer", json!({})).unwrap(); + app.sync_views(); + assert!(app.distort.liquify.is_none()); + let past = app.session.active().unwrap().history.past_len(); + app.session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(app.session.active().unwrap().history.past_len(), past - 1); + } +} diff --git a/crates/ui-egui/src/gallery_ui.rs b/crates/ui-egui/src/gallery_ui.rs index 89eac953e9..b5e50734f9 100644 --- a/crates/ui-egui/src/gallery_ui.rs +++ b/crates/ui-egui/src/gallery_ui.rs @@ -15,7 +15,7 @@ use std::hash::{Hash, Hasher}; use egui::{Align2, Color32, FontId, Rect as ERect, Sense, Stroke, TextureHandle, pos2, vec2}; use photocraft_algo::{FilterParams, GALLERY_CATEGORIES, GalleryFilter}; -use photocraft_doc::LayerId; +use photocraft_doc::{DocId, LayerId}; use photocraft_geom::Rect; use photocraft_raster::{Interrupt, Surface}; use serde_json::{Map, Value, json}; @@ -66,6 +66,8 @@ fn default_params(f: GalleryFilter) -> Map { } pub struct GalleryDialog { + /// Document this dialog belongs to (#5115). + pub document: DocId, pub layer: LayerId, layer_name: String, canvas: Rect, @@ -100,6 +102,7 @@ pub struct GalleryDialog { impl GalleryDialog { pub fn describe(&self) -> Value { json!({ + "document": self.document.0, "layer": self.layer.0, "effects": self.effects.iter().map(EffectLayer::to_json).collect::>(), "selected": self.selected, @@ -351,6 +354,7 @@ fn open_on( let open_cats = std::array::from_fn(|i| effects[sel].filter.category() == GALLERY_CATEGORIES[i]); let history = EditHistory::new(&effects); app.distort.gallery = Some(GalleryDialog { + document: st.doc.id, layer, layer_name, canvas, diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 886fffecaf..6e17af0a67 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1489,6 +1489,7 @@ impl PhotocraftApp { // ... and to the document it was started on. crate::canvas::polygon_cancel_stale(self); crate::crop_ui::cancel_stale(self); + crate::distort_ui::cancel_stale(self); crate::perspective_crop_ui::cancel_stale(self); // A pen path under construction belongs to the document it was started on too. vector_ui::cancel_stale(self); diff --git a/crates/ui-egui/src/liquify_ui.rs b/crates/ui-egui/src/liquify_ui.rs index cf1c932518..43a792d3ff 100644 --- a/crates/ui-egui/src/liquify_ui.rs +++ b/crates/ui-egui/src/liquify_ui.rs @@ -10,7 +10,7 @@ use egui::{Align2, Color32, FontId, Pos2, Rect as ERect, Sense, Stroke, TextureHandle, pos2, vec2}; use photocraft_algo::liquify::{LiquifyField, LiquifyStroke, LiquifyTool, ProxyImage, auto_cell}; -use photocraft_doc::{Document, LayerId}; +use photocraft_doc::{DocId, Document, LayerId}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -153,6 +153,8 @@ impl LiquifyOpts { } pub struct LiquifyDialog { + /// Document this dialog belongs to (#5115). + pub document: DocId, pub layer: LayerId, layer_name: String, canvas: Rect, @@ -219,6 +221,7 @@ fn union_rect(a: Option, b: Rect) -> Option { impl LiquifyDialog { pub fn describe(&self) -> Value { json!({ + "document": self.document.0, "layer": self.layer.0, "tool": self.opts.tool, "size": self.opts.size, @@ -529,6 +532,7 @@ fn open_on( let out = vec![[0u8; 4]; proxy.w * proxy.h]; let center = [f64::from(canvas.x0 + canvas.x1) / 2.0, f64::from(canvas.y0 + canvas.y1) / 2.0]; let mut d = LiquifyDialog { + document: st.doc.id, layer, layer_name: name, canvas, diff --git a/crates/ui-egui/src/perspective_ui.rs b/crates/ui-egui/src/perspective_ui.rs index e39a791d4f..b383184975 100644 --- a/crates/ui-egui/src/perspective_ui.rs +++ b/crates/ui-egui/src/perspective_ui.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use egui::{Color32, Pos2, Stroke, TextureHandle, pos2}; use photocraft_algo::perspective::{PerspectiveMap, Plane, Straighten, linked_corners, plane_point, straighten}; -use photocraft_doc::{Document, LayerId}; +use photocraft_doc::{DocId, Document, LayerId}; use photocraft_geom::Rect; use serde_json::{Value, json}; @@ -39,6 +39,8 @@ enum Drag { pub struct PerspSession { pub key: u64, + /// Document this session belongs to (#5115). + pub document: DocId, pub layer: LayerId, command: String, bounds: Rect, @@ -56,7 +58,7 @@ const GRID: usize = 40; impl PerspSession { pub fn describe(&self) -> Value { - json!({"layer": self.layer.0, "mode": self.mode, "planes": self.planes}) + json!({"document": self.document.0, "layer": self.layer.0, "mode": self.mode, "planes": self.planes}) } fn update_grid(&mut self) { @@ -134,6 +136,7 @@ pub fn begin(app: &mut PhotocraftApp, ctx: &egui::Context, command: &str) -> Res let texture = ctx.load_texture(format!("persp-{key}"), crate::distort_ui::surface_image(&surface, bounds, 2048), egui::TextureOptions::LINEAR); let mut s = PerspSession { key, + document: st.doc.id, layer, command: command.to_string(), bounds, diff --git a/crates/ui-egui/src/puppet_ui.rs b/crates/ui-egui/src/puppet_ui.rs index 3045ca1ca6..831c8bad37 100644 --- a/crates/ui-egui/src/puppet_ui.rs +++ b/crates/ui-egui/src/puppet_ui.rs @@ -13,7 +13,7 @@ use std::sync::Arc; use egui::{Color32, Stroke, TextureHandle, pos2, vec2}; use photocraft_algo::puppet::{ITERATIONS, PuppetDensity, PuppetMesh, PuppetMode, PuppetPin, PuppetSolver, PuppetWarp, build_mesh}; -use photocraft_doc::{Document, LayerId}; +use photocraft_doc::{DocId, Document, LayerId}; use photocraft_geom::Rect; use photocraft_raster::Surface; use serde_json::{Value, json}; @@ -23,6 +23,8 @@ use crate::canvas::{ToolEvent, ViewXform}; pub struct PuppetSession { pub key: u64, + /// Document this session belongs to (#5115). + pub document: DocId, pub layer: LayerId, /// Command to commit with (`edit.puppetWarp` or `layer.smartObjects.puppetWarp`). command: String, @@ -98,6 +100,7 @@ impl PuppetSession { pub fn describe(&self) -> Value { json!({ + "document": self.document.0, "layer": self.layer.0, "pins": self.warp.pins, "mode": self.warp.mode, @@ -202,6 +205,7 @@ pub fn begin(app: &mut PhotocraftApp, ctx: &egui::Context, command: &str) -> Res let texture = ctx.load_texture(format!("puppet-{key}"), img, egui::TextureOptions::LINEAR); let mut s = PuppetSession { key, + document: st.doc.id, layer, command: command.to_string(), surface, @@ -760,10 +764,13 @@ mod tests { crate::distort_ui::drop_foreign(&mut app); assert!(app.distort.puppet.is_some()); app.session.execute("file.new", json!({"width": 32, "height": 32, "depth": 8})).unwrap(); - assert!(crate::distort_ui::display_doc(&app, 1).is_some()); + let active = app.session.active_index().unwrap(); + // The session stays until drop_foreign; the preview must not follow this document (#5115). + assert!(app.distort.puppet.is_some()); + assert!(crate::distort_ui::display_doc(&app, active).is_none()); crate::distort_ui::drop_foreign(&mut app); assert!(app.distort.puppet.is_none()); - assert!(crate::distort_ui::display_doc(&app, 1).is_none()); + assert!(crate::distort_ui::display_doc(&app, active).is_none()); // Closing the document it was started on drops it too. assert!(app.session.set_active(0)); crate::distort_ui::menu(&mut app, &ctx, "edit.puppetWarp", &json!({})).unwrap().unwrap(); diff --git a/crates/ui-egui/src/shortcuts.rs b/crates/ui-egui/src/shortcuts.rs index 3300b9d92e..023bbb4d79 100644 --- a/crates/ui-egui/src/shortcuts.rs +++ b/crates/ui-egui/src/shortcuts.rs @@ -459,6 +459,7 @@ pub fn handle(app: &mut PhotocraftApp, ctx: &egui::Context) { // Liquify is a full-window custom dialog with focusable sliders. egui can therefore claim // keyboard input before the distortion-mode handler below runs. Give Liquify's local // shortcuts first refusal (Undo, brush size, tool keys), but never steal keys from text edits. + crate::distort_ui::cancel_stale(app); if app.distort.liquify.is_some() { // Undo and redo still apply while a numeric field (Size, Pressure, …) has focus. // Other Liquify keys must not steal characters from that field (#4027). diff --git a/crates/ui-egui/src/vp_ui.rs b/crates/ui-egui/src/vp_ui.rs index 3034c1c5f2..61ad0a4425 100644 --- a/crates/ui-egui/src/vp_ui.rs +++ b/crates/ui-egui/src/vp_ui.rs @@ -12,7 +12,7 @@ use egui::{Color32, Pos2, Stroke, TextureHandle, pos2}; use photocraft_algo::transform::Homography; use photocraft_algo::vanishing::{PlaneCheck, Scene, VpPlane}; -use photocraft_doc::{LayerContent, LayerId}; +use photocraft_doc::{DocId, LayerContent, LayerId}; use photocraft_engine::vp_cmds::VP; use photocraft_geom::Rect; use serde_json::{Value, json}; @@ -46,6 +46,8 @@ struct Clip { } pub struct VpSession { + /// Document this session belongs to (#5115). + pub document: DocId, pub layer: LayerId, canvas: Rect, pub planes: Vec, @@ -80,6 +82,7 @@ impl VpSession { let center = self.center(); let planes: Vec = self.planes.iter().map(|p| json!({"corners": p.corners, "check": p.check(center)})).collect(); json!({ + "document": self.document.0, "layer": self.layer.0, "tool": self.tool, "planes": planes, @@ -310,6 +313,7 @@ pub fn begin(app: &mut PhotocraftApp, ctx: &egui::Context) -> Result<(), String> }) }); app.distort.vanishing = Some(VpSession { + document: st.doc.id, layer, canvas, planes: Vec::new(), From 0b779b4fecf6ebbdf93956689a116989c6c872e1 Mon Sep 17 00:00:00 2001 From: FinnNk Date: Fri, 23 Oct 2026 23:36:01 +0100 Subject: [PATCH 10/16] fix(format): report a failed directory flush after a save instead of ignoring it (#5420) * fix(format): report a failed directory flush after a save instead of ignoring it atomic_write and the bundle writer discarded the directory fsync result once the rename had published the file, so a save whose rename might not survive a power loss reported success and its recovery data could be discarded. A real flush failure is now returned (the file is in place, the message says the save may not last), so callers keep the document unsaved and its recovery data, and a bundle save stops before removing objects the old manifest needs. A file system that can't flush directories at all (EINVAL, ENOTSUP, Unsupported) still counts as saved. Fixes #4968. Co-Authored-By: Claude Opus 5.5 * fix(format): a folder that can't be opened for a flush is not a failed save A write-and-search-only folder (no read permission) can't be opened to fsync it, so no flush can be attempted there. Treating that open failure as a failed flush made saves into such folders fail (the directory_symlinks permission tests on Linux). Only a flush that is attempted and fails is reported now. Co-Authored-By: Claude Opus 5.5 * fix(format): flush bundle folders like atomic_write, and test a failed manifest flush The bundle writer flushed the folder with sync_all, which on Apple platforms is F_FULLFSYNC and fails on SMB shares; it now goes through the same RealFs::sync_dir as atomic_write (sync_file's plain-fsync fallback, #1336). open_dir_for_sync and dir_synced are crate-private. A test-only hook fails the manifest's folder flush: the save fails, and the objects the previous manifest references stay until the next save. Co-Authored-By: Claude Opus 5.5 * fix(format): keep stale previews until the manifest commits; errno 45 only where it is ENOTSUP A directory save removed a thumbnail or composite the new document no longer has before writing the new manifest, so a failed manifest flush left the previous manifest naming a preview that was gone. They are now removed after the manifest commits, best-effort like unreferenced objects; the failed-flush test checks the thumbnail survives. errno 45 counted as "can't flush directories" on every Unix, but it is ENOTSUP only on Apple platforms and the BSDs (illumos: EDEADLK). Other Unix targets now accept only EINVAL and ErrorKind::Unsupported. Co-Authored-By: Claude Opus 5.5 * fix(format): flush the object folders before every manifest commit When an object's folder flush failed, a retried save found the object already written, reused it without flushing its folder, and then committed a manifest naming it: success was reported while the object's directory entry was still unconfirmed. Each directory save now flushes tiles/ and blobs/ once before writing the manifest, so a reused object is confirmed too. The flush error is shared with write_bundle_file (confirm_flushed), and a test retries a one-tile save whose tile folder flush failed. Co-Authored-By: Claude Opus 5.5 * fix(format): keep stale-preview removal before the manifest commit, as main requires f77ffa3e moved stale-preview removal after the manifest commit. That broke the hardening in 2d5a7497 (#4957), whose test denied_stale_preview_cleanup_preserves_the_previous_manifest requires a committed manifest never to sit beside a stale preview, so a failed removal must fail the save. Restore main's ordering; the object-folder flush and the failed-flush tests stay. Co-Authored-By: Claude Opus 5.5 * fix(format): flush the composite folder after removing a stale composite The stale composite/preview.png was removed before the manifest commit, but nothing flushed composite/, so a crash could bring it back beside the new manifest. The folder is now flushed when a stale composite was removed (the thumbnail lives in the bundle root, flushed with the manifest). Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/format/src/atomic.rs | 71 ++++++++++++++++- crates/format/src/atomic/tests.rs | 49 ++++++++++-- crates/format/src/store.rs | 125 +++++++++++++++++++++++++++++- 3 files changed, 232 insertions(+), 13 deletions(-) diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index 635bf924a2..8cd349efff 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -106,7 +106,7 @@ impl AtomicFs for RealFs { fn sync_dir(&self, dir: &Path) -> io::Result<()> { #[cfg(unix)] { - sync_file(&std::fs::File::open(dir)?) + open_dir_for_sync(dir)?.map_or(Ok(()), |d| sync_file(&d)) } #[cfg(windows)] { @@ -254,14 +254,79 @@ pub fn atomic_write_with_progress(fs: &dyn AtomicFs, retry: RenameRetry, path: & let _ = fs.remove(&tmp); return Err(rename_error(e, path)); } + // The new bytes are in place, but until the directory is flushed the rename may not survive + // a power loss (#4968). Windows fails the save on any failed flush (#7858). Elsewhere a file + // system that can't flush directories at all counts as done; any other failure is reported, + // so the caller keeps the document unsaved and its recovery data instead of discarding them + // on a save that may not last. #[cfg(windows)] fs.sync_dir(&dir).map_err(|e| publication_error(e, path))?; - // Keep the existing Unix policy here; #5420 separately changes its error handling. #[cfg(not(windows))] - let _ = fs.sync_dir(&dir); + dir_synced(fs.sync_dir(&dir)).map_err(|e| unconfirmed_error(e, path))?; Ok(()) } +/// `dir` opened to flush it, or `None` when it can't be opened for that because it has no read +/// permission (a write-and-search-only folder): no flush can be attempted there, so none failed. +/// Any other error is returned. +#[cfg(unix)] +fn open_dir_for_sync(dir: &Path) -> io::Result> { + match std::fs::File::open(dir) { + Ok(d) => Ok(Some(d)), + Err(e) if e.kind() == io::ErrorKind::PermissionDenied => Ok(None), + Err(e) => Err(e), + } +} + +/// `result` of flushing a directory, with "this file system can't flush directories" counted as +/// success: `EINVAL` (Linux file systems without directory fsync), `ENOTSUP`/`EOPNOTSUPP` and, +/// on Apple platforms, `ENOTTY`, or [`io::ErrorKind::Unsupported`]. Waiting or retrying won't +/// change those, and failing every save there would make saving impossible. Any other error is +/// returned: the flush was attempted and failed. +#[cfg(not(windows))] +pub(crate) fn dir_synced(result: io::Result<()>) -> io::Result<()> { + match result { + Err(e) if dir_sync_unsupported(&e) => Ok(()), + other => other, + } +} + +#[cfg(not(windows))] +fn dir_sync_unsupported(e: &io::Error) -> bool { + if e.kind() == io::ErrorKind::Unsupported { + return true; + } + // errno values: EINVAL is 22 everywhere; ENOTSUP and EOPNOTSUPP are 95 on Linux and + // Android, 45 and 102 on Apple platforms, 45 on the BSDs; ENOTTY is 25. Elsewhere 45 means + // something else (illumos: EDEADLK), so only EINVAL and `Unsupported` count there. + const EINVAL: i32 = 22; + let unsupported: &[i32] = if cfg!(any(target_os = "linux", target_os = "android")) { + &[EINVAL, 95] + } else if cfg!(target_vendor = "apple") { + &[EINVAL, 45, 102, 25] + } else if cfg!(any(target_os = "freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "dragonfly")) { + &[EINVAL, 45] + } else if cfg!(unix) { + &[EINVAL] + } else { + &[] + }; + e.raw_os_error().is_some_and(|code| unsupported.contains(&code)) +} + +/// The error for a file that was published but whose directory flush failed: same kind, saying +/// the file is in place but may not survive a power loss. (Windows reports `publication_error`.) +#[cfg(not(windows))] +fn unconfirmed_error(e: io::Error, path: &Path) -> io::Error { + context( + e, + format!( + "{} was written, but the folder could not be flushed to disk, so the save may not survive a power loss or crash; save again or to another folder", + path.display() + ), + ) +} + /// Run `rename`, retrying with backoff while it fails with `PermissionDenied` (see /// [`RenameRetry`]). For callers that rename through their own handles (capability directories). pub fn retry_rename(retry: RenameRetry, mut rename: impl FnMut() -> io::Result<()>) -> io::Result<()> { diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index d48cf7d9a6..bc26ae6d0a 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -41,8 +41,9 @@ struct Faulty { /// Fail the first N renames with PermissionDenied (a transient lock), then succeed. locked_renames: Cell, renames: Cell, - /// Failure of the durability barrier after publication (#7858). - fail_sync_dir: bool, + /// Fail the directory flush (the durability barrier after publication, #4968, #7858) + /// with this raw OS error, or with this kind. + fail_sync_dir: Option>, synced: Cell, } @@ -72,10 +73,11 @@ impl AtomicFs for Faulty { /// Count publication barriers and optionally fail before the real directory flush. fn sync_dir(&self, dir: &Path) -> io::Result<()> { self.synced.set(self.synced.get() + 1); - if self.fail_sync_dir { - return Err(io::Error::other("injected publication flush failure")); + match self.fail_sync_dir { + Some(Ok(code)) => Err(io::Error::from_raw_os_error(code)), + Some(Err(kind)) => Err(io::Error::new(kind, "injected directory flush failure")), + None => RealFs.sync_dir(dir), } - RealFs.sync_dir(dir) } fn copy_metadata(&self, from: &Path, to: &Path) -> io::Result<()> { RealFs.copy_metadata(from, to) @@ -92,7 +94,7 @@ fn a_failed_publication_flush_is_reported_and_can_be_retried() { let d = TempDir::new("publication-flush"); let p = d.0.join("document.pcraft"); std::fs::write(&p, ORIGINAL).unwrap(); - let fs = Faulty { fail_sync_dir: true, ..Default::default() }; + let fs = Faulty { fail_sync_dir: Some(Err(io::ErrorKind::Other)), ..Default::default() }; let error = atomic_write_with(&fs, NO_RETRY, &p, b"new document").unwrap_err(); assert_eq!(fs.synced.get(), 1, "{error}"); @@ -444,3 +446,38 @@ fn sync_file_flushes_a_local_file() { let file = std::fs::OpenOptions::new().write(true).create_new(true).open(&path).unwrap(); sync_file(&file).unwrap(); } + +/// #4968: the rename succeeded but the directory flush failed. The new bytes are in place, and +/// the save is reported as unconfirmed (same kind, saying so) instead of as a success, so the +/// caller keeps the document unsaved and its recovery data. +#[test] +fn a_failed_directory_flush_is_reported_not_swallowed() { + let d = TempDir::new("syncdir"); + let p = d.0.join("doc.pcraft"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = Faulty { fail_sync_dir: Some(Err(io::ErrorKind::Other)), ..Default::default() }; + let e = atomic_write_with(&fs, NO_RETRY, &p, b"new").unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::Other); + let msg = e.to_string(); + assert!(msg.contains("doc.pcraft") && msg.contains("may not survive"), "{msg}"); + assert_eq!(std::fs::read(&p).unwrap(), b"new", "published: the new bytes are in place"); + assert_no_temp(&d.0); +} + +/// A file system that can't flush directories at all (Linux's EINVAL, ENOTSUP, or an +/// `Unsupported` error) still saves: failing every save there would make saving impossible. +/// Windows fails the save on any failed flush (#7858), so this is the Unix rule. +#[cfg(not(windows))] +#[test] +fn a_directory_flush_the_file_system_cannot_do_still_saves() { + let d = TempDir::new("syncdir-unsupported"); + let p = d.0.join("doc.pcraft"); + let fs = Faulty { fail_sync_dir: Some(Err(io::ErrorKind::Unsupported)), ..Default::default() }; + atomic_write_with(&fs, NO_RETRY, &p, b"new").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"new"); + // EINVAL (22) means "can't flush a directory" only on Unix; EIO (5) is a real failure. + assert_eq!(dir_synced(Err(io::Error::from_raw_os_error(22))).is_ok(), cfg!(unix)); + assert!(dir_synced(Err(io::Error::from_raw_os_error(5))).is_err()); + let fs = Faulty { fail_sync_dir: Some(Ok(5)), ..Default::default() }; + assert!(atomic_write_with(&fs, NO_RETRY, &p, b"newer").is_err()); +} diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index 18e6a9b432..84124407e4 100644 --- a/crates/format/src/store.rs +++ b/crates/format/src/store.rs @@ -587,6 +587,50 @@ fn validate_bundle_file(root: &Path, path: &Path) -> Result<()> { plain_file_or_missing(path) } +/// Flush `dir`, the folder `written` was just renamed into, as `atomic_write` does (with the +/// full-flush fallback of `sync_file`, #1336). Windows flushes through the injected `AtomicFs` +/// and fails on any error (#7858). +#[cfg(not(windows))] +fn flush_bundle_dir(dir: &Path, written: &Path) -> std::io::Result<()> { + use crate::atomic::AtomicFs; + #[cfg(test)] + { + FLUSHED.with(|f| f.borrow_mut().push(dir.to_path_buf())); + let named = |p: Option<&Path>, name: &str| p.and_then(Path::file_name).is_some_and(|n| n == name); + if FAIL_DIR_FLUSH.with(|f| { + let hit = f.get().is_some_and(|name| named(Some(written), name) || named(written.parent(), name)); + if hit { + f.set(None); + } + hit + }) { + return Err(std::io::Error::other("injected folder flush failure")); + } + } + let _ = written; + crate::atomic::RealFs.sync_dir(dir) +} + +/// [`flush_bundle_dir`], as the save's error when the flush was attempted and failed: `written` +/// is in place, but may not survive a power loss or crash (#4968). +#[cfg(not(windows))] +fn confirm_flushed(dir: &Path, written: &Path) -> Result<()> { + crate::atomic::dir_synced(flush_bundle_dir(dir, written)).map_err(|e| { + FormatError::Io(std::io::Error::new( + e.kind(), + format!("{} was written, but its folder could not be flushed to disk, so the save may not survive a power loss or crash: {e}", written.display()), + )) + }) +} + +#[cfg(all(test, not(windows)))] +thread_local! { + /// Tests: the bundle file, or folder, whose next folder flush fails, once, on this thread. + static FAIL_DIR_FLUSH: std::cell::Cell> = const { std::cell::Cell::new(None) }; + /// Tests: the folders flushed on this thread, in order. + static FLUSHED: std::cell::RefCell> = const { std::cell::RefCell::new(Vec::new()) }; +} + /// Publish a validated bundle entry through a flushed temporary file and replacing rename. /// On Windows, propagate a failed directory barrier even though the new entry is already visible. fn write_bundle_file(root: &Path, path: &Path, data: &[u8], fs: &dyn crate::atomic::AtomicFs) -> Result<()> { @@ -639,10 +683,10 @@ fn write_bundle_file(root: &Path, path: &Path, data: &[u8], fs: &dyn crate::atom { fs.sync_dir(parent).map_err(|e| crate::atomic::publication_error(e, path))?; } - #[cfg(unix)] - { - let _ = std::fs::File::open(parent).and_then(|d| d.sync_all()); - } + // Elsewhere a failed flush is reported too (#4968), but a file system that can't flush a + // directory at all still saves, as in `atomic_write`. + #[cfg(not(windows))] + confirm_flushed(parent, path)?; Ok(()) })(); if result.is_err() && validate_bundle_file(root, &tmp).is_ok() { @@ -994,6 +1038,13 @@ impl PcraftWriter { fs.sync_dir(&dir.join(sub)).map_err(|e| crate::atomic::publication_error(e, &dir.join(sub)))?; } } + #[cfg(not(windows))] + for sub in ["tiles", "blobs"] { + let folder = dir.join(sub); + if folder.is_dir() { + confirm_flushed(&folder, &folder)?; + } + } write_bundle_file(dir, &dir.join(MANIFEST), &p.manifest, fs)?; // Omitted previews belong to the previous manifest until the new one commits. // After commit, a locked leftover is only housekeeping, just like unused objects. @@ -1156,6 +1207,72 @@ fn gc_pinned_directory_handles_resist_subdirectory_swap() { std::fs::remove_dir_all(root).unwrap(); } +/// #4968: a manifest whose folder can't be flushed fails the save, and the objects the previous +/// manifest references stay in place; the next save that flushes removes what is unreferenced. +#[cfg(all(test, not(windows)))] +#[test] +fn a_failed_manifest_flush_keeps_the_previous_objects() { + use std::sync::atomic::{AtomicU64, Ordering}; + + static NEXT: AtomicU64 = AtomicU64::new(0); + let dir = std::env::temp_dir().join(format!("pcraft-flush-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir_all(&dir).unwrap(); + let dir = std::fs::canonicalize(dir).unwrap(); + let mut doc = Document::new("flush", photocraft_doc::Size::new(64, 64), photocraft_color::ColorMode::Rgb, SampleType::U8); + let id = doc.insert_above(None, photocraft_doc::Layer::raster("paint", doc.pixel_format())); + let paint = |doc: &mut Document, red: f32| { + doc.layer_mut(id).unwrap().surface_mut().unwrap().fill_rect(photocraft_geom::Rect::new(0, 0, 64, 64), &[red, 0.5, 0.25, 1.0]); + }; + paint(&mut doc, 1.0); + let mut w = PcraftWriter::new(); + w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + let old = list_objects(&dir).unwrap(); + paint(&mut doc, 0.0); + FAIL_DIR_FLUSH.with(|f| f.set(Some(MANIFEST))); + let e = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap_err(); + assert!(e.to_string().contains("could not be flushed"), "{e}"); + let now = list_objects(&dir).unwrap(); + assert!(old.is_subset(&now), "the previous manifest's objects are kept"); + assert!(now.len() > old.len(), "the new objects were written"); + let stats = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + assert!(stats.objects_removed > 0, "the next save removes the unreferenced objects"); + assert_eq!(crate::load_path(&dir).unwrap(), doc); + std::fs::remove_dir_all(dir).unwrap(); +} + +/// An object whose folder flush failed is reused by the retried save, which writes no new object +/// there: that save still flushes the object folder before committing a manifest that names it. +#[cfg(all(test, not(windows)))] +#[test] +fn a_retried_save_flushes_the_folder_of_a_reused_object() { + use std::sync::atomic::{AtomicU64, Ordering}; + + static NEXT: AtomicU64 = AtomicU64::new(0); + let dir = std::env::temp_dir().join(format!("pcraft-retry-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir_all(&dir).unwrap(); + let dir = std::fs::canonicalize(dir).unwrap(); + // One layer of one tile: the retry finds its only object already written. + let mut doc = Document::new("retry", photocraft_doc::Size::new(16, 16), photocraft_color::ColorMode::Rgb, SampleType::U8); + let id = doc.insert_above(None, photocraft_doc::Layer::raster("paint", doc.pixel_format())); + doc.layer_mut(id).unwrap().surface_mut().unwrap().fill_rect(photocraft_geom::Rect::new(0, 0, 16, 16), &[1.0, 0.5, 0.25, 1.0]); + let mut w = PcraftWriter::new(); + FAIL_DIR_FLUSH.with(|f| f.set(Some("tiles"))); + let e = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap_err(); + assert!(e.to_string().contains("could not be flushed"), "{e}"); + assert!(!dir.join(MANIFEST).exists(), "nothing committed"); + let written = list_objects(&dir).unwrap(); + assert_eq!(written.iter().filter(|o| o.starts_with("tiles/")).count(), 1, "{written:?}"); + FLUSHED.with(|f| f.borrow_mut().clear()); + let stats = w.save_dir(&doc, &dir, &SaveOptions::default()).unwrap(); + assert_eq!(stats.tiles_written, 0, "the tile is reused"); + let flushed = FLUSHED.with(|f| f.borrow().clone()); + let tiles = flushed.iter().position(|d| *d == dir.join("tiles")); + let manifest = flushed.iter().rposition(|d| *d == dir); + assert!(tiles.is_some() && tiles < manifest, "tiles flushed before the manifest: {flushed:?}"); + assert_eq!(crate::load_path(&dir).unwrap(), doc); + std::fs::remove_dir_all(dir).unwrap(); +} + fn list_objects(dir: &Path) -> Result> { let mut out = HashSet::new(); for sub in ["tiles", "blobs"] { From 6a36c4867004b5ea7fb390cd1774361d473aaade Mon Sep 17 00:00:00 2001 From: A Hazy Reign Is Fluid <106657905+Ni-zav@users.noreply.github.com> Date: Sat, 24 Oct 2026 05:36:14 +0700 Subject: [PATCH 11/16] fix(automation): release idle headless TCP server ports (#3914) * fix(automation): release idle headless TCP server ports * fix(automation): retry aborted incoming TCP connections * test(automation): synchronize idle daemon client acceptance --------- Co-authored-by: Brandon Thomas --- crates/automation/src/rpc.rs | 99 ++++++++++++++++++++++++++++++------ docs/control-protocol.md | 5 ++ 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/crates/automation/src/rpc.rs b/crates/automation/src/rpc.rs index 7d52291f42..c0807838f0 100644 --- a/crates/automation/src/rpc.rs +++ b/crates/automation/src/rpc.rs @@ -22,7 +22,9 @@ use std::io::{BufRead, Write}; use std::net::TcpListener; use std::path::PathBuf; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex, PoisonError}; +use std::time::{Duration, Instant}; use base64::Engine as _; use serde_json::{Value, json}; @@ -315,32 +317,67 @@ fn serve_tcp_connection(h: &Mutex, stream: std::net::TcpStream, token: } } +/// Headless TCP daemons with no clients should eventually release their listening port (#3682). +const TCP_IDLE_TIMEOUT: Duration = Duration::from_secs(30 * 60); + /// Serve authenticated JSON lines on a loopback TCP address with one shared session. -/// Refuses non-loopback addresses and caps active connections and request bytes. +/// Refuses non-loopback addresses, caps connections and request bytes, and closes the listener +/// after an idle period with no connections. Active clients are never evicted by this timer. pub fn serve_tcp(addr: &str, h: Arc>, token: String, ready: impl FnOnce(std::net::SocketAddr)) -> Result<(), AutomationError> { + serve_tcp_until_idle(addr, h, token, ready, TCP_IDLE_TIMEOUT) +} + +fn serve_tcp_until_idle( + addr: &str, + h: Arc>, + token: String, + ready: impl FnOnce(std::net::SocketAddr), + idle: Duration, +) -> Result<(), AutomationError> { let listener = TcpListener::bind(addr).map_err(|e| AutomationError::Io(format!("bind {addr}: {e}")))?; let local = listener.local_addr().map_err(|e| AutomationError::Io(e.to_string()))?; if !local.ip().is_loopback() { return Err(bad(format!("{addr} is not a loopback address"))); } + listener.set_nonblocking(true).map_err(|e| AutomationError::Io(e.to_string()))?; ready(local); let limiter = ConnectionLimiter::new(MAX_CONNECTIONS); let token = Arc::new(token); - for stream in listener.incoming() { - let Ok(mut stream) = stream else { continue }; - let Some(permit) = limiter.try_acquire() else { - let _ = configure_stream(&stream); - let _ = writeln!(stream, "{}", json!({"id": null, "ok": false, "error": "connection limit reached"})); - continue; - }; - let h = h.clone(); - let token = Arc::clone(&token); - std::thread::spawn(move || { - let _permit = permit; - let _ = serve_tcp_connection(&h, stream, &token, AUTH_DEADLINE); - }); + let active = Arc::new(AtomicUsize::new(0)); + let mut last_active = Instant::now(); + loop { + match listener.accept() { + Ok((mut stream, _)) => { + last_active = Instant::now(); + let Some(permit) = limiter.try_acquire() else { + let _ = configure_stream(&stream); + let _ = writeln!(stream, "{}", json!({"id": null, "ok": false, "error": "connection limit reached"})); + continue; + }; + // The listener is nonblocking; individual requests still use blocking socket I/O. + let _ = stream.set_nonblocking(false); + let h = h.clone(); + let token = Arc::clone(&token); + let active = Arc::clone(&active); + active.fetch_add(1, Ordering::AcqRel); + std::thread::spawn(move || { + let _permit = permit; + let _ = serve_tcp_connection(&h, stream, &token, AUTH_DEADLINE); + active.fetch_sub(1, Ordering::AcqRel); + }); + } + Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { + if active.load(Ordering::Acquire) != 0 { + last_active = Instant::now(); + } else if last_active.elapsed() >= idle { + return Ok(()); + } + std::thread::sleep(Duration::from_millis(100)); + } + Err(e) if matches!(e.kind(), std::io::ErrorKind::Interrupted | std::io::ErrorKind::ConnectionAborted) => {} + Err(e) => return Err(AutomationError::Io(e.to_string())), + } } - Ok(()) } #[cfg(test)] @@ -1063,6 +1100,38 @@ mod tests { assert!(h.handle("methods", Value::Null).is_ok()); } + #[test] + fn idle_tcp_daemon_releases_its_port_but_keeps_active_clients() { + use std::io::BufReader; + const TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + let h = Arc::new(Mutex::new(Headless::new())); + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + let (done_tx, done_rx) = std::sync::mpsc::channel(); + let server = std::thread::spawn(move || { + let result = serve_tcp_until_idle("127.0.0.1:0", h, TOKEN.to_owned(), move |addr| ready_tx.send(addr).unwrap(), Duration::from_millis(200)); + done_tx.send(result).unwrap(); + }); + let addr = ready_rx.recv_timeout(Duration::from_secs(2)).unwrap(); + let mut client = std::net::TcpStream::connect(addr).unwrap(); + client.set_read_timeout(Some(Duration::from_secs(2))).unwrap(); + let mut reader = BufReader::new(client.try_clone().unwrap()); + writeln!(client, r#"{{"id":"auth","method":"auth","params":{{"token":"{TOKEN}"}}}}"#).unwrap(); + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + let auth: Value = serde_json::from_str(&line).unwrap(); + assert_eq!(auth["result"]["authenticated"], true); + // The port stays owned well past the idle limit while a client is connected. + assert!( + matches!(done_rx.recv_timeout(Duration::from_millis(450)), Err(std::sync::mpsc::RecvTimeoutError::Timeout)), + "daemon exited with a live client" + ); + drop(reader); + drop(client); + assert!(done_rx.recv_timeout(Duration::from_secs(3)).unwrap().is_ok(), "idle daemon did not exit cleanly"); + server.join().unwrap(); + assert!(TcpListener::bind(addr).is_ok(), "idle listener still owns its port"); + } + #[test] fn tcp_serves_loopback() { use std::io::{BufReader, Write as _}; diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 07d3a09183..673f5d15bd 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -361,6 +361,11 @@ capability interface. Interactive desktop file pickers retain normal user-select ## Headless server +An unused TCP headless server exits after 30 minutes without a client connection, releasing +its listening port so a later session can reuse it (#3682). An active connection prevents +idle shutdown, including during a long command. Clients should reconnect by restarting +`photocraft-cli serve` if they need a server after the idle deadline. + `photocraft-cli serve` keeps one headless engine session (no window, no GPU) and answers the same JSON-lines envelope on stdio, or on `127.0.0.1:` with `--port ` (loopback only; each authenticated connection shares the session). TCP uses the same first-frame `auth` exchange and From aaaa990314f653d2f410b72a65c521f56e6a5461 Mon Sep 17 00:00:00 2001 From: David Mann Date: Sat, 24 Oct 2026 00:36:28 +0200 Subject: [PATCH 12/16] Crop: Delete Cropped Pixels keeps type and smart objects whole (#7116) (#7117) * Crop: Delete Cropped Pixels keeps type and smart objects whole (#7116) crop_doc cropped every surface for_each_surface visits, including the rendered caches of type layers and smart objects. Those move by shifting their stored pixels, so the cut stayed and showed as soon as they were moved. Like Photoshop, Delete Cropped Pixels now deletes pixel-layer data (and fill caches) only; shapes are re-rendered as before. Co-Authored-By: Claude Opus 5.5 * Address review: Delete Cropped Pixels crops stored video frames again (#4956) The pixel-layer-only crop path skipped a video layer's stored frames and original footage, so scrubbing brought the cropped pixels back. Crop them with the displayed content, as before, and test a shrinking crop. Co-Authored-By: Claude Opus 5.5 (1M context) * Address CodeRabbit review: the turned crop keeps type and smart objects whole before rotating The pass that drops what a turned crop deletes before the rotation still cut every cached surface. The rotation re-renders type and smart objects, but a smart object whose source is unavailable keeps its cache, so it stayed clipped. That pass now uses the same pixel-layer-only rule as the crop itself. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/engine/src/image_cmds.rs | 107 +++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index d52522ca35..9259c5b2bd 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -371,6 +371,26 @@ pub(crate) fn translate_doc(doc: &mut Document, cmd: &str, dx: i32, dy: i32) -> Ok(()) } +/// Delete Cropped Pixels on a layer tree: crops pixel-layer data only to `r`. Type and smart +/// objects move by shifting their rendered pixels, so cutting those would clip them at the old +/// edge. A video layer's stored frames are its pixels too: the displayed content is only the frame +/// at the playhead, so the others would bring the cropped pixels back when scrubbing (#4956). +fn delete_cropped_pixels(layers: &mut [Layer], r: Rect) { + for_each_layer(layers, &mut |l| { + if let Some(fc) = &mut l.fill_cache { + fc.surface = crop_surface(&fc.surface, r); + } + if let Some(v) = &mut l.video { + for frame in v.frames.iter_mut().chain(v.original_frames.iter_mut().flatten()) { + *frame = crop_surface(frame, r); + } + } + if let LayerContent::Raster(s) = &mut l.content { + *s = crop_surface(s, r); + } + }); +} + /// Crops the document to `r` (in current document coordinates). /// A frame that extends past the canvas fills the Background's new area with `extension` (the /// background colour, as Photoshop's Crop tool does); other layers get transparent pixels there. @@ -382,7 +402,7 @@ fn crop_doc(doc: &mut Document, cmd: &str, r: Rect, delete_pixels: bool, extensi let old_canvas = doc.bounds(); let extends = r.x0 < old_canvas.x0 || r.y0 < old_canvas.y0 || r.x1 > old_canvas.x1 || r.y1 > old_canvas.y1; if delete_pixels { - for_each_surface(&mut doc.layers, false, &mut |surf, _| *surf = crop_surface(surf, r)); + delete_cropped_pixels(&mut doc.layers, r); } translate_doc(doc, cmd, dx, dy)?; doc.size = Size::new(r.width(), r.height()); @@ -753,7 +773,14 @@ fn rotated_crop(s: &mut Session, r: Rect, angle: f64, delete: bool, content_awar if delete { let keep = turned_crop_source(size, new, angle, target); s.edit("Crop", |doc, _| { - for_each_surface(&mut doc.layers, true, &mut |surf, _| *surf = crop_surface(surf, keep)); + // Type and smart objects stay whole, as in the crop itself: the rotation re-renders + // them, but a smart object whose source is unavailable keeps its cache. + delete_cropped_pixels(&mut doc.layers, keep); + for_each_layer(&mut doc.layers, &mut |l| { + if let Some(m) = &mut l.mask { + m.surface = crop_surface(&m.surface, keep); + } + }); for ch in doc.channels.iter_mut().chain(doc.quick_mask.as_mut()) { ch.surface = crop_surface(&ch.surface, keep); } @@ -1093,6 +1120,37 @@ mod tests { } } + /// Delete Cropped Pixels on a shrinking crop deletes the cropped area from every stored video + /// frame (and the original footage), not just the displayed one (#4956). + #[test] + fn delete_cropped_pixels_crops_stored_video_frames() { + let mut s = session(); + s.edit("video", |doc, active| { + let l = doc.layer_mut(active.unwrap()).unwrap(); + let red = l.surface().unwrap().clone(); + let mut blue = red.clone(); + blue.fill_rect(Rect::new(0, 0, 40, 20), &[0.0, 0.0, 1.0, 1.0]); + let mut v = photocraft_doc::VideoData::new(vec![red.clone(), blue.clone()], 24.0); + v.original_frames = Some(vec![red, blue]); + l.video = Some(v); + Ok(()) + }) + .unwrap(); + s.execute("image.crop", json!({"x": 15, "y": 0, "width": 25, "height": 20, "deleteCroppedPixels": true})).unwrap(); + assert_eq!(doc(&s).size, Size::new(25, 20)); + let v = doc(&s).layers[1].video.as_ref().unwrap(); + let originals = v.original_frames.as_ref().unwrap(); + assert_eq!((v.frames.len(), originals.len()), (2, 2)); + for frames in [&v.frames, originals] { + // The red square (10..20, 5..15) keeps only its right half; the full blue frame + // keeps exactly the 25×20 crop. + assert_eq!(frames[0].content_bounds(), Rect::new(0, 5, 5, 15)); + assert_eq!(frames[0].pixel(2, 10), vec![1.0, 0.0, 0.0, 1.0]); + assert_eq!(frames[1].content_bounds(), Rect::new(0, 0, 25, 20)); + assert_eq!(frames[1].pixel(24, 19), vec![0.0, 0.0, 1.0, 1.0]); + } + } + /// Resample on with only a new resolution keeps the print size, as Photoshop's Image Size /// does (an action's `imageSize` with `Rslt` alone); without `keepPrintSize` only the /// resolution changes. @@ -1400,6 +1458,51 @@ mod tests { assert_eq!(doc(&s).layers[0].surface().unwrap().rgba(1, 8), [0.0; 4]); } + /// Delete Cropped Pixels deletes pixel-layer data only: type and smart objects keep what lies + /// outside the frame, so moving them later doesn't show them cut at the old edge. + #[test] + fn delete_cropped_pixels_keeps_type_and_smart_objects_whole() { + let mut s = session(); + s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap(); + s.execute("type.create", json!({"x": 10, "y": 15, "text": "Hi", "size": 12})).unwrap(); + let bounds = |s: &Session| -> Vec { doc(s).layers[1..].iter().map(|l| l.surface().unwrap().content_bounds()).collect() }; + let before = bounds(&s); + assert!(before.iter().all(|b| b.x0 < 15 && b.x1 > 15), "both cross the crop edge: {before:?}"); + s.execute("image.crop", json!({"x": 15, "y": 0, "width": 25, "height": 20})).unwrap(); + assert!(matches!(doc(&s).layers[1].content, LayerContent::Smart(_)) && matches!(doc(&s).layers[2].content, LayerContent::Text(_))); + assert_eq!(bounds(&s), before.iter().map(|b| b.translate(-15, 0)).collect::>()); + } + + /// A turned crop deleting pixels keeps type and smart objects whole too, also a smart object + /// whose source is unavailable (the rotation can't re-render it, so it keeps its cache): each + /// ends up exactly as the same turned crop with Delete Cropped Pixels off leaves it. + #[test] + fn turned_crop_deleting_pixels_keeps_type_and_smart_objects_whole() { + let [deleted, kept] = [true, false].map(|delete| { + let mut s = session(); + s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap(); + s.edit("unlink", |doc, active| { + if let LayerContent::Smart(sm) = &mut doc.layer_mut(active.unwrap()).unwrap().content { + sm.source = photocraft_doc::SmartSource::Linked { path: "missing-photocraft-test-source.psd".into() }; + } + Ok(()) + }) + .unwrap(); + s.execute("type.create", json!({"x": 10, "y": 15, "text": "Hi", "size": 12})).unwrap(); + s.execute("image.crop", json!({"x": 20, "y": 0, "width": 20, "height": 20, "angle": 2.0, "deleteCroppedPixels": delete})).unwrap(); + s + }); + let (a, b) = (doc(&deleted), doc(&kept)); + assert!(matches!(a.layers[1].content, LayerContent::Smart(_)) && matches!(a.layers[2].content, LayerContent::Text(_))); + for i in [1, 2] { + let (sa, sb) = (a.layers[i].surface().unwrap(), b.layers[i].surface().unwrap()); + let r = sb.content_bounds(); + assert!(r.x0 < 0, "layer {i} reaches past the crop's left edge: {r:?}"); + assert_eq!(sa.content_bounds(), r, "layer {i}"); + assert_eq!(sa.read_region(r), sb.read_region(r), "layer {i}"); + } + } + /// The Background keeps the pixels a crop with Delete Cropped Pixels off hid: extending the /// canvas over them again (Reveal All, Canvas Size) shows them rather than painting them over /// with the background colour (#7086). From 80e83e0aa8084c26f39afeac92fffa07e02a3ef4 Mon Sep 17 00:00:00 2001 From: Michael Higgins <55243596+Higgs32584@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:36:41 -0400 Subject: [PATCH 13/16] PSD: keep Photoshop 2026 Light adjustment layers verbatim and warn (#6542) (#6815) A Light layer is stored under the Brightness/Contrast `brit` key with a `CgEd` descriptor carrying more settings; it imported as Brightness/Contrast with only Contrast. A `brit` whose `CgEd` has keys outside the Brightness/Contrast set now stays Adjustment::Unsupported, is written back byte for byte, and the import warns naming the layer. Co-authored-by: Claude Opus 5.5 Co-authored-by: Branch Sync Co-authored-by: Brandon Thomas --- crates/io/src/adjust_map.rs | 15 +++++++++++++++ crates/io/src/psd_export.rs | 36 ++++++++++++++++++++++++++++++++++++ crates/io/src/psd_import.rs | 10 ++++++++-- 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/crates/io/src/adjust_map.rs b/crates/io/src/adjust_map.rs index 053a38b99b..70d2479ee7 100644 --- a/crates/io/src/adjust_map.rs +++ b/crates/io/src/adjust_map.rs @@ -139,6 +139,19 @@ pub fn label(key: &[u8; 4]) -> &'static str { } } +/// Keys of a Brightness/Contrast `CgEd` descriptor: the ones [`write`] emits plus Photoshop's +/// preset bookkeeping. +const BRIT_CGED_KEYS: [&str; 9] = ["Vrsn", "Brgh", "Cntr", "means", "Lab ", "useLegacy", "Auto", "presetKind", "presetFileName"]; + +/// Whether a `brit` layer's `CgEd` descriptor holds settings Brightness/Contrast has no place +/// for. Photoshop 2026's Light adjustment (Exposure, Contrast, Highlights, Shadows, Whites, +/// Blacks) is stored under the `brit` key; read as Brightness/Contrast it kept only Contrast and +/// lost the rest without a word (#6542), so such a layer stays [`Adjustment::Unsupported`]. +pub fn is_extended_brit(cged: Option<&[u8]>) -> bool { + cged.and_then(|c| VersionedDescriptor::parse_prefix(c).ok()) + .is_some_and(|(v, _)| v.descriptor.items.iter().any(|(k, _)| !BRIT_CGED_KEYS.iter().any(|n| k.is(n)))) +} + /// Why [`parse`] keeps a block as [`Adjustment::Unsupported`], when there is more to say than /// "it could not be read" (so far: Color Lookup). pub fn unreadable_reason(key: &[u8; 4], data: &[u8]) -> Option { @@ -204,6 +217,8 @@ fn parse_any(key: &[u8; 4], data: &[u8], cged: Option<&[u8]>) -> Adjustment { b"nvrt" => Some(Adjustment::Invert), b"thrs" => be16(data, 0).map(|v| Adjustment::Threshold { level: f32::from(v) / 255.0 }), b"post" => be16(data, 0).map(|v| Adjustment::Posterize { levels: u32::from(v) }), + // Kept raw (with its `CgEd`, which the PSD writer keeps while the layer is unchanged). + b"brit" if is_extended_brit(cged) => None, b"brit" => { let modern = cged.and_then(|c| VersionedDescriptor::parse_prefix(c).ok()).and_then(|(v, _)| { let d = v.descriptor; diff --git a/crates/io/src/psd_export.rs b/crates/io/src/psd_export.rs index fd07adde50..6a476c553d 100644 --- a/crates/io/src/psd_export.rs +++ b/crates/io/src/psd_export.rs @@ -2113,6 +2113,42 @@ mod tests { assert_eq!(density, 0.5); } + #[test] + fn light_adjustment_is_kept_verbatim_and_warned() { + // #6542: Photoshop 2026's Light layer is a `brit` block whose `CgEd` descriptor carries + // more than Brightness/Contrast. Synthetic block: the extra key names are placeholders. + use photocraft_psd::descriptor::{Descriptor, Value, VersionedDescriptor}; + let brit = vec![0, 0, 0xff, 0xfe, 0, 127, 0, 0]; + let d = Descriptor::new("null") + .with("Vrsn", Value::Integer(1)) + .with("Brgh", Value::Integer(0)) + .with("Cntr", Value::Integer(-2)) + .with("Exps", Value::Double(-2.3)) + .with("Hghl", Value::Integer(-64)) + .with("useLegacy", Value::Boolean(false)); + let cged = VersionedDescriptor::new(d).to_bytes(); + let adjustment = crate::adjust_map::parse(b"brit", &brit, Some(&cged), crate::adjust_map::Channels::Rgb); + assert_eq!(adjustment, photocraft_doc::Adjustment::Unsupported { psd_key: "brit".into(), raw: brit.clone() }); + let mut doc = Document::with_background("Light", photocraft_geom::Size::new(8, 8), ColorMode::Rgb, SampleType::U8, photocraft_doc::Color::WHITE); + let mut layer = Layer::new("Light 1", LayerContent::Adjustment(adjustment)); + layer.psd_blocks = vec![(*b"brit", std::sync::Arc::new(brit.clone())), (*b"CgEd", std::sync::Arc::new(cged.clone()))]; + doc.layers.push(layer); + let block = |file: &PsdFile, key: &[u8; 4]| { + let records = &file.layer_info.as_ref().unwrap().layers; + records.iter().flat_map(|r| r.blocks.iter()).find(|b| b.key == *key).map(|b| b.data.clone()) + }; + let file = PsdFile::from_bytes(&document_to_psd(&doc).to_bytes().unwrap()).unwrap(); + let (restored, warnings) = crate::psd_import::psd_to_document(&file); + assert!(warnings.iter().any(|w| w.contains("\"Light 1\"") && w.contains("Light settings")), "{warnings:?}"); + // Saved again unchanged, both blocks are written back byte for byte. + let again = PsdFile::from_bytes(&document_to_psd(&restored).to_bytes().unwrap()).unwrap(); + assert_eq!(block(&again, b"brit").as_deref(), Some(&brit[..])); + assert_eq!(block(&again, b"CgEd").as_deref(), Some(&cged[..])); + // A plain Brightness/Contrast descriptor still reads as Brightness/Contrast. + let plain = crate::adjust_map::write(&photocraft_doc::Adjustment::BrightnessContrast { brightness: 5.0, contrast: -2.0, legacy: false }); + assert!(!crate::adjust_map::is_extended_brit(Some(&plain[1].1))); + } + #[test] fn size_limit_boundary_keeps_limit_and_switches_above_it() { assert!(!exceeds_psd_size_limit(Some(PSD_MAX_FILE_SIZE))); diff --git a/crates/io/src/psd_import.rs b/crates/io/src/psd_import.rs index bf4707bce7..8e26d99e81 100644 --- a/crates/io/src/psd_import.rs +++ b/crates/io/src/psd_import.rs @@ -377,10 +377,16 @@ impl Ctx<'_> { // Kept verbatim for saving, but it renders as nothing: say so rather than open the // layer silently without its effect (#1763). if matches!(adj, photocraft_doc::Adjustment::Unsupported { .. }) { - let why = adjust_map::unreadable_reason(k, &data).map(|r| format!(" ({r})")).unwrap_or_default(); + // Photoshop 2026's Light adjustment shares the Brightness/Contrast key (#6542). + let light = k == b"brit" && adjust_map::is_extended_brit(cged); + let why = if light { + " (Light adjustments, with Exposure, Highlights, Shadows, Whites and Blacks, aren't supported yet)".to_string() + } else { + adjust_map::unreadable_reason(k, &data).map(|r| format!(" ({r})")).unwrap_or_default() + }; self.warn(format!( "layer \"{name}\": its {} settings could not be read{why}; the layer is kept as saved but has no effect", - adjust_map::label(k) + if light { "Light" } else { adjust_map::label(k) } )); } LayerContent::Adjustment(adj) From afd5127dc85189bf51697260df859a47976c04c2 Mon Sep 17 00:00:00 2001 From: Omar Saidahmed Date: Fri, 23 Oct 2026 18:36:54 -0400 Subject: [PATCH 14/16] perf: skip redundant smart object resamples and per-pixel reads (#6406) * skip redundant smart object resamples and per-pixel reads * resample smart objects only when their re-render fails --------- Co-authored-by: Brandon Thomas --- crates/engine/src/image_cmds.rs | 4 + crates/engine/src/smart_cmds.rs | 27 +++++++ crates/engine/src/transform_cmds.rs | 6 +- .../engine/tests/smart_rerender_fallback.rs | 77 +++++++++++++++++++ crates/paint/src/replace.rs | 4 +- crates/raster/src/lib.rs | 3 +- 6 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 crates/engine/tests/smart_rerender_fallback.rs diff --git a/crates/engine/src/image_cmds.rs b/crates/engine/src/image_cmds.rs index 9259c5b2bd..6353f1f2cc 100644 --- a/crates/engine/src/image_cmds.rs +++ b/crates/engine/src/image_cmds.rs @@ -1,6 +1,8 @@ //! Image menu: Image Size, Canvas Size, Crop, Trim, Mode (colour model and //! bit depth) conversions, Duplicate. +use std::convert::Infallible; + use photocraft_algo::resample::{Resample, crop_surface, resize_surface_in_canvas, translate_surface}; use photocraft_color::{ColorMode, SampleType}; use photocraft_doc::{Document, Effect, Effects, FxPaint, Layer, LayerContent, LayerId, Size}; @@ -297,6 +299,7 @@ pub(crate) fn resample_doc(doc: &mut Document, nw: u32, nh: u32, filter: Resampl } // Content that reaches the canvas edge keeps it: a Background stays opaque to the border. let canvas = Rect::from_xywh(0, 0, ow, oh); + let taken = crate::smart_cmds::take_caches(&mut doc.layers); for_each_surface(&mut doc.layers, true, &mut |surf, is_mask| { *surf = resize_surface_in_canvas(surf, sx, sy, if is_mask { Resample::Bilinear } else { filter }, canvas); }); @@ -321,6 +324,7 @@ pub(crate) fn resample_doc(doc: &mut Document, nw: u32, nh: u32, filter: Resampl crate::canvas_geom::transform_geometry(doc, &photocraft_geom::Affine { m: [sx, 0.0, 0.0, sy, 0.0, 0.0] }); doc.size = Size::new(nw, nh); crate::canvas_geom::refresh(doc, crate::canvas_geom::Refresh::All); + let Ok(()) = crate::smart_cmds::restore_unrendered(doc, taken, |c| Ok::<_, Infallible>(resize_surface_in_canvas(c, sx, sy, filter, canvas))); } fn bad(cmd: &str, msg: impl Into) -> EngineError { diff --git a/crates/engine/src/smart_cmds.rs b/crates/engine/src/smart_cmds.rs index 556ad5c55a..24acea9ad9 100644 --- a/crates/engine/src/smart_cmds.rs +++ b/crates/engine/src/smart_cmds.rs @@ -186,6 +186,33 @@ pub fn source_decode_count(bytes: &[u8], fmt: PixelFormat) -> u32 { SOURCE_CACHE.lock().ok().and_then(|c| c.iter().find(|e| e.key == key).map(|e| e.decodes)).unwrap_or(0) } +pub(crate) fn take_caches(layers: &mut [Layer]) -> Vec<(LayerId, Surface)> { + let mut out = Vec::new(); + for l in layers { + match &mut l.content { + LayerContent::Smart(sm) => out.extend(sm.cache.take().map(|c| (l.id, c))), + LayerContent::Group(g) => out.extend(take_caches(&mut g.children)), + _ => {} + } + } + out +} + +pub(crate) fn restore_unrendered( + doc: &mut Document, + taken: Vec<(LayerId, Surface)>, + mut fallback: impl FnMut(&Surface) -> std::result::Result, +) -> std::result::Result<(), E> { + for (id, cache) in taken { + if let Some(LayerContent::Smart(sm)) = doc.layer_mut(id).map(|l| &mut l.content) + && sm.cache.is_none() + { + sm.cache = Some(fallback(&cache)?); + } + } + Ok(()) +} + fn buffer_to_surface(buf: &photocraft_compose::Buffer, fmt: PixelFormat) -> Surface { let n = fmt.channels(); let mut data = vec![0.0f32; buf.px.len() * n]; diff --git a/crates/engine/src/transform_cmds.rs b/crates/engine/src/transform_cmds.rs index a1b1aac379..1df4b1acee 100644 --- a/crates/engine/src/transform_cmds.rs +++ b/crates/engine/src/transform_cmds.rs @@ -838,13 +838,17 @@ fn transform_doc( // Photoshop turns the Background into a normal layer before transforming it. crate::extra_cmds::background_to_layer(doc, id); let l = doc.layer_mut(id).ok_or(EngineError::NoLayer(id))?; - transform_layer_with(if is_group { None } else { sel.as_ref() }, group, l, h, affine, interp, ctl)?; + let layer_sel = if is_group { None } else { sel.as_ref() }; + check_layer_budget(layer_sel, l, h)?; + let taken = if h.is_identity() { Vec::new() } else { crate::smart_cmds::take_caches(std::slice::from_mut(l)) }; + apply_transform(layer_sel, group, l, h, affine, interp, ctl)?; if !h.is_identity() { // Type layers re-render from their new transform. let snapshot = doc.clone(); if let Some(l) = doc.layer_mut(id) { refresh_text(&snapshot, l); } + crate::smart_cmds::restore_unrendered(doc, taken, |c| warp_surface(c, c.content_bounds(), h, interp))?; // The selection outline moves with the pixels. if let Some(sel) = &doc.selection { doc.selection = Some(warp_gray(sel, h, Interp::Bilinear)?).filter(|s| !s.content_bounds().is_empty()); diff --git a/crates/engine/tests/smart_rerender_fallback.rs b/crates/engine/tests/smart_rerender_fallback.rs new file mode 100644 index 0000000000..1359ec0aeb --- /dev/null +++ b/crates/engine/tests/smart_rerender_fallback.rs @@ -0,0 +1,77 @@ +use std::sync::Arc; + +use photocraft_doc::{LayerContent, SmartObject, SmartSource}; +use photocraft_engine::{EngineError, Session}; +use serde_json::{Value, json}; + +fn deny_blur(id: &str, _: &Value) -> photocraft_engine::Result<()> { + if id == "filter.blur.gaussianBlur" { Err(EngineError::Other("denied".into())) } else { Ok(()) } +} + +fn edits() -> [(&'static str, Value); 2] { + [("edit.transform", json!({"scaleX": 50, "scaleY": 100})), ("image.imageSize", json!({"width": 100, "height": 50}))] +} + +fn blurred_smart_object() -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 200, "height": 100})).unwrap(); + s.execute("layer.new.layer", json!({})).unwrap(); + s.execute("select.rect", json!({"x": 20, "y": 20, "width": 100, "height": 50})).unwrap(); + s.execute("edit.fill", json!({"contents": "color", "color": "#ff0000"})).unwrap(); + s.execute("select.deselect", json!({})).unwrap(); + s.execute("layer.smartObjects.convertToSmartObject", json!({})).unwrap(); + s.execute("filter.blur.gaussianBlur", json!({"radius": 2})).unwrap(); + s +} + +fn cache_width(s: &Session) -> i32 { + let st = s.active().unwrap(); + let LayerContent::Smart(sm) = &st.doc.layer(st.active_layer.unwrap()).unwrap().content else { panic!("not a smart object") }; + sm.cache.as_ref().unwrap().content_bounds().width() as i32 +} + +fn edit_smart_object(s: &mut Session, edit: impl FnOnce(&mut SmartObject)) { + let st = s.active_mut().unwrap(); + let id = st.active_layer.unwrap(); + let LayerContent::Smart(sm) = &mut Arc::make_mut(&mut st.doc).layer_mut(id).unwrap().content else { panic!("not a smart object") }; + edit(sm); +} + +#[test] +fn a_smart_object_whose_filters_are_denied_keeps_the_resampled_fallback() { + for (cmd, params) in edits() { + let mut s = blurred_smart_object(); + let before = cache_width(&s); + s.authorize = Some(deny_blur); + s.execute(cmd, params).unwrap(); + let after = cache_width(&s); + assert!(after < before * 3 / 4, "{cmd}: the fallback stayed at {after} px wide (was {before})"); + } +} + +#[test] +fn a_smart_object_that_fails_to_re_render_keeps_the_resampled_fallback() { + let unrenderable = + [SmartSource::Linked { path: "missing.psd".into() }, SmartSource::Embedded { file_name: "broken.png".into(), bytes: Arc::new(vec![0; 16]) }]; + for (cmd, params) in edits() { + for source in &unrenderable { + let mut s = blurred_smart_object(); + let before = cache_width(&s); + edit_smart_object(&mut s, |sm| sm.source = source.clone()); + s.execute(cmd, params.clone()).unwrap(); + let after = cache_width(&s); + assert!(after < before * 3 / 4, "{cmd} with {source:?}: the fallback stayed at {after} px wide (was {before})"); + } + } +} + +#[test] +fn an_allowed_smart_object_still_re_renders_at_the_new_size() { + for (cmd, params) in edits() { + let mut s = blurred_smart_object(); + let before = cache_width(&s); + s.execute(cmd, params).unwrap(); + let after = cache_width(&s); + assert!(after < before * 3 / 4, "{cmd}: {after} px wide (was {before})"); + } +} diff --git a/crates/paint/src/replace.rs b/crates/paint/src/replace.rs index 3cb02e9a24..20619cd48a 100644 --- a/crates/paint/src/replace.rs +++ b/crates/paint/src/replace.rs @@ -127,6 +127,8 @@ pub fn color_replacement_effect(pre: Surface, rs: ReplaceSettings) -> impl FnMut return; } let (w, h) = (r.width() as usize, r.height() as usize); + let mut pre_px = vec![[0.0f32; 4]; w * h]; + pre.read_rgba_into(r, &mut pre_px); // Per-pixel match weight. let mut weight = vec![0.0f32; w * h]; for y in r.y0..r.y1 { @@ -136,7 +138,7 @@ pub fn color_replacement_effect(pre: Surface, rs: ReplaceSettings) -> impl FnMut } // Match against the pre-stroke colours, so pixels recoloured by earlier dabs of // this stroke still count as the sampled colour. - let c = pre.rgba(x, y); + let c = pre_px[(y - r.y0) as usize * w + (x - r.x0) as usize]; if c[3] <= 0.0 { continue; } diff --git a/crates/raster/src/lib.rs b/crates/raster/src/lib.rs index 6ce0ea688d..fb6a6fd388 100644 --- a/crates/raster/src/lib.rs +++ b/crates/raster/src/lib.rs @@ -1116,7 +1116,8 @@ impl Surface { /// Composite-friendly RGBA read (converts from the surface's model). pub fn rgba(&self, x: i32, y: i32) -> [f32; 4] { - let p = self.pixel(x, y); + let mut p = [0.0f32; 5]; + self.read_pixel(x, y, &mut p); to_rgba(&self.format, &p) } } From ed0286d324e0b959eba041d0621cbada3e7f4578 Mon Sep 17 00:00:00 2001 From: Max Witt Date: Sat, 24 Oct 2026 00:37:08 +0200 Subject: [PATCH 15/16] New Document: category tabs and preset cards show a pointer, not a text cursor (#3647) Co-authored-by: Brandon Thomas --- crates/ui-egui/src/new_doc_ui.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ui-egui/src/new_doc_ui.rs b/crates/ui-egui/src/new_doc_ui.rs index 563a9a5fca..0fb7b7b21f 100644 --- a/crates/ui-egui/src/new_doc_ui.rs +++ b/crates/ui-egui/src/new_doc_ui.rs @@ -583,6 +583,7 @@ pub fn body(app: &mut crate::PhotocraftApp, ui: &mut egui::Ui, f: &mut Map chosen == p.0, From 28f3c18c0d11d69005d224a0a5d5edc56f1fe858 Mon Sep 17 00:00:00 2001 From: imSlicedBread <56609001+imSlicedBread@users.noreply.github.com> Date: Fri, 23 Oct 2026 15:37:21 -0700 Subject: [PATCH 16/16] Make magnetic lasso edge direction polarity-independent (#3415) * Fix magnetic lasso gradient polarity cost * Add magnetic lasso gradient polarity regressions * style: cargo fmt Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/algo/src/magnetic.rs | 11 +++++------ crates/algo/src/magnetic/tests.rs | 33 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/crates/algo/src/magnetic.rs b/crates/algo/src/magnetic.rs index f2dab08816..54759237cb 100644 --- a/crates/algo/src/magnetic.rs +++ b/crates/algo/src/magnetic.rs @@ -512,12 +512,11 @@ fn direction_cost(tp: [f32; 2], tq: [f32; 2], dx: i32, dy: i32, len: f32) -> f32 if tp == [0.0; 2] || tq == [0.0; 2] { return NEUTRAL_DIRECTION; } - let mut l = [dx as f32 / len, dy as f32 / len]; - if tp[0] * l[0] + tp[1] * l[1] < 0.0 { - l = [-l[0], -l[1]]; - } - let dp = (tp[0] * l[0] + tp[1] * l[1]).clamp(-1.0, 1.0); - let dq = (l[0] * tq[0] + l[1] * tq[1]).clamp(-1.0, 1.0); + // An edge tangent is an axis, not a directed vector: reversing the strongest channel's + // gradient must not make an otherwise straight edge expensive to follow. + let l = [dx as f32 / len, dy as f32 / len]; + let dp = (tp[0] * l[0] + tp[1] * l[1]).abs().clamp(0.0, 1.0); + let dq = (l[0] * tq[0] + l[1] * tq[1]).abs().clamp(0.0, 1.0); 2.0 / (3.0 * PI) * (dp.acos() + dq.acos()) } diff --git a/crates/algo/src/magnetic/tests.rs b/crates/algo/src/magnetic/tests.rs index ccfc73bc04..ee187bc781 100644 --- a/crates/algo/src/magnetic/tests.rs +++ b/crates/algo/src/magnetic/tests.rs @@ -55,6 +55,39 @@ fn snaps_to_the_most_prominent_edge() { assert!((p[0] - 18.0).abs() < 0.01, "{p:?}"); } +#[test] +fn gradient_polarity_does_not_make_a_straight_edge_look_like_a_turn() { + // A gradient can reverse across adjacent pixels when another colour channel becomes the + // strongest one. The edge tangent is an axis, so its sign must not change the path cost. + let forward = direction_cost([1.0, 0.0], [1.0, 0.0], 1, 0, 1.0); + let reversed = direction_cost([1.0, 0.0], [-1.0, 0.0], 1, 0, 1.0); + assert!(forward < 1e-6, "aligned edge: {forward}"); + assert!(reversed < 1e-6, "same edge with reversed gradient polarity: {reversed}"); +} + +#[test] +fn traces_a_straight_edge_through_a_gradient_polarity_change() { + let read = Cell::new(0); + // The contrast stays high, but the edge changes from dark-to-light to light-to-dark halfway + // down. The horizontal seam intersects it; the guide stays on the vertical contour. + let mut f = image( + |x, y| { + let left = x < 32; + let top = y < 32; + let white = left != top; + grey(if white { 255 } else { 0 }) + }, + &read, + ); + let mut t = Tracer::new(Rect::new(0, 0, 64, 64)); + let path = t.trace(&mut f, [32.0, 5.0], [32.0, 59.0], &[[32.0, 32.0]], Settings::default()); + assert_eq!(path.first(), Some(&[32.0, 5.0])); + assert_eq!(path.last(), Some(&[32.0, 59.0])); + for p in &path { + assert!((p[0] - 32.0).abs() < 1.0, "left the high-contrast edge: {p:?}; {path:?}"); + } +} + #[test] fn trace_follows_a_curved_edge() { let read = Cell::new(0);