diff --git a/Cargo.lock b/Cargo.lock index a8fb8c5..90491ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -160,6 +160,24 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "android-native-keyring-store" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "48c6349ddff23194f8fdce2ea8849380f5a4868c1648965b70e801e104cba9b3" +dependencies = [ + "base64", + "jni 0.21.1", + "keyring-core", + "log", + "ndk-context", + "regex", + "serde", + "serde_json", + "thiserror 2.0.20", + "tracing", +] + [[package]] name = "android-properties" version = "0.2.2" @@ -1376,9 +1394,11 @@ dependencies = [ name = "dr-plat" version = "0.1.0" dependencies = [ + "android-native-keyring-store", "dr-types", "env_logger", "keyring", + "keyring-core", "log", "thiserror 2.0.20", ] diff --git a/Cargo.toml b/Cargo.toml index 8f9ca96..e4f27d7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,6 +84,16 @@ base64 = "0.23" # (via ksecretd) both speak. keyring = { version = "4", features = ["v1"] } +# The Android half of the same project: a keyring-core CredentialStore backed +# by AndroidKeyStore AES-GCM over SharedPreferences (FR-PLAT-AND-1). It reads +# the JavaVM and Context from ndk-context, which android-activity populates +# before `android_main` runs, so no Kotlin shim of our own is needed. +# +# This is the keyring-core API, not the v1 `Entry` API the Linux path uses; +# the two impls are deliberately separate rather than sharing a code path. +android-native-keyring-store = "1.0.0" +keyring-core = "1" + # Decode. rawler is the pure-Rust decoder (D2); zune-jpeg decodes the # embedded previews rawler extracts. # Catalog. `bundled` compiles SQLite from source rather than linking the diff --git a/apps/darkroom-android/src/lib.rs b/apps/darkroom-android/src/lib.rs index 574d729..55def51 100644 --- a/apps/darkroom-android/src/lib.rs +++ b/apps/darkroom-android/src/lib.rs @@ -24,6 +24,14 @@ fn android_main(app: slint::android::AndroidApp) { .with_tag("DarkRoom"), ); + // Panics go to stderr, and Android discards stderr. Without this hook a + // worker thread that panics is invisible: the process survives, the + // channel it was writing to closes, and the UI reports only that + // something "failed unexpectedly" with no way to find out what. + std::panic::set_hook(Box::new(|info| { + log::error!("panic: {info}"); + })); + log::info!("DarkRoom v{}", env!("CARGO_PKG_VERSION")); if let Err(e) = slint::android::init(app) { diff --git a/core/dr-thumbs/src/lib.rs b/core/dr-thumbs/src/lib.rs index 712c588..49568b1 100644 --- a/core/dr-thumbs/src/lib.rs +++ b/core/dr-thumbs/src/lib.rs @@ -55,7 +55,58 @@ pub use error::ThumbError; /// per shard, so the 17k-image reference library lands in ~14 shards. pub const SHARD_MAX_BYTES: u64 = 25 * 1024 * 1024; -/// Long edge of a stored thumbnail. +/// Which resolution a stored thumbnail is. +/// +/// Two classes rather than one, because the grid zooms: 256px is right for a +/// wall of small cells and soft on a large one, while storing everything large +/// would take the reference library from ~200 MB to ~860 MB — and the shards +/// **sync**, so that is transfer cost on every device, not just disk. +/// +/// The discriminant is part of the store key, so both classes coexist and a +/// library thumbnailed at one size is not invalidated by the other appearing. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[repr(i64)] +pub enum ThumbSize { + /// Grid cells at their usual size. ~10 KB each. + Grid = 0, + /// Zoomed cells, the loupe, and the filmstrip. ~45 KB each, fetched only + /// where something actually asks for that detail. + Large = 1, +} + +impl ThumbSize { + /// Long edge in pixels. + pub fn edge(self) -> u32 { + match self { + ThumbSize::Grid => 256, + ThumbSize::Large => 1024, + } + } + + /// The smallest class that can fill a cell of this size without visibly + /// softening. + /// + /// Compared against the *drawn* size, so a high-DPI display asking for + /// 300 logical pixels at 2x gets the large class, as it should. + pub fn for_cell(pixels: u32) -> Self { + if pixels > ThumbSize::Grid.edge() { + ThumbSize::Large + } else { + ThumbSize::Grid + } + } + + fn from_i64(v: i64) -> Self { + match v { + 1 => ThumbSize::Large, + _ => ThumbSize::Grid, + } + } +} + +/// Long edge of a grid thumbnail. +/// +/// Kept as the name callers already use; prefer [`ThumbSize::edge`]. pub const THUMBNAIL_EDGE: u32 = 256; /// A thumbnail's bytes and dimensions. @@ -84,6 +135,10 @@ impl ThumbStore { index.pragma_update(None, "journal_mode", "WAL")?; index.pragma_update(None, "synchronous", "NORMAL")?; index.execute_batch(INDEX_SCHEMA)?; + // A store written before the size class existed holds grid-sized + // entries under a bare `file_id` key; bring it forward rather than + // discarding every thumbnail already fetched. + migrate_size_column(&index, "entries")?; Ok(Self { dir: dir.to_path_buf(), @@ -92,12 +147,12 @@ impl ThumbStore { } /// Fetch a thumbnail by file id. - pub fn get(&self, file_id: u64) -> Result, ThumbError> { + pub fn get(&self, file_id: u64, size: ThumbSize) -> Result, ThumbError> { let shard: Option = self .index .query_row( - "SELECT shard FROM entries WHERE file_id = ?1", - [file_id as i64], + "SELECT shard FROM entries WHERE file_id = ?1 AND size = ?2", + [file_id as i64, size as i64], |r| r.get(0), ) .ok(); @@ -109,8 +164,8 @@ impl ThumbStore { let conn = self.open_shard(shard as u32, false)?; let got = conn .query_row( - "SELECT width, height, bytes FROM thumbs WHERE file_id = ?1", - [file_id as i64], + "SELECT width, height, bytes FROM thumbs WHERE file_id = ?1 AND size = ?2", + [file_id as i64, size as i64], |r| { Ok(Thumbnail { width: r.get::<_, i64>(0)? as u32, @@ -128,11 +183,11 @@ impl ThumbStore { /// /// Cheaper than [`get`](Self::get) — the index alone answers it, with no /// shard opened and no blob read. - pub fn contains(&self, file_id: u64) -> bool { + pub fn contains(&self, file_id: u64, size: ThumbSize) -> bool { self.index .query_row( - "SELECT 1 FROM entries WHERE file_id = ?1", - [file_id as i64], + "SELECT 1 FROM entries WHERE file_id = ?1 AND size = ?2", + [file_id as i64, size as i64], |_| Ok(()), ) .is_ok() @@ -142,11 +197,11 @@ impl ThumbStore { /// /// The grid's real question — "what must I fetch for these cells" — asked /// in one pass rather than one query per cell. - pub fn missing(&self, file_ids: &[u64]) -> Vec { + pub fn missing(&self, file_ids: &[u64], size: ThumbSize) -> Vec { file_ids .iter() .copied() - .filter(|id| !self.contains(*id)) + .filter(|id| !self.contains(*id, size)) .collect() } @@ -155,13 +210,20 @@ impl ThumbStore { /// Re-storing an existing id overwrites in place rather than migrating it /// to the active shard: a sealed shard must stay byte-identical for other /// clients, and rewriting one would force them all to re-sync it. - pub fn put(&mut self, file_id: u64, thumb: &Thumbnail) -> Result { - if let Some(existing) = self.shard_of(file_id) { + pub fn put( + &mut self, + file_id: u64, + size: ThumbSize, + thumb: &Thumbnail, + ) -> Result { + if let Some(existing) = self.shard_of(file_id, size) { let conn = self.open_shard(existing, true)?; conn.execute( - "UPDATE thumbs SET width = ?2, height = ?3, bytes = ?4 WHERE file_id = ?1", + "UPDATE thumbs SET width = ?3, height = ?4, bytes = ?5 + WHERE file_id = ?1 AND size = ?2", rusqlite::params![ file_id as i64, + size as i64, thumb.width as i64, thumb.height as i64, thumb.bytes @@ -173,11 +235,13 @@ impl ThumbStore { let shard = self.active_shard(thumb.bytes.len() as u64)?; let conn = self.open_shard(shard, true)?; conn.execute( - "INSERT INTO thumbs(file_id, width, height, bytes) VALUES (?1, ?2, ?3, ?4) - ON CONFLICT(file_id) DO UPDATE SET + "INSERT INTO thumbs(file_id, size, width, height, bytes) + VALUES (?1, ?2, ?3, ?4, ?5) + ON CONFLICT(file_id, size) DO UPDATE SET width = excluded.width, height = excluded.height, bytes = excluded.bytes", rusqlite::params![ file_id as i64, + size as i64, thumb.width as i64, thumb.height as i64, thumb.bytes @@ -185,9 +249,15 @@ impl ThumbStore { )?; self.index.execute( - "INSERT INTO entries(file_id, shard, bytes) VALUES (?1, ?2, ?3) - ON CONFLICT(file_id) DO UPDATE SET shard = excluded.shard, bytes = excluded.bytes", - rusqlite::params![file_id as i64, shard as i64, thumb.bytes.len() as i64], + "INSERT INTO entries(file_id, size, shard, bytes) VALUES (?1, ?2, ?3, ?4) + ON CONFLICT(file_id, size) DO UPDATE SET + shard = excluded.shard, bytes = excluded.bytes", + rusqlite::params![ + file_id as i64, + size as i64, + shard as i64, + thumb.bytes.len() as i64 + ], )?; self.index.execute( "INSERT INTO shards(id, bytes, sealed) VALUES (?1, ?2, 0) @@ -198,11 +268,11 @@ impl ThumbStore { Ok(shard) } - fn shard_of(&self, file_id: u64) -> Option { + fn shard_of(&self, file_id: u64, size: ThumbSize) -> Option { self.index .query_row( - "SELECT shard FROM entries WHERE file_id = ?1", - [file_id as i64], + "SELECT shard FROM entries WHERE file_id = ?1 AND size = ?2", + [file_id as i64, size as i64], |r| r.get::<_, i64>(0), ) .ok() @@ -299,31 +369,43 @@ impl ThumbStore { let tx = self.index.unchecked_transaction()?; for &file_id in file_ids { - let found: Option<(i64, i64, i64)> = tx - .query_row( - "SELECT e.shard, e.bytes, s.sealed + // Every size class, not just one: an image browsed at two sizes has + // two entries, and reading a single row would leave the other's + // bytes on the shard's tally forever — sealing it early on space + // nothing occupies. + let rows: Vec<(i64, i64, i64, i64)> = { + let mut stmt = tx.prepare( + "SELECT e.size, e.shard, e.bytes, s.sealed FROM entries e JOIN shards s ON s.id = e.shard WHERE e.file_id = ?1", - [file_id as i64], - |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?)), - ) - .ok(); - - let Some((shard, bytes, sealed)) = found else { - // Never stored, or already forgotten. Not an error: a purge runs - // over whatever the catalog knew about, and a thumbnail that was - // never fetched is the normal case. - continue; + )?; + let mapped = stmt.query_map([file_id as i64], |r| { + Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?)) + })?; + mapped.collect::, _>>()? }; - tx.execute("DELETE FROM entries WHERE file_id = ?1", [file_id as i64])?; - forgotten += 1; + // Never stored, or already forgotten. Not an error: a purge runs + // over whatever the catalog knew about, and a thumbnail that was + // never fetched is the normal case. + if rows.is_empty() { + continue; + } - if sealed == 0 { - // The active shard is not yet anyone's cached copy, so its bytes - // can genuinely be reclaimed and the accounting corrected — which - // also means the shard does not seal prematurely on space that is - // no longer used. + for (size, shard, bytes, sealed) in rows { + tx.execute( + "DELETE FROM entries WHERE file_id = ?1 AND size = ?2", + [file_id as i64, size], + )?; + forgotten += 1; + + if sealed != 0 { + continue; + } + // The active shard is not yet anyone's cached copy, so its + // bytes can genuinely be reclaimed and the accounting + // corrected — which also means the shard does not seal + // prematurely on space that is no longer used. in_unsealed.push((file_id, shard as u32)); tx.execute( "UPDATE shards SET bytes = max(0, bytes - ?2) WHERE id = ?1", @@ -409,26 +491,41 @@ impl ThumbStore { rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY | rusqlite::OpenFlags::SQLITE_OPEN_NO_MUTEX, )?; - let mut stmt = src.prepare("SELECT file_id, width, height, bytes FROM thumbs")?; + // A shard from a client that predates the size class has no `size` + // column; its thumbnails are all grid-sized, which is what the + // fallback below assumes. + let has_size = src + .prepare("SELECT * FROM thumbs LIMIT 0") + .map(|stmt| stmt.column_names().iter().any(|c| *c == "size")) + .unwrap_or(false); + + let sql = if has_size { + "SELECT file_id, size, width, height, bytes FROM thumbs" + } else { + "SELECT file_id, 0 AS size, width, height, bytes FROM thumbs" + }; + + let mut stmt = src.prepare(sql)?; let incoming = stmt .query_map([], |r| { Ok(( r.get::<_, i64>(0)? as u64, + ThumbSize::from_i64(r.get::<_, i64>(1)?), Thumbnail { - width: r.get::<_, i64>(1)? as u32, - height: r.get::<_, i64>(2)? as u32, - bytes: r.get(3)?, + width: r.get::<_, i64>(2)? as u32, + height: r.get::<_, i64>(3)? as u32, + bytes: r.get(4)?, }, )) })? .collect::, _>>()?; let mut adopted = 0; - for (file_id, thumb) in incoming { - if self.contains(file_id) { + for (file_id, size, thumb) in incoming { + if self.contains(file_id, size) { continue; } - self.put(file_id, &thumb)?; + self.put(file_id, size, &thumb)?; adopted += 1; } Ok(adopted) @@ -446,9 +543,13 @@ pub struct ShardInfo { const INDEX_SCHEMA: &str = r#" CREATE TABLE IF NOT EXISTS entries ( - file_id INTEGER PRIMARY KEY, -- oc:fileid, stable across rename/move + file_id INTEGER NOT NULL, -- oc:fileid, stable across rename/move + -- Which resolution. Part of the key so both classes coexist: adding the + -- large class must not invalidate a library already thumbnailed small. + size INTEGER NOT NULL DEFAULT 0, shard INTEGER NOT NULL, - bytes INTEGER NOT NULL + bytes INTEGER NOT NULL, + PRIMARY KEY(file_id, size) ); CREATE INDEX IF NOT EXISTS entries_shard ON entries(shard); @@ -464,10 +565,65 @@ CREATE TABLE IF NOT EXISTS shards ( const SHARD_SCHEMA: &str = r#" CREATE TABLE IF NOT EXISTS thumbs ( - file_id INTEGER PRIMARY KEY, + file_id INTEGER NOT NULL, + size INTEGER NOT NULL DEFAULT 0, width INTEGER NOT NULL, height INTEGER NOT NULL, - bytes BLOB NOT NULL + bytes BLOB NOT NULL, + PRIMARY KEY(file_id, size) +); +"#; + +/// Bring a store written before the size class existed up to date. +/// +/// Those entries are all grid-sized, which is what the column defaults to, so +/// the migration is purely structural — no thumbnail is discarded and nothing +/// is re-fetched. A store that has never been opened by an older build runs +/// this as a no-op. +/// +/// Sealed shards *are* rewritten here, which normally the design forbids +/// (their immutability is what makes syncing cheap). It is acceptable exactly +/// once: every client migrates the same way, and the alternative is discarding +/// every thumbnail already fetched. +fn migrate_size_column(conn: &Connection, table: &str) -> Result<(), ThumbError> { + let has_size: bool = conn + .prepare(&format!("SELECT * FROM {table} LIMIT 0")) + .map(|stmt| stmt.column_names().iter().any(|c| *c == "size")) + .unwrap_or(true); + if has_size { + return Ok(()); + } + + // SQLite cannot add a column to a primary key, so the table is rebuilt. + let tx = conn.unchecked_transaction()?; + tx.execute_batch(&format!( + "ALTER TABLE {table} RENAME TO {table}_old; + {} + INSERT INTO {table} SELECT file_id, 0, {cols} FROM {table}_old; + DROP TABLE {table}_old;", + if table == "entries" { + INDEX_ENTRIES_TABLE + } else { + SHARD_SCHEMA + }, + cols = if table == "entries" { + "shard, bytes" + } else { + "width, height, bytes" + } + ))?; + tx.commit()?; + Ok(()) +} + +/// The `entries` table alone, for the rebuild above. +const INDEX_ENTRIES_TABLE: &str = r#" +CREATE TABLE entries ( + file_id INTEGER NOT NULL, + size INTEGER NOT NULL DEFAULT 0, + shard INTEGER NOT NULL, + bytes INTEGER NOT NULL, + PRIMARY KEY(file_id, size) ); "#; @@ -498,14 +654,14 @@ mod tests { // The point of the whole method: a purged photograph must not keep a // preview, on this device or any that syncs the shards. let (mut s, _d) = store(); - s.put(1, &thumb(1024)).unwrap(); - s.put(2, &thumb(1024)).unwrap(); + s.put(1, ThumbSize::Grid, &thumb(1024)).unwrap(); + s.put(2, ThumbSize::Grid, &thumb(1024)).unwrap(); assert_eq!(s.forget(&[1]).unwrap(), 1); - assert!(s.get(1).unwrap().is_none()); - assert!(!s.contains(1)); + assert!(s.get(1, ThumbSize::Grid).unwrap().is_none()); + assert!(!s.contains(1, ThumbSize::Grid)); // And its neighbour is untouched. - assert!(s.get(2).unwrap().is_some()); + assert!(s.get(2, ThumbSize::Grid).unwrap().is_some()); assert_eq!(s.len(), 1); } @@ -514,7 +670,7 @@ mod tests { // Otherwise the shard seals on space nothing is using, and the store // grows a shard per purge. let (mut s, _d) = store(); - s.put(1, &thumb(4096)).unwrap(); + s.put(1, ThumbSize::Grid, &thumb(4096)).unwrap(); let before = s.shards().unwrap()[0].bytes; s.forget(&[1]).unwrap(); @@ -529,7 +685,7 @@ mod tests { let (mut s, _d) = store(); let big = (SHARD_MAX_BYTES / 4) as usize; for id in 0..5 { - s.put(id, &thumb(big)).unwrap(); + s.put(id, ThumbSize::Grid, &thumb(big)).unwrap(); } let shards = s.shards().unwrap(); assert!(shards[0].sealed, "precondition: shard 0 is sealed"); @@ -542,7 +698,7 @@ mod tests { assert_eq!(s.forget(&[0]).unwrap(), 1); // Unreachable through the index, which is what matters to a reader... - assert!(s.get(0).unwrap().is_none()); + assert!(s.get(0, ThumbSize::Grid).unwrap().is_none()); // ...but the file itself is untouched. assert_eq!( std::fs::metadata(&sealed_path).unwrap().len(), @@ -560,7 +716,7 @@ mod tests { // A purge runs over whatever the catalog knew; most images never had a // thumbnail fetched. let (mut s, _d) = store(); - s.put(1, &thumb(512)).unwrap(); + s.put(1, ThumbSize::Grid, &thumb(512)).unwrap(); assert_eq!(s.forget(&[42, 43]).unwrap(), 0); assert_eq!(s.forget(&[]).unwrap(), 0); assert_eq!(s.len(), 1, "nothing else went"); @@ -570,7 +726,7 @@ mod tests { fn forgetting_twice_is_idempotent() { // An empty-trash retried after a partial failure runs over the same ids. let (mut s, _d) = store(); - s.put(1, &thumb(512)).unwrap(); + s.put(1, ThumbSize::Grid, &thumb(512)).unwrap(); assert_eq!(s.forget(&[1]).unwrap(), 1); assert_eq!(s.forget(&[1]).unwrap(), 0); } @@ -580,18 +736,101 @@ mod tests { // Restoring from the server's own trashbin, or re-adding the same file: // the id is stable, so the store must accept it back. let (mut s, _d) = store(); - s.put(1, &thumb(512)).unwrap(); + s.put(1, ThumbSize::Grid, &thumb(512)).unwrap(); s.forget(&[1]).unwrap(); - s.put(1, &thumb(512)).unwrap(); - assert!(s.get(1).unwrap().is_some()); + s.put(1, ThumbSize::Grid, &thumb(512)).unwrap(); + assert!(s.get(1, ThumbSize::Grid).unwrap().is_some()); + } + + + #[test] + fn the_two_size_classes_coexist() { + // Adding the large class must not evict or shadow the grid one: the + // same photograph is legitimately stored at both. + let (mut s, _d) = store(); + s.put(7, ThumbSize::Grid, &thumb(100)).unwrap(); + s.put(7, ThumbSize::Large, &thumb(900)).unwrap(); + + assert_eq!(s.get(7, ThumbSize::Grid).unwrap().unwrap().bytes.len(), 100); + assert_eq!( + s.get(7, ThumbSize::Large).unwrap().unwrap().bytes.len(), + 900 + ); + assert_eq!(s.len(), 2, "counted separately"); + } + + #[test] + fn a_missing_large_is_not_satisfied_by_the_grid_one() { + // Otherwise a zoomed cell would silently show a 256px thumbnail + // upscaled, which is the softness the large class exists to avoid. + let (mut s, _d) = store(); + s.put(7, ThumbSize::Grid, &thumb(100)).unwrap(); + assert!(s.contains(7, ThumbSize::Grid)); + assert!(!s.contains(7, ThumbSize::Large)); + assert!(s.get(7, ThumbSize::Large).unwrap().is_none()); + assert_eq!(s.missing(&[7], ThumbSize::Large), vec![7]); + } + + #[test] + fn forgetting_an_image_drops_every_size() { + // A purged photograph must leave no preview at any size — the shards + // sync, so a survivor keeps appearing on every other client. + let (mut s, _d) = store(); + s.put(7, ThumbSize::Grid, &thumb(100)).unwrap(); + s.put(7, ThumbSize::Large, &thumb(900)).unwrap(); + + assert_eq!(s.forget(&[7]).unwrap(), 2, "both entries counted"); + assert!(!s.contains(7, ThumbSize::Grid)); + assert!(!s.contains(7, ThumbSize::Large)); + assert_eq!(s.len(), 0); + } + + #[test] + fn the_cell_size_picks_the_class() { + assert_eq!(ThumbSize::for_cell(180), ThumbSize::Grid); + assert_eq!(ThumbSize::for_cell(256), ThumbSize::Grid); + // Past the grid class's own edge, upscaling would show. + assert_eq!(ThumbSize::for_cell(257), ThumbSize::Large); + assert_eq!(ThumbSize::for_cell(400), ThumbSize::Large); + } + + #[test] + fn a_store_written_before_the_size_class_keeps_its_thumbnails() { + // The migration case: an existing library must not lose the thumbnails + // it already paid to fetch, and its entries are all grid-sized. + let dir = std::env::temp_dir().join(format!("dr-thumbs-migrate-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + // An index in the old shape: no `size`, `file_id` alone as the key. + { + 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, 64, 0); + INSERT INTO entries(file_id, shard, bytes) VALUES (42, 0, 64);", + ) + .unwrap(); + } + + let s = ThumbStore::open(&dir).unwrap(); + assert!( + s.contains(42, ThumbSize::Grid), + "an existing entry survives as grid-sized" + ); + assert_eq!(s.len(), 1); } #[test] fn a_stored_thumbnail_round_trips() { let (mut s, _d) = store(); - s.put(1001, &thumb(1024)).unwrap(); + s.put(1001, ThumbSize::Grid, &thumb(1024)).unwrap(); - let got = s.get(1001).unwrap().expect("stored"); + let got = s.get(1001, ThumbSize::Grid).unwrap().expect("stored"); assert_eq!(got.width, 256); assert_eq!(got.bytes.len(), 1024); } @@ -599,30 +838,30 @@ mod tests { #[test] fn an_unknown_id_is_none_not_an_error() { let (s, _d) = store(); - assert!(s.get(9999).unwrap().is_none()); + assert!(s.get(9999, ThumbSize::Grid).unwrap().is_none()); } #[test] fn contains_avoids_reading_the_blob() { let (mut s, _d) = store(); - s.put(1, &thumb(512)).unwrap(); - assert!(s.contains(1)); - assert!(!s.contains(2)); + s.put(1, ThumbSize::Grid, &thumb(512)).unwrap(); + assert!(s.contains(1, ThumbSize::Grid)); + assert!(!s.contains(2, ThumbSize::Grid)); } #[test] fn missing_reports_only_what_is_absent() { let (mut s, _d) = store(); - s.put(1, &thumb(64)).unwrap(); - s.put(3, &thumb(64)).unwrap(); - assert_eq!(s.missing(&[1, 2, 3, 4]), vec![2, 4]); + s.put(1, ThumbSize::Grid, &thumb(64)).unwrap(); + s.put(3, ThumbSize::Grid, &thumb(64)).unwrap(); + assert_eq!(s.missing(&[1, 2, 3, 4], ThumbSize::Grid), vec![2, 4]); } #[test] fn everything_small_lands_in_one_shard() { let (mut s, _d) = store(); for id in 0..20 { - s.put(id, &thumb(1024)).unwrap(); + s.put(id, ThumbSize::Grid, &thumb(1024)).unwrap(); } assert_eq!(s.shards().unwrap().len(), 1); assert_eq!(s.len(), 20); @@ -634,7 +873,7 @@ mod tests { // Quarter-cap blobs: the fifth cannot fit alongside four others. let big = (SHARD_MAX_BYTES / 4) as usize; for id in 0..5 { - s.put(id, &thumb(big)).unwrap(); + s.put(id, ThumbSize::Grid, &thumb(big)).unwrap(); } let shards = s.shards().unwrap(); @@ -651,15 +890,15 @@ mod tests { let (mut s, _d) = store(); let big = (SHARD_MAX_BYTES / 4) as usize; for id in 0..5 { - s.put(id, &thumb(big)).unwrap(); + s.put(id, ThumbSize::Grid, &thumb(big)).unwrap(); } // Small enough to fit in the sealed shard, but it must not go there. - s.put(100, &thumb(16)).unwrap(); + s.put(100, ThumbSize::Grid, &thumb(16)).unwrap(); let shards = s.shards().unwrap(); assert!(shards[0].sealed); assert_eq!( - s.shard_of(100), + s.shard_of(100, ThumbSize::Grid), Some(1), "new writes go to the open shard, not back into a sealed one" ); @@ -672,13 +911,13 @@ mod tests { let (mut s, _d) = store(); let big = (SHARD_MAX_BYTES / 4) as usize; for id in 0..5 { - s.put(id, &thumb(big)).unwrap(); + s.put(id, ThumbSize::Grid, &thumb(big)).unwrap(); } - let original = s.shard_of(0).unwrap(); + let original = s.shard_of(0, ThumbSize::Grid).unwrap(); - s.put(0, &thumb(32)).unwrap(); - assert_eq!(s.shard_of(0), Some(original), "must stay put"); - assert_eq!(s.get(0).unwrap().unwrap().bytes.len(), 32, "but update"); + s.put(0, ThumbSize::Grid, &thumb(32)).unwrap(); + assert_eq!(s.shard_of(0, ThumbSize::Grid), Some(original), "must stay put"); + assert_eq!(s.get(0, ThumbSize::Grid).unwrap().unwrap().bytes.len(), 32, "but update"); } #[test] @@ -691,35 +930,35 @@ mod tests { #[test] fn reopening_finds_what_was_stored() { let (mut s, dir) = store(); - s.put(42, &thumb(256)).unwrap(); + s.put(42, ThumbSize::Grid, &thumb(256)).unwrap(); drop(s); let reopened = ThumbStore::open(&dir).unwrap(); - assert!(reopened.contains(42)); + assert!(reopened.contains(42, ThumbSize::Grid)); assert_eq!(reopened.len(), 1); } #[test] fn merging_another_clients_shard_adopts_only_what_is_new() { let (mut mine, _d1) = store(); - mine.put(1, &thumb(64)).unwrap(); + mine.put(1, ThumbSize::Grid, &thumb(64)).unwrap(); // A second store standing in for another device's downloaded shard. let other_dir = std::env::temp_dir().join(format!("dr-thumbs-other-{}", std::process::id())); let _ = std::fs::remove_dir_all(&other_dir); let mut theirs = ThumbStore::open(&other_dir).unwrap(); - theirs.put(1, &thumb(999)).unwrap(); // we already have this one - theirs.put(2, &thumb(64)).unwrap(); - theirs.put(3, &thumb(64)).unwrap(); + theirs.put(1, ThumbSize::Grid, &thumb(999)).unwrap(); // we already have this one + theirs.put(2, ThumbSize::Grid, &thumb(64)).unwrap(); + theirs.put(3, ThumbSize::Grid, &thumb(64)).unwrap(); let adopted = mine.merge_shard(&theirs.shard_path(0)).unwrap(); assert_eq!(adopted, 2, "only the two we lacked"); assert_eq!( - mine.get(1).unwrap().unwrap().bytes.len(), + mine.get(1, ThumbSize::Grid).unwrap().unwrap().bytes.len(), 64, "ours is kept, not overwritten" ); - assert!(mine.contains(2) && mine.contains(3)); + assert!(mine.contains(2, ThumbSize::Grid) && mine.contains(3, ThumbSize::Grid)); } #[test] @@ -729,7 +968,7 @@ mod tests { std::env::temp_dir().join(format!("dr-thumbs-idem-{}", std::process::id())); let _ = std::fs::remove_dir_all(&other_dir); let mut theirs = ThumbStore::open(&other_dir).unwrap(); - theirs.put(10, &thumb(64)).unwrap(); + theirs.put(10, ThumbSize::Grid, &thumb(64)).unwrap(); assert_eq!(mine.merge_shard(&theirs.shard_path(0)).unwrap(), 1); assert_eq!( diff --git a/docker/android/package.sh b/docker/android/package.sh index d3f27dc..9ad34ed 100755 --- a/docker/android/package.sh +++ b/docker/android/package.sh @@ -22,7 +22,9 @@ PKG_DIR="${REPO}/apps/darkroom-android/android" # The container writes here (see build.sh); the APK is assembled in the same # place so both halves of the build agree on one output directory. CACHE="${XDG_CACHE_HOME:-${HOME}/.cache}/darkroom-android" -OUT="${CACHE}/apk" +# Under the cache's target/ rather than beside it: the container writes to +# /work/target-android/apk, and that is the directory bind-mounted here. +OUT="${CACHE}/target/apk" APK="${OUT}/darkroom.apk" INSTALL=0 diff --git a/docs/milestone-v0.1.md b/docs/milestone-v0.1.md index 50ba0da..7935bfc 100644 --- a/docs/milestone-v0.1.md +++ b/docs/milestone-v0.1.md @@ -315,6 +315,7 @@ pipeline, no tiling, no masks. It is deliberately the thinnest thing that still | SAF enumeration too slow at 10k files | Medium | S10 measures before commitment; batch and cache aggressively | | Embedded previews too small or absent on some bodies | High | Known — Sony embeds small previews, some bodies none. M-11's fallback chain handles it; detect per camera model | | **rawler exposes only full-resolution previews** | **Confirmed** | Measured 2026-08-09: rawler 0.7.2's CR2 decoder implements `full_image` only; `thumbnail_image`/`preview_image` are unimplemented defaults. Every rung resolves to a 5472×3648 decode at ~250 ms, 5× over NFR-P13. CR2 does carry smaller IFDs, so the fix is our own IFD walk or an upstream contribution — not a change to callers | +| **Android secret storage unimplemented** | **Confirmed** | Needs no investigation — `PlatformSecretStore` on Android is unimplemented by design, and fails loudly rather than silently no-opping (`platform/dr-plat/src/secrets.rs`). The fix is a real Keystore-over-JNI implementation (FR-PLAT-AND-1), which is `dr-plat-android` work not yet started | | reqwest Android TLS worse than expected | Medium | D7 escape hatch: `tls_certs_only` with `webpki-roots` | | GPU vendor divergence on Android | Medium | Two vendors in CI from the start | | Scope creeps toward editing | **High** | §4 is explicit; v0.1 is read-only against the server | diff --git a/platform/dr-plat/Cargo.toml b/platform/dr-plat/Cargo.toml index 6892ba4..11fbc00 100644 --- a/platform/dr-plat/Cargo.toml +++ b/platform/dr-plat/Cargo.toml @@ -13,5 +13,9 @@ log.workspace = true [target.'cfg(all(unix, not(target_os = "android")))'.dependencies] keyring.workspace = true +[target.'cfg(target_os = "android")'.dependencies] +android-native-keyring-store.workspace = true +keyring-core.workspace = true + [dev-dependencies] env_logger.workspace = true diff --git a/platform/dr-plat/src/secrets.rs b/platform/dr-plat/src/secrets.rs index 7aa243b..d2d4188 100644 --- a/platform/dr-plat/src/secrets.rs +++ b/platform/dr-plat/src/secrets.rs @@ -166,38 +166,137 @@ fn map_err(e: keyring::Error) -> SecretError { } } -/// Placeholder for platforms without an implementation yet. +/// Keystore-backed implementation (FR-PLAT-AND-1). /// -/// Android needs Keystore-backed storage via JNI (FR-PLAT-AND-1). Failing -/// loudly is deliberate: a silent no-op store would look like it worked and -/// then lose the credential. -#[cfg(not(all(unix, not(target_os = "android"))))] -pub struct PlatformSecretStore; +/// `android-native-keyring-store` encrypts each secret with an AES-GCM key +/// held in `AndroidKeyStore` and files the ciphertext in SharedPreferences. +/// The key never leaves the Keystore, so the preferences file is useless on +/// its own. This is the current approach rather than the deprecated +/// `EncryptedSharedPreferences` (REQ §11). +/// +/// It finds the JavaVM and Context through `ndk-context`, which +/// `android-activity` initialises before `android_main` is called. Nothing +/// here is usable before that point — hence the lazy handle below. +#[cfg(target_os = "android")] +pub struct PlatformSecretStore { + /// Built on first use, not in `new()`: construction needs the ndk-context + /// to be live, and `new()` may run early. Cached because store names are + /// unique — building one per call would fail on the second call. + store: std::sync::OnceLock, String>>, +} -#[cfg(not(all(unix, not(target_os = "android"))))] +#[cfg(target_os = "android")] impl PlatformSecretStore { pub fn new() -> Self { - Self + Self { + store: std::sync::OnceLock::new(), + } + } + + fn store(&self) -> Result<&std::sync::Arc, SecretError> { + self.store + .get_or_init(|| { + android_native_keyring_store::Store::new().map_err(|e| e.to_string()) + }) + .as_ref() + .map_err(|e| SecretError::Unavailable(e.clone())) + } + + /// A credential specifier for one secret. Filed under the same + /// service/key pair as the Linux path, so the two platforms agree on + /// naming even though the backing stores differ. + fn entry( + &self, + r: &SecretRef, + ) -> Result { + use keyring_core::api::CredentialStoreApi; + + self.store()? + .build(SERVICE, &r.entry_key(), None) + .map_err(map_err) } } -#[cfg(not(all(unix, not(target_os = "android"))))] +#[cfg(target_os = "android")] impl Default for PlatformSecretStore { fn default() -> Self { Self::new() } } -#[cfg(not(all(unix, not(target_os = "android"))))] +#[cfg(target_os = "android")] +impl SecretStore for PlatformSecretStore { + fn store(&self, secret_ref: &SecretRef, secret: &str) -> Result<(), SecretError> { + use keyring_core::api::CredentialApi; + + self.entry(secret_ref)?.set_password(secret).map_err(map_err) + } + + fn retrieve(&self, secret_ref: &SecretRef) -> Result { + use keyring_core::api::CredentialApi; + + self.entry(secret_ref)?.get_password().map_err(map_err) + } + + fn delete(&self, secret_ref: &SecretRef) -> Result<(), SecretError> { + use keyring_core::api::CredentialApi; + + match self.entry(secret_ref)?.delete_credential() { + Ok(()) => Ok(()), + // Logout must be idempotent, as on Linux. + Err(keyring_core::Error::NoEntry) => Ok(()), + Err(e) => Err(map_err(e)), + } + } + + fn is_available(&self) -> bool { + // Unlike Linux there is no daemon to be absent: if the store builds, + // Keystore is there. Building is the whole probe. + self.store().is_ok() + } +} + +#[cfg(target_os = "android")] +fn map_err(e: keyring_core::Error) -> SecretError { + match e { + keyring_core::Error::NoEntry => SecretError::NotFound, + keyring_core::Error::NoStorageAccess(e) => SecretError::Unavailable(e.to_string()), + keyring_core::Error::PlatformFailure(e) => SecretError::Unavailable(e.to_string()), + other => SecretError::Other(other.to_string()), + } +} + +/// Placeholder for platforms without an implementation yet. +/// +/// Failing loudly is deliberate: a silent no-op store would look like it +/// worked and then lose the credential. +#[cfg(not(any(all(unix, not(target_os = "android")), target_os = "android")))] +pub struct PlatformSecretStore; + +#[cfg(not(any(all(unix, not(target_os = "android")), target_os = "android")))] +impl PlatformSecretStore { + pub fn new() -> Self { + Self + } +} + +#[cfg(not(any(all(unix, not(target_os = "android")), target_os = "android")))] +impl Default for PlatformSecretStore { + fn default() -> Self { + Self::new() + } +} + +#[cfg(not(any(all(unix, not(target_os = "android")), target_os = "android")))] impl SecretStore for PlatformSecretStore { fn store(&self, _r: &SecretRef, _s: &str) -> Result<(), SecretError> { Err(SecretError::Unavailable( - "Keystore-backed storage is not implemented on this platform yet".into(), + "no secret store is implemented for this platform".into(), )) } fn retrieve(&self, _r: &SecretRef) -> Result { Err(SecretError::Unavailable( - "Keystore-backed storage is not implemented on this platform yet".into(), + "no secret store is implemented for this platform".into(), )) } fn delete(&self, _r: &SecretRef) -> Result<(), SecretError> { diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index af36cbf..2e0452b 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -249,11 +249,18 @@ fn sync_rows( ) { use slint::Model as _; - let (current, samples) = match session.borrow().as_ref() { - Some(s) => (s.rows(), s.curve_samples()), - None => (Vec::new(), Vec::new()), + let current = match session.borrow().as_ref() { + Some(s) => s.rows(), + None => Vec::new(), }; + // Whether the drawn curve has to be resampled. Sampling runs the spline 96 + // times and builds a fresh model, and `sync_rows` is called on *every* + // parameter event — so doing it unconditionally spent that on every + // exposure or contrast drag, none of which can change the curve's shape. + // Only a moved point can, and the in-place update below is what knows. + let mut curve_moved = false; + if current.len() == rows.row_count() { for (i, mut row) in current.into_iter().enumerate() { let existing = rows.row_data(i); @@ -268,8 +275,13 @@ fn sync_rows( // compares by identity, so a brand-new points model would make // every curve row look changed on every event. if let Some(previous) = existing.as_ref() { - if update_points_in_place(&previous.points, &row.points) { - row.points = previous.points.clone(); + match update_points_in_place(&previous.points, &row.points) { + PointsUpdate::Moved => { + curve_moved = true; + row.points = previous.points.clone(); + } + PointsUpdate::Unchanged => row.points = previous.points.clone(), + PointsUpdate::Incompatible => {} } } @@ -281,10 +293,21 @@ fn sync_rows( } } else { // A different image, so the control set itself changed. Rebuilding - // is correct here — there is no drag to preserve. + // is correct here — there is no drag to preserve, and the new image's + // curve must be drawn whatever shape it is in. rows.set_vec(current); + curve_moved = true; } + if !curve_moved { + return; + } + + let samples = match session.borrow().as_ref() { + Some(s) => s.curve_samples(), + None => Vec::new(), + }; + // The drawn curve follows the points. Replacing this model wholesale is // safe where replacing `rows` was not: nothing in it is a drag target. window.set_curve_samples(slint::ModelRc::new(slint::VecModel::from(samples))); @@ -298,12 +321,13 @@ fn sync_rows( fn update_points_in_place( existing: &slint::ModelRc, fresh: &slint::ModelRc, -) -> bool { +) -> PointsUpdate { use slint::Model as _; if existing.row_count() != fresh.row_count() { - return false; + return PointsUpdate::Incompatible; } + let mut moved = false; for i in 0..fresh.row_count() { let (Some(new), Some(old)) = (fresh.row_data(i), existing.row_data(i)) else { continue; @@ -312,9 +336,28 @@ fn update_points_in_place( // — the same reasoning as the row-level check above. if new != old { existing.set_row_data(i, new); + moved = true; } } - true + if moved { + PointsUpdate::Moved + } else { + PointsUpdate::Unchanged + } +} + +/// What [`update_points_in_place`] found, which decides two things: whether the +/// existing points model can be kept, and whether the drawn curve needs +/// resampling. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum PointsUpdate { + /// Lengths differ. The caller must take the fresh model wholesale — the + /// control set itself changed and there is no drag worth preserving. + Incompatible, + /// At least one coordinate was written through. + Moved, + /// Every coordinate already matched. + Unchanged, } /// TRACES: M-13 | M-14 @@ -1156,4 +1199,46 @@ mod tests { assert!(!is_supported(Path::new("a.txt"))); assert!(!is_supported(Path::new("noextension"))); } + + fn points(values: &[f32]) -> slint::ModelRc { + slint::ModelRc::new(slint::VecModel::from(values.to_vec())) + } + + #[test] + fn an_unmoved_curve_reports_no_change() { + // What spares every non-curve drag the 96-sample spline evaluation. + let existing = points(&[0.0, 0.0, 1.0, 1.0]); + let fresh = points(&[0.0, 0.0, 1.0, 1.0]); + assert_eq!( + update_points_in_place(&existing, &fresh), + PointsUpdate::Unchanged + ); + } + + #[test] + fn a_moved_point_reports_the_change_and_is_written_through() { + use slint::Model as _; + + let existing = points(&[0.0, 0.0, 1.0, 1.0]); + let fresh = points(&[0.0, 0.25, 1.0, 1.0]); + assert_eq!( + update_points_in_place(&existing, &fresh), + PointsUpdate::Moved + ); + // Written into the *existing* model: keeping its identity is what + // stops the drag's own TouchArea being destroyed mid-gesture. + assert_eq!(existing.row_data(1), Some(0.25)); + } + + #[test] + fn a_different_point_count_is_incompatible() { + // A different image, so there is no drag to preserve and the caller + // must take the fresh model wholesale. + let existing = points(&[0.0, 0.0]); + let fresh = points(&[0.0, 0.0, 1.0, 1.0]); + assert_eq!( + update_points_in_place(&existing, &fresh), + PointsUpdate::Incompatible + ); + } } diff --git a/ui/dr-ui/src/library.rs b/ui/dr-ui/src/library.rs index 97a2615..c6b38d3 100644 --- a/ui/dr-ui/src/library.rs +++ b/ui/dr-ui/src/library.rs @@ -693,6 +693,12 @@ pub struct ThumbnailRequest { pub size: u64, /// Catalog row, so EXIF read from the header can be written back. pub image_id: i64, + /// Which resolution this cell needs, from how large it is drawn. A zoomed + /// grid asks for the large class; a wall of small cells does not. + /// + /// Named apart from `size`, which is the file's length in bytes — the two + /// are unrelated and confusing them would fetch the wrong thing. + pub thumb_size: dr_thumbs::ThumbSize, /// Whether this image still needs its EXIF read. Where false the header is /// still fetched — the preview needs it — but nothing is parsed or written. pub needs_metadata: bool, @@ -791,7 +797,7 @@ pub fn spawn_thumbnails( let stored = req .file_id .zip(store.as_ref()) - .and_then(|(id, s)| s.get(id).ok().flatten()); + .and_then(|(id, s)| s.get(id, req.thumb_size).ok().flatten()); match stored.map(|t| dr_thumbs::decode_rgba(&t.bytes)) { Some(Ok((width, height, rgba))) => { @@ -980,7 +986,7 @@ async fn fetch_one( Ok(p) => p, Err(e) => return fail(e.to_string()), }; - preview.downscale_to(THUMBNAIL_EDGE); + preview.downscale_to(req.thumb_size.edge()); // Persist for next time, and for every other client that syncs the shard. // A store failure is logged and dropped: the pixels are already in hand, @@ -994,7 +1000,7 @@ async fn fetch_one( height: preview.height, bytes: encoded, }; - if let Err(e) = store.put(file_id, &thumb) { + if let Err(e) = store.put(file_id, req.thumb_size, &thumb) { log::debug!("storing thumbnail {file_id}: {e}"); } } @@ -1368,6 +1374,9 @@ fn next_outstanding( let rows = stmt .query_map([limit as i64], |r| { Ok(ThumbnailRequest { + // The sweep indexes dates, and reads headers only — the size + // never reaches a fetch, but it must name something. + thumb_size: dr_thumbs::ThumbSize::Grid, // Row index is meaningless here — the sweep touches no grid // cell, so nothing consumes it. row: 0, @@ -1749,6 +1758,65 @@ mod tests { assert_eq!(seen, chunk); } + + #[test] + fn a_scrub_ordinal_matches_the_grid_position() { + // The scrub's count and the grid's window must use *identical* + // predicates and ordering, or the view lands somewhere else. Counting + // only dated images against a grid that also shows undated ones put a + // click near the end of the axis near the top of the library. + let catalog = Catalog::in_memory().unwrap(); + let c = catalog.connection(); + c.execute( + "INSERT INTO roots(id, kind, label) VALUES (1, 'remote', 'lib')", + [], + ) + .unwrap(); + + // A mix: dated, undated, and one shadowed by a RAW sibling. + for (id, name, captured, shadow) in [ + (1i64, "a.CR2", Some(100i64), None), + (2, "b.CR2", Some(200), None), + (3, "b.JPG", Some(200), Some(2i64)), + (4, "c.CR2", Some(300), None), + (5, "d.CR2", None, None), + ] { + c.execute( + "INSERT INTO images(id, root_id, source_ref, captured_at, shadowed_by, added_at) + VALUES (?1, 1, ?2, ?3, ?4, 0)", + rusqlite::params![id, name, captured, shadow], + ) + .unwrap(); + } + + // The grid's own window, in its own order. + let cells = read_cells(&catalog, 0, 100).unwrap(); + let names: Vec<&str> = cells.iter().map(|c| c.name.as_str()).collect(); + assert_eq!( + names, + vec!["a.CR2", "b.CR2", "c.CR2", "d.CR2"], + "shadowed hidden, undated last" + ); + + // Scrubbing to each image's instant must give its index in that list. + for (when, expected) in [(100i64, 0usize), (200, 1), (300, 2)] { + let ordinal: i64 = c + .query_row( + "SELECT count(*) FROM images + WHERE shadowed_by IS NULL + AND captured_at IS NOT NULL + AND captured_at < ?1", + [when], + |r| r.get(0), + ) + .unwrap(); + assert_eq!( + ordinal as usize, expected, + "scrubbing to {when} must land on grid row {expected}" + ); + } + } + #[test] fn catalog_paths_separate_accounts() { // Two accounts on one machine must not share an index, or one @@ -2076,9 +2144,7 @@ mod tests { let mut store = ThumbStore::open(&dir).unwrap(); let bytes = dr_thumbs::encode_rgba(32, 32, &rgba).unwrap(); store - .put( - 4242, - &dr_thumbs::Thumbnail { + .put(4242, dr_thumbs::ThumbSize::Grid, &dr_thumbs::Thumbnail { width: 32, height: 32, bytes, @@ -2088,7 +2154,7 @@ mod tests { } let store = ThumbStore::open(&dir).unwrap(); - let stored = store.get(4242).unwrap().expect("persisted"); + let stored = store.get(4242, dr_thumbs::ThumbSize::Grid).unwrap().expect("persisted"); let (w, h, out) = dr_thumbs::decode_rgba(&stored.bytes).unwrap(); assert_eq!((w, h), (32, 32)); // Lossy, so compare approximately — a blue-ish pixel must stay blue. diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index 413df20..f1b3850 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -35,6 +35,15 @@ const INITIAL_WINDOW: usize = 60; /// re-query on every scroll tick. const MIN_WINDOW: usize = 24; +/// Cell-size bounds for the grid's zoom. +/// +/// The lower end is where a thumbnail stops being recognisable; the upper is +/// where one screen holds so few that the grid stops being a grid. Past 256px +/// the large thumbnail class is fetched, so the top of this range is sharp +/// rather than upscaled. +const MIN_CELL_SIZE: f32 = 90.0; +const MAX_CELL_SIZE: f32 = 420.0; + /// Library state for the running window. pub struct LibraryController { /// Shared with [`crate::collections_ui`], which edits collections against @@ -93,6 +102,12 @@ pub struct LibraryController { /// view as you zoom. timeline_zoom: RefCell, timeline_centre: RefCell>, + /// Pinch ratio accumulated since the last zoom step was taken. + /// + /// A pinch is continuous and zoom levels are discrete, so the ratio is + /// held until it reaches a doubling. Without it a slow spread would either + /// do nothing or, if each update were rounded, leap several levels. + pinch_accum: RefCell, /// The instant the grid is showing. `None` until the user has moved the /// timeline, which is what leaves the marker resting at the middle. current_bucket: RefCell>, @@ -144,6 +159,7 @@ impl LibraryController { viewing_trash: std::cell::Cell::new(false), timeline_zoom: RefCell::new(0), timeline_centre: RefCell::new(None), + pinch_accum: RefCell::new(1.0), current_bucket: RefCell::new(None), filter: RefCell::new(library::RatingFilter::default()), sidecar_timer: RefCell::new(None), @@ -848,6 +864,8 @@ fn request_thumbnails(window: &AppWindow, ctl: &Rc) { }; let wanted: Vec = { + // The drawn cell size decides which class to ask for. + let cell_pixels = window.get_library_cell_size().max(1.0) as u32; let paths = ctl.paths.borrow(); let file_ids = ctl.file_ids.borrow(); let sizes = ctl.sizes.borrow(); @@ -859,6 +877,10 @@ fn request_thumbnails(window: &AppWindow, ctl: &Rc) { .enumerate() .filter(|(i, _)| requested.insert(*i)) .map(|(i, p)| library::ThumbnailRequest { + // Chosen from how large the cell is actually drawn, so a + // zoomed grid asks for detail a 256px thumbnail cannot give + // and a wall of small cells does not pay for it. + thumb_size: dr_thumbs::ThumbSize::for_cell(cell_pixels), row: i, path: p.clone(), file_id: file_ids.get(i).copied().flatten(), @@ -1013,6 +1035,35 @@ fn drain_thumbnails( *ctl.thumb_timer.borrow_mut() = Some(timer); } +/// Move the timeline's zoom by whole levels. +/// +/// Shared by the wheel and the pinch so the two cannot drift apart in how they +/// clamp, or in where they choose to centre. +fn apply_zoom(window: &AppWindow, ctl: &Rc, delta: i32) { + let borrow = ctl.catalog.borrow(); + let Some(catalog) = borrow.as_ref() else { + return; + }; + + // Bounded: past ~2^12 the window is minutes wide and every bucket is + // empty, which reads as a broken axis rather than a deep zoom. + let next = (*ctl.timeline_zoom.borrow() + delta).clamp(0, 12); + if next == *ctl.timeline_zoom.borrow() { + return; + } + *ctl.timeline_zoom.borrow_mut() = next; + + // Zooming fully out forgets the centre, so the axis returns to describing + // the whole library rather than a remembered position. + if next == 0 { + *ctl.timeline_centre.borrow_mut() = None; + } else if ctl.timeline_centre.borrow().is_none() { + // First zoom centres on wherever the grid is, else the middle. + *ctl.timeline_centre.borrow_mut() = *ctl.current_bucket.borrow(); + } + refresh_timeline(window, catalog, ctl); +} + /// Push shards and the catalog to the server, and take what it has. /// /// Fired after the sweep completes, when there is a finished index worth @@ -1378,13 +1429,26 @@ fn scrub_to(window: &AppWindow, ctl: &Rc, when: i64) { let Some(catalog) = borrow.as_ref() else { return; }; - // How many dated images precede this instant, in the same order the - // grid uses. That ordinal *is* the scroll offset. + // How many rows precede this instant **in the grid's own ordering**. + // That ordinal is the scroll offset, so any disagreement with the + // grid's query lands the view somewhere else entirely. + // + // The earlier version counted only dated images. With 2,400 of 19,841 + // dated, a click near the end of the axis produced an ordinal of ~2,400 + // against a grid of 19,841 rows — the view landed near the top however + // far down the axis the pointer went. + // + // Undated images sort last (`captured_at IS NULL` first in the ORDER + // BY), so they never precede a dated one and the predicate below stays + // a simple `<`. Shadowed rows are excluded here exactly as the grid + // excludes them. catalog .connection() .query_row( "SELECT count(*) FROM images - WHERE captured_at IS NOT NULL AND captured_at < ?1", + WHERE shadowed_by IS NULL + AND captured_at IS NOT NULL + AND captured_at < ?1", [when], |r| r.get::<_, i64>(0), ) @@ -1493,6 +1557,42 @@ where }); } + // Ctrl+wheel or pinch over the grid resizes the cells. + // + // Geometric steps rather than fixed pixels: the same gesture should feel + // the same at 90px and at 400px, and a linear step is imperceptible at one + // end and violent at the other. + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + window.on_library_zoom_cells(move |delta| { + let Some(w) = weak.upgrade() else { return }; + + let current = w.get_library_cell_size(); + let next = if delta > 0 { + current * 1.25 + } else { + current / 1.25 + } + .clamp(MIN_CELL_SIZE, MAX_CELL_SIZE); + + if (next - current).abs() < 0.5 { + return; + } + w.set_library_cell_size(next); + + // Crossing the class boundary means the visible cells now want a + // resolution the store may not hold, and the window's capacity + // changed with the cell size. Both are answered by reloading. + let was = dr_thumbs::ThumbSize::for_cell(current as u32); + let now = dr_thumbs::ThumbSize::for_cell(next as u32); + if was != now { + ctl.requested.borrow_mut().clear(); + } + load_window(&w, &ctl); + }); + } + // Explicit sync, for when the user wants the exchange now rather than // after the next sweep. { @@ -1576,65 +1676,103 @@ where { let weak = window.as_weak(); let ctl = ctl.clone(); - window.on_library_timeline_pan(move |buckets| { + window.on_library_timeline_pan(move |fraction| { let Some(w) = weak.upgrade() else { return }; let borrow = ctl.catalog.borrow(); let Some(catalog) = borrow.as_ref() else { return }; let Some(full) = catalog_span(catalog) else { return }; - // Shift the centre by whole buckets of the *current* span, so a - // pan moves by what the user can see rather than a fixed duration. + // A fraction of the *visible* span, so dragging half the axis + // moves half a span's worth of time whatever the zoom — and a + // small movement produces a small shift rather than nothing. let zoom = *ctl.timeline_zoom.borrow(); let (from, to) = zoomed_span(full, zoom, *ctl.timeline_centre.borrow()); - let step = ((to - from) / 40).max(1); + let shift = ((to - from) as f64 * fraction as f64) as i64; + if shift == 0 { + return; + } let centre = ctl .timeline_centre .borrow() .unwrap_or((from + to) / 2) - + step * buckets as i64; + + shift; *ctl.timeline_centre.borrow_mut() = Some(centre.clamp(full.0, full.1)); refresh_timeline(&w, catalog, &ctl); }); } // The wheel zooms the axis: a sidebar is a scale, not a list. + // + // Shares `apply_zoom` with the pinch handler so the two cannot drift apart + // in how they clamp or where they centre. { let weak = window.as_weak(); let ctl = ctl.clone(); window.on_library_timeline_zoom(move |delta| { - let Some(w) = weak.upgrade() else { return }; - let borrow = ctl.catalog.borrow(); - let Some(catalog) = borrow.as_ref() else { return }; - - // Bounded: past ~2^12 the window is minutes wide and every bucket - // is empty, which reads as a broken axis rather than a deep zoom. - let next = (*ctl.timeline_zoom.borrow() + delta).clamp(0, 12); - if next == *ctl.timeline_zoom.borrow() { - return; + if let Some(w) = weak.upgrade() { + apply_zoom(&w, &ctl, delta); } - *ctl.timeline_zoom.borrow_mut() = next; - - // Zooming fully out forgets the centre, so the axis returns to - // describing the whole library rather than a remembered position. - if next == 0 { - *ctl.timeline_centre.borrow_mut() = None; - } else if ctl.timeline_centre.borrow().is_none() { - // First zoom centres on wherever the grid is, else the middle. - *ctl.timeline_centre.borrow_mut() = *ctl.current_bucket.borrow(); - } - refresh_timeline(&w, catalog, &ctl); }); } // Dragging the histogram moves the grid through time. + // + // The fraction is interpolated across the visible span rather than snapped + // to a bucket edge, so a slow drag advances continuously instead of sitting + // still and then jumping a whole month. { let weak = window.as_weak(); let ctl = ctl.clone(); - window.on_library_scrub(move |when| { - if let Some(w) = weak.upgrade() { - scrub_to(&w, &ctl, when as i64); + window.on_library_scrub_fraction(move |f| { + let Some(w) = weak.upgrade() else { return }; + let borrow = ctl.catalog.borrow(); + let Some(catalog) = borrow.as_ref() else { return }; + let Some(full) = catalog_span(catalog) else { return }; + + let (from, to) = zoomed_span( + full, + *ctl.timeline_zoom.borrow(), + *ctl.timeline_centre.borrow(), + ); + let when = from + ((to - from) as f64 * f.clamp(0.0, 1.0) as f64) as i64; + drop(borrow); + scrub_to(&w, &ctl, when); + }); + } + + // Pinch, for tablet: no wheel there, so this is the only way to reach the + // axis's zoom with a finger. + // + // A continuous ratio against discrete zoom levels, so the accumulated + // ratio is held and a level is taken each time it passes a doubling. That + // keeps a slow spread from either doing nothing or leaping several levels. + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + window.on_library_timeline_pinch(move |ratio| { + let Some(w) = weak.upgrade() else { return }; + if !(0.01..=100.0).contains(&ratio) { + return; } + + let mut accum = ctl.pinch_accum.borrow_mut(); + *accum *= ratio; + + // Whole doublings out of the accumulated ratio, remainder carried. + // + // `log2` rather than repeated halving: one update can carry a + // large ratio — a fast spread, or a trackpad reporting coarsely — + // and stepping once per update would turn an 8× pinch into a + // single level instead of three. + let steps = accum.log2().trunc() as i32; + if steps == 0 { + return; + } + *accum /= (2.0f32).powi(steps); + drop(accum); + + apply_zoom(&w, &ctl, steps); }); } @@ -1818,6 +1956,139 @@ mod tests { use super::*; + + /// The instant a scrub fraction names within a span, as the handler + /// computes it. + fn instant_at(span: (i64, i64), f: f32) -> i64 { + span.0 + ((span.1 - span.0) as f64 * f.clamp(0.0, 1.0) as f64) as i64 + } + + #[test] + fn a_scrub_fraction_interpolates_within_the_span() { + // The point of going fractional: a slow drag must advance + // continuously rather than sitting still until it crosses a bucket + // edge and then jumping a whole month. + let span = (0, 1_000); + assert_eq!(instant_at(span, 0.0), 0); + assert_eq!(instant_at(span, 0.5), 500); + assert_eq!(instant_at(span, 1.0), 1_000); + // Distinct fractions inside one bucket must give distinct instants. + assert_ne!(instant_at(span, 0.10), instant_at(span, 0.11)); + } + + #[test] + fn a_scrub_fraction_outside_the_axis_is_clamped() { + // A drag that leaves the widget still reports a position; it must land + // at an end rather than off the timeline. + let span = (100, 200); + assert_eq!(instant_at(span, -3.0), 100); + assert_eq!(instant_at(span, 9.9), 200); + } + + /// One pinch update, as the handler applies it: fold the ratio in, take + /// out whole doublings, carry the remainder. + fn pinch_step(accum: &mut f32, ratio: f32) -> i32 { + *accum *= ratio; + let steps = accum.log2().trunc() as i32; + if steps != 0 { + *accum /= (2.0f32).powi(steps); + } + steps + } + + #[test] + fn a_pinch_accumulates_until_it_reaches_a_doubling() { + // A continuous gesture against discrete levels. Small spreads must + // accumulate rather than each being rounded to a step. + let mut accum = 1.0f32; + let mut steps = 0; + for _ in 0..12 { + steps += pinch_step(&mut accum, 1.1); + } + // 1.1^12 is 3.138x, which is log2 = 1.65 — one whole doubling, with + // the rest carried rather than discarded or rounded up. + assert_eq!(steps, 1); + assert!((accum - 1.569).abs() < 0.01, "remainder carried: {accum}"); + } + + #[test] + fn one_large_pinch_yields_every_level_it_crossed() { + // The bug this replaced: stepping at most once per update turned an + // 8x spread — three doublings — into a single zoom level, so a fast + // gesture lost most of its travel. + let mut accum = 1.0f32; + assert_eq!(pinch_step(&mut accum, 8.0), 3); + assert!((accum - 1.0).abs() < 0.001); + + let mut accum = 1.0f32; + assert_eq!(pinch_step(&mut accum, 0.125), -3); + } + + #[test] + fn pinching_in_and_back_out_returns_to_where_it_started() { + let mut accum = 1.0f32; + let steps: i32 = [2.0f32, 2.0, 0.5, 0.5] + .iter() + .map(|r| pinch_step(&mut accum, *r)) + .sum(); + assert_eq!(steps, 0); + assert!((accum - 1.0).abs() < 0.001, "no drift: {accum}"); + } + + #[test] + fn a_pinch_below_a_doubling_takes_no_step() { + // Otherwise the axis would flicker between levels on the smallest + // finger movement. + let mut accum = 1.0f32; + assert_eq!(pinch_step(&mut accum, 1.4), 0); + assert_eq!(pinch_step(&mut accum, 0.72), 0, "back roughly to 1.0"); + } + + + /// One zoom step, as the handler applies it. + fn zoom_cell(current: f32, delta: i32) -> f32 { + let next = if delta > 0 { + current * 1.25 + } else { + current / 1.25 + }; + next.clamp(MIN_CELL_SIZE, MAX_CELL_SIZE) + } + + #[test] + fn cell_zoom_steps_geometrically_and_reverses() { + // Geometric so the gesture feels the same at either end; a fixed pixel + // step is imperceptible at 400px and violent at 90px. + let a = zoom_cell(180.0, 1); + assert!((a - 225.0).abs() < 0.01); + assert!((zoom_cell(a, -1) - 180.0).abs() < 0.01, "in then out returns"); + } + + #[test] + fn cell_zoom_stays_within_its_bounds() { + let mut size = 180.0; + for _ in 0..40 { + size = zoom_cell(size, 1); + } + assert_eq!(size, MAX_CELL_SIZE); + + for _ in 0..40 { + size = zoom_cell(size, -1); + } + assert_eq!(size, MIN_CELL_SIZE); + } + + #[test] + fn zooming_past_the_grid_class_asks_for_the_large_one() { + use dr_thumbs::ThumbSize; + // The point of the second class: past 256px a grid thumbnail is being + // upscaled, and the softness shows. + assert_eq!(ThumbSize::for_cell(180), ThumbSize::Grid); + assert_eq!(ThumbSize::for_cell(zoom_cell(225.0, 1) as u32), ThumbSize::Large); + // And zooming back down does not keep paying for it. + assert_eq!(ThumbSize::for_cell(zoom_cell(281.0, -1) as u32), ThumbSize::Grid); + } + #[test] fn zoom_zero_is_the_whole_library() { let full = (1_000, 2_000); diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 6b36148..234ee96 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -223,6 +223,8 @@ export component AppWindow inherits Window { callback library-timeline-zoom(int); in-out property library-columns: 1; in property library-syncing: false; + in-out property library-cell-size: 180px; + callback library-zoom-cells(int); in property library-scroll-to: 0; in property library-scroll-token: 0; callback library-sync-now(); @@ -474,6 +476,8 @@ export component AppWindow inherits Window { timeline-pan(d) => { root.library-timeline-pan(d); } timeline-zoom(d) => { root.library-timeline-zoom(d); } syncing: root.library-syncing; + cell-size: root.library-cell-size; + zoom-cells(d) => { root.library-zoom-cells(d); } scroll-to: root.library-scroll-to; scroll-token: root.library-scroll-token; sync-now() => { root.library-sync-now(); } diff --git a/ui/dr-ui/ui/library.slint b/ui/dr-ui/ui/library.slint index e112ea5..5871a61 100644 --- a/ui/dr-ui/ui/library.slint +++ b/ui/dr-ui/ui/library.slint @@ -510,9 +510,16 @@ export component LibraryGrid inherits Rectangle { /// rests at the middle rather than implying a choice not yet made. in property timeline-anchored: false; - callback scrub(int); + /// A fraction along the visible span, not a bucket index — the timeline + /// interpolates so a slow drag tracks the finger rather than snapping. + callback scrub-fraction(float); callback timeline-pan(float); callback timeline-zoom(int); + /// Pinch ratio: above 1 spreads (zoom in), below 1 pinches (zoom out). + callback timeline-pinch(float); + /// Ctrl+wheel or pinch over the grid: resize the cells. A signed step, + /// not a size, so Rust owns the bounds. + callback zoom-cells(int); callback columns-changed(int); callback sync-now(); /// The grid scrolled: the first visible image's ordinal in the library. @@ -634,7 +641,10 @@ export component LibraryGrid inherits Rectangle { // Cell geometry. Columns are derived from the available width so the grid // reflows with the window rather than fixing a count (FR-UI-1). - property cell-size: 180px; + // Zoomable, so the grid serves both jobs: fewer, larger images for + // judging one, and more, smaller ones for finding one. Driven from Rust so + // the value survives a scope change and the thumbnail class can follow it. + in property cell-size: 180px; property columns: max(1, floor((self.width - Theme.gap) / (cell-size + Theme.gap))); // Reported out so Rust can place month headings: a heading belongs on a // cell that begins a row, and only the grid knows how wide a row is. @@ -999,6 +1009,51 @@ export component LibraryGrid inherits Rectangle { // background still flicks the grid. (This is the part a hand-rolled // TouchArea gesture could not do — see the drag comments above.) if root.total > 0: grid-scroll := Flickable { + // Ctrl+wheel resizes the cells; a plain wheel is declined and + // falls through to the Flickable's own scrolling. Two jobs on + // one gesture, distinguished by the modifier — the convention + // every image browser uses. + // + // Declared *first* so it sits beneath the cells in z-order: + // their own touch areas still take clicks and drags, and only + // a wheel event nothing else claimed reaches this. + zoom-catcher := TouchArea { + width: 100%; + height: parent.viewport-height; + scroll-event(e) => { + if (e.modifiers.control) { + root.zoom-cells(e.delta-y > 0 ? 1 : -1); + return accept; + } + return reject; + } + } + + // Two-finger pinch, for tablet: the same gesture the timeline + // uses, applied to cell size rather than to time. + grid-pinch := ScaleRotateGestureHandler { + width: 100%; + height: 100%; + + property last-scale: 1.0; + + started => { self.last-scale = 1.0; } + updated => { + // A quarter-step either way is enough to act on: cell + // size is continuous, unlike the timeline's discrete + // zoom levels. + if (self.scale / max(0.01, self.last-scale) > 1.15) { + root.zoom-cells(1); + self.last-scale = self.scale; + } else if (self.scale / max(0.01, self.last-scale) < 0.87) { + root.zoom-cells(-1); + self.last-scale = self.scale; + } + } + ended => { self.last-scale = 1.0; } + cancelled => { self.last-scale = 1.0; } + } + // Follow a requested position. Without this a scrub moves the // *loaded window* while the viewport stays where it was, so // the cells are drawn thousands of rows away and the grid