From 1def7587f5887f0ba0273741b2e5a6a1251dc614 Mon Sep 17 00:00:00 2001 From: mpratsaki <110930793+mpratsaki@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:16:05 +0200 Subject: [PATCH] LRC-LIB-MISSING: a catalog change during a timed refresh is pending at once A catalog change seen while a timed refresh of the Missing Photos count is running cannot start its recount until the refresh ends, and the refresh is not pending work, so settle() could return before either finished and the row the recount adds moved the folder rows afterwards (CodeRabbit on #3392). The change is now counted as pending from the frame it is seen (`owed`); the recount takes that entry over when it starts and drops it once its count is stored. settling_waits_for_a_recount_owed_during_a_timed_refresh adds a missing photo while a slow refresh runs and fails without this. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/panels/left/sections.rs | 35 ++++++++++++++++------ crates/ui-egui/src/tests/panels.rs | 24 +++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/panels/left/sections.rs b/crates/ui-egui/src/panels/left/sections.rs index 7c59cf845..9d8abcd83 100644 --- a/crates/ui-egui/src/panels/left/sections.rs +++ b/crates/ui-egui/src/panels/left/sections.rs @@ -184,7 +184,7 @@ pub fn section_drag_feedback(app: &mut LightcraftApp, ctx: &egui::Context) { /// A section's header in `r`: a disclosure chevron in the gutter (where tree rows have their /// chevron), then the bold title; a click folds or unfolds the section (kept in the UI state, so /// it survives restarts), and what the section adds to its header (its button, see `header_button`) sits at -/// the visible edge. A folded section says how many rows it holds (`rows`, with View ▸ Show Photo +/// the visible edge. A folded section says how many rows it holds (`rows`, with View β–Έ Show Photo /// Counts). `pinned`: this is the copy kept at the top of the sidebar (see `pinned_header`). fn section_header(app: &mut LightcraftApp, ui: &mut egui::Ui, section: SidebarSection, r: Rect, rows: usize, pinned: bool) -> HeaderOutcome { let t = Tokens::get(ui.ctx()); @@ -350,12 +350,12 @@ pub(super) fn sections_menu(app: &mut LightcraftApp, ui: &mut egui::Ui, on: Opti } } -/// The menu of Albums' +: Create Album, Smart Album, Folder… +/// The menu of Albums' +: Create Album, Smart Album, Folder… fn new_album_menu(app: &mut LightcraftApp, ui: &mut egui::Ui) { - if ui.button(crate::i18n::tr("Create Album…")).clicked() { + if ui.button(crate::i18n::tr("Create Album…")).clicked() { app.ui.dialog = Some(crate::state::Dialog::NewAlbum { name: String::new(), folder: false, parent: None, include_selected: true }); } - if ui.button(crate::i18n::tr("Create Smart Album…")).clicked() { + if ui.button(crate::i18n::tr("Create Smart Album…")).clicked() { app.ui.dialog = Some(crate::state::Dialog::SmartRules { id: None, name: String::new(), @@ -363,16 +363,16 @@ fn new_album_menu(app: &mut LightcraftApp, ui: &mut egui::Ui) { parent: None, }); } - if ui.button(crate::i18n::tr("Create Smart Album from Filter…")).clicked() { + if ui.button(crate::i18n::tr("Create Smart Album from Filter…")).clicked() { app.ui.dialog = Some(crate::state::Dialog::NewSmartAlbum { name: String::new(), parent: None }); } - if ui.button(crate::i18n::tr("Create Folder…")).clicked() { + if ui.button(crate::i18n::tr("Create Folder…")).clicked() { app.ui.dialog = Some(crate::state::Dialog::NewAlbum { name: String::new(), folder: true, parent: None, include_selected: false }); } // only once the albums were put in an order by hand if app.session.catalog.album_children_are_ordered(None) { ui.separator(); - if ui.button(crate::i18n::tr("Sort Albums A–Z")).on_hover_text(crate::i18n::tr("Go back to listing them by name")).clicked() { + if ui.button(crate::i18n::tr("Sort Albums A–Z")).on_hover_text(crate::i18n::tr("Go back to listing them by name")).clicked() { let _ = app.run("album.sort", json!({})); } } @@ -386,6 +386,8 @@ fn new_album_menu(app: &mut LightcraftApp, ui: &mut egui::Ui) { /// ([`super::fs_cached_running`]): the Missing Photos row they add or take away moves every row /// below it. The timed refresh is not: it rarely changes the count, and on a share where counting /// takes seconds, a headless run's fast simulated clock would start the next one before settling. +/// A catalog change during a timed refresh is pending from the frame it is seen, although its +/// recount starts only when the refresh ends. pub(super) fn missing_count(app: &mut LightcraftApp, ui: &mut egui::Ui) -> usize { use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering::AcqRel, Ordering::Relaxed}; #[derive(Clone, Default)] @@ -394,19 +396,34 @@ pub(super) fn missing_count(app: &mut LightcraftApp, ui: &mut egui::Ui) -> usize running: std::sync::Arc, at: f64, rev: u64, + /// A catalog change arrived while a count was running: its recount is pending work + /// already, held until the recount stores its result. + owed: bool, } let id = egui::Id::unique("missing-count"); let now = ui.input(|i| i.time); let rev = app.session.catalog.revision; let mut job: Job = ui.data(|d| d.get_temp(id)).unwrap_or(Job { rev: u64::MAX, at: f64::MIN, ..Default::default() }); + let running = job.running.load(Relaxed); + if running && job.rev != rev && !job.owed { + // the recount waits for the running count (a timed refresh, not waited for), but settling + // must wait for it from now on + super::local::fs_running_counter(ui.ctx()).fetch_add(1, AcqRel); + job.owed = true; + ui.data_mut(|d| d.insert_temp(id, job.clone())); + } let fresh = now - job.at < 5.0 && job.rev == rev; - if !fresh && !job.running.load(Relaxed) { + if !fresh && !running { // the same scope as the Missing Photos view: library photos only, never Local browse records let paths = lightcraft_engine::cmd::missing::candidates(&app.session.catalog); let pending = (job.rev != rev).then(|| super::local::fs_running_counter(ui.ctx())); - if let Some(p) = &pending { + if let Some(p) = &pending + && !job.owed + { p.fetch_add(1, AcqRel); } + // the recount takes over the entry held while it waited + job.owed = false; job.running.store(true, Relaxed); job.at = now; job.rev = rev; diff --git a/crates/ui-egui/src/tests/panels.rs b/crates/ui-egui/src/tests/panels.rs index bc37df9fc..194e235e4 100644 --- a/crates/ui-egui/src/tests/panels.rs +++ b/crates/ui-egui/src/tests/panels.rs @@ -1888,6 +1888,30 @@ fn settling_waits_for_the_missing_photos_count() { assert!(h.settle(Duration::from_secs(30)), "the sidebar settles while a slow count refreshes"); } +/// A catalog change while a slow timed refresh of the count runs: its recount starts only when +/// the refresh ends, but settling waits for it from the frame the change is seen, so the Missing +/// Photos row it adds is on screen once the sidebar settled. +#[test] +fn settling_waits_for_a_recount_owed_during_a_timed_refresh() { + use lightcraft_catalog::{Op, Photo, Source}; + let dir = std::env::temp_dir().join(format!("lc-ui-missing-owed-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let here = dir.join("a.jpg"); + std::fs::write(&here, b"photo").unwrap(); + // settling takes the first count (2 s), in which the simulated clock passes the 5 s refresh + // interval: a timed refresh is running when it returns + let delay = Duration::from_secs(2); + let services = Services { png: None, ..Default::default() }; + let mut h = folders_app_prepared(&[&here.to_string_lossy()], [1400.0, 900.0], services, |app| app.missing_count_test_delay = Some(delay)); + assert!(!has(&h, "source:missing"), "no photo is missing yet"); + let id = h.app.session.catalog.alloc_photo_id(); + let gone = Photo::new(id, Source::File { path: "/lc-missing-owed/gone.jpg".into() }, "gone.jpg", "JPEG", 60, 40, "2026-01-01T10:00:00"); + h.app.session.catalog.apply(Op::AddPhoto { photo: Box::new(gone) }).unwrap(); + assert!(h.settle(Duration::from_secs(30)), "the sidebar settles"); + assert!(has(&h, "source:missing"), "the Missing Photos row is on screen once the sidebar settled"); + let _ = std::fs::remove_dir_all(dir); +} + /// Right-click a folder ▸ Set Color Label ▸ a colour: the folder's row shows the label's dot (a /// row without a label shows none), and None takes it off again. #[test]