diff --git a/Cargo.lock b/Cargo.lock index fd3c095..e8c2db6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -39,6 +39,17 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "aae1277d39aeec15cb388266ecc24b11c80469deae6067e17a1a7aa9e5c1f234" +[[package]] +name = "aes" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b169f7a6d4742236a0a00c541b845991d0ac43e546831af1249753ab4c3aa3a0" +dependencies = [ + "cfg-if", + "cipher", + "cpufeatures", +] + [[package]] name = "ahash" version = "0.8.12" @@ -201,6 +212,17 @@ version = "1.0.104" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "330a5ed07fa54e4702c9d6c4174f74427fc0ef6e214bbd677ae50a5099946470" +[[package]] +name = "apple-native-keyring-store" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b350bfd03649e07aa05c0a81b3e15934374e585c98204a57e20b9d49f49bb9a" +dependencies = [ + "keyring-core", + "log", + "security-framework", +] + [[package]] name = "arbitrary" version = "1.4.2" @@ -554,6 +576,24 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0d8c1fef690941d3e7788d328517591fecc684c084084702d6ff1641e993699a" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + +[[package]] +name = "block-padding" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8894febbff9f758034a5b8e12d87918f56dfc64a8e1fe757d65e29041538d93" +dependencies = [ + "generic-array", +] + [[package]] name = "block2" version = "0.5.1" @@ -712,6 +752,15 @@ dependencies = [ "wayland-client", ] +[[package]] +name = "cbc" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26b52a9543ae338f279b96b0b9fed9c8093744685043739079ce85cd58f289a6" +dependencies = [ + "cipher", +] + [[package]] name = "cc" version = "1.4.2" @@ -773,6 +822,16 @@ dependencies = [ "windows-link", ] +[[package]] +name = "cipher" +version = "0.4.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "773f3b9af64447d2ce9850330c473515014aa235e6a783b02db81ff39e4a3dad" +dependencies = [ + "crypto-common", + "inout", +] + [[package]] name = "clang-sys" version = "1.9.1" @@ -952,6 +1011,15 @@ version = "3.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7704b5fdd17b18ae31c4c1da5a2e0305a2bf17b5249300a9ee9ed7b72114c636" +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + [[package]] name = "crc32fast" version = "1.5.0" @@ -1007,6 +1075,16 @@ version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "460fbee9c2c2f33933d720630a6a0bac33ba7053db5344fac858d4b8952d77d5" +[[package]] +name = "crypto-common" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78c8292055d1c1df0cce5d180393dc8cce0abec0a7102adb6c7b1eef6016d60a" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "ctor" version = "0.10.1" @@ -1109,6 +1187,17 @@ dependencies = [ "syn 3.0.3", ] +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", + "subtle", +] + [[package]] name = "dispatch" version = "0.2.0" @@ -1203,6 +1292,14 @@ dependencies = [ "wgpu", ] +[[package]] +name = "dr-lens" +version = "0.1.0" +dependencies = [ + "lensfun", + "log", +] + [[package]] name = "dr-pipeline" version = "0.1.0" @@ -1211,6 +1308,17 @@ dependencies = [ "log", ] +[[package]] +name = "dr-plat" +version = "0.1.0" +dependencies = [ + "dr-types", + "env_logger", + "keyring", + "log", + "thiserror 2.0.20", +] + [[package]] name = "dr-sync" version = "0.1.0" @@ -1228,6 +1336,7 @@ version = "0.1.0" dependencies = [ "async-trait", "dr-decode", + "dr-plat", "dr-sync", "dr-types", "env_logger", @@ -1257,6 +1366,8 @@ dependencies = [ "dr-decode", "dr-gpu", "dr-pipeline", + "dr-plat", + "dr-sync-nextcloud", "dr-types", "log", "pollster", @@ -1635,7 +1746,7 @@ dependencies = [ "objc2-foundation 0.3.2", "parlance", "read-fonts 0.39.2", - "roxmltree", + "roxmltree 0.21.1", "smallvec", "windows 0.62.2", "windows-core 0.62.2", @@ -1801,6 +1912,16 @@ dependencies = [ "libc", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "gethostname" version = "1.1.0" @@ -2131,6 +2252,24 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dfa686283ad6dd069f105e5ab091b04c62850d3e4cf5d67debad1933f55023df" +[[package]] +name = "hkdf" +version = "0.12.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b5f8eb2ad728638ea2c7d47a21db23b7b58a72ed6a38256b8a1849f15fbbdf7" +dependencies = [ + "hmac", +] + +[[package]] +name = "hmac" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" +dependencies = [ + "digest", +] + [[package]] name = "htmlparser" version = "0.2.1" @@ -2772,6 +2911,16 @@ dependencies = [ "hashbrown 0.17.1", ] +[[package]] +name = "inout" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "879f10e63c20629ecabbb64a8010319738c66a5cd0c29b02d63d272b03751d01" +dependencies = [ + "block-padding", + "generic-array", +] + [[package]] name = "input" version = "0.10.0" @@ -3162,6 +3311,27 @@ dependencies = [ "unicode-segmentation", ] +[[package]] +name = "keyring" +version = "4.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72585bb6cc9bc370d1d545b7e23fcce71dfd4461c5e15275e3cf51bdfd9a980a" +dependencies = [ + "apple-native-keyring-store", + "keyring-core", + "windows-native-keyring-store", + "zbus-secret-service-keyring-store", +] + +[[package]] +name = "keyring-core" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb1e621458ca9c51aa110bd0339d4751a056b9576bf1253aee1aa560dda0fc9d" +dependencies = [ + "log", +] + [[package]] name = "khronos-egl" version = "6.0.0" @@ -3203,6 +3373,18 @@ version = "0.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7a79a3332a6609480d7d0c9eab957bca6b455b91bb84e66d19f5ff66294b85b8" +[[package]] +name = "lensfun" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fe52f943cdba39214e90c8ff14c10cebc0f597dcbbbcdab15a68902fc8392bd8" +dependencies = [ + "flate2", + "regex", + "roxmltree 0.20.0", + "thiserror 2.0.20", +] + [[package]] name = "libc" version = "0.2.189" @@ -4944,6 +5126,12 @@ dependencies = [ "text-size", ] +[[package]] +name = "roxmltree" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c20b6793b5c2fa6553b250154b78d6d0db37e72700ae35fad9387a46f487c97" + [[package]] name = "roxmltree" version = "0.21.1" @@ -5190,6 +5378,25 @@ dependencies = [ "tiny-skia 0.11.4", ] +[[package]] +name = "secret-service" +version = "5.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a62d7f86047af0077255a29494136b9aaaf697c76ff70b8e49cded4e2623c14" +dependencies = [ + "aes", + "cbc", + "futures-util", + "generic-array", + "getrandom 0.2.17", + "hkdf", + "num", + "once_cell", + "serde", + "sha2", + "zbus", +] + [[package]] name = "security-framework" version = "3.7.0" @@ -5291,6 +5498,17 @@ dependencies = [ "serde_core", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "shlex" version = "1.3.0" @@ -6225,6 +6443,12 @@ dependencies = [ "serde", ] +[[package]] +name = "typenum" +version = "1.20.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" + [[package]] name = "udev" version = "0.9.3" @@ -6364,7 +6588,7 @@ dependencies = [ "kurbo", "log", "pico-args", - "roxmltree", + "roxmltree 0.21.1", "rustybuzz", "simplecss", "siphasher", @@ -6969,6 +7193,19 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-native-keyring-store" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "063426e76fdec7438d56bb777f67e318a84a25c707b07e575cb8b78e10c028f8" +dependencies = [ + "byteorder", + "keyring-core", + "regex", + "windows-sys 0.61.2", + "zeroize", +] + [[package]] name = "windows-numerics" version = "0.3.1" @@ -7458,6 +7695,17 @@ dependencies = [ "zvariant", ] +[[package]] +name = "zbus-secret-service-keyring-store" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ccede190ba363386a24e8021c7f3848393976609ec9f5d1f8c6c09ef37075b4" +dependencies = [ + "keyring-core", + "secret-service", + "zbus", +] + [[package]] name = "zbus_macros" version = "5.18.0" diff --git a/Cargo.toml b/Cargo.toml index 716a936..dce9a32 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,9 +5,11 @@ members = [ "core/dr-catalog", "core/dr-decode", "core/dr-gpu", + "core/dr-lens", "core/dr-pipeline", "core/dr-sync", "core/dr-sync-nextcloud", + "platform/dr-plat", "ui/dr-ui", "apps/darkroom-desktop", "tools/traceability", @@ -26,7 +28,9 @@ dr-types = { path = "core/dr-types" } dr-catalog = { path = "core/dr-catalog" } dr-decode = { path = "core/dr-decode" } dr-gpu = { path = "core/dr-gpu" } +dr-lens = { path = "core/dr-lens" } dr-pipeline = { path = "core/dr-pipeline" } +dr-plat = { path = "platform/dr-plat" } dr-sync = { path = "core/dr-sync" } dr-sync-nextcloud = { path = "core/dr-sync-nextcloud" } dr-ui = { path = "ui/dr-ui" } @@ -63,6 +67,13 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" base64 = "0.23" +# Platform secure storage: Secret Service on Linux, Keystore on Android +# (FR-NC-2). Credentials never touch the catalog or a plain file. +# keyring 4 restructured its features: `v1` is the default set and brings +# the zbus Secret Service backend, which is what GNOME Keyring and KWallet +# (via ksecretd) both speak. +keyring = { version = "4", features = ["v1"] } + # 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 @@ -78,6 +89,27 @@ rawler = "0.7" zune-jpeg = "0.4.21" bytemuck = { version = "1", features = ["derive"] } +# Lens correction profiles. A pure-Rust port of Lensfun rather than a binding +# to the C library, for the same cross-compilation reason as the TLS and +# SQLite choices above: liblensfun would be a third C dependency to satisfy +# under the Android NDK. +# +# The database ships *inside* the crate — 56 XML files, gzipped at build time +# and decompressed on first lookup. That matters beyond convenience: Android +# gives us no filesystem path (ARCH §6.9), so a database loaded from a +# system directory would have nowhere to live there. +# +# Licence: LGPL-3.0-or-later, which upgrades cleanly into our GPLv3 (D8). +# The upstream Lensfun *database* is CC-BY-SA and is redistributed by the +# crate; attribution belongs in the about screen. +# +# Caveat worth remembering: this is a third-party port at 0.7.0, not upstream +# Lensfun. Verified working against the bundled database (interpolation +# between calibration points, and an unknown lens returning empty rather than +# panicking), but the pipeline talks to it through its own profile types so +# swapping it out is not a pipeline change. +lensfun = "0.7" + [profile.dev] # Dependencies optimised even in dev builds — wgpu and image decoding are # unusably slow otherwise, and they rarely need debugging. diff --git a/core/dr-gpu/examples/develop.rs b/core/dr-gpu/examples/develop.rs index 50abc8f..5285787 100644 --- a/core/dr-gpu/examples/develop.rs +++ b/core/dr-gpu/examples/develop.rs @@ -12,8 +12,8 @@ //! it. This is a diagnostic, not the export path (FR-EXP-*). use dr_gpu::{AdjustPass, Demosaicer, GpuContext}; -use dr_pipeline::ops::{colour, exposure, tone, white_balance}; -use dr_pipeline::EditGraph; +use dr_pipeline::ops::{colour, colour_mixer, contrast, exposure, tone, white_balance}; +use dr_pipeline::{EditGraph, ParamId}; fn main() { env_logger::init(); @@ -65,6 +65,27 @@ fn main() { graph.set_param(colour::BRILLIANCE_ID, colour::BRILLIANCE, 40.0); graph.set_param(white_balance::ID, white_balance::TEMPERATURE, 15.0); } + // Contrast alone, so its effect can be judged without anything else + // moving. + "contrast" => { + graph.set_param(contrast::ID, contrast::CONTRAST, 60.0); + } + "flat" => { + graph.set_param(contrast::ID, contrast::CONTRAST, -60.0); + } + // The mixer, pushed hard on the two things this scene actually has: + // green vegetation and grey-blue rock. + "mixer" => { + graph.set_param(colour_mixer::ID, ParamId("green_sat"), 80.0); + graph.set_param(colour_mixer::ID, ParamId("green_hue"), -40.0); + graph.set_param(colour_mixer::ID, ParamId("chartreuse_sat"), 60.0); + graph.set_param(colour_mixer::ID, ParamId("azure_lum"), -50.0); + } + // One band only, to check the weighting really is selective rather + // than affecting the whole image. + "mixer_one" => { + graph.set_param(colour_mixer::ID, ParamId("green_sat"), 100.0); + } _ => {} } diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index f91be06..f28777c 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -21,10 +21,15 @@ use wgpu::util::DeviceExt; use crate::{DemosaicedImage, GpuContext, GpuError}; -/// Number of leading floats in the generated uniform block that the composer -/// reserves for base parameters — three padded matrix rows and the as-shot -/// white balance. Must match `BASE_UNIFORM_FIELDS` in dr-pipeline. -const BASE_FIELDS: usize = 16; +/// Leading floats the composer reserves before any operation's own uniforms: +/// three padded matrix rows, the as-shot white balance, and framing's block. +/// +/// Imported rather than restated. It was a local literal, which was a latent +/// bug of exactly the kind that is invisible until it is severe: growing the +/// reserved block on the pipeline side would leave this short, and every +/// operation's uniforms would silently shift out from under the shader that +/// reads them. +const RESERVED_FIELDS: usize = dr_pipeline::RESERVED_UNIFORM_FIELDS; /// Runs composed operation chains against demosaiced images. pub struct AdjustPass { @@ -206,10 +211,11 @@ impl AdjustPass { // Base uniforms: the camera matrix and as-shot white balance, which // every generated shader reads regardless of which operations are - // active. + // active. Framing's slots follow them and are filled by the composer, + // which is why only the first sixteen are written here. let mut uniforms = shader.uniforms.clone(); - if uniforms.len() < BASE_FIELDS { - uniforms.resize(BASE_FIELDS, 0.0); + if uniforms.len() < RESERVED_FIELDS { + uniforms.resize(RESERVED_FIELDS, 0.0); } let m = source.color_matrix(); let wb = source.as_shot_wb(); @@ -373,7 +379,7 @@ fn numbered(src: &str) -> String { mod tests { use super::*; use dr_decode::{CfaPattern, CropRect, RawImage}; - use dr_pipeline::ops::{colour, exposure, tone, white_balance}; + use dr_pipeline::ops::{colour, exposure}; use dr_pipeline::EditGraph; use crate::Demosaicer; @@ -420,6 +426,14 @@ mod tests { } fn read_centre(ctx: &GpuContext, tex: &wgpu::Texture) -> [u8; 4] { + let (w, h) = (tex.width(), tex.height()); + read_pixel(ctx, tex, w / 2, h / 2) + } + + /// One pixel, by coordinate. What the geometry tests need: proving a + /// rotation moved content requires looking somewhere other than the + /// centre, which every rotation leaves fixed. + fn read_pixel(ctx: &GpuContext, tex: &wgpu::Texture, x: u32, y: u32) -> [u8; 4] { let w = tex.width(); let h = tex.height(); let unpadded = w * 4; @@ -466,7 +480,7 @@ mod tests { rx.recv().expect("map").expect("map ok"); let data = slice.get_mapped_range(); - let off = ((h / 2) * padded + (w / 2) * 4) as usize; + let off = (y.min(h - 1) * padded + x.min(w - 1) * 4) as usize; let px = [data[off], data[off + 1], data[off + 2], data[off + 3]]; drop(data); buf.unmap(); @@ -521,6 +535,195 @@ mod tests { } } + /// An image bright on one side and dark on the other, so a transform that + /// moves content is visible. A flat grey cannot show a rotation at all. + /// + /// `vertical` puts the bright band at the top; otherwise at the left. + fn split_image(ctx: &GpuContext, vertical: bool) -> DemosaicedImage { + let size = 32u32; + let mut data = vec![0u16; (size * size) as usize]; + for y in 0..size { + for x in 0..size { + let near_start = if vertical { y } else { x } < size / 2; + data[(y * size + x) as usize] = if near_start { 12000 } else { 500 }; + } + } + let raw = RawImage { + width: size, + height: size, + data, + cfa_pattern: CfaPattern::Rggb, + black_level: [0; 4], + white_level: 16383, + wb_coeffs: [1.0, 1.0, 1.0, 1.0], + color_matrix: Some([1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0]), + crop: CropRect { + x: 0, + y: 0, + width: size, + height: size, + }, + }; + Demosaicer::new(ctx) + .expect("demosaicer") + .run(&raw) + .expect("demosaic") + } + + #[test] + fn a_quarter_turn_moves_a_vertical_edge_to_a_horizontal_one() { + // The end-to-end check that the coordinate permutation is wired the + // right way round. A left-bright image turned 90° clockwise must come + // out top-bright; getting the sign wrong yields bottom-bright, which + // compiles perfectly and is simply the wrong image. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = split_image(&ctx, false); + + let mut g = EditGraph::default_chain(); + g.rotate_quarters(1); + let (w, h) = g.output_size(32, 32); + let shader = g.compose(); + let tex = pass.render(&img, &shader, w, h).expect("render"); + + let top = read_pixel(&ctx, tex, w / 2, h / 8)[0]; + let bottom = read_pixel(&ctx, tex, w / 2, h * 7 / 8)[0]; + assert!( + top > bottom + 40, + "a left-bright image turned 90° clockwise should be top-bright, \ + got top={top} bottom={bottom}" + ); + } + + #[test] + fn a_horizontal_flip_swaps_the_sides() { + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = split_image(&ctx, false); + + let mut g = EditGraph::default_chain(); + g.set_param(dr_pipeline::framing::ID, dr_pipeline::framing::FLIP_H, 1.0); + let shader = g.compose(); + let tex = pass.render(&img, &shader, 32, 32).expect("render"); + + let left = read_pixel(&ctx, tex, 4, 16)[0]; + let right = read_pixel(&ctx, tex, 28, 16)[0]; + assert!( + right > left + 40, + "flipping a left-bright image should make it right-bright, \ + got left={left} right={right}" + ); + } + + #[test] + fn cropping_to_one_half_shows_only_that_half() { + // The property a crop exists for, checked against content rather than + // against the output dimensions alone: a crop of the dark side must + // be dark everywhere, edge to edge. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = split_image(&ctx, false); + + let mut g = EditGraph::default_chain(); + g.set_crop(dr_pipeline::CropRect { + x: 0.5, + y: 0.0, + width: 0.5, + height: 1.0, + }); + let (w, h) = g.output_size(32, 32); + assert_eq!((w, h), (16, 32), "half a 32px frame is 16px wide"); + + let shader = g.compose(); + let tex = pass.render(&img, &shader, w, h).expect("render"); + assert_eq!((tex.width(), tex.height()), (16, 32)); + + for x in [1, w / 2, w - 2] { + let v = read_pixel(&ctx, tex, x, h / 2)[0]; + assert!(v < 90, "cropped to the dark half, x={x} came out {v}"); + } + } + + #[test] + fn straightening_darkens_the_exposed_corners() { + // Rotating a frame inside its own bounds leaves no source pixel at the + // corners. They must read black rather than a smeared edge pixel — the + // difference between "the frame is rotated" and "the image is smudged". + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = split_image(&ctx, false); + + let mut g = EditGraph::default_chain(); + g.set_param(dr_pipeline::framing::ID, dr_pipeline::framing::ANGLE, 30.0); + let shader = g.compose(); + let tex = pass.render(&img, &shader, 32, 32).expect("render"); + + // The top-left corner of a 30° rotation is off the source. + let corner = read_pixel(&ctx, tex, 0, 0); + assert_eq!( + corner, + [0, 0, 0, 255], + "an exposed corner must be black and opaque" + ); + } + + #[test] + fn dragging_the_crop_does_not_recompile() { + // The cache contract for framing, which is what makes an interactive + // crop drag viable: the rect changes every frame, and each frame must + // reuse the compiled pipeline. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = grey_image(&ctx, 4000); + + let mut g = EditGraph::default_chain(); + for i in 1..=10 { + let inset = i as f32 * 0.02; + g.set_crop(dr_pipeline::CropRect { + x: inset, + y: inset, + width: 1.0 - 2.0 * inset, + height: 1.0 - 2.0 * inset, + }); + let (w, h) = g.output_size(64, 64); + pass.render(&img, &g.compose(), w, h).expect("render"); + } + + assert_eq!( + pass.cached_pipelines(), + 1, + "ten crop rectangles must share one compiled pipeline" + ); + } + + #[test] + fn straightening_compiles_its_own_pipeline_but_reuses_it() { + // Straightening changes the sampling path from an integer load to a + // bilinear fetch, so it *must* compile a second pipeline — and then + // must stop at two however far the slider travels. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = grey_image(&ctx, 4000); + + let mut g = EditGraph::default_chain(); + pass.render(&img, &g.compose(), 32, 32).expect("render"); + assert_eq!(pass.cached_pipelines(), 1); + + for i in 1..=8 { + g.set_param( + dr_pipeline::framing::ID, + dr_pipeline::framing::ANGLE, + i as f32 * 0.5, + ); + pass.render(&img, &g.compose(), 32, 32).expect("render"); + } + assert_eq!( + pass.cached_pipelines(), + 2, + "straightening compiles one more pipeline, not one per angle" + ); + } + #[test] fn the_whole_chain_at_once_compiles() { // Individually-valid fragments can still collide when combined — @@ -543,10 +746,21 @@ mod tests { let shader = g.compose(); assert_eq!( shader.source.matches("---- ").count(), - g.descriptors().len(), - "every operation should be active" + // Every operation, plus framing — which emits a stage of its own + // rather than an operation block, and is not in `descriptors`. + g.descriptors().len() + 1, + "every operation and the framing should be active" ); - pass.render(&img, &shader, 32, 32) + assert!( + shader.source.contains("---- framing ----"), + "framing must reach the shader alongside the colour operations" + ); + + // Cropped, so the render is against an output size that is not the + // source size — the case where a wrong dispatch or a wrong texture + // allocation would show up. + let (w, h) = g.output_size(32, 32); + pass.render(&img, &shader, w, h) .expect("the full chain must compile"); } diff --git a/core/dr-lens/Cargo.toml b/core/dr-lens/Cargo.toml new file mode 100644 index 0000000..3ec0211 --- /dev/null +++ b/core/dr-lens/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "dr-lens" +version.workspace = true +edition.workspace = true +rust-version.workspace = true +license.workspace = true + +# Isolated from dr-pipeline deliberately. That crate has no dependencies at +# all so its codegen stays testable without a device (ARCH §6.5a); pulling an +# XML parser and 5.5 MB of profile data into it would cost exactly the +# property it is organised around. The pipeline consumes the coefficient +# structs this crate produces, and never links the database. +[dependencies] +lensfun.workspace = true +log.workspace = true diff --git a/core/dr-lens/src/lib.rs b/core/dr-lens/src/lib.rs new file mode 100644 index 0000000..e154f54 --- /dev/null +++ b/core/dr-lens/src/lib.rs @@ -0,0 +1,300 @@ +//! Lens profile lookup — the Lensfun database, reduced to coefficients. +//! +//! # What this crate is for +//! +//! `dr-pipeline` knows the *maths* of lens correction: the `ptlens` +//! polynomial, the `poly3` per-channel scale, the `pa` vignetting curve. It +//! does not know which coefficients belong to which lens, and deliberately +//! has no dependencies with which to find out. +//! +//! This crate closes that gap. Given what EXIF reports — a lens name, a focal +//! length, an aperture — it returns the coefficients for that shot, and +//! nothing else. The pipeline consumes plain `f32`s and never links the +//! database. +//! +//! # Why a separate crate rather than a module +//! +//! Two reasons, both about keeping a dependency contained: +//! +//! - **`dr-pipeline` has no dependencies on purpose.** Its codegen is testable +//! without a GPU, and adding an XML parser plus 5.5 MB of profile data to it +//! would cost exactly the property it is organised around (ARCH §6.5a). +//! - **The `lensfun` crate is a third-party port**, not upstream Lensfun. +//! Confining it behind [`LensProfile`] means replacing it — with the C +//! library, with our own parser, with vendor-supplied profiles — touches +//! this crate and nothing downstream. +//! +//! # Matching is best-effort, and says so +//! +//! EXIF lens names are not clean identifiers. Different bodies report the same +//! lens differently, third-party lenses often report nothing, and adapted +//! manual lenses report nothing at all. So every lookup returns an `Option`, +//! and the UI is expected to say plainly whether a profile was found — an +//! automatic correction that silently did nothing is worse than one the user +//! can see is unavailable (ARCH §9.4 applies the same honesty rule to sync). + +use std::sync::OnceLock; + +/// The `ptlens` distortion coefficients, as Lensfun stores them. +/// +/// Mirrors `dr_pipeline::ops::distortion::PtLens`. Duplicated rather than +/// shared because the dependency would have to run the wrong way: the +/// pipeline must not depend on the profile database. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Distortion { + pub a: f32, + pub b: f32, + pub c: f32, +} + +/// Lateral chromatic aberration: a per-channel radial scale. +/// +/// Red and blue are scaled about the optical axis; green is the reference and +/// is never moved, so a correction that is wrong still leaves the image +/// sharp in one channel rather than softening all three. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Tca { + pub red_scale: f32, + pub blue_scale: f32, +} + +/// The `pa` vignetting polynomial: `1 + k1·r² + k2·r⁴ + k3·r⁶`. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Vignetting { + pub k1: f32, + pub k2: f32, + pub k3: f32, +} + +/// Everything known about one lens at one set of shooting parameters. +/// +/// Each field is independently optional: the Lensfun database frequently +/// carries distortion for a lens but no vignetting, or covers only part of a +/// zoom's range. A partial profile is useful and must not be discarded. +#[derive(Debug, Clone, Copy, PartialEq, Default)] +pub struct LensProfile { + pub distortion: Option, + pub tca: Option, + pub vignetting: Option, +} + +impl LensProfile { + /// Whether this profile carries anything at all. + pub fn is_empty(&self) -> bool { + self.distortion.is_none() && self.tca.is_none() && self.vignetting.is_none() + } +} + +/// What EXIF reports about a shot, as far as lens correction cares. +#[derive(Debug, Clone, PartialEq)] +pub struct ShotInfo<'a> { + /// The lens model string. Frequently absent or unhelpful. + pub lens: &'a str, + /// Focal length in mm. Selects between a zoom's calibration points. + pub focal_length: f32, + /// Aperture as an f-number. Vignetting depends on it strongly — a lens + /// wide open can be two stops down in the corners and clean by f/8. + pub aperture: f32, + /// Focus distance in metres, when known. Vignetting varies with it, but + /// EXIF rarely reports it, so the default stands in for "far away". + pub distance: f32, +} + +impl<'a> ShotInfo<'a> { + /// The distance Lensfun's calibrations use for "not a close-up". + /// + /// Most database entries are measured at 1000 m — effectively infinity — + /// and EXIF almost never carries focus distance, so this is the value + /// nearly every lookup uses. + pub const FAR: f32 = 1000.0; + + pub fn new(lens: &'a str, focal_length: f32, aperture: f32) -> Self { + Self { + lens, + focal_length, + aperture, + distance: Self::FAR, + } + } +} + +/// The bundled Lensfun database, loaded once on first use. +/// +/// Decompressing 56 XML files costs enough to be worth doing once, and little +/// enough not to be worth doing in the background. `OnceLock` rather than a +/// constructor the callers must thread through: this is a read-only reference +/// table, and making every call site own a handle to it would buy nothing. +fn database() -> Option<&'static lensfun::Database> { + static DB: OnceLock> = OnceLock::new(); + DB.get_or_init(|| match lensfun::Database::load_bundled() { + Ok(db) => Some(db), + Err(e) => { + // Not fatal. Manual correction still works, so the develop panel + // stays usable with the automatic profile absent. + log::warn!("lens profile database unavailable: {e}"); + None + } + }) + .as_ref() +} + +/// Look up correction coefficients for a shot. +/// +/// Returns `None` when the lens is unknown — which is common and not an +/// error. A blank lens name short-circuits, because matching on one returns +/// arbitrary entries rather than no entries. +pub fn lookup(shot: &ShotInfo<'_>) -> Option { + if shot.lens.trim().is_empty() { + return None; + } + + let db = database()?; + let matches = db.find_lenses(None, shot.lens); + // The database returns candidates ranked by match quality; anything + // beyond the best is a different lens that merely reads similarly. + let lens = matches.first()?; + + let profile = LensProfile { + distortion: lens + .interpolate_distortion(shot.focal_length) + .and_then(|d| match d.model { + lensfun::DistortionModel::Ptlens { a, b, c } => Some(Distortion { a, b, c }), + // Other models exist in the database (`poly3`, `fov1`). Rather + // than approximate one with another — which would correct by + // the wrong curve and look like a bad profile — report nothing + // and let the manual control take over. + _ => None, + }), + tca: lens + .interpolate_tca(shot.focal_length) + .and_then(|t| match t.model { + lensfun::TcaModel::Poly3 { red, blue } => Some(Tca { + // Index 0 is the linear radial scale `v`, which carries + // essentially all of the correction; the higher terms are + // zero throughout the database in practice. + red_scale: red[0], + blue_scale: blue[0], + }), + _ => None, + }), + vignetting: lens + .interpolate_vignetting(shot.focal_length, shot.aperture, shot.distance) + .and_then(|v| match v.model { + lensfun::VignettingModel::Pa { k1, k2, k3 } => Some(Vignetting { k1, k2, k3 }), + _ => None, + }), + }; + + // A match that yielded no usable coefficients is the same as no match, and + // reporting it as a hit would tell the user a correction is active when + // nothing is being corrected. + (!profile.is_empty()).then_some(profile) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A lens with dense calibration across a zoom range, so interpolation is + /// genuinely exercised. + const KNOWN: &str = "Canon EF 16-35mm f/2.8L USM"; + + #[test] + fn the_bundled_database_loads() { + // The property that makes this crate work on Android: no system + // library, no data directory, no filesystem path (ARCH §6.9). + assert!(database().is_some(), "the bundled database must load"); + } + + #[test] + fn a_known_lens_resolves() { + let profile = lookup(&ShotInfo::new(KNOWN, 20.0, 2.8)).expect("a stocked lens"); + assert!(profile.distortion.is_some(), "distortion expected"); + assert!(!profile.is_empty()); + } + + #[test] + fn an_unknown_lens_is_none_rather_than_a_panic() { + // Adapted and third-party lenses report names absent from the + // database. That is the normal case, not an error. + assert!(lookup(&ShotInfo::new("Nonexistent 999mm f/0.5", 50.0, 2.0)).is_none()); + } + + #[test] + fn a_blank_lens_name_does_not_match_arbitrarily() { + // EXIF often carries no lens at all. Matching on an empty string + // returns unrelated entries, which would silently apply another + // lens's correction — worse than applying none. + for name in ["", " "] { + assert!( + lookup(&ShotInfo::new(name, 50.0, 2.0)).is_none(), + "{name:?}" + ); + } + } + + #[test] + fn coefficients_interpolate_between_calibration_points() { + // 20mm and 22mm are calibrated; 21mm is not. If interpolation were + // absent, a zoom would snap between corrections mid-range. + let at20 = lookup(&ShotInfo::new(KNOWN, 20.0, 2.8)).and_then(|p| p.distortion); + let at21 = lookup(&ShotInfo::new(KNOWN, 21.0, 2.8)).and_then(|p| p.distortion); + let at22 = lookup(&ShotInfo::new(KNOWN, 22.0, 2.8)).and_then(|p| p.distortion); + + let (a, b, c) = (at20.unwrap(), at21.unwrap(), at22.unwrap()); + assert_ne!(a, b, "21mm must not reuse the 20mm coefficients verbatim"); + let between = (a.a.min(c.a)..=a.a.max(c.a)).contains(&b.a); + assert!( + between, + "21mm coefficient {} is outside [{}, {}]", + b.a, a.a, c.a + ); + } + + #[test] + fn vignetting_responds_to_aperture() { + // The reason aperture is in `ShotInfo` at all: a lens wide open + // vignettes heavily and is clean stopped down, so a correction that + // ignored aperture would be wrong at both ends. + let wide = lookup(&ShotInfo::new(KNOWN, 20.0, 2.8)).and_then(|p| p.vignetting); + let stopped = lookup(&ShotInfo::new(KNOWN, 20.0, 8.0)).and_then(|p| p.vignetting); + + if let (Some(w), Some(s)) = (wide, stopped) { + assert_ne!(w, s, "aperture must change the vignetting curve"); + } + } + + #[test] + fn tca_leaves_green_as_the_reference() { + // Green is never scaled, so red and blue are corrected towards it. + // Both scales sit within a fraction of a percent of 1.0; anything + // far from that would be a unit error rather than a real correction. + if let Some(tca) = lookup(&ShotInfo::new(KNOWN, 20.0, 2.8)).and_then(|p| p.tca) { + for s in [tca.red_scale, tca.blue_scale] { + assert!( + (0.99..=1.01).contains(&s), + "{s} is not a plausible per-channel scale" + ); + } + } + } + + #[test] + fn a_far_focus_distance_is_the_default() { + // EXIF rarely carries focus distance, so nearly every real lookup + // relies on this standing in. + assert_eq!(ShotInfo::new(KNOWN, 20.0, 2.8).distance, ShotInfo::FAR); + } + + #[test] + fn repeated_lookups_reuse_one_database() { + // The database is decompressed on first use; a second lookup must not + // pay for it again. + assert!(lookup(&ShotInfo::new(KNOWN, 20.0, 2.8)).is_some()); + assert!(lookup(&ShotInfo::new(KNOWN, 24.0, 4.0)).is_some()); + assert!(std::ptr::eq( + database().expect("loaded"), + database().expect("loaded") + )); + } +} diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 7d5d99e..576d1d0 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -8,7 +8,7 @@ //! been answered. And a crop changes the output's dimensions and aspect //! ratio, which no colour fragment can express. //! -//! [`crate::warp::Warp`] is closer: it also rewrites coordinates before the +//! [`crate::lens::Warp`] is closer: it also rewrites coordinates before the //! fetch. But a warp is a *correction to the optics* — distortion and CA are //! properties of the lens, defined about the optical axis, over the whole //! frame the lens projected. Framing is a decision about *composition*, made @@ -144,13 +144,16 @@ impl CropRect { pub fn normalised(self) -> Self { let x = finite(self.x, 0.0).clamp(0.0, 1.0 - Self::MIN_EXTENT); let y = finite(self.y, 0.0).clamp(0.0, 1.0 - Self::MIN_EXTENT); - let width = finite(self.width, 1.0).clamp(Self::MIN_EXTENT, 1.0 - x); - let height = finite(self.height, 1.0).clamp(Self::MIN_EXTENT, 1.0 - y); Self { x, y, - width, - height, + // `max` before `min`, not `f32::clamp`. With the origin at its + // limit, `1.0 - x` rounds to fractionally *below* `MIN_EXTENT` — + // an inverted range, which `clamp` panics on rather than + // resolving. Ordering it this way lets the lower bound win, which + // is also the answer that keeps the rect non-degenerate. + width: finite(self.width, 1.0).min(1.0 - x).max(Self::MIN_EXTENT), + height: finite(self.height, 1.0).min(1.0 - y).max(Self::MIN_EXTENT), } } } @@ -168,6 +171,7 @@ fn finite(v: f32, fallback: f32) -> f32 { } } +/// TRACES: FR-DEV-3 | FR-DEV-3d /// Crop, straighten, rotation and flips for one image. /// /// Holds no GPU state: like the rest of the graph this is CPU-side, so a lost @@ -424,12 +428,25 @@ impl Framing { /// position, ready for the warp chain. /// /// Leaves the result in `p`: centre `(0, 0)`, `r == 1` at the corner — - /// exactly the space [`crate::warp`] documents, so lens correction + /// exactly the space [`crate::lens`] documents, so lens correction /// composes on top of this without either stage naming the other. /// /// `aspect` is left in scope alongside it, since the warp chain and the /// sampler both need it to return to texture coordinates. pub fn wgsl_prologue(&self) -> String { + // Neutral framing still has to produce `p`, since the warp chain and + // the sampler read it either way. It emits no `---- ` marker: those + // count active stages, and a neutral graph must generate none. + if !self.is_active() { + return " // Source position, normalised and centred: the whole frame, unrotated. + let src_dims = textureDimensions(source); + let aspect = vec2(f32(src_dims.x) / f32(src_dims.y), 1.0); + let uv = (vec2(gid.xy) + vec2(0.5)) / vec2(dims); + var p = (uv - vec2(0.5)) * aspect; +" + .into(); + } + let mut s = String::new(); s.push_str( @@ -444,19 +461,6 @@ impl Framing { ", ); - if !self.is_active() { - // Neutral framing still has to produce `p`, since the warp chain - // and the sampler read it either way. It is only the crop, - // rotation and flip steps that vanish. - s.push_str( - " - // Framing is neutral: the whole frame, unrotated. - var p = (uv - vec2(0.5)) * aspect; -", - ); - return s; - } - s.push_str( " // Into the crop rect. @@ -559,6 +563,18 @@ mod tests { assert!(!src.contains("framing_angle")); } + #[test] + fn neutral_framing_emits_no_stage_marker() { + // `---- ` markers count *active* stages, and a neutral graph must + // generate none — the assertion behind "opening an image shows the + // image" is written against that count. + assert!(!Framing::new().wgsl_prologue().contains("---- ")); + + let mut f = Framing::new(); + f.set_param(ANGLE, 2.0); + assert!(f.wgsl_prologue().contains("---- framing ----")); + } + #[test] fn an_active_framing_reads_the_crop_rect() { let mut f = Framing::new(); @@ -664,6 +680,43 @@ mod tests { assert!(f.crop().width.is_finite() && f.crop().x.is_finite()); } + #[test] + fn an_origin_at_its_limit_does_not_panic() { + // Found by the codegen test that drives every parameter to its + // maximum. With the origin at `1 - MIN_EXTENT`, `1.0 - x` rounds to + // just under `MIN_EXTENT`, and `f32::clamp` panics on an inverted + // range rather than resolving it — a crash reachable by dragging a + // crop handle to the edge. + for origin in [1.0 - CropRect::MIN_EXTENT, 0.99, 0.999_999, 1.0, f32::MAX] { + let c = CropRect { + x: origin, + y: origin, + width: 1.0, + height: 1.0, + } + .normalised(); + assert!( + c.width >= CropRect::MIN_EXTENT && c.height >= CropRect::MIN_EXTENT, + "origin {origin} produced a degenerate rect: {c:?}" + ); + } + } + + #[test] + fn every_parameter_at_its_extremes_is_survivable() { + // The whole descriptor driven to both ends, which is what a codegen + // test does and what a corrupt sidecar can do. + for p in DESCRIPTOR.params { + for value in [-1e9, -1.0, 0.0, 1.0, 1e9, f32::NAN] { + let mut f = Framing::new(); + f.set_param(p.id, p.clamp(value)); + let (w, h) = f.output_size(6000, 4000); + assert!(w >= 1 && h >= 1, "{} at {value} gave {w}x{h}", p.id); + assert!(f.uniforms().iter().all(|v| v.is_finite())); + } + } + } + #[test] fn output_size_rounds_rather_than_truncating() { // Truncation biases every crop smaller; half of 101 should be 51. diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index df16f1e..1953934 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -398,7 +398,14 @@ mod tests { // something the UI would have to hardcode. let g = EditGraph::default_chain(); let caps = g.capabilities(); - assert_eq!(caps.len(), g.descriptors().len()); + // Every operation, plus framing — which is not an operation and so + // is absent from `descriptors`, but must still reach the panel. + assert_eq!(caps.len(), g.descriptors().len() + 1); + assert!( + caps.iter().any(|c| c.id == crate::framing::ID), + "framing must appear in the capability list, or the UI cannot \ + build a crop control without naming it" + ); for cap in &caps { assert!(!cap.params.is_empty(), "{} exposes no parameters", cap.id); @@ -458,13 +465,25 @@ mod tests { fn capabilities_survive_a_round_trip_through_set_param() { // The UI reads a capability, writes the value back, and must get the // same thing out — no hidden scaling between the two. + // + // Written at the parameter's declared precision, because that is what + // the UI can actually produce: a control declaring 0 decimals emits + // whole numbers, and a stage free to quantise to them is behaving + // correctly rather than losing the value. let mut g = EditGraph::default_chain(); for cap in g.capabilities() { for p in &cap.params { - if let ParamKind::Scalar { max, .. } = p.kind { - let target = max * 0.5; + if let ParamKind::Scalar { max, precision, .. } = p.kind { + let step = 10f32.powi(i32::from(precision)); + let target = (max * 0.5 * step).round() / step; g.set_param(cap.id, p.id, target); - assert_eq!(g.param(cap.id, p.id), Some(target)); + assert_eq!( + g.param(cap.id, p.id), + Some(target), + "{}.{} did not round-trip", + cap.id, + p.id + ); } } } diff --git a/core/dr-pipeline/src/warp.rs b/core/dr-pipeline/src/lens.rs similarity index 100% rename from core/dr-pipeline/src/warp.rs rename to core/dr-pipeline/src/lens.rs diff --git a/core/dr-pipeline/src/lib.rs b/core/dr-pipeline/src/lib.rs index 313af98..1f994b7 100644 --- a/core/dr-pipeline/src/lib.rs +++ b/core/dr-pipeline/src/lib.rs @@ -34,6 +34,7 @@ pub mod descriptor; pub mod framing; pub mod graph; +pub mod lens; pub mod operation; pub mod ops; @@ -42,8 +43,10 @@ pub use descriptor::{ }; pub use framing::{CropRect, Framing}; pub use graph::{EditGraph, OpCapability, ParamCapability}; +pub use lens::{compose_warps, ComposedWarp, Warp}; pub use operation::{ compose, compose_with_framing, Affects, ComposedShader, Helper, Operation, Uniform, + RESERVED_UNIFORM_FIELDS, }; #[cfg(test)] diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index a58f0ab..557f299 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -128,6 +128,14 @@ pub struct ComposedShader { /// are needed by every generated shader in any case. const BASE_UNIFORM_FIELDS: usize = 16; +/// Where an operation's own uniforms begin in the generated block. +/// +/// The base fields, then framing's. Exported because `dr-gpu` writes the +/// camera matrix into the leading slots by index and would otherwise carry +/// its own copy of this arithmetic — a duplicate that silently corrupts every +/// operation's uniforms the moment either block changes size. +pub const RESERVED_UNIFORM_FIELDS: usize = BASE_UNIFORM_FIELDS + FRAMING_UNIFORM_FIELDS; + /// Compose enabled operations into a single compute shader. /// /// Inactive operations are skipped entirely — they contribute no code, no @@ -417,7 +425,7 @@ fn hash_structure(active: &[&dyn Operation]) -> u64 { /// Whole-word matching matters: an operation with uniforms `amount` and /// `amount_hi` must not have the first rewrite corrupt the second. /// -/// Shared with [`crate::warp`], which prefixes its uniforms by the same rule +/// Shared with [`crate::lens`], which prefixes its uniforms by the same rule /// and must not diverge from it. /// Comments are skipped. A fragment explaining what `factor` does should not /// have its prose rewritten to `u.saturation_factor` — the generated source @@ -545,11 +553,17 @@ mod tests { ); assert_eq!( shader.uniforms.len(), - BASE_UNIFORM_FIELDS, + PREAMBLE_FIELDS, "it must contribute no uniforms either" ); } + /// Uniform slots reserved before any operation's own: the camera matrix + /// and as-shot white balance, plus framing. The same constant `dr-gpu` + /// writes against, so these offsets cannot agree with each other while + /// disagreeing with the shader. + const PREAMBLE_FIELDS: usize = RESERVED_UNIFORM_FIELDS; + #[test] fn an_active_operation_appears_once() { let ops = vec![fake(&DESC_A, 2.0, false)]; @@ -575,8 +589,8 @@ mod tests { fn uniform_values_follow_declaration_order() { let ops = vec![fake(&DESC_A, 1.5, false), fake(&DESC_B, 2.5, false)]; let shader = compose(&ops); - assert_eq!(shader.uniforms[BASE_UNIFORM_FIELDS], 1.5); - assert_eq!(shader.uniforms[BASE_UNIFORM_FIELDS + 1], 2.5); + assert_eq!(shader.uniforms[PREAMBLE_FIELDS], 1.5); + assert_eq!(shader.uniforms[PREAMBLE_FIELDS + 1], 2.5); } #[test] diff --git a/core/dr-pipeline/src/ops/aberration.rs b/core/dr-pipeline/src/ops/aberration.rs new file mode 100644 index 0000000..316274f --- /dev/null +++ b/core/dr-pipeline/src/ops/aberration.rs @@ -0,0 +1,305 @@ +//! Lateral chromatic aberration correction. +//! +//! The purple-and-green fringing on high-contrast edges toward the frame +//! corners. A lens focuses short wavelengths and long wavelengths at slightly +//! different magnifications, so the red, green and blue images it projects are +//! very slightly different sizes. Rescaling two of them about the optical axis +//! puts them back on top of each other. +//! +//! # Why this cannot be an `Operation` +//! +//! This is the correction that forced [`crate::lens::Warp`] to exist. An +//! [`crate::operation::Operation`] receives `c` — a colour already sampled, +//! with all three channels fetched from *one* coordinate. Lateral CA needs +//! three *different* coordinates, and by the time an operation runs, the +//! information needed to pick them is gone. So it declares +//! [`Warp::splits_channels`] and the composer emits the three-sample path. +//! +//! # The model +//! +//! Lensfun's `poly3`, reduced to its linear term: a per-channel radial scale +//! with green as the fixed reference. +//! +//! ```text +//! r_red = r · v_red +//! r_blue = r · v_blue +//! ``` +//! +//! Green is never moved, and that is a deliberate asymmetry rather than an +//! arbitrary choice of reference. Green carries most of the luminance a Bayer +//! sensor records — twice the photosites of red or blue — so leaving it +//! untouched means a mis-set correction shifts the channels that contribute +//! least to perceived sharpness. Scaling all three about a virtual reference +//! would soften the image even when the correction is right. + +use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; +use crate::lens::Warp; +use crate::operation::{Helper, Uniform}; + +pub const ID: OpId = OpId("aberration"); +pub const RED: ParamId = ParamId("red"); +pub const BLUE: ParamId = ParamId("blue"); + +/// The radial scale at full slider travel, as a fraction. +/// +/// Lateral CA is a tiny effect — the database's own coefficients sit within +/// ±0.1% — so a slider spanning ±0.5% covers every real lens with enough +/// resolution left to tune by eye at 100%. +const MAX_SCALE: f32 = 0.005; + +static DESCRIPTOR: OpDescriptor = OpDescriptor { + id: ID, + label: LocalizedKey("op.aberration"), + params: &[ + // Two independent controls rather than one: the red and blue + // displacements are caused by different ends of the spectrum and are + // not symmetric, so a single "fringing" slider could not remove both. + ParamDescriptor::scalar( + "red", + "param.aberration.red", + -100.0, + 100.0, + 0.0, + Unit::None, + Scale::Linear, + 0, + ), + ParamDescriptor::scalar( + "blue", + "param.aberration.blue", + -100.0, + 100.0, + 0.0, + Unit::None, + Scale::Linear, + 0, + ), + ], +}; + +#[derive(Debug, Default, Clone)] +pub struct Aberration { + red: f32, + blue: f32, + /// Per-channel scales from a lens profile, when one is loaded. + profile: Option<(f32, f32)>, +} + +impl Aberration { + pub fn new() -> Self { + Self::default() + } + + /// Apply a profile's red and blue radial scales. + /// + /// As with distortion, the sliders then trim rather than replace: CA + /// varies between copies of a lens and with focus distance, so a profile + /// gets close and the user finishes the job. + pub fn set_profile(&mut self, scales: Option<(f32, f32)>) { + self.profile = scales; + } + + /// The effective per-channel scales, profile plus manual trim. + fn scales(&self) -> (f32, f32) { + let (base_r, base_b) = self.profile.unwrap_or((1.0, 1.0)); + ( + base_r + self.red / 100.0 * MAX_SCALE, + base_b + self.blue / 100.0 * MAX_SCALE, + ) + } +} + +impl Warp for Aberration { + fn descriptor(&self) -> &'static OpDescriptor { + &DESCRIPTOR + } + + fn set_param(&mut self, id: ParamId, value: f32) { + match id { + RED => self.red = value, + BLUE => self.blue = value, + _ => log::warn!("aberration: unknown parameter {id}"), + } + } + + fn param(&self, id: ParamId) -> f32 { + match id { + RED => self.red, + BLUE => self.blue, + _ => 0.0, + } + } + + fn is_active(&self) -> bool { + let (r, b) = self.scales(); + r != 1.0 || b != 1.0 + } + + fn wgsl_body(&self) -> String { + // `p_r` and `p_b` enter equal to `p` and are carried out of the block. + // Green is deliberately absent: it is the reference and never moves. + "\ +p_r = p * ca_red; +p_b = p * ca_blue;" + .into() + } + + fn uniforms(&self) -> Vec { + let (red, blue) = self.scales(); + vec![ + Uniform { + name: "ca_red", + value: red, + }, + Uniform { + name: "ca_blue", + value: blue, + }, + ] + } + + fn splits_channels(&self) -> bool { + true + } + + fn helpers(&self) -> &'static [Helper] { + &[] + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn neutral_does_nothing() { + let a = Aberration::new(); + assert!(!a.is_active()); + assert_eq!(a.scales(), (1.0, 1.0)); + } + + #[test] + fn a_neutral_correction_leaves_every_channel_coincident() { + // If the channels diverge at neutral, an image with no CA correction + // is resampled into colour fringing that was not there. + let (r, b) = Aberration::new().scales(); + assert_eq!(r, 1.0); + assert_eq!(b, 1.0); + } + + #[test] + fn the_channels_are_controlled_independently() { + // Red and blue displacement have different causes and are not + // symmetric; one slider could not remove both. + let mut a = Aberration::new(); + a.set_param(RED, 100.0); + let (r, b) = a.scales(); + assert!(r > 1.0, "red should be scaled"); + assert_eq!(b, 1.0, "blue must be untouched by the red control"); + } + + #[test] + fn green_is_never_scaled() { + // The reference channel. Asserted through the shader body, since that + // is where a stray green term would actually do damage. + let mut a = Aberration::new(); + a.set_param(RED, 50.0); + a.set_param(BLUE, -50.0); + let body = a.wgsl_body(); + assert!(body.contains("p_r"), "red must be displaced"); + assert!(body.contains("p_b"), "blue must be displaced"); + assert!( + !body.contains("p_g"), + "green is the reference and must never be displaced" + ); + } + + #[test] + fn the_centre_never_moves() { + // A radial scale about the optical axis leaves r = 0 fixed whatever + // the coefficients, which is why CA correction cannot shift a frame. + let mut a = Aberration::new(); + a.set_param(RED, 100.0); + a.set_param(BLUE, -100.0); + let (r, b) = a.scales(); + for scale in [r, b] { + assert_eq!(0.0 * scale, 0.0); + } + } + + #[test] + fn the_correction_stays_subpixel_at_the_extremes() { + // Lateral CA is a fraction of a percent. If full travel displaced a + // corner by more than a pixel or two on a 6000px frame, the slider + // would be a smear control rather than a correction. + let mut a = Aberration::new(); + a.set_param(RED, 100.0); + a.set_param(BLUE, -100.0); + let (r, b) = a.scales(); + // Half-diagonal of a 6000x4000 frame, the worst case. + let half_diag = ((6000.0f32 / 2.0).powi(2) + (4000.0f32 / 2.0).powi(2)).sqrt(); + for scale in [r, b] { + let px = (scale - 1.0).abs() * half_diag; + assert!(px < 25.0, "full travel displaces the corner by {px} px"); + } + } + + #[test] + fn it_always_requests_the_per_channel_path() { + // The declaration that makes the composer emit three samples. Without + // it the fragment would write `p_r`/`p_b` that nothing reads. + assert!(Aberration::new().splits_channels()); + } + + #[test] + fn a_profile_corrects_with_both_sliders_at_zero() { + let mut a = Aberration::new(); + assert!(!a.is_active()); + a.set_profile(Some((1.0003211, 1.0000667))); + assert!(a.is_active()); + assert_eq!(a.param(RED), 0.0); + assert_eq!(a.param(BLUE), 0.0); + } + + #[test] + fn the_sliders_trim_a_loaded_profile() { + // CA varies between copies of a lens and with focus distance, so a + // profile must remain tunable rather than being all-or-nothing. + let profile = (1.0003, 1.0001); + let mut a = Aberration::new(); + a.set_profile(Some(profile)); + a.set_param(RED, 100.0); + + let (r, b) = a.scales(); + assert!((r - (profile.0 + MAX_SCALE)).abs() < 1e-9); + assert_eq!(b, profile.1, "the red trim must not disturb blue"); + } + + #[test] + fn a_profile_can_be_cleared() { + let mut a = Aberration::new(); + a.set_profile(Some((1.0003, 1.0001))); + assert!(a.is_active()); + a.set_profile(None); + assert!(!a.is_active()); + } + + #[test] + fn the_wgsl_body_reads_its_declared_uniforms() { + let mut a = Aberration::new(); + a.set_param(RED, 50.0); + let body = a.wgsl_body(); + for u in a.uniforms() { + assert!(body.contains(u.name), "{} is declared but unused", u.name); + } + } + + #[test] + fn every_default_is_neutral() { + let mut a = Aberration::new(); + for p in DESCRIPTOR.params { + a.set_param(p.id, p.default); + } + assert!(!a.is_active(), "descriptor defaults must be neutral"); + } +} diff --git a/core/dr-pipeline/src/ops/distortion.rs b/core/dr-pipeline/src/ops/distortion.rs index 7c70f0b..e20707f 100644 --- a/core/dr-pipeline/src/ops/distortion.rs +++ b/core/dr-pipeline/src/ops/distortion.rs @@ -1,7 +1,7 @@ //! Geometric distortion correction. //! //! Straightens the lines a lens bends: barrel distortion on wide angles, -//! pincushion on telephotos. A [`crate::warp::Warp`] rather than an +//! pincushion on telephotos. A [`crate::lens::Warp`] rather than an //! [`crate::operation::Operation`], because it changes *where* a pixel is read //! from rather than what its value becomes. //! @@ -26,11 +26,9 @@ //! profile is a "make the horizon straight" task, which one term does well. //! The full triple is reachable by loading a profile. -use crate::descriptor::{ - LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit, -}; +use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; +use crate::lens::Warp; use crate::operation::{Helper, Uniform}; -use crate::warp::Warp; pub const ID: OpId = OpId("distortion"); pub const AMOUNT: ParamId = ParamId("amount"); diff --git a/core/dr-pipeline/src/ops/mod.rs b/core/dr-pipeline/src/ops/mod.rs index 38e51fc..a189d8b 100644 --- a/core/dr-pipeline/src/ops/mod.rs +++ b/core/dr-pipeline/src/ops/mod.rs @@ -1,21 +1,33 @@ //! The develop operations. //! -//! Each operation is a self-contained file implementing -//! [`crate::operation::Operation`]. Adding one means writing that file and -//! adding it to [`crate::graph::EditGraph::default_chain`] — no central +//! Each operation is a self-contained file. Adding one means writing that file +//! and adding it to [`crate::graph::EditGraph::default_chain`] — no central //! shader to edit, no UI change (FR-DEV-3c). +//! +//! Most implement [`crate::operation::Operation`], a function from colour to +//! colour. The optical corrections ([`distortion`]) implement +//! [`crate::lens::Warp`] instead, because they rewrite *coordinates* before +//! the source is sampled rather than transforming a colour after it. Both +//! publish the same [`crate::descriptor::OpDescriptor`], so the UI builds +//! controls for them identically and never learns the difference. +pub mod aberration; pub mod colour; pub mod colour_mixer; pub mod contrast; +pub mod distortion; pub mod exposure; pub mod helpers; pub mod tone; +pub mod vignetting; pub mod white_balance; +pub use aberration::Aberration; pub use colour::{Brilliance, Saturation, Vibrance}; pub use colour_mixer::ColourMixer; pub use contrast::Contrast; +pub use distortion::Distortion; pub use exposure::Exposure; pub use tone::{BlacksWhites, HighlightsShadows}; +pub use vignetting::Vignetting; pub use white_balance::WhiteBalance; diff --git a/core/dr-pipeline/src/ops/vignetting.rs b/core/dr-pipeline/src/ops/vignetting.rs new file mode 100644 index 0000000..b1547c2 --- /dev/null +++ b/core/dr-pipeline/src/ops/vignetting.rs @@ -0,0 +1,375 @@ +//! Vignetting correction — the corner falloff a lens imposes. +//! +//! Every lens delivers less light to the corners than to the centre, by up to +//! two stops wide open. This restores it. +//! +//! # Why this one *is* an `Operation` +//! +//! Distortion and CA are [`crate::lens::Warp`]s because they change which +//! pixel is read. Vignetting does not: it applies a gain to the pixel already +//! there. That the gain happens to depend on the pixel's *position* does not +//! make it a coordinate transform — the sampling is unchanged, so it composes +//! as an ordinary colour fragment and costs no extra texture read. +//! +//! It needs the pixel's normalised radius, which a colour fragment is not +//! otherwise given. The composer publishes `radius` in the shader prologue for +//! exactly this reason: it is derived from coordinates the prologue has +//! already computed, so making it available costs nothing. +//! +//! # The model +//! +//! Lensfun's `pa` polynomial, in even powers of the radius: +//! +//! ```text +//! attenuation = 1 + k1·r² + k2·r⁴ + k3·r⁶ +//! ``` +//! +//! Only even powers, because vignetting is symmetric about the optical axis — +//! an odd term would describe a lens brighter on one side than the other, +//! which is a mount fault rather than a lens characteristic. +//! +//! The value is what the lens *did*, so correcting means dividing by it. That +//! division is the whole reason this operation must run before the tonal +//! stages: a corner recovered by two stops has to be recovered while the +//! highlight headroom to hold it still exists (ARCH §5.2). + +use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; +use crate::operation::{Helper, Operation, Uniform}; + +pub const ID: OpId = OpId("vignetting"); +pub const AMOUNT: ParamId = ParamId("amount"); + +/// The `k1` coefficient at full manual travel. +/// +/// Chosen so +100 lifts the extreme corner by roughly a stop, which covers a +/// fast prime wide open — the case that actually needs correcting. +const MAX_K1: f32 = -0.5; + +static DESCRIPTOR: OpDescriptor = OpDescriptor { + id: ID, + label: LocalizedKey("op.vignetting"), + // Bidirectional deliberately. Negative values *add* falloff, which is a + // legitimate creative choice as well as a correction, and a control that + // only removed vignetting would need a second one beside it to put any + // back. + params: &[ParamDescriptor::amount("amount", "param.vignetting.amount")], +}; + +/// The `pa` polynomial coefficients, as Lensfun stores them. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Pa { + pub k1: f32, + pub k2: f32, + pub k3: f32, +} + +#[derive(Debug, Default, Clone)] +pub struct Vignetting { + amount: f32, + profile: Option, +} + +impl Vignetting { + pub fn new() -> Self { + Self::default() + } + + /// Apply a lens profile's falloff coefficients. + pub fn set_profile(&mut self, profile: Option) { + self.profile = profile; + } + + /// The effective coefficients: profile plus manual trim on `k1`. + /// + /// The trim lands on `k1` alone. It is the dominant term, and adjusting + /// the higher orders by eye would change the *shape* of the falloff rather + /// than its depth — which is not what a photographer reaching for this + /// slider wants. + fn coefficients(&self) -> Pa { + let trim = self.amount / 100.0 * MAX_K1; + match self.profile { + Some(p) => Pa { + k1: p.k1 + trim, + k2: p.k2, + k3: p.k3, + }, + None => Pa { + k1: trim, + k2: 0.0, + k3: 0.0, + }, + } + } +} + +impl Operation for Vignetting { + fn descriptor(&self) -> &'static OpDescriptor { + &DESCRIPTOR + } + + fn set_param(&mut self, id: ParamId, value: f32) { + match id { + AMOUNT => self.amount = value, + _ => log::warn!("vignetting: unknown parameter {id}"), + } + } + + fn param(&self, id: ParamId) -> f32 { + match id { + AMOUNT => self.amount, + _ => 0.0, + } + } + + fn is_active(&self) -> bool { + let c = self.coefficients(); + c.k1 != 0.0 || c.k2 != 0.0 || c.k3 != 0.0 + } + + fn wgsl_body(&self) -> String { + // `radius` comes from the prologue: the pixel's distance from the + // optical axis, normalised so the corner is 1. + "\ +c = c / vignette_attenuation(radius, vig_k1, vig_k2, vig_k3);" + .into() + } + + fn uniforms(&self) -> Vec { + let c = self.coefficients(); + vec![ + Uniform { + name: "vig_k1", + value: c.k1, + }, + Uniform { + name: "vig_k2", + value: c.k2, + }, + Uniform { + name: "vig_k3", + value: c.k3, + }, + ] + } + + fn helpers(&self) -> &'static [Helper] { + VIGNETTE + } +} + +static VIGNETTE: &[Helper] = &[Helper { + name: "vignette_attenuation", + source: "\ +// Lensfun's `pa` vignetting polynomial: 1 + k1*r^2 + k2*r^4 + k3*r^6. +// +// Returns what the lens *did* to this pixel, so correcting divides by it. +// Even powers only: vignetting is symmetric about the optical axis, and an +// odd term would describe a lens brighter on one side than the other. +// +// The result is floored well above zero. A profile evaluated slightly outside +// its calibrated range can produce a near-zero or negative attenuation, and +// dividing by that turns the extreme corners into blown or inverted pixels — +// a far more visible fault than the under-correction the floor causes. +fn vignette_attenuation(r: f32, k1: f32, k2: f32, k3: f32) -> f32 { + let r2 = r * r; + let a = 1.0 + r2 * (k1 + r2 * (k2 + r2 * k3)); + return max(a, 0.05); +}", +}]; + +#[cfg(test)] +mod tests { + use super::*; + + /// The attenuation the shader would compute, mirrored on the CPU so the + /// maths is testable without a device (ARCH §6.5a). + fn attenuation(c: Pa, r: f32) -> f32 { + let r2 = r * r; + (1.0 + r2 * (c.k1 + r2 * (c.k2 + r2 * c.k3))).max(0.05) + } + + #[test] + fn neutral_does_nothing() { + let v = Vignetting::new(); + assert!(!v.is_active()); + let c = v.coefficients(); + assert_eq!((c.k1, c.k2, c.k3), (0.0, 0.0, 0.0)); + } + + #[test] + fn a_neutral_polynomial_is_unity_everywhere() { + // Otherwise opening an image would rescale its brightness for nothing. + let c = Vignetting::new().coefficients(); + for r in [0.0, 0.25, 0.5, 0.75, 1.0] { + assert!((attenuation(c, r) - 1.0).abs() < 1e-6, "r={r} was scaled"); + } + } + + #[test] + fn the_centre_is_never_altered() { + // r = 0 kills every term, so the optical axis keeps its exposure + // whatever the coefficients. If this failed, the correction would be + // an exposure control with a gradient attached. + for amount in [-100.0, -50.0, 50.0, 100.0] { + let mut v = Vignetting::new(); + v.set_param(AMOUNT, amount); + assert!( + (attenuation(v.coefficients(), 0.0) - 1.0).abs() < 1e-6, + "amount {amount} changed the centre" + ); + } + } + + #[test] + fn a_positive_amount_brightens_the_corners() { + // The correcting direction: the lens darkened the corners, so the + // attenuation there must be below 1 and the division lifts them. + let mut v = Vignetting::new(); + v.set_param(AMOUNT, 100.0); + let a = attenuation(v.coefficients(), 1.0); + assert!(a < 1.0, "corner attenuation was {a}, expected < 1"); + } + + #[test] + fn a_negative_amount_darkens_them_instead() { + // The creative direction. A control that only removed vignetting + // would need a second one beside it to add any back. + let mut v = Vignetting::new(); + v.set_param(AMOUNT, -100.0); + assert!(attenuation(v.coefficients(), 1.0) > 1.0); + } + + #[test] + fn falloff_increases_monotonically_with_radius() { + // Vignetting is a smooth darkening toward the corners. A polynomial + // that reversed partway would produce a visible bright ring, which + // reads as a rendering fault rather than as a wrong setting. + let mut v = Vignetting::new(); + v.set_param(AMOUNT, 100.0); + let c = v.coefficients(); + let mut prev = f32::MAX; + for i in 0..=100 { + let a = attenuation(c, i as f32 / 100.0); + assert!( + a <= prev + 1e-6, + "attenuation rose at r={}", + i as f32 / 100.0 + ); + prev = a; + } + } + + #[test] + fn full_correction_is_worth_about_a_stop() { + // The calibration behind MAX_K1. If this drifts, the slider either + // cannot fix a fast prime or overshoots wildly at half travel. + let mut v = Vignetting::new(); + v.set_param(AMOUNT, 100.0); + let gain = 1.0 / attenuation(v.coefficients(), 1.0); + assert!( + (1.5..=2.5).contains(&gain), + "corner gain was {gain}x, expected roughly a stop" + ); + } + + #[test] + fn the_attenuation_never_reaches_zero() { + // Division by a near-zero attenuation blows the corners to white or + // inverts them. The floor must hold even for coefficients well past + // anything the sliders can reach. + let extreme = Pa { + k1: -5.0, + k2: -5.0, + k3: -5.0, + }; + for i in 0..=100 { + let a = attenuation(extreme, i as f32 / 100.0); + assert!(a >= 0.05, "attenuation fell to {a}"); + assert!(a.is_finite() && a > 0.0); + } + } + + #[test] + fn a_profile_corrects_with_the_slider_at_zero() { + let mut v = Vignetting::new(); + assert!(!v.is_active()); + v.set_profile(Some(Pa { + k1: -0.3499, + k2: -0.914, + k3: 0.6689, + })); + assert!(v.is_active()); + assert_eq!(v.param(AMOUNT), 0.0); + } + + #[test] + fn a_real_profile_darkens_the_corners() { + // Coefficients taken from the Lensfun entry for the Canon EF 16-35mm + // f/2.8L at 20mm, f/2.8 — a real lens wide open, which is the case + // this correction exists for. + let profile = Pa { + k1: -0.3499, + k2: -0.914, + k3: 0.6689, + }; + let mut v = Vignetting::new(); + v.set_profile(Some(profile)); + + let c = v.coefficients(); + assert!(attenuation(c, 1.0) < attenuation(c, 0.0)); + assert!((attenuation(c, 0.0) - 1.0).abs() < 1e-6); + } + + #[test] + fn the_slider_trims_a_loaded_profile_rather_than_replacing_it() { + let profile = Pa { + k1: -0.35, + k2: -0.91, + k3: 0.67, + }; + let mut v = Vignetting::new(); + v.set_profile(Some(profile)); + v.set_param(AMOUNT, 100.0); + + let c = v.coefficients(); + assert_eq!(c.k2, profile.k2, "the higher orders must survive a trim"); + assert_eq!(c.k3, profile.k3); + assert!((c.k1 - (profile.k1 + MAX_K1)).abs() < 1e-6); + } + + #[test] + fn a_profile_can_be_cleared() { + let mut v = Vignetting::new(); + v.set_profile(Some(Pa { + k1: -0.3, + k2: 0.0, + k3: 0.0, + })); + assert!(v.is_active()); + v.set_profile(None); + assert!(!v.is_active()); + } + + #[test] + fn the_wgsl_body_reads_its_declared_uniforms_and_the_radius() { + let mut v = Vignetting::new(); + v.set_param(AMOUNT, 50.0); + let body = v.wgsl_body(); + for u in v.uniforms() { + assert!(body.contains(u.name), "{} is declared but unused", u.name); + } + assert!( + body.contains("radius"), + "vignetting is radial and must read the prologue's radius" + ); + } + + #[test] + fn every_default_is_neutral() { + let mut v = Vignetting::new(); + for p in DESCRIPTOR.params { + v.set_param(p.id, p.default); + } + assert!(!v.is_active()); + } +} diff --git a/core/dr-sync-nextcloud/Cargo.toml b/core/dr-sync-nextcloud/Cargo.toml index c530bc8..d5c092e 100644 --- a/core/dr-sync-nextcloud/Cargo.toml +++ b/core/dr-sync-nextcloud/Cargo.toml @@ -8,6 +8,7 @@ license.workspace = true [dependencies] dr-types.workspace = true dr-sync.workspace = true +dr-plat.workspace = true reqwest.workspace = true rustls.workspace = true quick-xml.workspace = true diff --git a/core/dr-sync-nextcloud/examples/connect.rs b/core/dr-sync-nextcloud/examples/connect.rs index 009992c..91508c9 100644 --- a/core/dr-sync-nextcloud/examples/connect.rs +++ b/core/dr-sync-nextcloud/examples/connect.rs @@ -20,11 +20,11 @@ //! FR-NC-2 requires the real app to use platform secure storage. use std::collections::HashMap; -use std::path::PathBuf; use std::time::Instant; +use dr_plat::PlatformSecretStore; use dr_sync::{RemoteBackend, RemoteId, RemotePath, SyncStrategy}; -use dr_sync_nextcloud::{auth, AppCredentials, NextcloudBackend}; +use dr_sync_nextcloud::{auth, AppCredentials, NextcloudBackend, Session, SessionStore}; #[tokio::main] async fn main() { @@ -55,26 +55,33 @@ async fn main() { dr_types::FormatFilter::all() }; - let creds = match load_cached(&server) { - Some(c) => { - println!("using cached credentials for {}", c.login_name); - c - } - None => match authenticate(&server).await { - Ok(c) => c, + // Sessions persist across runs: credentials in the platform keyring + // (FR-NC-2), everything else as ordinary config. + let sessions = SessionStore::open(Box::new(PlatformSecretStore::new())); + if !sessions.can_remember() { + println!("note: no secrets daemon — sign-in will not persist this session"); + } + + let existing = sessions + .current() + .filter(|s| s.server == server.trim_end_matches('/')); + + let (session, creds) = match existing { + Some(s) => match sessions.credentials(&s) { + Ok(c) => { + println!("signed in: {}", s.describe()); + (s, c) + } Err(e) => { - eprintln!("authentication failed: {e}"); - std::process::exit(1); + // Revoked server-side, or the keyring was cleared. + println!("stored credential unusable ({e}); signing in again"); + sign_in(&server, &sessions).await } }, + None => sign_in(&server, &sessions).await, }; - // The DAV base needs the *user id*, which may differ from the login name - // (a login can be an email address). OCS reports the real one. - let user_id = fetch_user_id(&creds).await.unwrap_or_else(|e| { - eprintln!("could not resolve user id ({e}); falling back to login name"); - creds.login_name.clone() - }); + let user_id = session.user_id.clone(); println!("user id: {user_id}"); let backend = NextcloudBackend::new(&creds, &user_id).expect("build backend"); @@ -205,7 +212,17 @@ async fn main() { println!("\n[range] skipped — no RAW over 300KB found"); } - println!("\nscan complete"); + // Remember what was scanned, so the next launch resumes here. + let mut updated = session.clone(); + updated.root = start_path.clone(); + updated.set_format_filter(&filter); + if let Err(e) = sessions.update(&updated) { + eprintln!("could not update session: {e}"); + } else { + println!("\nremembered: {}", updated.describe()); + } + + println!("scan complete"); } fn describe_filter(f: &dr_types::FormatFilter) -> String { @@ -217,24 +234,48 @@ fn describe_filter(f: &dr_types::FormatFilter) -> String { } } -async fn authenticate(server: &str) -> Result { - let client = - dr_sync_nextcloud::http_client("DarkRoom (connect example)").map_err(|e| e.to_string())?; +/// Run Login Flow v2 and persist the result. +async fn sign_in(server: &str, sessions: &SessionStore) -> (Session, AppCredentials) { + let client = match dr_sync_nextcloud::http_client("DarkRoom") { + Ok(c) => c, + Err(e) => { + eprintln!("could not build http client: {e}"); + std::process::exit(1); + } + }; - let flow = auth::begin(&client, server, "DarkRoom (connect example)") - .await - .map_err(|e| e.to_string())?; + let flow = match auth::begin(&client, server, "DarkRoom (connect example)").await { + Ok(f) => f, + Err(e) => { + eprintln!("could not start login: {e}"); + std::process::exit(1); + } + }; println!("\n Open this in a browser and approve:\n"); println!(" {}\n", flow.login_url); println!(" waiting (20 minute limit)…"); - let creds = auth::poll(&client, &flow) - .await - .map_err(|e| e.to_string())?; + let creds = match auth::poll(&client, &flow).await { + Ok(c) => c, + Err(e) => { + eprintln!("login failed: {e}"); + std::process::exit(1); + } + }; println!(" authenticated as {}", creds.login_name); - save_cached(server, &creds); - Ok(creds) + + let user_id = fetch_user_id(&creds).await.unwrap_or_else(|e| { + eprintln!(" could not resolve user id ({e}); using login name"); + creds.login_name.clone() + }); + + let session = Session::new(&creds, user_id); + match sessions.save(&session, &creds) { + Ok(()) => println!(" session saved to {}", sessions.config_path().display()), + Err(e) => eprintln!(" could not persist session: {e}"), + } + (session, creds) } /// Resolve the real user id, which the DAV path needs. @@ -262,38 +303,6 @@ async fn fetch_user_id(creds: &AppCredentials) -> Result { .ok_or_else(|| "no id in OCS response".to_string()) } -fn cache_path(server: &str) -> PathBuf { - let dir = std::env::var_os("XDG_CACHE_HOME") - .map(PathBuf::from) - .unwrap_or_else(|| PathBuf::from(std::env::var("HOME").unwrap_or_default()).join(".cache")) - .join("darkroom"); - let _ = std::fs::create_dir_all(&dir); - let key: String = server - .chars() - .map(|c| if c.is_alphanumeric() { c } else { '_' }) - .collect(); - dir.join(format!("{key}.json")) -} - -fn load_cached(server: &str) -> Option { - let text = std::fs::read_to_string(cache_path(server)).ok()?; - serde_json::from_str(&text).ok() -} - -fn save_cached(server: &str, creds: &AppCredentials) { - let path = cache_path(server); - if let Ok(json) = serde_json::to_string(creds) { - let _ = std::fs::write(&path, json); - // Testing convenience only — FR-NC-2 requires platform secure storage. - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let _ = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)); - } - println!(" cached credentials at {}", path.display()); - } -} - fn human(bytes: u64) -> String { match bytes { b if b >= 1_000_000_000 => format!("{:.1}GB", b as f64 / 1e9), diff --git a/core/dr-sync-nextcloud/src/lib.rs b/core/dr-sync-nextcloud/src/lib.rs index 2831f1a..db4b635 100644 --- a/core/dr-sync-nextcloud/src/lib.rs +++ b/core/dr-sync-nextcloud/src/lib.rs @@ -16,9 +16,11 @@ use dr_sync::{ pub mod auth; pub mod desktop_client; mod propfind; +pub mod session; pub use auth::{AppCredentials, LoginFlow}; pub use desktop_client::DesktopClient; +pub use session::{Session, SessionError, SessionStore}; /// Chunk sizes Nextcloud's chunked upload v2 accepts. const CHUNKS: ChunkConstraints = ChunkConstraints { diff --git a/core/dr-sync-nextcloud/src/session.rs b/core/dr-sync-nextcloud/src/session.rs new file mode 100644 index 0000000..2139e7b --- /dev/null +++ b/core/dr-sync-nextcloud/src/session.rs @@ -0,0 +1,419 @@ +//! Account sessions — logging in once and staying logged in. +//! +//! Splits deliberately in two: +//! +//! - **Credentials** go to platform secure storage (FR-NC-2). Never the +//! catalog, never a file, never a log line. +//! - **Everything else** — server, login, chosen root, format filter — is +//! ordinary configuration, safe to write as plain JSON. +//! +//! That split is what lets the app show "signed in as duncan, watching +//! /PhotosRaw" before it has touched the keyring, and re-authenticate cleanly +//! if the credential has been revoked server-side. + +use std::path::{Path, PathBuf}; + +use dr_plat::{SecretError, SecretRef, SecretStore}; +use dr_sync::RemoteError; +use dr_types::{Format, FormatFilter}; +use serde::{Deserialize, Serialize}; + +use crate::AppCredentials; + +/// A configured account, minus its credential. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Session { + pub server: String, + pub login: String, + /// The DAV path segment, which may differ from `login` — a login can be + /// an email address while the user id is something else. + pub user_id: String, + /// The folder chosen as the library root. Empty means the account root. + #[serde(default)] + pub root: String, + /// Which formats the scan looks for (the tick-boxes). + #[serde(default)] + pub formats: Vec, + /// Unix seconds of the last completed scan, for display. + #[serde(default)] + pub last_scan: Option, +} + +impl Session { + pub fn new(creds: &AppCredentials, user_id: impl Into) -> Self { + Self { + server: creds.server.trim_end_matches('/').to_string(), + login: creds.login_name.clone(), + user_id: user_id.into(), + root: String::new(), + formats: Vec::new(), + last_scan: None, + } + } + + /// The stored format selection, defaulting to every supported format. + /// + /// An unconfigured session must find everything rather than nothing. + pub fn format_filter(&self) -> FormatFilter { + if self.formats.is_empty() { + FormatFilter::all() + } else { + FormatFilter::from_formats( + self.formats + .iter() + .filter_map(|s| Format::from_extension(&s.to_ascii_lowercase())), + ) + } + } + + pub fn set_format_filter(&mut self, filter: &FormatFilter) { + self.formats = filter + .iter() + .map(|f| format!("{f:?}").to_lowercase()) + .collect(); + } + + /// Where this session's credential lives. + pub fn secret_ref(&self) -> SecretRef { + SecretRef::app_password(&self.server, &self.login) + } + + /// A short description for the UI. + pub fn describe(&self) -> String { + let host = self + .server + .trim_start_matches("https://") + .trim_start_matches("http://"); + if self.root.is_empty() { + format!("{} on {host}", self.login) + } else { + format!("{} on {host}/{}", self.login, self.root) + } + } +} + +/// TRACES: FR-NC-1 | FR-NC-2 | M-1 | M-2 +/// Loads and saves sessions, keeping credentials in secure storage. +pub struct SessionStore { + config_path: PathBuf, + secrets: Box, +} + +/// What is written to disk. Versioned so a format change is a migration +/// rather than a parse failure. +#[derive(Debug, Default, Serialize, Deserialize)] +struct ConfigFile { + #[serde(default = "one")] + version: u32, + #[serde(default)] + sessions: Vec, +} + +fn one() -> u32 { + 1 +} + +impl SessionStore { + /// Open the store at the platform config location. + /// + /// Linux: `$XDG_CONFIG_HOME/darkroom/sessions.json`, falling back to + /// `~/.config` (FR-PLAT-LIN-1). + pub fn open(secrets: Box) -> Self { + let dir = std::env::var_os("XDG_CONFIG_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| { + PathBuf::from(std::env::var("HOME").unwrap_or_default()).join(".config") + }) + .join("darkroom"); + Self::open_at(dir.join("sessions.json"), secrets) + } + + /// Open at an explicit path — used by tests, and by anything wanting a + /// non-default config location. + pub fn open_at(config_path: PathBuf, secrets: Box) -> Self { + Self { + config_path, + secrets, + } + } + + pub fn config_path(&self) -> &Path { + &self.config_path + } + + /// Whether credentials can be remembered at all. + /// + /// Where false the UI should say sign-in will not persist, rather than + /// letting the user discover it next launch. + pub fn can_remember(&self) -> bool { + self.secrets.is_available() + } + + /// Every configured session. Missing or unreadable config yields an empty + /// list rather than an error — a first run is not a failure. + pub fn list(&self) -> Vec { + self.read_config().sessions + } + + /// The most recently configured session, if any. + pub fn current(&self) -> Option { + self.read_config().sessions.into_iter().next_back() + } + + /// Persist a session and its credential. + /// + /// The credential goes to secure storage first: if that fails there is no + /// point recording a session that cannot authenticate. + pub fn save(&self, session: &Session, creds: &AppCredentials) -> Result<(), SessionError> { + self.secrets + .store(&session.secret_ref(), &creds.app_password)?; + + let mut config = self.read_config(); + config + .sessions + .retain(|s| !(s.server == session.server && s.login == session.login)); + config.sessions.push(session.clone()); + self.write_config(&config) + } + + /// Update a session's settings, leaving its credential untouched. + pub fn update(&self, session: &Session) -> Result<(), SessionError> { + let mut config = self.read_config(); + match config + .sessions + .iter_mut() + .find(|s| s.server == session.server && s.login == session.login) + { + Some(existing) => *existing = session.clone(), + None => config.sessions.push(session.clone()), + } + self.write_config(&config) + } + + /// Rebuild credentials for a session from secure storage. + /// + /// [`SecretError::NotFound`] means the credential was revoked or the + /// keyring was cleared — the caller re-runs the login flow. + pub fn credentials(&self, session: &Session) -> Result { + let password = self.secrets.retrieve(&session.secret_ref())?; + Ok(AppCredentials { + server: session.server.clone(), + login_name: session.login.clone(), + app_password: password, + }) + } + + /// Forget a session and delete its credential. + /// + /// The credential is removed even if the config write fails, so a logout + /// never leaves a usable secret behind. + pub fn forget(&self, session: &Session) -> Result<(), SessionError> { + let deleted = self.secrets.delete(&session.secret_ref()); + + let mut config = self.read_config(); + config + .sessions + .retain(|s| !(s.server == session.server && s.login == session.login)); + let written = self.write_config(&config); + + deleted?; + written + } + + fn read_config(&self) -> ConfigFile { + std::fs::read_to_string(&self.config_path) + .ok() + .and_then(|t| serde_json::from_str(&t).ok()) + .unwrap_or_default() + } + + fn write_config(&self, config: &ConfigFile) -> Result<(), SessionError> { + if let Some(parent) = self.config_path.parent() { + std::fs::create_dir_all(parent)?; + } + let json = serde_json::to_string_pretty(config)?; + + // Write and rename, so an interrupted save cannot truncate an + // existing config. + let tmp = self.config_path.with_extension("tmp"); + std::fs::write(&tmp, json)?; + std::fs::rename(&tmp, &self.config_path)?; + Ok(()) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum SessionError { + #[error("secure storage: {0}")] + Secret(#[from] SecretError), + + #[error("config io: {0}")] + Io(#[from] std::io::Error), + + #[error("config format: {0}")] + Serde(#[from] serde_json::Error), + + #[error(transparent)] + Remote(#[from] RemoteError), +} + +#[cfg(test)] +mod tests { + use super::*; + use dr_plat::EphemeralSecretStore; + + fn creds() -> AppCredentials { + AppCredentials { + server: "https://cloud.example/".into(), + login_name: "duncan".into(), + app_password: "secret-token".into(), + } + } + + fn store_in(dir: &Path) -> SessionStore { + SessionStore::open_at( + dir.join("sessions.json"), + Box::new(EphemeralSecretStore::new()), + ) + } + + fn tmpdir(name: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("darkroom-test-{name}")); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + d + } + + #[test] + fn a_saved_session_survives_reopening() { + let dir = tmpdir("survives"); + let secrets = Box::new(EphemeralSecretStore::new()); + + // Same secret store instance, as a real process would have. + let store = SessionStore::open_at(dir.join("sessions.json"), secrets); + let mut s = Session::new(&creds(), "duncan"); + s.root = "PhotosRaw".into(); + store.save(&s, &creds()).unwrap(); + + let reloaded = store.current().expect("session persisted"); + assert_eq!(reloaded.login, "duncan"); + assert_eq!(reloaded.root, "PhotosRaw"); + // Trailing slash normalised, so URLs built from it are consistent. + assert_eq!(reloaded.server, "https://cloud.example"); + } + + #[test] + fn the_credential_never_reaches_the_config_file() { + // NFR-SEC-2: the whole point of the split. + let dir = tmpdir("nocreds"); + let store = store_in(&dir); + let s = Session::new(&creds(), "duncan"); + store.save(&s, &creds()).unwrap(); + + let text = std::fs::read_to_string(dir.join("sessions.json")).unwrap(); + assert!(!text.contains("secret-token"), "credential leaked to disk"); + assert!(text.contains("duncan"), "session metadata should be there"); + } + + #[test] + fn credentials_round_trip_through_secure_storage() { + let dir = tmpdir("roundtrip"); + let store = store_in(&dir); + let s = Session::new(&creds(), "duncan"); + store.save(&s, &creds()).unwrap(); + + let got = store.credentials(&s).unwrap(); + assert_eq!(got.app_password, "secret-token"); + assert_eq!(got.login_name, "duncan"); + } + + #[test] + fn forgetting_removes_both_halves() { + let dir = tmpdir("forget"); + let store = store_in(&dir); + let s = Session::new(&creds(), "duncan"); + store.save(&s, &creds()).unwrap(); + + store.forget(&s).unwrap(); + assert!(store.current().is_none()); + assert!(matches!( + store.credentials(&s), + Err(SessionError::Secret(SecretError::NotFound)) + )); + } + + #[test] + fn saving_the_same_account_twice_does_not_duplicate_it() { + let dir = tmpdir("dedupe"); + let store = store_in(&dir); + let mut s = Session::new(&creds(), "duncan"); + store.save(&s, &creds()).unwrap(); + s.root = "Photos".into(); + store.save(&s, &creds()).unwrap(); + + assert_eq!(store.list().len(), 1); + assert_eq!(store.current().unwrap().root, "Photos"); + } + + #[test] + fn a_missing_config_is_a_first_run_not_an_error() { + let dir = tmpdir("firstrun"); + let store = store_in(&dir); + assert!(store.list().is_empty()); + assert!(store.current().is_none()); + } + + #[test] + fn a_corrupt_config_does_not_prevent_starting() { + // Better to present a first-run state than to refuse to launch. + let dir = tmpdir("corrupt"); + std::fs::write(dir.join("sessions.json"), "{ not json").unwrap(); + let store = store_in(&dir); + assert!(store.list().is_empty()); + } + + #[test] + fn format_selection_round_trips() { + let dir = tmpdir("formats"); + let store = store_in(&dir); + let mut s = Session::new(&creds(), "duncan"); + s.set_format_filter(&FormatFilter::from_formats([Format::Cr2, Format::Dng])); + store.save(&s, &creds()).unwrap(); + + let f = store.current().unwrap().format_filter(); + assert!(f.allows(Format::Cr2)); + assert!(f.allows(Format::Dng)); + assert!(!f.allows(Format::Nef)); + } + + #[test] + fn an_unset_filter_means_every_format() { + // Never "no formats", which would silently find nothing. + let s = Session::new(&creds(), "duncan"); + let f = s.format_filter(); + assert!(f.allows(Format::Cr2)); + assert!(f.allows(Format::Jpeg)); + } + + #[test] + fn describe_is_readable_and_hides_the_scheme() { + let mut s = Session::new(&creds(), "duncan"); + assert_eq!(s.describe(), "duncan on cloud.example"); + s.root = "PhotosRaw".into(); + assert_eq!(s.describe(), "duncan on cloud.example/PhotosRaw"); + } + + #[test] + fn updating_settings_leaves_the_credential_alone() { + let dir = tmpdir("update"); + let store = store_in(&dir); + let mut s = Session::new(&creds(), "duncan"); + store.save(&s, &creds()).unwrap(); + + s.root = "Elsewhere".into(); + store.update(&s).unwrap(); + + assert_eq!(store.current().unwrap().root, "Elsewhere"); + assert_eq!(store.credentials(&s).unwrap().app_password, "secret-token"); + } +} diff --git a/docs/architecture.md b/docs/architecture.md index bc271ec..2a126b2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -643,6 +643,16 @@ dir_validator(root) `folders.etag` must exist from schema v1. Adding it later means a migration plus a full re-scan of every user's library (ARCH §6.6). +**Validated against a real library, 2026-08-09.** A cold recursive scan of a 17,185-RAW library +(7,836 CR2 + 9,349 DNG) across 334 directories completed in **34.1 s** — `Depth: 1` per directory, +never `Depth: infinity`. Two implementation details worth keeping: + +- **The format filter sees through VFS placeholder suffixes**, so a dehydrated `IMG.CR2.nextcloud` + matches as the CR2 it stands for rather than being skipped as an unknown type (§9.0). +- **Pruning is capability-gated, not assumed.** With `LocalEtags` a directory probe costs a request + and proves nothing about children, so it is pure overhead; a test asserts zero probes in that + case. Only `PropagatingEtags` makes an unchanged parent prove an unchanged subtree. + ### 8.5 Sidecar conflict resolution `put` with `Precondition::IfMatch(validator)`. On precondition failure: @@ -920,6 +930,22 @@ from schema v1. primary path; server previews are opportunistic. `preview_max_filesize_image` defaults to 50 MB, excluding many RAWs even where a provider exists. +**Measured against a real instance, 2026-08-09, and the case is stronger than assumed.** +`/core/preview` returned HTTP 400 for *every* parameter combination attempted — including bare +`?fileId=N`, and including a JPEG the same server reported as `nc:has-preview=true`. The cause is +unexplained: it is a server-side preview configuration issue, not a request-shape error on our side. + +Recorded as **unexplained rather than understood**, because the distinction matters if someone later +tries to depend on this endpoint. Nothing does today — the connector treats a preview failure as a +miss and falls through to range extraction, which is the designed behaviour rather than a +workaround. + +**Range extraction validated on the same library.** 262 KB read from a 21.5 MB DNG in 119 ms — +**1.22% of the file** — yielded camera model and ISO. Extrapolated across the 17,185-file test +library, cataloguing by whole-file fetch would move roughly 370 GB; the range path moves a few MB. +That ratio is the difference between a viable mobile experience and an unusable one, and it is why +§6.7 treats range reads as the mechanism and server previews as a bonus. + ### 6.8 Sync is selective, not mirror-style ### 6.9 Android forbids the filesystem-scan model diff --git a/docs/catalog.md b/docs/catalog.md index 3e7fb43..4b7cd91 100644 --- a/docs/catalog.md +++ b/docs/catalog.md @@ -458,7 +458,12 @@ Not "on first connect" as a bulk operation. Thumbnails are generated: For a local library this converges on "everything, eventually", because scrolling reaches everything and the background pass has nothing else to do. For a remote library it converges on "what you -actually browsed", which is the difference between a few hundred megabytes and a hundred gigabytes. +actually browsed". + +**Measured on a real 17,185-RAW library, 2026-08-09:** cataloguing it by whole-file fetch would move +roughly **370 GB**; the range-extract path moves a few MB for the images actually viewed. This is +the single largest cost difference in the design, and it is why §7.1 is a list of narrow triggers +rather than "generate them all on connect". ### 7.2 How, by availability @@ -466,7 +471,7 @@ actually browsed", which is the difference between a few hundred megabytes and a |---|---|---| | `Original`, local | Embedded JPEG via `dr-decode` preview path | ~200 KB read, no demosaic | | `Original`, no embedded preview | Full decode, downscale | Expensive — `Background` only | -| Remote | Range-extract embedded JPEG (FR-NC-3) | 1–3 MB vs 25–100 MB | +| Remote | Range-extract embedded JPEG (FR-NC-3) | 1–3 MB vs 25–100 MB — **measured: 262 KB of a 21.5 MB DNG, 119 ms, 1.22% of the file** | | Placeholder / `Offline` | None — render the offline affordance | 0 | The remote path deliberately does **not** ask the Nextcloud client to hydrate the file. ARCH §9.0 @@ -490,7 +495,73 @@ which never evict at all (FR-NC-6b). --- -## 8. What this document does not settle +## 8. Syncing the catalog file + +Decided 2026-08-09. **This qualifies [architecture.md §6.12](architecture.md)** — the catalog +remains a rebuildable index, but the file itself now travels to Nextcloud. The qualification is +worth stating precisely, because the sidecar-authoritative model is load-bearing and this is the +one place it bends. + +### 8.1 Why collections forced this + +Every other thing the catalog holds has authoritative backing outside it. Ratings, labels, +keywords, and edit graphs live in sidecars next to the images, so a rebuild recovers them. +**Collections do not.** A manual collection is a set of images the user assembled by hand; nothing +in the filesystem records it. Losing the catalog loses them, and no rescan brings them back. + +So collections need to be durable across devices somehow. Syncing the catalog file is the chosen +mechanism. + +### 8.2 What the file sync does and does not carry + +Only **collections and their membership** merge. The rest of a catalog describes *local* state — +folder mtimes, cache file paths, job rows, `tier_actual` — and importing another device's version +of those would be actively wrong. The downloaded remote is read for its collections and discarded. + +This is what keeps §6.12 substantially intact: nothing here makes the local database authoritative +for anything a rebuild could not recover. The catalog is still deletable. What syncs is one table +pair that had no other home. + +### 8.3 Two hazards the implementation must handle + +**A WAL database is not one file.** Committed transactions can sit in `catalog.sqlite-wal` with the +main file lagging, so copying `catalog.sqlite` alone uploads a torn snapshot — internally consistent +as of some older point, silently missing everything since. Upload therefore runs a `TRUNCATE` +checkpoint and then SQLite's backup API, which serialises against concurrent writers rather than +racing them. It never copies the live file. + +**Integer primary keys are not identities.** Two devices each allocate `collections.id = 1` for +different collections, so a row-level merge keyed on the integer id would collide them. Collections +therefore carry a **UUID**, and membership maps across devices by **image content hash**. The +integer ids stay local and are never compared across catalogs. + +### 8.4 Merge rules + +| Concern | Rule | Why | +|---|---|---| +| Which collection wins | Higher `revision` — a counter bumped per local edit. `modified` only breaks an exact tie | A device with a skewed clock cannot silently overwrite real work. The same reason FR-NC-9 avoids mtime for sidecars | +| Membership | **Set union**, not last-writer-wins | Two devices adding different images to one collection keep both. The exception — a removal racing an addition — resolves toward the addition, which is recoverable by removing it again. A lost addition is not | +| Deletion | Tombstone (`deleted = 1`) carrying a revision | Without it, merging against a device that still holds the collection resurrects it. With a revision, deletion competes on equal footing with a rename | +| An image the remote has and we do not | Skip the membership row | It joins on a later merge, once a scan has catalogued the file. Not an error | +| A remote from a newer schema | Decline before attaching | Attempting it would fail mid-transaction rather than declining cleanly | + +Merging is idempotent: running it twice reports no changes the second time. That property is tested, +because a merge that oscillates would upload on every sync forever. + +### 8.5 What was rejected + +**Replace-if-newer.** The literal reading of "sync the file and take the newer one". Rejected +because it is not a merge: whichever device syncs second loses every collection the first did not +have. Binary SQLite files do not merge, so "newer wins" means "older is destroyed". + +**A `collections.drsc` sidecar at the library root.** The alternative that would have kept §6.12 +untouched, merging as text the way edit sidecars do. Viable, and cheaper in machinery, but it means +a second serialisation format and a second merge implementation for the same data. Recorded here +because if the SQLite path proves troublesome, this is the fallback with a known shape. + +--- + +## 9. What this document does not settle - **FTS.** `Selector::Text` is a `LIKE` scan over filename and keywords. Adequate at 50k; if free text over description and title becomes a real workflow, an FTS5 table is the answer, and it is diff --git a/docs/traceability.md b/docs/traceability.md index b39bd60..7f04d76 100644 --- a/docs/traceability.md +++ b/docs/traceability.md @@ -9,18 +9,18 @@ Denominators are parsed from [`requirements.md`](requirements.md) at run time, n | Metric | Value | |---|---| -| Source files scanned | 44 | -| TRACES tags found | 37 | +| Source files scanned | 63 | +| TRACES tags found | 49 | | Requirements defined | 143 | -| Requirements covered | 36 | -| **Coverage** | **25.2%** (36/143) | +| Requirements covered | 49 | +| **Coverage** | **34.3%** (49/143) | ### By type | Type | Covered | Defined | |---|---|---| -| FR | 27 | 90 | -| NFR | 7 | 47 | +| FR | 36 | 90 | +| NFR | 11 | 47 | | R | 2 | 6 | ## Orphan tags @@ -33,36 +33,49 @@ _None._ | ID | Tagged in | |---|---| -| FR-CAT-1 | [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473), [`tools/traceability/src/lib.rs:505`](../tools/traceability/src/lib.rs#L505) | -| FR-CAT-1a | [`core/dr-types/src/lib.rs:23`](../core/dr-types/src/lib.rs#L23) | -| FR-CAT-2 | [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473) | +| FR-CAT-1 | [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`core/dr-sync/src/scan.rs:50`](../core/dr-sync/src/scan.rs#L50), [`core/dr-types/src/lib.rs:174`](../core/dr-types/src/lib.rs#L174), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473), [`tools/traceability/src/lib.rs:505`](../tools/traceability/src/lib.rs#L505) | +| FR-CAT-1a | [`core/dr-types/src/lib.rs:36`](../core/dr-types/src/lib.rs#L36) | +| FR-CAT-2 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/schema.rs:1`](../core/dr-catalog/src/schema.rs#L1), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473) | +| FR-CAT-3 | [`core/dr-catalog/src/jobs.rs:1`](../core/dr-catalog/src/jobs.rs#L1) | +| FR-CAT-4 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/query.rs:1`](../core/dr-catalog/src/query.rs#L1) | | FR-CAT-5 | [`core/dr-decode/src/lib.rs:216`](../core/dr-decode/src/lib.rs#L216) | -| FR-CAT-9 | [`core/dr-sync-nextcloud/src/desktop_client.rs:30`](../core/dr-sync-nextcloud/src/desktop_client.rs#L30), [`core/dr-types/src/lib.rs:80`](../core/dr-types/src/lib.rs#L80) | +| FR-CAT-6 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/query.rs:1`](../core/dr-catalog/src/query.rs#L1), [`core/dr-types/src/selector.rs:1`](../core/dr-types/src/selector.rs#L1) | +| FR-CAT-7 | [`core/dr-catalog/src/merge.rs:1`](../core/dr-catalog/src/merge.rs#L1), [`core/dr-catalog/src/sync.rs:1`](../core/dr-catalog/src/sync.rs#L1), [`core/dr-types/src/selector.rs:1`](../core/dr-types/src/selector.rs#L1) | +| FR-CAT-9 | [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`core/dr-sync-nextcloud/src/desktop_client.rs:30`](../core/dr-sync-nextcloud/src/desktop_client.rs#L30), [`core/dr-types/src/lib.rs:93`](../core/dr-types/src/lib.rs#L93) | | FR-CULL-1 | [`core/dr-decode/src/preview.rs:96`](../core/dr-decode/src/preview.rs#L96) | | FR-CULL-2 | [`core/dr-decode/src/preview.rs:123`](../core/dr-decode/src/preview.rs#L123) | -| FR-DEV-3a | [`core/dr-pipeline/src/graph.rs:14`](../core/dr-pipeline/src/graph.rs#L14), [`core/dr-pipeline/src/graph.rs:32`](../core/dr-pipeline/src/graph.rs#L32), [`core/dr-pipeline/src/graph.rs:88`](../core/dr-pipeline/src/graph.rs#L88) | -| FR-DEV-3b | [`core/dr-pipeline/src/graph.rs:32`](../core/dr-pipeline/src/graph.rs#L32) | -| FR-DEV-3c | [`core/dr-pipeline/src/graph.rs:88`](../core/dr-pipeline/src/graph.rs#L88) | -| FR-DEV-3e | [`core/dr-decode/src/lib.rs:325`](../core/dr-decode/src/lib.rs#L325), [`core/dr-decode/src/lib.rs:439`](../core/dr-decode/src/lib.rs#L439) | +| FR-DEV-3 | [`core/dr-pipeline/src/framing.rs:174`](../core/dr-pipeline/src/framing.rs#L174) | +| FR-DEV-3a | [`core/dr-pipeline/src/graph.rs:128`](../core/dr-pipeline/src/graph.rs#L128), [`core/dr-pipeline/src/graph.rs:15`](../core/dr-pipeline/src/graph.rs#L15), [`core/dr-pipeline/src/graph.rs:33`](../core/dr-pipeline/src/graph.rs#L33) | +| FR-DEV-3b | [`core/dr-pipeline/src/graph.rs:33`](../core/dr-pipeline/src/graph.rs#L33) | +| FR-DEV-3c | [`core/dr-pipeline/src/graph.rs:128`](../core/dr-pipeline/src/graph.rs#L128) | +| FR-DEV-3d | [`core/dr-pipeline/src/framing.rs:174`](../core/dr-pipeline/src/framing.rs#L174) | +| FR-DEV-3e | [`core/dr-decode/src/lib.rs:325`](../core/dr-decode/src/lib.rs#L325), [`core/dr-decode/src/lib.rs:445`](../core/dr-decode/src/lib.rs#L445) | | FR-DEV-4 | [`core/dr-gpu/src/lib.rs:123`](../core/dr-gpu/src/lib.rs#L123) | | FR-DSP-1 | [`ui/dr-ui/src/lib.rs:31`](../ui/dr-ui/src/lib.rs#L31) | | FR-EXP-9 | [`core/dr-decode/src/lib.rs:242`](../core/dr-decode/src/lib.rs#L242) | | FR-NC-1 | [`core/dr-sync-nextcloud/src/auth.rs:132`](../core/dr-sync-nextcloud/src/auth.rs#L132), [`core/dr-sync-nextcloud/src/auth.rs:44`](../core/dr-sync-nextcloud/src/auth.rs#L44) | -| FR-NC-12 | [`core/dr-sync-nextcloud/src/lib.rs:32`](../core/dr-sync-nextcloud/src/lib.rs#L32), [`core/dr-sync/src/lib.rs:128`](../core/dr-sync/src/lib.rs#L128), [`core/dr-sync/src/lib.rs:34`](../core/dr-sync/src/lib.rs#L34) | +| FR-NC-12 | [`core/dr-sync-nextcloud/src/lib.rs:34`](../core/dr-sync-nextcloud/src/lib.rs#L34), [`core/dr-sync/src/lib.rs:130`](../core/dr-sync/src/lib.rs#L130), [`core/dr-sync/src/lib.rs:36`](../core/dr-sync/src/lib.rs#L36) | | FR-NC-3 | [`core/dr-decode/src/preview.rs:123`](../core/dr-decode/src/preview.rs#L123), [`core/dr-sync/src/capability.rs:41`](../core/dr-sync/src/capability.rs#L41) | -| FR-NC-4 | [`core/dr-sync-nextcloud/src/propfind.rs:100`](../core/dr-sync-nextcloud/src/propfind.rs#L100), [`core/dr-sync-nextcloud/src/propfind.rs:51`](../core/dr-sync-nextcloud/src/propfind.rs#L51), [`core/dr-sync/src/capability.rs:6`](../core/dr-sync/src/capability.rs#L6), [`core/dr-sync/src/lib.rs:128`](../core/dr-sync/src/lib.rs#L128) | +| FR-NC-4 | [`core/dr-sync-nextcloud/src/propfind.rs:100`](../core/dr-sync-nextcloud/src/propfind.rs#L100), [`core/dr-sync-nextcloud/src/propfind.rs:51`](../core/dr-sync-nextcloud/src/propfind.rs#L51), [`core/dr-sync/src/capability.rs:6`](../core/dr-sync/src/capability.rs#L6), [`core/dr-sync/src/lib.rs:130`](../core/dr-sync/src/lib.rs#L130), [`core/dr-sync/src/scan.rs:50`](../core/dr-sync/src/scan.rs#L50) | | FR-NC-5 | [`core/dr-sync-nextcloud/src/propfind.rs:51`](../core/dr-sync-nextcloud/src/propfind.rs#L51) | -| FR-NC-6c | [`core/dr-sync-nextcloud/src/desktop_client.rs:30`](../core/dr-sync-nextcloud/src/desktop_client.rs#L30), [`core/dr-types/src/lib.rs:131`](../core/dr-types/src/lib.rs#L131), [`core/dr-types/src/lib.rs:80`](../core/dr-types/src/lib.rs#L80) | -| FR-PLAT-AND-1 | [`core/dr-types/src/lib.rs:23`](../core/dr-types/src/lib.rs#L23) | -| FR-RAW-1 | [`core/dr-decode/src/lib.rs:174`](../core/dr-decode/src/lib.rs#L174), [`core/dr-types/src/lib.rs:90`](../core/dr-types/src/lib.rs#L90) | +| FR-NC-6a | [`core/dr-types/src/selector.rs:1`](../core/dr-types/src/selector.rs#L1) | +| FR-NC-6c | [`core/dr-sync-nextcloud/src/desktop_client.rs:30`](../core/dr-sync-nextcloud/src/desktop_client.rs#L30), [`core/dr-types/src/lib.rs:175`](../core/dr-types/src/lib.rs#L175), [`core/dr-types/src/lib.rs:93`](../core/dr-types/src/lib.rs#L93) | +| FR-NC-9 | [`core/dr-catalog/src/merge.rs:1`](../core/dr-catalog/src/merge.rs#L1), [`core/dr-catalog/src/sync.rs:1`](../core/dr-catalog/src/sync.rs#L1) | +| FR-PLAT-AND-1 | [`core/dr-types/src/lib.rs:36`](../core/dr-types/src/lib.rs#L36) | +| FR-PLAT-AND-3 | [`core/dr-catalog/src/jobs.rs:1`](../core/dr-catalog/src/jobs.rs#L1) | +| FR-RAW-1 | [`core/dr-decode/src/lib.rs:174`](../core/dr-decode/src/lib.rs#L174), [`core/dr-types/src/lib.rs:103`](../core/dr-types/src/lib.rs#L103), [`core/dr-types/src/lib.rs:174`](../core/dr-types/src/lib.rs#L174) | | FR-RAW-3 | [`core/dr-decode/src/lib.rs:242`](../core/dr-decode/src/lib.rs#L242), [`core/dr-decode/src/lib.rs:70`](../core/dr-decode/src/lib.rs#L70) | | FR-RAW-4 | [`core/dr-decode/src/error.rs:1`](../core/dr-decode/src/error.rs#L1) | | FR-RAW-5 | [`core/dr-decode/src/lib.rs:98`](../core/dr-decode/src/lib.rs#L98) | | FR-UI-1 | [`ui/dr-ui/src/lib.rs:39`](../ui/dr-ui/src/lib.rs#L39) | | FR-UI-2 | [`ui/dr-ui/src/lib.rs:39`](../ui/dr-ui/src/lib.rs#L39) | +| NFR-ARCH-2 | [`core/dr-catalog/src/jobs.rs:1`](../core/dr-catalog/src/jobs.rs#L1) | +| NFR-ARCH-4 | [`core/dr-catalog/src/error.rs:1`](../core/dr-catalog/src/error.rs#L1) | | NFR-OPS-1 | [`tools/traceability/src/lib.rs:266`](../tools/traceability/src/lib.rs#L266) | -| NFR-P1 | [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473) | +| NFR-P1 | [`core/dr-catalog/src/lib.rs:1`](../core/dr-catalog/src/lib.rs#L1), [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473) | | NFR-P13 | [`core/dr-decode/src/preview.rs:96`](../core/dr-decode/src/preview.rs#L96) | +| NFR-R1 | [`core/dr-catalog/src/sync.rs:1`](../core/dr-catalog/src/sync.rs#L1) | +| NFR-R5 | [`core/dr-catalog/src/error.rs:1`](../core/dr-catalog/src/error.rs#L1), [`core/dr-catalog/src/schema.rs:1`](../core/dr-catalog/src/schema.rs#L1) | | NFR-R7 | [`core/dr-gpu/src/error.rs:1`](../core/dr-gpu/src/error.rs#L1) | | NFR-R8 | [`core/dr-gpu/src/error.rs:1`](../core/dr-gpu/src/error.rs#L1) | | NFR-RES-1 | [`ui/dr-ui/src/lib.rs:31`](../ui/dr-ui/src/lib.rs#L31) | @@ -72,7 +85,7 @@ _None._ ## Not yet tagged -107 of 143 requirements have no implementation tag. Expected while the codebase is young; each should gain one as it is built. +94 of 143 requirements have no implementation tag. Expected while the codebase is young; each should gain one as it is built.
Show untagged requirements @@ -81,10 +94,6 @@ _None._ - FR-CAT-12 - FR-CAT-13 - FR-CAT-14 -- FR-CAT-3 -- FR-CAT-4 -- FR-CAT-6 -- FR-CAT-7 - FR-CAT-8 - FR-CULL-3 - FR-CULL-4 @@ -93,8 +102,6 @@ _None._ - FR-CULL-7 - FR-DEV-1 - FR-DEV-2 -- FR-DEV-3 -- FR-DEV-3d - FR-DEV-3f - FR-DEV-3g - FR-DEV-5 @@ -120,13 +127,10 @@ _None._ - FR-NC-11 - FR-NC-2 - FR-NC-6 -- FR-NC-6a - FR-NC-6b - FR-NC-7 - FR-NC-8 -- FR-NC-9 - FR-PLAT-AND-2 -- FR-PLAT-AND-3 - FR-PLAT-AND-4 - FR-PLAT-AND-5 - FR-PLAT-AND-6 @@ -143,9 +147,7 @@ _None._ - NFR-A11Y-2 - NFR-A11Y-3 - NFR-ARCH-1 -- NFR-ARCH-2 - NFR-ARCH-3 -- NFR-ARCH-4 - NFR-COMPAT-1 - NFR-COMPAT-2 - NFR-OPS-2 @@ -167,11 +169,9 @@ _None._ - NFR-PORT-1 - NFR-PORT-2 - NFR-PORT-3 -- NFR-R1 - NFR-R2 - NFR-R3 - NFR-R4 -- NFR-R5 - NFR-R6 - NFR-RES-2 - NFR-RES-3 diff --git a/platform/dr-plat/Cargo.toml b/platform/dr-plat/Cargo.toml new file mode 100644 index 0000000..6892ba4 --- /dev/null +++ b/platform/dr-plat/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "dr-plat" +version.workspace = true +edition.workspace = true +rust-version.workspace = true +license.workspace = true + +[dependencies] +dr-types.workspace = true +thiserror.workspace = true +log.workspace = true + +[target.'cfg(all(unix, not(target_os = "android")))'.dependencies] +keyring.workspace = true + +[dev-dependencies] +env_logger.workspace = true diff --git a/platform/dr-plat/examples/keyring_check.rs b/platform/dr-plat/examples/keyring_check.rs new file mode 100644 index 0000000..5548ada --- /dev/null +++ b/platform/dr-plat/examples/keyring_check.rs @@ -0,0 +1,64 @@ +//! Verify credentials round-trip through the real platform secret store. +//! +//! cargo run -p dr-plat --example keyring_check +//! +//! Writes a test value, reads it back, deletes it. Touches nothing else. + +use dr_plat::{PlatformSecretStore, SecretRef, SecretStore}; + +fn main() { + env_logger::init(); + let store = PlatformSecretStore::new(); + + println!("store available: {}", store.is_available()); + if !store.is_available() { + println!("no secrets daemon — the app would run in degraded mode"); + return; + } + + let r = SecretRef::app_password("https://test.invalid", "darkroom-selftest"); + let secret = "test-token-do-not-reuse"; + + print!("store … "); + match store.store(&r, secret) { + Ok(()) => println!("ok"), + Err(e) => { + println!("FAILED: {e}"); + std::process::exit(1); + } + } + + print!("retrieve … "); + match store.retrieve(&r) { + Ok(v) if v == secret => println!("ok (round-tripped)"), + Ok(_) => { + println!("FAILED: wrong value"); + std::process::exit(1); + } + Err(e) => { + println!("FAILED: {e}"); + std::process::exit(1); + } + } + + print!("delete … "); + match store.delete(&r) { + Ok(()) => println!("ok"), + Err(e) => { + println!("FAILED: {e}"); + std::process::exit(1); + } + } + + print!("confirm gone … "); + match store.retrieve(&r) { + Err(dr_plat::SecretError::NotFound) => println!("ok"), + Ok(_) => { + println!("FAILED: still present after delete"); + std::process::exit(1); + } + Err(e) => println!("unexpected: {e}"), + } + + println!("\nplatform secret store works (FR-NC-2)"); +} diff --git a/platform/dr-plat/src/lib.rs b/platform/dr-plat/src/lib.rs new file mode 100644 index 0000000..f0151cf --- /dev/null +++ b/platform/dr-plat/src/lib.rs @@ -0,0 +1,11 @@ +//! Platform abstraction for DarkRoom. +//! +//! Traits are defined here and implemented per platform, then injected at +//! construction, so `core/` contains no `#[cfg(target_os)]` (NFR-PORT-1, +//! ARCH §10). + +pub mod secrets; + +pub use secrets::{ + EphemeralSecretStore, PlatformSecretStore, SecretError, SecretKind, SecretRef, SecretStore, +}; diff --git a/platform/dr-plat/src/secrets.rs b/platform/dr-plat/src/secrets.rs new file mode 100644 index 0000000..7aa243b --- /dev/null +++ b/platform/dr-plat/src/secrets.rs @@ -0,0 +1,317 @@ +//! Platform secure storage for credentials (FR-NC-2, NFR-SEC-2). +//! +//! Credentials are **never** written to the catalog, to a plain file, or to +//! logs. On Linux they go to the Secret Service (GNOME Keyring, or KWallet via +//! `ksecretd`, which exposes the same D-Bus interface). On Android they belong +//! in Keystore-backed storage. +//! +//! Absence of a secrets daemon is an explicit degraded mode, not a silent +//! fallback to plaintext: a headless box or a minimal window manager may have +//! none, and quietly writing a password to disk there would be worse than +//! refusing. + +use std::fmt; + +/// Which secret is being stored, so one account can hold several. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SecretKind { + /// A Nextcloud app password from Login Flow v2. Device-scoped and + /// individually revocable — never the user's actual password. + AppPassword, +} + +impl SecretKind { + fn as_str(self) -> &'static str { + match self { + SecretKind::AppPassword => "app-password", + } + } +} + +/// Where a credential lives: one account on one server. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SecretRef { + pub server: String, + pub login: String, + pub kind: SecretKind, +} + +impl SecretRef { + pub fn app_password(server: impl Into, login: impl Into) -> Self { + Self { + server: server.into(), + login: login.into(), + kind: SecretKind::AppPassword, + } + } + + /// The key under which the platform store holds this secret. + /// + /// Includes the server so two accounts on different servers with the same + /// login do not collide. + fn entry_key(&self) -> String { + format!("{}@{}#{}", self.login, self.server, self.kind.as_str()) + } +} + +/// Deliberately opaque: the whole point is that a credential never appears in +/// a log line or an error message (NFR-SEC-2). +impl fmt::Display for SecretRef { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{} on {}", self.login, self.server) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum SecretError { + /// No secrets daemon. The app runs in a degraded mode where the user + /// re-authenticates each session, rather than storing anything in plain. + #[error("no platform secret store available: {0}")] + Unavailable(String), + + #[error("secret not found")] + NotFound, + + #[error("secret store rejected the request: {0}")] + Denied(String), + + #[error("secret store error: {0}")] + Other(String), +} + +/// TRACES: FR-NC-2 | NFR-SEC-2 | M-2 +/// Store, retrieve and delete credentials. +/// +/// Implemented per platform and injected at construction, so `core/` contains +/// no `#[cfg(target_os)]` (NFR-PORT-1). +pub trait SecretStore: Send + Sync { + fn store(&self, secret_ref: &SecretRef, secret: &str) -> Result<(), SecretError>; + fn retrieve(&self, secret_ref: &SecretRef) -> Result; + fn delete(&self, secret_ref: &SecretRef) -> Result<(), SecretError>; + + /// Whether the store is usable right now. + /// + /// Checked before offering to remember a login, so the UI can say + /// "you will need to sign in each time" rather than failing later. + fn is_available(&self) -> bool; +} + +/// The service name entries are filed under. +const SERVICE: &str = "DarkRoom"; + +/// Secret Service implementation (GNOME Keyring, KWallet via ksecretd). +#[cfg(all(unix, not(target_os = "android")))] +pub struct PlatformSecretStore; + +#[cfg(all(unix, not(target_os = "android")))] +impl PlatformSecretStore { + pub fn new() -> Self { + Self + } + + fn entry(r: &SecretRef) -> Result { + keyring::Entry::new(SERVICE, &r.entry_key()).map_err(map_err) + } +} + +#[cfg(all(unix, not(target_os = "android")))] +impl Default for PlatformSecretStore { + fn default() -> Self { + Self::new() + } +} + +#[cfg(all(unix, not(target_os = "android")))] +impl SecretStore for PlatformSecretStore { + fn store(&self, secret_ref: &SecretRef, secret: &str) -> Result<(), SecretError> { + Self::entry(secret_ref)? + .set_password(secret) + .map_err(map_err) + } + + fn retrieve(&self, secret_ref: &SecretRef) -> Result { + Self::entry(secret_ref)?.get_password().map_err(map_err) + } + + fn delete(&self, secret_ref: &SecretRef) -> Result<(), SecretError> { + match Self::entry(secret_ref)?.delete_credential() { + Ok(()) => Ok(()), + // Deleting an absent secret is the desired end state, not a + // failure — logout must be idempotent. + Err(keyring::Error::NoEntry) => Ok(()), + Err(e) => Err(map_err(e)), + } + } + + fn is_available(&self) -> bool { + // Probing a name that will not exist distinguishes "daemon absent" + // from "secret absent": the former errors, the latter reports NoEntry. + match keyring::Entry::new(SERVICE, "__availability_probe__") { + Ok(e) => !matches!( + e.get_password(), + Err(keyring::Error::PlatformFailure(_)) | Err(keyring::Error::NoStorageAccess(_)) + ), + Err(_) => false, + } + } +} + +#[cfg(all(unix, not(target_os = "android")))] +fn map_err(e: keyring::Error) -> SecretError { + match e { + keyring::Error::NoEntry => SecretError::NotFound, + keyring::Error::NoStorageAccess(e) => SecretError::Unavailable(e.to_string()), + keyring::Error::PlatformFailure(e) => SecretError::Unavailable(e.to_string()), + other => SecretError::Other(other.to_string()), + } +} + +/// Placeholder for platforms without an implementation yet. +/// +/// 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; + +#[cfg(not(all(unix, not(target_os = "android"))))] +impl PlatformSecretStore { + pub fn new() -> Self { + Self + } +} + +#[cfg(not(all(unix, not(target_os = "android"))))] +impl Default for PlatformSecretStore { + fn default() -> Self { + Self::new() + } +} + +#[cfg(not(all(unix, not(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(), + )) + } + fn retrieve(&self, _r: &SecretRef) -> Result { + Err(SecretError::Unavailable( + "Keystore-backed storage is not implemented on this platform yet".into(), + )) + } + fn delete(&self, _r: &SecretRef) -> Result<(), SecretError> { + Ok(()) + } + fn is_available(&self) -> bool { + false + } +} + +/// An in-memory store for tests and for the degraded no-daemon mode. +/// +/// Credentials live only as long as the process, so a user without a secrets +/// daemon re-authenticates each session — which is the honest behaviour. +#[derive(Default)] +pub struct EphemeralSecretStore { + entries: std::sync::Mutex>, +} + +impl EphemeralSecretStore { + pub fn new() -> Self { + Self::default() + } +} + +impl SecretStore for EphemeralSecretStore { + fn store(&self, secret_ref: &SecretRef, secret: &str) -> Result<(), SecretError> { + self.entries + .lock() + .map_err(|e| SecretError::Other(e.to_string()))? + .insert(secret_ref.entry_key(), secret.to_string()); + Ok(()) + } + + fn retrieve(&self, secret_ref: &SecretRef) -> Result { + self.entries + .lock() + .map_err(|e| SecretError::Other(e.to_string()))? + .get(&secret_ref.entry_key()) + .cloned() + .ok_or(SecretError::NotFound) + } + + fn delete(&self, secret_ref: &SecretRef) -> Result<(), SecretError> { + self.entries + .lock() + .map_err(|e| SecretError::Other(e.to_string()))? + .remove(&secret_ref.entry_key()); + Ok(()) + } + + fn is_available(&self) -> bool { + true + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn keys_separate_accounts_across_servers() { + // Same login on two servers must not collide, or signing into the + // second would overwrite the first. + let a = SecretRef::app_password("https://a.example", "duncan"); + let b = SecretRef::app_password("https://b.example", "duncan"); + assert_ne!(a.entry_key(), b.entry_key()); + } + + #[test] + fn keys_separate_logins_on_one_server() { + let a = SecretRef::app_password("https://a.example", "duncan"); + let b = SecretRef::app_password("https://a.example", "someone"); + assert_ne!(a.entry_key(), b.entry_key()); + } + + #[test] + fn display_never_reveals_the_secret_or_the_key() { + let r = SecretRef::app_password("https://cloud.example", "duncan"); + let shown = r.to_string(); + assert!(shown.contains("duncan")); + assert!( + !shown.contains("app-password"), + "internal key must not leak" + ); + } + + #[test] + fn ephemeral_round_trips() { + let s = EphemeralSecretStore::new(); + let r = SecretRef::app_password("https://cloud.example", "duncan"); + + assert!(matches!(s.retrieve(&r), Err(SecretError::NotFound))); + s.store(&r, "token-value").unwrap(); + assert_eq!(s.retrieve(&r).unwrap(), "token-value"); + } + + #[test] + fn deleting_is_idempotent() { + // Logout must succeed whether or not a credential is present. + let s = EphemeralSecretStore::new(); + let r = SecretRef::app_password("https://cloud.example", "duncan"); + assert!(s.delete(&r).is_ok()); + s.store(&r, "x").unwrap(); + assert!(s.delete(&r).is_ok()); + assert!(matches!(s.retrieve(&r), Err(SecretError::NotFound))); + } + + #[test] + fn storing_twice_overwrites() { + let s = EphemeralSecretStore::new(); + let r = SecretRef::app_password("https://cloud.example", "duncan"); + s.store(&r, "first").unwrap(); + s.store(&r, "second").unwrap(); + assert_eq!(s.retrieve(&r).unwrap(), "second"); + } +} diff --git a/ui/dr-ui/Cargo.toml b/ui/dr-ui/Cargo.toml index 6e244cf..9ee1dbe 100644 --- a/ui/dr-ui/Cargo.toml +++ b/ui/dr-ui/Cargo.toml @@ -12,6 +12,8 @@ dr-types.workspace = true # must come off when S1 lands (ARCH §6.1, AC-8). dr-gpu = { workspace = true, features = ["readback"] } dr-decode.workspace = true +dr-plat.workspace = true +dr-sync-nextcloud.workspace = true dr-pipeline.workspace = true slint = { workspace = true, features = ["compat-1-2", "renderer-femtovg", "backend-winit"] } wgpu.workspace = true diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index a268b25..c1765f4 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -11,7 +11,7 @@ use dr_decode::RawImage; use dr_gpu::{AdjustPass, DemosaicedImage, Demosaicer, GpuContext}; -use dr_pipeline::{EditGraph, OpId, ParamId, ParamKind, Unit}; +use dr_pipeline::{CropRect, EditGraph, OpId, ParamId, ParamKind, Unit}; use crate::labels; use crate::ParamRow; @@ -135,8 +135,13 @@ impl DevelopSession { pub fn render(&mut self, width: u32, height: u32) -> Result { // Fit the render to the viewport while preserving aspect, so the // pass does no work on pixels the view will letterbox away. + // + // Fitted against the *framed* size, not the sensor's: a crop changes + // the aspect ratio, and fitting the uncropped shape would letterbox + // to the wrong box and render the crop squashed. let (sw, sh) = self.demosaiced.size(); - let (w, h) = fit(sw, sh, width.max(1), height.max(1)); + let (fw, fh) = self.graph.output_size(sw, sh); + let (w, h) = fit(fw, fh, width.max(1), height.max(1)); let shader = self.graph.compose(); self.adjust @@ -149,11 +154,46 @@ impl DevelopSession { Ok(slint::Image::from_rgba8(buffer)) } - /// The image's natural aspect ratio, for sizing the viewport. + /// The displayed size, for sizing the viewport. + /// + /// The *framed* size, not the sensor's: cropping and quarter turns change + /// the aspect ratio, and a viewport sized to the sensor would letterbox a + /// cropped image against the wrong shape. pub fn source_size(&self) -> (u32, u32) { + let (w, h) = self.demosaiced.size(); + self.graph.output_size(w, h) + } + + /// The sensor's own dimensions, before framing. + /// + /// What a crop overlay needs: its handles are placed against the full + /// frame, since that is what the user is selecting *from*. + pub fn sensor_size(&self) -> (u32, u32) { self.demosaiced.size() } + /// Set the crop rectangle, in fractions of the source. + pub fn set_crop(&mut self, rect: CropRect) { + self.graph.set_crop(rect); + } + + pub fn crop(&self) -> CropRect { + self.graph.crop() + } + + /// Rotate by quarter turns, wrapping. The rotate-left/right buttons. + pub fn rotate_quarters(&mut self, turns: i32) { + self.graph.rotate_quarters(turns); + } + + /// The largest centred crop that, at the current straightening angle, + /// contains no undefined area. What a "straighten and fill" action + /// applies. + pub fn max_inscribed_crop(&self) -> CropRect { + let (w, h) = self.demosaiced.size(); + self.graph.framing().max_inscribed_crop(w, h) + } + /// How many shader pipelines have been compiled. Surfaced so the status /// strip can show that slider movement is not recompiling. pub fn compiled_pipelines(&self) -> usize { @@ -203,10 +243,23 @@ mod tests { #[test] fn every_capability_becomes_exactly_one_row() { // The UI shows what the pipeline offers — no more, and nothing - // dropped. + // dropped. Asserted against the chain rather than a literal count, + // so operations can be added without editing this, and so the test + // actually checks the correspondence rather than restating a number. let graph = EditGraph::default_chain(); - let expected: usize = graph.capabilities().iter().map(|c| c.params.len()).sum(); - assert_eq!(expected, 10, "seven operations, ten parameters"); + let caps = graph.capabilities(); + let expected: usize = caps.iter().map(|c| c.params.len()).sum(); + + assert!(expected > 0, "the chain must expose some parameters"); + // Every (operation, parameter) pair must be reachable as a distinct + // row index; a collision would route two sliders to one parameter. + let mut seen = std::collections::HashSet::new(); + for (oi, cap) in caps.iter().enumerate() { + for (pi, _) in cap.params.iter().enumerate() { + assert!(seen.insert((oi, pi)), "duplicate row index"); + } + } + assert_eq!(seen.len(), expected); } #[test] diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index aed20a7..f73a1c8 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -20,6 +20,7 @@ pub fn resolve(key: &str) -> String { "op.brilliance" => "Brilliance".into(), "op.vibrance" => "Vibrance".into(), "op.saturation" => "Saturation".into(), + "op.framing" => "Crop & Rotate".into(), // Parameters "param.temperature" => "Temperature".into(), @@ -33,6 +34,17 @@ pub fn resolve(key: &str) -> String { "param.vibrance" => "Vibrance".into(), "param.saturation" => "Saturation".into(), + // Framing. "Straighten" rather than "Angle" because that is the task + // the control performs; the number it reports is still degrees. + "param.angle" => "Straighten".into(), + "param.rotation" => "Rotate".into(), + "param.flip_h" => "Flip Horizontal".into(), + "param.flip_v" => "Flip Vertical".into(), + "param.crop_x" => "Crop Left".into(), + "param.crop_y" => "Crop Top".into(), + "param.crop_w" => "Crop Width".into(), + "param.crop_h" => "Crop Height".into(), + other => derive(other), } } diff --git a/ui/dr-ui/src/launch.rs b/ui/dr-ui/src/launch.rs new file mode 100644 index 0000000..f1c35ff --- /dev/null +++ b/ui/dr-ui/src/launch.rs @@ -0,0 +1,384 @@ +//! Launch screen state: connect an account, choose a library, sign out. +//! +//! The state machine lives here, separate from the Slint bindings, so it can +//! be tested without a display server. `dr-ui` owns presentation; what a +//! login *is* belongs to the connector. + +use dr_sync_nextcloud::{Session, SessionStore}; +use dr_types::{Format, FormatFilter}; + +/// What the launch screen is currently doing. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum LaunchState { + /// No account configured; waiting for a server address. + SignedOut, + /// A login flow is open and the user must approve it in a browser. + /// + /// Carries the URL so the screen can show and copy it — the app never + /// handles the password itself (FR-NC-1). + AwaitingApproval { login_url: String }, + /// An account is configured. + SignedIn { session: Session }, + /// Working; the reason is shown so a pause is never unexplained. + /// + /// Carries the session where there is one, so a failure mid-work returns + /// to the signed-in screen rather than signing the user out. + Busy { + message: String, + session: Option>, + }, +} + +/// TRACES: FR-NC-1 | FR-NC-4 | M-1 | M-3 | M-4 +/// Everything the launch screen renders from. +#[derive(Debug, Clone)] +pub struct LaunchModel { + pub state: LaunchState, + /// Last-used server, prefilled so a returning user need not retype it. + pub server_url: String, + pub error: Option, + pub status: Option, + /// False where no secrets daemon exists (FR-NC-2). The screen must say so + /// up front rather than letting the user discover it next launch. + pub can_remember: bool, + /// Which formats to scan for, in `Format::ALL` order. + pub formats: Vec<(Format, bool)>, +} + +impl Default for LaunchModel { + fn default() -> Self { + Self { + state: LaunchState::SignedOut, + server_url: String::new(), + error: None, + status: None, + can_remember: true, + formats: Format::ALL.iter().map(|f| (*f, f.is_raw())).collect(), + } + } +} + +impl LaunchModel { + /// Build from stored sessions, resuming the last account if there is one. + pub fn from_store(store: &SessionStore) -> Self { + let can_remember = store.can_remember(); + + match store.current() { + Some(session) => { + let filter = session.format_filter(); + Self { + server_url: session.server.clone(), + formats: Format::ALL + .iter() + .map(|f| (*f, filter.allows(*f))) + .collect(), + state: LaunchState::SignedIn { session }, + can_remember, + ..Default::default() + } + } + None => Self { + can_remember, + ..Default::default() + }, + } + } + + pub fn is_signed_in(&self) -> bool { + matches!(self.state, LaunchState::SignedIn { .. }) + } + + pub fn is_busy(&self) -> bool { + matches!(self.state, LaunchState::Busy { .. }) + } + + pub fn session(&self) -> Option<&Session> { + match &self.state { + LaunchState::SignedIn { session } => Some(session), + // A session survives a busy period; a scan failure must not log + // the user out. + LaunchState::Busy { + session: Some(s), .. + } => Some(s), + _ => None, + } + } + + /// The account line, e.g. "duncan on cloud.example". + pub fn account_label(&self) -> String { + self.session().map(|s| s.describe()).unwrap_or_default() + } + + /// The chosen library folder, empty until one is picked. + pub fn library_root(&self) -> String { + self.session().map(|s| s.root.clone()).unwrap_or_default() + } + + /// The login URL while approval is pending. + pub fn login_url(&self) -> String { + match &self.state { + LaunchState::AwaitingApproval { login_url } => login_url.clone(), + _ => String::new(), + } + } + + /// Whether "Open library" should be clickable. + /// + /// Requires a signed-in account *and* a chosen folder: opening without one + /// would scan the whole account, which on a real library is thousands of + /// directories the user did not ask for. + pub fn can_open_library(&self) -> bool { + self.is_signed_in() && !self.library_root().is_empty() + } + + pub fn format_filter(&self) -> FormatFilter { + FormatFilter::from_formats(self.formats.iter().filter(|(_, on)| *on).map(|(f, _)| *f)) + } + + /// Toggle one format tick-box. + pub fn set_format(&mut self, index: usize, enabled: bool) { + if let Some(entry) = self.formats.get_mut(index) { + entry.1 = enabled; + } + } + + // --- transitions --------------------------------------------------- + + pub fn begin_sign_in(&mut self, server: impl Into) { + self.server_url = normalise_server(&server.into()); + self.error = None; + self.state = LaunchState::Busy { + message: "Contacting server…".into(), + session: None, + }; + } + + pub fn await_approval(&mut self, login_url: impl Into) { + self.status = Some("Approve the sign-in in your browser.".into()); + self.state = LaunchState::AwaitingApproval { + login_url: login_url.into(), + }; + } + + pub fn signed_in(&mut self, session: Session) { + self.error = None; + self.status = None; + self.server_url = session.server.clone(); + let filter = session.format_filter(); + // Adopt the session's stored selection, so a returning user sees the + // tick-boxes they left. + if !session.formats.is_empty() { + self.formats = Format::ALL + .iter() + .map(|f| (*f, filter.allows(*f))) + .collect(); + } + self.state = LaunchState::SignedIn { session }; + } + + pub fn signed_out(&mut self) { + self.error = None; + self.status = None; + self.state = LaunchState::SignedOut; + } + + pub fn fail(&mut self, message: impl Into) { + self.status = None; + self.error = Some(message.into()); + // Return to whichever resting state makes sense, so a failure never + // strands the screen in Busy with no way forward. + self.state = match self.session() { + Some(s) => LaunchState::SignedIn { session: s.clone() }, + None => LaunchState::SignedOut, + }; + } + + pub fn busy(&mut self, message: impl Into) { + self.error = None; + let session = self.session().cloned().map(Box::new); + self.state = LaunchState::Busy { + message: message.into(), + session, + }; + } +} + +/// Normalise a server address typed by hand. +/// +/// Users type `cloud.example.com`, not a URL. Assume HTTPS rather than +/// failing, and never silently accept plain HTTP — NFR-SEC-3 requires TLS, +/// and an unencrypted default would be a security decision made on the user's +/// behalf without telling them. +pub fn normalise_server(input: &str) -> String { + let s = input.trim().trim_end_matches('/'); + if s.is_empty() { + return String::new(); + } + if s.starts_with("https://") { + s.to_string() + } else if let Some(rest) = s.strip_prefix("http://") { + // Upgrade rather than accept. If the server genuinely has no TLS the + // connection fails loudly, which is the correct outcome. + format!("https://{rest}") + } else { + format!("https://{s}") + } +} + +#[cfg(test)] +mod tests { + use super::*; + use dr_plat::EphemeralSecretStore; + use dr_sync_nextcloud::AppCredentials; + + fn creds() -> AppCredentials { + AppCredentials { + server: "https://cloud.example".into(), + login_name: "duncan".into(), + app_password: "token".into(), + } + } + + fn session_with_root(root: &str) -> Session { + let mut s = Session::new(&creds(), "duncan"); + s.root = root.into(); + s + } + + #[test] + fn a_fresh_model_is_signed_out_with_raw_preselected() { + let m = LaunchModel::default(); + assert!(!m.is_signed_in()); + // RAW ticked, JPEG not: a RAW editor's sensible default. + let f = m.format_filter(); + assert!(f.allows(Format::Cr2)); + assert!(!f.allows(Format::Jpeg)); + } + + #[test] + fn opening_a_library_needs_both_an_account_and_a_folder() { + let mut m = LaunchModel::default(); + assert!(!m.can_open_library(), "signed out"); + + m.signed_in(session_with_root("")); + assert!( + !m.can_open_library(), + "no folder — would scan the whole account" + ); + + m.signed_in(session_with_root("PhotosRaw")); + assert!(m.can_open_library()); + } + + #[test] + fn a_failure_never_strands_the_screen_in_busy() { + let mut m = LaunchModel::default(); + m.begin_sign_in("cloud.example"); + assert!(m.is_busy()); + + m.fail("server unreachable"); + assert!(!m.is_busy(), "must return to a resting state"); + assert_eq!(m.state, LaunchState::SignedOut); + assert!(m.error.is_some()); + } + + #[test] + fn a_failure_while_signed_in_returns_to_signed_in() { + let mut m = LaunchModel::default(); + m.signed_in(session_with_root("PhotosRaw")); + m.busy("Scanning…"); + m.fail("scan failed"); + + assert!(m.is_signed_in(), "a failed scan must not sign the user out"); + assert!(m.error.is_some()); + } + + #[test] + fn the_login_url_is_exposed_only_while_pending() { + let mut m = LaunchModel::default(); + assert_eq!(m.login_url(), ""); + + m.await_approval("https://cloud.example/login/v2/flow/abc"); + assert!(m.login_url().contains("/flow/abc")); + + m.signed_in(session_with_root("")); + assert_eq!(m.login_url(), "", "must not linger after sign-in"); + } + + #[test] + fn server_addresses_are_normalised_to_https() { + assert_eq!(normalise_server("cloud.example"), "https://cloud.example"); + assert_eq!( + normalise_server("https://cloud.example/"), + "https://cloud.example" + ); + assert_eq!( + normalise_server(" cloud.example "), + "https://cloud.example" + ); + assert_eq!(normalise_server(""), ""); + } + + #[test] + fn plain_http_is_upgraded_rather_than_accepted() { + // NFR-SEC-3: TLS is required. Failing loudly beats silently sending a + // credential in the clear. + assert_eq!( + normalise_server("http://cloud.example"), + "https://cloud.example" + ); + } + + #[test] + fn format_toggles_apply_and_out_of_range_is_ignored() { + let mut m = LaunchModel::default(); + let jpeg = Format::ALL.iter().position(|f| *f == Format::Jpeg).unwrap(); + + m.set_format(jpeg, true); + assert!(m.format_filter().allows(Format::Jpeg)); + + // Must not panic on a stale index from the UI. + m.set_format(9999, true); + } + + #[test] + fn a_stored_session_is_resumed_with_its_format_selection() { + let dir = std::env::temp_dir().join("darkroom-launch-test"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + let store = SessionStore::open_at( + dir.join("sessions.json"), + Box::new(EphemeralSecretStore::new()), + ); + let mut s = session_with_root("PhotosRaw"); + s.set_format_filter(&FormatFilter::from_formats([Format::Cr2])); + store.save(&s, &creds()).unwrap(); + + let m = LaunchModel::from_store(&store); + assert!(m.is_signed_in()); + assert_eq!(m.library_root(), "PhotosRaw"); + assert!(m.format_filter().allows(Format::Cr2)); + assert!(!m.format_filter().allows(Format::Dng)); + assert_eq!(m.server_url, "https://cloud.example"); + } + + #[test] + fn no_stored_session_yields_a_signed_out_model() { + let dir = std::env::temp_dir().join("darkroom-launch-empty"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + let store = SessionStore::open_at( + dir.join("sessions.json"), + Box::new(EphemeralSecretStore::new()), + ); + let m = LaunchModel::from_store(&store); + assert!(!m.is_signed_in()); + } + + #[test] + fn account_label_is_empty_when_signed_out() { + assert_eq!(LaunchModel::default().account_label(), ""); + } +} diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index d853e4a..de1da00 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -26,6 +26,8 @@ use dr_decode::{Metadata, PreviewSize}; pub use develop::DevelopSession; +pub mod launch; + slint::include_modules!(); /// TRACES: FR-DSP-1 | NFR-RES-1 diff --git a/ui/dr-ui/ui/launch.slint b/ui/dr-ui/ui/launch.slint new file mode 100644 index 0000000..b61f701 --- /dev/null +++ b/ui/dr-ui/ui/launch.slint @@ -0,0 +1,349 @@ +import { Theme } from "theme.slint"; + +// Launch screen: connect an account, or resume a saved one. +// +// Deliberately separate from AppWindow. It is the first thing a user sees +// with no library configured, and the place they return to in order to sign +// out or switch account (FR-NC-1, FR-NC-4). + +// One tick-box in the format selection. +component FormatCheck inherits Rectangle { + in property label; + in-out property checked; + callback toggled(bool); + + // FR-UI-3: 44pt minimum hit target under touch. The drawn row is + // shorter, so the touch area extends beyond the visible bounds. + height: 32px; + + touch := TouchArea { + height: max(parent.height, Theme.touch-target); + y: (parent.height - self.height) / 2; + clicked => { + root.checked = !root.checked; + root.toggled(root.checked); + } + } + + HorizontalLayout { + spacing: Theme.gap; + alignment: start; + + Rectangle { + width: 18px; + height: 18px; + y: (parent.height - self.height) / 2; + border-radius: 3px; + border-width: 1px; + border-color: root.checked ? Theme.accent : Theme.rule; + background: root.checked ? Theme.accent : transparent; + + Text { + text: "✓"; + color: #fff; + font-size: 12px; + visible: root.checked; + horizontal-alignment: center; + vertical-alignment: center; + width: 100%; + height: 100%; + } + } + + Text { + text: root.label; + color: touch.has-hover ? Theme.ink : Theme.ink-dim; + font-size: Theme.text; + vertical-alignment: center; + } + } +} + +component Button inherits Rectangle { + in property label; + in property primary: false; + in property enabled: true; + callback clicked(); + + height: Theme.touch-target; + border-radius: 4px; + background: root.primary + ? (touch.has-hover && root.enabled ? Theme.accent-dim : Theme.accent) + : (touch.has-hover && root.enabled ? Theme.surface-raised : transparent); + border-width: root.primary ? 0px : 1px; + border-color: Theme.rule; + opacity: root.enabled ? 1.0 : 0.45; + + touch := TouchArea { + enabled: root.enabled; + clicked => { root.clicked(); } + } + + Text { + text: root.label; + color: root.primary ? #fff : Theme.ink; + font-size: Theme.text; + font-weight: 600; + horizontal-alignment: center; + vertical-alignment: center; + width: 100%; + height: 100%; + } +} + +export component LaunchScreen inherits Rectangle { + // --- state in --- + in property signed-in: false; + in property account: ""; + in property library-root: ""; + in property server-url: ""; + in property busy: false; + in property status: ""; + in property error: ""; + /// The URL to approve in a browser, shown while a login is pending. + in property login-url: ""; + /// False where no secrets daemon exists — the user must be told sign-in + /// will not persist rather than discovering it next launch. + in property can-remember: true; + + in-out property <[string]> format-labels; + in-out property <[bool]> format-checked; + + // --- events out --- + callback sign-in(string); + callback sign-out(); + callback choose-folder(); + callback open-library(); + callback format-toggled(int, bool); + callback copy-login-url(); + + background: Theme.ground; + + VerticalLayout { + alignment: center; + padding: Theme.gap-lg; + + Rectangle { + max-width: 460px; + horizontal-stretch: 0; + + VerticalLayout { + spacing: Theme.gap-lg; + + // --- masthead --- + VerticalLayout { + spacing: Theme.gap-sm; + Text { + text: "DarkRoom"; + color: Theme.ink; + font-size: Theme.text-xl; + font-weight: 800; + letter-spacing: -0.5px; + } + Text { + text: root.signed-in + ? "Connected" + : "Connect a Nextcloud account to begin"; + color: Theme.ink-dim; + font-size: Theme.text; + } + } + + Rectangle { height: 1px; background: Theme.rule; } + + // --- signed out: server entry --- + if !root.signed-in && root.login-url == "": VerticalLayout { + spacing: Theme.gap; + + Text { + text: "SERVER"; + color: Theme.accent; + font-size: Theme.text-sm; + font-weight: 700; + letter-spacing: 1.2px; + } + + Rectangle { + height: Theme.touch-target; + border-radius: 4px; + border-width: 1px; + border-color: server-input.has-focus ? Theme.accent : Theme.rule; + background: Theme.surface; + + server-input := TextInput { + text: root.server-url; + color: Theme.ink; + font-size: Theme.text; + vertical-alignment: center; + width: parent.width - 2 * Theme.gap; + x: Theme.gap; + height: 100%; + single-line: true; + accepted => { root.sign-in(self.text); } + } + + // Placeholder, since TextInput has none of its own. + Text { + text: "https://cloud.example.com"; + color: Theme.ink-faint; + font-size: Theme.text; + vertical-alignment: center; + x: Theme.gap; + height: 100%; + visible: server-input.text == ""; + } + } + + if !root.can-remember: Text { + text: "No system keyring found — you will need to sign in each time."; + color: Theme.warn-ink; + font-size: Theme.text-sm; + wrap: word-wrap; + } + + Button { + label: root.busy ? "Connecting…" : "Sign in"; + primary: true; + enabled: !root.busy && server-input.text != ""; + clicked => { root.sign-in(server-input.text); } + } + + Text { + text: "Sign-in happens in your browser. DarkRoom never sees your password."; + color: Theme.ink-faint; + font-size: Theme.text-sm; + wrap: word-wrap; + } + } + + // --- login pending: the browser step --- + if root.login-url != "": VerticalLayout { + spacing: Theme.gap; + + Text { + text: "APPROVE IN YOUR BROWSER"; + color: Theme.accent; + font-size: Theme.text-sm; + font-weight: 700; + letter-spacing: 1.2px; + } + + Rectangle { + background: Theme.surface; + border-radius: 4px; + border-width: 1px; + border-color: Theme.rule; + + VerticalLayout { + padding: Theme.gap; + Text { + text: root.login-url; + color: Theme.ink-dim; + font-size: Theme.text-sm; + wrap: char-wrap; + } + } + } + + Button { + label: "Copy link"; + clicked => { root.copy-login-url(); } + } + + Text { + text: "Waiting for approval…"; + color: Theme.ink-faint; + font-size: Theme.text-sm; + } + } + + // --- signed in --- + if root.signed-in: VerticalLayout { + spacing: Theme.gap; + + Text { + text: "ACCOUNT"; + color: Theme.accent; + font-size: Theme.text-sm; + font-weight: 700; + letter-spacing: 1.2px; + } + Text { + text: root.account; + color: Theme.ink; + font-size: Theme.text; + } + + Rectangle { height: Theme.gap-sm; } + + Text { + text: "LIBRARY FOLDER"; + color: Theme.accent; + font-size: Theme.text-sm; + font-weight: 700; + letter-spacing: 1.2px; + } + HorizontalLayout { + spacing: Theme.gap; + Text { + text: root.library-root == "" ? "(not chosen)" : root.library-root; + color: root.library-root == "" ? Theme.ink-faint : Theme.ink; + font-size: Theme.text; + vertical-alignment: center; + horizontal-stretch: 1; + overflow: elide; + } + Button { + label: "Choose…"; + width: 110px; + clicked => { root.choose-folder(); } + } + } + + Rectangle { height: Theme.gap-sm; } + + Text { + text: "SCAN FOR"; + color: Theme.accent; + font-size: Theme.text-sm; + font-weight: 700; + letter-spacing: 1.2px; + } + + for label[i] in root.format-labels: FormatCheck { + label: label; + checked: root.format-checked[i]; + toggled(on) => { root.format-toggled(i, on); } + } + + Rectangle { height: Theme.gap; } + + Button { + label: root.busy ? "Scanning…" : "Open library"; + primary: true; + enabled: !root.busy && root.library-root != ""; + clicked => { root.open-library(); } + } + Button { + label: "Sign out"; + clicked => { root.sign-out(); } + } + } + + // --- feedback --- + if root.status != "": Text { + text: root.status; + color: Theme.ink-dim; + font-size: Theme.text-sm; + wrap: word-wrap; + } + if root.error != "": Text { + text: root.error; + color: Theme.accent; + font-size: Theme.text-sm; + wrap: word-wrap; + } + } + } + } +} diff --git a/ui/dr-ui/ui/theme.slint b/ui/dr-ui/ui/theme.slint index cb1012e..e965d2e 100644 --- a/ui/dr-ui/ui/theme.slint +++ b/ui/dr-ui/ui/theme.slint @@ -15,6 +15,9 @@ export global Theme { out property accent: #D9543C; out property accent-dim: #8F2E1E; + // Semantic, distinct from the accent: a caution is not an action. + out property warn-ink: #C99A4A; + out property gap-sm: 6px; out property gap: 12px; out property gap-lg: 20px;