diff --git a/core/dr-catalog/src/keywords.rs b/core/dr-catalog/src/keywords.rs index c3676ba..4326234 100644 --- a/core/dr-catalog/src/keywords.rs +++ b/core/dr-catalog/src/keywords.rs @@ -1,4 +1,4 @@ -//! TRACES: FR-CAT-5 | FR-CAT-6 | FR-CAT-13 | NFR-R5 +//! TRACES: FR-CAT-5 | FR-CAT-6 | NFR-R5 //! Keywords: the vocabulary, the assignments, and the edits the UI performs. //! //! The read half of this shipped with the catalog and the write half did not. diff --git a/core/dr-pipeline/src/sidecar.rs b/core/dr-pipeline/src/sidecar.rs index f70d91c..6f83616 100644 --- a/core/dr-pipeline/src/sidecar.rs +++ b/core/dr-pipeline/src/sidecar.rs @@ -1727,6 +1727,24 @@ mod tests { assert!(!restored.is_neutral()); } + /// TRACES: FR-PLG-8 + /// FR-PLG-8's preservation clause, and only that clause. + /// + /// The requirement opens by saying the sidecar "already preserves lines it + /// does not understand verbatim and writes them back untouched", and that + /// the property "is now load-bearing and shall be treated as such". This is + /// the test that makes it load-bearing rather than incidental. + /// + /// **It is weaker than the requirement's stated acceptance criterion**, + /// which is that the re-saved file be *byte-identical* to the original. + /// This asserts `contains`. The two halves of that criterion exist + /// separately — `writing_the_same_state_twice_is_byte_identical` proves + /// byte identity for content this build understands, and this proves + /// survival for content it does not — and nothing yet joins them into the + /// one claim FR-PLG-8 makes. Everything else the requirement asks for + /// (aggregated alerts, the export gate, rendering without rather than + /// guessing) is unbuilt, which is why the tag is on these two tests and + /// not on the module. #[test] fn an_unknown_operation_survives_a_round_trip() { // The data-loss case that matters: a device running an older build @@ -1742,6 +1760,13 @@ mod tests { ); } + /// TRACES: FR-PLG-8 + /// The other half of preservation: kept in the file, kept out of the edit. + /// + /// "Render without, never render a guess" is a separate clause, and this is + /// not it — that one is about a *recognised* operation whose plugin is + /// missing. This is the narrower claim that an unparsed line cannot reach + /// the graph by accident, which is what makes preserving it safe. #[test] fn an_unknown_operation_does_not_reach_the_graph() { let text = "drsc 1\n\n[version u1]\nname = Default\nrevision = 1\nmodified = 0\n\ diff --git a/core/dr-types/src/lib.rs b/core/dr-types/src/lib.rs index 987c111..505f930 100644 --- a/core/dr-types/src/lib.rs +++ b/core/dr-types/src/lib.rs @@ -51,7 +51,7 @@ pub struct CollectionId(pub u64); #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct FolderId(pub u64); -/// TRACES: FR-CAT-1a | FR-PLAT-AND-1 +/// TRACES: FR-CAT-1a /// An opaque, re-resolvable reference to source image data. /// /// **Never a filesystem path.** Android's Storage Access Framework provides no diff --git a/docs/outstanding.md b/docs/outstanding.md index 1bc0473..0987d27 100644 --- a/docs/outstanding.md +++ b/docs/outstanding.md @@ -177,13 +177,18 @@ The Android app is not a stub — it builds an APK, runs the whole application, models, and has been measured on a tablet ([faces.md §12.1](faces.md), [technical-debt.md TD-1](technical-debt.md)). What is missing is the platform contract around it. -**FR-PLAT-AND-1 is tagged and should not be relied on.** The requirement demands that library access -be obtained *exclusively* through the Storage Access Framework. There is no SAF code: no -`ACTION_OPEN_DOCUMENT_TREE`, no `takePersistableUriPermission`, no `DocumentsContract`. The two tags -rest on a `SourceRef::Document` variant that nothing constructs and a volumes helper, which is the -"plumbing a future feature would use" case [CONTRIBUTING.md](../CONTRIBUTING.md) and -[code-health.md CH-4](code-health.md) both warn about. Android reaches a library through a Nextcloud -account or a folder, over paths, like the desktop. +**FR-PLAT-AND-1 is untagged, and what it was tagged for was intent rather than code.** +The requirement demands that library access be obtained *exclusively* through the Storage Access +Framework. There is no SAF code: no `ACTION_OPEN_DOCUMENT_TREE`, no `takePersistableUriPermission`, +no `DocumentsContract`. Its two tags rested on a `SourceRef::Document` variant constructed only +inside `#[cfg(test)]` — `LocalStorage::open` refuses it, and the test that proves so is named +`a_reference_of_the_wrong_kind_is_refused_rather_than_guessed_at` — and on +`dr_plat::imports_supported`, which *returns false on Android* and whose own documentation says it +"stops being false when a SAF implementation lands". The second tag documented the absence of the +thing it was counted as evidence for. Both have been removed; this is the "plumbing a future feature +would use" case [CONTRIBUTING.md](../CONTRIBUTING.md) and [code-health.md CH-4](code-health.md) both +warn about. Android reaches a library through a Nextcloud account or a folder, over paths, like the +desktop. That has a consequence for the rest of the cluster: **FR-PLAT-AND-2** — detecting the loss of a granted tree permission and marking images offline rather than deleting rows — cannot be built until @@ -220,10 +225,20 @@ the host is invisible to a sandboxed process as well. The fix is an `ashpd` dire `LocalStorage::grant`, not a change to the manifest. No Flatpak has been built here — `flatpak-builder` is not installed — so the permission set is reasoned, not observed. -**NFR-COMPAT-2 — distribution channels.** Unstated, and this is the requirement that makes the -others binding: §4.8 observes that the decision to publish on Play is what turns SAF from a -preference into a constraint. Spike S11, the Play permissions dry-run that would settle it, has not -run. Related, NFR-COMPAT-1's baseline is real but scattered — API 28/36 live in the Android +**NFR-COMPAT-2 — distribution channels. Stated, which is all this requirement asks.** The paragraph +above cites [distribution.md](distribution.md) and it is the same document that answers this: §1 +names five channels and their state — Arch source package and Flatpak in tree, AppImage a v1 channel +whose recipe is not written, F-Droid a v1 channel not yet submitted, and Play explicitly **not** v1. +The requirement is to *state* the channels, and they are stated, including the deferral. + +What remains is the coupling the requirement points at rather than the statement it demands. §4.8 +observes that publishing on Play is what turns SAF from a preference into a constraint, and +distribution.md §6 argues the coupling runs the other way for this project — F-Droid asks nothing +that ARCH §6.9 does not already require. Spike S11, the Play permissions dry-run, has not run, and +until it does that argument is reasoned rather than confirmed. Two of the five channels also exist +as decisions rather than as recipes, and no Flatpak has been built here at all. + +Related, NFR-COMPAT-1's baseline is real but scattered — API 28/36 live in the Android Dockerfile and are checked in CI against the built ELF, which is good — while the items the requirement singles out are missing: whether `shaderFloat16` and 16-bit storage are required (the one it flags as jeopardising R1), minimum RAM, minimum desktop Mesa, and a named reference device @@ -255,9 +270,22 @@ on one control — the parameter slider in `adjust.slint` — and nothing is set all. Everything else in eighteen Slint files is unnamed to AT-SPI and TalkBack. The requirement's own caveat, that Slint's Android accessibility needs verifying, is spike S13, which has not run. -**NFR-A11Y-3 — Colour-independent status.** No compliance work found. This is cheap to satisfy while -a control is being written and expensive to retrofit across forty of them, which is an argument for -doing it as part of the NFR-A11Y-2 pass rather than after it. +**NFR-A11Y-3 — Colour-independent status.** Built where a control exists, and now tagged: the +clipping readout pairs a marker that appears or disappears with a figure in words, the rating strip +is a solid star against an outline in an achromatic palette, the pick/reject mark is a tick against +a cross, and the focus-peaking colour chips say "Red" and "Cyan" rather than showing swatches. Each +of those already carried the reasoning in a comment naming this requirement and simply had no +`TRACES` line. + +Two caveats, because the tag now says more than the evidence does. **Only the clipping clause has a +test** — `a_clipping_figure_distinguishes_none_from_nearly_none`, which pins `<0.1%` apart from `0%` +so the figure cannot contradict the lit marker beside it. The three Slint components are +inspected-and-argued, not asserted, and nothing would fail if a future edit made a star differ only +in tint. **And the requirement's first named example has no interface at all**: catalog colour +labels are a nullable `label INTEGER` column on the versions table and are set and shown nowhere, so +the clause about them is untestable rather than satisfied. That clause closes when the label UI is +built, not before, and it should be built with a shape from the outset — which is the same argument +as below, for doing this alongside NFR-A11Y-2 rather than after it. --- @@ -277,11 +305,14 @@ well optimised — ETag pruning under FR-NC-4 turns an unchanged 50k library int gap is narrower than it reads. It is the *first* build against a large remote library that pays, and that is the moment a new user meets. -**FR-CAT-13 — XMP interoperability, tagged and not met.** Read and write standard XMP sidecars. The -single tag sits on `keywords.rs`, which stores keywords; no XMP is parsed or written anywhere in the -tree, and `dr-export`'s metadata module says so about its own half ("neither is read by `dr-decode` -today"). Listed here rather than silently, because a tag makes a gap invisible and this one is -load-bearing for interoperating with the editors FR-CAT-14 imports from. +**FR-CAT-13 — XMP interoperability, untagged and not met.** Read and write standard XMP sidecars. +Its single tag sat on `keywords.rs`, which stores keywords in the catalog and mentions `dc:subject` +in a comment about what a keyword's text is *for*; no XMP is parsed or written anywhere in the tree, +and `dr-export`'s metadata module says so about its own half ("neither is read by `dr-decode` +today"). The tag has been removed — `dr-preset-xmp` is not the counter-example it looks like, being +a reader of Lightroom *presets* under FR-DEV-6, which is a different file and a different purpose. +Listed here rather than silently, because a tag makes a gap invisible and this one is load-bearing +for interoperating with the editors FR-CAT-14 imports from. --- @@ -320,12 +351,19 @@ fixed before spike S9", because S9 both validates R1 and calibrates what toleran The threshold was never fixed and S9 has not run, so R1 currently has no acceptance criterion at all — there is nothing a test could assert. -Worse, the matrix reports R1 as *covered*. Both of its tags are string literals inside the -traceability tool's own unit tests (`tools/traceability/src/lib.rs`), which the tool scans along with -everything else, because a fixture demonstrating tag extraction is indistinguishable from a tag. -NFR-OPS-1 is covered the same way, from a tag on `compute_coverage` — and no rotating, size-capped -on-disk log exists; logging goes to stderr and logcat. These are two of the cases -[CONTRIBUTING.md](../CONTRIBUTING.md) already warns about, now named. +The matrix used to report R1 as *covered*, and what covered it was two string literals: fixtures +inside the traceability tool's own unit tests, which the tool scans along with everything else, +because a fixture demonstrating tag extraction was indistinguishable from a tag. The extractor now +asks where the tag sits — a tag is the first word of a comment, not a string appearing anywhere on a +line — and R1 is untagged again, which is the honest reading while it has no acceptance criterion to +tag anything against. +NFR-OPS-1 was covered by tags that were real rather than fixtures, which is the worse case of the +two: one on `compute_coverage` and one on the gesture extractor, both on the traceability tool. A +coverage calculation and a documentation generator are not diagnostics under any reading, and the +requirement asks for a rotating, size-capped on-disk log in the XDG state directory, credential +redaction, and a consented diagnostics bundle. None of that exists — logging goes to stderr and +logcat — so both tags have been removed and NFR-OPS-1 is untagged. It is the case +[CONTRIBUTING.md](../CONTRIBUTING.md) warns about in its own words: a tag proves a tag exists. **R2 — Efficient display of huge RAW libraries.** Its acceptance criterion contains "*(figure TBD)*" — the scroll velocity below which no cell may render as a placeholder — and asks for a stated diff --git a/docs/requirements.md b/docs/requirements.md index b683e74..b238dc1 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -55,7 +55,7 @@ These are the user's stated requirements, restated as testable criteria. | **R2** | Efficient display of huge RAW libraries | A 50,000-image catalog scrolls at 60fps sustained, with a stated prefetch margin and cache-hit rate sufficient that no cell renders as a placeholder at a scroll velocity of *(figure TBD)* rows/second. Catalog opens in under 2s. | | **R3** | Make a RAW beautiful at 8-bit output | Full non-destructive develop chain at high internal precision, with camera input profiles (FR-DEV-3e) and a colour-managed path to 8/16-bit export. | | **R4** | HW acceleration and parallelism | All per-pixel work runs on GPU compute. CPU work (decode, I/O) is parallelised across cores. The UI executor never blocks on image work (NFR-ARCH-1). | -| **R5** | Work on downscaled proxies for display | Display pipeline operates at viewport resolution, not source resolution. Only visible tiles are computed; panning recomputes only newly exposed tiles. | +| **R5** | Work on downscaled proxies for display | Display pipeline operates at viewport resolution, not source resolution. The tiling clauses this criterion used to carry have been struck — see below. | | **R6** | Nextcloud integration | Browse, download, and upload images and edit metadata against a Nextcloud instance, offline-capable. | **On R1's tolerance.** An earlier draft required output to be *bit-identical* across platforms. @@ -72,6 +72,31 @@ Where genuine bit-identity is required — cache keys, edit-graph hashing (§5.2 applies to *integer* operations on CPU-side state, which are deterministic, never to GPU float results. +**On R5's tiling.** An earlier draft added two clauses to R5's criterion: *"only visible tiles are +computed; panning recomputes only newly exposed tiles"*. They have been struck, and the reason is +the same one FR-DSP-2 was rewritten for rather than implemented: +[frame-budget.md](frame-budget.md) measured it. + +R5's actual demand is met and tested. The display pipeline works at viewport resolution: +`Framing::view` shrinks the sampled region while the render target keeps its size, so zooming raises +the resolution the pipeline works at rather than magnifying pixels already drawn, and +`core/dr-gpu/tests/zoom_resolution.rs` establishes it as a pixel equality rather than an impression +of sharpness. + +Tiling is a different claim, and it was written in as though it were the mechanism by which the +first one is achieved. It is not. Recomputing the *entire* 4K viewport costs 4.5 ms of a 16 ms +budget, so a perfect tile cache saves at most that, in exchange for a cache keyed by +`(VersionId, tile, zoom, graph_hash_prefix)` that has to stay correct across every parameter change +in the graph — a large correctness surface bought with a small number. And for the one stage that +does miss the budget, tiling makes it worse: that stage is a convolution, and a tiled convolution +reads a halo per tile, so at the 52 px radius measured at 4K a 256 px tile would read (256+104)² +taps instead of 256², very nearly twice the work. + +The intent behind the struck clauses — that the display path must not do work proportional to the +source image — survives in the clause that remains, which is the honest statement of it. Tiling +stays where FR-DSP-2 puts it: a scheduling concern for export and thumbnailing, both of which +already run off the frame path. + --- ## 3. Functional requirements @@ -181,10 +206,34 @@ destructive action, since that figure is what tells the user whether they meant CR3), Nikon (NEF), Sony (ARW), Fujifilm (RAF, including X-Trans), Panasonic (RW2), Olympus (ORF), Adobe DNG. Additional formats are a coverage goal, not a launch blocker. -**FR-RAW-2 — Decoder abstraction.** RAW decoding sits behind a trait taking a `SourceRef` -(FR-CAT-1a), not a filesystem path, so the same decoder works over a local file, an Android SAF -document, or a byte range fetched from Nextcloud. A second implementation may be added for broader -camera coverage without changing callers (D2). +**FR-RAW-2 — Decoder abstraction.** RAW decoding takes **bytes**, never a filesystem path and never +a reference it would have to resolve. Resolving a `SourceRef` (FR-CAT-1a) to bytes is +`Storage::open`'s job and happens at the caller, so the same decoder works over a local file, an +Android SAF document, or a byte range fetched from Nextcloud. A second implementation may be added +for broader camera coverage without changing callers (D2). + +*On the change of mechanism.* This clause used to require "a trait taking a `SourceRef`". The +purpose — that no decoder API takes a path, so nothing in the decode path assumes a filesystem — +is met and is not in question: `dr_decode::decode` takes `&[u8]`, and there is no path-based entry +point in the crate. The mechanism was wrong, and stating it that way would have made the design +worse. + +A `SourceRef` is opaque by construction; the only thing that turns one into readable bytes is +`Storage`, in `platform/dr-plat`. A decoder taking a `SourceRef` would therefore have to take a +`Storage` alongside it, which moves retry, permission loss and remote fetching inside the decoder +and leaves it constructible only where a `Storage` exists. Bytes in, image out, is both narrower and +more portable: the decoder has no idea where its input came from, which is the property this +requirement is actually asking for. + +It also serves the Nextcloud case better rather than worse, which is the one a byte-oriented API +looks like it would lose. `dr_decode::HEADER_BYTES` declares how much of a file the decoder needs to +read metadata, and `import.rs` fetches exactly that range through `Storage::read_range` before +calling `dr_decode::metadata`. The decoder states its requirement and the storage layer satisfies +it; a decoder holding its own `SourceRef` would have had to implement the range policy itself. + +What is genuinely not built is the trait. There is one decoder, reached through free functions, so +"without changing callers" is a claim nothing yet tests. The clause stands as written and is +outstanding work, not a satisfied one. **FR-RAW-3 — Sensor data handling.** Correctly apply per-camera black/white levels, CFA pattern identification, and camera-native colour matrices. Demosaic quality shall be selectable, with at @@ -493,8 +542,24 @@ region. ### 3.6 Export -**FR-EXP-1 — Formats.** Export to JPEG, PNG, TIFF (8 and 16-bit), and AVIF or JPEG XL. Quality, -chroma subsampling, and bit depth are configurable. +**FR-EXP-1 — Formats.** Export to JPEG, PNG, and TIFF (8 and 16-bit). Quality, chroma subsampling, +and bit depth are configurable. + +**AVIF and JPEG XL are post-v1** and are not part of this requirement's acceptance. Either may still +be listed in the settings page before its encoder exists, on one condition: choosing it shall fail +with a typed error naming the format, never with a file. The test +`every_offered_format_either_encodes_or_explains_itself` walks every format the page offers and +enforces exactly that, so a format cannot be added to the picker and quietly reach an encoder that +does not handle it. + +*On splitting this requirement.* It read as one undifferentiated list — "JPEG, PNG, TIFF (8 and +16-bit), and AVIF or JPEG XL" — which left it neither met nor unmet. Three formats, both TIFF +depths, and the configurability clause are built, encode, embed their profile, and are tested; the +fourth item is a deliberate deferral, and the encoders for it are the two with the least settled +library support. Fused into one sentence, the only choices were a tag asserting something untrue or +no tag at all, and the second is the worse of the two: it would have removed the register's record +of four-fifths of a requirement that is finished. The deferral is now stated where it can be read as +a decision rather than inferred from an error variant. **FR-EXP-2 — Colour space.** Export in a selectable output colour space (sRGB, Display P3, Adobe RGB, ProPhoto), with the correct ICC profile embedded. @@ -1305,13 +1370,25 @@ These are targets to design against and measure, on the reference desktop | **NFR-P14** | Focus peaking overlay ready | < 100 ms after preview | < 150 ms | | **NFR-P15** | Drawn mask stroke → visible (ARCH §6.11) | < 16 ms, no cursor lag | < 16 ms | | **NFR-P10** | Touch gesture → visual response | < 16 ms | < 16 ms | -| **NFR-P11** | Layout class transition (window resize) | No dropped frames, no state loss | n/a | +| **NFR-P11** | Layout class transition (window resize) | No dropped frames. No loss of *photographic* state: the open image and version, the selection, scroll position, the in-progress edit and its undo history, and the current mode. Panel disclosure is explicitly exempt — see below | n/a | | **NFR-P12** | Warm-start shader pipeline setup (cached) | < 100 ms | < 100 ms | Every target above requires a stated measurement method, workload, and pass threshold before it is testable. NFR-P8 in particular must state whether it measures RSS inclusive or exclusive of GPU allocations, and whether it holds after SQLite's page cache warms on a 50k catalog. +**On what NFR-P11 means by state.** It said "no state loss", which the implementation contradicts on +purpose, so the requirement has been made specific rather than left to be read as forbidding +something it should not. `apply_layout_class` discards the user's panel open/closed choices when the +class changes, and the argument for that is sound: a choice made in landscape answers a different +question from the one portrait asks, and carrying it across is how a photographer ends up with a +232 px sidebar on a screen with no room for it and no memory of having asked for it. Panel +disclosure is a *default*, re-derived per class, with the user's disagreement remembered only within +the class where it was expressed. + +Everything the photographer produced or navigated to is a different matter, and none of it may be +touched by a resize. That is the list in the criterion, and it is the testable half. + **Performance regressions fail the build.** §9's benchmark suite runs per-commit; a regression beyond a stated tolerance is a build failure, not a notification. Performance work rots otherwise. diff --git a/platform/dr-plat/src/volumes.rs b/platform/dr-plat/src/volumes.rs index 0f0faa1..4d5a942 100644 --- a/platform/dr-plat/src/volumes.rs +++ b/platform/dr-plat/src/volumes.rs @@ -59,7 +59,7 @@ impl Volume { } } -/// TRACES: FR-CAT-10 | FR-PLAT-AND-1 | NFR-PORT-1 +/// TRACES: FR-CAT-10 | NFR-PORT-1 /// Whether an import can reach a card on this platform at all. /// /// **Not the same question as "did [`volumes`] find anything".** An empty list diff --git a/tools/traceability/src/gestures.rs b/tools/traceability/src/gestures.rs index 3d10f00..c0783bf 100644 --- a/tools/traceability/src/gestures.rs +++ b/tools/traceability/src/gestures.rs @@ -1,4 +1,4 @@ -//! TRACES: FR-UI-4 | NFR-OPS-1 +//! TRACES: FR-UI-4 //! Interaction documentation, extracted from the code that implements it. //! //! # Why this exists at all diff --git a/tools/traceability/src/lib.rs b/tools/traceability/src/lib.rs index bbc8e29..fa648fc 100644 --- a/tools/traceability/src/lib.rs +++ b/tools/traceability/src/lib.rs @@ -19,6 +19,19 @@ //! count as the numerator is precisely what lets a ratio exceed 100%, since //! a tag naming a deleted requirement would count as covered. Such tags are //! reported as orphans instead. +//! +//! # And why the extractor is fussy about where a tag sits +//! +//! A third rule, learned the same way. `SOURCE_ROOTS` includes `tools`, so this +//! crate scans itself, and the fixtures below demonstrating tag extraction were +//! read as tags: R1 — cross-platform output within a bounded tolerance — was +//! reported implemented on the strength of two string literals in a unit test. +//! +//! 3. **A tag is a comment whose first word is `TRACES:`**, not a line in which +//! the string appears. See `tag_body`. It is a rule about position rather +//! than about string literals, because `schema.rs` writes six real tags +//! *inside* string literals — the SQL it embeds is commented with `--` — so +//! an extractor that refused string literals would lose more than it saved. use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; @@ -182,16 +195,43 @@ pub fn is_requirement(id: &str) -> bool { REQUIREMENT_TYPES.contains(&type_of(id)) } +/// Comment openers a tag may be introduced by. +/// +/// `--` is here for the SQL embedded in `schema.rs`, which carries real tags +/// inside Rust string literals; `#` for YAML; `*` for the continuation lines of +/// a block comment. +const COMMENT_OPENERS: &[&str] = &["///", "//!", "//", "