From 2d30a2b669ae008915d02633bbcf0fa16ba09568 Mon Sep 17 00:00:00 2001 From: ljkx <177671075+ljkx@users.noreply.github.com> Date: Mon, 2 Nov 2026 09:37:01 +0100 Subject: [PATCH] Gradient stops: the release frame decides drag-off; review fixes (#10742) The Gradient Editor/Properties strip and Layer Style's Gradient Overlay ramp ignored the release frame. When the last pointer move came in the same frame as the release, the stop stayed in the state the previous frame left it in. A stop flicked off the bar was kept, and one brought back onto the bar at release was deleted. Both now apply the same drag-off rule on that frame, as Gradient Map and the shape strip already did. The editor strip also counted opacity stops from its preview, which already leaves a dragged-off stop out. So while an opacity stop was off the bar, the preview flipped between deleting and moving it every frame. It now counts the committed stops, like colour stops. Tests: a quick release off the bar and back onto it (both editors); colour stops dragged up and opacity stops dragged down are kept; a cancelled Gradient Map drag adds no history step. The parity row names Gradient Map's control "Remove stop". Co-Authored-By: Claude Opus 5.5 --- .../src/dialogs/adjust_editors/ui_tests.rs | 2 + .../src/dialogs/layer_style/gradient_field.rs | 44 +++++++++++++-- crates/ui-egui/src/tools/gradient_ui.rs | 10 ++-- .../src/tools/gradient_ui/editor_tests.rs | 54 ++++++++++++++++++- docs/ui-parity.md | 2 +- 5 files changed, 102 insertions(+), 10 deletions(-) diff --git a/crates/ui-egui/src/dialogs/adjust_editors/ui_tests.rs b/crates/ui-egui/src/dialogs/adjust_editors/ui_tests.rs index bb0f737265..625039a692 100644 --- a/crates/ui-egui/src/dialogs/adjust_editors/ui_tests.rs +++ b/crates/ui-egui/src/dialogs/adjust_editors/ui_tests.rs @@ -570,6 +570,7 @@ fn gradient_map_stop_dragged_down_off_the_bar_is_removed_and_dragging_back_cance h.state_mut().run("layer.setAdjustment", json!({"layer": id.0, "stops": [[0, "#000000"], [0.5, "#ff0000"], [1, "#ffffff"]]})).unwrap(); h.run_steps(3); let three = layer(&h).1; + let before_cancel = h.state().session.active().unwrap().history.past_len(); let live_stops = |h: &Harness<'_, PhotocraftApp>| h.state().live_adjust.as_ref().and_then(|(_, v)| v["stops"].as_array().map(Vec::len)); // Off and back: nothing removed. let start = map_handle(&h, 0.5); @@ -589,6 +590,7 @@ fn gradient_map_stop_dragged_down_off_the_bar_is_removed_and_dragging_back_cance h.drop_at(start); h.run_steps(3); assert_eq!(map_stops(&layer(&h).1), map_stops(&three), "dragging back cancels the removal"); + assert_eq!(h.state().session.active().unwrap().history.past_len(), before_cancel, "a cancelled drag adds no history step"); // Off and released: removed in one step. let steps = h.state().session.active().unwrap().history.past_len(); drag(&mut h, start, away); diff --git a/crates/ui-egui/src/dialogs/layer_style/gradient_field.rs b/crates/ui-egui/src/dialogs/layer_style/gradient_field.rs index c59b666b3f..e5e253caab 100644 --- a/crates/ui-egui/src/dialogs/layer_style/gradient_field.rs +++ b/crates/ui-egui/src/dialogs/layer_style/gradient_field.rs @@ -235,10 +235,9 @@ pub(super) fn row(app: &mut PhotocraftApp, ui: &mut egui::Ui, f: &mut Map(drag_key)); - } else if resp.dragged() { + // The release frame applies the same rule (its last move may cross the edge), then ends + // the drag. + if resp.dragged() || resp.drag_stopped() { match drag.removed { None if off && stops.len() > 2 => { if let Some(stop) = stops.get(drag.index).copied() @@ -271,7 +270,11 @@ pub(super) fn row(app: &mut PhotocraftApp, ui: &mut egui::Ui, f: &mut Map(drag_key)); + } else { + ui.data_mut(|d| d.insert_temp(drag_key, drag)); + } } } else if resp.drag_stopped() { ui.data_mut(|d| d.remove::(drag_key)); @@ -455,6 +458,37 @@ mod tests { assert_eq!(stops.iter().map(|s| super::super::hex(&s.1)).collect::>(), ["#000000", "#ffffff"], "{stops:?}"); } + /// The release frame counts: a stop that leaves the strip in the frame it is released is + /// removed, and one brought back onto the strip in that frame is kept. + #[test] + fn the_release_frame_decides_whether_a_stop_comes_off_the_ramp() { + for back in [true, false] { + let (mut app, parent) = dialog_app(); + let effect = crate::layer_style::effects_of(&app.ui.dialogs[0].fields)[0]["id"].as_str().unwrap().to_string(); + set_param(&mut app.ui.dialog_mut(parent).unwrap().fields, &effect, KEY, json!([[0.0, "#000000"], [0.5, "#ff0000"], [1.0, "#ffffff"]])); + let mut h = harness(app, parent); + let (_, bar): (String, Rect) = h.ctx.data(|d| d.get_temp(bar_id())).expect("the ramp was drawn"); + let mid = pos2(bar.center().x, bar.bottom() + 9.0); + let away = pos2(mid.x, mid.y + 30.0); + press(&mut h, mid); + h.event(egui::Event::PointerMoved(pos2(mid.x, mid.y + 11.0))); + h.run_steps(1); + if back { + h.event(egui::Event::PointerMoved(away)); + h.run_steps(1); + assert_eq!(stops_in(h.state(), parent).len(), 2, "off the strip the stop is gone"); + } + let to = if back { mid } else { away }; + // The last move and the release in one frame (`Harness::event` gives each its own). + let input = &mut h.input_mut().events; + input.push(egui::Event::PointerMoved(to)); + input.push(egui::Event::PointerButton { pos: to, button: egui::PointerButton::Primary, pressed: false, modifiers: Default::default() }); + h.run_steps(2); + let stops = stops_in(h.state(), parent); + assert_eq!(stops.len(), if back { 3 } else { 2 }, "back {back}: {stops:?}"); + } + } + /// A preset from the Gradients panel becomes the page's stops; the Delete button and the /// Location field work on the selected stop. #[test] diff --git a/crates/ui-egui/src/tools/gradient_ui.rs b/crates/ui-egui/src/tools/gradient_ui.rs index 12d2faeedd..daa9a9344f 100644 --- a/crates/ui-egui/src/tools/gradient_ui.rs +++ b/crates/ui-egui/src/tools/gradient_ui.rs @@ -903,7 +903,10 @@ fn selection_after_edit(f: &Fill, p: &Value, previous: Option) -> Option } fn stop_strip(ui: &mut egui::Ui, key: egui::Id, f: &Fill, midpoints: bool, pending: Option<&Value>, colors: ([f32; 4], [f32; 4])) -> Option { - let Fill::Gradient { stops, .. } = f else { return None }; + // The stops as committed: a drag edit (and the two-stop minimum) addresses these, not the + // pending preview, which may already leave the dragged stop out. + let Fill::Gradient { stops, opacity_stops, .. } = f else { return None }; + let opacity_count = if opacity_stops.is_empty() { 2 } else { opacity_stops.len() }; let t = Tokens::get(ui.ctx()); let w = ui.available_width().max(120.0); let (outer, resp) = ui.allocate_exact_size(vec2(w, 58.0), Sense::click_and_drag()); @@ -1015,14 +1018,15 @@ fn stop_strip(ui: &mut egui::Ui, key: egui::Id, f: &Fill, midpoints: bool, pendi let edit = match m { Marker::Color(i) if off && stops.len() > 2 => json!({"action": "delete", "index": i}), Marker::Color(i) => json!({"action": "move", "index": i, "location": loc}), - Marker::Opacity(i) if off && ops.len() > 2 => json!({"action": "delete", "kind": "opacity", "index": i}), + Marker::Opacity(i) if off && opacity_count > 2 => json!({"action": "delete", "kind": "opacity", "index": i}), Marker::Opacity(i) => json!({"action": "move", "kind": "opacity", "index": i, "location": loc}), Marker::Mid(i) => { let (a, b) = (sorted.get(i).map_or(0.0, |s| s.0), sorted.get(i + 1).map_or(1.0, |s| s.0)); json!({"action": "midpoint", "index": i, "location": ((loc - a) / (b - a).max(1e-4)).clamp(0.05, 0.95)}) } }; - if resp.dragged() { + // The release frame counts too: its own last move decides whether the stop comes off. + if resp.dragged() || resp.drag_stopped() { preview = Some(edit); } } diff --git a/crates/ui-egui/src/tools/gradient_ui/editor_tests.rs b/crates/ui-egui/src/tools/gradient_ui/editor_tests.rs index c761838f36..34636bd376 100644 --- a/crates/ui-egui/src/tools/gradient_ui/editor_tests.rs +++ b/crates/ui-egui/src/tools/gradient_ui/editor_tests.rs @@ -260,7 +260,7 @@ fn the_last_two_color_stops_cannot_be_dragged_off() { } } -/// An opacity stop dragged up off the bar is removed; dragging it down over the ramp is not. +/// An opacity stop dragged up off the bar is removed while two remain. #[test] fn an_opacity_stop_dragged_up_off_the_bar_is_removed() { for layer in [true, false] { @@ -277,3 +277,55 @@ fn an_opacity_stop_dragged_up_off_the_bar_is_removed() { assert_eq!(sink.fill(h.state()).unwrap(), before, "layer {layer}: two opacity stops always remain"); } } + +/// Only the stop's own direction takes it off: an opacity stop dragged down over the ramp and a +/// colour stop dragged up over it are kept, however far they go. +#[test] +fn a_stop_dragged_the_other_way_is_kept() { + for layer in [true, false] { + let (mut h, sink) = editor(layer); + for opacity in [true, false] { + let from = marker(&h, 0.5, opacity); + let to = from + vec2(0.0, if opacity { 70.0 } else { -70.0 }); + drag_through(&mut h, &[from, to], |h| assert_eq!(shown(h, sink), (3, 3), "layer {layer} opacity {opacity}: still shown")); + assert_eq!(counts(&h, sink), (3, 3), "layer {layer} opacity {opacity}: the stop is kept"); + } + } +} + +/// Press on a stop, drag a little along the bar, then move to each of `then` in its own frame; +/// the last move and the release arrive in the same frame, as a quick flick does. +fn flick(h: &mut Harness, from: Pos2, then: &[Pos2]) { + h.event(egui::Event::PointerMoved(from)); + h.run_steps(1); + h.event(egui::Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed: true, modifiers: egui::Modifiers::NONE }); + h.run_steps(1); + h.event(egui::Event::PointerMoved(from + vec2(10.0, 0.0))); + h.run_steps(1); + let Some((&last, moves)) = then.split_last() else { return }; + for &p in moves { + h.event(egui::Event::PointerMoved(p)); + h.run_steps(1); + } + // `Harness::event` runs one frame per event, so both go straight into the next frame. + let input = &mut h.input_mut().events; + input.push(egui::Event::PointerMoved(last)); + input.push(egui::Event::PointerButton { pos: last, button: egui::PointerButton::Primary, pressed: false, modifiers: egui::Modifiers::NONE }); + h.run_steps(3); +} + +/// The release frame counts: a stop flicked off the bar and released in the same frame is +/// removed, and one brought back onto the bar in the release frame is kept. +#[test] +fn the_release_frame_decides_whether_the_stop_comes_off() { + for layer in [true, false] { + let (mut h, sink) = editor(layer); + let from = marker(&h, 0.5, false); + let away = from + vec2(10.0, 40.0); + flick(&mut h, from, &[away, from]); + assert_eq!(counts(&h, sink), (3, 3), "layer {layer}: back on the bar at release, the stop is kept"); + let from = marker(&h, 0.5, false); + flick(&mut h, from, &[from + vec2(10.0, 40.0)]); + assert_eq!(counts(&h, sink), (2, 3), "layer {layer}: released off the bar in the same frame, the stop is removed"); + } +} diff --git a/docs/ui-parity.md b/docs/ui-parity.md index 8bc4974646..abf9431ef4 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -240,7 +240,7 @@ Paragraph; the rest open as in-app floating windows. | Color panel modes from its panel menu (Hue Cube, Brightness Cube, Color Wheel, Grayscale/RGB/HSB/CMYK/Lab/Web Color sliders), colour ramp (RGB/CMYK Spectrum, Grayscale Ramp, Current Colors, Make Ramp Web Safe; Shift-click cycles, Alt-click picks the other colour), Copy Color as HTML / Hex Code; the out-of-gamut ⚠ with the nearest printable colour under the chips (sliders always; the cubes when the field is at least 53 pt tall); the same in every theme | done | `color_panel_ui.rs`, `color_panel_tests.rs`; UI-217-17, #2569, #6000, #8672 | | A colour picked in the Color or Swatches panel or the Color Picker recolours the selection: selected characters while editing; every selected type layer with a Type tool; every selected shape's fill with a shape, Pen or path selection tool; only the foreground with other tools, the Move tool too; a swatch click recolours the selected type layers and shape fills whatever the tool (#9238); one history step (a Color panel drag too) | done: `apply_color.rs`, `apply_color_tests.rs` | | | The Type tool's Color Picker (Text Color) and a gradient fill's Color Picker (Stop Color) preview the picked colour on the canvas before OK (the selected characters, every selected type layer, or the stop); Cancel restores the document with no history step, OK commits the previewed edit in one step (#5530) | done: `dialogs/color_picker_preview.rs`, `dialogs/color_picker_preview/tests.rs` | | -| Gradient stops come off the bar: a colour stop dragged down, or an opacity stop dragged up, by about a marker's height is removed on release while two of its kind remain; while it is off the ramp and canvas preview show it gone, and dragging it back onto the bar cancels; the selected stop's Delete button does the same. Gradient Editor, Properties, the shape gradient popover, Gradient Map and Layer Style (Gradient Overlay) share one rule (#10742) | done: `tools/gradient_ui.rs` `stop_dragged_off`, `tools/gradient_ui/editor_tests.rs`, `adjust_editors/ui_tests.rs`, `layer_style/gradient_field.rs`. Differs: Gradient Map and Layer Style show colour stops only; no Delete-key shortcut for the selected stop | | +| Gradient stops come off the bar: a colour stop dragged down, or an opacity stop dragged up, by about a marker's height is removed on release while two of its kind remain; while it is off the ramp and canvas preview show it gone, and dragging it back onto the bar cancels; the selected stop's Delete button (Remove stop, the × beside the colour, in Gradient Map) does the same. Gradient Editor, Properties, the shape gradient popover, Gradient Map and Layer Style (Gradient Overlay) share one rule (#10742) | done: `tools/gradient_ui.rs` `stop_dragged_off`, `tools/gradient_ui/editor_tests.rs`, `adjust_editors/ui_tests.rs`, `layer_style/gradient_field.rs`. Differs: Gradient Map and Layer Style show colour stops only; no Delete-key shortcut for the selected stop | | | Window size and dock width remembered | done | UI-217-2 | | Histogram panel: Compact (the default, of the entire image), Expanded and All Channels views (the channel kept across them); RGB/CMYK/Lab/Gray channels, Luminosity and Colors (overlaid, mixed where they overlap); Show Channels in Color and Show Statistics; Source (Entire Image, Selected Layer, Adjustment Composite) for multilayer documents; Mean, Std Dev, Median, Pixels, Level, Count, Percentile and Cache Level for the pointer level or a dragged range; the cached-data warning, Uncached Refresh (menu, button, warning click, double-click); the adjusted histogram over the original while an adjustment previews; Cache Levels 1–8 in Preferences › Performance; Color Sampler levels marked on the graph | partial | `histogram_panel.rs`, `histogram_panel_tests.rs`, `tone.rs`; differs: filter dialogs that preview on the CPU proxy aren't counted live, a selection doesn't limit the histogram yet, with Source Selected Layer an adjustment dialog's preview isn't counted, and new strings are English-only so far | | Command palette with recents | done | UI-217-7 |