From 84a0693076a9cd6cf94691e8af324f8036a16ebc Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Tue, 3 Nov 2026 00:45:53 -0500 Subject: [PATCH] Fill section: the canvas crop and gradient handles follow the characters' shared fill The handles read the paint list the Fill section and the colour picker show (character_fills), so with the characters' image already in Crop mode the crop handles show and edit it, not the layer's own fill at that index; writes still go through fill.set, which moves the characters' list to the layer first. No undo step on entering crop mode. Test canvas::handles::character_fill_tests. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas/handles.rs | 22 +++++++--- .../canvas/handles/character_fill_tests.rs | 40 +++++++++++++++++++ docs/ui-parity.md | 2 +- 3 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 crates/ui-egui/src/canvas/handles/character_fill_tests.rs diff --git a/crates/ui-egui/src/canvas/handles.rs b/crates/ui-egui/src/canvas/handles.rs index 137e2c92b..b9711dfa8 100644 --- a/crates/ui-egui/src/canvas/handles.rs +++ b/crates/ui-egui/src/canvas/handles.rs @@ -40,6 +40,15 @@ enum Drag { /// The Crop-mode image being edited: (node, fills, index, paint, image rect in the unit box). type UnitRect = (f64, f64, f64, f64); +/// The paint list a picker paint at an index belongs to on layer `n`, as the Fill section and the +/// colour picker list it: for fills, a text's characters' shared fills (`character_fills`: the +/// layer's own when the characters have none), else the layer's strokes. None when a text's +/// characters have different fills. Writes go through `fill.set`, which moves the characters' +/// list to the layer first, so the handles read and write the same paint (#1856). +fn edited_list(n: &figcraft_doc::Node, fills: bool) -> Option> { + if fills { figcraft_engine::cmd::lists::character_fills(n) } else { Some(n.strokes.clone()) } +} + /// The paint open in the colour picker, else the image fill in crop mode (while its layer is the /// selection). fn crop_target(app: &FigApp) -> Option<(NodeId, bool, usize, ImagePaint, UnitRect)> { @@ -53,7 +62,7 @@ fn crop_target(app: &FigApp) -> Option<(NodeId, bool, usize, ImagePaint, UnitRec }; let doc = app.session.doc().ok()?; let n = doc.node(id)?; - let list = if fills { &n.fills } else { &n.strokes }; + let list = edited_list(n, fills)?; let PaintKind::Image(img) = &list.get(index)?.kind else { return None }; if img.scale_mode != ImageScaleMode::Crop { return None; @@ -193,7 +202,7 @@ fn gradient_target(app: &FigApp) -> Option<(NodeId, bool, usize, figcraft_color: return None; } let n = app.session.doc().ok()?.node(id)?; - let list = if *fills { &n.fills } else { &n.strokes }; + let list = edited_list(n, *fills)?; match &list.get(*index)?.kind { PaintKind::Gradient(g) => Some((id, *fills, *index, g.clone())), _ => None, @@ -233,7 +242,7 @@ fn line_param(a: Pos2, b: Pos2, p: Pos2) -> Option<(f64, f32)> { fn set_gradient(app: &mut FigApp, id: NodeId, fills: bool, index: usize, g: figcraft_color::Gradient) { let Some(n) = app.session.doc().ok().and_then(|d| d.node(id)).cloned() else { return }; - let list = if fills { &n.fills } else { &n.strokes }; + let Some(list) = edited_list(&n, fills) else { return }; let Some(mut paint) = list.get(index).cloned() else { return }; paint.kind = PaintKind::Gradient(g); let cmd = if fills { "fill.set" } else { "stroke.set" }; @@ -652,7 +661,7 @@ fn apply(app: &mut FigApp, drag: &Drag, pos: Pos2, shift: bool) { Drag::Gradient { id, fills, index, which } => { let Some(u) = screen_to_unit(app, *id, pos) else { return }; let Some(n) = app.session.doc().ok().and_then(|d| d.node(*id)).cloned() else { return }; - let list = if *fills { &n.fills } else { &n.strokes }; + let Some(list) = edited_list(&n, *fills) else { return }; let Some(mut paint) = list.get(*index).cloned() else { return }; if let PaintKind::Gradient(g) = &mut paint.kind { match which { @@ -727,7 +736,7 @@ fn apply(app: &mut FigApp, drag: &Drag, pos: Pos2, shift: bool) { (ax.min(ax + nw), ay.min(ay + nh), ax.max(ax + nw), ay.max(ay + nh)) }; let Some(n) = app.session.doc().ok().and_then(|d| d.node(*id)).cloned() else { return }; - let list = if *fills { &n.fills } else { &n.strokes }; + let Some(list) = edited_list(&n, *fills) else { return }; let Some(mut paint) = list.get(*index).cloned() else { return }; if let PaintKind::Image(img) = &mut paint.kind { img.crop = Some(ImagePaint::crop_from_rect(r.0, r.1, r.2, r.3)); @@ -749,3 +758,6 @@ fn apply(app: &mut FigApp, drag: &Drag, pos: Pos2, shift: bool) { } } } + +#[cfg(test)] +mod character_fill_tests; diff --git a/crates/ui-egui/src/canvas/handles/character_fill_tests.rs b/crates/ui-egui/src/canvas/handles/character_fill_tests.rs new file mode 100644 index 000000000..59488cd3b --- /dev/null +++ b/crates/ui-egui/src/canvas/handles/character_fill_tests.rs @@ -0,0 +1,40 @@ +//! The canvas crop handles on a text whose characters share an image fill the layer doesn't have +//! (#1856): they show and edit the characters' image, the one the Fill section and the colour +//! picker show, not the layer's own fill at that index. + +use base64::Engine as _; +use figcraft_color::{Color, ImageScaleMode, Paint, PaintKind}; +use figcraft_engine::Session; +use serde_json::json; + +use super::crop_target; +use crate::colorpicker::PickerTarget; +use crate::layers_drag_tests::Ui; +use crate::present_tests::id; + +#[test] +fn the_crop_handles_follow_the_characters_shared_image_in_crop_mode() { + let mut s = Session::new(); + let t = id(&s.execute("text.create", &json!({"text": "Hello", "x": 0, "y": 0})).unwrap()); + s.execute("fill.set", &json!({"ids": [t.0], "color": "#00FF00"})).unwrap(); + let image = image::RgbaImage::from_pixel(40, 20, image::Rgba([200, 30, 30, 255])); + let mut png = std::io::Cursor::new(Vec::new()); + image.write_to(&mut png, image::ImageFormat::Png).unwrap(); + let data = base64::engine::general_purpose::STANDARD.encode(png.into_inner()); + s.execute("image.place", &json!({"data": data, "target": t.0, "mode": "fill"})).unwrap(); + s.execute("image.crop", &json!({"ids": [t.0], "index": 1})).unwrap(); + // The cropped image goes to the characters; the layer keeps only its green. + let cropped = s.doc().unwrap().node(t).unwrap().fills[1].clone(); + s.execute("text.style", &json!({"id": t.0, "start": 0, "end": 5, "textFills": [cropped]})).unwrap(); + s.doc_mut().unwrap().get_mut(t).unwrap().fills = vec![Paint::solid(Color::from_hex("#00FF00").unwrap())]; + let mut u = Ui::new(s); + // The picker on the Fill section's row: the characters' image, already in Crop mode. + u.app.ui.picker = Some(PickerTarget::Paint { fills: true, index: 0, ids: vec![t] }); + let (at, fills, index, img, _) = crop_target(&u.app).expect("the handles show the characters' image"); + assert_eq!((at, fills, index), (t, true, 0)); + assert_eq!(img.scale_mode, ImageScaleMode::Crop); + assert!( + matches!(u.app.session.doc().unwrap().node(t).unwrap().fills[0].kind, PaintKind::Solid { .. }), + "the layer's own fill is not what they show" + ); +} diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 0bdb62441..0bc09a912 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -33,7 +33,7 @@ interaction feel not yet compared. | Colour picker placement and dismissal | done | Opens 8 pt beside the sidebar at the opening control, lifted to fit the screen; outside presses (including the sidebar), Escape and X close it. Reopening resets a dragged position; eyedropper sampling keeps it open. | | Colour picker image well | partial | the Image tab shows a checkered placeholder and Upload from computer until there is a file (no dialog at once); an image paint shows its asset preview; a file dropped on the open picker replaces the image, keeping the fill mode, crop, position and adjustments (`image.replace`). Layers' fills and strokes only (text ranges, styles and selection colours keep Upload). The Fill and Stroke swatches take no drop: a dragged file's position isn't known, so a drop with none goes to the open picker | | Colour picker on a new fill or stroke | done | + opens the picker on the new paint when the selection shares one paint list; on a mixed selection + adds a paint to each layer and no picker opens (#404) | -| Fill section on a text with coloured characters | done | a text's Fill section shows what its characters show: characters coloured differently while editing are mixed content ("Click + to replace mixed content", as a mixed selection; the developer documentation's `fills`: "Text nodes can have multiple sets of fills if some characters are colored differently than others"), and characters that all share fills the layer doesn't have show those. Any layer-level fill edit (a row, its picker, +, reordering, `fill.*`, `node.set`) reaches every character, starting from the list they showed; the characters' own fills go in the same undo step (`text_mixed_fill_tests`, engine `text_fill_tests`; #1856). A range selected while editing keeps its own Fill row | +| Fill section on a text with coloured characters | done | a text's Fill section shows what its characters show: characters coloured differently while editing are mixed content ("Click + to replace mixed content", as a mixed selection; the developer documentation's `fills`: "Text nodes can have multiple sets of fills if some characters are colored differently than others"), and characters that all share fills the layer doesn't have show those. Any layer-level fill edit (a row, its picker, +, reordering, `fill.*`, `node.set`) reaches every character, starting from the list they showed; the characters' own fills go in the same undo step; the canvas gradient and crop handles show and edit the paint the row shows, the characters' own, also when their image is already in Crop mode (`text_mixed_fill_tests`, `canvas::handles::character_fill_tests`, engine `text_fill_tests`; #1856). A range selected while editing keeps its own Fill row | | Blend mode menus: the layer's (Design panel) and each paint's (colour picker) | done | Pass through and Normal are distinct (Normal isolates a group's or frame's children); paints have no pass-through; plus darker drawn exactly on the canvas | ## Layers and Assets panels