fix(ui-windows): DPI-scale layout intrinsics (refs #5884); unblock crate unit tests#6637
Conversation
layout.rs authored every intrinsic control size in raw 96-DPI pixels while fonts/window/spacing were already DPI-scaled, so fixed-height controls clipped their own scaled fonts on >100% displays (#5884 family). Route all intrinsic constants, button padding, the combobox dropdown inflate, and measure_text_height fallbacks through a central dpi() helper; measured text extents stay physical. scale_by(px, s<=1) is identity so 100% displays are bit-for-bit unchanged (unit-tested). Also add #[link(name = dwmapi)] to the hand-declared DWM externs: without it cargo test -p perry-ui-windows failed at link (LNK2019 DwmSetWindowAttribute), which had silently kept the whole in-crate test suite unrunnable.
|
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)
📝 WalkthroughWalkthroughWindows layout calculations now scale intrinsic and text dimensions by DPI, including picker and combobox sizing, with helper tests. DWM API usage also declares explicit ChangesWindows DPI-aware layout
DWM API linkage
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-ui-windows/src/layout.rs (1)
217-221: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winScale the picker/combobox create-time height too. The
dpi(200)move here should stay in sync with the fixed200height incrates/perry-ui-windows/src/widgets/picker.rsandcrates/perry-ui-windows/src/widgets/combobox.rs; otherwise high-DPI controls can be created at the wrong size before the first relayout.🤖 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 `@crates/perry-ui-windows/src/layout.rs` around lines 217 - 221, Update the picker/combobox create-time height calculation in the move_h branch to scale the additional 200-pixel offset with DPI, keeping it consistent with the fixed height values in the Picker and Combobox widget implementations and preserving the existing minimum height behavior.
🤖 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 `@crates/perry-ui-windows/src/layout.rs`:
- Around line 390-408: Update the padding comment in the WidgetKind::Button
sizing branch to clarify that the code adds 16px total vertical padding and 32px
total horizontal padding, representing 8px and 16px per side respectively. Keep
the existing dpi(16) and dpi(32) calculations unchanged.
---
Outside diff comments:
In `@crates/perry-ui-windows/src/layout.rs`:
- Around line 217-221: Update the picker/combobox create-time height calculation
in the move_h branch to scale the additional 200-pixel offset with DPI, keeping
it consistent with the fixed height values in the Picker and Combobox widget
implementations and preserving the existing minimum height behavior.
🪄 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: 67305a62-7955-4d08-a5a5-a13dbc62e711
📒 Files selected for processing (3)
crates/perry-ui-windows/src/app.rscrates/perry-ui-windows/src/dwm.rscrates/perry-ui-windows/src/layout.rs
| WidgetKind::Button => { | ||
| #[cfg(target_os = "windows")] | ||
| { | ||
| if let Some(hwnd) = widgets::get_hwnd_safe(handle) { | ||
| let size = measure_text_height(hwnd, cross_size, vertical); | ||
| // Add padding: 8px vertical, 16px horizontal | ||
| return if vertical { size + 16 } else { size + 32 }; | ||
| // Add padding: 8px vertical, 16px horizontal (logical) | ||
| return if vertical { | ||
| size + dpi(16) | ||
| } else { | ||
| size + dpi(32) | ||
| }; | ||
| } | ||
| } | ||
| if vertical { | ||
| 34 | ||
| dpi(34) | ||
| } else { | ||
| 100 | ||
| dpi(100) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the padding comment vs. actual values.
The comment states "8px vertical, 16px horizontal (logical)" but the code adds dpi(16) and dpi(32) respectively — double the stated figures. This is likely intentional (per-side padding: 8px × 2 sides = 16, 16px × 2 sides = 32), but as written the comment invites a future contributor to "fix" the code to match the literal 8/16 values, silently halving button padding.
✏️ Suggested comment clarification
- // Add padding: 8px vertical, 16px horizontal (logical)
+ // Add padding (total, both sides): 16px vertical (8px top + 8px bottom),
+ // 32px horizontal (16px left + 16px right), logical pixels.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WidgetKind::Button => { | |
| #[cfg(target_os = "windows")] | |
| { | |
| if let Some(hwnd) = widgets::get_hwnd_safe(handle) { | |
| let size = measure_text_height(hwnd, cross_size, vertical); | |
| // Add padding: 8px vertical, 16px horizontal | |
| return if vertical { size + 16 } else { size + 32 }; | |
| // Add padding: 8px vertical, 16px horizontal (logical) | |
| return if vertical { | |
| size + dpi(16) | |
| } else { | |
| size + dpi(32) | |
| }; | |
| } | |
| } | |
| if vertical { | |
| 34 | |
| dpi(34) | |
| } else { | |
| 100 | |
| dpi(100) | |
| } | |
| } | |
| WidgetKind::Button => { | |
| #[cfg(target_os = "windows")] | |
| { | |
| if let Some(hwnd) = widgets::get_hwnd_safe(handle) { | |
| let size = measure_text_height(hwnd, cross_size, vertical); | |
| // Add padding (total, both sides): 16px vertical (8px top + 8px bottom), | |
| // 32px horizontal (16px left + 16px right), logical pixels. | |
| return if vertical { | |
| size + dpi(16) | |
| } else { | |
| size + dpi(32) | |
| }; | |
| } | |
| } | |
| if vertical { | |
| dpi(34) | |
| } else { | |
| dpi(100) | |
| } | |
| } |
🤖 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 `@crates/perry-ui-windows/src/layout.rs` around lines 390 - 408, Update the
padding comment in the WidgetKind::Button sizing branch to clarify that the code
adds 16px total vertical padding and 32px total horizontal padding, representing
8px and 16px per side respectively. Keep the existing dpi(16) and dpi(32)
calculations unchanged.
1 participant
Problem
DPI handling in the Windows UI backend is split-brain: the window size, fonts, stack spacing, and explicit
setSizedimensions are all multiplied byget_dpi_scale()at their entry points — but the layout engine's intrinsic control sizes never were.layout.rscontained no DPI reference at all:TextField=30,Toggle=24,Slider=24,Picker=28,ProgressView=20,DatePicker=28,Combobox=28, button padding+16/+32, the combobox dropdown inflate+200, and every fallback constant were raw 96-DPI pixels.On a 125–200% display (the default on modern laptops) every fixed-height control clips its own DPI-scaled font — the #5884 family of bugs. Because
layout.rsnever saw the scale, every future widget added there inherited the bug.Fix
dpi(px)helper inlayout.rs(scale_by(px, scale)pure core +get_dpi_scale()wrapper). All intrinsic constants, the button text padding, the combobox dropdown inflate, and themeasure_text_heightfallbacks/clamps now route through it.measure_text_heightresults (physical pixels — fonts are already scaled at creation),fixed_width/height(scaled at thesetSizeFFI), stack spacing (scaled at creation), insets (set unscaled at their own entry point — flagged in review as a follow-up if that entry point gains scaling).scale_byclamps: scale ≤ 1.0 returns the constant unchanged (Windows never scales below 100%; an unset/zero reading must not shrink controls). Rounds half-up at fractional factors.Also in this PR: the crate's unit tests could never link
cargo test -p perry-ui-windowsfailed withLNK2019: DwmSetWindowAttribute / DwmExtendFrameIntoClientArea— the hand-declared dwmapi externs (dwm.rs,app.rs) had no#[link(name = "dwmapi")], and only the final app link addsdwmapi.lib. That means the crate's existing in-crate#[cfg(test)]suites (app.rs, screenshot.rs, ffi/system.rs, lib.rs) could not have been run this way. Two#[link]attributes fix it; the test harness now links and runs.Verification (Windows 11 x64)
cargo test -p perry-ui-windows --release layout— links (first time) and passes 3/3 (scale_byidentity at 1.0, degenerate-scale guard, rounding at 125/150/175/200%).cargo build --release -p perry-ui-windowsclean; smoke app renders identically at this box's 96 DPI (scale 1.0 ⇒ mathematically a no-op, per the unit test).Refs #5884. No version bump / changelog per maintainer instruction.
Summary by CodeRabbit