fix(ui): support macOS button and label padding - #10234
proggeramlug wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughmacOS padding now supports buttons, text labels, and attributed-text labels. The implementation handles flipped coordinates, invalidates intrinsic sizing, preserves native widget properties, avoids duplicate runtime subclasses, reports unsupported views, and adds integration coverage and documentation. ChangesmacOS native padding
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant WidgetFactory
participant set_edge_insets
participant NSButton
participant PerryButtonPadding
WidgetFactory->>set_edge_insets: apply edge insets
set_edge_insets->>NSButton: route button padding
NSButton->>PerryButtonPadding: install or reuse subclass
PerryButtonPadding->>NSButton: invalidate intrinsic size and redraw
Merge Risk: ⚪ Minimal · up to MacOS padding behavior for labels and buttons is implemented with regression coverage for layout, rendering, resets, and drag/drop ordering. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 34.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 7 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
1 participant
Summary
macOS
setPaddingsilently ignored buttons and text labels. GiveButton,Text, andAttributedTextreal intrinsic-size and content-padding behavior, while preserving native styling and interaction. Unsupported native views now report a development-build diagnostic pointing callers to a padded stack.Changes
Related issue
Fixes #10159.
Test plan
On macOS arm64, using local development builds:
cargo build -p perry-ui-macos -j3— passed, including the native backend static library.RUST_TEST_THREADS=1 cargo test -p perry-ui-macos -j3— passed the unit test, existingnative_widget_orderintegration target, and newnative_widget_paddingintegration target.(top:3, left:5, bottom:7, right:11)adds exactly 16×10 points and moves the rendered content by 5×3 points.scripts/pre-tag-check.sh --quick— all checks passed except the existing public benchmark evidence-freshness failure. Its artifact, verifier, and every fingerprint input are unchanged from base6000a00dfe.git diff --check— passed.The AppKit tests require macOS and run on the process main thread. No full workspace or non-macOS backend build was run. Other native control families still need a padded stack, as documented. Native content-inset support remains limited to the listed controls.
Checklist
Cargo.toml,Cargo.lock,CLAUDE.md, andCHANGELOG.mdunchanged.fix:prefix.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
CI follow-up: the docs
build-and-freshnessjob fails its gettext catalog freshness check, with repository-wide catalog diffs as well as the new styling text. The separate TLS checker and lint/check/warnings jobs are also red; remaining gap and GC jobs are still running. Local macOS validation above does not imply a green CI gate.