From aa7e0bbc348530bbf648003fc309e51195f89282 Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Sun, 11 Oct 2026 21:54:42 +0200 Subject: [PATCH 1/2] 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 92a53195..619967f6 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 9a387c77..749d70f3 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 a14332c8..cec20e7d 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 00000000..1d7457cd --- /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 25edf3bf..a94f613a 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 From 6797c6b483b42c3344e6a51f96d4910e6c6d7953 Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Sun, 11 Oct 2026 22:11:02 +0200 Subject: [PATCH 2/2] fix: share the sidebar's off-screen row skip, keep widths stable (#1011) Review follow-up on the keyword sidebar fix: - The skip is a shared helper, `out_of_sight`, that Keywords and Folders call before a row's work; a row out of sight still takes its room, is listed for the control channel (agents and tests find it as before) and asks for its measured width, so the content no longer widens and narrows as rows scroll by. - The scroll to a keyword made from the header is a per-frame request for any row id (`SCROLL_TO_ROW`), set before the sections are drawn and cleared after: no frame stamp, nothing left behind. - keyword_rows/folder_rows are skip-or-draw then recurse (the drawn row is keyword_row/folder_row); leaves no longer look up an open state. - keyword_tree builds children lists as levels are made and sorts by the lower-case name it already computed; empty_hint uses ROW_HEIGHT; the test reuses tests_panels::scroll_sidebar. 58,000 photos / 4,000 keywords, release: 1.6 ms per unchanged frame (7.4 ms before the fix), keyword_tree 14 ms (156 ms). Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/keywords.rs | 31 ++- crates/ui-egui/src/panels/left.rs | 218 +++++++++++--------- crates/ui-egui/src/tests_keyword_sidebar.rs | 13 +- crates/ui-egui/src/tests_panels.rs | 2 +- 4 files changed, 142 insertions(+), 122 deletions(-) diff --git a/crates/catalog/src/keywords.rs b/crates/catalog/src/keywords.rs index 619967f6..0d8b666a 100644 --- a/crates/catalog/src/keywords.rs +++ b/crates/catalog/src/keywords.rs @@ -239,16 +239,19 @@ impl Catalog { 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: `seen` is the last photo counted) + // keywords share a parent: `seen` is the last photo counted); `sort` is the name in lower case struct Level { name: String, + sort: String, path: String, count: usize, - parent: Option, seen: usize, } let mut levels: Vec = Vec::new(); let mut by_lower: HashMap = HashMap::new(); + // each level's children (by index), and the top level + let mut kids: Vec> = Vec::new(); + let mut roots: Vec = Vec::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 @@ -258,7 +261,7 @@ impl Catalog { for (photo, (counts, keywords)) in std::iter::once((false, listed.as_slice())).chain(photos).enumerate() { for k in keywords { let chain = chains.entry(k.as_str()).or_insert_with(|| { - let mut chain = Vec::new(); + let mut chain: Vec = 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()) { @@ -267,13 +270,18 @@ impl Catalog { lower.push(SEP); } path.push_str(part); - lower.push_str(&part.to_lowercase()); + let sort = part.to_lowercase(); + lower.push_str(&sort); 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 }); + match chain.last().and_then(|&p| kids.get_mut(p)) { + Some(k) => k.push(i), + None => roots.push(i), + } + levels.push(Level { name: part.to_string(), sort, path: path.clone(), count: 0, seen: 0 }); + kids.push(Vec::new()); by_lower.insert(lower.clone(), i); i } @@ -294,17 +302,8 @@ impl Catalog { } } } - // 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(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.sort_by(|&a, &b| levels.get(a).map(|l| &l.sort).cmp(&levels.get(b).map(|l| &l.sort))); list.into_iter() .filter_map(|i| { let lv = levels.get(i)?; diff --git a/crates/ui-egui/src/panels/left.rs b/crates/ui-egui/src/panels/left.rs index cec20e7d..24e1b33a 100644 --- a/crates/ui-egui/src/panels/left.rs +++ b/crates/ui-egui/src/panels/left.rs @@ -24,8 +24,9 @@ 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"; +/// Temp-data key of the row (its widget id after `source:`, lower-case) the sidebar scrolls to +/// this frame: set before the sections are drawn, cleared after. +const SCROLL_TO_ROW: &str = "left-scroll-to-row"; /// A row says how wide it needs to be. fn note_width(ui: &egui::Ui, w: f32) { @@ -82,7 +83,11 @@ fn row_sensed( 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(), ROW_HEIGHT), sense); + if scroll_asked(ui, id) { + ui.scroll_to_rect_animation(r, None, egui::style::ScrollAnimation::none()); + } register(ui.ctx(), format!("source:{id}"), r); + let mark_w = if mark.is_some() { MARK_W } else { 0.0 }; let said = spoken.unwrap_or(label); let name = match count { Some(n) => crate::i18n::tr_format!("{label}, {n} photos", label = said, n = n), @@ -114,7 +119,6 @@ fn row_sensed( let label_left = r.left() + 42.0 + indent; let count_left = count_galley.as_ref().map_or(edge - 18.0, |g| edge - 18.0 - g.size().x); // a label's dot sits just before the count - let mark_w = if mark.is_some() { MARK_W } else { 0.0 }; if let Some(c) = mark { let dot = Rect::from_center_size(pos2(count_left - MARK_W / 2.0, r.center().y), vec2(8.0, 8.0)); ui.painter().circle_filled(dot.center(), 4.0, c); @@ -128,20 +132,47 @@ fn row_sensed( let label_rect = ui.painter().text(pos2(label_left, r.center().y), Align2::LEFT_CENTER, &shown, font.clone(), color); register(ui.ctx(), format!("label:{id}"), label_rect); let resp = if shown != label { resp.on_hover_text(label) } else { resp }; - // a row asks for room for its name up to a share of a panel, so one very long name does not - // make everything scroll: depth does that - let mut needed = 42.0 + indent + full_w.min(MAX_NAME_NEED) + 18.0 + mark_w; + note_width(ui, row_need(indent, full_w, mark_w, count_galley.as_ref().map(|g| g.size().x))); if let Some(galley) = count_galley { let rect = Rect::from_min_size(pos2(edge - 18.0 - galley.size().x, r.center().y - galley.size().y / 2.0), galley.size()); - needed += rect.width() + 16.0; register(ui.ctx(), format!("count:{id}"), rect); ui.painter().galley(rect.min, galley, t.text_dim); } - note_width(ui, needed); - let _ = app; resp } +/// Whether the row `id` about to be drawn is out of sight: then it only takes its room, is listed +/// for the control channel and asks for its width (so the content does not widen and narrow as +/// rows scroll by), and the caller skips it. A library can have thousands of keywords or folders, +/// and drawing them all every frame slows the whole app down (issue #1011). A row the sidebar is +/// asked to scroll to is drawn. `label`, `count` and `mark` as for [`row_sensed`]. +fn out_of_sight(app: &LightcraftApp, ui: &mut egui::Ui, id: &str, label: &str, count: Option, indent: f32, mark: bool) -> bool { + let r = Rect::from_min_size(ui.cursor().min, vec2(ui.available_width(), ROW_HEIGHT)); + if ui.is_rect_visible(r) || scroll_asked(ui, id) { + return false; + } + ui.allocate_space(r.size()); + register(ui.ctx(), format!("source:{id}"), r); + let t = Tokens::get(ui.ctx()); + let painter = ui.painter(); + let name_w = painter.layout_no_wrap(label.to_string(), t.font(13.5), t.text_label).size().x; + let count_w = count.filter(|_| app.ui.show_counts).map(|n| painter.layout_no_wrap(n.to_string(), t.font(12.5), t.text_dim).size().x); + note_width(ui, row_need(indent, name_w, if mark { MARK_W } else { 0.0 }, count_w)); + true +} + +/// Whether the sidebar is asked to scroll to the row `id` this frame ([`SCROLL_TO_ROW`]). +fn scroll_asked(ui: &egui::Ui, id: &str) -> bool { + ui.data(|d| d.get_temp::(egui::Id::new(SCROLL_TO_ROW))).is_some_and(|want| want == id.to_lowercase()) +} + +/// How wide a row needs to be, from its indent, name, colour dot and count widths. It asks for +/// room for its name up to a share of a panel, so one very long name does not make everything +/// scroll: depth does that. +fn row_need(indent: f32, name_w: f32, mark_w: f32, count_w: Option) -> f32 { + 42.0 + indent + name_w.min(MAX_NAME_NEED) + 18.0 + mark_w + count_w.map_or(0.0, |w| w + 16.0) +} + /// The disclosure triangle at the left of a tree row (`indent` is the row's): right-pointing /// when closed, down when open, registered as widget `widget` for the control channel. Returns /// its response: the caller flips the open state on `clicked()`. @@ -272,7 +303,7 @@ fn pinned_header(app: &mut LightcraftApp, ui: &mut egui::Ui, placed: &[PlacedSec /// `sidebarEmpty:`), so the section stays where it is instead of disappearing. fn empty_hint(ui: &mut egui::Ui, section: SidebarSection) { let t = Tokens::get(ui.ctx()); - let (r, _) = ui.allocate_exact_size(vec2(ui.available_width(), 29.0), Sense::hover()); + let (r, _) = ui.allocate_exact_size(vec2(ui.available_width(), ROW_HEIGHT), Sense::hover()); register(ui.ctx(), format!("sidebarEmpty:{}", section.id()), r); let text = crate::i18n::tr(section.empty_hint()); let galley = ui.painter().layout_no_wrap(text.to_string(), t.font(12.5), t.text_dim); @@ -644,9 +675,8 @@ 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))); + // …and the sidebar scrolls to it + ui.data_mut(|d| d.insert_temp(egui::Id::new(SCROLL_TO_ROW), format!("keyword:{path}").to_lowercase())); } // Local keeps track of the folder being browsed whether or not it is drawn let local = local_now(app, ui); @@ -680,6 +710,7 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { }; placed.push(at); } + ui.data_mut(|d| d.remove::(egui::Id::new(SCROLL_TO_ROW))); // room below the last section (a section dragged to the end is dropped here) ui.add_space(SECTION_GAP * 2.0); pinned_header(app, ui, &placed, view_top); @@ -1670,44 +1701,51 @@ fn folder_rows(app: &mut LightcraftApp, ui: &mut egui::Ui, nodes: &[FolderNode], let leads_on = n.own == 0 && n.children.len() == 1; let mut open = tree_open(ui, open_id, n.volume || leads_on); // a row whose path would cover other disks' photos too only opens and closes - let selectable = n.selectable; - let sel = selectable - && app.session.source == LibrarySource::LibraryFolder - && app.session.library_folder.as_deref().is_some_and(|f| same_folder(f, &n.path)); - let name = if selectable { n.name.clone() } else { crate::i18n::tr("This Computer").to_string() }; - // the label is said with the name, as it is shown beside it - let spoken = match n.label { - Some(l) => format!("{name}, {}", crate::i18n::color_label(&app.session.catalog, l)), - None => name.clone(), - }; - let mark = n.label.map(crate::theme::label_color); + let name = if n.selectable { n.name.clone() } else { crate::i18n::tr("This Computer").to_string() }; let id = format!("libfolder:{}", n.path); - let resp = row_sensed(app, ui, &id, Icon::Folder, &name, Some(&spoken), Some(n.count), sel, indent, Sense::click(), mark); - let mut toggled = false; - if !n.children.is_empty() { - let tr = tree_toggle(ui, &resp, indent, open_id, &mut open, format!("libraryFolderToggle:{}", n.path)); - toggled = tr.clicked(); - // the triangle sits on the row and takes its clicks: the menu opens from it too - row_menu(app, &tr, n); + if !out_of_sight(app, ui, &id, &name, Some(n.count), indent, n.label.is_some()) { + folder_row(app, ui, n, &id, &name, indent, open_id, &mut open); } - let tip = if n.children.is_empty() { n.path.clone() } else { format!("{} ({})", n.path, crate::i18n::tr("includes the folders inside it")) }; - let resp = resp.on_hover_text(tip); - if resp.clicked() && !toggled { - if selectable { - // a source like an album or a Local folder: it replaces what the grid showed - let _ = app.run("library.source", json!({"kind": "libraryFolder", "path": n.path})); - } else { - open = !open; - set_tree_open(ui, open_id, open); - } - } - row_menu(app, &resp, n); if open && !n.children.is_empty() { folder_rows(app, ui, &n.children, indent + 16.0); } } } +/// One drawn row of [`folder_rows`], shown as `name`; `open_id` and `open` are its triangle's. +#[allow(clippy::too_many_arguments)] +fn folder_row(app: &mut LightcraftApp, ui: &mut egui::Ui, n: &FolderNode, id: &str, name: &str, indent: f32, open_id: egui::Id, open: &mut bool) { + let sel = n.selectable + && app.session.source == LibrarySource::LibraryFolder + && app.session.library_folder.as_deref().is_some_and(|f| same_folder(f, &n.path)); + // the label is said with the name, as it is shown beside it + let spoken = match n.label { + Some(l) => format!("{name}, {}", crate::i18n::color_label(&app.session.catalog, l)), + None => name.to_string(), + }; + let mark = n.label.map(crate::theme::label_color); + let resp = row_sensed(app, ui, id, Icon::Folder, name, Some(&spoken), Some(n.count), sel, indent, Sense::click(), mark); + let mut toggled = false; + if !n.children.is_empty() { + let tr = tree_toggle(ui, &resp, indent, open_id, open, format!("libraryFolderToggle:{}", n.path)); + toggled = tr.clicked(); + // the triangle sits on the row and takes its clicks: the menu opens from it too + row_menu(app, &tr, n); + } + let tip = if n.children.is_empty() { n.path.clone() } else { format!("{} ({})", n.path, crate::i18n::tr("includes the folders inside it")) }; + let resp = resp.on_hover_text(tip); + if resp.clicked() && !toggled { + if n.selectable { + // a source like an album or a Local folder: it replaces what the grid showed + let _ = app.run("library.source", json!({"kind": "libraryFolder", "path": n.path})); + } else { + *open = !*open; + set_tree_open(ui, open_id, *open); + } + } + row_menu(app, &resp, n); +} + /// The context menu of a Folders row: a folder's own, a disk's (remove it), none for the /// startup disk. fn row_menu(app: &mut LightcraftApp, resp: &egui::Response, n: &FolderNode) { @@ -1838,66 +1876,54 @@ 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 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; + // leaves (most keywords) have no open state to look up + let open_id = (!n.children.is_empty()).then(|| egui::Id::new(("kw-open", n.path.to_lowercase()))); + let mut open = open_id.is_some_and(|k| tree_open(ui, k, false)); + let id = format!("keyword:{}", n.path); + if !out_of_sight(app, ui, &id, &n.name, Some(n.count), indent, false) { + keyword_row(app, ui, n, &id, indent, open_id, &mut open); } - 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) }; - browse_all_photos(app, !sel); - let _ = app.run("library.filter", json!({"keyword": v})); - } - resp.context_menu(|ui| { - let has_sel = app.session.active().is_some(); - if ui.add_enabled(has_sel, egui::Button::new(crate::i18n::tr("Add to Selected Photos"))).clicked() { - let _ = app.run("photo.setMeta", json!({"addKeywords": [n.path]})); - } - if ui.add_enabled(has_sel, egui::Button::new(crate::i18n::tr("Remove from Selected Photos"))).clicked() { - let _ = app.run("photo.setMeta", json!({"removeKeywords": [n.path]})); - } - ui.separator(); - if ui.button(crate::i18n::tr("Rename Keyword…")).clicked() { - app.ui.dialog = Some(crate::state::Dialog::RenameKeyword { from: n.path.clone(), to: n.path.clone() }); - } - if ui.button(crate::i18n::tr("Merge into…")).clicked() { - app.ui.dialog = Some(crate::state::Dialog::MergeKeywords { from: vec![n.path.clone()], into: String::new() }); - } - if ui.button(crate::i18n::tr("Delete Keyword")).clicked() { - let _ = app.run("keyword.delete", json!({"keyword": n.path})); - } - }); - if open && !n.children.is_empty() { + if open { keyword_rows(app, ui, &n.children, indent + 16.0); } } } +/// One drawn row of [`keyword_rows`]; `open_id` and `open` are its triangle's when it has rows inside it. +fn keyword_row(app: &mut LightcraftApp, ui: &mut egui::Ui, n: &KeywordNode, id: &str, indent: f32, open_id: Option, open: &mut bool) { + let sel = app.session.filter.keyword.as_deref().is_some_and(|k| k.eq_ignore_ascii_case(&n.path)); + let resp = row(app, ui, id, Icon::Tag, &n.name, Some(n.count), sel, indent); + if let Some(k) = open_id { + tree_toggle(ui, &resp, indent, k, open, format!("keywordToggle:{}", n.path)); + } + 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) }; + browse_all_photos(app, !sel); + let _ = app.run("library.filter", json!({"keyword": v})); + } + resp.context_menu(|ui| { + let has_sel = app.session.active().is_some(); + if ui.add_enabled(has_sel, egui::Button::new(crate::i18n::tr("Add to Selected Photos"))).clicked() { + let _ = app.run("photo.setMeta", json!({"addKeywords": [n.path]})); + } + if ui.add_enabled(has_sel, egui::Button::new(crate::i18n::tr("Remove from Selected Photos"))).clicked() { + let _ = app.run("photo.setMeta", json!({"removeKeywords": [n.path]})); + } + ui.separator(); + if ui.button(crate::i18n::tr("Rename Keyword…")).clicked() { + app.ui.dialog = Some(crate::state::Dialog::RenameKeyword { from: n.path.clone(), to: n.path.clone() }); + } + if ui.button(crate::i18n::tr("Merge into…")).clicked() { + app.ui.dialog = Some(crate::state::Dialog::MergeKeywords { from: vec![n.path.clone()], into: String::new() }); + } + if ui.button(crate::i18n::tr("Delete Keyword")).clicked() { + let _ = app.run("keyword.delete", json!({"keyword": n.path})); + } + }); +} + #[cfg(test)] mod tests { use super::local_places; diff --git a/crates/ui-egui/src/tests_keyword_sidebar.rs b/crates/ui-egui/src/tests_keyword_sidebar.rs index 1d7457cd..95c81ff0 100644 --- a/crates/ui-egui/src/tests_keyword_sidebar.rs +++ b/crates/ui-egui/src/tests_keyword_sidebar.rs @@ -35,9 +35,10 @@ fn app(photos: u64, keywords: u64) -> Headless { h } -/// The keyword rows drawn this frame. +/// The keyword rows drawn this frame (every row is listed as `source:…`; only a drawn one has its +/// `label:…`). fn keyword_rows(h: &Headless) -> Vec { - h.app.widgets.iter().filter_map(|(w, _)| w.strip_prefix("source:keyword:")).map(str::to_string).collect() + h.app.widgets.iter().filter_map(|(w, _)| w.strip_prefix("label:keyword:")).map(str::to_string).collect() } /// Given a library with 2,000 keywords, when the sidebar's Keywords section is open, then a frame @@ -52,13 +53,7 @@ fn keywords_section_draws_only_the_rows_near_the_screen() { 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); + crate::tests_panels::scroll_sidebar(&mut h, -(KEYWORDS as f32) * 40.0); 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:?}"); diff --git a/crates/ui-egui/src/tests_panels.rs b/crates/ui-egui/src/tests_panels.rs index a94f613a..15f8fb0d 100644 --- a/crates/ui-egui/src/tests_panels.rs +++ b/crates/ui-egui/src/tests_panels.rs @@ -729,7 +729,7 @@ fn the_header_of_a_long_section_stays_in_sight() { } /// Scroll the sidebar (the pointer over it) by `dy`. -fn scroll_sidebar(h: &mut Headless, dy: f32) { +pub(crate) fn scroll_sidebar(h: &mut Headless, dy: f32) { h.request("ui.move", json!({"x": 120.0, "y": 400.0}), T); let r = h.request("ui.scroll", json!({"dy": dy}), T); assert_eq!(r["ok"], true, "{r}");