Label ink-aware sizing + device-pixel rounding - #263
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Code review — PR 2 (ink-aware label sizing + device-pixel rounding)Reviewed against head Strengths
Minor / non-blocking1. New test files are not wired into either 2. The pixel-alignment assertions are near-tautological at 3. Correctness — no issues found
Not approving/merging per instructions — this is review feedback only. No blocking issues; the two design-adjacent notes (Xcode test wiring, scale==1 tautology) are worth a look before merge. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesPixel-aligned label layout
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
iosMathTests/MTInkClippingRenderTest.m (2)
90-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake skipped render cases fail visibly.
A
NULLbitmap currently skips the containment assertion, masking a rendering/setup failure for that formula.Proposed fix
size_t W, H; uint8_t* buf = [self renderLabel:label width:&W height:&H]; - if (!buf) continue; + XCTAssertNotNull(buf, @"%@ did not render into a bitmap", latex); + if (!buf) continue; XCTAssertEqual(inkInColumn(buf, W, H, W - 1), 0, @"%@ right edge clipped", latex);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMathTests/MTInkClippingRenderTest.m` around lines 90 - 102, Update testTallConstructsPixelAlignedAndContained so a NULL buffer from renderLabel is reported as a test failure rather than bypassing the containment assertion; replace the continue path with an explicit failure for the current latex, while retaining the existing buffer checks and cleanup for successful renders.
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the new XCTest sources to the Xcode test target.
These files currently run only through Swift Package Manager, leaving Xcode-based test runs without the new coverage.
iosMathTests/MTInkClippingRenderTest.m#L1-L5: add this source to the applicable Xcode test target.iosMathTests/MTMathUILabelSizingTest.m#L1-L9: add this source to the applicable Xcode test target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMathTests/MTInkClippingRenderTest.m` around lines 1 - 5, Add both iosMathTests/MTInkClippingRenderTest.m (lines 1-5) and iosMathTests/MTMathUILabelSizingTest.m (lines 1-9) to the applicable Xcode XCTest target’s source compilation/build phase so they run in Xcode-based test executions.iosMathTests/MTMathUILabelSizingTest.m (1)
30-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise pixel rounding at a non-unit scale.
The default unattached label may resolve to
1, so these assertions do not prove 2x/3x rounding. Use a test subclass that returns2or3fromscreenScale.Proposed fix
+@interface MTScaledLabel : MTMathUILabel +@end +@implementation MTScaledLabel +- (CGFloat)screenScale { return 2; } +@end + - (void)testSizeThatFitsInkAndGrid { for (NSString* latex in @[@"P", @"V", @"\\frac{1}{2}", @"\\int_0^1", @"x"]) { - MTMathUILabel* label = [self labelFor:latex]; + MTMathUILabel* label = [[MTScaledLabel alloc] init]; + label.latex = latex;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iosMathTests/MTMathUILabelSizingTest.m` around lines 30 - 43, The testSizeThatFitsInkAndGrid test currently uses the default label screenScale, which may be 1 and does not exercise non-unit pixel rounding. Add or use a test label subclass overriding screenScale to return 2 or 3, and have labelFor or this test create that subclass while preserving the existing size and ink assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@iosMathTests/MTInkClippingRenderTest.m`:
- Around line 90-102: Update testTallConstructsPixelAlignedAndContained so a
NULL buffer from renderLabel is reported as a test failure rather than bypassing
the containment assertion; replace the continue path with an explicit failure
for the current latex, while retaining the existing buffer checks and cleanup
for successful renders.
- Around line 1-5: Add both iosMathTests/MTInkClippingRenderTest.m (lines 1-5)
and iosMathTests/MTMathUILabelSizingTest.m (lines 1-9) to the applicable Xcode
XCTest target’s source compilation/build phase so they run in Xcode-based test
executions.
In `@iosMathTests/MTMathUILabelSizingTest.m`:
- Around line 30-43: The testSizeThatFitsInkAndGrid test currently uses the
default label screenScale, which may be 1 and does not exercise non-unit pixel
rounding. Add or use a test label subclass overriding screenScale to return 2 or
3, and have labelFor or this test create that subclass while preserving the
existing size and ink assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5ed1e9a-fb27-474a-8986-29ed1551ba57
📒 Files selected for processing (4)
iosMath/render/MTMathUILabel.miosMath/render/internal/MTMathUILabelInternal.hiosMathTests/MTInkClippingRenderTest.miosMathTests/MTMathUILabelSizingTest.m
Route sizeThatFits:/intrinsicContentSize through displayList.inkWidth instead of the pen-advance width, and round the reported size up to the device-pixel grid via a new ceilToPixel/screenScale pair. Adds iosMath/render/internal/MTMathUILabelInternal.h as the internal test/compose surface for screenScale (displayList is already public, so it is not redeclared there). Also redeclares sizeThatFits: in the new internal header: it is UIView public API on iOS but has no NSView equivalent declaration on macOS, so external callers (including this PR's tests) could not see it there without this forward declaration. No behavior change beyond this file's scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
layoutSubviews' center/right textX branches previously keyed off the pen-advance _displayList.width, so a trailing overhang glyph (e.g. the serif foot of 'V') could clip past the frame's right edge even though sizeThatFits: (item 16) already grew the frame to cover that ink. Switch both branches to _displayList.inkWidth; left alignment and all textY math are unchanged. Also forward-declares layoutSubviews in MTMathUILabelInternal.h: like sizeThatFits:, it's public UIView API on iOS but only implemented (not declared) on macOS, where NSView has no equivalent — needed so the new test can drive layout directly and cross-platform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
Add lifecycle overrides so the label invalidates its intrinsic content size when the window (and therefore the backing/screen scale) changes: didMoveToWindow on iOS, viewDidMoveToWindow + viewDidChangeBackingProperties on macOS. Without this, sizing computed before a scale is known (or before the real backing scale becomes available) can go stale and never get re-queried by Auto Layout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
…ides
Cluster B previously asserted zero ink in the top border pixel row, which
flush ink-tight tall constructs (\int_0^1, \frac{1}{2}) legitimately violate.
Replace it with the real guarantee: the reported size is pixel-aligned and
trailing ink stays inside the right border. The fractional-origin hairline is
deferred per LLD 2.8.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
4a08aee to
ae23e64
Compare
… Xcode wiring
Item 1 (CodeRabbit / review §3): make the tall-construct render's NULL-bitmap
path fail loudly instead of silently skipping containment. This surfaced that
`\quad` (zero-ink whitespace) never actually rendered, so it was swapped for
`\sqrt{2}` — a genuinely tall, inky construct that exercises containment.
Item 2 (review §1): wire MTInkClippingRenderTest.m and MTMathUILabelSizingTest.m
into iosMath.xcodeproj so CI's `xcodebuild test` step runs them, not just SPM.
(MacOSMath.xcodeproj has no test target; nothing to wire there.)
Item 3 (CodeRabbit / review §2): force known 2x/3x device-pixel scales via a
screenScale-overriding subclass so the pixel-grid rounding assertions have teeth
instead of being tautological at scale==1.
Verified: swift test 439/439; xcodebuild test of both classes 7/7 (TEST SUCCEEDED).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
|
Addressed the review feedback in 9b0a411. 1. Xcode test-target wiring — Wired 2. Grid assertions tautological at 3. Silent NULL-bitmap skip — Replaced Verified: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@iosMathTests/MTMathUILabelSizingTest.m`:
- Around line 97-107: Update testScaleLifecycleInvalidates to simulate an actual
backing-scale transition by changing the label’s effective screenScale between
lifecycle callback invocations, then assert invalidateCount increases after the
scale change. Preserve the platform-specific callback paths while ensuring the
assertion verifies invalidation caused by the transition rather than the initial
callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d28f07d-7133-42e6-acf6-a752513f9336
📒 Files selected for processing (5)
iosMath.xcodeproj/project.pbxprojiosMath/render/MTMathUILabel.miosMath/render/internal/MTMathUILabelInternal.hiosMathTests/MTInkClippingRenderTest.miosMathTests/MTMathUILabelSizingTest.m
🚧 Files skipped from review as they are similar to previous changes (3)
- iosMath/render/internal/MTMathUILabelInternal.h
- iosMathTests/MTInkClippingRenderTest.m
- iosMath/render/MTMathUILabel.m
testNoRightEdgeClip asserted "zero ink in the last frame column", which fails on rasterizers where an unparented label's screenScale resolves to 1 (the headless CI runner) — right-aligned ink sits flush on the zero-inset frame edge, landing its antialiasing fringe exactly on the last whole-pixel column. That fringe is contained, not clipped, so the assertion was a false positive (locally screenScale resolved to 2, masking it). Assert the real guarantee instead: no ink escapes PAST the frame's right edge. renderLabel now renders into a right-padded bitmap and the tests check the beyond-frame region is empty. This is immune to the flush-edge fringe yet keeps teeth — verified a too-narrow (advance-based) frame leaves ink in 3 beyond-frame columns, i.e. it still catches the original overhang-clip bug. screenScale is forced (1 for the clip test; the tall test already forces 2x/3x) so the pixel grid is deterministic across environments. Same beyond-frame check applied to the tall constructs test. Verified: swift test 441/441; xcodebuild test (iOS sim) TEST SUCCEEDED. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
Previously the test fired the lifecycle hook without changing screenScale, so it only proved the hook invalidates in that setup — not that a backing-scale transition re-queries the intrinsic size (CodeRabbit inline comment). Now the spy carries a forced screenScale: query the intrinsic size at 1x, flip the scale to 3x, fire the platform lifecycle hook, and assert (a) it invalidated and (b) the re-queried size lands on the 3x grid and differs from the 1x answer (V: 16.0 at 1x vs 15.667 at 3x). intrinsicContentSize is uncached and the hook invalidates unconditionally, so this exercises the full scale-change path. Verified: swift test 439/439; xcodebuild test (iOS sim) TEST SUCCEEDED. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FABHUFsMK8vRX4wzCANDRT
1 participant
PR 2 of 2 — Label ink-aware sizing + device-pixel rounding
Part 2 of the ink-aware width + device-pixel size rounding work (issues #213, #98, #57). This PR routes
MTMathUILabel's reporting/positioning surface through the ink extent added in PR 1, rounds the reported size up to the device-pixel grid, and re-queries when the backing scale becomes known.Plan:
docs/plans/2026-07-14-ink-aware-width.mdLLD:
docs/lld/2026-07-06-ink-aware-width.mdStack
feature/ink-aware-width-pr1)Goal
Route the label's reporting/positioning surface through
inkWidth, round the reported size up to the device-pixel grid, and re-query when the backing scale becomes known.Commits (one per plan item)
[item 16]Route label sizing/alignment through inkWidth + ceil to the device-pixel grid[item 17]Resolve device-pixel scale (contentScaleFactor / backingScaleFactor)[item 18]Re-query intrinsic size on window/backing-scale change[item 19]Assert ink-clip render guarantees the feature actually providesTesting
iosMathTests/MTMathUILabelSizingTest.m(5 tests): inkWidth routing, device-pixel grid rounding, alignment, and scale-change re-query.iosMathTests/MTInkClippingRenderTest.m(2 tests): renders the laid-out display tree and asserts (a) trailing overhang glyphs never clip the right border at any alignment (Cluster A), and (b) tall constructs (\frac{1}{2},\int_0^1, …) report a pixel-aligned size with trailing ink contained inside the right border (Cluster B).Note on the Cluster B render assertion
The render test asserts the guarantee this feature actually provides — a pixel-aligned reported size (frame edges on whole device pixels, so no fractional-size resample) plus right-edge ink containment. It deliberately does not assert "zero ink in the top border pixel row": flush, ink-tight tall constructs legitimately place ink against their ink-tight vertical boundary, so that row contains genuine (un-clipped) ink. The true hairline artifact is a fractional-origin compositing resample, addressed by snapping the draw origin — deferred per LLD §2.8.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
sizeThatFits:calculations to use pixel-rounded measurements for consistent device-pixel rendering.Tests
sizeThatFits:,intrinsicContentSize, nil-latexbehavior, and intrinsic invalidation lifecycle.