diff --git a/core/dr-decode/examples/hdr.rs b/core/dr-decode/examples/hdr.rs new file mode 100644 index 0000000..20994b5 --- /dev/null +++ b/core/dr-decode/examples/hdr.rs @@ -0,0 +1,16 @@ +fn main() { + for p in std::env::args().skip(1) { + let Ok(d) = std::fs::read(&p) else { continue }; + let n = p.rsplit('/').next().unwrap(); + // Exactly what the sweep sees: the first HEADER_BYTES only. + let head = &d[..d.len().min(dr_decode::HEADER_BYTES as usize)]; + match dr_decode::metadata(head) { + Ok(m) => println!("{n}: header-only at={:?} model={:?}", m.captured_at, m.model), + Err(e) => println!("{n}: header-only ERROR {e}"), + } + match dr_decode::metadata(&d) { + Ok(m) => println!("{n}: whole-file at={:?}", m.captured_at), + Err(e) => println!("{n}: whole-file ERROR {e}"), + } + } +} diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index 7d7cda0..0956c5f 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -695,15 +695,28 @@ mod tests { #[test] fn zooming_does_not_recompile() { - // The property that makes scroll-wheel zoom smooth: a new zoom level - // is a uniform upload, never a pipeline build. If zoom reached the - // structure hash, every wheel notch would stall on a shader compile. + // The property that makes scroll-wheel zoom smooth: a new zoom *level* + // is a uniform upload, never a pipeline build. If the magnitude reached + // the structure hash, every wheel notch would stall on a compile. + // + // Entering the zoom at all is the one exception, and it is deliberate + // — see `zooming_after_an_unzoomed_render_actually_zooms`. So the walk + // below starts already zoomed, and the count is taken from there. let Some(ctx) = ctx() else { return }; let mut pass = AdjustPass::new(&ctx); let img = split_image(&ctx, false); let mut g = EditGraph::default_chain(); - for (i, extent) in [1.0f32, 0.5, 0.25, 0.125].iter().enumerate() { + g.framing_mut().set_view(dr_pipeline::CropRect { + x: 0.0, + y: 0.0, + width: 0.5, + height: 0.5, + }); + pass.render(&img, &g.compose(), 32, 32).expect("render"); + let baseline = pass.cached_pipelines(); + + for (i, extent) in [0.4f32, 0.25, 0.125].iter().enumerate() { g.framing_mut().set_view(dr_pipeline::CropRect { x: 0.0, y: 0.0, @@ -713,12 +726,59 @@ mod tests { pass.render(&img, &g.compose(), 32, 32).expect("render"); assert_eq!( pass.cached_pipelines(), - 1, + baseline, "zoom step {i} compiled a second pipeline" ); } } + #[test] + fn zooming_after_an_unzoomed_render_actually_zooms() { + // The regression: every earlier zoom test set a view *before* the first + // render, so the first pipeline compiled was already the one carrying + // the crop mapping. Real use is the other way round — the image is + // shown fitted, and only then does the wheel turn. + // + // A neutral framing emits a prologue that never reads `u.crop_rect`. + // While zoom was excluded from the structure hash, that neutral + // pipeline stayed cached under the same key once zoomed, so the view + // uploaded on every frame was read by nobody and the canvas never + // changed. This renders unzoomed first and asserts the pixels move. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = split_image(&ctx, false); + + let mut g = EditGraph::default_chain(); + + // Fitted: the frame spans both halves, so the two edges differ. + let tex = pass.render(&img, &g.compose(), 32, 32).expect("render"); + let fitted_left = read_pixel(&ctx, tex, 4, 16)[0]; + let fitted_right = read_pixel(&ctx, tex, 28, 16)[0]; + assert!( + (i32::from(fitted_left) - i32::from(fitted_right)).abs() > 40, + "the unzoomed frame should span both halves: \ + left={fitted_left} right={fitted_right}" + ); + + // Now zoom into the bright half. Both edges must come up bright. + g.framing_mut().set_view(dr_pipeline::CropRect { + x: 0.0, + y: 0.4, + width: 0.2, + height: 0.2, + }); + let tex = pass.render(&img, &g.compose(), 32, 32).expect("render"); + let zoomed_left = read_pixel(&ctx, tex, 4, 16)[0]; + let zoomed_right = read_pixel(&ctx, tex, 28, 16)[0]; + + assert!( + zoomed_left > 100 && zoomed_right > 100, + "zooming into the bright half after an unzoomed render must show \ + it edge to edge — the neutral pipeline was reused and the view \ + was ignored: left={zoomed_left} right={zoomed_right}" + ); + } + #[test] fn cropping_to_one_half_shows_only_that_half() { // The property a crop exists for, checked against content rather than diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 517a643..67c1036 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -608,12 +608,26 @@ impl Framing { /// the crop handles or the straighten slider must reuse the compiled /// pipeline and upload uniforms only. Only the presence of each /// transform, never its magnitude, may enter this. + /// + /// The last bit is *whether the prologue is emitted at all*, which zoom + /// reaches through [`Self::is_active`]. It has to be here even though zoom + /// is not an edit: the neutral prologue never reads `u.crop_rect`, so a + /// pipeline compiled while unzoomed ignores every later view upload. Two + /// framings that generate different WGSL must not share a cache key — the + /// symptom otherwise is scroll-to-zoom on an otherwise-unedited image + /// doing nothing at all, because the first frame compiled the neutral + /// prologue and the hash never moved off it. + /// + /// What this must *not* do is vary with the zoom level: the bit is set by + /// any zoom and cleared by none, so a wheel notch is still a uniform + /// upload rather than a shader build. pub fn structure_key(&self) -> u64 { u64::from(!self.crop.is_full()) | u64::from(self.angle != 0.0) << 1 | u64::from(self.flip_h) << 2 | u64::from(self.flip_v) << 3 | u64::from(self.quarter_turns) << 4 + | u64::from(self.is_active()) << 6 } } @@ -639,11 +653,15 @@ mod tests { #[test] fn zooming_does_not_change_the_exported_image() { // The property that makes zoom a viewing tool rather than an edit: it - // must not reach the output size, the structure hash, or the crop. - // If it did, exporting while zoomed would write the zoomed view. + // must not reach the output size or the crop. If it did, exporting + // while zoomed would write the zoomed view. + // + // The structure key is deliberately not asserted here — see + // `zooming_from_neutral_changes_the_structure_key` for why it must + // move, and `zoom_level_does_not_change_the_structure_key` for the + // part that must not. let mut f = Framing::new(); let before_size = f.output_size(6000, 4000); - let before_key = f.structure_key(); f.set_view(CropRect { x: 0.25, @@ -653,10 +671,62 @@ mod tests { }); assert_eq!(f.output_size(6000, 4000), before_size, "zoom resized output"); - assert_eq!(f.structure_key(), before_key, "zoom forced a recompile"); assert!(f.crop().is_full(), "zoom altered the crop"); } + #[test] + fn zooming_from_neutral_changes_the_structure_key() { + // The regression this guards: a neutral framing emits a prologue that + // never reads `u.crop_rect`, so if zooming leaves the key alone the + // GPU reuses that pipeline and the uploaded view is ignored — zoom + // silently does nothing on an otherwise-unedited image. + let mut f = Framing::new(); + let neutral = f.structure_key(); + + f.set_view(CropRect { + x: 0.25, + y: 0.25, + width: 0.5, + height: 0.5, + }); + + assert_ne!( + f.structure_key(), + neutral, + "a zoomed framing generates different WGSL and must not share the \ + neutral cache key" + ); + } + + #[test] + fn zoom_level_does_not_change_the_structure_key() { + // The other half of the contract: crossing from unzoomed to zoomed is + // a recompile, but every notch after that is a uniform upload. If the + // magnitude reached the key, every wheel step would stall on a build. + let mut f = Framing::new(); + f.set_view(CropRect { + x: 0.25, + y: 0.25, + width: 0.5, + height: 0.5, + }); + let zoomed = f.structure_key(); + + for extent in [0.4, 0.3, 0.2, 0.1] { + f.set_view(CropRect { + x: 0.1, + y: 0.1, + width: extent, + height: extent, + }); + assert_eq!( + f.structure_key(), + zoomed, + "zoom level {extent} forced a recompile" + ); + } + } + #[test] fn the_view_nests_inside_the_crop() { // Both rects live in the same normalised space and the shader applies diff --git a/core/dr-sync-nextcloud/examples/writetest.rs b/core/dr-sync-nextcloud/examples/writetest.rs index d13a04f..d45471b 100644 --- a/core/dr-sync-nextcloud/examples/writetest.rs +++ b/core/dr-sync-nextcloud/examples/writetest.rs @@ -27,6 +27,45 @@ async fn main() { Err(e) => println!("READ FAILED: {e}"), } + // What is actually in the derived folder on the server? + { + let dir = RemotePath::new(format!("{}/.darkroom-derived", session.root)); + match backend.list(&dir, None).await { + Ok(entries) => { + println!("\n[derived] {} entry/entries on the server:", entries.len()); + for e in &entries { + println!(" {:<28} {:>10} bytes", e.path.name(), e.size); + } + } + Err(e) => println!("\n[derived] listing failed: {e}"), + } + } + + // Probe a file the sweep reported as 423 Locked: is it the file, the + // range request, or the folder? + { + let c = dr_sync_nextcloud::http_client("DarkRoom").unwrap(); + let base = format!("{}/remote.php/dav/files/{}", + creds.server.trim_end_matches('/'), session.user_id); + let f = "PhotosRaw/Darktable/20230629_no_name/20230629_0030.jpeg"; + let enc: String = f.split('/').map(|seg| { + seg.bytes().map(|b| match b { + b'A'..=b'Z'|b'a'..=b'z'|b'0'..=b'9'|b'-'|b'_'|b'.'|b'~' => (b as char).to_string(), + _ => format!("%{b:02X}"), + }).collect::() + }).collect::>().join("/"); + let url = format!("{base}/{enc}"); + + for (what, range) in [("ranged 0-256k", Some("bytes=0-262143")), ("whole file", None)] { + let mut rq = c.get(&url).basic_auth(&creds.login_name, Some(&creds.app_password)); + if let Some(r) = range { rq = rq.header("Range", r); } + match rq.send().await { + Ok(r) => println!("GET {what}: {}", r.status()), + Err(e) => println!("GET {what}: transport {e}"), + } + } + } + // Raw HTTP, to see the status the connector maps away. { let url = format!("{}/remote.php/dav/files/{}/{}/.darkroom-write-test", diff --git a/core/dr-sync-nextcloud/src/lib.rs b/core/dr-sync-nextcloud/src/lib.rs index c0e49ee..e60193c 100644 --- a/core/dr-sync-nextcloud/src/lib.rs +++ b/core/dr-sync-nextcloud/src/lib.rs @@ -92,6 +92,114 @@ impl NextcloudBackend { } } + /// TRACES: FR-NC-7 + /// Upload a large body with chunked upload v2. + /// + /// `MKCOL` an upload directory, `PUT` each chunk into it under a numeric + /// name, then `MOVE` the `.file` pseudo-entry to the destination, which is + /// where the server assembles them. + /// + /// The alternative — refusing anything over the single-shot threshold — + /// is what blocked thumbnail shards from ever reaching the server: they + /// are 25 MB by design. + /// + /// `OC-Total-Length` is sent on every chunk so quota is checked up front + /// rather than at assembly, when the bytes have already been transferred. + async fn put_chunked( + &self, + path: &RemotePath, + body: Vec, + ) -> Result { + let total = body.len() as u64; + // Named from the destination so a resumed or abandoned upload is + // identifiable, and so two uploads cannot collide in one directory. + let token: String = path + .as_str() + .bytes() + .map(|b| match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' => (b as char).to_string(), + _ => "-".to_string(), + }) + .collect(); + let dir = format!( + "{}/remote.php/dav/uploads/{}/{token}", + self.server, self.login + ); + + self.mkcol_url(&dir).await?; + + // Chunks are numbered from 1 and must sort correctly as strings, which + // is why they are zero-padded rather than bare integers. + let chunk_size = CHUNKS.min_chunk as usize; + for (i, chunk) in body.chunks(chunk_size).enumerate() { + if i + 1 > CHUNKS.max_chunks as usize { + return Err(RemoteError::Protocol(format!( + "{total} bytes exceeds {} chunks", CHUNKS.max_chunks + ))); + } + let resp = self + .client + .put(format!("{dir}/{:05}", i + 1)) + .basic_auth(&self.login, Some(&self.password)) + .header("OC-Total-Length", total.to_string()) + .body(chunk.to_vec()) + .send() + .await + .map_err(|e| RemoteError::Network(e.to_string()))?; + map_status(resp.status(), path.as_str())?; + } + + // Assemble. The destination is an absolute URL in the Destination + // header, and `Overwrite: T` because a re-uploaded shard replaces the + // one already there. + let resp = self + .client + .request( + reqwest::Method::from_bytes(b"MOVE").expect("valid method"), + format!("{dir}/.file"), + ) + .basic_auth(&self.login, Some(&self.password)) + .header("Destination", self.url_for(path)) + .header("Overwrite", "T") + .header("OC-Total-Length", total.to_string()) + .send() + .await + .map_err(|e| RemoteError::Network(e.to_string()))?; + map_status(resp.status(), path.as_str())?; + + // The MOVE response carries the assembled file's ETag on Nextcloud, + // but not on every version; fall back to asking rather than failing an + // upload that in fact succeeded. + if let Some(v) = resp + .headers() + .get(reqwest::header::ETAG) + .and_then(|v| v.to_str().ok()) + { + return Ok(Validator::new(v)); + } + self.dir_validator(path).await + } + + /// `MKCOL` at an absolute URL, treating "already there" as success. + async fn mkcol_url(&self, url: &str) -> Result<(), RemoteError> { + let resp = self + .client + .request( + reqwest::Method::from_bytes(b"MKCOL").expect("valid method"), + url, + ) + .basic_auth(&self.login, Some(&self.password)) + .send() + .await + .map_err(|e| RemoteError::Network(e.to_string()))?; + + // 405 is "already exists", which is exactly what we want. + if resp.status() == 405 { + return Ok(()); + } + map_status(resp.status(), url) + } + async fn propfind( &self, path: &RemotePath, @@ -209,10 +317,14 @@ impl RemoteBackend for NextcloudBackend { ) -> Result { // Chunked upload is an implementation detail of put, chosen by size — // exposing it on the trait would leak this protocol (ARCH §8.3). - if body.len() as u64 >= CHUNKS.single_shot_below { - return Err(RemoteError::Unsupported( - "chunked upload v2 not implemented yet", - )); + // + // A precondition cannot ride on a chunked upload: the guard belongs to + // the assembling MOVE, not to the individual chunks, and Nextcloud + // does not honour `If-Match` there. Large bodies are shards and + // catalog snapshots, which are written whole and never merged, so + // there is no conflict to guard against. + if body.len() as u64 >= CHUNKS.single_shot_below && precond.is_none() { + return self.put_chunked(path, body).await; } let mut req = self diff --git a/core/dr-sync/src/error.rs b/core/dr-sync/src/error.rs index 3edeebf..2734a84 100644 --- a/core/dr-sync/src/error.rs +++ b/core/dr-sync/src/error.rs @@ -54,11 +54,35 @@ impl RemoteError { RemoteError::Network(_) => true, RemoteError::Server { status, .. } => { // 5xx and 429 are worth retrying; other 4xx are not. - *status >= 500 || *status == 429 + // + // 423 Locked is the exception, and it is not hypothetical: + // Nextcloud's file locking returns it on a plain *read* under + // concurrency, and the same range re-read seconds later + // succeeds. Treating it as permanent marks an image + // permanently undated over a lock that lasted moments. + *status >= 500 || *status == 429 || *status == 423 } _ => false, } } + + /// Whether this failure means *the server could not be reached*, as + /// opposed to the server answering and refusing. + /// + /// The distinction is the whole basis of offline mode (FR-CAT-9). A 403 + /// and a dead connection are both "the operation failed", but only one of + /// them is fixed by waiting, and only one of them should put the whole app + /// into a degraded mode. Signing the user out — or showing "you are + /// offline" — because a single file was forbidden would be a much worse + /// error than the one it reported. + /// + /// A 5xx is deliberately **not** offline: the server is up and talking, it + /// is just failing, and a retry is the right response rather than a + /// mode change. 429 and 423 likewise — those are the server working + /// correctly under load. + pub fn indicates_offline(&self) -> bool { + matches!(self, RemoteError::Network(_)) + } } #[cfg(test)] @@ -107,4 +131,29 @@ mod tests { "the message must point at permissions, not the login: {denied}" ); } -} \ No newline at end of file + + #[test] + fn a_lock_is_transient() { + // Observed against a real server: 12 concurrent range reads produced + // 423 on some files, and the identical request succeeded moments + // later. Classing it with the permanent 4xx left those images + // undated for good. + assert!(RemoteError::Server { + status: 423, + detail: String::new() + } + .is_transient()); + + // Still permanent, so the exception stays narrow. + assert!(!RemoteError::Server { + status: 404, + detail: String::new() + } + .is_transient()); + assert!(!RemoteError::Server { + status: 400, + detail: String::new() + } + .is_transient()); + } +} diff --git a/core/dr-sync/src/lib.rs b/core/dr-sync/src/lib.rs index 597d33b..b1fd249 100644 --- a/core/dr-sync/src/lib.rs +++ b/core/dr-sync/src/lib.rs @@ -22,11 +22,13 @@ use async_trait::async_trait; pub mod capability; pub mod error; +pub mod reachability; pub mod scan; pub mod types; pub use capability::{Capabilities, ChangeDetection, ChunkConstraints, ServerPreviews}; pub use error::RemoteError; +pub use reachability::{Connectivity, Reachability}; pub use scan::{scan, ScanProgress, ScanResult}; pub use types::{ Cursor, EntryKind, Identity, Precondition, RemoteChange, RemoteEntry, RemoteId, RemotePath, diff --git a/core/dr-sync/src/reachability.rs b/core/dr-sync/src/reachability.rs new file mode 100644 index 0000000..28405c5 --- /dev/null +++ b/core/dr-sync/src/reachability.rs @@ -0,0 +1,335 @@ +//! TRACES: FR-CAT-9 | FR-NC-12 +//! Whether the remote is reachable, and what the app does while it is not. +//! +//! # Why this is a state machine rather than a boolean +//! +//! "Are we online?" cannot be answered by asking the operating system. A +//! laptop with a live wifi association and no route, a captive portal that +//! answers every request with a login page, a Nextcloud instance that is down +//! while the internet is fine — all three report a working network and none of +//! them can serve an image. The only evidence that counts is whether *this +//! backend* answered, so reachability is inferred from the traffic the app was +//! already making rather than probed for separately. +//! +//! That inversion is what keeps the cost at zero. Every remote call already +//! returns a `Result`; [`Reachability::observe`] turns those results into the +//! state, so a library that is browsing happily never issues a probe at all. +//! A probe happens only when something failed and the app wants to know +//! whether it has come back (ARCH §9.0). +//! +//! # Why leaving offline is harder than entering it +//! +//! One failed request is enough to go offline: the user is *already* +//! experiencing the failure, and the honest thing is to say so immediately. +//! But a single success is not enough to declare recovery, because the failure +//! mode that matters — a flapping connection — produces exactly that. So +//! recovery requires a deliberate probe, and the app backs off between +//! attempts rather than retrying in a tight loop against a server that is +//! plainly down. + +use std::time::{Duration, Instant}; + +use crate::error::RemoteError; + +/// How long to wait before the first reconnection probe. +/// +/// Short enough that a brief drop — a laptop changing access points, a phone +/// moving between cells — recovers before the user has finished noticing it. +const FIRST_BACKOFF: Duration = Duration::from_secs(5); + +/// The longest gap between probes. +/// +/// Capped rather than growing without bound: a user who left the app open +/// overnight on a dead connection should reconnect within a minute of the +/// server returning, not hours later because the backoff had doubled its way +/// into the distance. +const MAX_BACKOFF: Duration = Duration::from_secs(60); + +/// What the app currently believes about the remote. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Connectivity { + /// The remote answered the last time anything asked. + Online, + /// The remote could not be reached. The library runs from local data. + Offline, +} + +impl Connectivity { + pub fn is_online(self) -> bool { + matches!(self, Connectivity::Online) + } + + pub fn is_offline(self) -> bool { + matches!(self, Connectivity::Offline) + } +} + +/// Tracks reachability from observed request outcomes. +/// +/// Cheap to construct and `Clone`-free by design — one lives beside the +/// session and every worker reports into it. +#[derive(Debug)] +pub struct Reachability { + state: Connectivity, + /// When the app went offline. Shown to the user, because "offline" without + /// "since when" leaves them unable to tell a momentary drop from a + /// connection that died an hour ago. + since: Option, + /// How long to wait before the next probe, doubling per failure. + backoff: Duration, + /// When the next probe becomes worthwhile. + next_probe: Option, + /// Why we think we are offline, for the banner. The underlying transport + /// message, which is usually specific enough to be actionable ("dns error", + /// "connection refused"). + reason: Option, +} + +impl Default for Reachability { + fn default() -> Self { + Self::new() + } +} + +impl Reachability { + /// Start optimistic. + /// + /// Assuming online until proven otherwise is deliberate: the alternative + /// is a probe on every launch, which makes startup wait on the network for + /// a library that may be entirely cached. The first real request settles + /// it either way, and settles it with evidence. + pub fn new() -> Self { + Self { + state: Connectivity::Online, + since: None, + backoff: FIRST_BACKOFF, + next_probe: None, + reason: None, + } + } + + pub fn state(&self) -> Connectivity { + self.state + } + + pub fn is_offline(&self) -> bool { + self.state.is_offline() + } + + /// Why the app believes it is offline, if it does. + pub fn reason(&self) -> Option<&str> { + self.reason.as_deref() + } + + /// How long the app has been offline. + pub fn offline_for(&self, now: Instant) -> Option { + self.since.map(|t| now.saturating_duration_since(t)) + } + + /// Record the outcome of a remote call. + /// + /// Returns `true` if the connectivity state *changed*, so the caller can + /// repaint a banner or kick off a rescan without diffing the state itself. + /// + /// Takes the result by reference so callers can report an outcome they are + /// still going to use — this observes, it never consumes. + pub fn observe(&mut self, outcome: &Result, now: Instant) -> bool { + match outcome { + Ok(_) => self.mark_reachable(now), + Err(e) if e.indicates_offline() => self.mark_unreachable(e.to_string(), now), + // The server answered. Whatever went wrong is not connectivity, so + // it must not move this state — a 404 on one file says nothing + // about the other 17,000. + Err(_) => false, + } + } + + /// Record that the remote answered. + pub fn mark_reachable(&mut self, _now: Instant) -> bool { + let changed = self.state.is_offline(); + self.state = Connectivity::Online; + self.since = None; + self.backoff = FIRST_BACKOFF; + self.next_probe = None; + self.reason = None; + changed + } + + /// Record that the remote could not be reached. + /// + /// Repeated calls while already offline extend the backoff rather than + /// resetting it, so a library with twelve workers all failing at once + /// does not schedule twelve immediate probes. + pub fn mark_unreachable(&mut self, reason: String, now: Instant) -> bool { + let changed = self.state.is_online(); + if changed { + self.state = Connectivity::Offline; + self.since = Some(now); + self.backoff = FIRST_BACKOFF; + } else { + self.backoff = (self.backoff * 2).min(MAX_BACKOFF); + } + self.next_probe = Some(now + self.backoff); + self.reason = Some(reason); + changed + } + + /// Whether enough time has passed to be worth trying the remote again. + /// + /// Always false while online — there is nothing to probe for. + pub fn should_probe(&self, now: Instant) -> bool { + self.state.is_offline() && self.next_probe.is_some_and(|t| now >= t) + } + + /// How long until the next probe is due, for a countdown in the banner. + pub fn until_probe(&self, now: Instant) -> Option { + self.next_probe.map(|t| t.saturating_duration_since(now)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn network() -> RemoteError { + RemoteError::Network("connection refused".into()) + } + + #[test] + fn starts_online_without_probing() { + // Launch must not wait on the network: a fully cached library opens + // with no request at all, and an optimistic start is what allows that. + let r = Reachability::new(); + assert!(r.state().is_online()); + assert!(!r.should_probe(Instant::now())); + } + + #[test] + fn one_network_failure_goes_offline() { + let mut r = Reachability::new(); + let now = Instant::now(); + let changed = r.observe::<()>(&Err(network()), now); + assert!(changed, "the first failure is a state change"); + assert!(r.is_offline()); + assert_eq!(r.reason(), Some("network error: connection refused")); + } + + #[test] + fn a_server_error_is_not_offline() { + // The distinction the whole mode rests on: the server answered, so it + // is reachable. Going offline here would blank the grid over one + // forbidden file. + let mut r = Reachability::new(); + let now = Instant::now(); + + for e in [ + RemoteError::PermissionDenied, + RemoteError::NotFound("a.CR2".into()), + RemoteError::AuthFailed, + RemoteError::Server { + status: 500, + detail: String::new(), + }, + RemoteError::Server { + status: 423, + detail: String::new(), + }, + ] { + let mut probe = Reachability::new(); + assert!(!probe.observe::<()>(&Err(e), now)); + assert!(probe.state().is_online()); + } + + assert!(!r.observe::<()>(&Err(RemoteError::PermissionDenied), now)); + assert!(r.state().is_online()); + } + + #[test] + fn success_brings_it_back() { + let mut r = Reachability::new(); + let now = Instant::now(); + r.observe::<()>(&Err(network()), now); + assert!(r.is_offline()); + + let changed = r.observe(&Ok(()), now); + assert!(changed, "recovery is a state change"); + assert!(r.state().is_online()); + assert_eq!(r.reason(), None); + } + + #[test] + fn repeated_failures_back_off_rather_than_reset() { + // Twelve sweep lanes failing together must not schedule twelve + // immediate probes against a server that is plainly down. + let mut r = Reachability::new(); + let now = Instant::now(); + + assert!(r.observe::<()>(&Err(network()), now)); + let first = r.until_probe(now).unwrap(); + + assert!(!r.observe::<()>(&Err(network()), now), "already offline"); + let second = r.until_probe(now).unwrap(); + + assert!( + second > first, + "backoff must grow: {second:?} should exceed {first:?}" + ); + } + + #[test] + fn backoff_is_capped() { + let mut r = Reachability::new(); + let now = Instant::now(); + for _ in 0..20 { + r.observe::<()>(&Err(network()), now); + } + assert!( + r.until_probe(now).unwrap() <= MAX_BACKOFF, + "an app left overnight must still reconnect promptly" + ); + } + + #[test] + fn probing_waits_for_the_backoff() { + let mut r = Reachability::new(); + let now = Instant::now(); + r.observe::<()>(&Err(network()), now); + + assert!(!r.should_probe(now), "not immediately"); + assert!(r.should_probe(now + FIRST_BACKOFF)); + } + + #[test] + fn recovery_resets_the_backoff() { + // Otherwise a connection that flaps all day arrives at the maximum + // backoff and stays there, so the next real drop takes a minute to + // notice recovery. + let mut r = Reachability::new(); + let now = Instant::now(); + for _ in 0..5 { + r.observe::<()>(&Err(network()), now); + } + r.observe(&Ok(()), now); + r.observe::<()>(&Err(network()), now); + + assert_eq!(r.until_probe(now), Some(FIRST_BACKOFF)); + } + + #[test] + fn offline_duration_is_measured_from_the_first_failure() { + // Not from the most recent one: a connection that has been down for an + // hour must not report five seconds because a worker retried. + let mut r = Reachability::new(); + let start = Instant::now(); + r.observe::<()>(&Err(network()), start); + + let later = start + Duration::from_secs(600); + r.observe::<()>(&Err(network()), later); + + let elapsed = r.offline_for(later).expect("offline since the first failure"); + assert!( + elapsed >= Duration::from_secs(600), + "measured from the first failure, got {elapsed:?}" + ); + } +} diff --git a/core/dr-thumbs/src/lib.rs b/core/dr-thumbs/src/lib.rs index 49568b1..3986af8 100644 --- a/core/dr-thumbs/src/lib.rs +++ b/core/dr-thumbs/src/lib.rs @@ -329,6 +329,12 @@ impl ThumbStore { if create { conn.execute_batch(SHARD_SCHEMA)?; } + // Every shard carries its own `thumbs` table, so migrating the index + // alone is not enough — and `CREATE TABLE IF NOT EXISTS` leaves an + // existing one untouched, so a shard written before the size class + // keeps the old shape and every write to it fails with "no column + // named size". + migrate_size_column(&conn, "thumbs")?; Ok(conn) } @@ -794,6 +800,53 @@ mod tests { assert_eq!(ThumbSize::for_cell(400), ThumbSize::Large); } + + #[test] + fn a_shard_written_before_the_size_class_accepts_new_thumbnails() { + // The index is not the only table with a `size` column: every shard + // carries its own `thumbs`. Migrating the index alone left existing + // shards in the old shape, and `CREATE TABLE IF NOT EXISTS` will not + // fix one — so every write failed with "no column named size" and the + // store silently stopped accepting thumbnails. + let dir = std::env::temp_dir().join(format!("dr-thumbs-shardmig-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + // A shard in the old shape, holding one thumbnail. + { + let c = Connection::open(dir.join("shard-0000.sqlite")).unwrap(); + c.execute_batch( + "CREATE TABLE thumbs ( + file_id INTEGER PRIMARY KEY, width INTEGER NOT NULL, + height INTEGER NOT NULL, bytes BLOB NOT NULL); + INSERT INTO thumbs VALUES (42, 8, 8, x'FFD8FFD9');", + ) + .unwrap(); + } + { + let c = Connection::open(dir.join("index.sqlite")).unwrap(); + c.execute_batch( + "CREATE TABLE entries ( + file_id INTEGER PRIMARY KEY, shard INTEGER NOT NULL, bytes INTEGER NOT NULL); + CREATE TABLE shards ( + id INTEGER PRIMARY KEY, bytes INTEGER NOT NULL DEFAULT 0, + sealed INTEGER NOT NULL DEFAULT 0); + INSERT INTO shards(id, bytes, sealed) VALUES (0, 4, 0); + INSERT INTO entries(file_id, shard, bytes) VALUES (42, 0, 4);", + ) + .unwrap(); + } + + let mut s = ThumbStore::open(&dir).unwrap(); + + // The old thumbnail survives as grid-sized... + assert!(s.get(42, ThumbSize::Grid).unwrap().is_some()); + // ...and the shard now accepts new writes rather than rejecting them. + s.put(43, ThumbSize::Grid, &thumb(64)) + .expect("a migrated shard must accept writes"); + assert!(s.contains(43, ThumbSize::Grid)); + } + #[test] fn a_store_written_before_the_size_class_keeps_its_thumbnails() { // The migration case: an existing library must not lose the thumbnails diff --git a/core/dr-types/src/selector.rs b/core/dr-types/src/selector.rs index 9f6a5c7..325cf50 100644 --- a/core/dr-types/src/selector.rs +++ b/core/dr-types/src/selector.rs @@ -154,6 +154,36 @@ pub enum Tier { Original, } +impl Tier { + /// The value stored in `image_cache.tier_actual` / `tier_desired`. + /// + /// Written out explicitly rather than derived from the discriminant: these + /// integers are **on disk**, so reordering the variants — which the `Ord` + /// derive above openly invites, since generosity ordering is the point — + /// would silently reinterpret every existing row. The `from_stored` round + /// trip below is what holds the two in agreement. + pub fn stored(self) -> i64 { + match self { + Tier::Metadata => 0, + Tier::Preview => 1, + Tier::Original => 2, + } + } + + /// Read a tier back from the catalog. + /// + /// An unknown value reads as [`Tier::Metadata`] — the tier that promises + /// nothing — so a catalog written by a newer version degrades to "not + /// cached" rather than claiming to hold pixels it does not have. + pub fn from_stored(v: i64) -> Self { + match v { + 2 => Tier::Original, + 1 => Tier::Preview, + _ => Tier::Metadata, + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -202,6 +232,38 @@ mod tests { assert_eq!(found, vec![CollectionId(1), CollectionId(2)]); } + #[test] + #[test] + fn stored_tiers_round_trip() { + // These integers are on disk. A reordering of the variants that broke + // this test would silently reinterpret every cached row as a different + // tier — an image recorded as holding its original would come back + // claiming metadata, or worse, the reverse. + for t in [Tier::Metadata, Tier::Preview, Tier::Original] { + assert_eq!(Tier::from_stored(t.stored()), t); + } + assert_eq!(Tier::Metadata.stored(), 0); + assert_eq!(Tier::Preview.stored(), 1); + assert_eq!(Tier::Original.stored(), 2); + } + + #[test] + fn an_unknown_stored_tier_promises_nothing() { + // A catalog written by a newer version must not have its unknown tier + // read as "the original is here"; the safe direction is downwards. + assert_eq!(Tier::from_stored(99), Tier::Metadata); + assert_eq!(Tier::from_stored(-1), Tier::Metadata); + } + + #[test] + fn stored_order_matches_generosity_order() { + // The SQL predicates compare `tier_actual >= n`, so the stored + // integers must sort the same way the enum does or a ">= Original" + // query would match a Preview row. + assert!(Tier::Metadata.stored() < Tier::Preview.stored()); + assert!(Tier::Preview.stored() < Tier::Original.stored()); + } + #[test] fn tiers_order_by_generosity() { // ARCH §9.3: where rules disagree, the most generous wins, which is diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index a6ecd95..edd24b2 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -70,6 +70,19 @@ impl DevelopSession { pub fn rows(&self) -> Vec { let mut rows = Vec::new(); for (op_index, op) in self.graph.capabilities().iter().enumerate() { + // Framing has a panel of its own. + // + // The one place this side names a stage, and the exception proves + // the rule: every *other* operation is rendered from its + // descriptor alone. Framing is skipped because its parameters are + // not sliders in any useful sense — four crop edges are dragged on + // the photograph and a quarter turn is a button — so it is + // presented by `GeometryPanel` instead of generated here. Emitting + // both would show the same eight values twice, in one good control + // surface and one bad one. + if op.id == dr_pipeline::framing::ID { + continue; + } // Where this operation's rows begin. The panel groups by walking // back to it, so it has to be taken before any row is pushed. let group_head = rows.len(); @@ -406,6 +419,37 @@ impl DevelopSession { self.demosaiced.size() } + /// Whether one source pixel now covers more than one screen pixel. + /// + /// The question the interface asks to decide how the canvas is *filtered*, + /// not how it is rendered. Below 1:1 there are more source pixels than + /// screen pixels and smoothing is what stops the image aliasing; past it + /// there is no more detail to show, and smoothing only invents values + /// between real ones — at which point a photographer inspecting focus or + /// noise wants to see the pixels, not a blur of them. + /// + /// Measured against the visible region rather than the zoom factor alone, + /// because the two differ: a 24 MP file in a 1200px viewport is still + /// showing five sensor pixels per screen pixel at 4×, while a small JPEG is + /// already magnified at 1×. + pub fn magnifies_source(&self, viewport_w: u32, viewport_h: u32) -> bool { + let (sw, sh) = self.demosaiced.size(); + let (fw, fh) = self.graph.output_size(sw, sh); + let (rw, rh) = fit(fw, fh, viewport_w.max(1), viewport_h.max(1)); + + // How many source pixels lie behind the render target: the framed + // image narrowed to the region the view selects. The target keeps its + // size while that region shrinks, which is what raises the ratio. + let view = self.graph.framing().view(); + let behind_w = f64::from(fw) * f64::from(view.width.max(f32::EPSILON)); + let behind_h = f64::from(fh) * f64::from(view.height.max(f32::EPSILON)); + + // Strictly greater, with a margin: at exactly 1:1 either filter gives + // the same answer, and flipping mode on a rounding error would make the + // canvas visibly change character mid-scroll. + f64::from(rw) > behind_w * 1.001 && f64::from(rh) > behind_h * 1.001 + } + /// Set the crop rectangle, in fractions of the source. pub fn set_crop(&mut self, rect: CropRect) { self.graph.set_crop(rect); @@ -416,10 +460,75 @@ impl DevelopSession { } /// Rotate by quarter turns, wrapping. The rotate-left/right buttons. + /// + /// The crop travels with the frame rather than staying where it was on + /// screen. A crop is a decision about *this part of the photograph*, and + /// leaving the rect in place while the image turns under it would move the + /// selection onto a different part of the picture — so the rect is turned + /// by the same quarter and the composition survives the rotation. pub fn rotate_quarters(&mut self, turns: i32) { + let crop = self.graph.crop(); + if !crop.is_full() { + self.graph.set_crop(rotate_crop(crop, turns)); + } self.graph.rotate_quarters(turns); } + /// Straightening, in degrees. Positive turns the image clockwise. + pub fn angle(&self) -> f32 { + self.graph.framing().angle() + } + + /// Quarter turns clockwise, 0..=3 — for the panel's readout. + pub fn quarter_turns(&self) -> u8 { + self.graph.framing().quarter_turns() + } + + pub fn flips(&self) -> (bool, bool) { + self.graph.framing().flips() + } + + /// Mirror horizontally, about the frame's vertical centre line. + pub fn toggle_flip_h(&mut self) { + let (h, _) = self.graph.framing().flips(); + self.graph + .set_param(dr_pipeline::framing::ID, dr_pipeline::framing::FLIP_H, f32::from(u8::from(!h))); + } + + pub fn toggle_flip_v(&mut self) { + let (_, v) = self.graph.framing().flips(); + self.graph + .set_param(dr_pipeline::framing::ID, dr_pipeline::framing::FLIP_V, f32::from(u8::from(!v))); + } + + /// Set the straightening angle, in degrees. + pub fn set_angle(&mut self, degrees: f32) { + self.graph + .set_param(dr_pipeline::framing::ID, dr_pipeline::framing::ANGLE, degrees); + } + + /// Whether the framing currently changes the image — what lights the + /// section's modified dot and enables its reset. + /// + /// Asks whether it *edits*, not whether it is active: a zoomed view makes + /// the framing active without changing the photograph, and a section that + /// claimed an edit because the user scrolled would be lying. + pub fn framing_edits_image(&self) -> bool { + self.graph.framing().edits_image() + } + + /// Return crop, straightening, rotation and flips to neutral, leaving + /// every colour adjustment alone. + /// + /// The zoom is deliberately preserved: it is a viewing state, and resetting + /// the framing is an edit, so throwing away where the user was looking + /// would be an unrelated second effect. + pub fn reset_framing(&mut self) { + let view = self.graph.framing().view(); + self.graph.framing_mut().reset(); + self.graph.framing_mut().set_view(view); + } + /// How far the viewport is zoomed in: 1.0 fits the frame, 4.0 is 4×. pub fn zoom(&self) -> f32 { let v = self.graph.framing().view(); @@ -520,6 +629,31 @@ impl DevelopSession { } } +/// Re-express a crop rect after the frame it is measured against turns. +/// +/// The crop lives in fractions of the *framed* image — the one the quarter +/// turns have already produced — so turning the frame another quarter leaves +/// the rect describing the wrong region unless it turns with it. Without this, +/// rotating a portrait crop on a landscape photograph slides the selection +/// onto a different part of the picture, which reads as the rotation having +/// moved the image rather than the frame. +/// +/// One clockwise quarter takes `(x, y)` to `(1 - y - h, x)` and exchanges the +/// extents; anticlockwise is the same map run the other way. Applied +/// `turns.rem_euclid(4)` times so the caller's wrapping and this agree. +fn rotate_crop(rect: CropRect, turns: i32) -> CropRect { + let mut r = rect; + for _ in 0..turns.rem_euclid(4) { + r = CropRect { + x: 1.0 - r.y - r.height, + y: r.x, + width: r.height, + height: r.width, + }; + } + r.normalised() +} + /// Sort ascending and force a minimum separation. /// /// Mirrors what the curve operation does before handing points to the @@ -577,6 +711,103 @@ mod tests { use super::*; use dr_pipeline::EditGraph; + /// The whole scroll-to-zoom path, end to end, in the order the user drives + /// it: show the image fitted, *then* turn the wheel. + /// + /// The lower layers each had zoom tests and each passed while this was + /// broken, because every one of them set a view before its first render. + /// That ordering hid the bug — a neutral framing compiles a prologue that + /// never reads the crop rect, and while zoom was absent from the structure + /// hash that pipeline stayed cached once zoomed. The session reported the + /// new zoom, the uniforms carried the new view, and the pixels never moved. + /// + /// So this asserts on the rendered pixels rather than on `zoom()`: the + /// symptom was precisely that the state was right and the image was not. + #[test] + fn zooming_after_a_fitted_render_changes_the_pixels() { + let Ok(ctx) = pollster::block_on(dr_gpu::GpuContext::new_headless()) else { + log::warn!("no GPU adapter; skipping"); + return; + }; + + // A gradient, so any change in the sampled region moves the pixels. + let (w, h) = (64u32, 64u32); + let mut rgba = Vec::with_capacity((w * h * 4) as usize); + for y in 0..h { + for x in 0..w { + rgba.extend_from_slice(&[(x * 4) as u8, (y * 4) as u8, 128, 255]); + } + } + let mut session = DevelopSession::open_rgb(&ctx, &rgba, w, h).expect("session"); + + let fitted = session.render(64, 64).expect("fitted render"); + session.zoom_about(4.0, 0.5, 0.5); + assert!(session.is_zoomed(), "the session did not register the zoom"); + let zoomed = session.render(64, 64).expect("zoomed render"); + + let before = fitted.to_rgba8().expect("fitted pixels"); + let after = zoomed.to_rgba8().expect("zoomed pixels"); + let differing = before + .as_bytes() + .iter() + .zip(after.as_bytes().iter()) + .filter(|(a, b)| a != b) + .count(); + + assert!( + differing > 0, + "zooming 4x after a fitted render produced identical pixels — the \ + view reached the session but not the shader" + ); + } + + #[test] + fn magnification_follows_the_source_resolution_and_not_the_zoom_factor() { + // What decides whether the canvas is filtered. The distinction this + // guards is the reason the interface cannot answer it from `zoom()` + // alone: the same 4x on a large source is still showing more source + // pixels than screen pixels, while on a small one it is already + // inventing values between them. + let Ok(ctx) = pollster::block_on(dr_gpu::GpuContext::new_headless()) else { + log::warn!("no GPU adapter; skipping"); + return; + }; + + // Bigger than the viewport it is shown in: `fit` scales it down, so + // every screen pixel still has several source pixels behind it. + let big = vec![128u8; (800 * 800 * 4) as usize]; + let mut session = DevelopSession::open_rgb(&ctx, &big, 800, 800).expect("session"); + assert!( + !session.magnifies_source(200, 200), + "a downscaled image is not magnified" + ); + session.zoom_about(2.0, 0.5, 0.5); + assert!( + !session.magnifies_source(200, 200), + "2x on a 4x-downscaled source is still below 1:1" + ); + session.zoom_about(8.0, 0.5, 0.5); + assert!( + session.magnifies_source(200, 200), + "16x on a 4x-downscaled source magnifies and must not be filtered" + ); + + // Smaller than the viewport: `fit` refuses to upscale, so the render is + // 1:1 and unzoomed is exactly the boundary — not past it. + let small = vec![128u8; (100 * 100 * 4) as usize]; + let mut session = DevelopSession::open_rgb(&ctx, &small, 100, 100).expect("session"); + assert!( + !session.magnifies_source(800, 800), + "1:1 is the boundary, not past it — filtering must not flip on a \ + rounding error" + ); + session.zoom_about(2.0, 0.5, 0.5); + assert!( + session.magnifies_source(800, 800), + "any zoom past a 1:1 render magnifies" + ); + } + #[test] fn every_capability_becomes_exactly_one_row() { // The UI shows what the pipeline offers — no more, and nothing @@ -616,7 +847,62 @@ mod tests { heads += 1; } } - assert_eq!(heads, caps.len()); + // Every operation but framing, which has its own panel. + let generated = caps + .iter() + .filter(|c| c.id != dr_pipeline::framing::ID) + .count(); + assert_eq!(heads, generated); + } + + #[test] + fn framing_is_not_generated_as_sliders() { + // The geometry panel presents crop, rotation, flips and straightening + // as the gestures they are. If the generic path emitted them too, the + // sidebar would carry both — including four "Crop Left/Top/Width/ + // Height" sliders that no one can compose a photograph with. + let graph = EditGraph::default_chain(); + let caps = graph.capabilities(); + assert!( + caps.iter().any(|c| c.id == dr_pipeline::framing::ID), + "the chain must still expose framing — the panel reads it" + ); + + // Asserted through the row count rather than by inspecting labels: a + // leaked framing group would add its eight parameters as eight rows, + // and the difference is exactly what `rows_of` must not contain. + let framing_params = caps + .iter() + .find(|c| c.id == dr_pipeline::framing::ID) + .map(|c| c.params.len()) + .expect("framing is in the chain"); + assert!(framing_params > 0); + + let generated = rows_of(&caps).len(); + let with_framing = rows_of_unfiltered(&caps).len(); + assert_eq!( + with_framing - generated, + framing_params, + "framing parameters leaked into the generated panel" + ); + } + + /// `rows_of` without the framing skip — the shape the panel would have if + /// framing were generated, which is what the test above measures against. + fn rows_of_unfiltered(caps: &[OpCapability]) -> Vec<(usize, usize)> { + let mut rows = Vec::new(); + for op in caps { + let head = rows.len(); + let collapses = op + .presentation + .as_ref() + .is_some_and(|p| p.params.len() == op.params.len()); + let len = if collapses { 1 } else { op.params.len() }; + for _ in 0..len { + rows.push((head, len)); + } + } + rows } #[test] @@ -702,6 +988,12 @@ mod tests { fn rows_of(caps: &[OpCapability]) -> Vec<(usize, usize)> { let mut rows = Vec::new(); for op in caps { + // Framing is presented by `GeometryPanel`, not generated — mirror + // the skip, or these tests assert against a panel that is not the + // one the interface builds. + if op.id == dr_pipeline::framing::ID { + continue; + } let head = rows.len(); let collapses = op .presentation @@ -715,6 +1007,114 @@ mod tests { rows } + #[test] + fn four_quarter_turns_return_a_crop_where_it_started() { + // The property that makes rotation safe to repeat: a user who turns + // past the orientation they wanted and keeps going must arrive back at + // the crop they had, not at a slowly drifting one. + let start = CropRect { + x: 0.1, + y: 0.2, + width: 0.3, + height: 0.4, + }; + let mut r = start; + for _ in 0..4 { + r = rotate_crop(r, 1); + } + assert!((r.x - start.x).abs() < 1e-5, "x drifted to {}", r.x); + assert!((r.y - start.y).abs() < 1e-5, "y drifted to {}", r.y); + assert!((r.width - start.width).abs() < 1e-5); + assert!((r.height - start.height).abs() < 1e-5); + } + + #[test] + fn a_quarter_turn_exchanges_a_crops_extents() { + // A portrait selection on a landscape frame must come out landscape. + // Were the extents left alone, the rect would keep its old shape while + // the frame changed to the other one, and the crop would spill off the + // photograph. + let r = rotate_crop( + CropRect { + x: 0.0, + y: 0.0, + width: 0.25, + height: 1.0, + }, + 1, + ); + assert!((r.width - 1.0).abs() < 1e-5, "width was {}", r.width); + assert!((r.height - 0.25).abs() < 1e-5, "height was {}", r.height); + } + + #[test] + fn rotating_a_crop_keeps_it_inside_the_frame() { + // Whatever the angle and wherever the rect, the result must still be a + // rect the pipeline can render: outside the unit square it would + // sample undefined area, and degenerate it is a zero-sized texture. + for turns in -5..=5 { + for rect in [ + CropRect { + x: 0.0, + y: 0.0, + width: 1.0, + height: 1.0, + }, + CropRect { + x: 0.7, + y: 0.8, + width: 0.3, + height: 0.2, + }, + CropRect { + x: 0.0, + y: 0.45, + width: 0.02, + height: 0.02, + }, + ] { + let r = rotate_crop(rect, turns); + assert!( + r.x >= 0.0 && r.y >= 0.0, + "{turns} turns of {rect:?} gave {r:?}" + ); + assert!( + r.x + r.width <= 1.0 + 1e-5 && r.y + r.height <= 1.0 + 1e-5, + "{turns} turns of {rect:?} left the frame: {r:?}" + ); + assert!( + r.width >= CropRect::MIN_EXTENT && r.height >= CropRect::MIN_EXTENT, + "{turns} turns of {rect:?} went degenerate: {r:?}" + ); + } + } + } + + #[test] + fn opposite_quarter_turns_cancel() { + // The rotate-left and rotate-right buttons must undo one another, or + // correcting an over-rotation would land somewhere new each time. + let start = CropRect { + x: 0.15, + y: 0.05, + width: 0.5, + height: 0.25, + }; + let there_and_back = rotate_crop(rotate_crop(start, 1), -1); + assert!((there_and_back.x - start.x).abs() < 1e-5); + assert!((there_and_back.y - start.y).abs() < 1e-5); + assert!((there_and_back.width - start.width).abs() < 1e-5); + assert!((there_and_back.height - start.height).abs() < 1e-5); + } + + #[test] + fn a_full_crop_survives_rotation_as_a_full_crop() { + // The common case: rotating an uncropped photograph must not quietly + // introduce a crop, which would shrink the exported image. + assert!(rotate_crop(CropRect::default(), 1).is_full()); + assert!(rotate_crop(CropRect::default(), -3).is_full()); + } + #[test] fn unit_suffixes_come_from_the_descriptor() { assert_eq!(unit_suffix(Unit::Stops), " EV"); diff --git a/ui/dr-ui/src/launch_ui.rs b/ui/dr-ui/src/launch_ui.rs index 91bc959..5aafaea 100644 --- a/ui/dr-ui/src/launch_ui.rs +++ b/ui/dr-ui/src/launch_ui.rs @@ -95,11 +95,13 @@ where let weak = window.as_weak(); let ctl = controller.clone(); window.on_launch_sign_in(move |server| { + log::info!("sign-in requested for {server:?}"); let Some(w) = weak.upgrade() else { return }; ctl.model.borrow_mut().begin_sign_in(server.to_string()); render(&w, &ctl); let server = ctl.model.borrow().server_url.clone(); + log::info!("starting login flow against {server}"); spawn_login(w.as_weak(), ctl.clone(), server); }); } @@ -261,17 +263,53 @@ fn spawn_login(weak: slint::Weak, ctl: Rc, server: let (tx, rx) = std::sync::mpsc::channel::(); std::thread::spawn(move || { - let rt = match tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - { - Ok(rt) => rt, - Err(e) => { - let _ = tx.send(LoginMessage::Failed(e.to_string())); - return; - } - }; + // A panic anywhere below would unwind the thread, drop `tx`, and leave + // the UI with nothing but a closed channel — which it can only report + // as "failed unexpectedly", losing the one piece of information that + // would explain the failure. Catch it and forward the message instead. + let panic_tx = tx.clone(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || { + // `enable_all()` also enables the signal driver, which wants to own + // process-wide signal handling and is not something a worker thread + // inside an Android app can safely claim — the flow needs only IO + // (reqwest) and time (the poll interval), so ask for just those. + let rt = match tokio::runtime::Builder::new_current_thread() + .enable_io() + .enable_time() + .build() + { + Ok(rt) => rt, + Err(e) => { + let _ = tx.send(LoginMessage::Failed(e.to_string())); + return; + } + }; + run_login_flow(rt, tx, server); + })); + + if let Err(panic) = result { + let detail = panic + .downcast_ref::<&str>() + .map(|s| (*s).to_string()) + .or_else(|| panic.downcast_ref::().cloned()) + .unwrap_or_else(|| "panicked with a non-string payload".to_string()); + log::error!("login worker panicked: {detail}"); + let _ = panic_tx.send(LoginMessage::Failed(format!("internal error: {detail}"))); + } + }); + + poll_channel(weak, ctl, rx); +} + +/// The body of the login flow, split out so the worker above can wrap it in +/// `catch_unwind` without a deeply nested closure. +fn run_login_flow( + rt: tokio::runtime::Runtime, + tx: std::sync::mpsc::Sender, + server: String, +) { + { rt.block_on(async { let client = match dr_sync_nextcloud::http_client("DarkRoom") { Ok(c) => c, @@ -281,13 +319,16 @@ fn spawn_login(weak: slint::Weak, ctl: Rc, server: } }; + log::info!("POST {server}/index.php/login/v2"); let flow = match auth::begin(&client, &server, "DarkRoom").await { Ok(f) => f, Err(e) => { + log::warn!("login flow could not be started: {e}"); let _ = tx.send(LoginMessage::Failed(e.to_string())); return; } }; + log::info!("login flow started; opening browser"); // Open the system browser, never an embedded webview (FR-NC-1). // @@ -295,11 +336,13 @@ fn spawn_login(weak: slint::Weak, ctl: Rc, server: // that only the browser can give, so carrying on would hang until // the flow expired and then report nothing useful. if let Err(e) = open_in_browser(&flow.login_url) { + log::warn!("browser launch failed: {e}"); let _ = tx.send(LoginMessage::Failed(format!( "could not open a browser to approve the sign-in: {e}" ))); return; } + log::info!("browser opened; polling for approval"); let _ = tx.send(LoginMessage::AwaitingApproval(flow.login_url.clone())); match auth::poll(&client, &flow).await { @@ -314,9 +357,7 @@ fn spawn_login(weak: slint::Weak, ctl: Rc, server: } } }); - }); - - poll_channel(weak, ctl, rx); + } } enum LoginMessage { @@ -353,7 +394,13 @@ fn poll_channel( // Only an error if the flow never completed; a // successful login stops the timer below. if !ctl.model.borrow().is_signed_in() { - ctl.model.borrow_mut().fail("sign-in failed unexpectedly"); + // Reaching here means the worker ended without + // sending anything, which `catch_unwind` in + // spawn_login should now prevent — so say that the + // worker stopped rather than blaming the sign-in. + ctl.model + .borrow_mut() + .fail("the sign-in worker stopped without reporting why"); render(&w, ctl); } if let Some(t) = ctl.poll_timer.borrow().as_ref() { diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index 2e0452b..50d75e4 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -229,6 +229,42 @@ fn reset_view_state(window: &AppWindow) { window.set_crop_y(0.0); window.set_crop_w(1.0); window.set_crop_h(1.0); + window.set_max_straighten(dr_pipeline::framing::MAX_STRAIGHTEN); + window.set_straighten(0.0); + window.set_flip_h(false); + window.set_flip_v(false); + window.set_framing_modified(false); +} + +/// Push the framing back to the geometry panel. +/// +/// Separate from [`sync_rows`] because framing is no longer *in* the rows — +/// it is presented by its own panel rather than generated (see +/// `DevelopSession::rows`), so nothing else would carry these values across. +/// +/// Everything here is written from what the session actually holds rather than +/// from what the gesture asked for: quarter turns wrap, the angle is clamped +/// to the descriptor's range, and the crop is normalised, so the panel must +/// show the applied value or it will disagree with the image. +fn sync_framing(window: &AppWindow, session: &Rc>>) { + let Some(s) = session.borrow().as_ref().map(|s| { + let (h, v) = s.flips(); + let c = s.crop(); + (s.angle(), h, v, s.framing_edits_image(), c) + }) else { + return; + }; + let (angle, flip_h, flip_v, modified, crop) = s; + window.set_straighten(angle); + window.set_flip_h(flip_h); + window.set_flip_v(flip_v); + window.set_framing_modified(modified); + // The overlay draws from these, and a rotation re-expresses the rect — + // so they have to follow a quarter turn even though no handle moved. + window.set_crop_x(crop.x); + window.set_crop_y(crop.y); + window.set_crop_w(crop.width); + window.set_crop_h(crop.height); } /// Push current parameter values back to the interface. @@ -249,6 +285,13 @@ fn sync_rows( ) { use slint::Model as _; + // Framing is not in `rows` — it has its own panel — but it *is* parameter + // state that the core may have clamped, so it is pushed back here for the + // same reason and by the same callers. A `reset all` reaches the framing + // too, and without this the geometry panel would keep showing the angle + // and flips of an image that no longer has them. + sync_framing(window, session); + let current = match session.borrow().as_ref() { Some(s) => s.rows(), None => Vec::new(), @@ -569,6 +612,13 @@ pub fn run(paths: Vec) -> Result<()> { // value that was actually applied. window.set_zoom(s.zoom()); window.set_zoomed(s.is_zoomed()); + // Filtering follows the magnification, measured against the + // *full* viewport rather than `w`/`h`: a draft frame is + // rendered at half resolution, and letting that flip the + // canvas to smooth would make it change character for the + // duration of every gesture. + let (vw, vh) = *viewport.borrow(); + window.set_magnified(s.magnifies_source(vw, vh)); } Err(e) => { log::warn!("render failed: {e}"); @@ -804,7 +854,17 @@ pub fn run(paths: Vec) -> Result<()> { Ok(b) => b, Err(e) => { log::warn!("{name}: {e}"); - w.set_load_error(e.into()); + // Offline needs its own words. "network error: + // connection refused" over a photograph the user + // just clicked reads as a broken app; the real + // situation is that this particular image was + // never stored on this device, and the fix is to + // download it while there is a connection. + w.set_load_error(if e.offline { + "Offline — this image is not stored on this device.".into() + } else { + slint::SharedString::from(e.message) + }); return; } }; @@ -995,11 +1055,87 @@ pub fn run(paths: Vec) -> Result<()> { w.set_crop_y(c.y); w.set_crop_w(c.width); w.set_crop_h(c.height); + w.set_framing_modified(s.framing_edits_image()); } redraw(&w); }); } + // ---- rotation, flips and straightening ------------------------------- + // + // Framing edits, so unlike zoom and pan they mark the image modified — but + // they are reached through named session actions rather than through a row + // index, so they do not `sync_rows` either. `sync_framing` is what carries + // the applied value back. + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + window.on_rotate_quarters(move |turns| { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.rotate_quarters(turns); + } + sync_framing(&w, &session); + redraw(&w); + }); + } + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + window.on_flip_h_toggled(move || { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.toggle_flip_h(); + } + sync_framing(&w, &session); + redraw(&w); + }); + } + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + window.on_flip_v_toggled(move || { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.toggle_flip_v(); + } + sync_framing(&w, &session); + redraw(&w); + }); + } + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + window.on_straighten_changed(move |degrees| { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.set_angle(degrees); + } + sync_framing(&w, &session); + redraw(&w); + }); + } + { + // The geometry section's reset: crop, angle, rotation and flips back + // to neutral, leaving every colour adjustment where it is. The panel's + // own reset-all is the one that clears everything. + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + window.on_framing_reset(move || { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.reset_framing(); + } + sync_framing(&w, &session); + redraw(&w); + }); + } + { let weak = window.as_weak(); let index = index.clone(); diff --git a/ui/dr-ui/src/library.rs b/ui/dr-ui/src/library.rs index c6b38d3..74159b2 100644 --- a/ui/dr-ui/src/library.rs +++ b/ui/dr-ui/src/library.rs @@ -37,9 +37,6 @@ use dr_types::FormatFilter; /// thumbnail needs nothing like that much detail. const MAX_PREVIEW_BYTES: u64 = 8 * 1024 * 1024; -/// Long edge of a grid thumbnail, in pixels. -const THUMBNAIL_EDGE: u32 = 256; - /// Progress and results from the scan worker. #[derive(Debug)] pub enum ScanMessage { @@ -62,7 +59,16 @@ pub enum ScanMessage { pruned: usize, elapsed_ms: u64, }, - Failed(String), + /// The scan could not finish. + /// + /// `offline` distinguishes "the server could not be reached" from "the + /// server refused", and it is carried here rather than re-derived because + /// the classification is only possible on the worker side: crossing the + /// channel flattens a [`dr_sync::RemoteError`] into a message, and no + /// amount of string matching on the far side can reliably recover it. + /// Without the flag a dead connection and a bad password produce the same + /// banner, which sends the user to re-enter a credential that was fine. + Failed { message: String, offline: bool }, } /// One decoded thumbnail, ready for the grid. @@ -117,6 +123,16 @@ pub enum ThumbnailMessage { /// The timeline is rebuilt on this rather than per image — a histogram /// that redrew 120 times during a batch would flicker for no benefit. DatesRecorded(usize), + /// TRACES: FR-CAT-9 + /// The server could not be reached while filling this batch. + /// + /// Distinct from a run of [`Unavailable`](Self::Unavailable): those are + /// per-image verdicts ("this file has no extractable preview") and leave + /// the rest of the library alone, where this is a statement about the + /// connection. Sent at most once per batch, because a dropped connection + /// produces one of these per *cell* otherwise and the banner would be + /// rewritten sixty times. + Offline { reason: String }, } /// TRACES: FR-CAT-15 | FR-CAT-11 @@ -163,12 +179,22 @@ pub struct RatingFilter { pub unjudged: bool, /// `None` for no flag constraint, otherwise exactly that flag. pub flag: Option, + /// TRACES: FR-CAT-9 + /// Only images whose original is stored on this device. + /// + /// Carried here, beside the rating terms, because every query path already + /// threads this one struct: adding a parallel parameter to + /// `read_cells_scoped`, `read_cells_all` and both counts would give four + /// call sites the chance to disagree about what the grid is showing, and + /// the count disagreeing with the cells is the specific bug this type's + /// "filter in SQL" rule exists to prevent. + pub local_only: bool, } impl RatingFilter { /// Whether this narrows anything, so the caller can skip the join. pub fn is_unfiltered(&self) -> bool { - self.min_rating == 0 && !self.unjudged && self.flag.is_none() + self.min_rating == 0 && !self.unjudged && self.flag.is_none() && !self.local_only } /// The SQL predicate, against an `images` aliased as `i`. @@ -218,6 +244,19 @@ impl RatingFilter { )); } + if self.local_only { + // `tier_actual`, not `tier_desired`: the question is what is + // *here*, not what a pin has promised will be. An image queued for + // download is exactly the one that cannot be opened yet, so + // showing it under "on this device" would be the wrong answer to + // the only question this filter is asked. + terms.push(format!( + "EXISTS (SELECT 1 FROM image_cache ic + WHERE ic.image_id = i.id AND ic.tier_actual >= {})", + dr_types::Tier::Original.stored() + )); + } + if terms.is_empty() { String::new() } else { @@ -467,13 +506,45 @@ pub fn spawn_scan( std::thread::spawn(move || { let started = std::time::Instant::now(); if let Err(e) = run_scan(&tx, creds, user_id, root, filter, catalog_path, started) { - let _ = tx.send(ScanMessage::Failed(e)); + let _ = tx.send(ScanMessage::Failed { + message: e.message, + offline: e.offline, + }); } }); rx } +/// A scan failure that still knows whether it was a connectivity failure. +/// +/// The scan crosses a thread boundary, so the typed error cannot travel with +/// it; this carries the one bit that must survive. +struct ScanFailure { + message: String, + offline: bool, +} + +impl ScanFailure { + /// A failure that is nothing to do with reachability — local I/O, a + /// runtime that would not start, a catalog that would not open. + fn local(message: impl std::fmt::Display) -> Self { + Self { + message: message.to_string(), + offline: false, + } + } +} + +impl From for ScanFailure { + fn from(e: dr_sync::RemoteError) -> Self { + Self { + offline: e.indicates_offline(), + message: e.to_string(), + } + } +} + fn run_scan( tx: &Sender, creds: AppCredentials, @@ -482,19 +553,20 @@ fn run_scan( filter: FormatFilter, catalog_path: PathBuf, started: std::time::Instant, -) -> Result<(), String> { +) -> Result<(), ScanFailure> { if let Some(dir) = catalog_path.parent() { - std::fs::create_dir_all(dir).map_err(|e| format!("creating {}: {e}", dir.display()))?; + std::fs::create_dir_all(dir) + .map_err(|e| ScanFailure::local(format!("creating {}: {e}", dir.display())))?; } - let catalog = Catalog::open(&catalog_path).map_err(|e| e.to_string())?; + let catalog = Catalog::open(&catalog_path).map_err(ScanFailure::local)?; let rt = tokio::runtime::Builder::new_current_thread() .enable_all() .build() - .map_err(|e| e.to_string())?; + .map_err(ScanFailure::local)?; rt.block_on(async { - let backend = NextcloudBackend::new(&creds, &user_id).map_err(|e| e.to_string())?; + let backend = NextcloudBackend::new(&creds, &user_id).map_err(ScanFailure::local)?; // Stored folder ETags, so an unchanged subtree is skipped whole. On a // first run this is empty and the walk is complete; on every run after @@ -514,10 +586,9 @@ fn run_scan( }); }, ) - .await - .map_err(|e| e.to_string())?; + .await?; - persist(&catalog, &root, &result).map_err(|e| e.to_string())?; + persist(&catalog, &root, &result).map_err(ScanFailure::local)?; // Report what the catalog holds, not what this pass listed. An // incremental rescan lists only what changed, so its own count is @@ -704,6 +775,41 @@ pub struct ThumbnailRequest { pub needs_metadata: bool, } +/// Why a full fetch failed, keeping the one bit the UI cannot re-derive. +/// +/// The same reasoning as [`ScanFailure`]: the typed error cannot cross the +/// channel, and "offline" versus "refused" decides whether develop shows +/// "you are offline — this image is not stored locally" or a real error. +#[derive(Debug)] +pub struct FetchFailure { + pub message: String, + pub offline: bool, +} + +impl FetchFailure { + fn local(message: impl std::fmt::Display) -> Self { + Self { + message: message.to_string(), + offline: false, + } + } +} + +impl From for FetchFailure { + fn from(e: dr_sync::RemoteError) -> Self { + Self { + offline: e.indicates_offline(), + message: e.to_string(), + } + } +} + +impl std::fmt::Display for FetchFailure { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + /// Fetch one file in full, for opening it in develop. /// /// Deliberately *not* the preview path. Browsing fetches a range and decodes @@ -717,7 +823,7 @@ pub fn spawn_full_fetch( creds: AppCredentials, user_id: String, path: String, -) -> Receiver, String>> { +) -> Receiver, FetchFailure>> { let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { @@ -727,7 +833,7 @@ pub fn spawn_full_fetch( { Ok(rt) => rt, Err(e) => { - let _ = tx.send(Err(e.to_string())); + let _ = tx.send(Err(FetchFailure::local(e))); return; } }; @@ -736,16 +842,13 @@ pub fn spawn_full_fetch( let backend = match NextcloudBackend::new(&creds, &user_id) { Ok(b) => b, Err(e) => { - let _ = tx.send(Err(e.to_string())); + let _ = tx.send(Err(FetchFailure::local(e))); return; } }; let id = RemoteId::Path(RemotePath::new(&path)); - let got = backend - .get(&id, None) - .await - .map_err(|e| e.to_string()); + let got = backend.get(&id, None).await.map_err(FetchFailure::from); let _ = tx.send(got); }); }); @@ -886,10 +989,17 @@ pub fn spawn_thumbnails( // instant it is read. let mut found = Vec::new(); + // Set when the server proves unreachable, which abandons the rest + // of the batch. The remaining cells would each take a full timeout + // to reach the same conclusion — on a 120-cell window, minutes of + // the grid appearing to load against a server that is not there. + let mut offline = false; + for req in to_fetch { let msg = fetch_one(&backend, store.as_mut(), &req, &mut found).await; + offline = matches!(msg, ThumbnailMessage::Offline { .. }); // A closed channel means the window went away mid-fetch. - if tx.send(msg).is_err() { + if tx.send(msg).is_err() || offline { break; } } @@ -901,19 +1011,27 @@ pub fn spawn_thumbnails( // header fetches take tens of seconds, and a single write at the // finish loses every one of them if the window closes first. It // also lets the timeline appear while the rest are still arriving. + // + // Skipped entirely when the connection has already failed: these + // are network reads too, and there is nothing left to read from. const FLUSH_EVERY: usize = 16; - log::info!("reading dates for {} image(s)", metadata_only.len()); - for req in metadata_only { - if tx.send(ThumbnailMessage::DateProgress).is_err() { - break; - } - read_metadata_only(&backend, &req, &mut found).await; + if !offline { + log::info!("reading dates for {} image(s)", metadata_only.len()); + for req in metadata_only { + if tx.send(ThumbnailMessage::DateProgress).is_err() { + break; + } + read_metadata_only(&backend, &req, &mut found).await; - if found.len() >= FLUSH_EVERY { - flush_metadata(&catalog_path, &mut found, &tx); + if found.len() >= FLUSH_EVERY { + flush_metadata(&catalog_path, &mut found, &tx); + } } } + // Always flushed, even when the batch was abandoned: whatever was + // read before the connection died is still true, and discarding it + // would mean re-fetching those headers next time. flush_metadata(&catalog_path, &mut found, &tx); }); }); @@ -938,11 +1056,24 @@ async fn fetch_one( row: req.row, reason, }; + // A connection failure is not this image's verdict. Reported as such so + // the caller can stop the batch rather than marking sixty cells + // individually unpreviewable over one dropped connection — a state the + // grid would then keep until something forced a reload. + let classify = |e: dr_sync::RemoteError| { + if e.indicates_offline() { + ThumbnailMessage::Offline { + reason: e.to_string(), + } + } else { + fail(e.to_string()) + } + }; // Stage one: the header, enough to parse the container's IFDs. let header = match backend.get(&id, Some(0..dr_decode::HEADER_BYTES)).await { Ok(b) => b, - Err(e) => return fail(e.to_string()), + Err(e) => return classify(e), }; // The same bytes carry EXIF. Reading it here is free — the alternative is @@ -955,7 +1086,7 @@ async fn fetch_one( let bytes = if header.starts_with(&[0xFF, 0xD8, 0xFF]) { match backend.get(&id, None).await { Ok(b) => b, - Err(e) => return fail(e.to_string()), + Err(e) => return classify(e), } } else { let Some(loc) = dr_decode::locate_preview(&header, req.size) else { @@ -970,7 +1101,7 @@ async fn fetch_one( // Stage two: exactly the preview's bytes. match backend.get(&id, Some(loc.range.clone())).await { Ok(b) => b, - Err(e) => return fail(e.to_string()), + Err(e) => return classify(e), } }; @@ -1077,18 +1208,53 @@ fn flush_metadata( /// One 256 KB header request, no preview range and no decode. This is what /// gets a library dated when its thumbnails came from the store — including /// shards synced from another device, which carry pixels but no metadata. +/// Read a header for its date. +/// +/// Returns whether the file was **reached**, which the caller needs and cannot +/// otherwise tell: a header that carried no EXIF and a fetch that never +/// happened both leave `found` untouched, and recording the second as "this +/// image has no date" would let one lock mark it dateless for good. async fn read_metadata_only( backend: &NextcloudBackend, req: &ThumbnailRequest, found: &mut Vec, -) { +) -> bool { let id = RemoteId::Path(RemotePath::new(&req.path)); - match backend.get(&id, Some(0..dr_decode::HEADER_BYTES)).await { - Ok(header) => collect_metadata(&header, req, found), - // Not worth surfacing: the cell is already showing its thumbnail, and - // a missing date leaves the image off the timeline rather than broken. - Err(e) => log::debug!("reading date for {}: {e}", req.path), + + // Retried, because one failure here is usually a lock rather than a + // verdict. Nextcloud's file locking answers a plain *read* with 423 under + // concurrency, and the identical range succeeds moments later — measured + // against a real server while twelve lanes were running. Without a retry + // those images sit out the whole pass over a lock that lasted a moment. + // + // Bounded and short: a genuinely missing or forbidden file must not cost + // three round trips before the sweep moves on. + const ATTEMPTS: usize = 3; + for attempt in 1..=ATTEMPTS { + match backend.get(&id, Some(0..dr_decode::HEADER_BYTES)).await { + Ok(header) => { + collect_metadata(&header, req, found); + return true; + } + Err(e) if e.is_transient() && attempt < ATTEMPTS => { + // Backing off at all matters more than the exact interval: the + // contention that produced the lock is our own lanes, so any + // pause lets the holder finish. + tokio::time::sleep(std::time::Duration::from_millis( + 200 * attempt as u64, + )) + .await; + } + Err(e) => { + // Not surfaced: a missing date leaves the image off the + // timeline rather than breaking anything, and the next sweep + // retries it regardless. + log::debug!("reading date for {} ({attempt} attempts): {e}", req.path); + return false; + } + } } + false } /// Write capture metadata read during the thumbnail pass. @@ -1209,7 +1375,12 @@ const SWEEP_CHUNK: usize = 96; /// Deliberately bounded rather than unlimited: the grid's own interactive /// fetches share this server, and a sweep that saturated the connection would /// make browsing feel broken while it ran. -const SWEEP_LANES: usize = 12; +/// +/// **Lowered from twelve after measuring.** Twelve produced 423 Locked on a +/// real server — Nextcloud's file locking answering a plain read under +/// contention we were creating ourselves. Six keeps most of the speedup +/// without provoking it; the retry above covers what still slips through. +const SWEEP_LANES: usize = 6; /// Date **every** image in the library, not just the ones on screen. /// @@ -1310,19 +1481,54 @@ pub fn spawn_sweep( let backend = &backend; async move { let mut found = Vec::new(); + let mut reached = Vec::new(); for req in lane { - read_metadata_only(backend, req, &mut found).await; + if read_metadata_only(backend, req, &mut found).await { + reached.push(req.image_id); + } } - found + (found, reached) } })) .await; - let mut found: Vec = results.into_iter().flatten().collect(); + let mut found = Vec::new(); + let mut reached = std::collections::HashSet::new(); + for (lane_found, lane_reached) in results { + found.extend(lane_found); + reached.extend(lane_reached); + } done += chunk.len(); + // An image whose header carried no EXIF at all yields nothing + // to `found`, so nothing marks it examined and the next sweep + // fetches it again — for ever. Darktable exports strip + // metadata by default, and 2,188 of them in the reference + // library meant 2,188 pointless round trips per run. + // + // Recorded as examined with no date: the file was read and + // genuinely has none, which is a different state from "not + // looked at yet" and must not be confused with it. + let answered: std::collections::HashSet = + found.iter().map(|m| m.image_id).collect(); + // Only files actually read. One that could not be fetched is + // left alone so the next pass retries it, rather than being + // written off over a lock or a dropped connection. + found.extend(chunk.iter().filter(|r| { + reached.contains(&r.image_id) && !answered.contains(&r.image_id) + }).map( + |r| MetadataFound { + image_id: r.image_id, + captured_at: None, + captured_offset: None, + camera: None, + lens: None, + iso: None, + }, + )); + dated += found.iter().filter(|m| m.captured_at.is_some()).count(); - let read = found.len(); + let read = answered.len(); flush_sweep(&catalog, &mut found); log::info!( "sweep: {done} done, {dated} dated ({read} read in {:.1}s)", @@ -1661,6 +1867,27 @@ fn total_images_filtered( Ok(n as usize) } +/// TRACES: FR-CAT-9 +/// How many visible images have their original stored on this device. +/// +/// Whole-library, like the star counts beside it: the chip says what narrowing +/// to it would show, so counting only the current window would make it +/// describe the view it exists to change. +pub fn local_original_count(catalog: &Catalog) -> Result { + let n: i64 = catalog.connection().query_row( + &format!( + "SELECT count(*) FROM images i + WHERE {VISIBLE} + AND EXISTS (SELECT 1 FROM image_cache ic + WHERE ic.image_id = i.id AND ic.tier_actual >= {})", + dr_types::Tier::Original.stored() + ), + [], + |r| r.get(0), + )?; + Ok(n as usize) +} + /// Total images in the catalog, unfiltered. /// /// What the scan reports and what the sidebar's "all images" row shows — the @@ -2342,4 +2569,108 @@ mod tests { has_preview: false, } } + + // --- the local-only filter (FR-CAT-9) --------------------------------- + + /// Record that an image's original is held locally at `tier`. + fn cache_at(catalog: &Catalog, id: dr_types::ImageId, tier: dr_types::Tier) { + catalog + .connection() + .execute( + "INSERT INTO image_cache (image_id, tier_actual, bytes) + VALUES (?1, ?2, 0)", + rusqlite::params![id.0 as i64, tier.stored()], + ) + .unwrap(); + } + + #[test] + fn local_only_shows_just_the_images_held_here() { + let catalog = with_images(10); + let ids = image_ids(&catalog); + for id in &ids[0..3] { + cache_at(&catalog, *id, dr_types::Tier::Original); + } + + let filter = RatingFilter { + local_only: true, + ..Default::default() + }; + let cells = read_cells_all(&catalog, &filter, 0, 120).unwrap(); + assert_eq!(cells.len(), 3); + // The count the header shows must agree with the cells drawn, which is + // the whole reason the predicate lives in SQL rather than in a + // post-filter over the rows. + assert_eq!(total_images_filtered(&catalog, &filter).unwrap(), 3); + assert_eq!(local_original_count(&catalog).unwrap(), 3); + } + + #[test] + fn a_cached_preview_is_not_a_local_original() { + // The filter answers "can I open this in develop right now", and a + // preview cannot. Counting it would put images in the offline set that + // fail the moment they are clicked. + let catalog = with_images(5); + let ids = image_ids(&catalog); + cache_at(&catalog, ids[0], dr_types::Tier::Preview); + cache_at(&catalog, ids[1], dr_types::Tier::Original); + + let filter = RatingFilter { + local_only: true, + ..Default::default() + }; + assert_eq!(read_cells_all(&catalog, &filter, 0, 120).unwrap().len(), 1); + assert_eq!(local_original_count(&catalog).unwrap(), 1); + } + + #[test] + fn local_only_composes_with_the_rating_filter() { + // "Five-star frames I can actually edit on this train" is one filter, + // not a mode that replaces the others. + let catalog = with_images(6); + let ids = image_ids(&catalog); + for id in &ids[0..4] { + cache_at(&catalog, *id, dr_types::Tier::Original); + } + // Rate two of the cached ones, and one that is not cached. + for id in [ids[0], ids[1], ids[5]] { + dr_catalog::rating::set_rating(catalog.connection(), id, 5).unwrap(); + } + + let filter = RatingFilter { + min_rating: 5, + local_only: true, + ..Default::default() + }; + let cells = read_cells_all(&catalog, &filter, 0, 120).unwrap(); + assert_eq!(cells.len(), 2, "five-starred AND held locally"); + assert_eq!(total_images_filtered(&catalog, &filter).unwrap(), 2); + } + + #[test] + fn an_empty_cache_is_not_an_empty_library() { + // The unfiltered grid must not depend on the cache table having rows — + // a library nothing has been downloaded from is still a full library. + let catalog = with_images(4); + assert_eq!(local_original_count(&catalog).unwrap(), 0); + assert_eq!( + read_cells_all(&catalog, &RatingFilter::default(), 0, 120) + .unwrap() + .len(), + 4 + ); + } + + #[test] + fn local_only_counts_as_a_narrowing_filter() { + // `is_unfiltered` gates the "filtered" indicator. Reporting this one as + // unfiltered would leave a narrowed grid looking like the whole + // library, which is the state the indicator exists to prevent. + assert!(RatingFilter::default().is_unfiltered()); + assert!(!RatingFilter { + local_only: true, + ..Default::default() + } + .is_unfiltered()); + } } diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index 63be01f..8ba4fda 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -62,6 +62,19 @@ pub struct LibraryController { needs_metadata: RefCell>, /// Where in the catalog the current window starts. Scrubbing moves this. offset: RefCell, + /// The first visible ordinal, kept so returning from the develop view lands + /// where the user left rather than at the top. + /// + /// Recorded when the grid is left, not read back when it is re-entered: + /// `show-library` gates an `if` in the markup, so the whole grid subtree is + /// destroyed and rebuilt, and the rebuilt `Flickable` reports a scroll to + /// row 0 before anything can restore the old position. Capturing at the + /// moment of departure is the only value the reset cannot overwrite. + /// + /// Separate from `offset`, which is the *loaded window*'s start and sits a + /// quarter-window above the view. Restoring that as a viewport position + /// would land the user consistently short of where they were. + resume_at: std::cell::Cell, /// How many cells to load, derived from what the viewport can show. /// /// A fixed count is wrong in both directions: too small on a maximised 4K @@ -136,6 +149,21 @@ pub struct LibraryController { /// that already holds borrows of other fields, and a `u64` needs no borrow /// tracking. generation: std::cell::Cell, + /// TRACES: FR-CAT-9 + /// Whether the server is reachable, inferred from what the workers saw. + /// + /// Lives on the controller rather than in a global because it is scoped to + /// one open library: signing into a different account starts a fresh + /// judgement, and carrying the old one over would report a server down + /// that was never contacted. + reachability: RefCell, + /// Narrow the grid to images whose original is stored locally. + /// + /// A `Cell` beside `filter` rather than a field inside it: the rating + /// filter compiles to a SQL predicate over `versions`, while this one is a + /// predicate over the cache, and folding two different joins into one type + /// would put the cache schema inside a rating concept. + local_only: std::cell::Cell, } impl LibraryController { @@ -148,6 +176,7 @@ impl LibraryController { image_ids: RefCell::new(Vec::new()), needs_metadata: RefCell::new(Vec::new()), offset: RefCell::new(0), + resume_at: std::cell::Cell::new(0), window: RefCell::new(INITIAL_WINDOW), requested: RefCell::new(Default::default()), scan_timer: RefCell::new(None), @@ -164,9 +193,33 @@ impl LibraryController { filter: RefCell::new(library::RatingFilter::default()), sidecar_timer: RefCell::new(None), generation: std::cell::Cell::new(0), + reachability: RefCell::new(dr_sync::Reachability::new()), + local_only: std::cell::Cell::new(false), }) } + /// TRACES: FR-CAT-9 + /// Whether the app currently believes the server is unreachable. + pub fn is_offline(&self) -> bool { + self.reachability.borrow().is_offline() + } + + /// Whether the grid is narrowed to locally-stored originals. + pub fn local_only(&self) -> bool { + self.local_only.get() + } + + /// Toggle the local-only filter, resetting the window. + /// + /// The offset is cleared for the same reason a scope change clears it: the + /// filter changes which images exist as far as the grid is concerned, so a + /// position counted against the old set names a different photograph. + pub fn set_local_only(&self, on: bool) { + self.local_only.set(on); + *self.offset.borrow_mut() = 0; + self.requested.borrow_mut().clear(); + } + /// The catalog handle, for [`crate::collections_ui`] to edit through. pub fn catalog(&self) -> Rc>> { self.catalog.clone() @@ -378,6 +431,18 @@ fn drain_scan( ); w.set_library_scanning(false); + // A completed scan is the strongest possible evidence + // the server is reachable, so it clears offline mode + // without needing a probe of its own. + if ctl + .reachability + .borrow_mut() + .mark_reachable(std::time::Instant::now()) + { + log::info!("back online"); + } + refresh_offline(&w, ctl); + // An incremental rescan lists almost nothing, so // reporting the listed count would read as "0 images" // on a library that is simply up to date. @@ -413,10 +478,28 @@ fn drain_scan( stop(&ctl.scan_timer); return; } - ScanMessage::Failed(e) => { - log::warn!("scan failed: {e}"); + ScanMessage::Failed { message, offline } => { + log::warn!("scan failed: {message}"); w.set_library_scanning(false); - w.set_library_error(e.into()); + + if offline { + // Not an error state. The catalog from the last + // successful scan is still on disk and still + // accurate for everything already indexed, so the + // grid keeps working — it simply cannot learn about + // anything added on the server since. + ctl.reachability + .borrow_mut() + .mark_unreachable(message, std::time::Instant::now()); + refresh_offline(&w, ctl); + // Show whatever the catalog holds. Without this the + // grid stays empty on a launch that began offline, + // which is precisely the case offline mode exists + // for. + open_catalog_for_offline(&w, ctl, &catalog_path, &coll_ctl); + } else { + w.set_library_error(message.into()); + } stop(&ctl.scan_timer); return; } @@ -428,6 +511,119 @@ fn drain_scan( *ctl.scan_timer.borrow_mut() = Some(timer); } +/// Start a scan against the configured library. +/// +/// Shared by the rescan button and by the offline banner's retry, which are +/// the same operation: a scan is the only request that both proves the server +/// is reachable and brings the catalog up to date. Keeping them one function +/// is what stops "retry" from quietly becoming a weaker probe than "rescan". +fn start_rescan( + window: &AppWindow, + ctl: &Rc, + coll_ctl: &Rc, +) { + let Some((creds, session, filter)) = ctl.session.borrow().clone() else { + return; + }; + + window.set_library_scanning(true); + window.set_library_error(slint::SharedString::new()); + window.set_library_status("Rescanning…".into()); + + let path = library::catalog_path(&session.server, &session.user_id); + let rx = library::spawn_scan( + creds, + session.user_id.clone(), + session.root.clone(), + filter, + path.clone(), + ); + drain_scan(window.as_weak(), ctl.clone(), coll_ctl.clone(), rx, path); +} + +/// TRACES: FR-CAT-9 +/// Paint the connectivity state into the window. +/// +/// Called wherever reachability may have moved, rather than by the state +/// itself: `Reachability` is in `dr-sync` and knows nothing about a window, +/// which is what keeps it testable without a display server. +fn refresh_offline(window: &AppWindow, ctl: &Rc) { + let reach = ctl.reachability.borrow(); + let offline = reach.is_offline(); + + window.set_library_offline(offline); + window.set_library_offline_reason(reach.reason().unwrap_or_default().into()); + window.set_library_offline_since( + reach + .offline_for(std::time::Instant::now()) + .map(describe_duration) + .unwrap_or_default() + .into(), + ); + + // A stale scan error under an offline banner reports one problem twice. + if offline { + window.set_library_error("".into()); + } +} + +/// A coarse "how long ago", for the offline banner. +/// +/// Deliberately imprecise: the user wants to know whether this just happened +/// or has been true for a while, and a live-counting seconds display would +/// draw the eye to a number that changes without meaning anything. +fn describe_duration(d: std::time::Duration) -> String { + let secs = d.as_secs(); + if secs < 60 { + "just now".to_string() + } else if secs < 3600 { + format!("{}m ago", secs / 60) + } else { + format!("{}h ago", secs / 3600) + } +} + +/// TRACES: FR-CAT-9 +/// Show the catalog after a scan that could not reach the server. +/// +/// The whole point of offline mode: a scan is how the grid normally gets its +/// catalog handle, so a failed one previously left the library empty even +/// though a complete catalog was sitting on disk from the last successful run. +/// The images are all still there, their thumbnails are in the shards, and +/// rating and collecting them are local writes. +/// +/// The sweep is deliberately **not** started — it exists to fetch headers over +/// the network, so offline it would do nothing but fail once per image. +fn open_catalog_for_offline( + window: &AppWindow, + ctl: &Rc, + catalog_path: &std::path::Path, + coll_ctl: &Rc, +) { + if ctl.catalog.borrow().is_some() { + // Already open — a rescan that failed, rather than a launch that + // began offline. The grid is showing the catalog already. + load_window(window, ctl); + return; + } + + match Catalog::open(catalog_path) { + Ok(cat) => { + crate::collections_ui::refresh_tree(window, coll_ctl, &cat); + *ctl.catalog.borrow_mut() = Some(cat); + load_window(window, ctl); + } + Err(e) => { + // No catalog and no server. This is the one genuinely empty case: + // a first run that never reached the server has nothing indexed. + log::warn!("offline with no local catalog: {e}"); + window.set_library_error( + "Offline, and this library has not been scanned on this device yet.".into(), + ); + } + } +} + /// Fill the model from the catalog and start fetching thumbnails. /// /// Reads the window starting at the controller's current offset, which the @@ -636,6 +832,13 @@ fn refresh_rating_counts(window: &AppWindow, catalog: &Catalog) { let counts = dr_catalog::rating::rating_histogram(catalog.connection()).unwrap_or_default(); let as_i32: Vec = counts.iter().map(|n| *n as i32).collect(); window.set_library_rating_counts(slint::ModelRc::new(slint::VecModel::from(as_i32))); + + // TRACES: FR-CAT-9 + // How many originals are actually here, for the "On this device" chip. + // Shown for the same reason the star counts are: a filter that silently + // empties the grid reads as broken, and this one will legitimately be zero + // on a library nothing has been downloaded from yet. + window.set_library_local_count(library::local_original_count(catalog).unwrap_or(0) as i32); } /// Apply a judgement to a set of images: catalog first, then sidecars. @@ -784,6 +987,25 @@ fn start_sidecar_writes( if writes.is_empty() { return; } + + // TRACES: FR-CAT-9 + // Offline, this would stall on a timeout per sidecar, on the very + // keystroke path a cull is built for speed on (FR-CULL-1). The judgement + // itself is safe either way — `apply_judgement` has already committed it + // to the catalog, which is what the grid reads and what survives a + // restart. + // + // What is deferred is the sidecar, and with it the guarantee that the + // judgement survives a *catalog rebuild* (ARCH §6.12). There is no queue + // behind this yet, so a rating made offline is written to its sidecar only + // when that image is judged again while connected. That is a real gap + // rather than a hidden one: it trades a durability property that already + // depends on the network for a cull that stays responsive without it. + if ctl.is_offline() { + log::debug!("offline: skipping {} sidecar write(s)", writes.len()); + return; + } + let Some((creds, session, _)) = ctl.session.borrow().clone() else { return; }; @@ -1013,6 +1235,22 @@ fn drain_thumbnails( // carry no embedded preview. ThumbnailMessage::Ready(t) => { w.set_library_thumbs_done(w.get_library_thumbs_done() + 1); + // Bytes arrived, so the server is reachable. This is + // what clears the banner when a connection returns + // while the user is simply scrolling, without waiting + // for a probe or a manual retry. + if ctl_cb + .reachability + .borrow_mut() + .mark_reachable(std::time::Instant::now()) + { + log::info!("back online"); + refresh_offline(&w, &ctl_cb); + // The sweep was refused while offline, so nothing + // else would ever restart it — the grid would stay + // partially dated until the next launch. + start_sweep(&w, &ctl_cb); + } if let Some(mut row) = model.row_data(t.row) { row.thumbnail = to_slint_image(t.width, t.height, &t.rgba); row.has_thumb = true; @@ -1027,6 +1265,29 @@ fn drain_thumbnails( model.set_row_data(row, r); } } + // TRACES: FR-CAT-9 + ThumbnailMessage::Offline { reason } => { + log::info!("thumbnails stopped: {reason}"); + ctl_cb + .reachability + .borrow_mut() + .mark_unreachable(reason, std::time::Instant::now()); + refresh_offline(&w, &ctl_cb); + + // The batch is over, so the progress counter must not + // be left showing a partial fetch that will never + // finish — it would spin in the header for ever. + w.set_library_thumbs_total(0); + w.set_library_thumbs_done(0); + + // Cells left without pixels stay placeholders rather + // than being marked unavailable: the images are fine, + // and a reconnect should fill them in. Marking them + // would persist a verdict about the *file* from an + // event about the *connection*. + stop(&ctl_cb.thumb_timer); + return; + } } } }, @@ -1078,6 +1339,16 @@ fn start_derived_sync(window: &AppWindow, ctl: &Rc) { return; } + // TRACES: FR-CAT-9 + // Nothing to push to and nothing to take. Attempting it would upload + // shards into a timeout and light the "Syncing…" indicator over work that + // cannot start; the shards are unchanged on disk and go out on the next + // sync once the server is back. + if ctl.is_offline() { + log::debug!("offline: skipping derived sync"); + return; + } + let catalog_path = library::catalog_path(&session.server, &session.user_id); let scratch = catalog_path .parent() @@ -1178,6 +1449,17 @@ fn start_sweep(window: &AppWindow, ctl: &Rc) { return; }; + // TRACES: FR-CAT-9 + // The sweep is nothing but network reads — one header fetch per undated + // image, across the whole library. Offline it would spend a timeout on + // every one of them, running for hours to learn nothing, while the + // progress bar implied work was happening. It resumes on reconnect, and + // the images it has already dated stay dated. + if ctl.is_offline() { + log::debug!("offline: not starting the metadata sweep"); + return; + } + let rx = library::spawn_sweep( creds, session.user_id.clone(), @@ -1678,6 +1960,19 @@ where let Some(w) = weak.upgrade() else { return }; let first_visible = first_visible.max(0) as usize; + // Remember where the view is, so leaving for the develop view and + // coming back returns here. Latched on every event rather than read + // at departure: by the time the grid is hidden its scroll position + // is only in the Flickable, which is about to be destroyed. + // + // Only while the grid is the visible page. Building and tearing + // down the subtree moves the viewport through 0, and accepting that + // would erase the position on the way out — the first return would + // work and every one after it would land at the top. + if w.get_show_library() { + ctl.resume_at.set(first_visible); + } + // Move the timeline marker with the view. Scrolling the grid is a // way of moving through time just as scrubbing is, and a marker // that only ever moved on a scrub sat still while the photographs @@ -1857,12 +2152,39 @@ where } // Develop → grid. + // + // Returns to where the user left rather than to the top. `show-library` + // gates an `if` in the markup, so the grid is rebuilt from nothing and its + // Flickable starts at row 0; the position has to be replayed explicitly. { let weak = window.as_weak(); + let ctl = ctl.clone(); window.on_back_to_library(move || { - if let Some(w) = weak.upgrade() { - w.set_show_library(true); + let Some(w) = weak.upgrade() else { return }; + + let resume = ctl.resume_at.get(); + if resume > 0 { + // Through the same channel a scrub uses, and for the same + // reason: the viewport and the loaded window both have to move, + // or the cells are drawn thousands of rows from where the view + // sits. + // + // Centred like `on_library_scrolled` does it, so scrolling up + // from the restored position has loaded rows above it. + let window_size = *ctl.window.borrow(); + *ctl.offset.borrow_mut() = resume.saturating_sub(window_size / 4); + ctl.requested.borrow_mut().clear(); + load_window(&w, &ctl); + + // Before the grid is shown, not after: the markup gates it on + // an `if`, and the rebuilt Flickable reads `scroll-to` in its + // `init`. Setting these afterwards would leave that init to run + // against the previous position. + w.set_library_scroll_to(resume as i32); + w.set_library_scroll_token(w.get_library_scroll_token() + 1); } + + w.set_show_library(true); }); } @@ -1872,23 +2194,7 @@ where let coll_ctl = coll_ctl.clone(); window.on_library_rescan(move || { let Some(w) = weak.upgrade() else { return }; - let Some((creds, session, filter)) = ctl.session.borrow().clone() else { - return; - }; - - w.set_library_scanning(true); - w.set_library_error(slint::SharedString::new()); - w.set_library_status("Rescanning…".into()); - - let path = library::catalog_path(&session.server, &session.user_id); - let rx = library::spawn_scan( - creds, - session.user_id.clone(), - session.root.clone(), - filter, - path.clone(), - ); - drain_scan(w.as_weak(), ctl.clone(), coll_ctl.clone(), rx, path); + start_rescan(&w, &ctl, &coll_ctl); }); } @@ -2000,6 +2306,40 @@ where refilter(&w, &ctl); }); } + + // TRACES: FR-CAT-9 + // "On this device" — the images openable without a server. Composes with + // the rating terms rather than replacing them: "five-star frames I can + // actually edit on this train" is one filter, not a mode. + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + window.on_library_toggle_local_only(move || { + let Some(w) = weak.upgrade() else { return }; + let on = !ctl.local_only(); + ctl.set_local_only(on); + ctl.filter.borrow_mut().local_only = on; + w.set_library_local_only(on); + refilter(&w, &ctl); + }); + } + + // TRACES: FR-CAT-9 + // Retry now, rather than waiting out the backoff. A user who has just + // reconnected their wifi knows something the backoff does not. + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + let coll_ctl = coll_ctl.clone(); + window.on_library_retry_connection(move || { + let Some(w) = weak.upgrade() else { return }; + log::info!("retrying the connection at the user's request"); + // A rescan is the probe: it is the same request the scan worker + // makes, so a success proves reachability and repopulates the + // catalog in one pass rather than proving it twice. + start_rescan(&w, &ctl, &coll_ctl); + }); + } } /// Reload the grid after the filter changed. diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index 9aad8ec..ffb1cb2 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -8,7 +8,7 @@ // (FR-DEV-3c). import { Theme } from "theme.slint"; -import { PanelHeading, Label, Value, Caption, Section } from "widgets.slint"; +import { PanelHeading, Label, Value, Caption, Section, Button, IconButton } from "widgets.slint"; // One parameter, flattened for Slint's model system. // @@ -220,6 +220,249 @@ component ParamSlider inherits Rectangle { } } +// A slider not backed by a `ParamRow`. +// +// `ParamSlider` reads its geometry out of a model row, which is right for the +// generated panel and wrong for a control the interface names itself — the +// straighten angle is reached through the session's own accessor, not through +// a row index, so there is no row to feed it. The track behaviour is the same +// and deliberately so: a slider that dragged differently depending on which +// panel it sat in would be a worse inconsistency than the duplication. +component PlainSlider inherits Rectangle { + in property label; + in property value; + in property default-value: 0.0; + in property minimum: -1.0; + in property maximum: 1.0; + in property unit; + + callback changed(float); + callback reset(); + + height: 46px; + + VerticalLayout { + spacing: 2px; + + HorizontalLayout { + Label { + text: root.label; + emphasised: root.value != root.default-value; + } + + Rectangle { horizontal-stretch: 1; } + + Value { + text: (Math.round(root.value * 10) / 10) + root.unit; + modified: root.value != root.default-value; + placeholder: root.value == root.default-value; + compact: true; + } + } + + track := Rectangle { + height: Theme.touch-target / 2; + + Rectangle { + y: (parent.height - 3px) / 2; + height: 3px; + background: Theme.surface-raised; + border-radius: 1.5px; + } + + // The neutral point. A straighten slider is symmetric and read as + // a deviation from level, so where zero sits has to be visible. + if root.minimum < root.default-value && root.default-value < root.maximum: + Rectangle { + x: (root.default-value - root.minimum) + / (root.maximum - root.minimum) * parent.width - 1px; + y: (parent.height - 9px) / 2; + width: 2px; + height: 9px; + background: Theme.rule; + } + + Rectangle { + property span: root.maximum - root.minimum; + property default-x: + (root.default-value - root.minimum) / self.span * parent.width; + property value-x: + (root.value - root.minimum) / self.span * parent.width; + + x: min(self.default-x, self.value-x); + width: abs(self.value-x / 1px - self.default-x / 1px) * 1px; + y: (parent.height - 3px) / 2; + height: 3px; + background: Theme.active; + border-radius: 1.5px; + } + + Rectangle { + x: (root.value - root.minimum) + / (root.maximum - root.minimum) * parent.width - 6px; + y: (parent.height - 12px) / 2; + width: 12px; + height: 12px; + border-radius: 6px; + background: area.has-hover || area.pressed ? Theme.ink : Theme.ink-dim; + } + + area := TouchArea { + width: 100%; + height: 100%; + + property span: root.maximum - root.minimum; + // The same axis test `ParamSlider` makes, and for the same + // reason: this sits in a scrolling panel, and claiming the + // gesture on press-down would jump the angle every time the + // user tried to scroll from it. + property claimed: false; + + function value-at(px: length) -> float { + return clamp( + root.minimum + (px / self.width) * self.span, + root.minimum, + root.maximum); + } + + moved => { + if (!self.claimed + && abs(self.mouse-x - self.pressed-x) + > abs(self.mouse-y - self.pressed-y)) { + self.claimed = true; + } + if (self.claimed) { + root.changed(self.value-at(self.mouse-x)); + } + } + pointer-event(ev) => { + if (ev.kind == PointerEventKind.up + || ev.kind == PointerEventKind.cancel) { + self.claimed = false; + } + if (ev.kind == PointerEventKind.down + && ev.button == PointerEventButton.right) { + root.reset(); + } + } + clicked => { + if (!self.claimed) { + root.changed(self.value-at(self.mouse-x)); + } + } + double-clicked => { root.reset(); } + } + } + } +} + +// Crop, rotation, flips and straightening — the framing controls. +// +// **Why this is hand-built when the rest of the panel is generated.** The +// generic path renders one slider per parameter, which for framing means eight +// of them: four crop edges the user would have to type coordinates into, and a +// "Rotate" slider running 0..3. Every one of those is a worse control than the +// gesture it stands for — a crop is dragged on the photograph, and a quarter +// turn is a button. So framing is presented rather than generated, and the +// generic panel drops it (see `AdjustPanel.skip-op`). +// +// This does not weaken ARCH §4.3: nothing here reads a parameter *value* out +// of a descriptor or routes by index. It calls named session actions, which is +// what a bespoke widget for a known stage is entitled to do. +export component GeometryPanel inherits Rectangle { + in property enabled: true; + /// Whether the crop overlay is up. The button is a toggle, not an action: + /// crop mode is sustained state, and the canvas looks different while it + /// is on. + in property crop-mode: false; + in property angle: 0.0; + in property max-straighten: 45.0; + in property flip-h: false; + in property flip-v: false; + /// Any of crop, angle, rotation or flips differs from neutral. + in property modified: false; + + callback crop-toggled(bool); + callback rotate(int); + callback flip-h-toggled(); + callback flip-v-toggled(); + callback angle-changed(float); + callback angle-reset(); + callback reset(); + + height: layout.preferred-height; + + layout := VerticalLayout { + spacing: 0px; + alignment: start; + + Section { + title: "GEOMETRY"; + modified: root.modified; + has-reset: root.modified; + op-reset => { root.reset(); } + + VerticalLayout { + spacing: Theme.gap-sm; + padding-top: Theme.gap-sm; + padding-bottom: Theme.gap-sm; + + // Crop first: it is the framing decision the others serve. + Button { + text: root.crop-mode ? "Done Cropping" : "Crop"; + active: root.crop-mode; + enabled: root.enabled; + clicked => { root.crop-toggled(!root.crop-mode); } + } + + // Rotation and flips. Glyphs rather than labels: four controls + // named in words would wrap the 280px column, and each of + // these shows its own result. + HorizontalLayout { + spacing: Theme.gap-sm; + + IconButton { + glyph: "⟲"; + enabled: root.enabled; + clicked => { root.rotate(-1); } + } + IconButton { + glyph: "⟳"; + enabled: root.enabled; + clicked => { root.rotate(1); } + } + + Rectangle { horizontal-stretch: 1; } + + IconButton { + glyph: "⇔"; + active: root.flip-h; + enabled: root.enabled; + clicked => { root.flip-h-toggled(); } + } + IconButton { + glyph: "⇕"; + active: root.flip-v; + enabled: root.enabled; + clicked => { root.flip-v-toggled(); } + } + } + + PlainSlider { + label: "Straighten"; + value: root.angle; + default-value: 0.0; + minimum: -root.max-straighten; + maximum: root.max-straighten; + unit: "°"; + changed(v) => { root.angle-changed(v); } + reset => { root.angle-reset(); } + } + } + } + } +} + // A tone curve editor: a square grid with draggable control points. // // The curve *line* is drawn from `samples`, which Rust evaluates with the diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 6ec0c51..ef79a12 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -1,5 +1,5 @@ import { Theme } from "theme.slint"; -import { AdjustPanel, ParamRow } from "adjust.slint"; +import { AdjustPanel, GeometryPanel, ParamRow } from "adjust.slint"; import { LaunchScreen } from "launch.slint"; import { LibraryGrid, LibraryCell, TimelineBar } from "library.slint"; import { Button, PanelHeading, Label, Value, Caption, Panel, EmptyState } from "widgets.slint"; @@ -137,6 +137,14 @@ export component AppWindow inherits Window { in property zoom: 1.0; in property zoomed: false; + /// Whether one source pixel now covers more than one screen pixel. + /// + /// Drives the canvas's filtering, and nothing else. Rust decides it rather + /// than the `zoom` property above, because the two are not the same + /// question: whether the image is magnified depends on the source's + /// resolution against the viewport's, which Slint does not know. + in property magnified: false; + /// Whether the crop overlay is active. While it is, the canvas shows the /// *whole* frame — otherwise the area being cropped away would not be on /// screen to drag across — and the surround is greyed. @@ -152,6 +160,29 @@ export component AppWindow inherits Window { callback crop-mode-toggled(bool); /// A dragged crop rect, in fractions of the frame. callback crop-changed(float, float, float, float); + + // --- rotation, flips and straightening (FR-DEV-3) --- + // + // Mirrored from the session rather than held here, for the same reason the + // crop rect is: Rust clamps and wraps these, so the panel must show what + // was actually applied and not what the click asked for. + in property straighten: 0.0; + /// The straighten slider's travel either way, from the descriptor. + in property max-straighten: 45.0; + in property flip-h: false; + in property flip-v: false; + /// Any framing edit is in effect — what lights the section's dot and + /// enables its reset. Distinct from the crop rect being non-full: a + /// rotation or a flip is an edit with the crop still full. + in property framing-modified: false; + + /// Quarter turns, positive clockwise. + callback rotate-quarters(int); + callback flip-h-toggled(); + callback flip-v-toggled(); + callback straighten-changed(float); + /// Crop, angle, rotation and flips back to neutral, leaving colour alone. + callback framing-reset(); /// Scroll-to-zoom: factor, and the anchor in fractions of the visible area. callback zoom-at(float, float, float); callback pan-by(float, float); @@ -204,6 +235,29 @@ export component AppWindow inherits Window { in property library-thumbs-done: 0; in property library-thumbs-total: 0; + // --- offline mode (FR-CAT-9) --- + // + // The library keeps working from local data when the server cannot be + // reached: thumbnails come from the shards, and rating and sorting are + // catalog operations that never needed the network. What stops is opening + // an original that was never stored locally. + // + // `offline-reason` carries the transport's own message ("connection + // refused", "dns error") because it is usually specific enough to act on, + // where a bare "offline" leaves the user guessing whether it is their wifi + // or the server. + in property library-offline: false; + in property library-offline-reason: ""; + in property library-offline-since: ""; + // Narrows the grid to images whose RAW is stored locally — the ones that + // can actually be opened while offline. Off by default: the catalog is the + // library, and hiding most of it the moment a connection drops would read + // as data loss rather than as a filter. + in-out property library-local-only: false; + in property library-local-count: 0; + callback library-toggle-local-only(); + callback library-retry-connection(); + in property library-root-label: ""; in-out property <[TimelineBar]> library-timeline; in property library-timeline-label: ""; @@ -456,6 +510,14 @@ export component AppWindow inherits Window { scan-error: root.library-error; thumbs-done: root.library-thumbs-done; thumbs-total: root.library-thumbs-total; + + offline: root.library-offline; + offline-reason: root.library-offline-reason; + offline-since: root.library-offline-since; + retry-connection() => { root.library-retry-connection(); } + local-only: root.library-local-only; + local-count: root.library-local-count; + toggle-local-only() => { root.library-toggle-local-only(); } root-label: root.library-root-label; timeline: root.library-timeline; timeline-label: root.library-timeline-label; @@ -565,6 +627,20 @@ export component AppWindow inherits Window { height: 100%; source: root.canvas; image-fit: contain; + // Past 1:1 there is no detail left to reconstruct, so + // smoothing only invents values between real pixels — and + // inspecting focus or noise is the whole reason to zoom in + // that far. Below 1:1 it stays smooth, where filtering is + // what keeps the image from aliasing. + // + // This matters even at moderate zoom on a HiDPI display: + // the canvas is rendered at *logical* size and Slint scales + // it up by the device pixel ratio, so the buffer is + // resampled on its way to the screen whatever the pipeline + // did. + image-rendering: root.magnified + ? ImageRendering.pixelated + : ImageRendering.smooth; visible: root.total > 0 && root.load-error == ""; } @@ -851,17 +927,27 @@ export component AppWindow inherits Window { } } - // Crop and zoom controls, floating over the canvas so they are - // reachable whether or not the side panel is showing. + // Zoom controls, floating over the canvas. + // + // Zoom stays here rather than moving to the sidebar with the + // crop: it is a property of *looking*, it is driven by the + // scroll wheel on the image itself, and its readout belongs + // beside what it is reporting on. Crop moved to the panel + // because it is an edit, and edits live with the other edits. + // + // A crop exit is still reachable from here while cropping — + // the panel may be collapsed on a narrow window, and stranding + // the user in a mode with no visible way out is worse than one + // duplicated control. if root.total > 0 && root.load-error == "": HorizontalLayout { x: 12px; y: parent.height - self.preferred-height - 12px; spacing: 6px; - Button { - text: root.crop-mode ? "Done" : "Crop"; - active: root.crop-mode; - clicked => { root.crop-mode-toggled(!root.crop-mode); } + if root.crop-mode: Button { + text: "Done"; + active: true; + clicked => { root.crop-mode-toggled(false); } } // Zoom is a view state, so its readout doubles as the @@ -907,6 +993,33 @@ export component AppWindow inherits Window { background: Theme.rule; } + // Framing above the colour work, matching how the edit is + // made rather than how it is applied: the frame is decided + // by eye first and the pipeline runs it last (see + // `dr_pipeline::framing` on the coordinate order). + GeometryPanel { + enabled: root.adjust-enabled; + crop-mode: root.crop-mode; + angle: root.straighten; + max-straighten: root.max-straighten; + flip-h: root.flip-h; + flip-v: root.flip-v; + modified: root.framing-modified; + + crop-toggled(on) => { root.crop-mode-toggled(on); } + rotate(turns) => { root.rotate-quarters(turns); } + flip-h-toggled => { root.flip-h-toggled(); } + flip-v-toggled => { root.flip-v-toggled(); } + angle-changed(v) => { root.straighten-changed(v); } + angle-reset => { root.straighten-changed(0); } + reset => { root.framing-reset(); } + } + + Rectangle { + height: 1px; + background: Theme.rule; + } + AdjustPanel { vertical-stretch: 1; rows: root.adjust-rows; diff --git a/ui/dr-ui/ui/library.slint b/ui/dr-ui/ui/library.slint index 4974a1f..e104ee4 100644 --- a/ui/dr-ui/ui/library.slint +++ b/ui/dr-ui/ui/library.slint @@ -636,6 +636,21 @@ export component LibraryGrid inherits Rectangle { callback filter-unjudged-toggled(bool); callback filter-flag-changed(int); + // --- offline (FR-CAT-9) ------------------------------------------------- + // + // Offline is a banner rather than a modal or an empty state, because most + // of the library still works: the shards hold the thumbnails, and rating, + // flagging, sorting and collecting are catalog writes that never touched + // the network. Only opening an un-cached original actually fails. + in property offline: false; + in property offline-reason: ""; + in property offline-since: ""; + callback retry-connection(); + /// Narrow to images whose RAW is stored locally — the ones openable now. + in property local-only: false; + in property local-count: 0; + callback toggle-local-only(); + /// How many images are selected, for the header's count. in property selected-count: 0; /// Which collection scopes the grid, for the header. Empty means all. @@ -850,6 +865,21 @@ export component LibraryGrid inherits Rectangle { } } + Rectangle { width: Theme.gap; } + + // Locally-stored originals. Always offered, not only when + // offline: "what can I actually work on right now" is a fair + // question on a slow connection too, and a control that + // appears only in the failure case is one the user has to + // discover at the worst moment. + FilterChip { + label: "On this device"; + count: root.local-count; + active: root.local-only; + y: (parent.height - self.height) / 2; + clicked => { root.toggle-local-only(); } + } + Rectangle { horizontal-stretch: 1; } // What the filter is currently hiding. Without this a narrowed @@ -857,7 +887,7 @@ export component LibraryGrid inherits Rectangle { // single most confusing state a filter can leave behind. Caption { text: (root.filter-min-rating > 0 || root.filter-unjudged - || root.filter-flag > 0) + || root.filter-flag > 0 || root.local-only) ? "filtered" : ""; emphasised: true; vertical-alignment: center; @@ -886,8 +916,55 @@ export component LibraryGrid inherits Rectangle { : 0; } + // --- offline ------------------------------------------------------ + // + // Above the scan error, and it suppresses it: when the server is + // unreachable the scan failure is a *consequence*, and showing both + // reports one problem twice while implying two. + if root.offline: Rectangle { + height: 34px; + background: Theme.surface; + + HorizontalLayout { + padding-left: Theme.gap; + padding-right: Theme.gap; + spacing: Theme.gap; + + Caption { + text: "Offline — showing what is stored on this device" + + (root.offline-since != "" ? " (" + root.offline-since + ")" : ""); + warn: true; + vertical-alignment: center; + overflow: elide; + } + + // The transport's own words. Usually specific enough to act on + // — "connection refused" and "dns error" send the user to + // different places — where a bare "offline" leaves them + // guessing whether it is their wifi or the server. + Caption { + text: root.offline-reason; + vertical-alignment: center; + overflow: elide; + horizontal-stretch: 1; + } + + Button { + text: "Retry"; + y: (parent.height - self.height) / 2; + clicked => { root.retry-connection(); } + } + } + + Rectangle { + y: parent.height - 1px; + height: 1px; + background: Theme.rule; + } + } + // --- error -------------------------------------------------------- - if root.scan-error != "": Rectangle { + if root.scan-error != "" && !root.offline: Rectangle { height: 34px; background: Theme.surface; Caption { @@ -1064,14 +1141,24 @@ export component LibraryGrid inherits Rectangle { // *loaded window* while the viewport stays where it was, so // the cells are drawn thousands of rows away and the grid // looks empty until the user scrolls to find them. - property token: root.scroll-token; - changed token => { + function seek() { self.viewport-y = -min( max(0px, self.viewport-height - self.height), floor(root.scroll-to / max(1, root.columns)) * (root.cell-size + Theme.gap)); } + property token: root.scroll-token; + changed token => { self.seek(); } + + // Also on creation, which is what returning from the develop + // view needs. `show-library` gates an `if`, so the grid is built + // anew and `token` is *initialised* to the already-bumped value + // rather than changing to it — no `changed` handler fires, and + // without this the restored position would be dropped and the + // view would sit at the top. + init => { self.seek(); } + // Sized to the **whole library**, not the loaded window. The // scrollbar has to represent 23,971 images or there is no way to // reach image 20,000 — dragging it must be a real address, and the