From 7d3c8c521fb0de3f95b53d1e90ca637e7f2b89fa Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 17 Aug 2026 09:59:57 +0200 Subject: [PATCH] Export a whole selection, on a thread that is not the interface's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The export button rendered, resampled and encoded a 24 MP frame on the UI thread and the window was dead for all of it. That was written down as a known compromise, on the grounds that a batch is what makes the wait intolerable rather than merely noticeable. This is the batch, so the compromise comes due. The grid's selection now exports (FR-EXP-7). A worker thread takes a clone of the `GpuContext` — an `Arc` pair over a device and a queue — and opens each photograph for itself: fetch, sidecar, decode, demosaic, render at full size, resample, sharpen, encode, write. Nothing of that touches the interface, which keeps drawing throughout, and the progress goes where every other background job's does: one row in the activity register, with a count and a bar. Why the worker does not borrow the session it could have had. A `DevelopSession` owns the `AdjustPass` the canvas renders from, so handing it to a worker would stop the develop view drawing for the length of the batch — the same freeze, moved. Opening a session per image instead costs a `Demosaicer` and an `AdjustPass` each time round, and the pipeline cache is per-pass so the composed shader is recompiled per image rather than once for the run. Against a full-resolution decode, render and encode that is a few percent, and it keeps this file out of the pipeline `develop` owns. A reusable export pass is the obvious next economy if a profile ever says so. The open image is the exception, and it is why the develop button is not simply a one-image batch. Its edit lives in the interface's session and may not have reached a sidecar yet, so a worker that re-opened the file would export the saved version rather than the one on screen. That frame is therefore rendered by the caller and handed over as `Source::Rendered`; everything after the render — the Lanczos reduction, the encode, the write, which is the larger half of the wait and all of its variance — still leaves the UI thread. So the develop export is no longer synchronous, but it is not fully off-thread either, and the doc comment says so rather than claiming otherwise. Cancellation (NFR-ARCH-3) is an `AtomicBool` read between stages, and the export button becomes the cancel button while a run is live — a batch that could only be stopped by not touching the selection would be a trap. Waits on another worker use `recv_timeout` rather than `recv`, so a cancelled batch sitting on a forty-megabyte download gives up within 100 ms instead of when the transfer finishes. The honest bound is worse than that: a frame already in render has no interior stopping point, so the worst case is one image. Closing that needs the render itself to become interruptible, which is NFR-ARCH-2's scheduler and not a finer poll here. Failures are per image and typed (NFR-ARCH-4). One unreadable body, one folder that cannot be written, one server that went away — each is a message on the channel, a line in the log, and a count in the summary, and the batch carries on. A run with any failure keeps its row until it is cleared, because that is the row somebody came to the list to find; a cancelled run does not, because they asked for it. Two collisions that look alike and are not. `CollisionPolicy` is the user's answer to "a file of this name was already there", and Overwrite is a fine answer to that. It is not an answer to "the frame I exported four seconds ago was also called this" — two folders in a library each holding an IMG_0001 is ordinary — so a name the run has already issued is always stepped past whatever the policy says about the folder. Both halves are held by tests. Supporting changes, each smaller than it sounds. `open_session` comes out of `load_bytes` so the worker shares the JPEG-versus-RAW routing rather than carrying a copy that would drift; the half that builds a `slint::Image` stays behind, where it belongs. `LibraryController::selected_image_paths` answers from the catalog rather than from the loaded window, because selection is by id and survives a scrub — a selection made before scrolling routinely names photographs no row holds. `cache_context_for` takes an id for the same reason, so a batch reads the originals cache instead of re-downloading three hundred files. `format_date` is shared so `{date}` and the timeline agree about what day a photograph was taken. Left undone, deliberately: the batch is sequential, where FR-EXP-7 asks for all available cores. Four full-resolution frames in flight is tens of megabytes each and a straightforward way to exhaust a tablet, and the GPU is shared with the interface in any case. Also undone: exporting with a chosen preset rather than the current export settings — that is FR-EXP-5's machinery, which does not exist yet. Co-Authored-By: Claude Opus 5 --- ui/dr-ui/src/activity.rs | 12 +- ui/dr-ui/src/export.rs | 951 ++++++++++++++++++++++++++++++++++++- ui/dr-ui/src/lib.rs | 351 ++++++++------ ui/dr-ui/src/library_ui.rs | 164 ++++++- ui/dr-ui/ui/app.slint | 19 + ui/dr-ui/ui/library.slint | 54 +++ 6 files changed, 1410 insertions(+), 141 deletions(-) diff --git a/ui/dr-ui/src/activity.rs b/ui/dr-ui/src/activity.rs index da3157a..8957de1 100644 --- a/ui/dr-ui/src/activity.rs +++ b/ui/dr-ui/src/activity.rs @@ -80,6 +80,9 @@ pub enum Kind { Sync, /// Moving files to the trash, restoring them, or emptying it. Trash, + /// TRACES: FR-EXP-7 + /// Rendering and writing finished files. + Export, } impl Kind { @@ -94,7 +97,14 @@ impl Kind { | Kind::Download | Kind::Upload | Kind::Sync - | Kind::Trash => true, + | Kind::Trash + // A batch export fetches every original it does not already have + // cached, which on a selection of three hundred RAW files is the + // largest transfer this application ever starts. It moves nothing + // at all when the whole selection is already on disk, but the coarse + // answer has to be the cautious one: the question this flag answers + // is asked by somebody on a metered connection. + | Kind::Export => true, // Reads headers over the network today, but it is bounded by the // catalog rather than by anything the user asked to move, and it // stores nothing. Calling it a transfer would put an hours-long diff --git a/ui/dr-ui/src/export.rs b/ui/dr-ui/src/export.rs index 1a88ff0..3a2118e 100644 --- a/ui/dr-ui/src/export.rs +++ b/ui/dr-ui/src/export.rs @@ -27,13 +27,45 @@ //! promise and never do. An export waiting to upload is a promise — the user //! was told the export succeeded — and sweeping it away to reclaim disk would //! destroy work that no longer exists anywhere else. +//! +//! # The batch (FR-EXP-7) +//! +//! The second half of this file is a worker that exports a whole selection. +//! It runs on a thread of its own and reaches the interface through the same +//! two mechanisms everything else here does: an `mpsc` channel drained by a +//! Slint timer, and a row in [`crate::activity`]. +//! +//! **Why the worker opens its own sessions.** A [`crate::DevelopSession`] owns a GPU +//! `AdjustPass`, and the one the interface is holding is the one the canvas +//! renders from — handing it to a worker would mean the develop view could not +//! draw while a batch ran, which is the freeze this exists to remove. A +//! `GpuContext` is an `Arc` pair over a device and queue and is cheap to +//! clone, so the worker takes a clone and opens each photograph for itself. +//! The cost is a `Demosaicer` and an `AdjustPass` per image rather than one +//! for the run; against a full-resolution decode, render and encode it is +//! small, and it keeps this file out of the pipeline that `develop` owns. +//! +//! **The open image is the exception.** Its edit lives in the interface's +//! session and may not have reached a sidecar yet, so a worker that re-opened +//! the file for itself would export the *saved* version rather than the one on +//! screen. That one frame is therefore rendered by the caller and handed over +//! as [`Source::Rendered`]; everything after the render still moves off the UI +//! thread. +use std::collections::HashSet; use std::path::{Path, PathBuf}; +use std::rc::Rc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::mpsc::{Receiver, RecvTimeoutError, Sender}; +use std::sync::Arc; +use std::time::Duration; -use dr_export::Encoded; +use dr_export::{Encoded, NameContext}; use dr_sync::{RemoteBackend, RemotePath}; use dr_sync_nextcloud::{AppCredentials, NextcloudBackend}; -use dr_types::ExportTarget; +use dr_types::{ExportSettings, ExportTarget}; + +use crate::AppWindow; /// Where exports wait for a server that is not there yet. /// @@ -371,6 +403,619 @@ pub fn spawn_upload( rx } +// --------------------------------------------------------------------------- +// Batch export (FR-EXP-7) +// --------------------------------------------------------------------------- + +/// TRACES: NFR-ARCH-3 +/// The stop flag a running batch reads. +/// +/// One bit rather than a channel. Cancellation is read thousands of times more +/// often than it is written, and a message would only be seen if the worker +/// happened to be listening at the moment the button was pressed rather than at +/// the next point it is safe to stop. +/// +/// `Relaxed` throughout: the flag guards no other data, so there is nothing for +/// an acquire/release pair to publish. The only ordering that matters is that +/// the write eventually becomes visible, which every ordering guarantees. +#[derive(Clone, Default)] +pub struct Cancel(Arc); + +impl Cancel { + pub fn cancel(&self) { + self.0.store(true, Ordering::Relaxed); + } + + pub fn is_cancelled(&self) -> bool { + self.0.load(Ordering::Relaxed) + } +} + +/// How long the worker blocks on another worker before rereading the flag. +/// +/// TRACES: NFR-ARCH-3 +/// This is the cancellation bound for everything the batch spends its time +/// *waiting* on — a download of tens of megabytes would otherwise hold a +/// cancelled batch open until the transfer finished. It is not the bound for +/// the frame being rendered and encoded when the button is pressed: that has no +/// interior stopping point, so the true worst case is one image, and on a 24 MP +/// frame that is well past NFR-ARCH-3's 100 ms target. Closing that gap needs +/// the render itself to become interruptible (NFR-ARCH-2's scheduler), not a +/// finer poll here. +const CANCEL_POLL: Duration = Duration::from_millis(100); + +/// How often the drain looks at what the worker has said. +const DRAIN_INTERVAL: Duration = Duration::from_millis(120); + +/// Where one image's pixels come from. +pub enum Source { + /// A photograph in the library, fetched and rendered by the worker. + /// + /// Carries its own cache context because that is assembled from the catalog + /// and the session, and a worker thread can reach neither. + Library { + path: String, + cache: Option, + }, + /// A frame the caller has already rendered — the image open in develop. + /// + /// See the module docs for why this one cannot be left to the worker. + Rendered { + stem: String, + frame: dr_export::Frame, + }, +} + +impl Source { + /// What to call this image in a progress row. + fn describe(&self) -> String { + match self { + Source::Library { path, .. } => path.rsplit('/').next().unwrap_or(path).to_string(), + Source::Rendered { stem, .. } => stem.clone(), + } + } +} + +/// Everything a batch needs, assembled on the UI thread. +/// +/// Assembled there and not in the worker because most of it is only reachable +/// from a `RefCell` the interface owns: the session, the settings record, the +/// catalog. Passing the finished answers means the worker borrows nothing. +pub struct BatchRequest { + pub sources: Vec, + /// Credentials for the account the library is open on. `None` where no + /// library is open, which is fine for a [`Source::Rendered`] and fatal for + /// anything that has to be fetched. + pub creds: Option<(AppCredentials, String)>, + pub settings: ExportSettings, + pub outbox: PathBuf, + pub sidecar_cache: PathBuf, + pub offline: bool, + /// `None` on a build with no adapter, where a library image cannot be + /// rendered at all — reported per image rather than refused up front, so + /// the reason lands in the same place every other failure does. + pub gpu: Option, +} + +/// TRACES: NFR-ARCH-4 +/// Why one image of a batch produced no file. +/// +/// One variant per stage. "Export failed" repeated forty times is not a report +/// anybody can act on: a destination folder that cannot be written is one fix, +/// a body rawler cannot decode is another, and a server that went away is a +/// third — and only the first is worth stopping the batch to correct. +#[derive(Debug, thiserror::Error)] +pub enum ItemError { + #[error("could not be fetched: {0}")] + Fetch(String), + + #[error("could not be opened for editing: {0}")] + Open(String), + + #[error("could not be rendered: {0}")] + Render(String), + + /// The name was taken and the collision policy is Skip. Not really a + /// failure — the user asked for exactly this — but it is still an image + /// that produced no file, and a batch reporting "40 exported" when it wrote + /// 31 would be lying. + #[error("a file of that name is already there, and the collision setting is Skip")] + NameTaken, + + #[error(transparent)] + Encode(#[from] dr_export::ExportError), + + #[error("could not be written: {0}")] + Place(String), +} + +/// What the worker says as it goes. +#[derive(Debug)] +pub enum BatchMessage { + /// Work has begun on one image; `done` counts the ones behind it. + Started { + done: usize, + total: usize, + name: String, + }, + /// One image is finished, for better or worse. + Item { + name: String, + outcome: Result, + }, + Finished { + exported: usize, + failed: usize, + cancelled: bool, + }, +} + +/// Export a selection on a thread of its own. +pub fn spawn_batch(request: BatchRequest, cancel: Cancel) -> Receiver { + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || run(request, &cancel, &tx)); + rx +} + +/// The batch itself, one image at a time. +/// +/// Sequential rather than parallel, which FR-EXP-7's "uses all available cores" +/// does not yet get. One reason and one excuse: the reason is that a +/// full-resolution frame is tens of megabytes and four in flight is a +/// straightforward way to exhaust a tablet; the excuse is that the GPU is +/// shared with the interface, and the render is where the time goes. +fn run(mut request: BatchRequest, cancel: &Cancel, tx: &Sender) { + let sources = std::mem::take(&mut request.sources); + let total = sources.len(); + + // Names this run has already written. See [`resolve_batch_name`] for why + // the destination alone is not enough to keep two exports apart. + let mut issued: HashSet = HashSet::new(); + let (mut exported, mut failed) = (0usize, 0usize); + + for (i, source) in sources.into_iter().enumerate() { + if cancel.is_cancelled() { + break; + } + + let name = source.describe(); + let _ = tx.send(BatchMessage::Started { + done: i, + total, + name: name.clone(), + }); + + // `None` is cancellation mid-image, which is not an outcome for this + // photograph: nothing failed, the user simply stopped asking. + let Some(outcome) = export_one(&request, source, i as u32 + 1, &mut issued, cancel) else { + break; + }; + + // Counted, reported, and then on to the next one. A failure here must + // not end the run — the whole point of exporting three hundred frames + // unattended is that the one unreadable file is a line in a report + // rather than an evening lost (FR-EXP-7). + match &outcome { + Ok(_) => exported += 1, + Err(_) => failed += 1, + } + let _ = tx.send(BatchMessage::Item { name, outcome }); + } + + let _ = tx.send(BatchMessage::Finished { + exported, + failed, + cancelled: cancel.is_cancelled(), + }); +} + +/// One image, start to finish. `None` where the run was cancelled part way. +fn export_one( + request: &BatchRequest, + source: Source, + sequence: u32, + issued: &mut HashSet, + cancel: &Cancel, +) -> Option> { + let (stem, date, frame) = match source { + Source::Rendered { stem, frame } => (stem, String::new(), frame), + Source::Library { path, cache } => { + match render_from_library(request, &path, cache, cancel)? { + Ok(rendered) => rendered, + Err(e) => return Some(Err(e)), + } + } + }; + + Some(place_frame(request, &stem, &date, sequence, &frame, issued)) +} + +/// Fetch a photograph, apply its stored edit, and render it at full size. +fn render_from_library( + request: &BatchRequest, + path: &str, + cache: Option, + cancel: &Cancel, +) -> Option> { + let Some((creds, user_id)) = request.creds.clone() else { + return Some(Err(ItemError::Fetch("no library is open".into()))); + }; + let Some(gpu) = request.gpu.as_ref() else { + return Some(Err(ItemError::Open( + "this build found no GPU adapter".into(), + ))); + }; + + // Both started before either is waited on, exactly as opening an image in + // develop does: the sidecar is three orders of magnitude smaller than the + // RAW, so it costs nothing to have in hand by the time there is a session + // to apply it to. + let sidecar_rx = crate::library::spawn_sidecar_fetch( + creds.clone(), + user_id.clone(), + path.to_string(), + request.sidecar_cache.clone(), + request.offline, + ); + let bytes_rx = crate::library::spawn_full_fetch(creds, user_id, path.to_string(), cache); + + let bytes = match wait_for(&bytes_rx, cancel) { + Waited::Got(Ok(bytes)) => bytes, + Waited::Got(Err(e)) => return Some(Err(ItemError::Fetch(e.message))), + Waited::Cancelled => return None, + Waited::Silent => { + return Some(Err(ItemError::Fetch( + "the download ended without answering".into(), + ))) + } + }; + + let sidecar = match wait_for(&sidecar_rx, cancel) { + Waited::Got(sidecar) => sidecar, + Waited::Cancelled => return None, + // An unedited photograph has no sidecar and a fetch that died looks + // identical from here. Exporting at defaults is what opening it would + // do, and refusing the image over a missing edit it may never have had + // would fail the common case. + Waited::Silent => None, + }; + + let meta = dr_decode::metadata(&bytes).unwrap_or_default(); + let orientation = meta.orientation.unwrap_or_default(); + // `{date}` is the capture date, not today's: a template naming exports by + // when the shutter fired is the reason the token exists. + let date = meta + .captured_at + .map(crate::library_ui::format_date) + .unwrap_or_default(); + + let mut session = match crate::open_session(gpu, &bytes, orientation) { + Ok(session) => session, + Err(e) => return Some(Err(ItemError::Open(e))), + }; + + // TRACES: FR-CAT-8 + // The stored edit. FR-EXP-9 is about resolution; this is the other half of + // exporting what the user actually has, and a batch that skipped it would + // write out three hundred unedited frames without saying so. + if let Some(version) = sidecar + .as_ref() + .and_then(dr_pipeline::Sidecar::default_version) + { + session.apply_version(version); + } + + // The last cheap place to stop. Everything past here is a full-resolution + // render and an encode with no interior stopping point — see [`CANCEL_POLL`]. + if cancel.is_cancelled() { + return None; + } + + let frame = match session.render_for_export() { + Ok(frame) => frame, + Err(e) => return Some(Err(ItemError::Render(e))), + }; + + let stem = Path::new(path) + .file_stem() + .map(|s| s.to_string_lossy().into_owned()) + .unwrap_or_else(|| "export".into()); + + Some(Ok((stem, date, frame))) +} + +/// Name, encode and write one rendered frame. +/// +/// The tail every source shares, whoever rendered it. +fn place_frame( + request: &BatchRequest, + stem: &str, + date: &str, + sequence: u32, + frame: &dr_export::Frame, + issued: &mut HashSet, +) -> Result { + // The size is resolved before the name because `{dimensions}` is one of the + // tokens a template can carry. + let (width, height) = dr_export::target_size( + frame.width, + frame.height, + request.settings.sizing, + request.settings.allow_upscaling, + ); + + let ctx = NameContext { + source_stem: stem, + sequence, + date, + width, + height, + preset: "", + }; + + let name = resolve_batch_name(&request.settings, &ctx, issued).ok_or(ItemError::NameTaken)?; + let encoded = dr_export::export(frame, &request.settings, name)?; + + place( + &encoded, + request.settings.target, + &request.settings.destination, + &request.outbox, + ) + .map_err(ItemError::Place) +} + +/// The name this export takes, avoiding both what was in the destination and +/// what this run has already written. +/// +/// Those are two different collisions and they deserve two different answers. +/// [`dr_types::CollisionPolicy`] is the user's answer to "a file of this name +/// was already there", and Overwrite is a perfectly reasonable one. It is not +/// an answer to "the frame I exported four seconds ago was also called this": +/// two photographs of the same stem in different folders are ordinary in a +/// library, and a batch that quietly handed back fewer files than images — +/// having destroyed its own output — is not something anybody asked for. So a +/// name this run issued is always stepped past, whatever the policy says about +/// the folder. +fn resolve_batch_name( + settings: &ExportSettings, + ctx: &NameContext<'_>, + issued: &mut HashSet, +) -> Option { + let dir = PathBuf::from(&settings.destination); + let taken = |name: &str| -> bool { + match settings.target { + dr_types::ExportTarget::Device => dir.join(name).exists(), + // A queued export cannot see the server, and may never be able to. + // Names are kept apart in the outbox instead — see [`stage`]. + dr_types::ExportTarget::Remote => false, + } + }; + + let name = dr_export::resolve_name( + &settings.filename_template, + ctx, + settings.format, + settings.collision, + &|name| taken(name) || issued.contains(name), + )?; + + // Only reachable under Overwrite, which hands back the taken name by + // design. Skip and Increment have already been through the closure above. + let name = if issued.contains(&name) { + step_past(&name, &|candidate| { + taken(candidate) || issued.contains(candidate) + })? + } else { + name + }; + + issued.insert(name.clone()); + Some(name) +} + +/// `photo.jpg` → `photo-1.jpg`, and on until the name is free. +fn step_past(name: &str, taken: &dyn Fn(&str) -> bool) -> Option { + let path = Path::new(name); + let stem = path + .file_stem() + .map(|s| s.to_string_lossy().into_owned()) + .unwrap_or_else(|| "export".into()); + let ext = path + .extension() + .map(|s| s.to_string_lossy().into_owned()) + .unwrap_or_default(); + + // Bounded for the same reason `resolve_name`'s own search is: a destination + // that reports every name as taken has to fail rather than spin. + (1..10_000) + .map(|n| { + if ext.is_empty() { + format!("{stem}-{n}") + } else { + format!("{stem}-{n}.{ext}") + } + }) + .find(|candidate| !taken(candidate)) +} + +/// What waiting on another worker produced. +/// +/// Three answers rather than an `Option`, because "the user cancelled" and "the +/// worker died without a word" have the same shape and want opposite treatment: +/// one ends the batch quietly, the other is a failure belonging to one image. +enum Waited { + Got(T), + Cancelled, + Silent, +} + +/// Block on another worker's answer without going deaf to cancellation. +fn wait_for(rx: &Receiver, cancel: &Cancel) -> Waited { + loop { + match rx.recv_timeout(CANCEL_POLL) { + Ok(value) => return Waited::Got(value), + Err(RecvTimeoutError::Timeout) => { + if cancel.is_cancelled() { + return Waited::Cancelled; + } + } + Err(RecvTimeoutError::Disconnected) => return Waited::Silent, + } + } +} + +/// Which status line a batch writes its commentary to. +/// +/// One drain serves both entry points rather than two near-identical timers, so +/// it has to be told where the words go: develop has its own line beside the +/// export button, and the grid has the header's. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Reporting { + Develop, + Library, +} + +/// TRACES: FR-EXP-7 | NFR-P9 +/// Drain a batch's progress on the UI thread. +/// +/// `slot` holds the timer, and dropping what was there before is what stops a +/// superseded batch's drain — and, through [`crate::activity::Activity`]'s +/// `Drop`, takes its row with it. +pub fn drain_batch( + weak: slint::Weak, + activity: &Rc, + slot: &Rc>>, + rx: Receiver, + total: usize, + reporting: Reporting, +) { + let job = activity.begin( + crate::activity::Kind::Export, + if total == 1 { + "Exporting".to_string() + } else { + format!("Exporting {total} images") + }, + ); + job.total(total); + + // The first failure, kept for the summary. Only the first: a run where + // every image failed for the same reason should say that reason once, and + // the rest are in the log. + let mut first_failure: Option = None; + + let timer = slint::Timer::default(); + let held = slot.clone(); + + timer.start(slint::TimerMode::Repeated, DRAIN_INTERVAL, move || { + let Some(w) = weak.upgrade() else { return }; + + loop { + let message = match rx.try_recv() { + Ok(m) => m, + Err(std::sync::mpsc::TryRecvError::Empty) => return, + Err(std::sync::mpsc::TryRecvError::Disconnected) => { + // A worker that died without a word must not leave the + // button saying "Cancel export" for the rest of the session. + job.fail("ended unexpectedly"); + settle(&w, reporting, "Export ended unexpectedly"); + stop_timer(&held); + return; + } + }; + + match message { + BatchMessage::Started { done, total, name } => { + job.progress(done, total); + job.detail(name.clone()); + report( + &w, + reporting, + &format!("Exporting {name} ({}/{total})", done + 1), + ); + } + BatchMessage::Item { name, outcome } => match outcome { + Ok(placed) => log::info!("{name}: {}", placed.describe()), + Err(e) => { + // Every failure is logged, not only the first: the + // summary is a sentence and this is the record of which + // photographs it is about (NFR-ARCH-4). + log::warn!("exporting {name}: {e}"); + first_failure.get_or_insert_with(|| format!("{name}: {e}")); + } + }, + BatchMessage::Finished { + exported, + failed, + cancelled, + } => { + let (text, is_failure) = + summarise(exported, failed, cancelled, first_failure.as_deref()); + if is_failure { + job.fail(text.clone()); + } else { + job.finish(text.clone()); + } + settle(&w, reporting, &text); + stop_timer(&held); + return; + } + } + } + }); + + *slot.borrow_mut() = Some(timer); +} + +/// How a finished batch reads, and whether it counts as a failure. +/// +/// A cancelled run is never a failure however little it exported: the user +/// stopped it, and a red row telling them so is the application arguing. A run +/// with even one failure is, and stays in the list until it is cleared — that +/// is the row somebody came looking for. +fn summarise( + exported: usize, + failed: usize, + cancelled: bool, + first_failure: Option<&str>, +) -> (String, bool) { + if cancelled { + return (format!("Cancelled after {exported}"), false); + } + if failed == 0 { + return (format!("Exported {exported}"), false); + } + let detail = first_failure.unwrap_or("see the log"); + ( + format!("Exported {exported}, {failed} failed — {detail}"), + true, + ) +} + +/// Say what the batch is doing on the line the caller came from. +fn report(window: &AppWindow, reporting: Reporting, text: &str) { + match reporting { + Reporting::Develop => window.set_export_status(text.into()), + Reporting::Library => window.set_library_status(text.into()), + } +} + +/// Report, and put the buttons back. +fn settle(window: &AppWindow, reporting: Reporting, text: &str) { + report(window, reporting, text); + match reporting { + Reporting::Develop => window.set_export_busy(false), + Reporting::Library => window.set_library_exporting(false), + } +} + +fn stop_timer(slot: &Rc>>) { + if let Some(timer) = slot.borrow().as_ref() { + timer.stop(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -596,4 +1241,306 @@ mod tests { .describe() .contains("Exported")); } + + // ----------------------------------------------------------------------- + // The batch (FR-EXP-7) + // ----------------------------------------------------------------------- + + /// Settings that write PNGs into `dir`, so a test can look at what landed. + fn to_folder(dir: &Path) -> ExportSettings { + ExportSettings { + format: dr_types::ExportFormat::Png, + target: ExportTarget::Device, + destination: dir.to_string_lossy().into_owned(), + ..Default::default() + } + } + + fn request(settings: ExportSettings, sources: Vec) -> BatchRequest { + BatchRequest { + sources, + creds: None, + settings, + outbox: std::env::temp_dir().join("dr-batch-test-outbox"), + sidecar_cache: std::env::temp_dir().join("dr-batch-test-sidecars"), + offline: true, + gpu: None, + } + } + + /// A frame of flat pixels — enough for the encoder, cheap for a test. + fn frame(width: u32, height: u32) -> dr_export::Frame { + dr_export::Frame::new(width, height, vec![128; (width * height * 4) as usize]) + .expect("well-formed") + } + + fn drive(request: BatchRequest, cancel: Cancel) -> Vec { + let (tx, rx) = std::sync::mpsc::channel(); + run(request, &cancel, &tx); + drop(tx); + rx.into_iter().collect() + } + + fn finished(messages: &[BatchMessage]) -> (usize, usize, bool) { + messages + .iter() + .find_map(|m| match m { + BatchMessage::Finished { + exported, + failed, + cancelled, + } => Some((*exported, *failed, *cancelled)), + _ => None, + }) + .expect("a batch always says it has finished") + } + + #[test] + fn a_rendered_frame_reaches_the_destination_folder() { + // The whole tail of the batch — name, size, sharpen, encode, write — + // with no GPU and no network, which is what makes it testable at all. + let dir = tmp(); + let out = dir.join("exports"); + let messages = drive( + request( + to_folder(&out), + vec![Source::Rendered { + stem: "IMG_0001".into(), + frame: frame(16, 12), + }], + ), + Cancel::default(), + ); + + assert_eq!(finished(&messages), (1, 0, false)); + assert!(out.join("IMG_0001.png").exists()); + } + + #[test] + fn two_images_of_one_name_both_survive_the_batch() { + // Two folders in a library holding an IMG_0001 each is ordinary, and a + // batch that wrote one file for two photographs would destroy work + // without saying anything. Overwrite is set deliberately: it is the + // user's answer about the *folder*, not about this run's own output. + let dir = tmp(); + let out = dir.join("exports"); + let mut settings = to_folder(&out); + settings.collision = dr_types::CollisionPolicy::Overwrite; + + let messages = drive( + request( + settings, + vec![ + Source::Rendered { + stem: "IMG_0001".into(), + frame: frame(16, 12), + }, + Source::Rendered { + stem: "IMG_0001".into(), + frame: frame(16, 12), + }, + ], + ), + Cancel::default(), + ); + + assert_eq!(finished(&messages), (2, 0, false)); + assert!(out.join("IMG_0001.png").exists()); + assert!(out.join("IMG_0001-1.png").exists()); + } + + #[test] + fn a_name_that_was_already_there_still_obeys_the_collision_setting() { + // The other half of the rule above: a file that existed *before* the + // batch is exactly what the policy is about, and Overwrite must still + // mean overwrite or the setting would do nothing. + let dir = tmp(); + let out = dir.join("exports"); + std::fs::create_dir_all(&out).expect("temp dir"); + std::fs::write(out.join("IMG_0001.png"), b"older").expect("seed"); + + let mut settings = to_folder(&out); + settings.collision = dr_types::CollisionPolicy::Overwrite; + + let mut issued = HashSet::new(); + let name = resolve_batch_name( + &settings, + &NameContext { + source_stem: "IMG_0001", + sequence: 1, + ..Default::default() + }, + &mut issued, + ); + assert_eq!(name.as_deref(), Some("IMG_0001.png")); + } + + #[test] + fn a_skipped_name_is_reported_rather_than_silently_dropped() { + // Skip is a legitimate answer, but the image still produced no file — + // and a batch claiming to have exported it would be lying. + let dir = tmp(); + let out = dir.join("exports"); + std::fs::create_dir_all(&out).expect("temp dir"); + std::fs::write(out.join("IMG_0001.png"), b"older").expect("seed"); + + let mut settings = to_folder(&out); + settings.collision = dr_types::CollisionPolicy::Skip; + + let messages = drive( + request( + settings, + vec![Source::Rendered { + stem: "IMG_0001".into(), + frame: frame(8, 8), + }], + ), + Cancel::default(), + ); + + assert_eq!(finished(&messages), (0, 1, false)); + assert!(matches!( + messages.iter().find_map(|m| match m { + BatchMessage::Item { outcome, .. } => Some(outcome), + _ => None, + }), + Some(Err(ItemError::NameTaken)) + )); + assert_eq!(std::fs::read(out.join("IMG_0001.png")).unwrap(), b"older"); + } + + #[test] + fn one_failure_does_not_abandon_the_rest_of_the_batch() { + // FR-EXP-7's central promise. The first source cannot be fetched — no + // library is open — and the two after it must still be attempted and + // still be counted. + let dir = tmp(); + let out = dir.join("exports"); + let messages = drive( + request( + to_folder(&out), + vec![ + Source::Library { + path: "Photos/broken.CR2".into(), + cache: None, + }, + Source::Rendered { + stem: "good-a".into(), + frame: frame(8, 8), + }, + Source::Rendered { + stem: "good-b".into(), + frame: frame(8, 8), + }, + ], + ), + Cancel::default(), + ); + + assert_eq!(finished(&messages), (2, 1, false)); + assert!(out.join("good-a.png").exists()); + assert!(out.join("good-b.png").exists()); + } + + #[test] + fn every_image_gets_its_own_outcome() { + // The report is per image, not one verdict for the run: two failures + // for two different reasons have to arrive as two messages, or a + // three-hundred-frame batch is unreportable (NFR-ARCH-4). + let dir = tmp(); + let messages = drive( + request( + to_folder(&dir.join("exports")), + vec![ + Source::Library { + path: "Photos/a.CR2".into(), + cache: None, + }, + Source::Rendered { + stem: "b".into(), + frame: frame(8, 8), + }, + ], + ), + Cancel::default(), + ); + + let outcomes: Vec<&Result> = messages + .iter() + .filter_map(|m| match m { + BatchMessage::Item { outcome, .. } => Some(outcome), + _ => None, + }) + .collect(); + assert_eq!(outcomes.len(), 2); + assert!(outcomes[0].is_err()); + assert!(outcomes[1].is_ok()); + } + + #[test] + fn a_cancelled_batch_stops_and_writes_nothing_more() { + // TRACES: NFR-ARCH-3 + // Cancelled before it began, which is the strongest form of the + // property: not one file, and the run still reports itself finished + // rather than leaving the interface waiting for a message. + let dir = tmp(); + let out = dir.join("exports"); + let cancel = Cancel::default(); + cancel.cancel(); + + let messages = drive( + request( + to_folder(&out), + vec![Source::Rendered { + stem: "IMG_0001".into(), + frame: frame(8, 8), + }], + ), + cancel, + ); + + assert_eq!(finished(&messages), (0, 0, true)); + assert!(!out.join("IMG_0001.png").exists()); + } + + #[test] + fn a_cancelled_wait_gives_up_instead_of_blocking_for_ever() { + // TRACES: NFR-ARCH-3 + // The bound on cancelling a batch that is waiting on a download. With + // a plain `recv` this test would hang, which is precisely the bug. + let (tx, rx) = std::sync::mpsc::channel::(); + let cancel = Cancel::default(); + cancel.cancel(); + + assert!(matches!(wait_for(&rx, &cancel), Waited::Cancelled)); + drop(tx); + } + + #[test] + fn a_worker_that_dies_is_not_mistaken_for_a_cancellation() { + // The two look identical from a channel and mean opposite things: one + // ends the batch, the other fails one image and moves on. + let (tx, rx) = std::sync::mpsc::channel::(); + drop(tx); + assert!(matches!(wait_for(&rx, &Cancel::default()), Waited::Silent)); + } + + #[test] + fn a_cancelled_run_is_not_reported_as_a_failure() { + // The user stopped it. A red row saying so is the application arguing + // with something it was told to do. + let (text, failed) = summarise(5, 0, true, None); + assert!(!failed, "{text}"); + assert!(text.contains('5'), "{text}"); + } + + #[test] + fn a_run_with_a_failure_keeps_its_row_and_names_one() { + // This is the row somebody comes to the activity list to find, so it + // has to survive being trimmed — and it has to say which photograph. + let (text, failed) = summarise(38, 2, false, Some("IMG_0007.CR2: could not be rendered")); + assert!(failed); + assert!(text.contains("IMG_0007.CR2"), "{text}"); + assert!(text.contains("38"), "{text}"); + } } diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index d7acf76..7decced 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -116,48 +116,12 @@ fn load(ctx: Option<&dr_gpu::GpuContext>, path: &Path) -> Result fn load_bytes(ctx: Option<&dr_gpu::GpuContext>, bytes: &[u8]) -> Result { let meta = dr_decode::metadata(bytes).unwrap_or_default(); - // Route by what the bytes actually are, not by extension (M-9). - // - // A JPEG has no sensor data and never will, so trying the RAW decoder - // first would be a guaranteed failure whose log line reads like a fault. - // It goes straight to the RGB path instead, which is what makes develop - // mode work on the JPEGs the library already indexes. - let is_jpeg = dr_decode::probe(bytes) == Some(dr_types::Format::Jpeg); - // How the file stored its pixels. A file that says nothing is taken as // upright — see `Orientation::from_exif`. let orientation = meta.orientation.unwrap_or_default(); if let Some(ctx) = ctx { - let opened = if is_jpeg { - dr_decode::decode_jpeg(bytes) - .map_err(|e| e.to_string()) - .and_then(|mut p| { - // Fit the device before uploading. A film scan runs to - // 13728×8928, well past the 8192 a typical GPU can hold, - // and refusing it would drop the image back to a - // read-only preview — the very thing this path exists to - // avoid. 8192 is still four times a 4K long edge. - let limit = dr_gpu::DemosaicedImage::max_dimension(ctx); - if p.width.max(p.height) > limit { - log::info!( - "{}×{} exceeds the {limit} texture limit; fitting to it", - p.width, - p.height - ); - p.downscale_to(limit); - } - DevelopSession::open_rgb(ctx, &p.rgba, p.width, p.height, orientation) - }) - } else { - // A failure here is expected for bodies rawler does not know, and - // must not stop the image displaying (FR-RAW-4). - dr_decode::decode(bytes) - .map_err(|e| e.to_string()) - .and_then(|raw| DevelopSession::open(ctx, &raw, orientation)) - }; - - match opened { + match open_session(ctx, bytes, orientation) { Ok(session) => { let (width, height) = session.source_size(); return Ok(Loaded { @@ -197,6 +161,51 @@ fn load_bytes(ctx: Option<&dr_gpu::GpuContext>, bytes: &[u8]) -> Result Result { + // Route by what the bytes actually are, not by extension (M-9). A JPEG has + // no sensor data and never will, so trying the RAW decoder first would be a + // guaranteed failure whose log line reads like a fault. + if dr_decode::probe(bytes) == Some(dr_types::Format::Jpeg) { + return dr_decode::decode_jpeg(bytes) + .map_err(|e| e.to_string()) + .and_then(|mut p| { + // Fit the device before uploading. A film scan runs to + // 13728×8928, well past the 8192 a typical GPU can hold, and + // refusing it would drop the image back to a read-only preview + // — the very thing this path exists to avoid. 8192 is still + // four times a 4K long edge. + let limit = dr_gpu::DemosaicedImage::max_dimension(ctx); + if p.width.max(p.height) > limit { + log::info!( + "{}×{} exceeds the {limit} texture limit; fitting to it", + p.width, + p.height + ); + p.downscale_to(limit); + } + DevelopSession::open_rgb(ctx, &p.rgba, p.width, p.height, orientation) + }); + } + + // A failure here is expected for bodies rawler does not know, and must not + // stop the image displaying (FR-RAW-4). + dr_decode::decode(bytes) + .map_err(|e| e.to_string()) + .and_then(|raw| DevelopSession::open(ctx, &raw, orientation)) +} + /// Collect displayable images from file or directory arguments. fn collect(paths: &[PathBuf]) -> Vec { let mut out = Vec::new(); @@ -289,93 +298,63 @@ fn sync_framing(window: &AppWindow, session: &Rc> window.set_crop_h(crop.height); } -/// TRACES: FR-EXP-6 | FR-EXP-9 -/// Render the open image at full resolution and place the result. +/// TRACES: FR-EXP-6 | FR-EXP-9 | FR-EXP-7 +/// Render the open image at full resolution, ready to be handed to the worker. /// -/// Synchronous, on the UI thread, and that is a known compromise rather than -/// an oversight. A full-resolution render plus a Lanczos reduction plus an -/// encode is hundreds of milliseconds on a 24 MP frame, and the window is -/// unresponsive for all of it. It is done this way because the alternative — -/// moving a `DevelopSession` and its GPU pass onto a worker — is a larger -/// change than the button is worth before batch export exists, and a batch is -/// what makes the wait intolerable rather than merely noticeable. The status -/// line says what is happening in the meantime. -fn export_now( +/// The render stays here, on the UI thread, and everything after it does not. +/// That split is deliberate rather than the remains of the synchronous version +/// this replaced: the frame belongs to the `DevelopSession` the interface owns, +/// and the edit in it may not have reached a sidecar yet, so a worker that +/// re-opened the photograph for itself would export the *saved* version rather +/// than the one on screen. +/// +/// What is left on this thread is a GPU pass and a readback. The Lanczos +/// reduction, the encode and the write — the larger half of the wait, and all +/// of its variance — leave with the frame. +fn render_open_frame( window: &AppWindow, session: &Rc>>, - settings: &Rc, - library: &Rc, -) -> Result { +) -> Result { let mut borrowed = session.borrow_mut(); let Some(session) = borrowed.as_mut() else { return Err("nothing is open".into()); }; - let stored = settings.snapshot(); let frame = session.render_for_export()?; - - // The size has to be resolved before the name, because `{dimensions}` is - // one of the tokens the template can carry. - let (tw, th) = dr_export::target_size( - frame.width, - frame.height, - stored.export.sizing, - stored.export.allow_upscaling, - ); - let filename = window.get_filename().to_string(); let stem = std::path::Path::new(&filename) .file_stem() .map(|s| s.to_string_lossy().into_owned()) .unwrap_or_else(|| "export".into()); - let outbox = match library.session() { - Some((_, s)) => export::outbox_dir(&s.server, &s.user_id), - // No account, so no outbox — a device export still works, and a - // remote one is refused below by `place` rather than here, so the - // message names the setting rather than the plumbing. - None => std::env::temp_dir().join("darkroom-outbox"), - }; + Ok(export::Source::Rendered { stem, frame }) +} - let ctx = dr_export::NameContext { - source_stem: &stem, - sequence: 1, - date: "", - width: tw, - height: th, - preset: "", - }; +/// TRACES: FR-EXP-7 +/// Everything a batch needs that only the UI thread can assemble. +fn batch_request( + sources: Vec, + settings: &Rc, + library: &Rc, + gpu: Option<&dr_gpu::GpuContext>, +) -> export::BatchRequest { + let stored = settings.snapshot(); - // What counts as "taken" depends on where this is going. A device export - // can look at the folder; a queued one is checked against the outbox, - // since the server cannot be reached from here and may not be reachable - // at all — see `export::stage` for why two queued exports of one name - // both survive regardless. - let target_dir = std::path::PathBuf::from(&stored.export.destination); - let taken = |name: &str| -> bool { - match stored.export.target { - dr_types::ExportTarget::Device => target_dir.join(name).exists(), - dr_types::ExportTarget::Remote => false, - } - }; - - let name = dr_export::resolve_name( - &stored.export.filename_template, - &ctx, - stored.export.format, - stored.export.collision, - &taken, - ) - .ok_or("a file of that name is already there, and the collision setting is Skip")?; - - let encoded = dr_export::export(&frame, &stored.export, name).map_err(|e| e.to_string())?; - - export::place( - &encoded, - stored.export.target, - &stored.export.destination, - &outbox, - ) + export::BatchRequest { + sources, + creds: library.credentials(), + settings: stored.export, + outbox: match library.session() { + Some((_, s)) => export::outbox_dir(&s.server, &s.user_id), + // No account, so no outbox — a device export still works, and a + // remote one is refused by `place` rather than here, so the message + // names the setting rather than the plumbing. + None => std::env::temp_dir().join("darkroom-outbox"), + }, + sidecar_cache: library.sidecar_cache_dir().unwrap_or_default(), + offline: library.is_offline(), + gpu: gpu.cloned(), + } } /// What the export button should say, given where an export would go. @@ -385,11 +364,19 @@ fn export_now( /// lands on this device or is queued for a server that may be unreachable. fn refresh_export_label(window: &AppWindow, settings: &Rc) { let stored = settings.snapshot(); - let label = match stored.export.target { - dr_types::ExportTarget::Device => "Export".to_string(), - dr_types::ExportTarget::Remote => "Export to Nextcloud".to_string(), - }; - window.set_export_label(label.into()); + let remote = stored.export.target == dr_types::ExportTarget::Remote; + window.set_export_label( + if remote { + "Export to Nextcloud" + } else { + "Export" + } + .into(), + ); + // The grid's button says the same thing about a selection, but it composes + // its own label around a count — so it is given the fact rather than the + // sentence (FR-EXP-7). + window.set_export_to_server(remote); } /// Push current parameter values back to the interface. @@ -1416,33 +1403,125 @@ pub fn run(paths: Vec) -> Result<()> { // // Generic by construction: they carry indices into the capability list, // so adding an operation needs no change here (FR-DEV-3c). - // --- export --------------------------------------------------------- + // --- export (FR-EXP-6, FR-EXP-7, FR-EXP-9) --------------------------- // - // Renders its own frame at full resolution rather than encoding what is - // on screen: the display render is deliberately viewport-sized - // (FR-DSP-1), and exporting that would hand the user a soft, screen-sized - // file with no indication anything had been lost (FR-EXP-9). + // Two buttons, one worker. The develop view exports the image on screen and + // the grid exports its selection; the only difference between them is who + // renders the frames, so both hand a `Vec` to the same batch and + // both report through the same activity row. + // + // A frame is always rendered at full resolution rather than taken from the + // canvas: the display render is deliberately viewport-sized (FR-DSP-1), and + // exporting that would hand the user a soft, screen-sized file with no + // indication anything had been lost (FR-EXP-9). { - let weak = window.as_weak(); - let session = session.clone(); - let settings = settings.clone(); - let library = library.clone(); - window.on_export_image(move || { - let Some(w) = weak.upgrade() else { return }; - w.set_export_busy(true); - // Pushed before the work rather than after: the render blocks the - // UI thread, so a label set afterwards would never be drawn in - // the "Exporting…" state at all. - w.set_export_status("Rendering…".into()); + // Replaced at each start, so the button always cancels the run it is + // sitting on and a stale token cancels nothing. + let cancel: Rc> = Rc::new(RefCell::new(export::Cancel::default())); + // Holds the drain. Assigning a new timer drops the previous one, which + // is what takes a superseded batch's row out of the register. + let drain: Rc>> = Rc::new(RefCell::new(None)); - let result = export_now(&w, &session, &settings, &library); - match result { - Ok(placed) => w.set_export_status(placed.describe().into()), - Err(e) => w.set_export_status(format!("Export failed: {e}").into()), - } - w.set_export_busy(false); - refresh_export_label(&w, &settings); - }); + let start = { + let settings = settings.clone(); + let library = library.clone(); + let activity = activity.clone(); + let gpu = gpu.clone(); + let cancel = cancel.clone(); + let drain = drain.clone(); + Rc::new( + move |window: &AppWindow, sources: Vec, to: export::Reporting| { + let total = sources.len(); + let request = batch_request(sources, &settings, &library, gpu.as_ref()); + + let token = export::Cancel::default(); + *cancel.borrow_mut() = token.clone(); + + let rx = export::spawn_batch(request, token); + export::drain_batch(window.as_weak(), &activity, &drain, rx, total, to); + }, + ) + }; + + { + let weak = window.as_weak(); + let session = session.clone(); + let settings = settings.clone(); + let start = start.clone(); + window.on_export_image(move || { + let Some(w) = weak.upgrade() else { return }; + // One batch at a time, and the grid's counts as one. The two + // buttons share a worker slot, so starting a second run would + // drop the first's drain — leaving a batch still writing files + // with no progress row and a button that never comes back. + if w.get_export_busy() || w.get_library_exporting() { + w.set_export_status("An export is already running".into()); + return; + } + w.set_export_busy(true); + // Set before the render rather than after: the render is the + // one part still on this thread, so a label written afterwards + // would never be drawn in the "Rendering…" state at all. + w.set_export_status("Rendering…".into()); + + match render_open_frame(&w, &session) { + Ok(source) => start(&w, vec![source], export::Reporting::Develop), + Err(e) => { + w.set_export_status(format!("Export failed: {e}").into()); + w.set_export_busy(false); + } + } + refresh_export_label(&w, &settings); + }); + } + + // TRACES: FR-EXP-7 + // The grid's selection. Nothing is rendered here — the worker fetches, + // decodes and renders each photograph itself, so this returns to the + // event loop immediately and a batch of three hundred is a progress bar + // rather than a frozen window. + { + let weak = window.as_weak(); + let library = library.clone(); + let collections = collections.clone(); + let start = start.clone(); + window.on_library_export_selection(move || { + let Some(w) = weak.upgrade() else { return }; + // See the develop button above for why one run excludes the + // other. The button is a cancel by then, so this only catches + // a batch started from develop and left running. + if w.get_library_exporting() || w.get_export_busy() { + w.set_library_status("An export is already running".into()); + return; + } + + let sources = library.export_sources(&collections.selected()); + if sources.is_empty() { + // Said out loud rather than ignored, matching what a paste + // or a judgement keystroke does with an empty selection. + w.set_library_status("Select an image first".into()); + return; + } + + w.set_library_exporting(true); + w.set_library_status(format!("Exporting {} images…", sources.len()).into()); + start(&w, sources, export::Reporting::Library); + }); + } + + // TRACES: NFR-ARCH-3 + { + let weak = window.as_weak(); + window.on_library_cancel_export(move || { + cancel.borrow().cancel(); + if let Some(w) = weak.upgrade() { + // The worker stops at the next point it is safe to — which + // may be a frame away — so the button says "asked for" and + // not "done". The drain writes the real answer. + w.set_library_status("Cancelling the export…".into()); + } + }); + } } { diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index 3fd6bbf..df6c895 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -418,6 +418,18 @@ impl LibraryController { /// grid row that has scrolled away, or no library open — and the fetch /// then simply goes to the network. pub fn cache_context(&self, path: &str) -> Option { + self.cache_context_for(self.image_id_for_path(path)?) + } + + /// TRACES: FR-EXP-7 | FR-NC-6a + /// The same, for an image named by id rather than by path. + /// + /// A batch export needs this one: its selection is by catalog id and may + /// include photographs that have scrolled out of the loaded window, where + /// [`Self::image_id_for_path`] has nothing to match against. Going through + /// the path would quietly hand those images no cache at all, and a batch of + /// three hundred would re-download every one of them. + pub fn cache_context_for(&self, image: dr_types::ImageId) -> Option { let borrow = self.session.borrow(); let (_, session, _) = borrow.as_ref()?; let catalog_path = library::catalog_path(&session.server, &session.user_id); @@ -427,7 +439,7 @@ impl LibraryController { Some(library::CacheContext { dir, catalog_path, - image: self.image_id_for_path(path)?, + image, // The user's ceiling, not the catalog's floor: read at each fetch // so a budget changed mid-session takes effect on the next one. budget: self.cache_budget.get(), @@ -480,6 +492,78 @@ impl LibraryController { .collect() } + /// TRACES: FR-EXP-7 + /// Remote paths for a set of selected images, in the order they were given. + /// + /// Answered from the catalog rather than from `paths`, which is only the + /// loaded window. Selection is by id precisely so that it survives a scrub + /// (see [`crate::collections_ui`]), so a selection made before scrolling + /// routinely names photographs no row currently holds — and an export that + /// silently dropped those would be worse than one that refused. + /// + /// An id the catalog has never heard of is skipped rather than reported: it + /// can only mean the image was deleted between the selection and the click, + /// and there is nothing to export and nothing to fix. + /// + /// Each path comes back beside the id it belongs to, because that skipping + /// means the ids that come out are not the ids that went in — and the caller + /// still has to find each photograph's cache, which is keyed on the id. + pub fn selected_image_paths( + &self, + images: &[dr_types::ImageId], + ) -> Vec<(dr_types::ImageId, String)> { + if images.is_empty() { + return Vec::new(); + } + + let borrow = self.catalog.borrow(); + let Some(catalog) = borrow.as_ref() else { + return Vec::new(); + }; + + let placeholders = std::iter::repeat_n("?", images.len()) + .collect::>() + .join(","); + let sql = format!("SELECT id, source_ref FROM images WHERE id IN ({placeholders})"); + let params: Vec = images + .iter() + .map(|i| rusqlite::types::Value::Integer(i.0 as i64)) + .collect(); + + let Ok(mut stmt) = catalog.connection().prepare(&sql) else { + return Vec::new(); + }; + let Ok(rows) = stmt.query_map(rusqlite::params_from_iter(params.iter()), |r| { + Ok((r.get::<_, i64>(0)?, r.get::<_, String>(1)?)) + }) else { + return Vec::new(); + }; + + // `IN` returns rows in whatever order suits SQLite, and the export's + // `{seq}` token counts through the batch — so the answer is put back + // into the order the caller asked in rather than the one it arrived in. + let found: std::collections::HashMap = rows.flatten().collect(); + images + .iter() + .filter_map(|id| found.get(&(id.0 as i64)).map(|path| (*id, path.clone()))) + .collect() + } + + /// TRACES: FR-EXP-7 + /// The selection, addressed the way an export worker can fetch it. + /// + /// Assembled here because a worker thread can reach neither the catalog nor + /// the session, and both are needed to say where a cached original lives. + pub fn export_sources(&self, images: &[dr_types::ImageId]) -> Vec { + self.selected_image_paths(images) + .into_iter() + .map(|(id, path)| crate::export::Source::Library { + path, + cache: self.cache_context_for(id), + }) + .collect() + } + /// Credentials and account for the open library, if one is open. /// /// What a full-file fetch needs: the grid's paths are remote, so opening @@ -2629,7 +2713,12 @@ fn format_bucket(t: i64, g: dr_catalog::Granularity) -> String { } } -fn format_date(t: i64) -> String { +/// A capture instant as `YYYY-MM-DD`. +/// +/// Shared with the exporter, which resolves the `{date}` token from the same +/// reading so a filename and the timeline cannot disagree about what day a +/// photograph was taken. +pub fn format_date(t: i64) -> String { let (y, m, d, _) = civil_from_unix(t); format!("{y}-{m:02}-{d:02}") } @@ -3721,4 +3810,75 @@ mod tests { assert_eq!(seen, vec![0, 1, 2, 3]); } + + /// A controller holding a catalog of four named images. + fn with_catalog() -> Rc { + let ctl = LibraryController::new(crate::activity::ActivityLog::new()); + let catalog = Catalog::in_memory().expect("in-memory catalog"); + let c = catalog.connection(); + c.execute( + "INSERT INTO roots(id, kind, label) VALUES (1, 'remote', 'lib')", + [], + ) + .expect("root"); + for (id, name) in [(1i64, "a.CR2"), (2, "b.CR2"), (3, "c.CR2"), (4, "d.CR2")] { + c.execute( + "INSERT INTO images(id, root_id, source_ref, added_at) VALUES (?1, 1, ?2, 0)", + rusqlite::params![id, name], + ) + .expect("image"); + } + *ctl.catalog.borrow_mut() = Some(catalog); + ctl + } + + /// Just the names, for assertions that are about order rather than ids. + fn named(ctl: &Rc, images: &[dr_types::ImageId]) -> Vec { + ctl.selected_image_paths(images) + .into_iter() + .map(|(_, path)| path) + .collect() + } + + /// TRACES: FR-EXP-7 + #[test] + fn a_selection_resolves_to_paths_in_the_order_it_was_given() { + // `IN (…)` returns rows in whatever order suits SQLite, and an export's + // `{seq}` token counts through the batch — so a lookup that handed back + // the database's order would number the photographs in an order the + // user never saw. + let ctl = with_catalog(); + let paths = named(&ctl, &[dr_types::ImageId(3), dr_types::ImageId(1)]); + assert_eq!(paths, vec!["c.CR2", "a.CR2"]); + } + + /// TRACES: FR-EXP-7 + #[test] + fn a_selection_outside_the_loaded_window_still_resolves() { + // The property the catalog lookup exists for. Selection is by id and + // survives a scrub, so a selection made before scrolling routinely + // names photographs no row holds — and `paths`, the loaded window, is + // empty here precisely to prove nothing is being read from it. + let ctl = with_catalog(); + assert!(ctl.paths.borrow().is_empty()); + assert_eq!(named(&ctl, &[dr_types::ImageId(4)]), ["d.CR2"]); + } + + /// TRACES: FR-EXP-7 + #[test] + fn an_image_that_vanished_under_the_selection_is_skipped() { + // Deleted between the selection and the click. There is nothing to + // export and nothing to fix, so it drops out rather than becoming a + // failure row the user can do nothing about. + let ctl = with_catalog(); + let paths = named( + &ctl, + &[ + dr_types::ImageId(1), + dr_types::ImageId(99), + dr_types::ImageId(2), + ], + ); + assert_eq!(paths, vec!["a.CR2", "b.CR2"]); + } } diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 1be7c1e..463825b 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -634,6 +634,19 @@ in property panel-visible: true; in property export-label: "Export"; in property export-busy: false; in property export-status; + /// TRACES: FR-EXP-7 + /// Whether an export would be queued for the server rather than written to + /// this device. The grid says the same thing as the develop button, but it + /// composes its own label around a selection count — so it is given the + /// fact and not the sentence. + in property export-to-server: false; + /// Whether a batch is running. The grid's export button becomes its cancel + /// button; there is nowhere else a long batch can be stopped from. + in property library-exporting: false; + /// Export the grid's selection in the background (FR-EXP-7). + callback library-export-selection(); + /// TRACES: NFR-ARCH-3 + callback library-cancel-export(); /// Show or hide the develop column. callback toggle-panel(); /// Export the image on screen, using the settings as they stand. @@ -917,6 +930,12 @@ in property panel-visible: true; root.paste-settings-to-selection(); } + // TRACES: FR-EXP-7 + exporting: root.library-exporting; + export-to-server: root.export-to-server; + export-selection => { root.library-export-selection(); } + cancel-export => { root.library-cancel-export(); } + sweep-done: root.library-sweep-done; sweep-total: root.library-sweep-total; diff --git a/ui/dr-ui/ui/library.slint b/ui/dr-ui/ui/library.slint index 39a97be..08d5b70 100644 --- a/ui/dr-ui/ui/library.slint +++ b/ui/dr-ui/ui/library.slint @@ -456,11 +456,17 @@ component HeaderActions inherits HorizontalLayout { /// Whether settings have been copied, and what pasting them would apply. in property settings-armed: false; in property settings-summary; + /// TRACES: FR-EXP-7 + /// Whether a batch is running, and where its files are going. + in property exporting: false; + in property export-to-server: false; /// Centres each button in a 44px header. Off in the disclosure row, which /// is sized to its content. in property centred: true; in property row-height: 44px; + callback export-selection(); + callback cancel-export(); callback paste-settings-to-selection(); callback remove-from-collection(); callback change-library(); @@ -486,6 +492,37 @@ component HeaderActions inherits HorizontalLayout { clicked => { root.paste-settings-to-selection(); } } + // TRACES: FR-EXP-7 | NFR-ARCH-3 + // Export the selection, and stop the batch that is running. + // + // One button doing both, because they are the same thought a moment apart + // and a separate cancel would have to appear from somewhere — shifting the + // row under the pointer at the exact moment the user is reaching for it. + // + // It stays while a batch runs whatever the selection has since become: the + // run is what the button now refers to, and a batch of three hundred that + // could only be stopped by not changing the selection would be a trap. + // + // The count is in the label rather than behind a confirmation, exactly as + // the paste above puts it there: "Export 40" read before the click is worth + // more than a dialogue asking the same question after it. + if root.selected-count > 0 || root.exporting: Button { + text: root.exporting + ? "Cancel export" + : (root.export-to-server + ? "Export " + root.selected-count + " to Nextcloud" + : "Export " + root.selected-count); + active: root.exporting; + y: root.centred ? (root.row-height - self.height) / 2 : 0; + clicked => { + if (root.exporting) { + root.cancel-export(); + } else { + root.export-selection(); + } + } + } + // Removing from a collection is only meaningful while the grid is scoped // to one. Offering it unscoped would invite the reading "remove from the // library", which nothing here does. @@ -753,6 +790,15 @@ export component LibraryGrid inherits Rectangle { in property settings-summary; callback paste-settings-to-selection(); + // TRACES: FR-EXP-7 + // Exporting the selection. The grid owns neither the settings that decide + // where the files go nor the worker that writes them — it reports what is + // selected and asks, exactly as it does for a paste. + in property exporting: false; + in property export-to-server: false; + callback export-selection(); + callback cancel-export(); + // --- the keyboard cursor ------------------------------------------------ // // Where the keyboard is in the library, as an **image ordinal** — not a @@ -891,6 +937,10 @@ export component LibraryGrid inherits Rectangle { scope-pinned: root.scope-pinned; settings-armed: root.settings-armed; settings-summary: root.settings-summary; + exporting: root.exporting; + export-to-server: root.export-to-server; + export-selection => { root.export-selection(); } + cancel-export => { root.cancel-export(); } paste-settings-to-selection => { root.paste-settings-to-selection(); } remove-from-collection => { root.remove-from-collection(); } change-library => { root.change-library(); } @@ -938,6 +988,10 @@ export component LibraryGrid inherits Rectangle { scope-pinned: root.scope-pinned; settings-armed: root.settings-armed; settings-summary: root.settings-summary; + exporting: root.exporting; + export-to-server: root.export-to-server; + export-selection => { root.export-selection(); } + cancel-export => { root.cancel-export(); } paste-settings-to-selection => { root.paste-settings-to-selection(); } remove-from-collection => { root.remove-from-collection(); } change-library => { root.change-library(); }