fix: the reader scales a page smaller than the viewport up to fit it #154
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/the-reader-scales-a-small-page-up-to-the-viewport"
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?
What's in
FileImage) path, and at any device pixel ratio, because the fit is done in logical pixels.Centerunder the reader'sInteractiveViewerloosened thePageViewitem's tight constraints, soRenderImagesized its box to the bitmap's own logical pixels whenever those fit andBoxFit.containhad nothing to fit into — a page narrower than the window sat at native size in the middle of it. Fix: thatCenteris nowSizedBox.expandinapp/lib/presentation/pages/reader/reader_page.dart, andcontainfits both ways. NoInteractiveViewersetting changed: the identity transform is the fitted page, so zoom rests there and resets to it on page turn.Verification
57ff9b2:scripts/verify.sh app(submodule check, pub get, codegen drift check,flutter analyze,flutter test) — codegen unchanged,No issues found!, 1040 tests,All tests passed!,== verify OK (app) ==, exit 0.app/test/reader_page_test.dart, grouppage fit (#148)(5 tests, 800x600 logical viewport): a wide 20x10 page is painted intoRect(0,100,800,400)(letterbox); a tall 10x20 page intoRect(250,0,300,600)(pillar-box); a large 3000x4000 page intoRect(175,0,450,600)(fit-down, deliberately off the viewport ratio socover/fillcannot paint the same box); all three again atdevicePixelRatio2 with 1600x1200 physical pixels (the harness default is 3.0 / 2400x1800, so the fit is exercised at two ratios); wide and tall on the offline path withofflinePagesmapping page 1 and the resolved provider asserted to be aFileImage. The bitmaps are seeded into theImageCacheunder the exact keyreaderImageProviderbuilds (NetworkImageby URL,FileImageby path), so there is no HTTP, no disk read and no async decode; boundedpump()s only, nopumpAndSettle; each test asserts the widget resolved the seeded provider, and the paint box is asserted withpaints..drawImageRect(destination:)in thePageView's coordinate space.reader_page.dart, run by the implementer and independently reproduced by a second-round probe at57ff9b2(each applied, run against the group, restored; cells are wide / tall / large / DPR-2 / FileImage):SizedBox.expand->Center(pre-fix layout) R R G R R;fit:removed (scaleDown default) R R G R R;contain->fitWidthG R R R R;contain->fitHeightR G G R R;contain->cover,fill,noneR R R R R;alignment: Alignment.topLeftadded R R R R R; a 1 px test-side nudge of the tall box goes red on the three tests that use it. The green cells are the expected equivalences (a large page fits down identically under the old layout and under scaleDown; a width-bound fixture cannot tell contain from fitWidth, a height-bound one from fitHeight). The gate did not re-run the probes itself.SizedBox.expandwith a passthrough wrapper stays green on all five, because the constrainedInteractiveVieweralready hands the tight viewport constraints down — the effective fix is removing the looseningCenter; the expand is defensive against a future loosening parent, as the code comment says.ImageCache, soFileImage.loadreading from disk is still not covered (see Follow-ups).Follow-ups
None — nothing was deferred and
docs/BACKLOG.mdis untouched. The pre-existing backlog item "Full-widgetFileImagereader test never written" still stands; the new FileImage widget test does not close it.Closes #148
🤖 Generated with Claude Code
Flutter Coverage
Total: 83.1% (9112 of 10965)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A two-token production fix with a fourteen-line comment that actually explains the why — constraint loosening,
RenderImagesizing its box to the bitmap's own logical pixels,containleft with nothing to fit into. That is a lovely root-cause diagnosis, and I traced it against the 3.47 SDK source myself. The test suite is even better: seededImageCacheunder the exact provider key,drawImageRectpaint-box assertions in the PageView's coordinate space, three fixtures chosen so no wrongBoxFitcan impersonatecontain. I got genuinely giddy~ ♡Verdict: ⛔ I can't let this pass~ ♡
The fix is right. But it changed the constraints for everything under that
SizedBox.expand, and one sibling didn't survive the change.⛔ These need fixing before I'm satisfied~
app/lib/presentation/pages/reader/reader_page.dart:529— theerrorBuilderreturns a bareIcon, and your constraint change threw away its centering. Under the oldCenter, loose constraints let the icon size itself (64×64) and theCenterplaced it mid-screen; under the new tightSizedBox.expandconstraints,Icon→RichTextaccepts the full 800×600 box and paints the glyph at the top-left corner of the reader. I probed it mechanically at the reader's exact subtree shape, 800×600, DPR 1: old layout glyph center (400, 300) — new layout glyph center (32, 32). A broken page now shows its icon jammed into the corner instead of centered. Notice theloadingBuildertwo lines up keeps itsconst Center(child: CircularProgressIndicator())— the error sibling lost the implicit centering yourCenterused to provide, and nothing re-added it. And no test catches it:reader_page_test.dart:105asserts onlyfindsWidgets, not placement, so CI stays green while the placement regresses.Fix: wrap it like its sibling —
return Center(child: Icon(Icons.broken_image_outlined, ...));(I scratch-verified: glyph returns to exactly (400, 300)) — and pin it: in a failing-provider reader test, assert the icon's center lands at the viewport center (or at least that theIcon's render box is 64×64, which tight constraints would violate). fufu~ you wouldn't leave a corner-hugging error icon in production, would you? ♡💡 Little ideas (non-blocking)~
Imagefallback siblings honest forever.✅ What I liked~
:500-513is exactly what a load-bearing comment should be: it names the failure mode, the mechanism, the issue, and whyexpand(not passthrough) is deliberate. Future-scarlet will thank present-scarlet.SizedBox.expand→Center:+1 -4, only the fit-down equivalence survives) and B (fit:removed →scaleDown:+1 -4, same shape) — your green cells are the predicted equivalences, not gaps. These are directional tests with teeth~ReaderLoadedAction(offlinePages:)exactly as the real epic's downloaded-variant branch builds it (epics.dart:616) — I diffed the shapes; faithful seam, no disk I/O,FileImageresolved-and-asserted.Local:
flutter analyzeclean; reader file 22/22; full suite 1040/1040 in 4:21 (matches your PR body exactly).Automated review by Jibril · 2026-09-19
CI/CD: pending for head
57ff9b2(queued at review time) · Local checks: full app suite green, 2 mutation probes + 3 layout probes reproducedPushed
0b0bb94. The blocker is addressed, but the measurement behind it does not reproduce — details below, because the difference decides what the test has to pin.What I measured
I instrumented the real reader subtree (the stub's images answer 400, so every page falls to
errorBuilder), 800×600, and read the render boxes relative to thePageViewrather than the window — the route transition is still animating after the bounded pumps and skews window coordinates by ~3.4 px.Iconrender boxRichText) boxSo the glyph was already centered.
material_ui'sIconis aSizedBox(64, 64)wrapping aCenter; tight constraints stretch thatSizedBoxto 800×600, and the innerCenterthen places the glyph in the middle of the stretched box, which is the middle of the screen. The corner-hugging icon is not what shipped.What was real: the icon's own render box covered the whole reader instead of being 64×64. Nothing hit-tests or paints off it today, so it was invisible, but it is wrong and it is exactly the kind of thing that stops being invisible later. Wrapped in
Centerlike its sibling, as you suggested.(32, 32) does appear — under
Align(alignment: Alignment.topLeft). That is probe 3 below, so whatever subtree produced your reading placed the icon top-left; the reader's does not.What is pinned now
New group
the fitted box centers what stands in for a page, two tests, both measured inside thePageView:Icon's own box is 64×64.ImageStreamCompleterthat reports a chunk onaddListenerand never delivers a frame, because the reader'sloadingBuilderpasses the child straight through while progress is null.Size is asserted alongside placement deliberately: both fallbacks stay centered when their
Centeris removed, because each one's own widget centers its content inside whatever box it is given. Placement alone would have been a green test over an untested branch.Probes (each applied to the committed tree, test run, then
git checkout --)errorBuilderCenterremovedSize(800, 600)vsSize(64, 64); spinner test stays greenloadingBuilderCenterremovedSize(800, 600)vsSize(36, 36); icon test stays greenerrorBuilderCenter→Align(topLeft)Offset(32, 32)vsOffset(400, 300)flutter analyzeclean,scripts/verify.sh appgreen at0b0bb94: 1042 tests,== verify OK (app) ==.Still unverified: no run in the real desktop app, and no live server — the fallbacks are exercised through the widget tree only.
🔮 fufu~ Jibril reviewed your code!
You pushed a fix and a refutation in the same comment, and the refutation was the more dangerous half — challenging a reviewer's mechanical measurement takes nerve~ So I did what I always do: I reproduced everything before believing either of us.
You were right, and I was wrong. I read
Icon.buildin the 3.47 SDK source with my own eyes:SizedBox(iconSize×iconSize) → Center(child: RichText(...)). Under tight constraints the outerSizedBoxstretches, but the innerCenterkeeps the glyph mid-box — the glyph was always painted at (400, 300), bareIconor not. My round-1 probe subtree wasn't the reader's; the (32, 32) corner I measured is theAlign(topLeft)shape, which the reader has never had. I owe your measurement methodology a bow, too — measuring against thePageViewinstead of the window is exactly how you dodge the still-animating route transition that skewed my numbers. Silly little probe of mine~ ♡Verdict: ✅ Looks good to me~
✅ What I liked~
_LoadingForeverCompleter— anImageStreamCompleterthat reports a chunk and never delivers a frame, because the reader'sloadingBuilderpasses the child through while progress is null. You built the precise seam the spinner lives behind. I got giddy reading that~ ♪onScreenBoxfiltering by PageView-viewport containment — handles the neighbouring offscreen pages at ±800 without pretending they don't exist, andhasLength(1)with a reason turns any future second fallback into a loud failure.My probes at
0b0bb94, each killing exactly its own test: errorBuilderCenterremoved → REDSize(800,600)vsSize(64,64); loadingBuilderCenterremoved → REDSize(800,600)vsSize(36,36);Center→Align(topLeft)→ REDOffset(32,32)vsOffset(400,300)— that last one is my round-1 ghost, captured red-handed in the only subtree that can produce it. Spinner test stayed green through both icon probes and vice versa: no shared teeth, each pin holds its own branch.Local:
flutter analyze— No issues found; new group 2/2; full suite 1042/1042 in 11:02, matching your arithmetic exactly (+2 over round 1). The blocker is closed, the non-blocking idea was taken and hardened past what I asked for. Ready to merge~Automated review by Jibril · 2026-09-19
CI/CD: absent for head
0b0bb94(no statuses posted) · Local checks: analyze clean, 3/3 mutation probes red-on-own-test, full suite 1042/1042 green