From 82d46b14cdfcbbd24bf15291844a6155d27ddd88 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 18:18:33 -0400 Subject: [PATCH] Synchronize Folder: progress counts the photo checks too (review fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dialog's "Scanning… n / m" counted only the files probed, so it could sit at m / m while the library's photos were still being checked. SyncProgress counts both halves (its `files.cancel` still stops the whole scan). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/sync.rs | 44 ++++++++++++++++++++++++++------- crates/engine/src/tests_sync.rs | 17 +++++++++++-- crates/ui-egui/src/sync.rs | 11 ++++----- 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/sync.rs b/crates/engine/src/sync.rs index 92b60aeab..596d10958 100644 --- a/crates/engine/src/sync.rs +++ b/crates/engine/src/sync.rs @@ -39,6 +39,24 @@ use crate::{EngineError, Result, Session}; /// The largest XMP sidecar read when looking for metadata updates; a bigger one is skipped. const MAX_SIDECAR_BYTES: u64 = 16 * 1024 * 1024; +/// How far a scan is: the folder's files probed ([`ScanProgress`], whose `cancel` stops the +/// whole scan) and the library's photos checked. +#[derive(Default)] +pub struct SyncProgress { + pub files: ScanProgress, + pub checked: std::sync::atomic::AtomicUsize, + pub to_check: std::sync::atomic::AtomicUsize, +} + +impl SyncProgress { + /// (done, of how many), both halves together. + pub fn counts(&self) -> (usize, usize) { + use std::sync::atomic::Ordering::Relaxed; + let done = self.files.done.load(Relaxed).saturating_add(self.checked.load(Relaxed)); + (done, self.files.total.load(Relaxed).saturating_add(self.to_check.load(Relaxed))) + } +} + /// What a scan of a folder needs, taken from the session so it can run on a worker thread. pub struct SyncInput { folder: String, @@ -155,8 +173,9 @@ impl SyncInput { /// /// Both halves wait on the disk (a network share answers each file slowly), so they run at once: /// the look for new files (which probes them on its own threads) beside the checks of the -/// library's photos, which run on several threads too. Stops early when `progress.cancel` is set. -pub fn scan_with(input: SyncInput, progress: &ScanProgress) -> FolderChanges { +/// library's photos, which run on several threads too. Stops early when `progress.files.cancel` +/// is set. +pub fn scan_with(input: SyncInput, progress: &SyncProgress) -> FolderChanges { let SyncInput { folder, scan, photos, labels } = input; let seen: Vec> = photos.iter().map(|(p, _)| Arc::clone(p)).collect(); if !cfg!(target_arch = "wasm32") && !Path::new(&folder).is_dir() { @@ -164,13 +183,13 @@ pub fn scan_with(input: SyncInput, progress: &ScanProgress) -> FolderChanges { } // (no threads on the web: one after the other there) #[cfg(target_arch = "wasm32")] - let (out, checks) = (crate::import::scan_with(scan, std::slice::from_ref(&folder), progress), check_photos(&photos, &labels, progress)); + let (out, checks) = (crate::import::scan_with(scan, std::slice::from_ref(&folder), &progress.files), check_photos(&photos, &labels, progress)); #[cfg(not(target_arch = "wasm32"))] let (out, checks) = std::thread::scope(|sc| { let photos = &photos; let labels = &labels; let checks = sc.spawn(move || check_photos(photos, labels, progress)); - let out = crate::import::scan_with(scan, std::slice::from_ref(&folder), progress); + let out = crate::import::scan_with(scan, std::slice::from_ref(&folder), &progress.files); // (a check that panicked counts as nothing found; the scan still reports the rest) (out, checks.join().unwrap_or_default()) }); @@ -245,8 +264,15 @@ fn check_photo(p: &Photo, naming: SidecarNaming, labels: &Catalog) -> Check { /// [`check_photo`] for each photo, in order, on several threads (each file is a wait on the /// disk, not work for the CPU). Photos not reached before a cancel count as nothing found. -fn check_photos(photos: &[(Arc, SidecarNaming)], labels: &Catalog, progress: &ScanProgress) -> Vec { +fn check_photos(photos: &[(Arc, SidecarNaming)], labels: &Catalog, progress: &SyncProgress) -> Vec { use std::sync::atomic::Ordering::Relaxed; + progress.to_check.store(photos.len(), Relaxed); + let cancel = &progress.files.cancel; + let check = |p: &Photo, naming: SidecarNaming| { + let c = check_photo(p, naming, labels); + progress.checked.fetch_add(1, Relaxed); + c + }; if cfg!(target_arch = "wasm32") { return vec![Check::Nothing; photos.len()]; } @@ -259,10 +285,10 @@ fn check_photos(photos: &[(Arc, SidecarNaming)], labels: &Catalog, progre std::thread::scope(|sc| { for _ in 0..n { sc.spawn(|| { - while !progress.cancel.load(Relaxed) { + while !cancel.load(Relaxed) { let i = next.fetch_add(1, Relaxed); let Some((p, naming)) = photos.get(i) else { break }; - let c = check_photo(p, *naming, labels); + let c = check(p, *naming); if let Some(slot) = found.lock().unwrap_or_else(std::sync::PoisonError::into_inner).get_mut(i) { *slot = c; } @@ -273,13 +299,13 @@ fn check_photos(photos: &[(Arc, SidecarNaming)], labels: &Catalog, progre return found.into_inner().unwrap_or_else(std::sync::PoisonError::into_inner); } } - photos.iter().map(|(p, naming)| if progress.cancel.load(Relaxed) { Check::Nothing } else { check_photo(p, *naming, labels) }).collect() + photos.iter().map(|(p, naming)| if cancel.load(Relaxed) { Check::Nothing } else { check(p, *naming) }).collect() } /// [`scan_with`] on the calling thread. pub fn scan(s: &mut Session, path: &str, disk: bool) -> Result { let input = SyncInput::new(s, path, disk)?; - let changes = scan_with(input, &ScanProgress::default()); + let changes = scan_with(input, &SyncProgress::default()); if changes.offline { return Err(not_there(&changes.path)); } diff --git a/crates/engine/src/tests_sync.rs b/crates/engine/src/tests_sync.rs index 583e448d2..2d5bcc915 100644 --- a/crates/engine/src/tests_sync.rs +++ b/crates/engine/src/tests_sync.rs @@ -244,8 +244,8 @@ fn a_cancelled_scan_stops_checking_files() { let mut s = library(&dir); std::fs::remove_file(dir.path("trip/day1/b.png")).unwrap(); let input = crate::sync::SyncInput::new(&mut s, &dir.path("trip"), false).unwrap(); - let progress = crate::import::ScanProgress::default(); - progress.cancel.store(true, std::sync::atomic::Ordering::Relaxed); + let progress = crate::sync::SyncProgress::default(); + progress.files.cancel.store(true, std::sync::atomic::Ordering::Relaxed); let c = crate::sync::scan_with(input, &progress); assert!(c.missing.is_empty() && c.metadata.is_empty(), "{c:?}"); } @@ -393,3 +393,16 @@ fn a_change_to_the_folders_photos_makes_the_scan_stale() { let r = s.execute("folder.synchronize", &json!({"path": dir.path("trip")})).unwrap(); assert_eq!(r["imported"], 1, "{r}"); } + +#[test] +fn the_progress_counts_the_photos_checked_too() { + use std::sync::atomic::Ordering::Relaxed; + let dir = Scratch::new("progress"); + let mut s = library(&dir); + let input = crate::sync::SyncInput::new(&mut s, &dir.path("trip"), false).unwrap(); + let progress = crate::sync::SyncProgress::default(); + crate::sync::scan_with(input, &progress); + assert_eq!((progress.checked.load(Relaxed), progress.to_check.load(Relaxed)), (2, 2)); + let (done, total) = progress.counts(); + assert!(done == total && total >= 2, "{done}/{total}"); +} diff --git a/crates/ui-egui/src/sync.rs b/crates/ui-egui/src/sync.rs index 456a1d492..642de3e5e 100644 --- a/crates/ui-egui/src/sync.rs +++ b/crates/ui-egui/src/sync.rs @@ -6,8 +6,7 @@ use std::sync::Arc; use std::sync::atomic::Ordering; -use lightcraft_engine::import::ScanProgress; -use lightcraft_engine::sync::{FolderChanges, SyncChoice, SyncInput, scan_with}; +use lightcraft_engine::sync::{FolderChanges, SyncChoice, SyncInput, SyncProgress, scan_with}; use serde_json::{Value, json}; use crate::LightcraftApp; @@ -17,7 +16,7 @@ use crate::theme::Tokens; /// A running scan of the dialog's folder. pub struct SyncTask { path: String, - progress: Arc, + progress: Arc, rx: std::sync::mpsc::Receiver, } @@ -25,7 +24,7 @@ pub struct SyncTask { pub fn open(app: &mut LightcraftApp, path: &str, name: &str, disk: bool) -> Result<(), String> { cancel(app); let input = SyncInput::new(&mut app.session, path, disk).map_err(|e| e.to_string())?; - let progress = Arc::new(ScanProgress::default()); + let progress = Arc::new(SyncProgress::default()); let (tx, rx) = std::sync::mpsc::channel(); let p = progress.clone(); let job = move || { @@ -54,7 +53,7 @@ pub fn open(app: &mut LightcraftApp, path: &str, name: &str, disk: bool) -> Resu /// `folder.scanChanges` is the agent's). fn cancel(app: &mut LightcraftApp) { if let Some(t) = app.sync.take() { - t.progress.cancel.store(true, Ordering::Relaxed); + t.progress.files.cancel.store(true, Ordering::Relaxed); } if std::mem::take(&mut app.sync_owns_changes) { app.session.folder_changes = None; @@ -109,7 +108,7 @@ pub fn poll(app: &mut LightcraftApp, ctx: &egui::Context) { /// How far the scan is (files probed, of how many). fn progress(app: &LightcraftApp) -> Option<(usize, usize)> { - app.sync.as_ref().map(|t| (t.progress.done.load(Ordering::Relaxed), t.progress.total.load(Ordering::Relaxed))) + app.sync.as_ref().map(|t| t.progress.counts()) } /// The dialog's body.