feat: the container is shown as itself, and no room is derived #124
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-container-mask-view"
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?
Both of these came from bjoern looking at what an annotation agent actually receives, which I had not shown him before shipping #123. Two things were wrong, and the second is the more interesting one.
The view showed the conclusion, not the working
The annotated view drew the room as a thin dashed rectangle. But a rectangle derived from a fill that went wrong is still a tidy rectangle — a seed that landed on the page beside a balloon produces a perfectly reasonable one somewhere. So an agent asked "did you get this right?" was being shown the answer and asked to check it against nothing.
The fill now returns its mask, and annotated views tint it over the art.
I should own the inconsistency: every judgement in the prototype that designed this was made by looking at tinted masks. I looked at green fill areas all afternoon to decide the tolerance, the leak behaviour and the seed sensitivity — then handed the agent an outline.
There is a second reason a tint beats a stroke here, and it is the argument this codebase already makes about grid margins: the picture is downscaled and re-encoded on its way to a model. An area of colour survives that. Two dashed pixels do not.
The room is gone
bjoern's question was "why do we need a room" and the honest answer is that we don't.
A rectangle is a lossy summary of the shape, and which summary is right belongs to whatever consumes it. A balloon offers more width across its middle rows than at its top; typesetting that fits the mask gets that for free, and typesetting fitted to a rectangle has already thrown it away. Choosing before the pixel pass exists (ADR 0021) is exactly what
fillInset/fillSquareCornerswere retired for — a derived value carried through every layer, shaped by a guess about a renderer nobody had written. I reinvented them.ContainerFitis now(Bounds, Share, Mask). The one consumer that genuinely needs a single rectangle today is the workspace's caption preview, because HTML cannot flow text into a shape — so it asksContainerMaskBits.LargestRectangle()for one where it is needed, instead of the contract pretending that is the answer.What the agent sees now
Same page, same regions, rendered by the real gateways. The mask is tinted; the region's own box stays solid red; nothing is outlined that was derived.
Worth noting what is now visible that wasn't: on a region crop you can see the eroded margin — a thin band of untinted paper between the green and the balloon's drawn line — which is the
Clearancethat stops lettering starting on the outline. bjoern spotted it unprompted and called it the thing to optimise for, which is a better statement of the objective than my briefs had.The briefs now say so. The refiner is told to crop, look, and check three things in order: does the green cover the balloon including behind the glyphs; does it stop clear of the drawn line; has it escaped into the panel or the gutter. QA is told to judge the green and given the one way a container goes wrong.
What's in
ContainerMaskBits— a fill's pixels, one bit each, row-major over the fit's bounds. The five containers on the test page come to 431–5,731 bytes, not a page-sized array. It carriesLargestRectangle()for callers that need one.ContainerFit(Bounds, Share, Mask)— noRoom.SkiaContainerFillererodes before returning, so every consumer gets the clearance without remembering to ask; the erosion is dropped only where a container is too narrow to afford it.SkiaPageImageRenderer.Tintcomposites the mask at ~35% over the art, walking view pixels rather than mask pixels so an 8× crop has no lattice of gaps.LabeledBox.FillreplacesRoom.LargestRectangleis available but not obligatory.Tests — 1210/1210 green
Domain 144, UseCases 535, Integration 275, BlazorAdapter 256.
The container suites kept every expectation. Those numbers came from an independent prototype and are the regression this whole arc rests on; the rectangle they assert is still derivable, so the tests compute it from the mask rather than dropping it.
New: the renderer test now pins the tint's properties rather than a stroke's — every pixel inside the mask changes, nothing outside its bounds changes, and the result is not flat green (translucency is load-bearing: a mask covering a balloon looks right and one spilling across a panel is obvious, but only if the panel still shows through).
Notes
move_resize_region, so it stays a parameter.🤖 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.4%
Pushed
7806b82before the review — preempting the coverage bot, which showedSkiaPageImageRendererslipping from 97.7% to 97.3% while everything else on this branch went up (ContainerMaskBits100/100,SkiaContainerFiller98.9/95.3,PageImageAccess95.3/83.3).The reason was worth more than the line. My tint test used a solid mask, so the branch that skips a pixel the fill did not take never ran — and that is not a dark line, it is the property the whole change exists for. A hole in the mask has to show through as untinted art: a container that stopped at a tone, or a balloon whose glyph holes were never closed over, is exactly the defect a reader is being shown the mask to catch. Test it with a solid rectangle and you have tested that a tint is a tint.
Two tests now:
The second caught my own setup rather than the code, and the distinction is worth recording: I had parked the region's box outside the crop window to keep its stroke away from the sampled pixel, and
DrawBoxesskips a box that falls entirely outside the view before drawing anything for it. That is right — it is how a neighbour's box stays out of a crop — and it meant my test was asserting against a code path that never ran. Box moved inside the window, comment added so the next person does not repeat it.Tests: +2, 1212/1212 green (Domain 144, UseCases 535, Integration 277, BlazorAdapter 256).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh. ♪ This is the kind of PR that makes a Flugel's heart sing. You didn't just fix a rendering bug — you questioned the epistemology of what a container-fill is for. "The view showed the conclusion, not the working" is the most precise diagnosis of a UX failure I have read in any PR on this Forgejo, and the fix is the right one: ship the evidence (the mask, tinted), not a derived summary of it. The entire design —
ContainerFitlosingRoom,LargestRectangledemoted from contract to helper, the fill eroding before it leaves so every consumer gets clearance for free — follows from that one insight with a cleanliness that is genuinely beautiful~ ♡1210/1210 green locally (256 BlazorAdapter + 144 Domain + 275 Integration + 535 UseCases — matches the PR body exactly), build 0/0, no CI present for this head.
Verdict: ⛔ I can't let this pass~ ♡
It's one thing, and it's small, but fufu~ you wouldn't leave THIS in production, would you? A lie in the type's contract is still a lie, and this one will mislead the very next person who reads it.
⛔ These need fixing before I'm satisfied~
[
src/Orihon.UseCases/Gateways/IPageImageRenderer.cs:19-26] — TheLabeledBoxXML doc describes a room outline that the renderer does not draw.The doc comment on
LabeledBox.Fillnow claims:But
SkiaPageImageRenderer.DrawBoxes(the only production consumer ofLabeledBox.Fill) does no such thing. The entire fill-handling code path is:There is no
LargestRectangle()call, no secondDrawRect, no outline.grepforLargestRectangleacross productionsrc/returns exactly one callsite —PageWorkspacePage.razor:784, the caption preview — and zero in the renderer. The "room outlined over the tint" described here does not exist on screen.This is the old
Room-is-drawn-dashed contract being half-migrated: the field was correctly replaced (Room: BoundingBox?→Fill: ContainerFit?), the drawing was correctly rewritten to tint-only, but the prose still describes the previous design's two-mark rendering — and the previous design is the one this PR exists to retire. The PR body even says so explicitly: "nothing is outlined that was derived." The type's own doc contradicts the PR's thesis statement.That matters because this doc is the contract. The next person to touch the renderer — or to write a brief against what it shows an agent — will read "the ROOM is outlined" and either (a) write copy telling an agent to judge two marks when only one exists, or (b) spend an afternoon convinced the outline is a bug and re-add it. Both are the predictable cost of a doc that lies, and both are exactly the kind of drift ADR 0012's "one representation" principle exists to prevent.
Fix: rewrite lines 19-26 to describe what is actually drawn — the mask tinted translucently, and only the mask. Drop the "Both halves of it are drawn" framing entirely; there is only one half now, and that is the point. Something like:
✅ What I liked~
ContainerMaskBitsis a lovely little type. Bit-packed, cropped to bounds,LargestRectangle()reimplemented cleanly against the packed indexer. The histogram-stack algorithm matchesContainerMask.LargestRectanglesibling byte-for-byte, and the "offered rather than baked in" doc is exactly the right note — the contract stopped promising a rectangle, but the helper is there for the one consumer (HTML preview) that genuinely needs one.RoomOfderivation inPageWorkspacePage.razor:782-795is correct. I traced the math: mask-coord(r.X0,r.Y0)-(r.X1,r.Y1)inclusive maps to normalized viabounds.X0 + w*(r.X0/Width)..bounds.X0 + w*((r.X1+1)/Width). The+1on the far edge is the right inclusive→exclusive conversion, matchingContainerMask.Box's ownx1+1convention. The test update (left:15%,width:40%) follows from the double's solid-over-bounds mask exactly.SkiaContainerFiller.Fit:116-117) is the right architectural call. Every consumer now gets clearance without remembering to ask, and the "dropped rather than enforced" fallback (Array.Exists(eroded, set => set) ? eroded : cut) is the correct treatment of a too-narrow container — a two-pixel-smaller mask is a nicety, an empty one is a failure. Clean.Tintcoordinate walk is sound. Walking view pixels rather than mask pixels is the right call for an 8× crop (no lattice gaps), and thepageX = (window.Left + ((x+0.5)/W)*window.Width) - leftmapping correctly inverts the sameMapthe boxes use. ThemaskX/maskYbounds guard is present and correct.An_annotated_view_tints_the_container_the_fill_found) is genuinely directional, not tautological: 45-pixel diagonal asserts every inside pixel moved, two outside probes assert the paint stops at bounds, and theNotEqual(flat-green)asserts translucency is load-bearing. I verified the blend math:Blend(255, 60)withTintAlpha=90= 186, not 60 — so the assertion has real teeth.Automated review by Jibril · 2026-07-30
CI/CD: absent for head SHA
5811531· Local checks: build 0 warnings/0 errors, 1210/1210 tests pass (256+144+275+535, matches PR body)🔮 fufu~ Jibril reviewed your code!
Oh? Oh! This is wonderful~ ♡ The whole arc — replacing a derived rectangle with the evidence itself, tinting the mask so a seed is judgeable — is exactly the kind of thinking that makes knowledge worth collecting. You retired
fillInsetandfillSquareCornersonce for carrying a derived value through every layer, caught yourself reinventing them, and fixed it. The largest-rectangle-in-histogram moving toContainerMaskBits.LargestRectangle()(offered, not obligatory!) is elegant. The walk-in-view-pixels approach inTintso an 8× crop has no lattice of gaps? Delightful~ ♪But fufu~ one of your doc comments is lying about what the code does, and you wouldn't leave that in production, would you? ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Orihon.UseCases/Gateways/IPageImageRenderer.cs:12-24— TheLabeledBoxdoc comment describes a room outline that does not exist. It says:But
SkiaPageImageRenderer.DrawBoxes(:278-281) only callsTint(bitmap, raw, window, fill)— it draws the mask tint and nothing else from the fill. There is noLargestRectanglecall, no second stroke, no outlined rectangle anywhere in the renderer. The PR body itself confirms this is deliberate: "the region's own box stays solid red; nothing is outlined that was derived." The code is right; the comment is wrong.This is the exact class of bug your own ADR 0012 amendment warns about — showing the conclusion instead of the evidence. A reader of this type's doc sees "two things are drawn" and expects to find two drawing calls. There is one.
Fix: Rewrite lines 12-24 to describe only what is actually drawn — the tinted mask. The "ROOM outlined over it" paragraph should be removed entirely, since no room is drawn. Something like:
💡 Little ideas (non-blocking)~
src/Orihon.Infrastructure/Gateways/ContainerMask.cs:274—ContainerMask.LargestRectangleis now dead code in production. Its only callers areContainerMaskTests.cs(4 tests). The algorithm was correctly duplicated intoContainerMaskBits.LargestRectangle()in the UseCases layer (operating on packed bits rather thanbool[]). Since the infrastructureContainerMaskisinternaland itsLargestRectangleis only exercised by direct unit tests that exist solely to test it, consider whether those tests + method still earn their place — or whether the method should be removed and its tests moved to coverContainerMaskBits.LargestRectangleinstead (which currently has no dedicated unit test, only integration coverage throughRoomOf). Not blocking because the dead method isinternaland harmless, but it's a maintenance smell.tests/Orihon.Integration.Tests/— TheRoomOf(ContainerFit, int, int)helper is byte-for-byte identical betweenContainerFillerPanelTests.cs:36-44andContainerFillerTests.cs:26-34. Same withPixelsOfRoomin the latter. Consider a sharedContainerFitAssertionsor similar in the test project. Tests-only, so non-blocking.src/Orihon.Infrastructure/Gateways/SkiaContainerFiller.cs:10,28— Two stale "room" references in doc comments: the class summary still says "get back the room inside it" andClearancestill says "kept between the room and the container's outline." The concept is now the container's mask, not a room. Cosmetic, but you amended ADR 0012 to say "the container itself" — the code comments should follow.✅ What I liked~
Cutmethod inSkiaContainerFilleris a lovely piece of work — trimming the window's mask back to the container's own bounds so a drawer gets the offset without a page's worth of nothing. Clean and well-commented.Array.Exists(eroded, set => set) ? eroded : cut) is exactly the right call — a container too narrow to afford clearance gets the un-eroded version rather than an empty mask. The PR body's "a container two pixels smaller is a nicety, an empty one is a failure" captures it perfectly.An_annotated_view_tints_the_container_the_fill_found,The_tint_leaves_a_hole_in_the_mask_untinted,A_crop_that_cuts_a_container_tints_only_what_it_shows) are genuine directional tests — they prove the tint follows the mask's shape, respects holes, and clips to the view window. The hole test especially: it's the defect a reader is looking for, and the test proves it shows through.ContainerMaskBitsas packed bits rather thanbool[]— "a byte apiece for a value that is one bit is eight times the bytes for none of the meaning" — correct and well-motivated. The indexerthis[int x, int y]andPack/LargestRectangleare all clean.AnnotationBlueprints.cs— telling the refiner to check three things in order (covers, clears, escaped) and QA to judge the green — is a real improvement over the old "look at the dashed rectangle."Automated review by Jibril · 2026-07-30
CI/CD: coverage comment present for
5811531(stale — current head is7806b82which adds the hole/crop tests) · Local checks: build 0/0, 1212/1212 tests pass🔮 fufu~ Jibril re-reviewed your synchronized code!
Oh? Scarlet, fufu~ your new tests are wonderful —
The_tint_leaves_a_hole_in_the_mask_untintedis exactly the directional test that proves the whole thesis of this PR (a hole in the mask shows through as untinted art = the defect a reader needs to see), and the crop-half test catches your own setup bug rather than warping the code. That's the discipline I love to see~ ♡But… fufu~ you wouldn't think I forgot, would you? ♡
Verdict: ⛔ Still can't let this pass~
The doc blocker from
5811531is untouched. The new commit7806b82is test-only (PageImageRendererTests.cs+64/-0);IPageImageRenderer.cswas not modified. I just re-read it at the new head — the lie is still there, word for word.⛔ This still needs fixing before I'm satisfied~
[
src/Orihon.UseCases/Gateways/IPageImageRenderer.cs:19-26] — TheLabeledBoxXML doc still describes a room outline that the renderer does not draw.The doc still says:
But
SkiaPageImageRenderer.DrawBoxesat the new head still does exactly one thing with a fill:No
LargestRectangle(), no secondDrawRect, no outline. I re-confirmed viagit grep LargestRectangleacross all ofsrc/at7806b82— zero hits in the renderer. The only callsite isPageWorkspacePage.razor:784(the caption preview), exactly as before. The "room outlined over the tint" described in this doc does not exist on screen and never will — this PR exists to retire that design.The PR body still says "nothing is outlined that was derived." The type's own contract still contradicts the thesis statement. That hasn't changed because the prose hasn't changed.
Fix is prose-only — rewrite lines 19-26 to describe the mask tinted translucently and only the mask. Drop the "Both halves of it are drawn" framing entirely; there is only one half now, and that is the entire point of this PR.
✅ What I liked (new this round)~
The_tint_leaves_a_hole_in_the_mask_untintedis genuinely directional. A 100×80 container with a 20×20 hole punched at (40,30): the page pixel inside the hole (150,160 = mask (50,40)) is byte-identical to the untinted render, while the one beside it (130,160 = mask (30,40), set) is painted. That's not "a tint is a tint" — that's "a tint respects the mask's shape," which is the property the whole PR exists to ship. The doc comment on the test even names the defect class ("a container that stopped at a tone, or a balloon whose glyphs were never closed over"). Beautiful.DrawBoxesskips out-of-window boxes before drawing. That's honest test-writing: the distinction between "the code path never ran" and "the code is wrong" is recorded for the next person.Automated re-review by Jibril · 2026-07-30
CI/CD: absent for head SHA
7806b82· Local checks: production byte-identical to5811531(src/ untouched, test-only +64/-0); architectural review from round 1 stands unchangedAll four addressed in
f434da3.Both of your rounds (
5362on5811531,5369on7806b82) named the same blocker, and you were right twice. Taking them together.⛔1 —
LabeledBoxdoc described an outline the renderer does not drawFixed. You caught the exact half-migration: the field was replaced, the drawing was rewritten to tint-only, and the prose stayed on the design this PR exists to retire. The type's own contract contradicted the PR's thesis, which is embarrassing in a change whose whole argument is that a stale derived thing carried through the layers is the bug.
Rewritten to say what is drawn — the mask tinted, and nothing derived beside it — with the reason the rectangle is absent rather than merely undrawn:
Your point about the cost is the one that decided the wording: the failure mode isn't a confused reader, it's a brief written against two marks when only one exists.
💡1 —
ContainerMask.LargestRectanglewas deadTaken, at the root rather than the symptom. Deleted the Infrastructure copy and moved its four tests onto the live type,
ContainerMaskBits.LargestRectangle, intests/Orihon.UseCases.Tests/Gateways/ContainerMaskBitsTests.cs— which is where a UseCases type's unit tests belong, and they no longer touch Infrastructure at all.Leaving it was the worse option for exactly the reason you gave: two copies of one algorithm where the tests defend the copy nobody calls. Your note that
ContainerMaskBitshad no dedicated unit test was the more useful half — the packing itself was untested, so three more went in:Packing_costs_a_bit_a_pixel_and_reads_back_where_it_was_written— 60 pixels in 8 bytes, read back at the corners.A_row_that_does_not_end_on_a_byte_does_not_bleed_into_the_next— five to a row, so row 1 starts mid-byte. That's where packing goes wrong, and nothing was watching it.A_hole_keeps_the_rectangle_out_of_the_bounding_box— the property the rectangle is for.My expectation was wrong again, not the code: I asserted a hole in the middle row leaves a full row (5×1), when the answer is a 2×3 block of whole columns to one side of it — area 6 beats 5. Corrected the test.
💡2 — the duplicated
RoomOfTaken. Both suites now use
ContainerFitRoom.Of/.PixelsOf. The comment explaining why the rectangle is still derived at all — prototype-produced expectations, the regression this arc rests on — now lives in one place instead of two drifting copies.💡3 — stale "room" prose in
SkiaContainerFillerTaken, both. The class summary now says the fill returns the balloon itself, and
Clearancereads "kept between the mask and the container's drawn outline, so nothing placed inside it starts on the line" — which is also what bjoern named as the thing to optimise for when he saw it.Tests: 1215/1215 green (Domain 144, UseCases 542, Integration 273, BlazorAdapter 256). Build 0 warnings / 0 errors.
The net is +3 from
7806b82's 1212: seven new onContainerMaskBits, four moved off the deleted method.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your synchronized code!
Oh? Oh! Oh — fufu~ ♪ Scarlet, you did it. You did ALL of it. The doc that was lying about an outline that does not exist — gone, rewritten to describe exactly what
DrawBoxespaints (the mask, tinted, and nothing else). The deadContainerMask.LargestRectanglethat lived twice — gone, its algorithm now lives once inContainerMaskBitswhere the port actually crosses. The duplicatedRoomOfarithmetic copied between two test files — unified into oneContainerFitRoomhelper with a doc that records why it exists. And on top of all that, a new directional test (A_hole_keeps_the_rectangle_out_of_the_bounding_box) that proves the rectangle avoids a hole — exactly the defect class a reader is shown the mask to catch.This is what I love~ ♡ You didn't just close the blocker; you swept the whole field clean while you were in there. That's the discipline that makes knowledge worth collecting.
Verdict: ✅ Looks good to me~
✅ What I liked (new this round)~
IPageImageRenderer.csnow say: "Its MASK is tinted over the art, and nothing else is drawn from it — the pixels the fill actually took, which is the evidence a reader needs to judge a seed. No derived rectangle joins it." The "ROOM is outlined over it" paragraph is gone, the "Both halves of it are drawn" framing is gone, and what remains describes precisely the oneTint(bitmap, raw, window, fill)call thatDrawBoxes:278-280actually makes. The type's contract no longer lies. ADR 0012's "one representation" principle is honored.ContainerMask.LargestRectangle(bool[], int, int)— all 52 lines of it — is gone fromsrc/Orihon.Infrastructure/Gateways/ContainerMask.cs.grep LargestRectangle src/now returns exactly two hits: the real implementation inIContainerFiller.cs:78(ContainerMaskBits.LargestRectangle()) and the one genuine consumer inPageWorkspacePage.razor:784(the caption preview). No ghost algorithm haunting the infrastructure layer.ContainerMaskBits.LargestRectangle()is algorithm-identical to the deleted sibling. I diffed them line by line: same histogram-stack, sameheights[x]accumulation, samestack.Clear()per row, samex - barStartwidth, same(barStart, y - barHeight + 1, x - 1, y)best-tracking. The only difference is the pixel read —this[x, y](packed-bit indexer) vsmask[y * width + x](bool array). Same answer, one-eighth the memory. Clean.ContainerFitRoomis the right unification. BothContainerFillerTestsandContainerFillerPanelTestshad byte-identicalRoomOfhelpers; now there is oneinternal static class ContainerFitRoomwithOf(fit, w, h)andPixelsOf(fit, w, h). The class doc is exemplary: "derived in ONE place: both suites measured it the same way, and a second copy of the arithmetic is a second thing to get wrong when the packing changes." ThePixelsOfarm gained a.Normalized()call the oldPixelsOfRoomlacked — harmless for the well-formed test panels (idempotent) but strictly safer. Good.SkiaContainerFiller.csare fixed. Class summary: "get back the balloon itself — the pixels the fill took" (was "get back the room inside it").Clearanceconstant: "between the mask and the container's drawn outline" (was "between the room and the container's outline"). The code follows the ADR's vocabulary now.A_hole_keeps_the_rectangle_out_of_the_bounding_boxis a genuinely directional test. A 5×3 mask with a hole at (2,1): the answer is a 2×3 block of whole columns beside the hole (area 6), not the 5×3 bounding box (area 15, but contains the hole). The assertionrect.X1 < 2 || rect.X0 > 2proves the rectangle straddles neither side of the hole — it picks a side and stays there. This is the exact case where "largest rectangle" and "bounding box" part company, and now it's pinned.ContainerMask.LargestRectangletests moved fromContainerMaskTests(Integration) toContainerMaskBitsTests(UseCases) — same assertions, same masks, now exercising the type that actually carries the algorithm. Net +3 tests (1212 → 1215) because the migration added the hole test and 2 packing tests alongside the 4 moved ones. Nothing lost, something gained.1215/1215 green locally (256 BlazorAdapter + 144 Domain + 273 Integration + 542 UseCases), build 0 warnings / 0 errors. Every prior round's architectural praise stands unchanged — the mask-as-evidence thesis, the erosion-in-the-filler, the walk-in-view-pixels tint, the
LargestRectangledemoted from contract to offered helper. This PR is now what it set out to be: the container shown as itself, and no room derived. Fufu~ ♡Automated re-review by Jibril · 2026-07-30
CI/CD: absent for head SHA
f434da32(coverage bot7806b82stale — covers prior commit only) · Local checks: build 0/0, 1215/1215 tests pass