From 06b0ad7af08bc73bff2406845d82942f2d5259d5 Mon Sep 17 00:00:00 2001 From: Carter Smith <51297686+carterwsmith@users.noreply.github.com> Date: Wed, 7 Oct 2026 02:56:00 -0700 Subject: [PATCH] Color Picker and Color panel fields accept typed values (#485) The Color Picker's hex field was rebuilt from the colour every frame, so anything short of six valid digits was thrown away and it stayed at 000000. It now shows what is being typed while it has focus. The Lab and CMYK fields applied every keystroke through clamped sRGB, so typing "60" into L went through L = 6 first and changed a and b. The dialog now keeps the typed Lab or CMYK values while they still produce the colour, as it already did for HSB. The Color panel's Hue and Saturation fields (Studio and Classic themes) reset to 0 on black and greys; they now keep the typed values. parse_hex no longer panics on multi-byte text. --- crates/ui-egui/src/color_picker_ui.rs | 210 +++++++++++++++++++++----- crates/ui-egui/src/panels.rs | 45 +++++- 2 files changed, 213 insertions(+), 42 deletions(-) diff --git a/crates/ui-egui/src/color_picker_ui.rs b/crates/ui-egui/src/color_picker_ui.rs index 7391bb09..2f769cad 100644 --- a/crates/ui-egui/src/color_picker_ui.rs +++ b/crates/ui-egui/src/color_picker_ui.rs @@ -55,7 +55,8 @@ pub fn parse_hex(s: &str) -> Option<[f32; 3]> { if h.len() != 6 { return None; } - let c = |i: usize| u8::from_str_radix(&h[i..i + 2], 16).ok().map(|v| f32::from(v) / 255.0); + // `get`, not `[..]`: typed text may be multi-byte, and a byte range can split a character. + let c = |i: usize| u8::from_str_radix(h.get(i..i + 2)?, 16).ok().map(|v| f32::from(v) / 255.0); Some([c(0)?, c(2)?, c(4)?]) } @@ -139,22 +140,63 @@ pub fn owns(f: &Map) -> bool { f.contains_key("__colorPicker") } -/// Current colour as (rgb, hsv), keeping the stored hue/saturation when they still match `color`. -pub fn current(f: &Map) -> ([f32; 3], [f32; 3]) { - let rgb = f.get("color").and_then(Value::as_str).and_then(parse_hex).unwrap_or([0.0; 3]); - let stored: Option<[f32; 3]> = f.get("__hsv").and_then(|v| serde_json::from_value(v.clone()).ok()); - let hsv = match stored { - Some(h) if hex(hsv_to_rgb(h[0], h[1], h[2])) == hex(rgb) => h, - _ => rgb_to_hsv(rgb), - }; - (rgb, hsv) +/// The colour in every model the dialog shows. +pub struct Components { + pub rgb: [f32; 3], + pub hsv: [f32; 3], + pub lab: [f32; 3], + pub cmyk: [f32; 4], } -fn set_rgb(f: &mut Map, rgb: [f32; 3], hsv: Option<[f32; 3]>) { +/// Component values an edit stores next to the new `color`. sRGB alone loses them: greys have no +/// hue, and Lab or CMYK values outside sRGB are clamped. Typing "60" into L passes through L = 6 +/// first; without the stored Lab the clamped colour would change `a` and `b` under the user. +enum Keep { + Nothing, + Hsv([f32; 3]), + Lab([f32; 3]), + Cmyk([f32; 4]), +} + +/// The stored `key` components, if they still produce `rgb`. +fn kept(f: &Map, key: &str, rgb: [f32; 3], to_rgb: impl Fn([f32; N]) -> [f32; 3]) -> Option<[f32; N]> +where + [f32; N]: serde::de::DeserializeOwned, +{ + let v: [f32; N] = f.get(key).and_then(|v| serde_json::from_value(v.clone()).ok())?; + (hex(to_rgb(v)) == hex(rgb)).then_some(v) +} + +/// Current colour, keeping the stored HSB, Lab or CMYK values while they still match `color`. +pub fn current(f: &Map) -> Components { + let rgb = f.get("color").and_then(Value::as_str).and_then(parse_hex).unwrap_or([0.0; 3]); + Components { + rgb, + hsv: kept(f, "__hsv", rgb, |h| hsv_to_rgb(h[0], h[1], h[2])).unwrap_or_else(|| rgb_to_hsv(rgb)), + lab: kept(f, "__lab", rgb, photocraft_color::convert::lab_to_srgb).unwrap_or_else(|| photocraft_color::convert::srgb_to_lab(rgb)), + cmyk: kept(f, "__cmyk", rgb, photocraft_color::convert::cmyk_to_rgb).unwrap_or_else(|| photocraft_color::convert::rgb_to_cmyk(rgb)), + } +} + +fn set_rgb(f: &mut Map, rgb: [f32; 3], keep: Keep) { let web = f.get("__webOnly").and_then(Value::as_bool).unwrap_or(false); - let rgb = if web { rgb.map(|v| (v * 5.0).round() / 5.0) } else { rgb }; + let (rgb, keep) = if web { (rgb.map(|v| (v * 5.0).round() / 5.0), Keep::Nothing) } else { (rgb, keep) }; f.insert("color".into(), json!(hex(rgb))); - f.insert("__hsv".into(), json!(hsv.filter(|_| !web).unwrap_or_else(|| rgb_to_hsv(rgb)))); + for key in ["__hsv", "__lab", "__cmyk"] { + f.remove(key); + } + match keep { + Keep::Nothing => {} + Keep::Hsv(h) => { + f.insert("__hsv".into(), json!(h)); + } + Keep::Lab(l) => { + f.insert("__lab".into(), json!(l)); + } + Keep::Cmyk(k) => { + f.insert("__cmyk".into(), json!(k)); + } + } } fn grid_mesh(rect: Rect, n: usize, color: impl Fn(f32, f32) -> [f32; 3]) -> Mesh { @@ -179,7 +221,8 @@ fn grid_mesh(rect: Rect, n: usize, color: impl Fn(f32, f32) -> [f32; 3]) -> Mesh pub fn body(ui: &mut egui::Ui, f: &mut Map) { let t = Tokens::get(ui.ctx()); let mode = f.get("__mode").and_then(Value::as_str).unwrap_or("h").to_string(); - let (rgb, hsv) = current(f); + let now = current(f); + let (rgb, hsv) = (now.rgb, now.hsv); let (fx, fy, fz) = locate(&mode, hsv, rgb); ui.horizontal_top(|ui| { // Colour field. @@ -191,13 +234,13 @@ pub fn body(ui: &mut egui::Ui, f: &mut Map) { if let Some(p) = resp.interact_pointer_pos().filter(|_| resp.dragged() || resp.clicked()) { 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 h = match mode.as_str() { - "h" => Some([fz * 360.0, x, 1.0 - y]), - "s" => Some([x * 360.0, fz, 1.0 - y]), - "v" => Some([x * 360.0, 1.0 - y, fz]), - _ => None, + let keep = match mode.as_str() { + "h" => Keep::Hsv([fz * 360.0, x, 1.0 - y]), + "s" => Keep::Hsv([x * 360.0, fz, 1.0 - y]), + "v" => Keep::Hsv([x * 360.0, 1.0 - y, fz]), + _ => Keep::Nothing, }; - set_rgb(f, c, h); + set_rgb(f, c, keep); } ui.add_space(6.0); // Component slider (hue runs 360° at the top to 0° at the bottom, like Photoshop). @@ -216,8 +259,10 @@ pub fn body(ui: &mut egui::Ui, f: &mut Map) { "v" => Some([hsv[0], hsv[1], z]), _ => None, }; - let c = h.map_or_else(|| slider_color(&mode, z, hsv, rgb), |h| hsv_to_rgb(h[0], h[1], h[2])); - set_rgb(f, c, h); + match h { + Some(h) => set_rgb(f, hsv_to_rgb(h[0], h[1], h[2]), Keep::Hsv(h)), + None => set_rgb(f, slider_color(&mode, z, hsv, rgb), Keep::Nothing), + } } ui.add_space(14.0); ui.vertical(|ui| { @@ -231,27 +276,26 @@ pub fn body(ui: &mut egui::Ui, f: &mut Map) { ui.painter().rect_stroke(sw, 0.0, Stroke::new(1.0, t.field_border), StrokeKind::Outside); let click_cur = ui.interact(cur, ui.id().with("cp-current"), Sense::click()); if click_cur.on_hover_text(tl!("Click to restore the current colour")).clicked() { - set_rgb(f, orig, None); + set_rgb(f, orig, Keep::Nothing); } ui.label(egui::RichText::new(tl!("current")).size(11.0).color(t.text_dim)); ui.add_space(10.0); let mut web = f.get("__webOnly").and_then(Value::as_bool).unwrap_or(false); if widgets::checkbox(ui, &mut web, tl!("Only Web Colors")).changed() { f.insert("__webOnly".into(), json!(web)); - set_rgb(f, rgb, None); + set_rgb(f, rgb, Keep::Nothing); } }); ui.add_space(10.0); - ui.vertical(|ui| fields(ui, f, &mode, rgb, hsv)); + ui.vertical(|ui| fields(ui, f, &mode, &now)); }); } /// Component radios and numeric fields: HSB, RGB, Lab, CMYK and hex. -fn fields(ui: &mut egui::Ui, f: &mut Map, mode: &str, rgb: [f32; 3], hsv: [f32; 3]) { +fn fields(ui: &mut egui::Ui, f: &mut Map, mode: &str, now: &Components) { let t = Tokens::get(ui.ctx()); - let lab = photocraft_color::convert::srgb_to_lab(rgb); - let cmyk = photocraft_color::convert::rgb_to_cmyk(rgb); - let mut edit: Option<([f32; 3], Option<[f32; 3]>)> = None; + let &Components { rgb, hsv, lab, cmyk } = now; + let mut edit: Option<([f32; 3], Keep)> = None; let mut new_mode: Option<&str> = None; egui::Grid::new("cp-fields").num_columns(4).spacing([6.0, 4.0]).show(ui, |ui| { let vals = [hsv[0], hsv[1] * 100.0, hsv[2] * 100.0, rgb[0] * 255.0, rgb[1] * 255.0, rgb[2] * 255.0]; @@ -275,10 +319,10 @@ fn fields(ui: &mut egui::Ui, f: &mut Map, mode: &str, rgb: [f32; if i < 3 { h[i] = if i == 0 { v } else { v / 100.0 }; c = hsv_to_rgb(h[0], h[1], h[2]); - edit = Some((c, Some(h))); + edit = Some((c, Keep::Hsv(h))); } else { c[i - 3] = v / 255.0; - edit = Some((c, None)); + edit = Some((c, Keep::Nothing)); } } ui.label(egui::RichText::new(units[i]).color(t.text_faint)); @@ -291,7 +335,7 @@ fn fields(ui: &mut egui::Ui, f: &mut Map, mode: &str, rgb: [f32; if widgets::value_field(ui, &mut x, range, "", 54.0).changed() { let mut l = lab; l[i] = x; - edit = Some((photocraft_color::convert::lab_to_srgb(l).map(|c| c.clamp(0.0, 1.0)), None)); + edit = Some((photocraft_color::convert::lab_to_srgb(l).map(|c| c.clamp(0.0, 1.0)), Keep::Lab(l))); } ui.label(""); ui.end_row(); @@ -303,27 +347,36 @@ fn fields(ui: &mut egui::Ui, f: &mut Map, mode: &str, rgb: [f32; if widgets::value_field(ui, &mut x, 0.0..=100.0, "", 54.0).changed() { let mut k = cmyk; k[i] = x / 100.0; - edit = Some((photocraft_color::convert::cmyk_to_rgb(k).map(|c| c.clamp(0.0, 1.0)), None)); + edit = Some((photocraft_color::convert::cmyk_to_rgb(k).map(|c| c.clamp(0.0, 1.0)), Keep::Cmyk(k))); } ui.label(egui::RichText::new("%").color(t.text_faint)); ui.end_row(); } ui.label(""); ui.label(egui::RichText::new("#").color(t.text_dim)); - let mut h = hex(rgb).trim_start_matches('#').to_string(); - let r = ui.add(egui::TextEdit::singleline(&mut h).desired_width(54.0).font(crate::theme::mono(12.0))); + // While the field has focus it shows what is being typed, not the colour: rebuilding it from + // the colour every frame would throw away every keystroke until six valid digits were in. + let id = ui.id().with("cp-hex"); + let typing = if ui.memory(|m| m.has_focus(id)) { ui.data(|d| d.get_temp::(id)) } else { None }; + let mut h = typing.unwrap_or_else(|| hex(rgb).trim_start_matches('#').to_string()); + let r = ui.add(egui::TextEdit::singleline(&mut h).id(id).char_limit(6).desired_width(54.0).font(crate::theme::mono(12.0))); + if r.has_focus() { + ui.data_mut(|d| d.insert_temp(id, h.clone())); + } else { + ui.data_mut(|d| d.remove::(id)); + } if r.changed() && let Some(c) = parse_hex(&h) { - edit = Some((c, None)); + edit = Some((c, Keep::Nothing)); } ui.end_row(); }); if let Some(m) = new_mode { f.insert("__mode".into(), json!(m)); } - if let Some((c, h)) = edit { - set_rgb(f, c, h); + if let Some((c, keep)) = edit { + set_rgb(f, c, keep); } } @@ -341,8 +394,87 @@ pub fn confirm(app: &mut PhotocraftApp, f: &Map) -> Result Harness<'static, Map> { + let mut f = Map::new(); + f.insert("__colorPicker".into(), json!("foreground")); + f.insert("color".into(), json!(color)); + let mut h = Harness::builder().with_size(vec2(700.0, 500.0)).build_ui_state( + |ui, f: &mut Map| { + if ui.ctx().fonts(|fonts| fonts.families().contains(&egui::FontFamily::Name("medium".into()))) { + body(ui, f); + } + }, + f, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ALL[0]); + h.run_steps(2); + h + } + + /// Type `text` into the focused field one key per frame, like a person typing. + fn type_keys(h: &mut Harness<'static, Map>, text: &str) { + for ch in text.chars() { + h.event(egui::Event::Text(ch.to_string())); + h.run_steps(1); + } + } + + fn color(h: &Harness<'static, Map>) -> String { + h.state().get("color").and_then(Value::as_str).unwrap().to_string() + } + + #[test] + fn hex_field_takes_typed_digits() { + let mut h = picker("#000000"); + h.get_by_role(Role::TextInput).click(); + h.run_steps(1); + h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::A); + h.run_steps(1); + type_keys(&mut h, "ff88"); + // Partial input stays in the field instead of snapping back to the colour. + assert_eq!(h.get_by_role(Role::TextInput).value().as_deref(), Some("ff88")); + assert_eq!(color(&h), "#000000"); + type_keys(&mut h, "00"); + assert_eq!(color(&h), "#ff8800"); + h.key_press(egui::Key::Tab); + h.run_steps(2); + assert_eq!(color(&h), "#ff8800"); + } + + #[test] + fn lab_and_cmyk_fields_take_multi_digit_values() { + let base = parse_hex("#336699").unwrap(); + let lab = photocraft_color::convert::srgb_to_lab(base); + let cmyk = photocraft_color::convert::rgb_to_cmyk(base); + // Fields in order: H S B R G B, L a b, C M Y K. + for (field, typed, want) in [ + (6, "60", photocraft_color::convert::lab_to_srgb([60.0, lab[1], lab[2]])), + (7, "30", photocraft_color::convert::lab_to_srgb([lab[0], 30.0, lab[2]])), + (9, "40", photocraft_color::convert::cmyk_to_rgb([0.4, cmyk[1], cmyk[2], cmyk[3]])), + ] { + let mut h = picker("#336699"); + h.query_all_by_role(Role::SpinButton).nth(field).unwrap().click(); + h.run_steps(1); + type_keys(&mut h, typed); + h.key_press(egui::Key::Tab); + h.run_steps(2); + assert_eq!(color(&h), hex(want), "field {field} <- {typed}"); + } + } + + #[test] + fn parse_hex_rejects_multibyte_text() { + // Six bytes, three characters: the byte range 0..2 splits "é", which used to panic. + assert_eq!(parse_hex("aé€"), None); + assert_eq!(parse_hex("#ff880"), None); + } + #[test] fn hsv_round_trips() { for c in [[1.0, 0.0, 0.0], [0.2, 0.6, 0.4], [0.5, 0.5, 0.5], [0.0, 0.0, 1.0], [0.9, 0.8, 0.1]] { @@ -372,7 +504,7 @@ mod tests { let mut f = Map::new(); f.insert("color".into(), json!("#808080")); f.insert("__hsv".into(), json!([200.0, 0.0, 128.0 / 255.0])); - assert_eq!(current(&f).1[0], 200.0); + assert_eq!(current(&f).hsv[0], 200.0); let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default()); let id = open(&mut app, "background"); let d = app.ui.dialog_mut(id).unwrap(); diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index 3bbc2717..8c226b8e 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -1209,7 +1209,11 @@ fn swatches(app: &mut PhotocraftApp, ui: &mut egui::Ui) { fn color_picker(app: &mut PhotocraftApp, ui: &mut egui::Ui) { let fg = app.session.tools.foreground; - let hsva0 = srgb_hsva(fg); + // Keep the last edited HSB while it still gives the foreground: black and greys have no hue or + // saturation of their own, so recomputing them from RGB would reset what was just typed to 0. + let key = egui::Id::new("color-picker-hsva"); + let stored: Option = ui.data(|d| d.get_temp(key)); + let hsva0 = stored.filter(|h| h.to_srgb() == srgb_bytes(fg)).unwrap_or_else(|| srgb_hsva(fg)); let mut h = hsva0.h * 360.0; let mut s = hsva0.s * 100.0; let mut v = hsva0.v * 100.0; @@ -1223,6 +1227,7 @@ fn color_picker(app: &mut PhotocraftApp, ui: &mut egui::Ui) { let hsva = egui::ecolor::Hsva::new(h / 360.0, s / 100.0, v / 100.0, 1.0); if hsva != hsva0 { app.session.tools.foreground = hsva_srgb(hsva); + ui.data_mut(|d| d.insert_temp(key, hsva)); } let [r, g, b, _] = hsva.to_srgba_unmultiplied(); let t = Tokens::get(ui.ctx()); @@ -2188,9 +2193,13 @@ fn color_field(app: &mut PhotocraftApp, ui: &mut egui::Ui) { /// Tool colours are sRGB-encoded floats; egui's Hsva works on sRGB bytes via these helpers /// (its `from_rgba_unmultiplied` expects *linear* RGB, which gave wrong readouts). -fn srgb_hsva(c: [f32; 4]) -> egui::ecolor::Hsva { +fn srgb_bytes(c: [f32; 4]) -> [u8; 3] { let q = |v: f32| (v.clamp(0.0, 1.0) * 255.0 + 0.5) as u8; - egui::ecolor::Hsva::from_srgb([q(c[0]), q(c[1]), q(c[2])]) + [q(c[0]), q(c[1]), q(c[2])] +} + +fn srgb_hsva(c: [f32; 4]) -> egui::ecolor::Hsva { + egui::ecolor::Hsva::from_srgb(srgb_bytes(c)) } fn hsva_srgb(h: egui::ecolor::Hsva) -> [f32; 4] { @@ -2386,6 +2395,36 @@ mod color_tests { } } } + + #[test] + fn hue_and_saturation_fields_take_values_on_black() { + use egui_kittest::kittest::Queryable; + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.session.tools.foreground = [0.0, 0.0, 0.0, 1.0]; + let mut h = egui_kittest::Harness::builder().with_size(vec2(300.0, 300.0)).build_ui_state( + |ui, app: &mut PhotocraftApp| { + if ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) { + color_picker(app, ui); + } + }, + app, + ); + PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::Studio); + h.run_steps(2); + // Hue, Saturation, Brightness, typed one key per frame. Black has no hue or saturation of its + // own, so the first two used to reset to 0 and this ended on white. + for (field, typed) in [(0, "120"), (1, "100"), (2, "100")] { + h.query_all_by_role(egui::accesskit::Role::SpinButton).nth(field).unwrap().click(); + h.run_steps(1); + for ch in typed.chars() { + h.event(egui::Event::Text(ch.to_string())); + h.run_steps(1); + } + h.key_press(egui::Key::Tab); + h.run_steps(2); + } + assert_eq!(srgb_bytes(h.state().session.tools.foreground), [0, 255, 0]); + } } #[cfg(test)]