From 02b66ddfcfd0ae122f71d84c48b9e72da0cf1517 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 9 Aug 2026 20:36:23 +0200 Subject: [PATCH] Document trash, collections, thumbnails, and the UI direction Specs for the work that follows: soft delete via a MOVE that preserves the remote id, the collection tree and smart collections, the thumbnail store, and the derived-state folder. Adds two design documents. ui-refinement.md names the structural gaps between the v0.1 UI and something that feels like a photo editor. view-composition.md proposes a view controller for the display layer, against the 500-line run() that has become one by accretion. Co-Authored-By: Claude Opus 5 --- docs/architecture.md | 85 +++++++ docs/catalog.md | 232 +++++++++++++++++- docs/requirements.md | 181 +++++++++++++- docs/traceability.md | 106 +++++---- docs/ui-refinement.md | 496 +++++++++++++++++++++++++++++++++++++++ docs/view-composition.md | 296 +++++++++++++++++++++++ 6 files changed, 1335 insertions(+), 61 deletions(-) create mode 100644 docs/ui-refinement.md create mode 100644 docs/view-composition.md diff --git a/docs/architecture.md b/docs/architecture.md index 2a126b2..bb76d44 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -269,6 +269,49 @@ UI; the core is unaware presentation varies. Adding an operation therefore requires no UI change (FR-DEV-3c) unless it needs a new `WidgetKind`. +### 4.3a The presentation contract + +The division of labour, stated once so neither side drifts: + +> **The core declares capabilities and hints. The frontend composes.** + +The core says what a parameter *is* (`ParamKind`), what an operation would +*like* (`Presentation`), and what a widget inherently *demands* +(`WidgetDemand`). It never says what is drawn, where it sits, how wide it is, +or whether it is currently visible. Those are compositional decisions and they +belong to whoever knows the window, the input modality and the platform — +which is never the core. + +**Hints are plural and ordered.** `Presentation.widgets` is a list of +`WidgetKind` in descending preference. The frontend walks it and takes the +first it both implements and can afford. Falling off the end is not an error: +every parameter remains an individually addressable scalar, so plain sliders +are always the final fallback and the edit still works — merely more tediously. +That fallback is why curve points are scalars rather than an opaque blob. + +**Demands describe the control, not the screen.** A hint may carry what the +widget inherently needs — two-dimensional direct manipulation, precision +pointing, a minimum useful number of simultaneous values. It must never carry +pixels, breakpoints, DPI, or a platform name. `min_width: 240px` in a +descriptor is the core making a layout decision, and a core that reasons about +pixels will eventually be wrong about a display it never saw. The frontend maps +demands onto its own thresholds; those thresholds live in `dr-ui` and may +differ per platform without the core knowing. + +**Presentation state is derived, never transmitted.** Whether a section is +collapsed, whether a group contains a modified value, where a heading falls — +all of it is computed frontend-side from capabilities plus current values. The +core exposing a `group_modified` flag or a `starts_group` marker would be the +core deciding the panel has groups at all, which is a composition decision. The +frontend has the descriptors and the values; that is sufficient to derive any +of it. + +**The test.** A second frontend — a CLI, a test harness, a differently-shaped +mobile UI — must be able to consume the same capability output and compose +something entirely different, without the core changing. If a core change +would be needed to lay something out differently, the boundary has been +crossed. + --- ## 5. GPU architecture @@ -444,10 +487,51 @@ mechanism rather than a last resort. | Version | UUID | everything | | Remote file | `oc:fileid` | server-side rename/move | | Cache entry | `(version, kind, resolution, graph_hash)` | — | +| Person | UUID | rename, merge, re-index | Content-hash identity is what makes FR-CAT-9's reconnection work: a moved file is recognised rather than re-imported, and a server-side move is not a re-download of 80 MB. +A person's identity is their UUID, never their name: renaming "Mum" to "Sarah" must not create a +second person, and two devices that name the same face independently must be mergeable rather than +duplicated (FR-CULL-12). Same reasoning as collections, same mechanism. + +### 6.4 People and faces + +Specified by FR-CULL-8 … FR-CULL-12 and NFR-SEC-5. Sketched here because the split across the trust +boundary is an architectural decision, not a schema detail. + +**Three entities.** A `face` is a detection: an image, a box, landmarks, a detector confidence, and +an embedding. A `person` is a UUID and a name. `face_person` links them, carrying a calibrated +probability and — critically — a **confirmed** flag separating what the user asserted from what the +system guessed. + +That flag is the whole design. Suggestions are derived data and may be recomputed at will; a +confirmation is a user judgement and is never overwritten by a later inference pass. Conflating them +would mean a model upgrade silently rewriting the user's own labelling, which is the kind of loss +this architecture exists to prevent. + +**Where each part lives, and why they differ:** + +| Data | Home | Rebuildable | Rationale | +|---|---|---|---| +| Embeddings, boxes, landmarks | Catalog only | Yes — re-index | Expensive but reproducible. ARCH §6.12: derived data belongs in the disposable index. | +| Cluster assignments, suggestions | Catalog only | Yes | Inference output; changes whenever the model or calibration does. | +| Confirmed person name on an image | **Sidecar** | **No** | A human judgement, same class as a rating or keyword (FR-CAT-8). Must survive catalog deletion. | +| Person entity (UUID, name) | Catalog, synced | No | The merge identity; travels with collections under FR-CAT-7's rules. | + +The asymmetry is deliberate: deleting the catalog costs an afternoon of re-indexing and loses nothing +the user typed. That is the same bargain §6.12 already makes everywhere else. + +**Detection runs on proxies, not originals.** FR-CULL-8 pins this to the FR-CULL-2 preview ladder, so +face indexing consumes the same artefacts the grid already built rather than forcing RAW decodes. The +consequence for §5's GPU budget is that face inference competes with thumbnailing, not with +rendering, and NFR-ARCH-2's priority classes already express that. + +**Embeddings never enter the diagnostics or crash paths** (NFR-SEC-5). This is a structural +exclusion, not a redaction rule: NFR-OPS-1's bundle is assembled from an allowlist, so a new table +does not silently become uploadable by existing. + --- ## 7. Concurrency @@ -1000,6 +1084,7 @@ Full rationale in [requirements.md §8](requirements.md). Summary: | D10 | Single adaptive interface | Decided | | D11 | Product positioning | Decided | | D12 | Scope versus pace | **Open** | +| D13 | Face inference runtime and model licensing | **Open** | --- diff --git a/docs/catalog.md b/docs/catalog.md index 4b7cd91..5c5deb5 100644 --- a/docs/catalog.md +++ b/docs/catalog.md @@ -372,12 +372,19 @@ pub enum Selector { IsoRange { min: u32, max: u32 }, // ⊕ Availability(Availability), // ⊕ "what can I edit right now" Text(String), // ⊕ filename/keyword substring + Person { id: PersonId, include_suggested: bool }, // ⊕ §10 (FR-CULL-11) All_(Vec), Any(Vec), Not(Box), } ``` +`Person` carries `include_suggested` rather than defaulting silently. A saved collection built from +confirmed faces must not quietly change membership because a later indexing pass guessed at another +face; the user chose "photos of Anna", not "photos the model currently believes contain Anna". The +default is `false`, and the interactive filter offers the looser form explicitly as a way to *find* +faces to confirm. + `Availability` as a selector earns its place: on a tablet the most useful filter is often "what do I actually have here", and it is also the natural thing to *pin* — "keep everything I've flagged that isn't already local". @@ -402,6 +409,7 @@ pub enum JobKind { ContentHash, // on demand only FetchPreview, // remote range-extract (FR-NC-3) FetchOriginal, // pinned or explicitly requested + DetectFaces, // §10, on the proxy tier (FR-CULL-8) } ``` @@ -482,16 +490,117 @@ Server previews (`/core/preview`) are tried only where PROPFIND reported `nc:has verified stock Nextcloud ships no RAW preview provider, so for RAW this is nearly always absent — it is an opportunistic saving, never the mechanism. -### 7.3 Storage +### 7.3 Storage: sharded, shared, synced -Thumbnails are content-addressed by `(content_hash | source_ref, size_class)` and stored as files -under the platform cache directory, with the `cache` table holding the index. Files, not BLOBs: -SQLite handles small blobs well but a 50k-image thumbnail cache is gigabytes, and mixing it into the -catalog would bloat the file the app must open in under two seconds. +Decided 2026-08-09, implemented in `dr-thumbs`. Thumbnails live in **sharded SQLite databases that +sync to Nextcloud**, so a second device gets a full grid without re-fetching a byte of RAW. -Two size classes at v1 — grid (256px) and filmstrip/loupe (1024px) — both long-edge, both JPEG. The -cache is LRU-capped per NFR-RES-4, and thumbnails evict before proxies and long after sidecars, -which never evict at all (FR-NC-6b). +```text +thumbs/ + index.sqlite fileid → shard, plus size accounting + shard-0000.sqlite ≤ 25 MB, sealed + shard-0001.sqlite ≤ 25 MB, active +``` + +**Why a thumbnail is worth syncing when the catalog mostly is not.** It is expensive to produce — a +range fetch plus a decode, per image — and byte-identical for every client looking at the same file. +This does not make it authoritative: losing the store costs regeneration and nothing else, so §6.12 +is untouched. + +**Why shards, and why small.** The 25 MB cap is about *sync granularity*, not SQLite's limits. One +growing database means every client re-downloads all of it whenever a single thumbnail is added. +With sequential fill only the newest shard is ever dirty, so an up-to-date client transfers one small +file. Sealed shards are immutable, which makes them safe to cache forever and cheap to skip. + +At ~20 KB per 256px JPEG a shard holds roughly 1,200 thumbnails, so the 17,185-image reference +library lands in ~14 shards. + +**Keyed on `oc:fileid`** — stable across server-side rename and move (FR-NC-5), and already in hand +from PROPFIND. Accepted consequence: shards are account-scoped, so the same photograph on two +servers is thumbnailed twice. + +**Stored as JPEG, not raw pixels.** A 256×170 RGBA buffer is ~174 KB against ~15 KB encoded. Since +shards sync, that 11× is transfer cost paid by every client, not just disk. + +Three invariants, each tested: + +| Invariant | Why it matters | +|---|---| +| A sealed shard never reopens | Reopening one forces every client that holds it to re-download | +| Re-storing an existing id updates in place, never migrates | Migrating would rewrite a sealed shard | +| Merging another client's shard is insert-only and idempotent | Both copies derive from the same bytes by the same code, so neither is better; preferring ours avoids dirtying a shard others have synced | + +**Not yet built:** the transfer itself. `ThumbStore::shards()` reports which shards are sealed — +the input a sync pass needs — and `merge_shard` adopts a downloaded one, but nothing uploads or +downloads them yet. Until that lands the store is a local cache that happens to have the right shape. + +Two size classes remain planned — grid (256px) and filmstrip/loupe (1024px). Only the grid class is +implemented. The cache is LRU-capped per NFR-RES-4, and thumbnails evict before proxies and long +after sidecars, which never evict at all (FR-NC-6b). + +--- + +## 7a. Editing collections + +Decided 2026-08-09. The schema for collections landed with §2 and the cross-device merge rules with +§8; this is the layer between them — the operations a user actually performs, in +`dr_catalog::collections`. + +### 7a.1 Hierarchy and membership are independent + +Two structures, deliberately not entangled: + +| | Mechanism | Meaning | +|---|---|---| +| Hierarchy | `collections.parent_id` | A collection inside a collection (Lightroom's "collection set"). A parent is an ordinary collection, not a separate kind, so a set can hold images of its own | +| Membership | `collection_members` | An image is in as many collections as the user likes. Nothing moves on disk; no collection owns an image | + +**Adding an image to a child does not write a row for the parent.** A parent's contents are the union +of its own members and its descendants', computed on read. Materialising it instead would make one +add touch every ancestor, and a reparent rewrite membership — both of which §8's row-level merge +would then have to reconcile. The read path pays a bounded tree walk instead, which at sidebar scale +is nothing. + +The consequence the UI depends on: dragging images onto a collection is **additive**. It does not +remove them from anywhere, which is why the gesture's default action is `copy` and not `move`. + +### 7a.2 Rules that exist to prevent silent damage + +| Rule | Why | +|---|---| +| Every mutation bumps `revision` | §8.4 resolves conflicts by revision. An edit that updates `modified` alone is invisible to the merge, so the *other* device silently wins and the user's work vanishes | +| A no-op add does **not** bump it | Otherwise an idle device that re-dropped the same images outranks one that did real work | +| Deleting a parent **promotes** its children | The schema's `ON DELETE CASCADE` would take the whole subtree. Losing a nested collection because its container was tidied away is not recoverable | +| Deletion leaves a tombstone | Without it, merging with a device that still holds the collection resurrects it (§8.4) | +| Cycles are refused at the write | Both kinds — parenting under a descendant, and a smart collection whose selector reaches itself. A cycle is unbounded recursion in the tree walk, so it must not be *representable*, not merely handled when drawn | +| Tree walks are depth-guarded anyway | A merge can deliver a row this device never validated. The read path must terminate, so it truncates and logs rather than hanging the UI thread | +| A drop onto a smart collection is refused | Its membership *is* its selector; member rows would be a second source of truth that nothing reads | +| Deep counts are `count(DISTINCT image_id)` | An image in both a parent and a child is one photograph. A count that disagrees with the number of cells drawn makes both untrustworthy | + +`collections.uuid` is generated from the OS CSPRNG. A collision fuses two unrelated collections at +the next merge, so the fallback path (used only if `/dev/urandom` cannot be read) logs loudly rather +than degrading identity quality in silence. + +### 7a.3 Drag and drop is Slint's, not ours + +The first implementation hand-rolled the gesture on `TouchArea` — tracking the press, measuring +travel to distinguish a click from a drag, and deciding the drop target from the last row hovered. +**It did not work**, for a reason worth recording: an interactive `Flickable` claims any drag +beginning inside it for scrolling and *cancels* the child `TouchArea`'s press, so the gesture could +never leave the grid. It also had a correctness hole — a tree rebuilt mid-drag could redirect the +drop, since a captured pointer is invisible to every other element. + +Slint 1.17's `DragArea`/`DropArea` own all of it: capture, the click-versus-drag threshold, +arbitration against the `Flickable`, the image under the cursor, and hit-testing the release. What +remains in `collections_ui` is only what Slint cannot know — the payload (which images, read from the +selection when the drag starts) and the **spring**: a dwell timer that opens a collapsed collection +so a nested child can be reached mid-drag, and closes again whatever the drag merely passed over. + +One hazard survives the change and is easy to reintroduce. Every consequence of a drop — rebuilding +the tree, refreshing the badges, rereading the grid — *replaces a Slint model*, and doing that inside +the `dropped` handler destroys the elements Slint is still using to deliver that event. So the drop +records its target and `drag-finished` acts on it. This is the same hazard `sync_rows` in `lib.rs` +documents for the adjust panel, and it presents as a control that works once and then goes dead. --- @@ -578,7 +687,104 @@ because if the SQLite path proves troublesome, this is the fallback with a known --- -## 9. Requirements touched +## 10. People and faces + +Specified by FR-CULL-8 … FR-CULL-12, NFR-SEC-5, [architecture.md §6.4](architecture.md). Gated on +spike S14 and decision D13 — the runtime and the model licences are unresolved, so this is the shape +of the subsystem, not a build order. + +### 10.1 Schema (a v5 migration) + +```sql +CREATE TABLE people ( + id INTEGER PRIMARY KEY, + uuid TEXT NOT NULL UNIQUE, -- merge identity, not the name (ARCH §6.3) + name TEXT NOT NULL, + -- Tombstone-by-redirect. A merged person must outlive its merge, or a + -- device that still has it resurrects it — same hazard collections have. + merged_into INTEGER REFERENCES people(id) ON DELETE SET NULL, + created INTEGER NOT NULL, + revision INTEGER NOT NULL DEFAULT 1, + modified INTEGER NOT NULL +); + +CREATE TABLE faces ( + id INTEGER PRIMARY KEY, + image_id INTEGER NOT NULL REFERENCES images(id) ON DELETE CASCADE, + -- Normalised to the image's long edge, so a face survives the proxy it was + -- found on being regenerated at another resolution. + x REAL NOT NULL, y REAL NOT NULL, w REAL NOT NULL, h REAL NOT NULL, + landmarks BLOB, -- 5 × (x, y) f32, the alignment input + detector_confidence REAL NOT NULL, + embedding BLOB NOT NULL, -- 512 × f16, L2-normalised + -- Which model produced this. An embedding is only comparable to others + -- from the same model; mixing them silently yields nonsense similarities. + model_id TEXT NOT NULL, + detected_at INTEGER NOT NULL +); +CREATE INDEX faces_image ON faces(image_id); + +CREATE TABLE face_person ( + face_id INTEGER PRIMARY KEY REFERENCES faces(id) ON DELETE CASCADE, + person_id INTEGER NOT NULL REFERENCES people(id) ON DELETE CASCADE, + -- Calibrated P(this face is this person), never a raw cosine (FR-CULL-9). + probability REAL NOT NULL, + -- The user said so. Never overwritten by a later inference pass. + confirmed INTEGER NOT NULL DEFAULT 0 +); +CREATE INDEX face_person_person ON face_person(person_id, confirmed); +``` + +Three things in that schema are load-bearing: + +**`model_id` on every face.** Embeddings from different models are not comparable — this is the one +mistake that produces plausible-looking garbage rather than an error. Storing the model with the +embedding means a model change is detectable and re-indexable, instead of quietly poisoning every +similarity in the library. + +**Normalised bounding boxes.** Detection runs on whichever proxy exists (FR-CULL-8). Storing pixel +coordinates would bind a face to a resolution that the cache is entitled to evict and regenerate +differently. + +**`confirmed` as a column, not a probability of 1.0.** A confirmation is a different kind of fact +from a confident guess, and collapsing them loses the ability to recompute suggestions without +touching user data. + +### 10.2 Why clustering is not a job kind + +Detection is per-image and parallel, so it is a job (`DetectFaces`, coalesced per image like any +other). Clustering is a *whole-library* operation over the embeddings detection produced — it has no +natural `subject_id`, and running it per-image would rebuild the world on every photograph. + +It therefore runs as a debounced library-level pass, triggered when detection has been idle and the +face count has moved materially since the last clustering. The same reasoning as sidecar writes: the +work is cheap to defer, expensive to repeat, and nobody is waiting on it. + +### 10.3 The calibration lives with the library + +FR-CULL-9 requires similarity to be a calibrated probability, fitted from this library's own faces. +That fit is a property of the catalog and its model, so it is stored alongside — a small table +holding the fit parameters, its validity flag, and a hash of the face set it was derived from, so a +materially changed library recomputes rather than trusting a stale fit. + +When the fit is not valid — a library with too few faces to have positive pairs — the UI says the +confidence is unavailable. It does not fall back to an untuned default dressed up as a measurement. + +### 10.4 What this does not settle + +- **Which model, and which runtime.** D13. Everything above holds regardless of the answer, which is + why it is specified in terms of "a 512-d embedding from a stated model" rather than a named one. +- **The clustering algorithm.** Density-based over the calibrated distance is the obvious starting + point, but the parameters are an S14 question, not a design-time one. +- **Whether embeddings sync.** NFR-SEC-5 permits it, opt-in. The shard mechanism in §7.3 is the + obvious carrier if they do, but nothing here depends on that decision. +- **Faces in trashed images.** FR-CAT-15's trash moves files; whether their faces stay indexed and + keep contributing to clusters is unspecified. Probably they should be excluded from suggestions but + not deleted, so a restore does not re-index. + +--- + +## 11. Requirements touched | ID | How this document addresses it | |---|---| @@ -587,7 +793,7 @@ because if the SQLite path proves troublesome, this is the fallback with a known | FR-CAT-4 | §4.1 windowed queries, memory independent of catalog size | | FR-CAT-5 | §3.5 two-pass metadata | | FR-CAT-6 | §4.3 indexed filter compilation, §5 selectors | -| FR-CAT-7 | §2 collections schema, §5 manual and smart | +| FR-CAT-7 | §2 collections schema, §5 manual and smart, §7a hierarchy, membership and editing | | FR-CAT-9 | §3.4 the offline/deleted distinction and the sweep guard | | FR-CAT-11 | §3.5 lazy content hashing | | FR-NC-3 | §7.2 range-extract for remote thumbnails | @@ -598,3 +804,9 @@ because if the SQLite path proves troublesome, this is the fallback with a known | NFR-ARCH-2 | §6.3 priority classes shared with the GPU scheduler | | NFR-ARCH-3 | §4.3 query cancellation, §6 job cancellation | | NFR-RES-4 | §7.3 LRU cap, eviction order | +| FR-CULL-8 | §10.1 `faces` schema, §6.1 `DetectFaces` job kind on the proxy tier | +| FR-CULL-9 | §10.3 per-library calibration, stored with its validity and source hash | +| FR-CULL-10 | §10.1 `people` and `face_person`, merge-by-redirect, §10.2 clustering as a library pass | +| FR-CULL-11 | §5 `Selector::Person`, confirmed-only by default | +| FR-CULL-12 | §10.1 derived data in the catalog; names to the sidecar, UUID as merge identity | +| NFR-SEC-5 | §10.4 sync left undecided and off; nothing in §10 emits an embedding | diff --git a/docs/requirements.md b/docs/requirements.md index 5082453..09508ee 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -752,6 +752,101 @@ originals — a substantially smaller sync problem than develop parity. *Design note:* pinch-zoom accidentally triggering ratings is a documented defect in Lightroom mobile. Gesture and rating targets must not overlap. +### 3.9.1 People + +Face recognition was deferred in §7 through the 2026-08-08 calibration. It is undeferred here in a +narrower form, and the narrowing is the point. + +**What changed.** The deferral treated "face recognition" as an AI feature adjacent to subject +masking. It is not the same kind of thing. Masking is a *taste* operation applied to one image; +grouping photographs by who is in them is a **mechanical grouping problem over the whole library**, +which is the category FR-CULL-5 already commits to and already justifies: grouping is the automated +capability photographers consistently praise, because it organises without deciding. Every argument +FR-CULL-5 makes for burst grouping applies unchanged to people grouping. Answering "where are the +frames with the bride in them" across a 4,000-image wedding is a culling operation, and culling is +the differentiator. + +**What is deliberately not in scope**, because it is the failure FR-CULL-5 names: no automated +*selection*. Nothing here rejects a frame, ranks a face, scores a smile, or detects a blink. The +feature produces a **filter**, never a judgement. The user's rating axes remain the only thing that +rejects a photograph. + +**FR-CULL-8 — Face detection.** The app shall detect faces in library images as a background job, +producing per-face a bounding box, five-point landmarks, a detector confidence, and a 512-dimension +embedding. + +Detection runs against the **thumbnail or proxy tier, never a full decode** (FR-CULL-2's ladder). +This is what makes indexing affordable: a library that has been browsed has already paid for its +proxies, so face indexing adds no RAW decodes that were not already happening. Where no proxy +exists, the job requests one at background priority rather than decoding inline. + +Detection is a job in the FR-CAT-3 queue and inherits its properties without exception: coalesced +per image, interruptible, resumable across process death (FR-PLAT-AND-3), and strictly preempted by +visible work (NFR-ARCH-2). A library indexes while idle or it does not index; it never competes with +the grid. + +*Acceptance:* indexing a 10k-image library completes without the grid dropping below NFR-P9's +interaction target at any point, and survives being killed and restarted with no repeated work +beyond the in-flight image. + +**FR-CULL-9 — Calibrated identity.** Face similarity shall be expressed as a **calibrated +probability that two faces are the same person**, not as a raw embedding distance. Every threshold +in the subsystem — clustering, suggestion, auto-confirmation — shall be stated in that probability +space, and no code path may threshold a bare cosine similarity. + +This is a hard requirement rather than an implementation detail because the failure mode is +invisible. A raw cosine means something different for every model, every population, and every face +size; a threshold tuned on one library silently misbehaves on another, and an uncalibrated +similarity still *looks* like a plausible number all the way to the user interface. A displayed +confidence that does not mean what it says is worse than no confidence, because it is trusted. + +The calibration shall be fitted per library from that library's own faces, and shall report whether +it is valid. Where it is not — too few examples to fit — the app shall say the confidence is +unavailable rather than present an untuned default as though it were measured. + +*Acceptance:* on a labelled corpus, the stated probability is within a documented tolerance of the +observed match rate across the probability range (a reliability-diagram check, not a single +accuracy figure). + +**FR-CULL-10 — Clustering and naming.** Detected faces shall be clustered into unnamed groups. The +user names a group, and that name applies to its members. A person is thereafter a first-class +catalog entity with a stable UUID, independent of any name given to them. + +The user shall be able to **merge** two groups that are the same person, **split** a group that is +not, **remove** a face from a person, and **rename** a person, at any time and without re-indexing. +Splitting must be as easy as merging: clustering will over-merge on siblings, on parents and +children, and on the same person a decade apart, and a tool that can only merge makes its own errors +permanent. + +Confirmation is explicit. A face is either **suggested** (the system's inference) or **confirmed** +(the user's judgement), and the two are never conflated in storage or in display. Suggestions may be +recomputed freely; confirmations are user data and are never overwritten by a later inference pass. + +**FR-CULL-11 — People as a selector term.** A person shall be a term in the §5 selector language, +composable with every other term. + +This is the requirement that pays for the subsystem, and it is nearly free once FR-CULL-10 exists: +because one predicate language serves the library filter, smart collections, and cache rules, a +person term yields all three at once — filter the grid to a person, save "every photo of Anna rated +three or higher" as a smart collection, and pin "every photo of my children" to stay local on the +tablet. The last is a genuinely new capability, not a restatement of the first two. + +Selectors shall distinguish confirmed from suggested membership, defaulting to confirmed-only, so a +saved collection does not silently change membership when a later indexing pass revises a guess. + +**FR-CULL-12 — Names are user data; embeddings are not.** A confirmed person name is a user +judgement of the same class as a rating or a keyword, and shall be written to the sidecar +(FR-CAT-8), so it survives catalog deletion and travels with the photograph. + +Embeddings, detections, cluster assignments, and unconfirmed suggestions are **derived data**. They +live in the catalog only, are rebuildable by re-indexing, and are never written to a sidecar. This +follows ARCH §6.12 exactly: the expensive-but-reproducible artefact stays in the disposable index, +and only the irreplaceable human judgement enters the trust path. + +The person UUID is what a cross-device merge keys on, in the same way collections merge (FR-CAT-7). +Two devices that independently name the same cluster produce two people; merging them is the +ordinary FR-CULL-10 merge, not a special case. + --- ## 4. Non-functional requirements @@ -864,6 +959,37 @@ validation in release builds. **NFR-SEC-4** — No telemetry without explicit opt-in. +**NFR-SEC-5 — Face data stays on the user's own hardware.** Face embeddings (FR-CULL-8) are handled +under a stricter rule than the rest of the catalog. + +This is a personal tool for personal libraries (§1.1, D11) — the people in these photographs are the +user's family and friends. That is the reason for the rule, not a reason to relax it: the data is +sensitive precisely because it is personal, and the user is the only party with any claim on it. + +- **Never leave the device by default.** Embeddings, face crops, and cluster assignments shall not be + transmitted, uploaded, or included in any diagnostics bundle (NFR-OPS-1) or crash report + (NFR-OPS-2), under any configuration. The diagnostics path has no opt-in for this; it is excluded + outright. +- **Sync is opt-in and separately consented.** Syncing embeddings to the user's own Nextcloud is + permitted — it is their server and their photographs, and it saves re-indexing a library per device + — but it is off by default, is not implied by enabling photo sync, and the consent states in plain + language what is being uploaded and why. Person *names*, being sidecar data (FR-CULL-12), sync with + the sidecar as ordinary metadata. +- **No third-party inference.** Face detection and embedding run locally. No image, crop, or + embedding is sent to a remote inference service, and the app ships no capability to do so. +- **Deletable, in one action.** The user shall be able to delete all face data — embeddings, + detections, clusters, and people — from a single control, without deleting the catalog or any + photograph, and to disable face indexing entirely so that no such data is produced. +- **Model weights are inspectable.** The models used shall be named and versioned in the about + screen, with their licences, so a user can determine what is running on their photographs. + +*Rationale:* the rest of this document treats privacy as a property of the network boundary — TLS, +credentials in secure storage, opt-in telemetry. Face data needs more than a well-defended boundary, +because it is not revocable once it has crossed one, and because it describes people who are not the +user. The prohibition is therefore structural rather than configurable: the code paths that would +upload an embedding to anyone but the user's own server do not exist. A setting can be changed by +accident, or by a future maintainer who has forgotten why it was there; an absent code path cannot. + ### 4.6 Execution model **NFR-ARCH-1 — Named executors.** The app defines distinct executors — UI, GPU submission, decode @@ -1028,6 +1154,56 @@ both a smaller build and a usable one — it needs no develop chain. Resolving D12 sets D3 and [architecture.md §10](architecture.md)'s Phase 2. +### D13 — face inference runtime and model licensing · **OPEN** + +§3.9.1 needs to run two neural networks locally. That collides with two settled positions, and +neither collision is small enough to leave implicit. + +**1. The pure-Rust dependency policy.** Every dependency choice in this project has gone the same +way, for the same stated reason: rustls over aws-lc-rs, bundled SQLite over the system library, a +Rust Lensfun port over liblensfun, zune-jpeg over libjpeg — no C dependency to satisfy under the +Android NDK (D1's whole premise). The obvious way to run ONNX models is the ONNX Runtime C++ library, +which would be the largest exception to that policy in the codebase, and it would land on the +platform the policy exists to protect. + +The options, in the order I would try them: + +| Option | Cost | +|---|---| +| **wgpu compute**, models hand-ported to WGSL | No new dependency at all — the GPU device and shader infrastructure already exist (ARCH §5). Highest implementation effort, and a ViT is a lot of shader. | +| **`burn`** with the wgpu backend | Pure Rust, uses the existing GPU. Young, and ONNX import maturity needs checking against these two specific graphs. | +| **`ort`** (ONNX Runtime bindings) | Fastest to working code, best operator coverage. Reintroduces the C dependency and the NDK cross-compilation problem the policy avoids. | + +The tension is real: the cheapest path is the one that breaks the rule. This is worth an explicit +decision rather than a default, and S14 is what informs it. + +**2. Model licensing is a distribution blocker, not a detail.** The obvious pretrained weights are +not redistributable under GPLv3. The InsightFace "buffalo" family — ArcFace and the SCRFD detector, +the standard choices — are **licensed for non-commercial research use only**, which is incompatible +with this project's licence and with Flatpak, F-Droid, and Play distribution (NFR-COMPAT-2). Other +candidate weights need their licences read individually rather than assumed. + +Two ways out, both with costs: + +- **Find permissively-licensed weights** and ship them in-tree. Clean, offline-first, consistent with + how the Lensfun database ships. Requires that suitable weights exist at acceptable accuracy. +- **Download models on first use**, with the user accepting the upstream licence. Sidesteps + redistribution but adds a network dependency to a feature that is otherwise entirely local, needs a + hosting story, and sits badly with the local-first posture of NFR-SEC-5. + +**This must be resolved before implementation, not during it.** Discovering at packaging time that +the feature cannot ship is the expensive failure, and it is entirely avoidable — it is a licence- +reading exercise, not a research question. S14 therefore puts it first. + +*Prior art available:* `../scene-actor-extraction` is a working implementation of this pipeline +(SCRFD detect → 5-point align → 512-d embedding → Platt-calibrated similarity), benchmarked at 67.4% +macro-F1 on held-out films. Its C++ does not port — different language, OpenCV and TensorRT +dependencies — but its **design decisions do**, and they are the expensive part: the calibrated +probability space that FR-CULL-9 requires, the discipline of never thresholding a bare cosine, and +the practice of leaving an uncertain face honestly unnamed. Personal libraries should also score +better than its film benchmark: cooperative subjects, better lighting, and a closed gallery of dozens +rather than thousands. + --- ## 7. Out of scope for v1 @@ -1042,7 +1218,7 @@ note where deferring now constrains the design later. | Focus stacking | Same provenance consideration. | | Print layout | — | | Soft proofing | Parameterise the output colour stage by an arbitrary profile so this becomes a UI addition, not a pipeline change. FR-EXP-3's print-dimension mode already half-commits to print workflows. | -| Face recognition | — | +| ~~Face recognition~~ | **Undeferred 2026-08-09**, in the narrower form specified in §3.9.1 (FR-CULL-8 … FR-CULL-12): people *grouping and search*, no automated selection. Reclassified as culling rather than AI — it is the same mechanical-grouping category as FR-CULL-5, not the taste operation AI masking is. Gated on spike S14 and decision D13. | | AI subject masking | Deferred per D11. Note darktable shipped this in 5.6 (June 2026), so the gap is now visible. **Conditions for deferring safely:** AI denoise ships in v1 (FR-DEV-3g ✓), manual masking is excellent including GPU-rasterised drawn masks (ARCH §6.11 ✓), and the product has a clear differentiator (culling, §3.9 ✓). When it does land, copy darktable's shape — prompt-point segmentation producing an *editable* mask that behaves like a hand-drawn one — not Adobe's opaque version. | | AI upscaling | Deferred. Lower priority than denoise, which has no manual fallback. | | Video | — | @@ -1077,6 +1253,8 @@ note where deferring now constrains the design later. | Adaptive layout (§3.5) | Snapshot tests at each breakpoint, and a resize test asserting no state loss across a layout-class transition. | | Touch targets (FR-UI-3) | Automated check that interactive elements meet the 44pt minimum in touch modality. | | Export sizing (FR-EXP-3) | Per-mode dimension assertions, including aspect preservation, fill-crop centring, and the upscale-disabled fallback. | +| Identity calibration (FR-CULL-9) | Reliability diagram over a hand-labelled corpus: stated probability against observed match rate, asserted within tolerance across the range — not a single accuracy figure, which would hide exactly the miscalibration this tests for. Plus a static assertion that no comparison thresholds a raw similarity. | +| Face data confinement (NFR-SEC-5) | Assert that a generated diagnostics bundle contains no embedding or face crop, and that with sync disabled no face data appears in any outbound request. Verified by inspecting what the code *can* emit, since the requirement is the absence of a path. | --- @@ -1108,6 +1286,7 @@ stacks. | **S8** | **Chunked upload v2** round-trip of a 100MB RAW, including resume after process kill | FR-NC-7 correctness | FR-NC-7 | | **S12** | **GPU device loss recovery:** induce `VK_ERROR_DEVICE_LOST` mid-render, verify recreation from the edit graph with no lost edits | Whether ARCH §6.10 and NFR-R7 hold | ARCH §6.10 | | **S13** | **Slint accessibility on Android:** verify TalkBack exposure of names, roles, and values | Whether NFR-A11Y-2 is achievable in the chosen toolkit | NFR-A11Y-2 | +| **S14** | **Face pipeline in Rust, on a real personal library:** run a detector plus an embedder over ~2,000 images through a Rust ONNX runtime, at proxy resolution, on the reference desktop. Measure per-image latency, cluster purity against hand-labelled truth, and fit the FR-CULL-9 calibration to see whether it converges on a library-sized sample. **Resolve the model licence question before writing any of it** | Whether §3.9.1 is buildable without breaking the pure-Rust dependency policy, and whether the accuracy is worth the subsystem | D13, FR-CULL-8, FR-CULL-9 | ### Why this order diff --git a/docs/traceability.md b/docs/traceability.md index 7f04d76..6c5e4c1 100644 --- a/docs/traceability.md +++ b/docs/traceability.md @@ -9,97 +9,109 @@ Denominators are parsed from [`requirements.md`](requirements.md) at run time, n | Metric | Value | |---|---| -| Source files scanned | 63 | -| TRACES tags found | 49 | -| Requirements defined | 143 | -| Requirements covered | 49 | -| **Coverage** | **34.3%** (49/143) | +| Source files scanned | 82 | +| TRACES tags found | 80 | +| Requirements defined | 149 | +| Requirements covered | 60 | +| **Coverage** | **40.3%** (60/149) | ### By type | Type | Covered | Defined | |---|---|---| -| FR | 36 | 90 | -| NFR | 11 | 47 | +| FR | 44 | 95 | +| NFR | 14 | 48 | | R | 2 | 6 | ## Orphan tags A tag naming an ID `requirements.md` does not define — what renumbering produces, and what a typo produces. -_None._ +- `FR-CAT-15` ## Tagged requirements | ID | Tagged in | |---|---| -| 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-1 | [`core/dr-catalog/src/scan.rs:1`](../core/dr-catalog/src/scan.rs#L1), [`core/dr-sync/src/scan.rs:83`](../core/dr-sync/src/scan.rs#L83), [`core/dr-types/src/lib.rs:176`](../core/dr-types/src/lib.rs#L176), [`tools/traceability/src/lib.rs:473`](../tools/traceability/src/lib.rs#L473), [`tools/traceability/src/lib.rs:505`](../tools/traceability/src/lib.rs#L505), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1) | +| FR-CAT-11 | [`ui/dr-ui/src/library.rs:122`](../ui/dr-ui/src/library.rs#L122) | +| FR-CAT-12 | [`core/dr-pipeline/src/sidecar.rs:108`](../core/dr-pipeline/src/sidecar.rs#L108) | +| FR-CAT-1a | [`core/dr-types/src/lib.rs:38`](../core/dr-types/src/lib.rs#L38) | | 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-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-CAT-3 | [`core/dr-catalog/src/jobs.rs:1`](../core/dr-catalog/src/jobs.rs#L1), [`core/dr-thumbs/src/codec.rs:1`](../core/dr-thumbs/src/codec.rs#L1), [`core/dr-thumbs/src/lib.rs:1`](../core/dr-thumbs/src/lib.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), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1), [`ui/dr-ui/src/library_ui.rs:1`](../ui/dr-ui/src/library_ui.rs#L1) | +| FR-CAT-5 | [`core/dr-catalog/src/rating.rs:1`](../core/dr-catalog/src/rating.rs#L1), [`core/dr-decode/src/lib.rs:231`](../core/dr-decode/src/lib.rs#L231), [`core/dr-pipeline/src/sidecar.rs:125`](../core/dr-pipeline/src/sidecar.rs#L125) | +| FR-CAT-6 | [`core/dr-catalog/src/collections.rs:1`](../core/dr-catalog/src/collections.rs#L1), [`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-catalog/src/rating.rs:1`](../core/dr-catalog/src/rating.rs#L1), [`core/dr-types/src/selector.rs:1`](../core/dr-types/src/selector.rs#L1), [`ui/dr-ui/src/library.rs:139`](../ui/dr-ui/src/library.rs#L139) | +| FR-CAT-7 | [`core/dr-catalog/src/collections.rs:1`](../core/dr-catalog/src/collections.rs#L1), [`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), [`ui/dr-ui/src/collections_ui.rs:1`](../ui/dr-ui/src/collections_ui.rs#L1), [`ui/dr-ui/ui/collections.slint:4`](../ui/dr-ui/ui/collections.slint#L4) | +| FR-CAT-8 | [`core/dr-pipeline/src/sidecar.rs:89`](../core/dr-pipeline/src/sidecar.rs#L89), [`ui/dr-ui/src/library.rs:228`](../ui/dr-ui/src/library.rs#L228) | +| 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:95`](../core/dr-types/src/lib.rs#L95) | | 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-CULL-2 | [`core/dr-decode/src/locate.rs:1`](../core/dr-decode/src/locate.rs#L1), [`core/dr-decode/src/preview.rs:123`](../core/dr-decode/src/preview.rs#L123) | +| FR-CULL-4 | [`core/dr-catalog/src/rating.rs:1`](../core/dr-catalog/src/rating.rs#L1), [`core/dr-pipeline/src/sidecar.rs:125`](../core/dr-pipeline/src/sidecar.rs#L125), [`ui/dr-ui/src/library.rs:139`](../ui/dr-ui/src/library.rs#L139), [`ui/dr-ui/src/library.rs:228`](../ui/dr-ui/src/library.rs#L228) | | 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-3a | [`core/dr-pipeline/src/descriptor.rs:91`](../core/dr-pipeline/src/descriptor.rs#L91), [`core/dr-pipeline/src/graph.rs:138`](../core/dr-pipeline/src/graph.rs#L138), [`core/dr-pipeline/src/graph.rs:15`](../core/dr-pipeline/src/graph.rs#L15), [`core/dr-pipeline/src/graph.rs:39`](../core/dr-pipeline/src/graph.rs#L39), [`core/dr-pipeline/src/operation.rs:104`](../core/dr-pipeline/src/operation.rs#L104) | +| FR-DEV-3b | [`core/dr-pipeline/src/descriptor.rs:91`](../core/dr-pipeline/src/descriptor.rs#L91), [`core/dr-pipeline/src/graph.rs:39`](../core/dr-pipeline/src/graph.rs#L39), [`core/dr-pipeline/src/operation.rs:104`](../core/dr-pipeline/src/operation.rs#L104) | +| FR-DEV-3c | [`core/dr-pipeline/src/graph.rs:138`](../core/dr-pipeline/src/graph.rs#L138) | | 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-3e | [`core/dr-decode/src/lib.rs:442`](../core/dr-decode/src/lib.rs#L442), [`core/dr-decode/src/lib.rs:562`](../core/dr-decode/src/lib.rs#L562) | | 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: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:130`](../core/dr-sync/src/lib.rs#L130), [`core/dr-sync/src/scan.rs:50`](../core/dr-sync/src/scan.rs#L50) | +| FR-DSP-1 | [`ui/dr-ui/src/lib.rs:40`](../ui/dr-ui/src/lib.rs#L40) | +| FR-EXP-9 | [`core/dr-decode/src/lib.rs:359`](../core/dr-decode/src/lib.rs#L359) | +| 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), [`core/dr-sync-nextcloud/src/session.rs:95`](../core/dr-sync-nextcloud/src/session.rs#L95), [`ui/dr-ui/src/launch.rs:49`](../ui/dr-ui/src/launch.rs#L49) | +| 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:153`](../core/dr-sync/src/lib.rs#L153), [`core/dr-sync/src/lib.rs:36`](../core/dr-sync/src/lib.rs#L36) | +| FR-NC-2 | [`core/dr-sync-nextcloud/src/session.rs:95`](../core/dr-sync-nextcloud/src/session.rs#L95) | +| FR-NC-3 | [`core/dr-decode/src/locate.rs:1`](../core/dr-decode/src/locate.rs#L1), [`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), [`core/dr-thumbs/src/lib.rs:1`](../core/dr-thumbs/src/lib.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1), [`ui/dr-ui/src/library_ui.rs:1`](../ui/dr-ui/src/library_ui.rs#L1) | +| 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:153`](../core/dr-sync/src/lib.rs#L153), [`core/dr-sync/src/scan.rs:83`](../core/dr-sync/src/scan.rs#L83), [`ui/dr-ui/src/launch.rs:49`](../ui/dr-ui/src/launch.rs#L49) | | FR-NC-5 | [`core/dr-sync-nextcloud/src/propfind.rs:51`](../core/dr-sync-nextcloud/src/propfind.rs#L51) | | 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-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:177`](../core/dr-types/src/lib.rs#L177), [`core/dr-types/src/lib.rs:95`](../core/dr-types/src/lib.rs#L95) | +| FR-NC-8 | [`core/dr-pipeline/src/sidecar.rs:108`](../core/dr-pipeline/src/sidecar.rs#L108), [`core/dr-pipeline/src/sidecar.rs:89`](../core/dr-pipeline/src/sidecar.rs#L89), [`ui/dr-ui/src/library.rs:228`](../ui/dr-ui/src/library.rs#L228) | +| 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), [`core/dr-pipeline/src/sidecar.rs:212`](../core/dr-pipeline/src/sidecar.rs#L212) | +| FR-PLAT-AND-1 | [`core/dr-types/src/lib.rs:38`](../core/dr-types/src/lib.rs#L38) | | 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-1 | [`core/dr-decode/src/lib.rs:189`](../core/dr-decode/src/lib.rs#L189), [`core/dr-types/src/lib.rs:105`](../core/dr-types/src/lib.rs#L105), [`core/dr-types/src/lib.rs:176`](../core/dr-types/src/lib.rs#L176) | +| FR-RAW-3 | [`core/dr-decode/src/lib.rs:359`](../core/dr-decode/src/lib.rs#L359), [`core/dr-decode/src/lib.rs:85`](../core/dr-decode/src/lib.rs#L85) | | 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) | +| FR-RAW-5 | [`core/dr-decode/src/lib.rs:113`](../core/dr-decode/src/lib.rs#L113) | +| FR-UI-1 | [`ui/dr-ui/src/lib.rs:48`](../ui/dr-ui/src/lib.rs#L48) | +| FR-UI-2 | [`ui/dr-ui/src/lib.rs:48`](../ui/dr-ui/src/lib.rs#L48) | +| FR-UI-3 | [`ui/dr-ui/ui/collections.slint:4`](../ui/dr-ui/ui/collections.slint#L4) | +| FR-UI-5 | [`ui/dr-ui/src/collections_ui.rs:1`](../ui/dr-ui/src/collections_ui.rs#L1), [`ui/dr-ui/ui/collections.slint:4`](../ui/dr-ui/ui/collections.slint#L4) | | 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-ARCH-4 | [`core/dr-catalog/src/error.rs:1`](../core/dr-catalog/src/error.rs#L1), [`core/dr-thumbs/src/error.rs:1`](../core/dr-thumbs/src/error.rs#L1) | | NFR-OPS-1 | [`tools/traceability/src/lib.rs:266`](../tools/traceability/src/lib.rs#L266) | | 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-P9 | [`ui/dr-ui/src/collections_ui.rs:1`](../ui/dr-ui/src/collections_ui.rs#L1), [`ui/dr-ui/src/library.rs:1`](../ui/dr-ui/src/library.rs#L1), [`ui/dr-ui/src/library_ui.rs:1`](../ui/dr-ui/src/library_ui.rs#L1), [`ui/dr-ui/src/trash.rs:1`](../ui/dr-ui/src/trash.rs#L1) | | 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-R2 | [`core/dr-catalog/src/trash.rs:1`](../core/dr-catalog/src/trash.rs#L1) | +| NFR-R5 | [`core/dr-catalog/src/collections.rs:1`](../core/dr-catalog/src/collections.rs#L1), [`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) | +| NFR-RES-1 | [`ui/dr-ui/src/lib.rs:40`](../ui/dr-ui/src/lib.rs#L40) | +| NFR-RES-4 | [`core/dr-thumbs/src/codec.rs:1`](../core/dr-thumbs/src/codec.rs#L1), [`core/dr-thumbs/src/lib.rs:1`](../core/dr-thumbs/src/lib.rs#L1), [`core/dr-thumbs/src/lib.rs:265`](../core/dr-thumbs/src/lib.rs#L265) | | NFR-SEC-1 | [`core/dr-decode/src/error.rs:1`](../core/dr-decode/src/error.rs#L1) | | R1 | [`tools/traceability/src/lib.rs:489`](../tools/traceability/src/lib.rs#L489), [`tools/traceability/src/lib.rs:493`](../tools/traceability/src/lib.rs#L493) | | R4 | [`core/dr-gpu/src/lib.rs:123`](../core/dr-gpu/src/lib.rs#L123) | ## Not yet tagged -94 of 143 requirements have no implementation tag. Expected while the codebase is young; each should gain one as it is built. +89 of 149 requirements have no implementation tag. Expected while the codebase is young; each should gain one as it is built.
Show untagged requirements - FR-CAT-10 -- FR-CAT-11 -- FR-CAT-12 - FR-CAT-13 - FR-CAT-14 -- FR-CAT-8 +- FR-CULL-10 +- FR-CULL-11 +- FR-CULL-12 - FR-CULL-3 -- FR-CULL-4 - FR-CULL-5 - FR-CULL-6 - FR-CULL-7 +- FR-CULL-8 +- FR-CULL-9 - FR-DEV-1 - FR-DEV-2 - FR-DEV-3f @@ -125,11 +137,9 @@ _None._ - FR-EXP-8 - FR-NC-10 - FR-NC-11 -- FR-NC-2 - FR-NC-6 - FR-NC-6b - FR-NC-7 -- FR-NC-8 - FR-PLAT-AND-2 - FR-PLAT-AND-4 - FR-PLAT-AND-5 @@ -138,9 +148,7 @@ _None._ - FR-PLAT-LIN-2 - FR-PLAT-LIN-3 - FR-RAW-2 -- FR-UI-3 - FR-UI-4 -- FR-UI-5 - FR-UI-6 - FR-UI-7 - NFR-A11Y-1 @@ -165,20 +173,18 @@ _None._ - NFR-P6 - NFR-P7 - NFR-P8 -- NFR-P9 - NFR-PORT-1 - NFR-PORT-2 - NFR-PORT-3 -- NFR-R2 - NFR-R3 - NFR-R4 - NFR-R6 - NFR-RES-2 - NFR-RES-3 -- NFR-RES-4 - NFR-SEC-2 - NFR-SEC-3 - NFR-SEC-4 +- NFR-SEC-5 - R2 - R3 - R5 diff --git a/docs/ui-refinement.md b/docs/ui-refinement.md new file mode 100644 index 0000000..79bd452 --- /dev/null +++ b/docs/ui-refinement.md @@ -0,0 +1,496 @@ +# UI refinement: toward a Lightroom-shaped darkroom + +TRACES: FR-UI-1 | FR-UI-3 | FR-DEV-3a | FR-CAT-4 + +## Why + +The v0.1 UI proved the architecture: capability-driven controls, a windowed +grid, a GPU canvas with no CPU round trip. What it has not yet done is *feel* +like a photo editor. The gaps are structural rather than cosmetic, and this +document names them so they can be closed independently. + +Four principles guide every change below. + +1. **The image is the subject.** Chrome recedes; nothing competes with the + photograph for attention or for colour. +2. **Colour is a signal, not a decoration.** The accent means *modified* or + *active*. Everywhere it currently means "heading" or "chrome", it is + spending a signal on noise. +3. **Navigation is continuous.** Moving between images should not be a change + of screen. Lightroom's filmstrip is the mechanism; modal view switching is + what it replaces. +4. **Density is earned.** A panel shows what has been touched; everything else + collapses out of the way. + +## Non-goals + +- No new pipeline operations, and no change to how operations reach the panel. + `AdjustPanel` must still learn its contents from the capability model and + must still name no operation (FR-DEV-3a). +- No change to the catalog schema, the sync layer, or the render path. +- No light theme. The ground stays dark (see `theme.slint` preamble). + +--- + +## Workstream S — The style layer + +Prerequisite for everything else that touches colour. Lands before B–F. + +### S1 — Near-neutral palette + +**Problem.** The original palette was a warm "darkroom safelight" brown — +ground `#14120F`, R twelve points above B, and the same cast through every +surface and ink. That biases the work. Simultaneous contrast pushes perception +of the image *away* from its surround, so warm chrome makes a neutral +photograph read cool; the photographer corrects toward warm to compensate and +every export drifts yellow. The file's own preamble had the right instinct — +a light UI biases judgement — and stopped one step short. Warmth biases it +too, and more quietly, because a warm cast reads as *cosy* rather than +*wrong*. + +**Done.** `theme.slint` now carries near-neutral greys with a 2–3 point cool +lift (pure R=G=B reads as dead; a trace of cool reads as instrument), and the +red accent is replaced by an achromatic `active` family. `warn-ink` is the +only hue left — a caution is genuinely a different kind of thing from an +active state. + +Migration shims alias `accent`/`accent-dim`/`accent-hover` onto the new +tokens, so the tree compiles while call sites migrate. **They are temporary.** +Delete them once `grep -rn 'Theme.accent' ui/` is empty. + +### S2 — `style.yaml` as the token source + +**Problem.** Tokens live in Slint, so tuning a palette means editing a +language file, and nothing else — docs, tooling, a future export theme — can +read them. + +**Deliverable.** `ui/dr-ui/style.yaml` becomes the source of truth for +**colours and lengths**: the whole current token set, nothing more. + +- `ui/dr-ui/build.rs` reads it and generates `theme.slint` at compile time. + Zero runtime cost, and a malformed file is a build error rather than a + failure in front of a photographer mid-edit. +- The generated file carries a "do not edit" banner naming its source, and + must land somewhere `.gitignore`d or clearly marked generated — a + hand-edited generated file is a bug that hides for weeks. +- **The prose survives.** The reasoning in today's `theme.slint` preamble and + its per-token comments is the most valuable thing in the file. YAML comments + carry across into the generated Slint, or the generator emits them from + structured fields. A token set with the *why* stripped out is a downgrade, + however tidy the pipeline. +- Debug-only live reload behind a feature flag: re-read the YAML at startup so + a palette can be tuned without a full rebuild. Off in release, where the + generated constants are what ship. + +**Not in the YAML.** Semantic components (S3). `PanelHeading` binds colour, +size, weight and letter-spacing into one concept — that is Slint, not data. +YAML holds leaf values; components compose them. + +**Dependency.** Adds a YAML parser to the UI's build-dependencies. Note that +`serde_yaml` was deprecated in 2024; prefer a maintained alternative. + +### S3 — Semantic components + +**Problem.** `theme.slint` says what `surface` is. It says nothing about what +a *panel heading* is — so every file re-derives one. `IMAGE`, `ADJUST`, the +six launch-screen headings, `library.slint:267` each independently spell out +colour, size, weight and letter-spacing for a single concept. That is why one +accent reached forty call sites: there was no single place to change it. + +**Deliverable.** `widgets.slint` grows from three primitives into the style +layer: + +- `PanelHeading` — the `IMAGE`/`ADJUST`/launch headings. One definition, one + place to decide headings are ink rather than accent. +- `Label`, `Value`, `Caption` — text roles, so `text-sm` + `ink-dim` stops + being copy-pasted. `Value` carries the modified state, since a value that + differs from its default is the one thing worth spotting at a glance. +- `Panel` — surface + rule + padding, currently rebuilt in four places. +- `Field` — the launch screen's text input with its focus border. + +**The rule this establishes.** Files consume components. Raw `Theme.*` is for +*composing* a component, not for styling a call site. A new colour literal or +a bare `Theme.ink-faint` in a screen file is a signal that a component is +missing. + +**Sweep.** Migrating call sites onto these components removes the `accent` +references as a side effect — the forty sites collapse into a handful of +component definitions. That is the point: fix the cause, not the symptom. +Delete the S1 shims when the grep comes back empty. + +**Done when.** `grep -rn 'Theme.accent' ui/` is empty, the shims are gone, no +screen file styles a heading inline, and the UI carries no hue outside +`warn-ink`. + +--- + +## Workstream P — Plural presentation hints + +Implements ARCH §4.3a. Touches `core/dr-pipeline` and `ui/dr-ui/src/develop.rs`; +no Slint change beyond what falls out of it. + +**Problem.** `Presentation.widget` is a single `WidgetKind`. An operation can +name one preferred control and nothing else, so a curve cannot say "a curve +editor is best, a parametric band control would do, and sliders are fine" and +let the frontend choose. Since layout class already drives compact-vs-expanded, +this is not hypothetical: a curve editor is comfortable in a 280px panel and +unusable in a 120px one, and today the frontend has no sanctioned way to +decide that — its only options are the named widget or nothing. + +**Deliverable.** + +- `Presentation.widget` becomes `widgets: &'static [WidgetKind]`, in descending + preference. The frontend takes the first it implements and can afford. +- A `WidgetDemand` describing what a widget inherently requires — candidates: + two-dimensional direct manipulation, precision pointing, a minimum count of + simultaneous values. **No pixels, no breakpoints, no DPI, no platform names.** + Those are frontend thresholds and live in `dr-ui`. +- `develop.rs` chooses per operation: walk the hint list, take the first whose + demands the current layout satisfies, else fall through to plain scalars. + The existing `ParamRow.kind` string is where exhaustive matching is currently + lost — the choice must happen in Rust against the enum, before flattening. +- The tone curve declares `[Curve]` with its real demands. Nothing else needs + to change; operations wanting sliders still say nothing at all. + +**Verification.** The existing `the_curve_collapses_to_a_single_row` test in +`develop.rs` covers the happy path. Add its complement: with a layout that +cannot satisfy the curve's demands, the same capability must produce ten +addressable scalar rows and remain fully editable. That test is the contract — +it is what makes the fallback real rather than aspirational. + +**Do not** let a demand grow a `min_width`. If one seems necessary, the +demand vocabulary is wrong; widen the vocabulary, not the abstraction. + +--- + +## Workstream M — The colour mixer is unreadable + +Found in use, not in review. The panel currently renders the mixer as +thirty-six anonymous sliders reading `Hue 0 / Sat 16 / Lum 0` twelve times +over, with nothing saying which band any row belongs to. Three separate +defects meet here. + +### M1 — Band identity is lost (a bug, not a style issue) + +`labels.rs` has no `param.mixer.*` entries, so all thirty-six keys fall +through to a default that yields the bare channel name. The core is not at +fault: it declares `param.mixer.orange.sat`, and `BANDS` carries `key: +"orange"` with `hue: 30.0`. The identity is present in the capability and +discarded at resolution. + +**Deliverable.** Resolve mixer keys to their band. A row reads `Orange · Sat`, +or `Sat` under a band heading — M3 decides which. + +### M2 — Bands carry their centre hue as capability data + +**Deliverable.** `ParamDescriptor` gains an optional band hue in degrees, set +by `band_params!` from `BANDS`. The UI converts degrees to a swatch. + +**Why this is not a §4.3a violation.** A band's centre hue is a *fact about +the operation* — the mixer genuinely acts on the 30° band, and that number is +what it acts on. The core says "this parameter belongs to the band centred at +30°". It does not say what colour to draw, at what saturation or lightness, or +whether to draw a swatch at all. Those conversions are presentation and live +in `dr-ui`; the swatch's saturation and lightness belong in `style.yaml`. + +The line to hold: a hue in degrees is data. A hex colour in a descriptor +would be the core deciding appearance, and is forbidden. + +**Swatches are the one sanctioned exception to the achromatic palette.** A +swatch is not chrome — it is data identifying which hue band a row edits, +exactly as an image is data. That is categorically different from an accent +decorating a heading, which is what the palette rule forbids. Keep them small +and let them identify, never dominate. + +### M3 — Structure: twelve collapsible bands + +**Deliverable.** The mixer renders as twelve `Section`s — one per band, titled +by band name, carrying its swatch — each holding Hue, Sat and Lum. Collapsed +by default; the modified dot (Workstream C) shows which bands hold an edit +without expanding them. + +This needs no new `WidgetKind`: it is Workstream C's sections applied to a +grouping the frontend derives from parameter ids. Depends on C. + +### M4 — A single-parameter operation should not cost a heading + +Vibrance and Saturation each render a section heading above one slider, +spending two lines and a visual break on one control. An operation whose +parameters number one wants to *be* a named row, not a group containing one. + +**Deliverable.** The panel collapses a single-parameter operation into one +row labelled by the operation. Derived frontend-side from the parameter +count — the core says nothing about it, per §4.3a. + +**Done when.** Every mixer row says which band it edits; bands collapse with +modified state visible while collapsed; Vibrance and Saturation are one row +each; and no hex colour appears in any descriptor. + +--- + +## Workstream V — Vertical density + +The panel spends too much height on too little information. Reported from +use, and it compounds Workstream M: at thirty-six mixer rows the waste is +measured in whole screens. + +**The arithmetic.** `ParamSlider` is a fixed 46px carrying an 11px label and a +3px track. The track region is `Theme.touch-target / 2` — 22px — which is a +finger-sized allowance drawn for a pointer, and the label occupies a line of +its own above it. Every section heading adds a further `Theme.gap` (12px) +spacer plus a 2px rule. Twelve mixer bands at three rows each is roughly +1650px of panel, most of it air. + +**The cause is layout, not spacing tokens.** Shaving pixels uniformly would +compress the readable parts along with the waste. The row is stacked when it +could be inline: name left, value right, track beneath — which is what +Lightroom does, in about 32px. + +**Deliverable.** + +- Rework `ParamSlider` so label and value share one line and the track sits + under them. Target ~32px per row, down from 46px. +- The *drawn* track shrinks; the **TouchArea does not**. FR-UI-3 is about the + finger, and `Button` already establishes the pattern — draw at control + height, grow the hit target past the ink and centre it. A denser panel must + not become a less touchable one. +- Section heading spacing comes from one token, not an inline `Rectangle { + height: Theme.gap }`. A spacer rectangle written inline is how the panel + ended up with spacing nobody can adjust centrally. +- Re-check the compact layout class after the change: rows that work at 280px + may crowd at narrower widths, where the touch overhang also matters most. + +**Constraint.** Density is not the goal; *legibility per pixel* is. If a row +gets shorter and harder to read, it has failed. The value readout in +particular carries the modified signal and must stay scannable. + +**Done when.** A parameter row is ~32px, hit targets still meet FR-UI-3 under +touch, heading spacing is tokenised, and the panel reads as easily at the new +density as the old. + +--- + +## Workstream A — Shared chrome primitives + +**Problem.** Buttons are hand-rolled `Rectangle` + `TouchArea` pairs in +`app.slint`, `library.slint`, and `launch.slint`, at three different sizes +(64×20, 110×28, 88×28) with three near-identical hover/press treatments. Any +consistency in the chrome is currently coincidental. + +**Deliverable.** A new `ui/dr-ui/ui/widgets.slint` exporting: + +- `Button` — `text`, `enabled`, `primary` (bool), `clicked()`. Height meets + `Theme.touch-target` under compact layout and may be denser when expanded. + Press and hover states derive from theme tokens, not literals. +- `IconButton` — square, for toolbar affordances that carry a glyph. +- `Section` — a collapsible container: `title`, `modified` (bool), + `expanded` (in-out bool), a default child slot. Draws the disclosure + triangle and the modified dot. Workstream C consumes this. + +Every existing hand-rolled button is replaced by `Button`. The visual result +should be a *narrower* range of sizes than today, not a wider one. + +**Theme additions.** `theme.slint` gains what the widgets need and no more: +`radius-sm`/`radius`, a `hover` and `pressed` surface token, and a +`modified` token aliased to `accent`. Adding tokens is preferred over +literals appearing in widget bodies. + +**Done when.** No `TouchArea` inside a `Rectangle` styled as a button remains +in `app.slint`, `library.slint`, or `library.slint`'s header. `cargo build` +clean, app launches, every button still fires its callback. + +--- + +## Workstream B — Canvas presentation + +**Problem.** The canvas fills its container edge to edge. The image reads as a +texture rather than a print, and there is no visual separation between the +photograph and the panel beside it. + +**Deliverable.** In `app.slint`'s `canvas-area`: + +- Inset the image by a margin that scales with the layout class — generous + when `expanded`, tighter when compact, never zero. The surrounding field is + `Theme.ground`. +- A subtle 1px `Theme.rule` border on the image bounds, so a dark photograph + does not bleed into the dark ground. This requires knowing the *fitted* + rectangle, not the container — if that proves awkward in Slint, a shadow or + a very slightly lighter mat behind the image is an acceptable substitute. + Pick one and say which in the summary. +- Empty and error states keep their current copy and centring. + +**Constraint.** `canvas-resized` must continue to report the *drawable* pixel +size — the render target follows the image area, not the container. Getting +this wrong shows up as a soft or stretched image, so verify the reported size +changes when the margin does. + +**Done when.** The image sits in a visible field with margin, the border or +mat is present, and resizing the window still produces a crisp canvas. + +--- + +## Workstream C — Collapsible adjust sections + +**Problem.** `AdjustPanel` renders every parameter of every operation, always +expanded. This is tolerable at today's operation count and unusable at +fifteen. Lightroom's right panel is a stack of collapsible modules whose +headers report whether anything inside has been touched. + +**Deliverable.** Rework the `for row[i] in root.rows` body in `adjust.slint` +to group by operation and wrap each group in `Section` (Workstream A). + +The hard part is that `rows` is a **flat** model with a `starts-group` flag — +Slint cannot easily nest a `for` inside a group boundary derived at runtime. +Two viable approaches; pick one and justify it briefly: + +1. **Flatten the collapse.** Keep the flat `for`, add an `expanded` bool per + op-index held in the panel, and make each non-heading row `visible: false` + and zero-height when its group is collapsed. Simple, no Rust change. +2. **Nest the model.** Have Rust supply `[[ParamRow]]` — one inner model per + operation. Cleaner Slint, but changes the `ParamRow` contract and the + `develop.rs` code that builds it. + +Approach 1 is likely correct for this pass; prefer it unless it proves +unworkable. + +**Modified indicator.** A group is modified when any row in it has +`value != default-value`. + +**This must be derived in `dr-ui`, not supplied by the core** (ARCH §4.3a). +A `group_modified` flag on a descriptor would be the core deciding the panel +has groups at all, which is a composition decision. `develop.rs` already holds +both the capabilities and the live values, so it can aggregate per operation +while flattening — that is frontend-side derivation and stays on the right +side of the line. What it must not do is ask the core for the answer. + +The same reasoning condemns the existing `starts-group` flag, which is the +core telling the panel where to draw section breaks. It predates this contract; +fold it into the same pass and derive grouping from `op-index` changes instead. + +**Also.** The per-group `reset` should live on the section header, alongside +the existing global `reset`. + +**Invariant.** This file must still name no operation. Collapse state is keyed +by `op-index`, never by label. + +**Done when.** Sections collapse and expand, collapsed state survives a slider +drag elsewhere in the panel, headers show a modified dot that appears and +disappears as values move off and back to default, and adding an operation to +the pipeline still requires no edit to `adjust.slint`. + +--- + +## Workstream D — Grid refinement + +**Problem.** Cells are boxes first and images second: `image-fit: contain` on +a square cell leaves landscape shots floating in dead space, the cell surface +contrasts with the ground so the grid reads as a rhythm of rectangles, and +there is hover state but no *selection* state. + +**Deliverable.** In `library.slint`: + +- Cells crop to fill (`image-fit: cover`) with `clip: true`, so the grid is a + rhythm of images. The filename caption stays. +- Cell background moves to `Theme.ground` or very near it; the frame recedes. +- A **selected** cell gets a persistent accent ring. Add + `in property selected-index` to `LibraryGrid`, defaulting to -1, and + have `library_ui.rs` set it when a cell is clicked. Hover stays distinct + from selection — a dimmer treatment. +- Keyboard navigation: arrow keys move the selection, Enter opens it. This + needs a `FocusScope` over the grid and a `selection-moved(int)` callback. + +**Done when.** The grid reads as images rather than boxes, the current image +is unambiguous, and arrows plus Enter navigate it without the mouse. + +--- + +## Workstream E — Chrome hierarchy + +**Problem.** `StatusBar` mixes three unrelated things: navigation (`‹ +Library`), identity (filename, position), and spike telemetry (fps, adapter, +backend, layout-class). The telemetry earned its place while assumption A1 was +open; it is now permanent furniture competing with the photograph. + +**Deliverable.** + +- The top strip carries identity and navigation only: filename, position, + and the library affordance. +- Telemetry moves behind a toggle — a keyboard shortcut (suggest `` ` ``) that + reveals a small diagnostics overlay in a corner of the canvas, carrying + backend, adapter, fps, and layout class. Default off. +- The accent stops being used for chrome. `backend` in the status bar, the + `IMAGE` and `ADJUST` panel headings, and the section headings in + `adjust.slint` all move to `Theme.ink-faint` or `ink-dim`. After this pass, + accent should appear only on: modified values, the modified dot, the curve + line, slider fill, selection, and progress. + +**Done when.** A fresh launch shows no fps counter and no accent-coloured +chrome; `` ` `` toggles the diagnostics overlay; every previously-visible +diagnostic is still reachable. + +--- + +## Workstream F — Filmstrip and unified view + +**The big one.** Depends on A (for `Button`) and D (for cell treatment and +selection). Should land last. + +**Problem.** Library and Develop are mutually exclusive screens +(`show-library` in `app.slint`). Every move between images is a change of +screen. Lightroom's continuity comes from the grid never fully leaving: it +collapses to a filmstrip along the bottom of the develop view, and clicking a +neighbour is navigation, not a mode change. + +**Deliverable.** + +- A `Filmstrip` component in a new `ui/dr-ui/ui/filmstrip.slint`, consuming + the **same** `[LibraryCell]` model and the same `selected-index` as + `LibraryGrid`. Horizontal, ~90px tall, scrolls to keep the selection + visible. +- Develop gains the filmstrip along its bottom edge, visible when a library is + open (i.e. when the current model is non-empty). Command-line file sets get + it too — they are also a list of images. +- Clicking a filmstrip cell loads that image. Arrow keys drive both the + filmstrip and the existing next/prev, which become the same action. +- `show-library` becomes a *mode* rather than a screen swap: Grid mode and + Develop mode over one shared library state, toggled by a `G`/`D` shortcut + and by the existing buttons. The `can-return-to-library` special case and + the `‹ Library` button both disappear. + +**Windowing constraint.** The filmstrip and the grid must share one windowed +model, not hold two. `LibraryController` currently keys its `WINDOW` on grid +scroll position; the filmstrip's window follows the *selection* instead. This +is the genuinely hard part of the workstream — the window must move as +selection walks past its edge, and thumbnail requests must not thrash when it +does. Resolve this explicitly rather than by widening `WINDOW`. + +**Done when.** Selecting an image in the grid enters develop with the +filmstrip showing neighbours; arrows walk the filmstrip and load images; +`G`/`D` toggles modes with selection preserved in both directions; a +17k-image library still holds a bounded number of live cells and does not +re-fetch thumbnails on every keystroke. + +--- + +## Sequencing + +``` +A (primitives) ──┬── C (sections) + ├── B (canvas) ── E (chrome) + └── D (grid) ──┐ + ├── F (filmstrip) + ┘ +``` + +A, B, D, and E are independent of each other once A lands; C depends on A; +F depends on A and D. B and E both touch `app.slint`, so they should not run +concurrently. + +## Verification, all workstreams + +- `cargo build` clean, no new Slint warnings — in particular no binding-loop + warnings, which `app.slint` already comments on at length and which can + panic at runtime. +- The app launches and reaches the grid. +- No workstream may break FR-DEV-3a: adding a pipeline operation must still + surface in the panel with no UI edit. diff --git a/docs/view-composition.md b/docs/view-composition.md new file mode 100644 index 0000000..f3fb5d5 --- /dev/null +++ b/docs/view-composition.md @@ -0,0 +1,296 @@ +# View composition: a controller for the display layer + +TRACES: FR-UI-1 | FR-UI-6 | FR-UI-8 | FR-DEV-3a | NFR-P9 + +**Status:** Draft · 2026-08-09 +**Companion to:** [architecture.md](architecture.md) §4.3a, [ui-refinement.md](ui-refinement.md) + +## Why + +`dr-ui` has four views — launch, library, develop, collections — and no view +layer. `run()` in `ui/dr-ui/src/lib.rs` is 500 lines that construct every +controller, wire every cross-view callback, own the develop session, and hold +the only complete picture of what is on screen. It has become the controller +by accretion rather than by design, and it shows in three specific ways. + +**The back-patched knot.** `run()` declares + +```rust +let open_from_library: Rc>>> = ... +``` + +wired empty at line 328 and filled at line 604, because the library grid needs +to open an image in develop and the develop closure needs the GPU context that +is built after the library is wired. The comment in the source is candid about +it: *"This cell is the knot between them: wired empty here, filled once `show` +exists."* A nullable function slot resolved at runtime is what a dependency +cycle looks like when the language will not let you write one directly. + +**Two copies of load-and-display.** `show` (lib.rs:524) and the remote-fetch +timer body (lib.rs:657) run the same sequence after their inputs diverge: set +`load_error` empty, set camera / exposure / dimensions from metadata, branch on +whether sensor data was recovered, then either `set_adjust_enabled(true)` + +`sync_rows` + `redraw`, or clear the session, empty the rows, disable adjust +and show the fallback image — and on failure, clear the session and report the +error. One takes a path and one takes bytes; everything downstream is written +twice. + +The shared preamble has *already* been factored out into `reset_view_state`, +which both branches call, so the direction is established — the post-load half +is simply the part that has not been done yet. The two halves have not yet +drifted in the fields they set; the argument for consolidating is to keep it +that way, since every future display field must currently be added in two +places. + +**Diffuse ownership of window state.** Distinct `window.set_*` calls by module: + +| Module | Distinct window properties written | +|---|---| +| `library_ui` | 24 | +| `lib.rs` (`run`) | 23 | +| `launch_ui` | 16 | +| `collections_ui` | 9 | + +No one owns "what is on screen". The clearest symptom is view switching +itself: `show-launch` and `show-library` are two booleans encoding one piece +of state, written from seven call sites across three modules +(`lib.rs:353`, `launch_ui.rs:171`, `library_ui.rs:246/254/1486/1645/1655`). +Nothing prevents both being true, and `app.slint` compensates with +`if !root.show-launch && root.show-library` chains at lines 349, 386 and 491. + +`develop.rs` is the exception that proves the point: it writes no window +properties at all, taking capabilities in and returning rows and images out. +It is the one module already shaped the way this document argues for. + +None of this is broken. It works, and several of the surrounding patterns are +load-bearing and correct — the mpsc-plus-timer worker shape exists because +Slint's event loop must never block (NFR-P9), and `sync_rows` mutates rows in +place because replacing the model breaks slider dragging. This document +changes ownership, not threading and not Slint model handling. + +## Non-goals + +- No change to the threading model. Workers stay on threads; results still + return through channels drained by Slint timers (NFR-P9). +- No change to the pipeline, catalog, sync layer, or render path. +- No change to how operations reach the adjust panel. `develop.rs` already + composes from capabilities and names no operation (FR-DEV-3a); that stays. +- **No dynamic view instantiation.** See the constraint below. + +## The constraint that shapes all of this + +Slint has no runtime component instantiation. Views are selected by statically +compiled conditionals over properties — `if root.show-launch: LaunchScreen` +and friends — so every view must exist in the `.slint` source at build time. + +Descriptors therefore drive **composition and chrome**: which views exist, +their labels, their order, whether each is currently available, and which one +is active. They cannot conjure a view body. This is a smaller claim than +"self-describing modules" might suggest, and stating it here is deliberate: +a design that assumed otherwise would hit the wall at codegen. + +`build.rs` already generates `theme.slint` from `style.yaml`, so generating a +tab bar from the descriptor set is *possible* later. It is not proposed now — +generated `.slint` is markedly harder to debug than generated tokens, and the +win does not yet justify it. + +--- + +## Stage 1 — `ViewController` + +Independently landable, and worth landing whether or not stages 2 and 3 +follow. + +A `ViewController` owns the window handle and the state that currently floats +in `run()`'s closure captures: `session`, `entries`, `index`, `viewport`, +`rows`, and `redraw`. Sub-controllers are held by it rather than wired to each +other through `run()`. + +```rust +pub struct ViewController { + window: slint::Weak, + gpu: Option, + session: RefCell>, + entries: RefCell>, + index: Cell, + viewport: Cell<(u32, u32)>, + rows: Rc>, + library: Rc, + collections: Rc, + launch: Rc, +} +``` + +Three changes follow, in order: + +**1a. One `present()`.** Both load routes converge on a single method: + +```rust +impl ViewController { + fn present(&self, loaded: Result, name: &str) { ... } +} +``` + +The local path calls `self.present(load(gpu, path), &name)`; the remote timer +calls `self.present(load_bytes(gpu, &bytes), &name)`. The duplicated post-load +sequence exists once, so a new display field can only be added in one place. + +**1b. `Rc` replaces the nullable callback cell.** The grid's +click handler captures the controller and calls +`controller.open_remote(path)`. Both sides now depend on the controller rather +than on each other, so the cycle disappears and the `Option` slot with it. + +**1c. Collapse the four adjust callbacks.** `on_param_changed` (lib.rs:702), +`on_param_reset` (716), `on_reset_all` (730) and `on_curve_reset` (747) are +four blocks differing only in which `DevelopSession` method they call; each +then runs the identical `sync_rows` + `redraw` pair. A single +`self.mutate_session(|s| ...)` helper that performs the mutation and then +re-syncs makes the shared tail impossible to forget. + +Note that not every handler wants that tail — the view/pan handlers below them +deliberately `redraw` without `sync_rows`, because panning changes no +parameter. The helper must therefore be the *opinionated* path for parameter +mutation, not a mandatory funnel for everything that touches the session. + +**Verification.** These are refactors with no behavioural change: existing +tests in `lib.rs` (`describe_camera`, `describe_exposure`, `collect`) must pass +untouched, and manual checks cover launch → library → develop, next/previous, +slider drag, reset, and canvas resize. + +--- + +## Stage 2 — The `View` trait + +The same discipline `descriptor.rs` already applies to operations, applied one +level up. That precedent matters: the core publishes `OpDescriptor` / +`ParamKind` / `Presentation`, and `develop.rs` builds controls from it without +naming a single operation. Views are the same shape of problem. + +```rust +pub struct ViewDescriptor { + pub id: ViewId, + pub label: LocalizedKey, + pub kind: ViewKind, +} + +/// Closed, not a string — a controller must be able to match exhaustively +/// and know it has covered everything. Same reasoning as `WidgetKind`. +pub enum ViewKind { + /// Occupies the window alone; no chrome, not tabbable. Launch. + Modal, + /// Participates in the tab set. Library, develop. + Primary, + /// Renders beside a primary view. Collections. + Adjunct, +} + +pub enum Availability { + Available, + /// Greyed out, with a reason the UI can show. Not hidden — a missing + /// tab is indistinguishable from a bug. + Unavailable(LocalizedKey), +} + +pub trait View { + fn descriptor(&self) -> ViewDescriptor; + fn availability(&self, ctx: &AppContext) -> Availability; + fn activate(&self, ctx: &AppContext) {} + fn deactivate(&self, ctx: &AppContext) {} +} +``` + +Two properties are carried up from `descriptor.rs` deliberately, because they +are why that design works: + +- **`ViewKind` is a closed enum.** Its `WidgetKind` counterpart says so + explicitly: *"a UI must be able to match exhaustively and know it has + covered everything the core can ask for."* +- **Descriptors are hints with a working fallback.** A curve degrades to + sliders. A view whose `kind` a shell does not implement still renders + standalone; nothing about tabbing is required for a view to function. + +Labels are `LocalizedKey`, reusing the type `dr-pipeline` already exports — +`dr-ui` depends on `dr-pipeline`, so this adds no coupling, and it keeps view +labels on the same footing as operation labels. `labels::resolve` already takes +a `&str` and derives a readable fallback for uncatalogued keys, so a new view +appears with a sensible label before anyone writes a translation, exactly as a +new operation does today. + +`availability` is the part that earns the trait rather than merely tidying. +Develop-without-a-GPU and collections-without-a-catalog are handled +inconsistently today — `run()` logs a warning and sets `backend` to "NO GPU", +`library_ui` guards each catalog access separately — and the differences are +not intentional. One method, asked before a view is offered, makes those cases +uniform and gives the shell something honest to display. + +`activate`/`deactivate` exist so switching away can stop timers and release +GPU resources instead of leaking them, which nothing does today. + +**Scope check.** Four views, all known, all in-tree, no third-party authors. +This abstraction has to justify itself against an `if` chain, and on tab +composition alone it would be close. The `availability` consolidation is what +tips it, because that is a correctness fix rather than a tidiness one. + +**Verification.** Stage 2 retrofits the trait to the four existing views and +changes no Slint. The descriptors must reproduce current behaviour exactly +before stage 3 consumes them. + +--- + +## Stage 3 — Data-driven view switching + +With descriptors in place, the boolean pair collapses: + +``` +- in property show-launch; +- in property show-library; ++ in property active-view; ++ in-out property <[TabEntry]> view-tabs; +``` + +`app.slint`'s conditionals key off `active-view == "library"` rather than a +two-boolean conjunction, and the illegal both-true state stops being +representable. `view-tabs` is a model the controller publishes from the +descriptor set, carrying label, id, and availability — so the tab bar is built +from data even though the view bodies are static. + +Only `ViewController` writes `active-view`. The seven scattered `set_show_*` +call sites become `controller.activate(ViewId::Library)`, which is also the +hook where `deactivate` on the outgoing view runs. + +**Verification.** Behavioural parity on every transition currently reachable: +launch → library on sign-in, library → develop on cell click, develop → +library on back, and the startup paths in `launch::Startup` (all three arms). + +--- + +## Sequencing + +| Stage | Depends on | Independently valuable | +|---|---|---| +| 1 — `ViewController` + `present()` | — | Yes: removes the cycle and the duplication | +| 2 — `View` trait | 1 | Yes: uniform availability handling | +| 3 — `active-view` | 2 | Yes: illegal states unrepresentable | + +Stage 1 first regardless. The descriptor layer needs a controller to live in, +and the `present()` duplication is a live bug source independent of how views +are composed. + +## Requirements + +Stages 1 and 2 are traced by existing IDs — FR-UI-6 (shared components), +FR-DEV-3a (self-describing), NFR-P9 (no UI-thread blocking). Stage 3's claim, +that view composition is data-driven and view identity single-valued, has no +requirement covering it. Proposed for `requirements.md` §3.5, after FR-UI-7: + +> **FR-UI-8 — Self-describing views.** Each view publishes a descriptor — +> identity, label key, kind, and current availability — and a single +> controller composes the interface from the descriptor set. View identity is +> single-valued: exactly one primary view is active at a time. A view that +> cannot currently function reports why, and the shell presents it as +> unavailable rather than omitting it. Adding a view requires no change to the +> shell beyond registering it and declaring its body. + +Not added to the register by this document; adding it is a separate edit, +since `requirements.md` is the register of record and renumbering there ripples +into `traceability.md`.