Fix Windows DPI issues more #2470
Conversation
📝 WalkthroughWalkthroughThe PR changes WindowsSampler to operate in physical pixel space (physical = virtual * dpi_scale) across sampling paths, removes stored virtual screen dimensions and related multi-monitor queries/imports, and updates mocks/tests to use physical-width/height and adjusted DPI-aware expectations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 (3)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (3)
538-552: Test logic appears inconsistent with the DPI model.The test comment says "Physical cursor at 2000, 1000 should map to logical 1000, 500", but then passes
(2000, 1000)directly tosample_grid. According to the implementation,sample_gridexpects virtual coordinates (which it converts to physical internally).If
2000, 1000are physical coordinates and DPI is 2.0, you should pass virtual coordinates(1000, 500)tosample_grid. The mock then converts1000 * 2.0 = 2000internally.As written, passing
(2000, 1000)with 2.0 DPI will compute physical center at4000, 2000, which may exceed the 2560×1440 screen bounds.🔎 Suggested fix
#[test] fn test_windows_sampler_dpi_grid_sampling_200_percent() { // Test grid sampling at 200% DPI let mut sampler = MockWindowsSampler::new_with_dpi(2560, 1440, 2.0); - // Physical cursor at 2000, 1000 should map to logical 1000, 500 - let grid = sampler.sample_grid(2000, 1000, 5, 1.0).unwrap(); + // Virtual cursor at 500, 250 should map to physical 1000, 500 + let grid = sampler.sample_grid(500, 250, 5, 1.0).unwrap(); assert_eq!(grid.len(), 5); - // Center pixel should be at logical coordinates (1000, 500) + // Center pixel should be at physical coordinates (1000, 500) let center = &grid[2][2]; assert_eq!(center.b, (1000 % 256) as u8); assert_eq!(center.g, (500 % 256) as u8); }
592-621: Test logic is inverted—color derivation contradicts the mock.The mock's
sample_pixelcomputes color from physical coordinates:physical_x = x * dpi_scale. But this test expectscolor.bto equal(physical_x / scale) % 256, which inverts the relationship.For example, at 200% scale with
physical_x = 1000passed in:
- Mock computes:
physical_x = 1000 * 2.0 = 2000, socolor.b = 2000 % 256 = 208- Test expects:
expected_b = (1000 / 2.0) % 256 = 500 % 256 = 244This test will fail. The loop variable
physical_xis actually being used as a virtual coordinate input.🔎 Suggested fix
#[test] fn test_windows_sampler_dpi_various_scales() { // Test common Windows DPI scaling values let scales = vec![ (1.0, "100%"), // No scaling (1.25, "125%"), // Recommended for 1080p (1.5, "150%"), // Recommended for 1440p (1.75, "175%"), // Less common (2.0, "200%"), // Recommended for 4K/5K (2.5, "250%"), // High DPI (3.0, "300%"), // Very high DPI ]; for (scale, description) in scales { - let mut sampler = MockWindowsSampler::new_with_dpi(1920, 1080, scale); + // Physical dimensions should accommodate scaled coordinates + let physical_w = (1920.0 * scale) as i32; + let physical_h = (1080.0 * scale) as i32; + let mut sampler = MockWindowsSampler::new_with_dpi(physical_w, physical_h, scale); - let physical_x = 1000; - let expected_logical_x = (physical_x as f64 / scale) as i32; + // Pass virtual coordinates, mock converts to physical + let virtual_x = 500; + let expected_physical_x = (virtual_x as f64 * scale) as i32; - let color = sampler.sample_pixel(physical_x, 500).unwrap(); + let color = sampler.sample_pixel(virtual_x, 250).unwrap(); // Verify coordinate conversion works for this scale - let expected_b = (expected_logical_x % 256) as u8; + let expected_b = (expected_physical_x % 256) as u8; assert_eq!( color.b, expected_b, - "DPI scaling {} failed: physical {} should map to logical {}", - description, physical_x, expected_logical_x + "DPI scaling {} failed: virtual {} should map to physical {}", + description, virtual_x, expected_physical_x ); } }
667-719: Test has inverted coordinate logic that may cause false positives.This test creates a sampler with physical
2560×1440at 2.0 DPI, then passesphysical_center_x = 2000tosample_grid. Butsample_gridexpects virtual coordinates—passing 2000 as virtual yields physical 4000, which exceeds the 2560 width.The test then tries to verify grid pixels by:
- Converting the "physical" center back to "logical":
2000 / 2.0 = 1000- Then converting logical back to physical:
logical * 2.0This double-conversion masks the bug because the math cancels out. The test should use virtual coordinates directly.
🔎 Suggested fix
#[test] fn test_windows_sampler_dpi_grid_edge_alignment() { // Test that grid pixels align correctly with individual samples at 200% DPI - let mut sampler = MockWindowsSampler::new_with_dpi(2560, 1440, 2.0); + // Physical screen 5120x2880, virtual 2560x1440 at 200% DPI + let mut sampler = MockWindowsSampler::new_with_dpi(5120, 2880, 2.0); - let physical_center_x = 2000; - let physical_center_y = 1000; + // Use virtual coordinates (what Electron passes) + let virtual_center_x = 1000; + let virtual_center_y = 500; let grid_size = 5; - let grid = sampler.sample_grid(physical_center_x, physical_center_y, grid_size, 1.0).unwrap(); + let grid = sampler.sample_grid(virtual_center_x, virtual_center_y, grid_size, 1.0).unwrap(); // Verify each grid pixel matches individual sample let half_size = (grid_size / 2) as i32; - - // Convert physical center to logical coordinates first - let logical_center_x = (physical_center_x as f64 / 2.0) as i32; - let logical_center_y = (physical_center_y as f64 / 2.0) as i32; for row in 0..grid_size { for col in 0..grid_size { - // Calculate offsets in logical space (to match sample_grid behavior) - let offset_x = (col as i32 - half_size); - let offset_y = (row as i32 - half_size); - - // Apply offsets in logical space - let logical_x = logical_center_x + offset_x; - let logical_y = logical_center_y + offset_y; - - // Convert back to physical coordinates for sample_pixel - let physical_x = (logical_x as f64 * 2.0) as i32; - let physical_y = (logical_y as f64 * 2.0) as i32; + // Virtual coords for this grid cell + let virtual_x = virtual_center_x + (col as i32 - half_size); + let virtual_y = virtual_center_y + (row as i32 - half_size); let grid_color = &grid[row][col]; - let individual_color = sampler.sample_pixel(physical_x, physical_y).unwrap(); + let individual_color = sampler.sample_pixel(virtual_x, virtual_y).unwrap(); // ...assertions... } } }
🧹 Nitpick comments (2)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
623-631: Bounds check uses inconsistent coordinate interpretation.The sampler has physical dimensions
2560×1440with 2.0 DPI scale. The test passes(6000, 3000)expecting an out-of-bounds error.With the mock's logic:
physical_x = 6000 * 2.0 = 12000, which indeed exceeds2560. However, the comment says "Physical coordinate 6000 maps to logical 3000", which inverts the actual conversion direction.The test happens to pass (both interpretations exceed bounds), but the comment is misleading.
🔎 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 + // Virtual coordinate 6000 maps to physical 12000, which exceeds 2560 let result = sampler.sample_pixel(6000, 3000); assert!(result.is_err(), "Should fail for out-of-bounds coordinates"); }
633-665: Same coordinate confusion in fallback test.The test passes
physical_center_x = 1000tosample_grid, but based on the mock, this is interpreted as virtual coordinates and scaled to physical 2000. The variable naming and comments suggest physical coordinates are being passed directly.The test logic may still pass due to the deterministic color formula, but the naming is misleading.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
electron-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 (1)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (4)
sample_pixel(59-86)new(19-47)get_cursor_position(88-100)sample_grid(104-228)
⏰ 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 (5)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (3)
61-68: LGTM! DPI coordinate conversion for sample_pixel is correct.The conversion from virtual to physical coordinates (
physical = virtual * dpi_scale) correctly handles DPI-aware Electron apps whereGetCursorPosreturns virtual coordinates butGetPixelexpects physical coordinates.
108-116: Verify the grid offset behavior at high DPI scales.The center is correctly converted to physical coordinates, but the grid offsets (
half_size) remain in logical pixel units. This means:
- At 200% DPI with a 9×9 grid, you sample physical pixels at
[center-4, center+4](9 physical pixels apart)- Rather than covering the equivalent of 9 virtual pixels (which would be 18 physical pixels apart)
If the intent is to sample each physical pixel within the grid (1:1 physical sampling around the cursor), this is correct. If the intent is to sample each virtual pixel the user sees, the offsets would need scaling too.
Please confirm this matches the expected magnifier behavior.
238-250: Fallback implementation is consistent with sample_grid.The coordinate conversion and offset logic matches
sample_grid, ensuring consistent behavior when BitBlt fails and falls back to pixel-by-pixel sampling.electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
24-30: LGTM! Parameter rename improves clarity.Renaming to
physical_width/physical_heightmakes the coordinate space explicit, aligning with the DPI-aware model.
35-51: Mock implementation correctly mirrors the production DPI behavior.The mock now converts virtual input coordinates to physical space using
dpi_scale, matching the realWindowsSampler::sample_pixelimplementation.
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 (2)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
685-710: Critical: Undefined variables in assertion messages.Lines 699 and 704 reference
physical_xandphysical_yvariables that are not defined in scope. The code only definesvirtual_xandvirtual_yat lines 690-691.🔎 Proposed fix
assert_eq!( grid_color.r, individual_color.r, - "Mismatch at grid[{}][{}] (physical {}, {})", - row, col, physical_x, physical_y + "Mismatch at grid[{}][{}] (virtual {}, {})", + row, col, virtual_x, virtual_y ); assert_eq!( grid_color.g, individual_color.g, - "Mismatch at grid[{}][{}] (physical {}, {})", - row, col, physical_x, physical_y + "Mismatch at grid[{}][{}] (virtual {}, {})", + row, col, virtual_x, virtual_y ); assert_eq!( grid_color.b, individual_color.b, - "Mismatch at grid[{}][{}] (physical {}, {})", - row, col, physical_x, physical_y + "Mismatch at grid[{}][{}] (virtual {}, {})", + row, col, virtual_x, virtual_y );
627-632: Minor: Misleading comment about coordinate mapping.The comment states "Physical coordinate 6000 maps to logical 3000" but this reverses the actual conversion. The test passes virtual coordinates (6000, 3000) to
sample_pixel, which converts them to physical (12000, 6000) at 200% DPI. The correct explanation would be: "Virtual coordinates (6000, 3000) map to physical (12000, 6000), exceeding the 2560x1440 physical bounds."🔎 Proposed fix
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 + // Virtual coordinates (6000, 3000) map to physical (12000, 6000), exceeding physical bounds (2560, 1440) let result = sampler.sample_pixel(6000, 3000); assert!(result.is_err(), "Should fail for out-of-bounds coordinates"); }
📜 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 (2)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (4)
sample_pixel(59-86)new(19-47)get_cursor_position(88-100)sample_grid(104-228)electron-app/magnifier/rust-sampler/tests/types_tests.rs (2)
sample_pixel(129-136)get_cursor_position(138-140)
⏰ 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 (2)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
9-90: LGTM! Mock implementation correctly simulates DPI-aware coordinate handling.The mock properly converts virtual coordinates to physical space before bounds checking and color generation, matching the real Windows implementation behavior.
476-536: Excellent DPI test coverage.The test suite comprehensively covers DPI scaling scenarios (100%, 150%, 200%, and more) with correct virtual-to-physical coordinate conversions. The test cases at lines 510-516 provide good documentation of the expected mapping behavior.
Also applies to: 594-622
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs
Outdated
Show resolved
Hide resolved
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)
624-632: Minor: Confusing comment about coordinate mapping.The test logic is correct, but the comment on line 629 is misleading. It states "Physical coordinate 6000 maps to logical 3000," but
sample_pixelaccepts virtual coordinates, not physical. The actual flow is: virtual 6000 → physical 12000 (6000 × 2.0), which exceeds the physical screen width of 2560 and correctly fails.🔎 Suggested clarification
- // Physical coordinate 6000 maps to logical 3000, which is > 2560 + // Virtual coordinate 6000 maps to physical 12000, which exceeds physical screen width 2560 let result = sampler.sample_pixel(6000, 3000);
♻️ Duplicate comments (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
669-712: Critical: Grid verification logic remains incorrect for DPI scaling.This is the same critical issue identified in the previous review. The test adds grid offsets directly to virtual coordinates (line 690), but
sample_gridapplies those offsets in physical space (line 72).At 200% DPI with
virtual_center_x = 1000:
physical_center_x = 2000- For
col = 0(offset = -2), grid samples physical coordinate2000 + (-2) = 1998- Current test:
virtual_x = 1000 + (-2) = 998→ converts to physical1996❌- Correct: Convert physical
1998back to virtual999✓The test currently passes only for the center pixel (offset = 0) and will fail for any other grid position at DPI ≠ 100%.
🔎 Proposed fix to convert physical coordinates back to virtual
+ // Convert virtual center to physical to match grid implementation + let physical_center_x = (virtual_center_x as f64 * sampler.dpi_scale) as i32; + let physical_center_y = (virtual_center_y as f64 * sampler.dpi_scale) as i32; + // Calculate offsets (applied in physical space by our implementation) - let offset_x = col as i32 - half_size; - let offset_y = row as i32 - half_size; + let offset_x = (col as i32 - half_size); + let offset_y = (row as i32 - half_size); - // Virtual coordinates for this grid position - let virtual_x = virtual_center_x + offset_x; - let virtual_y = virtual_center_y + offset_y; + // Grid samples at this physical position + let physical_x = physical_center_x + offset_x; + let physical_y = physical_center_y + offset_y; + + // Convert back to virtual coordinates for verification + let virtual_x = (physical_x as f64 / sampler.dpi_scale).round() as i32; + let virtual_y = (physical_y as f64 / sampler.dpi_scale).round() as i32; let grid_color = &grid[row][col]; let individual_color = sampler.sample_pixel(virtual_x, virtual_y).unwrap();Note: This requires accessing
sampler.dpi_scale. You can either make the field public or add a getter method toMockWindowsSampler.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
electron-app/magnifier/rust-sampler/src/sampler/windows.rselectron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- electron-app/magnifier/rust-sampler/src/sampler/windows.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 (2)
sample_pixel(48-75)new(17-36)electron-app/magnifier/rust-sampler/src/types.rs (2)
sample_pixel(81-81)new(12-18)
⏰ 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/tests/windows_sampler_tests.rs (9)
9-13: LGTM: Struct fields correctly document physical dimensions.The updated comments clearly distinguish physical screen dimensions from virtual coordinates, which is essential for understanding DPI-aware behavior.
24-30: LGTM: Constructor signature clarifies physical dimension parameters.The renamed parameters (
physical_width,physical_height) make the DPI-aware coordinate system explicit and align with the implementation inwindows.rs.
34-52: LGTM: sample_pixel correctly implements DPI-aware coordinate conversion.The implementation properly simulates the Windows DPI behavior where virtual coordinates are converted to physical coordinates before sampling, matching the pattern in
windows.rslines 47-74.
54-57: LGTM: get_cursor_position correctly returns virtual coordinates.The comment and implementation properly simulate
GetCursorPosbehavior, which returns virtual coordinates in DPI-aware applications.
59-89: LGTM: sample_grid correctly operates in physical pixel space.The grid sampling override properly:
- Converts the virtual center to physical coordinates (lines 64-66)
- Applies offsets in physical space (lines 72-73)
- Performs bounds checking and color calculation using physical coordinates (lines 76-82)
This accurately simulates DPI-aware grid sampling behavior.
476-500: LGTM: DPI scaling tests correctly verify coordinate conversion.The tests at 150% and 200% DPI properly validate virtual-to-physical coordinate conversion, with assertions that verify colors are computed from the correct physical coordinates.
504-536: LGTM: Comprehensive coordinate conversion validation.The test thoroughly validates virtual-to-physical coordinate conversion at 200% DPI across multiple test cases, with clear documentation of expected mappings.
538-591: LGTM: Grid sampling tests correctly validate DPI-aware behavior.Tests verify that grid sampling operates in physical space and produces colors based on physical coordinates, properly validating the fix for the reported 200% DPI offset issue.
594-622: LGTM: Comprehensive validation across multiple DPI scales.The test validates DPI-aware coordinate conversion across common Windows scaling factors (100%-300%), ensuring the implementation works correctly at various DPI settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
624-632: Clarify the misleading comment about coordinate conversion.The comment on line 629 states "Physical coordinate 6000 maps to logical 3000" but the test actually passes virtual coordinate 6000 to
sample_pixel, which converts it to physical 12000 (6000 × 2.0), exceeding the screen width of 2560. The test logic is correct, but the comment describes the opposite conversion.🔎 Suggested comment clarification
- // Physical coordinate 6000 maps to logical 3000, which is > 2560 + // Virtual coordinate 6000 maps to physical 12000, which exceeds physical width 2560 let result = sampler.sample_pixel(6000, 3000); assert!(result.is_err(), "Should fail for out-of-bounds coordinates");
📜 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 (1)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (2)
sample_pixel(48-75)new(17-36)
⏰ 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 (2)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
9-90: Mock implementation correctly reflects DPI-aware coordinate handling.The mock sampler properly simulates the virtual-to-physical coordinate conversion that the real WindowsSampler performs. The implementation consistently:
- Accepts virtual coordinates as input (what GetCursorPos returns)
- Converts to physical coordinates via
dpi_scalemultiplication (what GetPixel expects)- Performs bounds checking and color calculations in physical space
This aligns with the real implementation shown in the relevant code snippets.
462-717: Comprehensive DPI scaling tests correctly validate virtual-to-physical conversion.The DPI test suite thoroughly validates the coordinate conversion across multiple scaling factors (100% through 300%). The tests properly:
- Set up physical screen dimensions that match the scaling factor
- Pass virtual coordinates to
sample_pixel- Verify colors match expected physical coordinate calculations
- Test grid sampling with correct physical-space offset calculations
Notably,
test_windows_sampler_dpi_grid_edge_alignment(lines 668-717) correctly addresses the critical issue mentioned in the past review comments by:
- Converting the virtual center to physical coordinates (lines 687-688)
- Verifying the grid samples at the correct physical positions (lines 703-716)
- Applying offsets in physical space rather than virtual space
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.