Merge: stop the matrix counting the tool's own test fixtures as coverage
A tag was any line containing the string, so the traceability tool scanned tools/ -- its own source -- and its test fixtures became coverage. R1 and NFR-OPS-1 were tagged on nothing but that. Four requirements had sites that were never implementations: the same fixtures also fed FR-CAT-1, FR-CAT-2 and NFR-P1, gestures.rs fed FR-UI-4 from a push_str, and build.rs fed FR-DEV-3a and FR-DEV-3c from the tag it emits into generated code. A tag is now a comment whose first word is TRACES:. Excluding cfg(test) was rejected because tags on tests are this project's recommended practice, and keying on string literals was impossible because schema.rs carries six genuine tags inside Rust strings -- the SQL it embeds is commented with --. Four tags removed as unearned: FR-CAT-13 (no XMP is parsed or written anywhere), FR-PLAT-AND-1 (SourceRef::Document is constructed only in test modules, and the second tag sat on imports_supported, which documents the absence). Two added as earned: NFR-A11Y-3, which was built and untagged, and FR-PLG-8's preservation clause. Four requirements amended rather than built, each with its reasoning: R5's tiling clauses struck, NFR-P11 naming the state that must survive, FR-RAW-2's mechanism reworded to bytes-in, FR-EXP-1's AVIF and JPEG XL stated as post-v1. Coverage falls 122 to 120 of 179, and means more than it did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -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.
|
||||
|
||||
@@ -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\
|
||||
|
||||
@@ -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
|
||||
|
||||
+63
-25
@@ -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
|
||||
|
||||
+85
-8
@@ -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.
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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] = &["///", "//!", "//", "<!--", "/*", "*", "--", "#"];
|
||||
|
||||
/// The body of a tag comment, if this line is one.
|
||||
///
|
||||
/// **A tag is a comment whose first word is `TRACES:`** — not any line in which
|
||||
/// the string appears. The tool scans `tools/`, which is its own source, so
|
||||
/// without this rule its unit-test fixtures are tags: a one-line literal in
|
||||
/// `context_looks_forward_then_backward` had R1 reported as implemented, and
|
||||
/// the register believed it. That is not a quirk of this crate. `build.rs`
|
||||
/// emits a tag into generated code from a string literal, and any test
|
||||
/// anywhere that exercises the extractor would do the same.
|
||||
///
|
||||
/// The rule does not merely exclude string literals — it cannot tell one from a
|
||||
/// comment, and `schema.rs` proves it must not try, since its `-- TRACES:` tags
|
||||
/// live inside string literals and are entirely real. What it asks instead is
|
||||
/// where on the line the tag sits: a tag written to be read is the first thing
|
||||
/// in its comment, and a tag quoted inside an expression never is.
|
||||
fn tag_body(line: &str) -> Option<&str> {
|
||||
let line = line.trim_start();
|
||||
let after_opener = COMMENT_OPENERS.iter().find_map(|o| line.strip_prefix(o))?;
|
||||
after_opener.trim_start().strip_prefix("TRACES:")
|
||||
}
|
||||
|
||||
/// Extract every `TRACES:` tag from a source file's text.
|
||||
pub fn extract_from_text(text: &str, path: &str) -> Vec<TraceEntry> {
|
||||
let lines: Vec<&str> = text.lines().collect();
|
||||
let mut out = Vec::new();
|
||||
|
||||
for (idx, line) in lines.iter().enumerate() {
|
||||
let Some(pos) = line.find("TRACES:") else {
|
||||
let Some(tail) = tag_body(line) else {
|
||||
continue;
|
||||
};
|
||||
let tail = &line[pos + "TRACES:".len()..];
|
||||
let ids = parse_ids(tail);
|
||||
if ids.is_empty() {
|
||||
continue;
|
||||
@@ -265,7 +305,6 @@ fn trim_body(line: &str) -> String {
|
||||
///
|
||||
/// This signature is the fix for the 158% bug: `defined` is required, so there
|
||||
/// is nowhere for a frozen denominator to hide.
|
||||
/// TRACES: NFR-OPS-1
|
||||
pub fn compute_coverage(traced: &BTreeSet<String>, defined: &DefinedRequirements) -> Coverage {
|
||||
let traced_reqs: BTreeSet<&String> = traced.iter().filter(|id| is_requirement(id)).collect();
|
||||
|
||||
@@ -309,6 +348,25 @@ pub fn compute_coverage(traced: &BTreeSet<String>, defined: &DefinedRequirements
|
||||
/// implementing it is generated into `OUT_DIR`, which is not scanned and could
|
||||
/// not be linked to from the report if it were. Without this a node would have
|
||||
/// nowhere to record the requirement it satisfies.
|
||||
///
|
||||
/// # What is deliberately absent, and why widening this is the wrong fix
|
||||
///
|
||||
/// `AndroidManifest.xml`, the Flatpak manifest, the Dockerfile and the CI
|
||||
/// workflows all carry requirements — SAF grants, sandbox permissions, the API
|
||||
/// levels NFR-COMPAT-1 names — and none of them can hold a tag. That reads like
|
||||
/// a gap in this list. It is not one.
|
||||
///
|
||||
/// A tag proves that a tag exists, not that the file under it does the thing,
|
||||
/// and a declarative manifest is the case where the difference bites hardest:
|
||||
/// an intent filter can be deleted and the tag above it still says the
|
||||
/// requirement is met. The convention already in the tree answers it better —
|
||||
/// a Rust test `include_str!`s the file and asserts what must be in it, and
|
||||
/// the tag goes on the test. That satisfies what CONTRIBUTING.md asks of any
|
||||
/// tag, that it name something a test would fail without, which a comment in a
|
||||
/// manifest never can.
|
||||
///
|
||||
/// So the list stays as it is, and a config file is tagged through the test
|
||||
/// that reads it.
|
||||
pub const SOURCE_SUFFIXES: &[&str] = &[".rs", ".slint", ".wgsl", ".yaml"];
|
||||
|
||||
/// Directories never scanned.
|
||||
@@ -477,16 +535,23 @@ This is discussed in FR-CAT-1 and also FR-CAT-1 again.
|
||||
|
||||
#[test]
|
||||
fn extracts_tags_with_both_separators() {
|
||||
// **The ids here are UT and IT deliberately.** A multi-line literal is
|
||||
// the one fixture shape `tag_body` cannot rule out: its lines begin
|
||||
// with `///` in the file as well as in the string, so this really is a
|
||||
// tag as far as any line-oriented reader can tell. Naming a
|
||||
// requirement here would report it implemented by the traceability
|
||||
// tool. UT and IT are excluded from coverage by `is_requirement`, so
|
||||
// the fixture can be as tag-shaped as it likes and still cost nothing.
|
||||
//
|
||||
// The requirement id *shapes* are exercised by `darkroom_id_shapes_parse`
|
||||
// below, which needs no `TRACES:` at all to do it.
|
||||
let src = "\
|
||||
/// TRACES: FR-CAT-1, FR-CAT-2 | NFR-P1
|
||||
/// TRACES: UT-001, UT-002 | IT-003
|
||||
pub fn scan() {}
|
||||
";
|
||||
let traces = extract_from_text(src, "x.rs");
|
||||
assert_eq!(traces.len(), 1);
|
||||
assert_eq!(
|
||||
traces[0].requirements,
|
||||
vec!["FR-CAT-1", "FR-CAT-2", "NFR-P1"]
|
||||
);
|
||||
assert_eq!(traces[0].requirements, vec!["UT-001", "UT-002", "IT-003"]);
|
||||
assert_eq!(traces[0].line, 1);
|
||||
assert_eq!(traces[0].context, "pub fn scan()");
|
||||
}
|
||||
@@ -502,6 +567,71 @@ pub fn scan() {}
|
||||
assert_eq!(back[0].context, "pub fn render()");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_quoted_tag_is_not_a_tag() {
|
||||
// What made the tool report `R1` as implemented: a fixture in this very
|
||||
// module, scanned along with everything else, because a line mentioning
|
||||
// `TRACES:` was taken for a line carrying it.
|
||||
let quoted = extract_from_text(
|
||||
r#"let s = "// TRACES: FR-CAT-1"; // and a real one below"#,
|
||||
"x.rs",
|
||||
);
|
||||
assert!(quoted.is_empty(), "a tag inside an expression is not a tag");
|
||||
|
||||
// Emitted into generated code, which is the `build.rs` case.
|
||||
let emitted = extract_from_text(r#" "/// TRACES: FR-DEV-3a\n","#, "x.rs");
|
||||
assert!(emitted.is_empty(), "a tag being written out is not a tag");
|
||||
|
||||
// Prose about the mechanism, which is what this crate's own doc
|
||||
// comments are full of.
|
||||
let prose = extract_from_text("/// Extract every `TRACES:` tag. FR-CAT-1", "x.rs");
|
||||
assert!(prose.is_empty(), "a tag named in prose is not a tag");
|
||||
|
||||
// And the forms that must keep working: SQL inside a Rust literal,
|
||||
// which is how `schema.rs` tags its migrations, and YAML.
|
||||
let sql = extract_from_text(" -- TRACES: FR-CULL-8\n", "x.rs");
|
||||
assert_eq!(sql[0].requirements, vec!["FR-CULL-8"]);
|
||||
let yaml = extract_from_text("# TRACES: FR-DEV-3a\nid: exposure\n", "x.yaml");
|
||||
assert_eq!(yaml[0].requirements, vec!["FR-DEV-3a"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn this_crates_own_fixtures_cannot_reach_the_register() {
|
||||
// `SOURCE_ROOTS` includes `tools`, so this crate is scanned by the tool
|
||||
// it implements and a fixture below is indistinguishable from a tag on
|
||||
// the code above. `tag_body` closes every shape but one — a multi-line
|
||||
// literal whose lines begin with a comment opener — and this closes
|
||||
// that one, by asking where the tag is rather than what it looks like.
|
||||
//
|
||||
// Tagging the tool's real code is still allowed; two such tags were
|
||||
// wrong on other grounds and were removed, but the rule here is only
|
||||
// that a fixture may not name a requirement. Use a `UT-` or `IT-` id.
|
||||
for (name, src) in [
|
||||
("lib.rs", include_str!("lib.rs")),
|
||||
("gestures.rs", include_str!("gestures.rs")),
|
||||
("main.rs", include_str!("main.rs")),
|
||||
] {
|
||||
let fixtures_begin = src
|
||||
.lines()
|
||||
.position(|l| l.trim_start().starts_with("mod tests"))
|
||||
.map_or(usize::MAX, |i| i + 1);
|
||||
|
||||
for entry in extract_from_text(src, name) {
|
||||
let reqs: Vec<&String> = entry
|
||||
.requirements
|
||||
.iter()
|
||||
.filter(|id| is_requirement(id))
|
||||
.collect();
|
||||
assert!(
|
||||
entry.line < fixtures_begin || reqs.is_empty(),
|
||||
"{name}:{} is a test fixture naming {reqs:?}, which the \
|
||||
register would read as implemented. Use a UT- or IT- id.",
|
||||
entry.line,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_tag_with_no_ids_is_ignored() {
|
||||
let traces = extract_from_text("// TRACES: see the design doc\n", "x.rs");
|
||||
|
||||
@@ -353,6 +353,7 @@ fn fraction(clipped: u32, pixels: u32) -> f32 {
|
||||
}
|
||||
}
|
||||
|
||||
/// TRACES: NFR-A11Y-3
|
||||
/// How much of the frame is gone, as a figure rather than a colour.
|
||||
///
|
||||
/// NFR-A11Y-3 asks that no status be carried by hue alone, and this is the
|
||||
@@ -364,6 +365,8 @@ fn fraction(clipped: u32, pixels: u32) -> f32 {
|
||||
/// Distinguishes "none" from "not none but under a tenth of a percent": those
|
||||
/// are different answers, and rounding the second to `0.0%` would tell a
|
||||
/// photographer their highlights were safe when the indicator beside it is lit.
|
||||
/// `a_clipping_figure_distinguishes_none_from_nearly_none` is the test that
|
||||
/// would fail if this became a colour again.
|
||||
fn percentage(clipped: u32, pixels: u32) -> String {
|
||||
if pixels == 0 || clipped == 0 {
|
||||
return "0%".into();
|
||||
|
||||
@@ -134,6 +134,7 @@ component Trace inherits Rectangle {
|
||||
}
|
||||
}
|
||||
|
||||
// TRACES: NFR-A11Y-3
|
||||
// A clipping readout: a lit marker and a figure.
|
||||
//
|
||||
// **Two affordances for one fact, and NFR-A11Y-3 is why.** No status in this
|
||||
|
||||
@@ -649,6 +649,7 @@ export component PhotoRoll inherits Rectangle {
|
||||
}
|
||||
}
|
||||
|
||||
// TRACES: NFR-A11Y-3
|
||||
// A row of five stars, readable at a glance and clickable to set a rating.
|
||||
//
|
||||
// **Filled versus empty carries the meaning, not colour.** NFR-A11Y-3 forbids
|
||||
@@ -796,6 +797,7 @@ export component StarStrip inherits Rectangle {
|
||||
}
|
||||
}
|
||||
|
||||
// TRACES: NFR-A11Y-3
|
||||
// The pick/reject mark.
|
||||
//
|
||||
// A shape rather than a colour, for the same NFR-A11Y-3 reason as the stars:
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
// TRACES: FR-CULL-3
|
||||
// TRACES: FR-CULL-3 | NFR-A11Y-3
|
||||
// The focus-peaking switch, and the two choices it exposes.
|
||||
//
|
||||
// **An instrument, not an operation**, exactly as the histogram above it is:
|
||||
|
||||
Reference in New Issue
Block a user