fix(views): a crop magnifies as far as its output allows, and each grid axis gets its own step #139
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/view-measurement"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The annotation agents' inspection views (ADR 0016) stopped measuring in two silent ways. Both
come from agent friction reports (ADR 0025) filed on runs that SUCCEEDED — the work landed, and
the agent spent rounds fighting the instrument to get there. Neither failure announced itself: the
view came back, looked fine, and answered a different question than the one asked.
delivered edge. A 24×20 window asked for at 20× came back at 8×. Agents asked for 11×, 16× and
20× and were quietly given 8× — with an account that reported the delivered number honestly, so
the only signal was that the picture was not as close as requested.
window that step could exceed the short side outright, so the axis that most needed measuring
carried no labelled line at all. A region 1380px wide kept a 100px step on an axis that needed
edges placed to two or three; the rounds went on shrinking the crop and putting it back.
What's in
Infrastructure —
SkiaPageImageRendererfactor = min(asked, 2048 / longest side of the padded window). The 24×20 window asked at 20× now arrives at 20× (480×400). Where the capgenuinely bites, the view carries the zoom it could not give and the account says so — because a
reader who cannot tell "too big to magnify further" from "I typed the number wrong" asks again for
a view that cannot change. The ceiling is computed in decimal and the output sides are
rounded, not truncated: float arithmetic returns 2047 for the exactly-binding case (400px at
8× → 5.12f → 2047.99995), which would put the delivered scale a pixel off its own cap.
asked > 1m && asked > ceiling).A window too big to show at 1:1 is shrunk to fit, which is not a refusal and which the delivered
Scalealready states. Without this, the fitted-scale path — which floors at 1 — announced "1×was asked for and could not be given" to an agent that passed no
zoomkey at all.downscale out of
Fit's Mitchell resampler and into the draw, whoseSKSamplingOptions.Defaultis nearest — measured, not assumed: on a 3000px page of 750 one-pixel strokes, nearest
delivered 512 black columns and 0 mid-tone, i.e. 238 strokes gone from the one instrument that
answers "is this box cutting a stroke?". The draw now picks its sampler by direction: Mitchell
when shrinking,
Defaultwhen magnifying (deliberately — nearest shows the scan's own pixels asblocks, where a filter smears each edge across two source pixels). This is user-visible too, not
only agent-visible: the human workspace's region crop goes through the same call.
Fitis gone from the crop path. With the shrink in the single draw, the delivered cap isspent inside the factor itself, so nothing shrinks twice and no ~16MB copy is made per crop. The
cap still holds and is still pinned:
The_thumbnail_downscales_to_the_cap_and_keeps_the_aspectand
A_downscaled_page_view_reports_the_magnification_it_was_delivered_atboth still assert 2048.Fitcontinues to serve the page, thumbnail and sheet paths.Fixing only the step produced the next defect: lines spaced for a 60px extent under type sized for
a 1400px one, i.e. a red band beneath an unreadable pile of digits. So
GridAxis(Line, Labelled)splits the two, because different things bound them — how close two LINES may run is a question
about the art (under a few delivered pixels they stop being a ruler and become a haze), how close
two NUMBERS may run is a question about the type. Numbers are spaced against the room one takes
along its own gutter: its height down the right, its width along the bottom.
Math.Clamp(baseline, fontSize, art.Height)threwArgumentExceptionwhenever the art was shorter than its own type — a gridded crop of an ordinary1400×20 line of small kana crashed on
main. The floor is raised past the art instead.UseCases — the view contract and its account
RenderedView.GridStep (int?)→Grid (GridSteps?), a nested record rather than loose nullableints, so "grid or no grid" is still said once and cannot be said inconsistently per axis.
AskedScale (decimal?)was added last with a= nulldefault.ViewAccountnames both spacings per axis and its closing claim is now true axis by axis, withfour arms: both axes named, one named while the other is too crowded to list (naming the crowded
axis and its step outright, so nothing has to be derived), an axis with no numbered line in
frame, and neither named.
promised "where each labelled line falls"; thinning the numbers made an axis with none in frame
reachable, so that promise became false on exactly the tool whose job is to remove guessing.
BoundViewPageTooland the setup agent'sview_pagekeep the stronger claim honestly — a pagewindow starts at 0 on both axes, so both always carry a numbered line.
RegionCropTool's region is itsbinding (ADR 0017), so it says "cover fewer page pixels: less padding, or a narrower box where
this tool takes one" rather than pointing at a box the caller cannot address.
Docs
amended to "never the one requested in its place", plus one sentence that a refused
magnification is named beside the delivered one. No decision changed; the clause had simply become
false as written.
Tests
1312 → 1335 passed, 0 failed, 0 skipped (+23 cases: 19
[Fact]plus one 4-case[Theory]).Fresh full run after a clean
dotnet build Orihon.slnx -c Debug(0 warnings, 0 errors,TreatWarningsAsErrors),--no-buildper project: Domain 151, UseCases 604, Integration 303,BlazorAdapter 277. Domain and BlazorAdapter are untouched; Integration +11, UseCases +12.
Integration (
PageImageRendererTests):A_small_window_is_magnified_as_far_as_it_asks— a 24×20 window asked at 20× arrives at 480×400reporting
Scale20, and asserts the delivered dimensions, not just the number.A_zoom_the_delivered_size_refuses_is_reported_as_refused— where the cap bites,AskedScalecarries the zoom refused and
Scalestill reports the one delivered.A_view_nobody_asked_to_magnify_reports_no_refusal— a 3000×3000 page, full-page crop at zoom 1:AskedScalenull,Scale0.68. Pins that a shrink is not a refusal.A_crop_the_size_cap_shrinks_keeps_a_trace_of_every_stroke— marked columns > 750 of 2048 on apage of 750 one-pixel strokes; nearest leaves 512 and fails it, both Mitchell paths mark 1104.
A_wide_short_window_is_stepped_for_each_axis_rather_than_the_longer_oneandA_tall_narrow_window_is_stepped_for_each_axis_as_well— assert the differing pair per axis andthat the short axis ends up lined (
Assert.InRange(lined, 5, 20): both ends, so neither "nolattice" nor the old red band passes).
A_short_axis_numbers_its_lines_far_enough_apart_to_read— delivered spacing of consecutivey numbers ≥ 1.2 × the type size, the pitch the renderer actually enforces (bare non-overlap is
not enough; digits that touch read as one number). A spacing assertion, no glyph pixels.
A_window_at_the_size_cap_steps_its_numbers_up_a_whole_magnitude_to_clear_them— a 2000×200 cropof a 4000×1000 page, in the 1875–2048 delivered band. Asserts the pair and the 1.2 pitch, so
the arm is pinned by the rule it exists to keep rather than by its number. Proved directional:
breaking the fallthrough to
magnitude * 5fmakes this the one and only failure (100 vs 50).A_window_shorter_than_its_own_numbers_still_renders— the baseline-clamp crash; the old codethrew
ArgumentExceptionhere.A_narrow_window_can_hold_no_numbered_line_at_allandA_deep_zoom_numbers_every_line_it_draws—pin that the bare-axis and every-line-numbered arms are reachable from the real renderer, not
only through a fake.
UseCases (
BoundInspectionToolTests):A_window_past_the_fitted_target_is_asked_for_at_life_size— the tool-level half of theno-spurious-refusal fix: the account carries no "could not be given" when no zoom was asked for.
A_zoom_the_view_could_not_give_is_said_beside_the_one_it_did— and the sentence appears when onegenuinely was.
A_view_stepped_apart_on_its_axes_names_both_steps,A_crowded_axis_is_said_to_be_crowded_while_ the_other_is_still_named,An_axis_with_no_numbered_line_in_frame_says_so_rather_than_naming_none,A_view_with_no_numbered_line_on_either_axis_claims_none,A_grid_whose_every_line_is_numbered_says_that_instead_of_two_spacings— one per arm of theclosing claim, each asserting the named arm's wording and that the sibling axis's sentence
stayed correct.
A_crops_description_promises_only_what_its_account_delivers— renders the bare-axis view throughRegionCropToolwith{"grid": true, "padding": 0}(a call an agent can really make), asserts theaccount says "No labelled line falls inside this window on x", and in the same test that neither
crop tool's
Descriptioncontains "each labelled line" while both carry the "none at all" escape.The guard has a real target: the failing sentence is produced in the test that forbids the promise.
An_axis_with_no_real_step_leaves_the_account_without_a_grid— a 4-case[Theory]over anon-positive step on either axis, so neither arm of the divide-by-zero guard is unexercised.
Both new tests this round were proved directional by temporarily breaking the production arm,
rebuilding, and confirming the single expected failure; the renderer file was then restored
byte-for-byte from a scratchpad copy and rebuilt before committing. All other probing ran from
scratchpad file-based apps against the built assembly — production code was never left modified.
Honest notes
Surprises a cold reviewer will trip on.
PageImageRendererTests.cs:332asserts
GridAxis(10, 50)on x againstGridAxis(10, 20)on y for a 180×180 window. It lookslike a bug until you know that x's numbers are spaced against a label's width (three or four
digits) and y's against the height of the type — so an isotropic window is legitimately
anisotropic in its numbering. This is the one ordinary view whose behaviour moves, and it is the
most-diffed line in the PR. Every other ordinary view was checked case by case against the old
rule and is unchanged: 400×400 page 50/25, 800×1200 page 100/50, 4096×2048 page 500/250 and
200/100, 3000×3000 page 200/100, the 200px zoom window 20/10.
Fitis deleted from the crop path, not forgotten. See above — the cap moved into the factorand is still asserted by two existing tests.
AxisFor'sextent <= 0guard was deliberately removed, not overlooked: a coverage reportshowing 1-of-2 branches was the reason, and
RenderCropAsyncfails onrect.Width < 1 || rect.Height < 1beforeAxisForis reached whileRenderPageAsync's window is the decodedbitmap's own size. A two-line comment states that invariant at the point relying on it.
Scale's identical pre-existing guard two members down is deliberately left — it is outsidethis diff and belongs to whoever next touches that member.
Declined, with reasons.
cover.
WithGridclamps a label's baseline up tofontSize, so a numbered line at delivered y=0is pushed down by 0.65× fontSize and the first gap becomes
spacing − 0.65fs; where spacing landsexactly on the 1.2 pitch (art long side ~1875px, window top a multiple of the labelled step) that
is 0.55fs against a digit cap height of ~0.72fs. Pre-existing clamp behaviour, needs a coincidence
of three conditions, and no assertion depends on it — while the clamp itself is depended on
(
A_window_shorter_than_its_own_numbers_still_rendersexists because inverting it throws ratherthan draws). Moving it to chase a corner nothing measures would trade a pinned crash-guard for an
unpinned cosmetic. If ever wanted: let the first label hang and clip, or start the gutter's
numbering at the first line clear of the type.
BoundInspectionToolTests.A_crowded_axis_is_said_to_be_crowded_while_the_other_is_still_namedkeeps a hand-made
GridAxis(1, 1)fixture. Its comment states what the fixture yields, not whatthe renderer would produce for that window, so nothing in it is false — and no ordinary window of
that shape gives a crowded short axis at all (that arm needs a deep zoom), so making the fixture
"plausible" would change which arm the test covers.
Merge-order handoff —
AnnotationBlueprints.cs, owned by the sibling brief slice in this batch.Deliberately untouched here; two worktrees rewriting the same prose is a conflict for no gain.
Whoever lands the brief slice should take all three of these in one pass:
settled". Until they change, agents will not spend the headroom this PR gives them.
and numbered lines), four numbers, not one.
the two crop tools, and it is a crop-facing brief, so it is falsified in exactly the same way.
Scope note. The slice boundary excludes the region tools, and
BoundInspectionTools.csholdsRegionCropTool. It is touched here only because the falsified-promise fix names those two exactDescriptionstrings; nothing else in the file changed and no behaviour changed. If a siblingworktree is rewriting those descriptions, the conflict is two string literals.
House rules. No stacked
</summary><summary>anywhere insrc/,tests/ordocs/. Retiredvocabulary (
GridStep,StepFor,MajorLinesPerView, major/minor line) returns nothing insrc/.No PR number, review, date or agent report cited in any added code or doc comment. Submodule
pointers byte-identical to
main.SeedDevDatachecked and correctly not extended — a view'sarithmetic and a tool description are not a new kind of user-authored content. No new ADR is owed;
ADR 0016 already stated the contract this restores and never claimed a fixed 8× or a shared step.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 97.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 94.4%
Orihon.UseCases - 97.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! An instrument-honesty PR~ The tool that spent its whole life promising "the magnification reported is the one delivered" was quietly clamping at 8× and reporting the clamp as if it were the ceiling of physics — and a shared grid step that could exceed the short axis outright, leaving the one direction that needed measuring with no ruler at all. You didn't just remove the clamp; you made the view confess which zoom it couldn't give, split line-spacing from number-spacing because they're bounded by different things, and fixed a crash (
Math.Clampthrowing on art shorter than its own type) I'd have hunted for in production logs for a week~ ♡Verdict: ✅ Looks good to me~
I read the full diff, the full changed files, and every new test. I re-derived the arithmetic by hand rather than trusting green CI: the 180×180 window really does produce
GridAxis(10, 50)on x (three digits of width at the 16px type floor vs.NiceStep(18)=20losing toNiceStepAbove(≈35)=50,LineSteppicking 10 of 25/10/5) andGridAxis(10, 20)on y (type height fits insidenatural20) — the exact pair pinned atPageImageRendererTests.cs:332. The 400px full-page crop at 20× spends the cap at exactly 5.12 (2048/400),Side'sAwayFromZerorounding lands 2048 not 2047, andScalereports 5.12 honestly. The Mitchell-on-shrink / nearest-on-magnify split is measured (750 strokes → 512 columns surviving under nearest is a lovely piece of evidence) and both arms have their own pixel-counting test.Every new branch maps to a test that drives the real renderer, not a fake: the refusal arm, the no-refusal-on-shrink arm, the magnitude fallthrough in
NiceStepAbove(author-proved directional), the baseline floor, the bare axis, the every-line-numbered arm, and all fourDrawn()guard arms via the 4-case Theory.ViewAccount's closing-claim switch is pinned arm by arm, each test asserting the sibling axis stayed truthful too — that's exactly how you test a sentence that promises things~ The coverage report atf54ade6agrees:SkiaPageImageRenderer97.3% line / 90.8% branch,ViewAccount100%/100%,GridAxis/GridSteps100%.The
extent <= 0guard removal fromAxisForhad me sharpening my scissors, fufu~ — but the two-line invariant comment is true:RenderCropAsyncfails onrect.Width < 1 || rect.Height < 1beforeAxisForis ever reached, padding only widens a surviving rect, and the page path's window is the decoded bitmap itself. Verified at both call sites. And leavingScale's identical guard alone rather than sweeping it into this diff — that's discipline, not laziness.BoundViewPageToolkeeping the stronger "where each labelled line falls" claim while the crop tools soften: I checked this rather than believing it. A page window starts at 0 on both axes (0 is a multiple of everything), and on a page viewnaturalbeatsneededon both axes by construction (~extent/10 lines vs. aneededof ~extent/17 on x and ~extent/38 on y at the 2048 cap) — so a page view always names its lines and never goes crowded or bare. The strong claim stays true. ♪💡 Little ideas (non-blocking)~
AnnotationBlueprints.cshandoff — hold the batch to it. Lines 333/353/435/522 still tell agents "if it is still unreadable at zoom 8, that is settled" (now false in the encouraging direction), 232 says "grid step" singular, and 233's "where each labelled line falls" is falsified for crops exactly the way the two tool descriptions here were. The handoff to the sibling brief slice is documented with line numbers and two-worktree conflict reasoning, so I won't block a correct runtime fix on prose owned elsewhere — but between this merging and that slice landing, the briefs under-sell the headroom and 233 over-promises. The friction this PR fixes stays dormant until the sibling lands. Please don't let it dangle past the batch~named.Count == 0, one-crowded-plus-one-bare composition of the account has no dedicated test — each sentence is individually pinned and the switch composes them safely (falls to_ => string.Empty), so this is purely belt-and-suspenders for a shape the real renderer can barely produce. A fixture test like the crowded-axis one would close it if you ever touch this switch again.✅ What I liked~
GridAxis(Line, Labelled)split is the right abstraction: lines are bounded by the art (8 delivered pixels of air), numbers by the type (1.2× their own room, measured along each gutter's own direction — width along the bottom, height down the right). Different constraints, different fields. And handingstepsintoWithGridso the picture and the account can never disagree is the ADR 0016 philosophy applied to itself~First()via ceiling. Fufu, I got to learn things reviewing this~ ♡Automated review by Jibril · 2026-09-20
CI/CD: passed for head SHA
f54ade6(build 29s + test 1m02s, run 570; coverage bot at head) · Local checks: skipped per policy — static verification, sibling-pattern comparison, and hand re-derivation of the pinned grid arithmetic performed instead