Merge pull request #2454 from marcoarnulfo/fix/recolor-wheel-handles

Recolor Artwork: dragging a wheel handle of CMYK art keeps the linked handles in place

crates/ui-egui/src/dialogs/recolor.rs

@@ -537,6 +537,9 @@ fn move_color(rows: &mut [Value], i: usize, hsb: [f32; 3], linked: bool) {
     }
 }
 
+/// The rows when a drag on the Edit tab's wheel started.
+const DRAG: &str = "__drag";
+
 /// The Edit tab. Returns true when the rows changed.
 fn edit_tab(ui: &mut Ui, d: &mut Dialog, rows: &mut [Value]) -> bool {
     let t = Tokens::get(ui.ctx());
@@ -569,9 +572,25 @@ fn edit_tab(ui: &mut Ui, d: &mut Dialog, rows: &mut [Value]) -> bool {
     if let Some(k) = w.pressed {
         d.fields.insert("__sel".into(), json!([active[k]]));
     }
-    if let Some((k, hsb)) = w.moved {
-        move_color(rows, active[k], hsb, linked);
-        changed = true;
+    // A drag (of a marker or the brightness slider) moves the rows as they were when it started:
+    // a new colour read back from its model (CMYK through the colour profile) isn't exactly the
+    // colour written, so building on last frame's rows drifted the linked colours (#1733).
+    if let Some((k, set)) = w.moved {
+        let start = match d.fields.get(DRAG).and_then(Value::as_array) {
+            Some(s) if s.len() == rows.len() => s.clone(),
+            _ => rows.to_vec(),
+        };
+        // What the control didn't set comes from the start too, not from last frame's colour.
+        if let Some(old) = start.get(active[k]).and_then(to_color).map(|c| c.to_hsb()) {
+            let hsb = [0, 1, 2].map(|n| set[n].unwrap_or(old[n]));
+            rows.clone_from_slice(&start);
+            move_color(rows, active[k], hsb, linked);
+            changed = true;
+        }
+        d.fields.insert(DRAG.into(), Value::Array(start));
+    }
+    if !ui.input(|i| i.pointer.any_down()) {
+        d.fields.remove(DRAG);
     }
     ui.add_space(6.0);
     // The new colours: click one to make it the base colour.

crates/ui-egui/src/panels/color_themes.rs

@@ -164,7 +164,10 @@ fn create(app: &mut VectorcraftApp, ui: &mut Ui) {
     if let Some(k) = w.pressed {
         d.sel = k;
     }
-    if let Some((k, hsb)) = w.moved {
+    if let Some((k, set)) = w.moved
+        && let Some(cur) = d.colors.get(k).map(Color::to_hsb)
+    {
+        let hsb = [0, 1, 2].map(|n| set[n].unwrap_or(cur[n]));
         move_on_wheel(&mut d.colors, k, hsb, d.rule.is_some());
     }
     ui.add_space(8.0);

crates/ui-egui/src/tests_recolor.rs

@@ -202,3 +202,157 @@ fn a_group_keeps_the_colours_no_art_row_took() {
     assert_eq!((&after[0], &after[1]), (&before[0], &json!("rgb 1 2 3")));
     assert_eq!(after[2..], before[2..]);
 }
+
+/// Recolor Artwork on art filled with `colors`, on the Edit tab, drawn by a kittest harness.
+fn edit_harness(colors: &[&str]) -> egui_kittest::Harness<'static, VectorcraftApp> {
+    let mut app = app();
+    art(&mut app, colors);
+    app.run("ui.recolorDialog", json!({})).unwrap();
+    set(&mut app, "tab", json!("edit"));
+    assert_eq!(field(&app, "linked"), json!(true));
+    let mut fonts = false;
+    let mut h = egui_kittest::Harness::builder().with_size(vec2(900.0, 800.0)).build_ui_state(
+        move |ui, app: &mut VectorcraftApp| {
+            if !std::mem::replace(&mut fonts, true) {
+                crate::theme::install_fonts(ui.ctx());
+                return;
+            }
+            dialogs::show(app, ui.ctx());
+        },
+        app,
+    );
+    h.run_steps(4);
+    h
+}
+
+/// The rect of the widget drawn `size` big (the wheel is 220 across, its Brightness slider 220 by 22).
+fn widget_of_size(h: &egui_kittest::Harness<'_, VectorcraftApp>, size: egui::Vec2) -> Rect {
+    h.ctx
+        .viewport(|vp| vp.prev_pass.widgets.layers().flat_map(|(_, w)| w.iter()).find(|w| w.rect.size() == size).map(|w| w.rect))
+        .expect("the widget")
+}
+
+fn new_hsb(app: &VectorcraftApp) -> Vec<[f32; 3]> {
+    field(app, "rows").as_array().unwrap().iter().map(|row| vectorcraft_engine::cmd::color_value(&row["to"]).unwrap().to_hsb()).collect()
+}
+
+/// #1733: on the Edit tab, dragging a handle of CMYK art (linked, Complementary) must leave every
+/// handle on the wheel. The others turn with it (intended), but they keep their place relative to
+/// it: they don't drift while the pointer is held still and don't end up under the dragged one.
+#[test]
+fn dragging_a_wheel_handle_keeps_the_other_handles_apart() {
+    let mut h = edit_harness(&["cmyk 0 100 100 0", "cmyk 100 0 0 0"]);
+    let wheel = widget_of_size(&h, vec2(220.0, 220.0));
+    let (c, r) = (wheel.center(), 100.0);
+    // Each row's new colour: its hue, and where its handle is drawn (as `widgets::harmony_wheel`).
+    let handles = |app: &VectorcraftApp| -> Vec<(f32, Pos2)> {
+        field(app, "rows")
+            .as_array()
+            .unwrap()
+            .iter()
+            .map(|row| {
+                let [hue, s, _] = vectorcraft_engine::cmd::color_value(&row["to"]).unwrap().to_hsb();
+                (hue, c + vec2(hue.to_radians().cos(), -hue.to_radians().sin()) * r * s)
+            })
+            .collect()
+    };
+    let gap = |a: f32, b: f32| (a - b).rem_euclid(360.0).min((b - a).rem_euclid(360.0));
+    let start = handles(h.state());
+    // Drag the red handle a quarter turn, a few points at a time.
+    h.hover_at(start[0].1);
+    h.run_steps(1);
+    h.drag_at(start[0].1);
+    h.run_steps(1);
+    let (mut p, s0) = (start[0].1, start[0].1.distance(c) / r);
+    for k in 1..=12 {
+        let (a, s) = ((start[0].0 + 6.0 * k as f32).to_radians(), s0 + (0.75 - s0) * k as f32 / 12.0);
+        p = c + vec2(a.cos(), -a.sin()) * r * s;
+        h.hover_at(p);
+        h.run_steps(1);
+    }
+    let moved = handles(h.state());
+    assert!(gap(moved[1].0, start[1].0) > 10.0, "linked: the other handle turned with it (intended): {start:?} -> {moved:?}");
+    let brightness = |app: &VectorcraftApp| -> Vec<f32> {
+        field(app, "rows").as_array().unwrap().iter().map(|row| vectorcraft_engine::cmd::color_value(&row["to"]).unwrap().to_hsb()[2]).collect()
+    };
+    let moved_v = brightness(h.state());
+    // Hold the pointer still: nothing should change.
+    h.run_steps(20);
+    let held = handles(h.state());
+    let held_v = brightness(h.state());
+    assert!(
+        moved_v.iter().zip(&held_v).all(|(a, b)| (a - b).abs() < 0.01),
+        "the brightness changed while the pointer was held still: {moved_v:?} -> {held_v:?}"
+    );
+    assert!(gap(held[1].0, moved[1].0) < 1.0, "the other handle turned on its own while the pointer was held still: {moved:?} -> {held:?}");
+    assert!(held[0].1.distance(held[1].1) > 18.0, "the other handle went under the dragged one (pointer {p:?}): {held:?}");
+    let rows = field(h.state(), "rows");
+    h.drop_at(p);
+    h.run_steps(1);
+    assert_eq!(field(h.state(), "rows"), rows, "releasing keeps the colours the drag ended on");
+    // OK keeps the whole drag as one undo step; undo brings the art's colours back.
+    let keys = |app: &mut VectorcraftApp| -> Vec<Value> {
+        let mut k: Vec<Value> =
+            app.session.execute("recolor.colors", &json!({})).unwrap()["colors"].as_array().unwrap().iter().map(|c| c["key"].clone()).collect();
+        k.sort_by_key(Value::to_string);
+        k
+    };
+    let app = h.state_mut();
+    let undo = app.session.doc().unwrap().history.undo.len();
+    dialogs::confirm(app).unwrap();
+    assert_eq!(app.session.doc().unwrap().history.undo.len(), undo + 1);
+    let mut to: Vec<Value> = rows.as_array().unwrap().iter().map(|r| r["to"].clone()).collect();
+    to.sort_by_key(Value::to_string);
+    assert_eq!(keys(app), to);
+    app.run("edit.undo", json!({})).unwrap();
+    assert_eq!(keys(app), [json!("cmyk 0 100 100 0"), json!("cmyk 100 0 0 0")]);
+}
+
+/// A marker dragged past the wheel's edge is fully saturated, every frame and after the release.
+#[test]
+fn a_marker_dragged_past_the_edge_stays_fully_saturated() {
+    let mut h = edit_harness(&["rgb 200 100 100", "rgb 100 200 200"]);
+    let wheel = widget_of_size(&h, vec2(220.0, 220.0));
+    let (c, r) = (wheel.center(), 100.0);
+    let [hue, s, _] = new_hsb(h.state())[0];
+    let along = |d: f32| c + vec2(hue.to_radians().cos(), -hue.to_radians().sin()) * d;
+    let (at, out) = (along(r * s), along(r + 8.0));
+    h.hover_at(at);
+    h.run_steps(1);
+    h.drag_at(at);
+    h.run_steps(1);
+    // Outwards a few points at a time, so the drag starts on the marker.
+    for k in 1..=10 {
+        h.hover_at(along(r * s + (r + 8.0 - r * s) * k as f32 / 10.0));
+        h.run_steps(1);
+    }
+    for _ in 0..3 {
+        h.run_steps(1);
+        assert_eq!(new_hsb(h.state())[0][1], 1.0, "held past the edge");
+    }
+    h.drop_at(out);
+    h.run_steps(1);
+    assert_eq!(new_hsb(h.state())[0][1], 1.0, "released past the edge");
+}
+
+/// The Brightness slider dragged to its top gives full brightness, every frame and after the release.
+#[test]
+fn the_brightness_slider_dragged_to_the_top_stays_there() {
+    let mut h = edit_harness(&["rgb 200 100 100", "rgb 100 200 200"]);
+    let slider = widget_of_size(&h, vec2(220.0, 22.0));
+    let v = new_hsb(h.state())[0][2];
+    let at = egui::pos2(slider.left() + 4.0 + v * (slider.width() - 8.0), slider.center().y);
+    let top = egui::pos2(slider.right() + 10.0, slider.center().y);
+    h.hover_at(at);
+    h.run_steps(1);
+    h.drag_at(at);
+    h.run_steps(1);
+    h.hover_at(top);
+    for _ in 0..3 {
+        h.run_steps(1);
+        assert_eq!(new_hsb(h.state())[0][2], 1.0, "held at the top");
+    }
+    h.drop_at(top);
+    h.run_steps(1);
+    assert_eq!(new_hsb(h.state())[0][2], 1.0, "released at the top");
+}

crates/ui-egui/src/widgets.rs

@@ -1979,9 +1979,9 @@ fn wheel_shape(c: Pos2, r: f32, v: f32) -> egui::Shape {
 pub struct WheelResponse {
     /// The colour whose marker was pressed (it becomes the selected one).
     pub pressed: Option<usize>,
-    /// A colour moved to a new hue, saturation and brightness (`[h°, s, v]`), by dragging its
-    /// marker or (the selected colour) the brightness slider.
-    pub moved: Option<(usize, [f32; 3])>,
+    /// A colour moved by dragging its marker (a new hue and saturation) or, the selected colour,
+    /// the brightness slider (a new brightness): `[h°, s, v]`, `None` where the control kept it.
+    pub moved: Option<(usize, [Option<f32>; 3])>,
 }
 
 /// The harmony wheel of Recolor Artwork's Edit tab and the Color Themes panel: a hue and saturation
@@ -2021,11 +2021,11 @@ pub fn harmony_wheel(ui: &mut Ui, id: &str, size: f32, colors: &[Color], base: O
         }
         if resp.dragged()
             && let (Some(i), Some(p)) = (ui.data(|m| m.get_temp::<Option<usize>>(drag)).flatten(), resp.interact_pointer_pos())
-            && let Some(old) = colors.get(i)
+            && i < colors.len()
         {
             let dv = p - c;
             let h = (-dv.y).atan2(dv.x).to_degrees().rem_euclid(360.0);
-            out.moved = Some((i, [h, (dv.length() / r).clamp(0.0, 1.0), old.to_hsb()[2]]));
+            out.moved = Some((i, [Some(h), Some((dv.length() / r).clamp(0.0, 1.0)), None]));
         }
         resp.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Slider, ui.is_enabled(), tl!("Color wheel")));
     });
@@ -2037,7 +2037,7 @@ pub fn harmony_wheel(ui: &mut Ui, id: &str, size: f32, colors: &[Color], base: O
             && let (Some(nv), _) =
                 color_slider_named(ui, (id, "brightness"), tl!("Brightness"), v, size, &|x| crate::panels::c32(&Color::from_hsb(h, s, x)))
         {
-            out.moved = Some((b, [h, s, nv]));
+            out.moved = Some((b, [None, None, Some(nv)]));
         }
     });
     out