shifathosn
/
photocraft
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Carter Smith 2026-10-07 02:56:00 -07:00 committed by GitHub
parent 51d85c1170
commit 06b0ad7af0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 213 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

210
crates/ui-egui/src/color_picker_ui.rs
View File

@ -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<String, Value>) -> 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<String, Value>) -> ([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<String, Value>, 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<const N: usize>(f: &Map<String, Value>, 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<String, Value>) -> 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<String, Value>, 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<String, Value>) {
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<String, Value>) {
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<String, Value>) {
"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<String, Value>) {
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<String, Value>, mode: &str, rgb: [f32; 3], hsv: [f32; 3]) {
fn fields(ui: &mut egui::Ui, f: &mut Map<String, Value>, 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<String, Value>, 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<String, Value>, 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<String, Value>, 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::<String>(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::<String>(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<String, Value>) -> Result<Value,
#[cfg(test)]
mod tests {
use egui::accesskit::Role;
use egui_kittest::Harness;
use egui_kittest::kittest::Queryable;
use super::*;
fn picker(color: &str) -> Harness<'static, Map<String, Value>> {
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<String, Value>| {
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<String, Value>>, 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, Value>>) -> 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();

45
crates/ui-egui/src/panels.rs
View File

@ -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<egui::ecolor::Hsva> = 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)]