[scroll] lock ComposeView's magnification, border and automatic content insets on macOS - #109
Conversation
…nt insets on macOS ComposeView's render bounds divided by the magnification and subtracted the border, and AppKit's automatic content insets wrote the window's title bar and toolbar overlap into the same `contentInsets` a caller sets, replacing the caller's value whenever the window's chrome changed. ComposeView never needed any of them, so they're now locked on macOS: `automaticallyAdjustsContentInsets` and `allowsMagnification` stay `false`, `magnification` stays 1, including through `setMagnification(_:centeredAt:)` and `magnify(toFit:)`, and `borderType` stays `.noBorder`. Changing them asserts and keeps the value, and the overrides are final. `renderBounds()` drops the border switch and the magnification division. iOS, tvOS and visionOS keep `contentInsetAdjustmentBehavior` adjustable, with `.never` as the default: UIKit keeps the caller's `contentInset` separate from the safe area, and layout and rendering work with automatic insets. Fitting content ignoring insets is tracked in #108. The document view and the clip view aren't guarded: removing or replacing them already fails at the next render pass, and a guard would only turn that into an assertion, at a small cost on every read. The render-bounds tests that existed only for borders are removed, and the ones that also covered scrollers or scaled bounds keep those parts. Fixes #103.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOn AppKit, ChangesComposeView scroll behavior
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No concrete merge-blocking issue is established. The AppKit restrictions and bounds-based sizing are consistent with the documented behavior; merge after normal platform checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change restricts customization rather than granting new access or authority. The main design risks are compatibility with downstream subclasses and reliance on AppKit preserving its default geometry settings. No introduced security vulnerability was established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation PR Resolution Implement the iOS automatic-inset lock with assertion and value preservation. Add the required Full details: Docstring CoverageExplanation Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #109 +/- ##
==========================================
+ Coverage 96.40% 96.41% +0.01%
==========================================
Files 114 114
Lines 7000 7026 +26
==========================================
+ Hits 6748 6774 +26
Misses 252 252
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… can't magnify it either Through an `NSScrollView` reference, the animator's `magnify(toFit:)` and `setMagnification(_:centeredAt:)` change the magnification without calling ComposeView's overrides, so a 100×100 view ended at a magnification of 0.25, showing a 400×400 area while its content laid out for 100×100. AppKit clamps every way of magnifying to `minMagnification` and `maxMagnification`, so ComposeView now sets both to 1 and locks them like the magnification: changing either asserts and keeps 1. AppKit doesn't read either while scrolling. The animator's `magnify(toFit:)` still scrolls to center the rect, which lays out and renders like any other scroll.
1 participant
Summary
ComposeViewnow keepsautomaticallyAdjustsContentInsetsandallowsMagnificationoff,magnification,minMagnificationandmaxMagnificationat 1, andborderTypeat.noBorder. Changing them asserts and keeps the value, and the animator can't magnify the view either.renderBounds(), which runs on every render pass, drops the border switch and the magnification division.contentInsetAdjustmentBehavioradjustable, with.neveras the default, as before.What changed
ComposeViewforautomaticallyAdjustsContentInsets,allowsMagnification,magnification,setMagnification(_:centeredAt:),magnify(toFit:)andborderType. A probe showed that AppKit never calls these setters itself, during setup, in a window, on a resize or while tiling, so the assertions fire only on a caller's change. It also showed that programmatic magnification works withoutallowsMagnification, and that each of the three magnification methods changes the magnification on its own, so all three are overridden.NSScrollViewreference, the animator'smagnify(toFit:)andsetMagnification(_:centeredAt:)change the magnification without calling the overrides (found in review: a 100×100 view ended at 0.25, showing a 400×400 area while its content laid out for 100×100). AppKit clamps every way of magnifying tominMagnificationandmaxMagnification, so both are set to 1 and locked the same way. AppKit never calls their setters itself and doesn't read them while scrolling. The animator'smagnify(toFit:)still scrolls to center the rect, which lays out and renders like any other scroll.renderBounds(): sizes the content from the bounds alone.test_magnification_animator_staysOnecalls both animator methods through anNSScrollViewreference, waits for each animation to finish, and checks that the magnification stays 1, the clip view keeps the view's size, and the rendered rows are the visible ones at full width. Without the range lock, it fails the way the review described..automaticset by the caller, a compose view in a navigation controller is inset by the bar, and the rendered rows match the visible area.ComposeView+RenderBoundsTests, the three tests that existed only for borders are removed. The legacy scrollers test keeps its content inset part without the border and magnification, and the scaled-bounds test uses a fractional frame instead of a border, covering both rounding directions at 1x and 2x.tile()entry no longer mentions the border.Decisions
Both are recorded on #103.
contentInsetseparate from the safe area adjustment, soScrollView's contract holds there. On macOS, AppKit writes its adjustment into the samecontentInsetsthe caller sets: in a probe, it replaced a caller's top inset of 10 with its own 32 when the toolbar hid. With automatic insets on iOS, layout and rendering work. The remaining gap, content that fits the bounds ignoring insets, also affects insets set by the caller on both platforms, so it's tracked in [scroll] MakeComposeView's scrolling and fitting content account for content insets #108.documentViewsetter, and would makecontentViewfinal. It would cost about 0.2 µs per scrolled frame.Test plan
renderBounds()runs under the tests.make formatandmake lint.Fixes #103
Summary by CodeRabbit
Behavior Changes
ComposeViewlays out content across its full bounds; borders and magnification no longer reduce the render area.ComposeViewrather than within its bounds.Documentation