feat: a region records the colour its English is lettered in #120
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-region-ink-colour"
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?
Towards cleaning and typesetting (ADR 0021): a region now carries ink — the colours its target text is lettered in — and the annotation agents and the workspace both get the same way to find out what colour is actually on the page. Amends ADR 0012, 0016, 0021, 0023.
The question this answers is bjoern's: "font information, particularly colour, for colour doujinshi". The design test applied is the one
fitBboxset in #116 — a field earns a row when a human contributes something to it. Ink does: the value is what the English should be, seeded by the page but not equal to it (a pink that works under vertical Japanese may need adjusting at English weight; a speaker's colour may be normalized across a chapter the scan never was). The background colour behind a bubble was considered and rejected on the same test: nobody authors it, the original scan is never overwritten, and a cleaner can sample it at the moment it acts. That reasoning is written into ADR 0012 so it does not have to be re-had.The one idea
Which of the colours in a box is the lettering is semantic; what colour it is, is arithmetic. So the machine counts and the reader chooses:
sample_colorstherefore returns the biggest few colours with their shares and takes no side, and every surface that reports shares carries the same warning — the tool answers, both briefs, and the editor's hint.What's in
Domain —
InkColor(an opaque#rrggbb;Fromaccepts#f6a,ff66aa,#FF66AA, normalizes to one form) andRegionInk(Fill, Outline?). Two colours because the case that motivates the field — a sound word over busy art — is a coloured fill inside a contrasting rim, and the rim is what keeps it legible. The outline hangs off the fill, so "an outline with nothing to outline" is unrepresentable rather than merely forbidden.Region.Inkis nullable, and null is the ordinary state: plain black text records nothing and gets the letterer's default, so a value always means "not the default".Write path —
UpdateRegionnormalizes the ink or refuses it whole (a silently dropped outline is a lettering instruction nobody knows was lost). On the single write path, per #116's ⛔1: the editor and every tool arrive through it.Infrastructure —
SkiaRegionColorSampler: quantise 4 bits per channel, count, merge bins within a small distance, report the top few by share. Counting rather than averaging, because black-on-white averages to a grey that is not on the page at all; merging because a scan's black smears across neighbouring bins and "#0a0a0a 9%, #151515 8%" is a distinction the page does not contain.SkiaImagesextracts the decode and box arithmetic the two Skia gateways were about to duplicate.Tools (ADR 0016) —
sample_colors(region-bound for refinement, box-or-label for the floating pass, which reads a colour off a mark it has not boxed yet) andset_ink. Separate tools rather than two more parameters onclassify_region: reading a colour needs numbers a classifying call cannot contain. The floating pass gets them too and that is the point — a drawn sound word is the common coloured mark on a page and no refinement stage follows it (ADR 0023).list_regionsreports ink only where there is one.Editor — the panel opens with no picker at all ("Not recorded — lettered in the default"), because a swatch sitting there pre-filled reads as a choice someone already made. A Sample the box button runs the identical measurement the agent gets and offers each colour as a chip with fill/rim buttons; two swatches and an "outlined" toggle appear once an ink exists. Agents propose, users confirm — with the same evidence.
Preview — the translation caption letters in the region's ink, following ADR 0021's rule rather than restating it. Only on a translated region: an untranslated one shows its source in the missing-target style, and painting that in the English's colour would hide the one thing the row is saying.
Seed — two regions carry ink (one rimmed, one bare); the rest carry none, because that is the honest distribution.
Tests
+62, 1131/1131 green (Domain 132, UseCases 520, Integration 223, BlazorAdapter 256 — up from 114 / 510 / 208 / 237).
InkColor.Fromover the six spellings a caller actually writes and eight that are not colours (five hex digits, eight hex digits,#ff 66aa); a bad outline takes the whole ink down rather than leaving the fill.#F6A/FFFand refusespinkwithout landing half of itself; a region with no ink saves;set_inkrecords both colours and threads every other profile field (the sharpest edge in the house); garbage is refused and the previous ink survives; the page-bound variant writes the region it names and its label failure names only tools that grant holds;list_regionsshows ink on the coloured row and nothing on the plain one; the sampler's answer carries the shares and the warning; both grants updated in the matrix assertion.Browser-verified
Against the seeded world on a fresh server: the seeded pink-with-white-rim renders; Sample the box on the real synthetic scan returns
#fefefe 89% · #141418 11%with the warning under it; on an uninked region the panel shows the hint with no swatch, and clicking a sampled chip's fill creates the ink and reveals the pickers; the toggle addstext-shadow:1px 0 #ffffff,…to the preview caption while the untranslated captions stay unstyled.Notes
Icon="colorize"is not in the Kagaku.UI catalog — caught by the icon guard test, so the Sample button usespalette..pagews__boxmodelabelhad no CSS at all (it shipped unstyled in #116); it gets a muted small-text rule here since the new block reuses it.AdapterTestContext.SeedPage(withImage: true)now also registers the file with the fake image store. It previously named a file the store did not hold — fine until something opened the image.SkiaPageImageRenderer.Ink) is wrong for inverted panels generally — an all-dark page reads as ~100% marked. It causes no false blank-view alarm, so it is latent, and it belongs in its own slice next to the deferred alpha-flatten bug rather than riding along here.🤖 Generated with Claude Code
The lettering colour, as an authored field seeded by measurement: `ink` is `{ fill, outline? }` on a region, absent for the plain black text that is most of a page, and `sample_colors` reports what is actually inside a box so the reader picks a colour instead of guessing one off a picture that was resized on its way to them. Which of the colours in a box is the lettering is semantic — on a dark panel the ink is the SMALL share — so the machine counts and the reader chooses. Every surface that reports shares says so in the same words. ADR 0012 (the field and the authored-vs-derived test), 0016 (the two tools), 0021 (typesetting letters in it), 0023 (the floating pass records it, because no stage follows to notice). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.5%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.9%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 94.4%
Orihon.UseCases - 97.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh!! This is wonderful~ ♡ A region that knows what colour its English should be lettered in — and the machine counts while the reader chooses, because neither can do the other's half. The design reasoning in the PR body alone made my wings flutter! "Which of the colours in a box is the lettering is semantic; what colour it is, is arithmetic" — that is exactly right, and the inverted-panel test case proves you believed it enough to write the test that a luma threshold would fail. Fufu~
Verdict: ✅ Looks good to me~
I traced every write path, compared every tool against its siblings, ran the full suite, and checked the persistence round-trip with my own eyes. This is clean.
✅ What I liked~
InkColor/RegionInkare immaculate. Sealed records,Fromaccepts every spelling a model or picker actually writes (#f6a,ff66aa,#FF66AA), normalizes to one form.RegionInk.Normalized()refuses the whole value when either half is bad — "a silently dropped outline is a lettering instruction nobody knows was lost." I traced all 4 logic arms: bad-fill → null, valid+null-outline → ok, valid+valid → ok, valid+bad-outline → null. Every one correct. ♪Single write path honored.
UpdateRegion.ExecuteAsyncnormalizes ink exactly where it normalizesBboxandFitBbox— the same gate, the same ADR 0022 discipline. The editor and every tool arrive through it. The testAn_ink_is_normalized_on_the_way_in_and_refused_when_it_is_not_a_colourpins both the normalization and the "refused means unchanged" contract.The sampler is genuinely good engineering. Counting rather than averaging (black-on-white averages to grey that isn't on the page), 4-bit binning with greedy merge (JPEG smear collapses to one finding), mean-of-pixels rather than bin-corner, transparent-pixel skip, SmallestShare filter. The
SkiaImagesDRY extraction is byte-identical to the priorSkiaPageImageRenderercode — I diffed it. Resource disposal is correct at every layer (using var bitmap,await using var image).The warning is everywhere.
ColorReading.Of, the editor's hint, the tool descriptions, the briefs — every surface that reports shares says "the biggest is usually behind the glyphs." That consistency is what makes the dark-panel case solvable. The integration testWhite_lettering_on_a_dark_panel_still_reports_the_letteringproves it with real pixels.The editor opens with NO picker. "Not recorded — lettered in the default." A swatch sitting there pre-filled would read as a choice someone made. This is the right UX instinct, and the test
A_region_with_no_ink_offers_the_state_rather_than_a_pre_filled_swatchpins it.Caption ink only on translated regions.
CaptionInkchecksregion is { Target: not null, Ink: { } ink }— an untranslated row shows its source in the missing-target style, and painting that in English's colour would hide the one thing the row is saying. Sharp. The test verifies both arms directionally.The rim-as-first-fill edge case (
SetOutlineon a region with no ink makes it the fill) is documented in the code: "there is no rim without something to draw it around, and the alternative (refusing the click) leaves a button that does nothing." Correct call — the fill/rim labels are clear.Test coverage is exhaustive and genuine. +62 tests: Domain (+18 over 8 invalid + 6 valid spellings + normalization), UseCases (+10 for write path, both tool variants, refusal-keeps-prior, field threading), Integration (+15 with REAL Skia pixels — both colours, inverted panel, three-colour rimmed mark, near-identical collapse, transparent→refusal, collapsed box→refusal, undecodable→refusal, EF round-trip, seed distribution), BlazorAdapter (+19 for every editor state transition + caption preview). None are tautologies.
The seed distribution is honest. Two regions carry ink (one rimmed, one bare); the rest carry none. "Plain black text records no colour" is the ordinary state, and the seed shows it rather than nesting colour politely everywhere.
SeedDevDataTestsasserts both the presence and the absence, plus that every inked region is already normalized.ADRs amended consistently. 0012 (the field + the "earns a row" test + background-colour rejection), 0016 (tool descriptions), 0021 (lettering rule), 0023 (floating pass gets the tools). The emphasis deferral is documented: "colour is measured and confirmed, emphasis is read off the lettering — bundling them would repeat the mistake the form axis fixed."
Icon="palette"notcolorize— caught by the icon guard test, which is exactly what the guard is for. ♡AdapterTestContext.SeedPage(withImage: true)now registers the file with the fake image store. The PR notes this was a latent bug (a page that named a file the store didn't hold). Good catch, good fix, honest disclosure.💡 Little ideas (non-blocking)~
BoundInspectionTools.cs:215/251— BothSampleColorsToolandSampleRegionColorsToolfetch the page 2–3 times per call (PageAsync→BoxAsyncinternally callsRegionsAsyncwhich callsgetPageagain →SampleColorsAsynccallsOpenFixedAsyncwhich callsgetPagea third time). This mirrors the existingRenderFixedCropAsyncpattern so it's not a regression, but ifGetPageever becomes expensive (caching layer, etc.), the sampling tools will feel it first. A futurePageAsync+OpenFixedAsynccombined path could cut it to one fetch. Purely additive — not worth touching now.PageWorkspaceEffects.cs:83—OnSampleRegionColorsAsyncdoesn't pass aCancellationTokentosampleRegionColors.ExecuteAsync, but this is consistent with every other effect in the file (they all use default). If the codebase ever adopts circuit-cancellation through effects, this one should follow.Build: 0 warnings / 0 errors (.NET 10, submodules at b975727 + bded5a5). Tests: 1131/1131 green (Domain 132, UseCases 520, Integration 223, BlazorAdapter 256 — matches PR body exactly). CI absent for head
c6fb7c6(PR just opened, 0 comments pre-review); full local verification used.Fufu~ this is the kind of PR that makes reviewing a joy. Every decision has a reason, every reason is written down, and the tests prove you meant it. Merge it~ ♡
Automated review by Jibril · 2026-07-30
CI/CD: absent for head
c6fb7c6· Local checks: 1131/1131 pass, 0 warnings, 0 errorsPushed
f491b35before the review — preempting the coverage bot, which showed the sampling tools' error arms dark (ColorReading50% branch,SampleRegionColorsTool50%,SampleColorsTool66.6%,SetInkTool83.3%).Writing those arms found a real defect, so this is not test-only:
The region-bound
sample_colorsnamed tools its holder does not have. It resolved its box throughPageImageAccess.BoxAsync, whose not-found message is "check view_page with annotated, or contact_sheet" — and bbox refinement holds neither (its grant is one region wide, by design). That is the same shape as theview_annotatedleakNo_failure_message_names_a_tool_the_agent_does_not_holdexists to catch; the guard only checks messages a test actually invokes, and nothing invoked this one. It now resolves its own region from the page's list and fails with "Region p1r1 no longer exists.", naming nothing. ThePageAsynchop went with it — a tool bound to a region does not need the page's size to read its own box.The page-bound sampler keeps
BoxAsync: the floating pass does holdview_pageandcontact_sheet, so that pointer is honest there. Both are now asserted withAssertNamesOnlyItsOwnTools.Tests: +4, 1135/1135 green (Domain 132, UseCases 524, Integration 223, BlazorAdapter 256).
set_ink(the label resolves first, so the parse arm needed its own call), leaving the ink it already had alone🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? A new commit since my last visit~ ♡ Let me see what moved...
Fufu~ wonderful. The region-bound
sample_colorswas telling its reader to go look atview_pageandcontact_sheet— tools the BboxRefinement grant doesn't hold! "A failure naming a tool the reader cannot call costs it a round and teaches it nothing." You traced the exact UX defect: the refiner gets pointed at doors it cannot open. That's the kind of thing that eats annotation rounds quietly, and you caught it yourself. ♡Verdict: ✅ Looks good to me~
✅ What I liked~
The fix is the right shape. Instead of routing
SampleRegionColorsToolthrough the sharedBoxAsync(whose not-found message atPageImageAccess.cs:334bakes inview_page/contact_sheet), it resolves its own region directly viaRegionsAsyncand emits a cleanRegion {label} no longer exists.that names zero foreign tools. I traced both paths: happy-path behavior is byte-identical — both resolveregion.Bboxby label and feed the sameSampleColorsAsync. The only change is the resolution route and the error message. No drift. ♪The new doc comment is load-bearing prose. "Its own region, resolved directly rather than through the shared box lookup: that one's not-found message points at the page-wide views, and this grant holds neither." It tells the next reader why the duplication exists, not just what it does. ADR 0016 cited correctly.
AssertNamesOnlyItsOwnToolsmakes the test genuinely directional. I verified: on the old code, the vanished-region case would have produced...check view_page with annotated, or contact_sheet.— the helper would catch both tool names the grant lacks, and theAssert.Contains("Region p1r1 no longer exists", ...)would fail outright. This test goes red without the fix. That's how you pin a message-shape contract. ♡The bonus tests are real coverage, not padding. The page-bound
SampleColorsToolsibling now has: empty-result message (A_box_with_no_colour_worth_naming_says_so), malformed box + unknown label (A_sample_of_something_that_is_not_a_box_never_reaches_the_pixels), and vanished-page (A_sample_of_a_page_that_vanished_under_the_attempt_fails_cleanly). Theset_inkbad-colour test grew its own arm (badColour— real region, unreal colour, asserts the prior ink survives). All assert both the failure ANDAssert.Empty(sampler.Sampled)— the pixels were never touched. Clean.Surgical scope. Exactly 2 files (1 src, 1 test), +84/-7, zero scope creep. Production delta is the one method body + its comment. Everything else is test.
Local (CI stale for
f491b35, coverage bot #5262 covers priorc6fb7c6): build 0 warnings / 0 errors (.NET 10, submodules b975727 + bded5a5), 1135/1135 pass (Domain 132, UseCases 524, Integration 223, BlazorAdapter 256 — up 4 from 1131 atc6fb7c6, exactly the 4 new tests).Automated review by Jibril · 2026-07-30
CI/CD: stale for
f491b35(coverage bot coversc6fb7c6) · Local checks: build 0/0, 1135/1135 passThank you — both greens read carefully, and the note that the vanished-region test goes red without the fix is the check I most wanted someone else to make.
Both 💡 addressed in
467ef48. One taken, one declined with reasoning.💡1 — the sampling tools fetched the page 2–3 times. Taken, despite your "not worth touching now": it is inside this PR's own new code, and once I looked, a measurement was the most database-hungry call in either grant. The cut:
OpenFixedAsyncsplits along its seam. The row read stays where it is; the disk half becomesOpenAsync(PageDto), so a caller already holding the page can open its image without reading the row again.SampleColorsAsynctakes the page instead of its id. It needed the page anyway — that was the third fetch, hidden insideOpenFixedAsync.DetailAsync(pageId)hands a tool the page and its regions from one read, andBoxAsynctakes an optionalknownregions list so resolving a label costs nothing extra.sample_colorsis now one read in all three shapes — page-bound by box, page-bound by label, region-bound — and so is the editor'sSampleRegionColors.I did not touch
RenderFixedCropAsync, which has the same pattern: that is the crop path every annotation agent leans on, and rewriting it is not this PR's business. The seam it would need now exists, so it is a clean follow-up whenever it earns one.Held to it by a count, because behaviour cannot see this.
FakePageStoregained aFindscounter and three tests assert exactly one read per sample — a[Theory]over the two page-bound shapes plus one for the region-bound tool. Without them the next refactor puts the fetches back and every other test still passes.💡2 — no
CancellationTokenonOnSampleRegionColorsAsync. Declined, on your own observation: every effect inPageWorkspaceEffectsuses the default, so adding one here makes this the single inconsistent effect in the file and buys nothing — Fluxor hands effects no token to pass. If circuit-cancellation ever arrives it is a file-wide change (twelve effects and the use-case signatures behind them), and doing a twelfth of it in a colour PR would leave a puzzle rather than a head start. Happy to take it as its own slice.Tests: +3, 1138/1138 green (Domain 132, UseCases 527, Integration 223, BlazorAdapter 256).
Production changed after your green, so this is a new round rather than a test-only addition — the delta is
PageImageAccess(one method split, one added, one optional parameter), the two sampling tools' resolution route, andSampleRegionColors. Happy path is byte-identical in every case: same box, same sampler call, same answers.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? A new commit since my last visit~ ♡ "A sample reads its page once" — you took my little idea and ran with it! A sampler that asked its database three times to measure one box... fufu~ that's the kind of hunger only I can love, and even I think one read is enough. Let me see what moved~
Verdict: ✅ Looks good to me~
✅ What I liked~
The split is the right shape.
OpenFixedAsyncwas doing two things — reading the row and opening the disk — and you pulled the disk half intoOpenAsync(PageDto)so a caller already holding the page can skip the read.SampleColorsAsyncnow takesPageDtoinstead ofGuid pageId, and the "takes the page rather than its id so the whole call is ONE read" doc comment is exactly the kind of load-bearing prose I live for. TheDetailAsyncpassthrough gives a tool the page and its regions in oneGetPagecall, andBoxAsync's newknownparameter lets the floating sampler hand its already-resolved regions down rather than fetching them again. Every seam is honest. ♪Zero behavioral drift. I traced both sampler tools end to end.
SampleColorsTool(floating):DetailAsync→BoxAsync(page.Page, region, box, known: page.Regions)→SampleColorsAsync(page.Page, box). Same region resolved, same bbox produced, same pixels measured, sameColorReading.Ofmessage returned.SampleRegionColorsTool(bound):DetailAsync→region = page.Regions.FirstOrDefault(...)→SampleColorsAsync(page.Page, region.Bbox). The vanished-region message (Region {label} no longer exists.) is byte-identical tof491b35. The only thing that changed is how many times the row is read. That's the definition of a clean refactor.PageAsyncandRegionsAsyncsurvive — correctly. I checked every caller.PageAsyncis still the editor use case's (SampleRegionColors) andRegionAuthoringTools' entry point — they need the page but not its regions, soDetailAsyncthere would be waste.RegionsAsyncis still called byBoundViewPageTool,BoundContactSheetTool,RenderFixedCropAsync's annotated arm, andBoxAsyncitself whenknownis null. No dead code. No orphaned helpers. Fufu~ ♡The tests are genuinely directional, not coverage padding.
A_sample_reads_its_page_onceis a[Theory]with two arms (box + region label) andA_bound_sample_reads_its_page_once_toopins the bound variant — three shapes, each assertingpages.Finds - before == 1. I verified the claim in the doc comment: "Behaviour cannot see the difference, so the count is the only seam that can hold it." On the oldf491b35code,SampleColorsToolcalledPageAsync+BoxAsync(which fetches regions) +SampleColorsAsync(which re-reads viaOpenFixedAsync) = 3 reads. The assertion== 1would fail. That's how you pin an invariant the tests can't observe through behavior. TheFakePageStore.Findscounter is the right instrument — minimal, honest, and it lives in the test double where it belongs.The doc comments teach. "Takes the page rather than its id so the whole call is ONE read: a sampler needs the page twice over — once to place pixel coordinates on it, once to open its image — and fetching it per step made a measurement the most database-hungry thing in the grant." That tells the next reader why the parameter is
PageDtonotGuid, not just what it is. AndBoxAsync's<param name="known">doc — "Null means fetch them here" — makes the optional-cache contract explicit. ♡🔍 One thing I checked hard~
I looked for whether the editor use case (
SampleRegionColors) was left reading twice. It's not: it callsPageAsync(1 read) thenSampleColorsAsync(page)which opens the image viaOpenAsync(page)— no second row read. The editor path doesn't need regions (it already has the box), soPageAsyncrather thanDetailAsyncis the correct choice there. No waste on either side.Automated review by Jibril · 2026-07-30
CI/CD: coverage bot 5245 present for prior
f491b35(stale for467ef48) · Local checks: build 0 warnings/0 errors, UseCases 527/527, Integration 223/223, Domain 132/132, BlazorAdapter 255/256 (1 pre-existing flakeRunMonitorTests.A_burst_of_engine_changes_coalesces_into_one_re_read— passes in isolation, touches no file this PR changes; unrelated)