From cc40722f70f15ace0eb17882bf9cea90346b0a65 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 19:12:50 +0000 Subject: [PATCH] Select Color: small cleanups found in review - Keep the modal keyboard comment with the code it describes and match shortcut keys without an extra closure. - Drop unused ColorEditor::mode, EyedropperMsg::Cancel and a duplicate rgb helper; re-export nearest_aci instead of wrapping it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013rGmDUNr1hUf4zo1X3dak8 --- src/app/update/mod.rs | 17 ++++++++--------- src/ui/color_select/editor.rs | 4 ---- src/ui/color_select/eyedropper.rs | 8 ++------ src/ui/color_select/math.rs | 6 +++--- src/ui/color_select/mod.rs | 8 ++------ src/ui/color_select/picker.rs | 9 ++++----- 6 files changed, 19 insertions(+), 33 deletions(-) diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 2dbc0c81a..6b25dd7e1 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -370,19 +370,11 @@ impl OpenCADStudio { let tab = &self.tabs[self.active_tab]; (started, tab.id, tab.scene.geometry_epoch) }); - // A modal dialog must capture the keyboard the same way it already - // captures the mouse. Otherwise keystrokes from the global key - // subscription leak past the modal into the command line and fire as - // commands once the dialog closes. While a modal is open, Escape - // closes it and every other keystroke-derived message is swallowed; - // the modal's own text fields keep working because they emit their own - // (non-blocked) messages. (#126) // The Select Color panel can open on top of another dialog, so it // takes the keyboard first: Esc cancels, Enter confirms, the arrows // walk the index palette, and every other keystroke is swallowed. if self.color_picker.is_some() { use crate::ui::color_select::ColorPickerMsg; - let last_key = |key: &str| key.rsplit('+').next().map(str::to_owned); match &msg { Message::CommandEscape => { return self.update(Message::ColorPicker(ColorPickerMsg::Cancel)); @@ -390,7 +382,7 @@ impl OpenCADStudio { Message::CommandFinalize => { return self.update(Message::ColorPicker(ColorPickerMsg::Ok)); } - Message::ShortcutPressed(key) => match last_key(key).as_deref() { + Message::ShortcutPressed(key) => match key.rsplit('+').next() { Some("ESCAPE") => { return self.update(Message::ColorPicker(ColorPickerMsg::Cancel)); } @@ -413,6 +405,13 @@ impl OpenCADStudio { return Task::none(); } } + // A modal dialog must capture the keyboard the same way it already + // captures the mouse. Otherwise keystrokes from the global key + // subscription leak past the modal into the command line and fire as + // commands once the dialog closes. While a modal is open, Escape + // closes it and every other keystroke-derived message is swallowed; + // the modal's own text fields keep working because they emit their own + // (non-blocked) messages. (#126) if self.active_modal.is_some() { if matches!(msg, Message::CommandEscape) || matches!(&msg, Message::ShortcutPressed(key) if key.rsplit('+').next() == Some("ESCAPE")) diff --git a/src/ui/color_select/editor.rs b/src/ui/color_select/editor.rs index c83ce5fe4..f003629c4 100644 --- a/src/ui/color_select/editor.rs +++ b/src/ui/color_select/editor.rs @@ -75,10 +75,6 @@ impl ColorEditor { self.rgb } - pub fn mode(&self) -> EditorMode { - self.mode - } - pub fn set_rgb(&mut self, (r, g, b): (u8, u8, u8)) { let next = math::rgb_to_hsb(r, g, b); // A gray has no hue and black no saturation; keep the previous ones diff --git a/src/ui/color_select/eyedropper.rs b/src/ui/color_select/eyedropper.rs index 87c839356..2c7050e38 100644 --- a/src/ui/color_select/eyedropper.rs +++ b/src/ui/color_select/eyedropper.rs @@ -5,6 +5,7 @@ //! the window; a magnifier beside the cursor shows the pixels around it from //! that screenshot, and a click picks the centre pixel. +use super::editor::rgb_color; use super::{math, ColorPickerMsg}; use crate::app::Message; use iced::widget::{column, container, mouse_area, pin, row, stack, text, Space}; @@ -23,7 +24,6 @@ pub enum EyedropperMsg { Move(Point), Shot(Screenshot), Pick, - Cancel, } #[derive(Default)] @@ -68,7 +68,7 @@ impl Eyedropper { for dy in -half..=half { let mut line = row![]; for dx in -half..=half { - let fill = sample(shot, cursor, dx, dy).map_or(Color::TRANSPARENT, rgb); + let fill = sample(shot, cursor, dx, dy).map_or(Color::TRANSPARENT, rgb_color); let centre = dx == 0 && dy == 0; line = line.push(container(Space::new()).width(CELL).height(CELL).style( move |_theme: &Theme| container::Style { @@ -133,10 +133,6 @@ impl Eyedropper { } } -fn rgb((r, g, b): (u8, u8, u8)) -> Color { - Color::from_rgb8(r, g, b) -} - /// The screenshot pixel `dx`, `dy` physical pixels away from a logical point. fn sample(shot: &Screenshot, at: Point, dx: i32, dy: i32) -> Option<(u8, u8, u8)> { let x = (at.x * shot.scale_factor) as i64 + i64::from(dx); diff --git a/src/ui/color_select/math.rs b/src/ui/color_select/math.rs index 4180d36d3..8e4aa7e5d 100644 --- a/src/ui/color_select/math.rs +++ b/src/ui/color_select/math.rs @@ -1,8 +1,8 @@ -//! Colour maths and text parsing for the Select Color panel. +//! Colour maths and formatting for the Select Color panel. //! //! Everything here is pure so it can be unit tested without a UI: HSB/RGB -//! conversion, hex formatting, nearest-index lookup and the parser behind the -//! panel's Color field. +//! conversion, hex formatting and parsing, nearest-index lookup and the +//! short names the panel shows. use codec::types::Color as AcadColor; diff --git a/src/ui/color_select/mod.rs b/src/ui/color_select/mod.rs index 591c1137f..ebf58586a 100644 --- a/src/ui/color_select/mod.rs +++ b/src/ui/color_select/mod.rs @@ -9,7 +9,8 @@ pub mod math; pub mod picker; pub mod widgets; -pub use picker::{ColorPicker, ColorPickerMsg, ColorPickerTab, Outcome}; +pub use math::nearest_aci; +pub use picker::{ColorPicker, ColorPickerMsg, Outcome}; use crate::app::Message; use crate::t; @@ -42,11 +43,6 @@ pub fn color_to_aci_string(c: AcadColor) -> String { } } -/// Return the closest CAD Color Index for an RGB colour. -pub fn nearest_aci(r: u8, g: u8, b: u8) -> u8 { - math::nearest_aci(r, g, b) -} - /// Decode an ACI integer string back into an `AcadColor`. pub fn aci_string_to_color(s: &str) -> AcadColor { match s.trim().parse::().unwrap_or(256) { diff --git a/src/ui/color_select/picker.rs b/src/ui/color_select/picker.rs index 090bc91fd..c1116f53a 100644 --- a/src/ui/color_select/picker.rs +++ b/src/ui/color_select/picker.rs @@ -2,7 +2,7 @@ //! //! The panel edits a pending colour and only reports it back on OK, so every //! caller keeps applying colours through the same `ColorWindowPick` path. -//! Opened for an interface colour (an Options background) it shows only the +//! Opened for an interface colour (any colour in Options) it shows only the //! colour editor: no tabs, no index colours. use super::editor::{rgb_color, ColorEditor, EditorMsg}; @@ -91,7 +91,7 @@ impl ColorPicker { self.tab } - /// Interface colours (Options backgrounds) only get the colour editor. + /// Interface colours (Options) only get the colour editor. fn editor_only(&self) -> bool { matches!(self.target, ColorPickTarget::Interface(_)) } @@ -122,9 +122,8 @@ impl ColorPicker { self.set_pending(AcadColor::Rgb { r, g, b }); } } - ColorPickerMsg::Eyedropper(EyedropperMsg::Cancel) | ColorPickerMsg::Cancel => { - self.eyedropper = None; - } + // Esc leaves the eyedropper but keeps the panel open. + ColorPickerMsg::Cancel => self.eyedropper = None, _ => {} } return None;