From 102429e6b56b72a50e22adbdca520d5c2414179a Mon Sep 17 00:00:00 2001 From: RealtaNua Date: Thu, 15 Oct 2026 18:13:47 +0700 Subject: [PATCH] fix(ui): scrubbing a colour picker field is one undo step (#256) The RGB/HSL/HSB fields committed every frame of a scrub, so one drag left about ten undo steps, and the alpha field ignored scrubs. Both now edit live and commit when let go. Refs #256 Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/ui-egui/src/colorpicker.rs | 37 ++++++++++++++++++++----------- crates/ui-egui/src/field_tests.rs | 37 +++++++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/crates/ui-egui/src/colorpicker.rs b/crates/ui-egui/src/colorpicker.rs index e55130c52..5c6f7a0fa 100644 --- a/crates/ui-egui/src/colorpicker.rs +++ b/crates/ui-egui/src/colorpicker.rs @@ -212,11 +212,13 @@ fn page_colors(app: &FigApp, ui: &Ui) -> Vec { /// Colour models the value row can show. const MODELS: [&str; 5] = ["Hex", "RGB", "HSL", "HSB", "CSS"]; -/// The model dropdown with its fields and the alpha field. Returns a committed colour. -fn model_fields(ui: &mut Ui, c: Color) -> Option { +/// The model dropdown with its fields and the alpha field. Returns a new colour and whether the +/// edit is done (false while a field is being scrubbed). +fn model_fields(ui: &mut Ui, c: Color) -> Option<(Color, bool)> { let model_id = Id::new("cp_model"); let model: usize = ui.data(|d| d.get_temp(model_id)).unwrap_or(0).min(MODELS.len() - 1); let mut out: Option = None; + let mut done = true; ui.horizontal(|ui| { ui.add_space(16.0); ui.spacing_mut().item_spacing.x = 4.0; @@ -238,7 +240,8 @@ fn model_fields(ui: &mut Ui, c: Color) -> Option { w, 1.0, ) { - Some(widgets::FieldEdit::Set(x)) | Some(widgets::FieldEdit::Scrub(x, _)) => Some(x), + Some(widgets::FieldEdit::Set(x)) => Some((x, true)), + Some(widgets::FieldEdit::Scrub(x, end)) => Some((x, end)), None => None, }; match MODELS.get(model).copied().unwrap_or("Hex") { @@ -246,7 +249,8 @@ fn model_fields(ui: &mut Ui, c: Color) -> Option { let [r, g, b, _] = c.to_rgba8(); let vals = [f64::from(r), f64::from(g), f64::from(b)]; for (k, v) in vals.iter().enumerate() { - if let Some(x) = num(ui, ["r", "g", "b"][k.min(2)], *v, 31.0) { + if let Some((x, end)) = num(ui, ["r", "g", "b"][k.min(2)], *v, 31.0) { + done = end; let mut rgb = vals; if let Some(slot) = rgb.get_mut(k) { *slot = x.clamp(0.0, 255.0); @@ -266,7 +270,8 @@ fn model_fields(ui: &mut Ui, c: Color) -> Option { }; let vals = [f64::from(h), f64::from(s2) * 100.0, f64::from(l2) * 100.0]; for (k, v) in vals.iter().enumerate() { - if let Some(x) = num(ui, ["h", "s", "l"][k.min(2)], *v, 31.0) { + if let Some((x, end)) = num(ui, ["h", "s", "l"][k.min(2)], *v, 31.0) { + done = end; let mut v3 = vals; if let Some(slot) = v3.get_mut(k) { *slot = if k == 0 { x.rem_euclid(360.0) } else { x.clamp(0.0, 100.0) }; @@ -288,14 +293,20 @@ fn model_fields(ui: &mut Ui, c: Color) -> Option { } } } - if MODELS.get(model) != Some(&"CSS") - && let Some(widgets::FieldEdit::Set(v)) = - widgets::number_field(ui, Id::new("cp_a"), widgets::Lead::None, Some((f64::from(out.unwrap_or(c).a) * 100.0).round()), "%", 44.0, 1.0) - { - out = Some(out.unwrap_or(c).with_alpha((v / 100.0).clamp(0.0, 1.0) as f32)); + if MODELS.get(model) != Some(&"CSS") { + let alpha = Some((f64::from(out.unwrap_or(c).a) * 100.0).round()); + let edit = match widgets::number_field(ui, Id::new("cp_a"), widgets::Lead::None, alpha, "%", 44.0, 1.0) { + Some(widgets::FieldEdit::Set(v)) => Some((v, true)), + Some(widgets::FieldEdit::Scrub(v, end)) => Some((v, end)), + None => None, + }; + if let Some((v, end)) = edit { + done = end; + out = Some(out.unwrap_or(c).with_alpha((v / 100.0).clamp(0.0, 1.0) as f32)); + } } }); - out + out.map(|c| (c, done)) } fn hue_color(ctx: &egui::Context, h: f32) -> Color32 { @@ -744,11 +755,11 @@ pub fn show(app: &mut FigApp, ctx: &egui::Context) { done |= d; } // Colour model fields (Hex, RGB, HSL, HSB or CSS) + alpha. - if let Some(c) = model_fields(ui, nc) { + if let Some((c, d)) = model_fields(ui, nc) { nc = c; hsb = Hsb::from_color(nc); changed = true; - done = true; + done |= d; } ui.data_mut(|d| d.insert_temp(Id::new("cp_hsb"), (hsb.h, hsb.s, hsb.b, nc.to_rgba8()))); // A Display P3 colour sRGB can't show: the canvas (sRGB) shows the nearest one. diff --git a/crates/ui-egui/src/field_tests.rs b/crates/ui-egui/src/field_tests.rs index 4635d8621..e1ff460d4 100644 --- a/crates/ui-egui/src/field_tests.rs +++ b/crates/ui-egui/src/field_tests.rs @@ -2,10 +2,10 @@ //! a scrub and an arrow key move a value, and the Design panel's fields driven through egui //! headless: scrubbing by the label with and without Shift and Alt, arrow keys, arithmetic on //! commit, Tab and Shift+Tab, Escape, fields showing "Mixed", and one undo step per scrub or -//! commit. +//! commit (the colour picker's fields too). use egui::accesskit::Role; -use egui::{Event, Key, Modifiers, Pos2, pos2}; +use egui::{Event, Id, Key, Modifiers, Pos2, pos2}; use figcraft_doc::NodeId; use figcraft_engine::Session; use serde_json::json; @@ -216,6 +216,11 @@ fn type_into(u: &mut Ui, name: &str, text: &str, end: Key) { /// Scrub field `name` by its label: `dx` points with `modifiers` held, then let go. fn scrub(u: &mut Ui, name: &str, dx: f32, modifiers: Modifiers) { let from = label(u, name); + drag(u, from, dx, modifiers); +} + +/// Press at `from`, move `dx` points sideways in small steps with `modifiers` held, then let go. +fn drag(u: &mut Ui, from: Pos2, dx: f32, modifiers: Modifiers) { let press = |pressed| Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed, modifiers }; u.frame_with(vec![Event::PointerMoved(from), press(true)], modifiers); let steps = 10; @@ -404,3 +409,31 @@ fn mixed_values_take_edits_relative_to_each_layer() { assert_eq!((geom(&u, a).0, geom(&u, b).0), (64.0, 64.0)); assert_eq!(field(&mut u, "X").1, "64"); } + +/// The middle of the label (what scrubs) of the field with id `id`, as last painted. +fn lead(u: &Ui, id: Id) -> Pos2 { + u.ctx.read_response(id.with("lead")).unwrap_or_else(|| panic!("no field {id:?}")).rect.center() +} + +#[test] +fn scrubbing_colour_picker_fields_is_live_and_one_undo_step() { + let (mut u, a, _) = setup(); + let rgba = |u: &Ui| u.app.session.doc().unwrap().node(a).unwrap().fills[0].solid_color().unwrap().to_rgba8(); + let [r0, .., a0] = rgba(&u); + u.ctx.data_mut(|d| d.insert_temp(Id::new("cp_model"), 1usize)); + u.app.ui.picker = Some(crate::colorpicker::PickerTarget::Paint { fills: true, index: 0, ids: vec![a] }); + // The picker window takes a frame to size itself. + for _ in 0..3 { + u.frame(vec![]); + } + let steps = undo_steps(&u); + let from = lead(&u, Id::new(("cp_f", "r"))); + drag(&mut u, from, -20.0, Modifiers::NONE); + assert_eq!((rgba(&u)[0], undo_steps(&u)), (r0 - 20, steps + 1), "R"); + // The alpha field: a scrub moves it too. + let from = lead(&u, Id::new("cp_a")); + drag(&mut u, from, -30.0, Modifiers::NONE); + let [.., alpha] = rgba(&u); + assert!(alpha < a0, "alpha {a0} → {alpha}"); + assert_eq!(undo_steps(&u), steps + 2, "alpha"); +}