From 345f836e9150afc3de7909d8273e57ca230a0ae1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 19:27:58 +0000 Subject: [PATCH] Select Color: review cleanups - Arrow keys only walk the colours on screen: the Document tab's search results, and the Color books matches while searching (the whole book, page after page, otherwise). One helper per tab now gives the visible colours to both the view and the arrow keys. - show_identity no longer returns an unused bool; ink() absorbs its one-use ink_is_dark helper. - document::layer_identity joins a layer's book and colour name, used by the Document scan and the panel's current colour instead of two copies. - Recently-used sorting looks a colour's rank up once (sort_by_cached_key) instead of on every comparison. - Searching compares case-insensitively without allocating a lower-case copy of every name on each redraw (ASCII queries). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013rGmDUNr1hUf4zo1X3dak8 --- src/app/update/mod.rs | 11 +-- src/ui/color_select/document.rs | 38 +++++----- src/ui/color_select/picker.rs | 127 ++++++++++++++++++++------------ 3 files changed, 101 insertions(+), 75 deletions(-) diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 27f2237b7..eee1f3f01 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -12081,13 +12081,10 @@ impl OpenCADStudio { return (color, false, self.ribbon.active_color_name()); } crate::app::ColorPickTarget::Layer(layer) => { - let name = document.layers.get(layer).and_then(|layer| { - match (&layer.book_name, &layer.color_name) { - (Some(book), Some(name)) => Some(format!("{book}${name}")), - (None, Some(name)) => Some(name.clone()), - _ => None, - } - }); + let name = document + .layers + .get(layer) + .and_then(crate::ui::color_select::document::layer_identity); return (color, false, name); } _ => return (color, false, None), diff --git a/src/ui/color_select/document.rs b/src/ui/color_select/document.rs index c257f9c1b..df024e6a4 100644 --- a/src/ui/color_select/document.rs +++ b/src/ui/color_select/document.rs @@ -89,19 +89,20 @@ fn key(color: AcadColor) -> Option { } } +/// A layer's colour-book identity, `BOOK$NAME`. Layers keep the book and +/// the colour name apart; objects store them joined like this. +pub fn layer_identity(layer: &codec::tables::Layer) -> Option { + match (&layer.book_name, &layer.color_name) { + (Some(book), Some(name)) => Some(format!("{book}${name}")), + (None, Some(name)) => Some(name.clone()), + _ => None, + } +} + /// Every concrete colour in the drawing. ByLayer, ByBlock and None are /// states rather than colours and are skipped. pub fn collect(document: &codec::CadDocument) -> Vec { - // Layers keep book and colour name apart; join them like objects do. - let layer_names: Vec> = document - .layers - .iter() - .map(|layer| match (&layer.book_name, &layer.color_name) { - (Some(book), Some(name)) => Some(format!("{book}${name}")), - (None, Some(name)) => Some(name.clone()), - _ => None, - }) - .collect(); + let layer_names: Vec> = document.layers.iter().map(layer_identity).collect(); let objects = document.entities().map(|entity| { let common = entity.common(); (common.color, common.color_name.as_deref(), common.handle) @@ -139,16 +140,15 @@ fn tally<'a>(uses: impl Iterator, Handle)>) - /// Sort for display. `recent` is this drawing's pick history, newest first; /// colours not picked this session follow, newest object first. pub fn sort(colors: &mut [DocColor], by: DocSort, recent: &[AcadColor]) { - let rank = |c: &DocColor| { - recent - .iter() - .position(|&r| r == c.color) - .unwrap_or(usize::MAX) - }; match by { - DocSort::Recent => { - colors.sort_by(|a, b| rank(a).cmp(&rank(b)).then(b.newest.cmp(&a.newest))) - } + // The rank is looked up once per colour, not on every comparison. + DocSort::Recent => colors.sort_by_cached_key(|c| { + let rank = recent + .iter() + .position(|&r| r == c.color) + .unwrap_or(usize::MAX); + (rank, std::cmp::Reverse(c.newest)) + }), DocSort::Usage => colors.sort_by(|a, b| b.uses.cmp(&a.uses).then(b.newest.cmp(&a.newest))), DocSort::Hue => colors.sort_by_cached_key(|c| { let (r, g, b) = math::color_rgb(c.color).unwrap_or_default(); diff --git a/src/ui/color_select/picker.rs b/src/ui/color_select/picker.rs index f2421bfb7..40f003ef4 100644 --- a/src/ui/color_select/picker.rs +++ b/src/ui/color_select/picker.rs @@ -180,19 +180,43 @@ impl ColorPicker { self.status = Some(status); } - /// Turn to the page holding `BOOK$NAME`. False when no loaded book has it. - fn show_identity(&mut self, identity: &str) -> bool { + /// Turn to the page holding `BOOK$NAME`, if a loaded book has it. + fn show_identity(&mut self, identity: &str) { let Some((book_name, name)) = identity.split_once('$') else { - return false; + return; }; for (i, book) in self.books.iter().enumerate() { if let Some(color) = book.find(name).filter(|_| book.name == book_name) { self.book = i; self.book_page = book.page_of(color); - return true; + return; } } - false + } + + /// The Document tab's colours that match the search, by index in `doc`. + fn doc_visible(&self) -> Vec { + let query = self.doc_query.trim().to_lowercase(); + (0..self.doc.len()) + .filter(|&i| query.is_empty() || contains_ignore_case(&self.doc[i].label(), &query)) + .collect() + } + + /// The shown book's colours on screen, by index in the book: the page, + /// or the first matches while searching. + fn book_visible(&self) -> Vec { + let Some(book) = self.books.get(self.book) else { + return Vec::new(); + }; + let query = self.book_query.trim().to_lowercase(); + if query.is_empty() { + let (start, colors) = book.page(self.book_page); + return (start..start + colors.len()).collect(); + } + (0..book.colors.len()) + .filter(|&i| contains_ignore_case(&book.colors[i].name, &query)) + .take(PAGE_BARS) + .collect() } /// The colour of the shown book that is the new colour, if any. @@ -275,22 +299,32 @@ impl ColorPicker { self.set_pending(AcadColor::Index(next)); } } + // Only over the colours on screen, so a search is respected. (ColorPickerTab::Document, _) => { - let at = self - .doc - .iter() - .position(|c| c.color == self.pending && c.name == self.pending_name); - if let Some(next) = step_list(at, self.doc.len(), DOC_COLUMNS, direction) { - self.update(ColorPickerMsg::PickDocument(next)); + let visible = self.doc_visible(); + let at = visible.iter().position(|&i| { + self.doc[i].color == self.pending && self.doc[i].name == self.pending_name + }); + if let Some(next) = step_list(at, visible.len(), DOC_COLUMNS, direction) { + self.update(ColorPickerMsg::PickDocument(visible[next])); } } (ColorPickerTab::Books, _) => { - let len = self - .books - .get(self.book) - .map_or(0, |book| book.colors.len()); - if let Some(next) = step_list(self.book_selection(), len, 1, direction) { - self.pick_book(next); + // Without a search the arrows walk the whole book, page + // after page. + let visible: Vec = if self.book_query.trim().is_empty() { + (0..self + .books + .get(self.book) + .map_or(0, |book| book.colors.len())) + .collect() + } else { + self.book_visible() + }; + let selected = self.book_selection(); + let at = visible.iter().position(|&i| Some(i) == selected); + if let Some(next) = step_list(at, visible.len(), 1, direction) { + self.pick_book(visible[next]); } } _ => {} @@ -627,12 +661,10 @@ impl ColorPicker { .into(); } // Indices stay those of `doc`, which the pick messages refer to. - let query = self.doc_query.trim().to_lowercase(); let shown: Vec<(usize, &DocColor)> = self - .doc - .iter() - .enumerate() - .filter(|(_, entry)| query.is_empty() || entry.label().to_lowercase().contains(&query)) + .doc_visible() + .into_iter() + .map(|i| (i, &self.doc[i])) .collect(); if shown.is_empty() { return column![header, empty_note(t!("No colors match").into_owned())] @@ -787,30 +819,16 @@ impl ColorPicker { // The page's colours, or the first matches while searching. let selected = self.book_selection(); - let query = self.book_query.trim().to_lowercase(); - let mut bars = column![].spacing(2).height(Length::Fill); - let mut shown = 0; - if query.is_empty() { - let (start, colors) = book.page(self.book_page); - for (i, color) in colors.iter().enumerate() { - bars = bars.push(book_bar(start + i, color, selected)); - shown += 1; - } - } else { - for (i, color) in book - .colors - .iter() - .enumerate() - .filter(|(_, color)| color.name.to_lowercase().contains(&query)) - .take(PAGE_BARS) - { - bars = bars.push(book_bar(i, color, selected)); - shown += 1; - } - } - let bars: Element<'_, Message> = if shown == 0 { + let searching = !self.book_query.trim().is_empty(); + let visible = self.book_visible(); + let bars: Element<'_, Message> = if visible.is_empty() { empty_note(t!("No colors match").into_owned()) } else { + let bars = visible + .iter() + .fold(column![].spacing(2).height(Length::Fill), |bars, &i| { + bars.push(book_bar(i, &book.colors[i], selected)) + }); mouse_area(bars) .on_scroll(|delta| { let (iced::mouse::ScrollDelta::Lines { y, .. } @@ -824,7 +842,7 @@ impl ColorPicker { let strip = (0..book.page_count()).fold(column![], |strip, page| { let (_, colors) = book.page(page); let fill = rgb_color(colors[colors.len() / 2].rgb); - let shown = query.is_empty() && page == self.book_page; + let shown = !searching && page == self.book_page; strip.push( button(Space::new()) .on_press(picker_msg(ColorPickerMsg::BookPage(page))) @@ -1054,14 +1072,25 @@ fn picker_msg(msg: ColorPickerMsg) -> Message { Message::ColorPicker(msg) } -fn ink_is_dark(color: Color) -> bool { - let [r, g, b, _] = color.into_rgba8(); - math::ink_is_dark(r, g, b) +/// Whether `haystack` contains `needle`, ignoring case; `needle` is already +/// lower case. ASCII needles, the usual case, compare without allocating. +fn contains_ignore_case(haystack: &str, needle: &str) -> bool { + if needle.is_empty() { + true + } else if needle.is_ascii() { + haystack + .as_bytes() + .windows(needle.len().max(1)) + .any(|window| window.eq_ignore_ascii_case(needle.as_bytes())) + } else { + haystack.to_lowercase().contains(needle) + } } /// Text that reads on top of `fill`. fn ink(fill: Color) -> Color { - if ink_is_dark(fill) { + let [r, g, b, _] = fill.into_rgba8(); + if math::ink_is_dark(r, g, b) { Color::from_rgb8(0x11, 0x12, 0x14) } else { Color::WHITE