Make windows sampler DPI aware #2493
Conversation
📝 WalkthroughWalkthroughRefactors DPI handling by initializing DPI awareness inside WindowsSampler, exposing a dpi_scale, and moving physical↔logical coordinate conversions into the sampler so the main loop operates on logical coordinates only. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Loop
participant Sampler as WindowsSampler
participant WinAPI as Win32 APIs (GDI, DPI)
Main->>Sampler: initialize WindowsSampler (creates dpi_scale, sets DPI context)
Main->>Sampler: get_cursor_position() -> logical cursor
Main->>Sampler: request sample_grid / sample_pixel with logical coords
Sampler->>WinAPI: convert logical -> physical (multiply by dpi_scale)
Sampler->>WinAPI: capture pixels (GetPixel / BitBlt + GetDIBits)
WinAPI-->>Sampler: raw pixel data (physical coords)
Sampler->>Sampler: convert physical -> logical (divide by dpi_scale) and return PixelData
Sampler-->>Main: PixelData (contains logical cursor and sampled pixels)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
34-53: Mocksample_pixeldoesn't match production DPI behavior.The mock's
sample_pixeltreatsx, yas physical coordinates without conversion, but the productionWindowsSampler::sample_pixel(windows.rs lines 61-62) converts logical to physical:// Production code let physical_x = (x as f64 * self.dpi_scale) as i32; let physical_y = (y as f64 * self.dpi_scale) as i32;This causes the DPI scaling tests (e.g.,
test_windows_sampler_dpi_150_percentat line 491) to fail because they pass logical coordinates expecting physical-based color results, but the mock doesn't perform the conversion.Proposed fix to align mock with production
fn sample_pixel(&mut self, x: i32, y: i32) -> Result<Color, String> { - // With DPI awareness enabled in the actual implementation: - // - x, y are already in physical coordinates (no conversion needed) - // - GetPixel expects physical coordinates - let physical_x = x; - let physical_y = y; + // With DPI awareness enabled in the actual implementation: + // - x, y are logical coordinates + // - Convert to physical coordinates internally (like production) + let physical_x = (x as f64 * self.dpi_scale) as i32; + let physical_y = (y as f64 * self.dpi_scale) as i32;
🧹 Nitpick comments (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
326-351: Testtest_windows_sampler_grid_pixel_alignmentonly works at 100% DPI due to mock inconsistency.This test compares
sample_gridresults to individualsample_pixelcalls, but the mock's inconsistent DPI handling (as noted above) means this test would fail at higher DPI scales. The test only passes because it usesMockWindowsSampler::new(1920, 1080)which defaults to 1.0 DPI scale.Consider adding a similar test with
new_with_dpito verify alignment at higher DPI scales once the mock is fixed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
electron-app/magnifier/rust-sampler/Cargo.tomlelectron-app/magnifier/rust-sampler/src/main.rselectron-app/magnifier/rust-sampler/src/sampler/windows.rselectron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs
🧰 Additional context used
🧬 Code graph analysis (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
sample_grid(107-228)electron-app/magnifier/rust-sampler/src/types.rs (1)
sample_grid(87-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Test Rust Sampler (windows-latest)
🔇 Additional comments (9)
electron-app/magnifier/rust-sampler/Cargo.toml (1)
31-37: LGTM!The
Win32_UI_HiDpifeature addition correctly enables access to the DPI awareness APIs (SetProcessDpiAwarenessContext,DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2) used in the Windows sampler implementation.electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
677-719: Good test coverage for DPI-aware grid sampling.This test thoroughly validates the coordinate conversion at 200% DPI by checking center, top-left, and bottom-right pixels against expected physical coordinate-based colors. The calculations in comments (e.g., "virtual (998, 498) → physical (1996, 996)") help document the expected behavior.
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (5)
17-44: DPI awareness initialization looks correct.Setting
DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2before GDI calls is the correct approach. Silently ignoring the error is reasonable since the app will fall back to system DPI awareness if this fails.One consideration:
SetProcessDpiAwarenessContextis a per-process setting and can only be called once successfully. If another part of the Electron app (or a dependency) has already set DPI awareness, this call will fail. The silent ignore handles this gracefully.
55-82: LGTM!The logical-to-physical coordinate conversion is correctly implemented. The error message preserving original logical coordinates aids debugging.
84-103: LGTM!The physical-to-logical conversion for cursor position correctly mirrors the inverse of the sampling conversion, ensuring coordinate consistency with Electron's coordinate system.
107-121: Verify intended grid sampling behavior at high DPI.The current implementation samples
grid_sizephysical pixels centered around the physical cursor position. At 200% DPI, a 9×9 grid samples a 9×9 physical pixel region, which corresponds to roughly 4.5×4.5 logical pixels on screen.This is likely the correct behavior for a color picker (sampling actual physical pixels), but differs from a "logical pixel" model where you'd capture
grid_size * dpi_scalephysical pixels to covergrid_sizelogical pixels.Please confirm this physical-pixel sampling behavior is intended for the magnifier's use case.
231-268: Fallback implementation is consistent with optimized path.The
sample_grid_fallbackcorrectly mirrors the DPI handling of the optimizedsample_gridmethod, ensuring consistent behavior when BitBlt fails.electron-app/magnifier/rust-sampler/src/main.rs (2)
98-103: Clean API simplification.Removing
dpi_scalefrom the sampling loop signature and encapsulating DPI handling within the platform-specific samplers is a good design improvement. This follows the pattern established by macOS and keeps the main loop platform-agnostic.
137-180: LGTM!The sampling loop now works entirely with logical coordinates, delegating DPI conversion to the platform samplers. The
cursor_posis correctly propagated to sampling functions and stored inPixelData, maintaining consistency with Electron's coordinate system.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (3)
633-641: Misleading comment on coordinate conversion direction.The comment on line 638 states "Physical coordinate 6000 maps to logical 3000" but the test passes
6000as a logical coordinate tosample_pixel, which then converts it to physical (6000 × 2.0 = 12000). The test correctly fails because 12000 exceeds the physical bounds, but the comment describes the conversion backwards.📝 Suggested comment fix
#[test] fn test_windows_sampler_dpi_out_of_bounds() { // Test that out-of-bounds checking works with DPI scaling let mut sampler = MockWindowsSampler::new_with_dpi(2560, 1440, 2.0); - // Physical coordinate 6000 maps to logical 3000, which is > 2560 + // Logical coordinate 6000 maps to physical 12000 (6000 * 2.0), which exceeds physical bounds 2560 let result = sampler.sample_pixel(6000, 3000); assert!(result.is_err(), "Should fail for out-of-bounds coordinates"); }
698-706: Incorrect modulo arithmetic in comments.The comments on lines 700-702 have incorrect calculations:
2000 % 256 = 208, not 2241000 % 256 = 232✓ (correct)3000 % 256 = 184, not 200The assertions will fail at runtime because the expected values don't match the actual modulo results.
🐛 Proposed fix
// Center samples at virtual position (1000, 500) -> physical (2000, 1000) // Colors are based on physical coordinates - let expected_b = (2000 % 256) as u8; // 2000 % 256 = 224 + let expected_b = (2000 % 256) as u8; // 2000 % 256 = 208 let expected_g = (1000 % 256) as u8; // 1000 % 256 = 232 - let expected_r = ((2000 + 1000) % 256) as u8; // 3000 % 256 = 200 + let expected_r = ((2000 + 1000) % 256) as u8; // 3000 % 256 = 184
710-718: Incorrect modulo arithmetic in edge pixel comments.Similar issue with the top-left and bottom-right pixel calculations:
- Line 712:
1996 % 256 = 204, not 220- Line 717:
2004 % 256 = 212, not 228These assertions will fail at runtime.
🐛 Proposed fix
// Top-left: virtual (998, 498) -> physical (1996, 996) let top_left = &grid[0][0]; - assert_eq!(top_left.b, (1996 % 256) as u8); // 1996 % 256 = 220 + assert_eq!(top_left.b, (1996 % 256) as u8); // 1996 % 256 = 204 assert_eq!(top_left.g, (996 % 256) as u8); // 996 % 256 = 228 // Bottom-right: virtual (1002, 502) -> physical (2004, 1004) let bottom_right = &grid[4][4]; - assert_eq!(bottom_right.b, (2004 % 256) as u8); // 2004 % 256 = 228 + assert_eq!(bottom_right.b, (2004 % 256) as u8); // 2004 % 256 = 212 assert_eq!(bottom_right.g, (1004 % 256) as u8); // 1004 % 256 = 236
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs
🧰 Additional context used
🧬 Code graph analysis (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (4)
electron-app/magnifier/rust-sampler/src/types.rs (1)
sample_grid(87-105)electron-app/magnifier/rust-sampler/src/sampler/linux.rs (1)
sample_grid(256-277)electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs (1)
sample_grid(396-435)electron-app/magnifier/rust-sampler/src/sampler/macos.rs (1)
sample_grid(125-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Rust Sampler (windows-latest)
- GitHub Check: Tests
🔇 Additional comments (4)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (4)
9-31: LGTM!The
MockWindowsSamplerstruct and constructors correctly model DPI-aware behavior with clear documentation distinguishing physical dimensions and DPI scale factor. The default 1.0 scale for 100% DPI is appropriate.
33-98: LGTM!The
PixelSamplertrait implementation correctly models the DPI-aware coordinate flow:
sample_pixelaccepts logical coordinates and converts to physical internallyget_cursor_positionreturns logical coordinates derived from simulated physical positionsample_gridoperates in logical space with internal physical conversionThis aligns with the macOS pattern mentioned in the comments and the production behavior described in the summary.
127-135: LGTM!The cursor position test correctly validates that at 100% DPI (scale 1.0), the physical cursor position (200, 200) equals the logical position (200, 200).
721-767: LGTM!The remaining tests for multi-monitor simulation, high DPI scaling, rapid sampling, and error messages are straightforward and correctly validate the expected behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.