From aa7e0bbc348530bbf648003fc309e51195f89282 Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Sun, 11 Oct 2026 21:54:42 +0200 Subject: [PATCH] fix: left panel slow with a large keyword list (#1011) Cause: the sidebar's Keywords section drew every keyword row on every frame (layout, text measuring, widget registration, hover text, context menu), even the thousands out of sight, and every catalog edit rebuilt the keyword tree from all photos with several String allocations per keyword level. On a 58,000-photo library with 4,000 keywords an unchanged frame took 7.4 ms in the sidebar alone and the frame after a rating 220 ms (156 ms of it the tree), release build on Apple Silicon. Fix: - Keywords rows out of sight only take their height (open children are still walked, so the layout is unchanged), as the grid already does (#35). A keyword made from the header's + is always drawn and the sidebar scrolls to it, so it is in sight even at the end of a long list. - Catalog::keyword_tree works out each distinct keyword string's levels once and then only counts (a per-level "last photo" stamp replaces the per-photo HashSet of Strings). Now: 0.55 ms per unchanged frame (7 rows drawn instead of 4,000), keyword_tree 17 ms, the frame after a rating 44 ms. Test: tests_keyword_sidebar::keywords_section_draws_only_the_rows_near_ the_screen (failed before: "2000 of 2000 keyword rows drawn"; now the rows near the screen, and scrolling reaches the last keyword), an ignored timing probe at the report's library size, a catalog test for a keyword repeated on one photo, and the_keywords_header_creates_a_keyword now checks the new keyword is in sight. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/keywords.rs | 105 +++++++++++++------- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/panels/left.rs | 34 ++++++- crates/ui-egui/src/tests_keyword_sidebar.rs | 92 +++++++++++++++++ crates/ui-egui/src/tests_panels.rs | 4 + 5 files changed, 198 insertions(+), 39 deletions(-) create mode 100644 crates/ui-egui/src/tests_keyword_sidebar.rs diff --git a/crates/catalog/src/keywords.rs b/crates/catalog/src/keywords.rs index 92a531951..619967f65 100644 --- a/crates/catalog/src/keywords.rs +++ b/crates/catalog/src/keywords.rs @@ -236,63 +236,85 @@ pub struct KeywordNode { impl Catalog { /// The keyword tree: every keyword level with photo counts, sorted by name (case-insensitive). pub fn keyword_tree(&self) -> Vec { - use std::collections::{HashMap, HashSet}; - // every keyword level (`travel`, `travel|italy`…) by its lower-case path: the name and + use std::collections::HashMap; + // every keyword level (`travel`, `travel|italy`…), one per lower-case path: the name and // path as first written, and the photos counted (once per photo, even when two of its - // keywords share a parent) + // keywords share a parent: `seen` is the last photo counted) struct Level { name: String, path: String, count: usize, - parent: Option, + parent: Option, + seen: usize, } - let mut levels: HashMap = HashMap::new(); - let mut this_photo: HashSet = HashSet::new(); + let mut levels: Vec = Vec::new(); + let mut by_lower: HashMap = HashMap::new(); // the keyword list first (no photos counted): a listed keyword is there without photos let listed: Vec = self.listed_keywords().map(|k| k.path.clone()).collect(); + // each keyword as written → its levels (its own and those above it), worked out once: a + // large library repeats a few thousand keywords tens of thousands of times (issue #1011) + let mut chains: HashMap<&str, Vec> = HashMap::new(); let photos = self.photos().filter(|p| p.in_library()).map(|p| (true, p.meta.keywords.as_slice())); - for (counts, keywords) in std::iter::once((false, listed.as_slice())).chain(photos) { - this_photo.clear(); + for (photo, (counts, keywords)) in std::iter::once((false, listed.as_slice())).chain(photos).enumerate() { for k in keywords { - let mut path = String::new(); - let mut lower = String::new(); - let mut parent: Option = None; - for part in k.split(SEP).map(str::trim).filter(|s| !s.is_empty()) { - if !path.is_empty() { - path.push(SEP); - lower.push(SEP); + let chain = chains.entry(k.as_str()).or_insert_with(|| { + let mut chain = Vec::new(); + let mut path = String::new(); + let mut lower = String::new(); + for part in k.split(SEP).map(str::trim).filter(|s| !s.is_empty()) { + if !path.is_empty() { + path.push(SEP); + lower.push(SEP); + } + path.push_str(part); + lower.push_str(&part.to_lowercase()); + let i = match by_lower.get(&lower) { + Some(&i) => i, + None => { + let i = levels.len(); + let parent = chain.last().copied(); + levels.push(Level { name: part.to_string(), path: path.clone(), count: 0, parent, seen: 0 }); + by_lower.insert(lower.clone(), i); + i + } + }; + chain.push(i); } - path.push_str(part); - lower.push_str(&part.to_lowercase()); - let l = levels.entry(lower.clone()).or_insert_with(|| Level { - name: part.to_string(), - path: path.clone(), - count: 0, - parent: parent.clone(), - }); - if counts && this_photo.insert(lower.clone()) { - l.count += 1; + chain + }); + if counts { + for &i in chain.iter() { + let Some(l) = levels.get_mut(i) else { continue }; + // photo 0 is the keyword list, which counts nothing + if l.seen != photo { + l.seen = photo; + l.count += 1; + } } - parent = Some(lower.clone()); } } } - let mut kids: HashMap, Vec> = HashMap::new(); - for (lower, l) in &levels { - kids.entry(l.parent.clone()).or_default().push(lower.clone()); + // each level's children; `roots` the top level + let mut kids: Vec> = vec![Vec::new(); levels.len()]; + let mut roots = Vec::new(); + for (i, l) in levels.iter().enumerate() { + match l.parent.and_then(|p| kids.get_mut(p)) { + Some(k) => k.push(i), + None => roots.push(i), + } } - fn build(at: Option, levels: &HashMap, kids: &mut HashMap, Vec>) -> Vec { - let Some(mut list) = kids.remove(&at) else { return Vec::new() }; - list.sort_by_key(|l| levels[l].name.to_lowercase()); + fn build(mut list: Vec, levels: &[Level], kids: &mut [Vec]) -> Vec { + list.sort_by_cached_key(|&i| levels.get(i).map(|l| l.name.to_lowercase())); list.into_iter() - .map(|l| { - let lv = &levels[&l]; - let children = build(Some(l.clone()), levels, kids); - KeywordNode { name: lv.name.clone(), path: lv.path.clone(), count: lv.count, children } + .filter_map(|i| { + let lv = levels.get(i)?; + let below = kids.get_mut(i).map(std::mem::take).unwrap_or_default(); + let children = build(below, levels, kids); + Some(KeywordNode { name: lv.name.clone(), path: lv.path.clone(), count: lv.count, children }) }) .collect() } - build(None, &levels, &mut kids) + build(roots, &levels, &mut kids) } /// `SetMeta` ops for every photo whose keywords `f` changes. @@ -804,6 +826,15 @@ mod tests { assert_eq!(c.query(&f, &Default::default()).len(), 3); } + /// A keyword written twice on a photo, or in two spellings, counts the photo once; the next + /// photo with it counts again. + #[test] + fn tree_counts_a_repeated_keyword_once_per_photo() { + let (c, _) = lib(&[&["sea|Baltic", "sea|Baltic", "Sea|baltic"], &["sea|baltic"], &[]]); + let t = c.keyword_tree(); + assert_eq!(tree_paths(&t), [("sea".to_string(), 2), ("sea|Baltic".to_string(), 2)]); + } + #[test] fn rename_delete_merge_are_single_undoable_batches() { let (mut c, ids) = lib(&[&["travel|italy|rome", "beach"], &["Travel|Italy"], &["italia", "travel|italy"], &["sea"]]); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 9a387c77c..749d70f3b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -80,6 +80,8 @@ mod tests_keyword_list; #[cfg(test)] mod tests_keyword_set; #[cfg(test)] +mod tests_keyword_sidebar; +#[cfg(test)] mod tests_keywording; #[cfg(test)] mod tests_labels; diff --git a/crates/ui-egui/src/panels/left.rs b/crates/ui-egui/src/panels/left.rs index a14332c82..cec20e7df 100644 --- a/crates/ui-egui/src/panels/left.rs +++ b/crates/ui-egui/src/panels/left.rs @@ -21,6 +21,12 @@ pub(crate) fn content_width(ctx: &egui::Context) -> f32 { /// The most a row asks for its name: longer ones are cut when there is no more room. const MAX_NAME_NEED: f32 = 140.0; +/// Height of a sidebar row. +const ROW_HEIGHT: f32 = 29.0; + +/// Temp-data key of the keyword (lower-case path) the sidebar scrolls to, and the frame it was asked for. +const KEYWORD_SCROLL_TO: &str = "left-keyword-scroll-to"; + /// A row says how wide it needs to be. fn note_width(ui: &egui::Ui, w: f32) { ui.data_mut(|d| { @@ -75,7 +81,7 @@ fn row_sensed( ) -> egui::Response { let label = if matches!(id, "all" | "recentlyAdded" | "picks" | "missing" | "recentlyDeleted") { crate::i18n::tr(label) } else { label }; let t = Tokens::get(ui.ctx()); - let (r, resp) = ui.allocate_exact_size(vec2(ui.available_width(), 29.0), sense); + let (r, resp) = ui.allocate_exact_size(vec2(ui.available_width(), ROW_HEIGHT), sense); register(ui.ctx(), format!("source:{id}"), r); let said = spoken.unwrap_or(label); let name = match count { @@ -638,6 +644,9 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { above.push_str(name); set_tree_open(ui, egui::Id::new(("kw-open", above.to_lowercase())), true); } + // …and the sidebar scrolls to it when its row is drawn (`keyword_rows`) + let frame = ui.ctx().cumulative_frame_nr(); + ui.data_mut(|d| d.insert_temp(egui::Id::new(KEYWORD_SCROLL_TO), (path.to_lowercase(), frame))); } // Local keeps track of the folder being browsed whether or not it is drawn let local = local_now(app, ui); @@ -1829,14 +1838,35 @@ fn folder_menu_for_library(app: &mut LightcraftApp, resp: &egui::Response, n: &F /// filters the grid by the keyword (children included), the triangle opens a level, and the /// context menu renames, merges or deletes the keyword across the library. fn keyword_rows(app: &mut LightcraftApp, ui: &mut egui::Ui, nodes: &[KeywordNode], indent: f32) { + // the keyword just made from the header, to scroll to (asked for a frame or two ago) + let scroll_id = egui::Id::new(KEYWORD_SCROLL_TO); + let frame = ui.ctx().cumulative_frame_nr(); + let scroll_to = ui.data(|d| d.get_temp::<(String, u64)>(scroll_id)).filter(|(_, asked)| frame.saturating_sub(*asked) <= 2).map(|(p, _)| p); for n in nodes { - let open_id = egui::Id::new(("kw-open", n.path.to_lowercase())); + let lower = n.path.to_lowercase(); + let reveal = scroll_to.as_deref() == Some(lower.as_str()); + let open_id = egui::Id::new(("kw-open", lower)); let mut open = tree_open(ui, open_id, false); + // a row out of sight only takes its room: a library can have thousands of keywords, and + // drawing them all every frame slows the whole app down (issue #1011) + let next = Rect::from_min_size(ui.cursor().min, vec2(ui.available_width(), ROW_HEIGHT)); + if !reveal && !ui.is_rect_visible(next) { + ui.allocate_space(next.size()); + note_width(ui, 42.0 + indent + 18.0); + if open && !n.children.is_empty() { + keyword_rows(app, ui, &n.children, indent + 16.0); + } + continue; + } let sel = app.session.filter.keyword.as_deref().is_some_and(|k| k.eq_ignore_ascii_case(&n.path)); let resp = row(app, ui, &format!("keyword:{}", n.path), Icon::Tag, &n.name, Some(n.count), sel, indent); if !n.children.is_empty() { tree_toggle(ui, &resp, indent, open_id, &mut open, format!("keywordToggle:{}", n.path)); } + if reveal { + ui.scroll_to_rect_animation(resp.rect, None, egui::style::ScrollAnimation::none()); + ui.data_mut(|d| d.remove::<(String, u64)>(scroll_id)); + } let resp = resp.on_hover_text(if n.children.is_empty() { n.path.clone() } else { format!("{} (includes the keywords below it)", n.path) }); if resp.clicked() { let v = if sel { serde_json::Value::Null } else { json!(n.path) }; diff --git a/crates/ui-egui/src/tests_keyword_sidebar.rs b/crates/ui-egui/src/tests_keyword_sidebar.rs new file mode 100644 index 000000000..1d7457cd6 --- /dev/null +++ b/crates/ui-egui/src/tests_keyword_sidebar.rs @@ -0,0 +1,92 @@ +//! Headless tests of the left sidebar's Keywords section on a large library (issue #1011): with +//! thousands of keywords listed, a frame draws only the rows near the screen, and the rows out +//! of sight still take their room, so scrolling reaches the last keyword. + +use std::time::{Duration, Instant}; + +use lightcraft_catalog::{Op, Photo, PhotoId, Source}; +use lightcraft_engine::Session; +use serde_json::json; + +use crate::headless::Headless; +use crate::{LightcraftApp, Services}; + +const T: Duration = Duration::from_secs(30); +const SETTLE: Duration = Duration::from_secs(120); + +/// `photos` photos with six keywords each out of `keywords` top-level ones (each with a child). +fn library(photos: u64, keywords: u64) -> Session { + let mut s = Session::new(); + for i in 0..photos { + let path = format!("/lightcraft-kw-test/IMG_{i:05}.jpg"); + let mut p = Photo::new(PhotoId(i + 1), Source::File { path }, &format!("IMG_{i:05}.jpg"), "JPEG", 60, 40, "2026-01-01T00:00:00"); + p.meta.keywords = (0..6).map(|k| format!("Keyword {:05}|Child", (i * 6 + k) % keywords)).collect(); + s.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + } + s +} + +fn app(photos: u64, keywords: u64) -> Headless { + let app = LightcraftApp::new(library(photos, keywords), Services { png: None, ..Default::default() }); + let mut h = Headless::new(app, [1400.0, 900.0], 1.0); + let r = h.request("ui.set", json!({"view": "photoGrid", "leftPanel": true}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + h +} + +/// The keyword rows drawn this frame. +fn keyword_rows(h: &Headless) -> Vec { + h.app.widgets.iter().filter_map(|(w, _)| w.strip_prefix("source:keyword:")).map(str::to_string).collect() +} + +/// Given a library with 2,000 keywords, when the sidebar's Keywords section is open, then a frame +/// draws only the few dozen rows that fit on screen (not all 2,000), and scrolling to the end +/// draws the last keyword: the rows out of sight keep their height. +#[test] +fn keywords_section_draws_only_the_rows_near_the_screen() { + const KEYWORDS: u64 = 2000; + let mut h = app(KEYWORDS / 6 + 1, KEYWORDS); + assert_eq!(h.app.caches.keyword_tree(&h.app.session.catalog).len(), KEYWORDS as usize); + let rows = keyword_rows(&h); + assert!(!rows.is_empty() && rows.len() < 100, "{} of {KEYWORDS} keyword rows drawn", rows.len()); + assert!(rows.contains(&"Keyword 00000".to_string()), "{rows:?}"); + // all the way down + h.request("ui.move", json!({"x": 120.0, "y": 400.0}), T); + for _ in 0..40 { + let r = h.request("ui.scroll", json!({"dy": -5000.0}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + } + h.settle(SETTLE); + let rows = keyword_rows(&h); + assert!(!rows.is_empty() && rows.len() < 100, "{} of {KEYWORDS} keyword rows drawn", rows.len()); + assert!(rows.contains(&"Keyword 01999".to_string()), "the last keyword is reached: {rows:?}"); + assert!(!rows.contains(&"Keyword 00000".to_string()), "{rows:?}"); +} + +/// Timings on a library the size of the report's (58,000 photos): `cargo test --release -p +/// lightcraft-ui-egui --lib probe_keyword_sidebar_cost -- --ignored --nocapture`. +#[test] +#[ignore = "timing probe"] +fn probe_keyword_sidebar_cost() { + let mut h = app(58_000, 4000); + let t = Instant::now(); + let tree = h.app.session.catalog.keyword_tree(); + eprintln!("keyword_tree: {:?} ({} top level)", t.elapsed(), tree.len()); + eprintln!("keyword rows drawn: {}", keyword_rows(&h).len()); + let t = Instant::now(); + for _ in 0..10 { + h.step(); + } + eprintln!("unchanged frame: {:?}", t.elapsed() / 10); + let t = Instant::now(); + for i in 0..10u64 { + let r = h.request("engine.execute", json!({"command": "library.select", "params": {"ids": [i + 1]}}), T); + assert_eq!(r["ok"], true, "{r}"); + let r = h.request("engine.execute", json!({"command": "photo.rate", "params": {"rating": (i % 5) + 1}}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + } + eprintln!("frame after a rating: {:?}", t.elapsed() / 10); +} diff --git a/crates/ui-egui/src/tests_panels.rs b/crates/ui-egui/src/tests_panels.rs index 25edf3bfa..a94f613a5 100644 --- a/crates/ui-egui/src/tests_panels.rs +++ b/crates/ui-egui/src/tests_panels.rs @@ -889,6 +889,10 @@ fn the_keywords_header_creates_a_keyword() { h.step(); h.step(); assert!(has(&h, "source:keyword:zebra crossing"), "the new keyword is listed"); + // the sidebar scrolls to it: it is listed last, below what the panel shows + h.settle(SETTLE); + let (row, panel) = (widget(&h, "source:keyword:zebra crossing"), widget(&h, "panel:left_panel")); + assert!(panel.contains_rect(row), "the new keyword is in sight: {row:?} in {panel:?}"); } /// A folded section's row count sits to the left of its header button; a section without a button