Still trying to fix Windows #2471
Conversation
|
Warning Rate limit exceeded@RobbieTheWagner has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR refactors DPI coordinate handling in the Windows sampler. The 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)
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: 1
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 (2)
33-52: Mocksample_pixelbehavior doesn't match real implementation.The mock applies DPI scaling (lines 37-38:
physical_x = x * dpi_scale), but the realWindowsSampler::sample_pixelinwindows.rsuses coordinates directly without any DPI conversion:// Real implementation (windows.rs line 55) let color_ref = GetPixel(self.hdc, x, y);This mismatch means tests like
test_windows_sampler_dpi_200_percentwill pass with the mock but don't accurately reflect production behavior. Either:
- The real implementation should also apply DPI scaling, or
- The mock should use coordinates directly to match the real implementation
If the real implementation is correct (no DPI scaling), update mock to match:
fn sample_pixel(&mut self, x: i32, y: i32) -> Result<Color, String> { - // Simulate DPI coordinate conversion (virtual -> physical) - // In DPI-aware Electron: GetCursorPos gives virtual, GetPixel expects physical - let physical_x = (x as f64 * self.dpi_scale) as i32; - let physical_y = (y as f64 * self.dpi_scale) as i32; - - // screen_width/height are physical dimensions - if physical_x < 0 || physical_y < 0 || physical_x >= self.screen_width || physical_y >= self.screen_height { + // For DPI-unaware process, coordinates are used directly (virtualized space) + // screen_width/height should represent virtual dimensions for accurate simulation + if x < 0 || y < 0 || x >= self.screen_width || y >= self.screen_height { return Err(format!("Failed to get pixel at ({}, {})", x, y)); } - // Simulate COLORREF BGR format conversion to RGB - // Use physical coordinates for color calculation (what GetPixel sees) - let b_component = (physical_x % 256) as u8; - let g_component = (physical_y % 256) as u8; - let r_component = ((physical_x + physical_y) % 256) as u8; + // Use coordinates directly (virtualized space) + let b_component = (x % 256) as u8; + let g_component = (y % 256) as u8; + let r_component = ((x + y) % 256) as u8;
64-101: Mocksample_gridalso diverges from real implementation.Similar to
sample_pixel, the mock applies DPI scaling to center coordinates (lines 70-71) and grid offsets (lines 74, 81-82), but the realWindowsSampler::sample_griduses coordinates directly:// Real implementation (windows.rs lines 102-103) let x_start = center_x - half_size; let y_start = center_y - half_size;This inconsistency propagates through all the DPI-related tests (lines 474-736).
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
94-142: Fix coordinate space mismatch betweenget_cursor_position()andsample_grid().The
get_cursor_position()function converts virtual coordinates to physical coordinates (line 84-85), butmain.rspasses these physical coordinates directly tosample_grid()(line 159), which expects virtual coordinates for its BitBlt calls on a DPI-unaware process. On high-DPI displays, this causes sampling from incorrect screen locations.Either convert the physical coordinates back to virtual before calling
sample_grid(), or haveget_cursor_position()return virtual coordinates to keep the internal sampler API consistent.
📜 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
⏰ 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 (3)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (3)
48-73: Coordinate handling looks correct for DPI-unaware process.The approach of using coordinates directly for
GetPixelon a DPI-unaware process is sound—bothGetCursorPosand the screen DC operate in the same virtualized coordinate space, so no conversion is needed here.
75-92: Verify Electron receives and uses physical coordinates correctly.The conversion from virtual to physical coordinates for Electron window positioning looks correct. However, this assumes Electron expects physical pixel coordinates for the magnifier window placement.
Please verify that Electron's window positioning code expects physical coordinates. If Electron's
BrowserWindow.setPosition()or similar APIs are also DPI-aware, they might already expect logical/virtual coordinates, which could cause double-scaling.
216-250: Fallback implementation is consistent with the main path.The fallback correctly mirrors the coordinate handling approach of the optimized BitBlt path.
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)
64-96: Critical: Mock behavior doesn't match production implementation.The comments state "Production sample_grid operates in virtual coordinates" (lines 69, 73), but the mock converts virtual to physical coordinates for bounds checking and color calculation (lines 77-80). This contradicts production behavior.
Production code evidence:
- windows.rs sample_grid (relevant_code_snippets): Uses
center_xdirectly with comment "For a DPI-unaware process, all GDI operations use virtualized coordinates. No conversion needed"- types.rs sample_grid (relevant_code_snippets): Uses
center_x + (col as i32 - half_size)directly without any DPI scalingThe mock's DPI conversion at lines 79-80 tests behavior that doesn't exist in production. A previous review flagged this exact issue, suggesting to "update the test to match production behavior by removing DPI scaling."
🔎 Proposed fix to match production behavior
fn sample_grid(&mut self, center_x: i32, center_y: i32, grid_size: usize, _scale_factor: f64) -> Result<Vec<Vec<Color>>, String> { let half_size = (grid_size / 2) as i32; let mut grid = Vec::with_capacity(grid_size); - // Production sample_grid operates in virtual coordinates (no DPI scaling) + // Production sample_grid uses coordinates directly without DPI conversion for row in 0..grid_size { let mut row_pixels = Vec::with_capacity(grid_size); for col in 0..grid_size { - // Calculate virtual pixel coordinates (matches production behavior) - let virtual_x = center_x + (col as i32 - half_size); - let virtual_y = center_y + (row as i32 - half_size); - - // Convert virtual to physical for bounds checking and color calculation - // (since screen_width/screen_height are physical dimensions) - let physical_x = (virtual_x as f64 * self.dpi_scale) as i32; - let physical_y = (virtual_y as f64 * self.dpi_scale) as i32; + // Use coordinates directly like production (no DPI conversion) + let x = center_x + (col as i32 - half_size); + let y = center_y + (row as i32 - half_size); - // Sample in physical space - if physical_x < 0 || physical_y < 0 || physical_x >= self.screen_width || physical_y >= self.screen_height { + // Bounds check + if x < 0 || y < 0 || x >= self.screen_width || y >= self.screen_height { row_pixels.push(Color::new(128, 128, 128)); } else { - let b_component = (physical_x % 256) as u8; - let g_component = (physical_y % 256) as u8; - let r_component = ((physical_x + physical_y) % 256) as u8; + let b_component = (x % 256) as u8; + let g_component = (y % 256) as u8; + let r_component = ((x + y) % 256) as u8; row_pixels.push(Color::new(r_component, g_component, b_component)); } } grid.push(row_pixels); } Ok(grid) }
♻️ Duplicate comments (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
691-717: Test expectations assume DPI conversion that production doesn't perform.The test expects colors based on DPI-scaled physical coordinates (lines 696-700: virtual 1000,500 → physical 2000,1000), but production
sample_griduses coordinates directly without DPI conversion. This test validates the mock's DPI-scaling behavior, not production behavior.The previous review comment flagged this same area, stating: "Since the real
sample_griduses coordinates directly without scaling, these assertions test logic that doesn't exist in production" and suggested "removing DPI scaling from the expected-coordinate calculations."This PR's changes appear to have gone in the opposite direction by maintaining (and documenting) the DPI-scaled expectations rather than aligning with production's no-conversion approach.
🔎 Proposed fix to match production expectations
If production uses coordinates directly without DPI conversion:
- // 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_g = (1000 % 256) as u8; // 1000 % 256 = 232 - let expected_r = ((2000 + 1000) % 256) as u8; // 3000 % 256 = 200 + // Center samples at position (1000, 500) - no DPI conversion + // Colors are based on input coordinates directly + let expected_b = (1000 % 256) as u8; // 1000 % 256 = 232 + let expected_g = (500 % 256) as u8; // 500 % 256 = 244 + let expected_r = ((1000 + 500) % 256) as u8; // 1500 % 256 = 220 assert_eq!(center_pixel.r, expected_r, "Center pixel R component mismatch"); assert_eq!(center_pixel.g, expected_g, "Center pixel G component mismatch"); assert_eq!(center_pixel.b, expected_b, "Center pixel B component mismatch"); - // Grid samples at virtual offsets from center (1000, 500) - // Virtual half_size = 2 for 5x5 grid - // Top-left: virtual (998, 498) -> physical (1996, 996) + // Top-left: (998, 498) - no DPI conversion let top_left = &grid[0][0]; - assert_eq!(top_left.b, (1996 % 256) as u8); // 1996 % 256 = 220 - assert_eq!(top_left.g, (996 % 256) as u8); // 996 % 256 = 228 + assert_eq!(top_left.b, (998 % 256) as u8); // 998 % 256 = 230 + assert_eq!(top_left.g, (498 % 256) as u8); // 498 % 256 = 242 - // Bottom-right: virtual (1002, 502) -> physical (2004, 1004) + // Bottom-right: (1002, 502) - no DPI conversion let bottom_right = &grid[4][4]; - assert_eq!(bottom_right.b, (2004 % 256) as u8); // 2004 % 256 = 228 - assert_eq!(bottom_right.g, (1004 % 256) as u8); // 1004 % 256 = 236 + assert_eq!(bottom_right.b, (1002 % 256) as u8); // 1002 % 256 = 234 + assert_eq!(bottom_right.g, (502 % 256) as u8); // 502 % 256 = 246
📜 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 (1)
sample_grid(96-213)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: Test Rust Sampler (windows-latest)
- GitHub Check: Tests
🔇 Additional comments (2)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
127-133: This comment is incorrect — the test is properly configured and production code does perform DPI scaling.The production
get_cursor_position()explicitly converts virtual coordinates to physical coordinates by multiplying bydpi_scale(lines 59–60 in windows.rs). The mock correctly simulates this behavior.The test at lines 127–133 uses
MockWindowsSampler::new(1920, 1080), which setsdpi_scale = 1.0(100% DPI). At this scale, virtual coordinates (100, 100) correctly map to physical coordinates (100, 100). The test is not claiming to work at other DPI scales—it specifically tests the 100% DPI scenario.Other DPI tests in the suite are properly configured:
test_windows_sampler_dpi_200_percent(line 497),test_windows_sampler_dpi_grid_sampling_200_percent(line 546), andtest_windows_sampler_dpi_5120x2880_display(line 563) all usenew_with_dpi()with appropriate scale factors (e.g., 2.0 for 200% DPI) and expect correctly scaled coordinates. There is no systematic inconsistency.Likely an incorrect or invalid review comment.
54-62: I need the review comment to rewrite. Please provide the review comment within<review_comment>tags along with any relevant context such as the file path, code snippets, or related information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
electron-app/magnifier/rust-sampler/src/main.rs (2)
70-83: The downcasting approach is fragile and may silently fail.Using
Anytrait downcasting to accessdpi_scaleis unusual and has several issues:
- Requires
WindowsSamplerto implementAny(auto-implemented for'statictypes)- The fallback to
1.0silently masks downcast failures, which could lead to incorrect coordinate handling on Windows🔎 Recommended: Add dpi_scale() to the PixelSampler trait
In
types.rs:pub trait PixelSampler { fn sample_pixel(&mut self, x: i32, y: i32) -> Result<Color, String>; fn get_cursor_position(&self) -> Result<Point, String>; fn sample_grid(&mut self, center_x: i32, center_y: i32, grid_size: usize, scale_factor: f64) -> Result<Vec<Vec<Color>>, String>; + fn dpi_scale(&self) -> f64 { 1.0 } // Default implementation for non-Windows platforms }Then in
main.rs:- #[cfg(target_os = "windows")] - let dpi_scale = { - use std::any::Any; - use sampler::WindowsSampler; - if let Some(windows_sampler) = (&mut *sampler as &mut dyn Any).downcast_mut::<WindowsSampler>() { - windows_sampler.dpi_scale - } else { - 1.0 - } - }; - #[cfg(not(target_os = "windows"))] - let dpi_scale = 1.0; + let dpi_scale = sampler.dpi_scale();
153-182: The coordinate flow is correct but adds roundtrip complexity.The physical → virtual conversion correctly reverses the virtual → physical conversion done in
get_cursor_position. The roundtrip:
GetCursorPosreturns virtual coords (e.g., 1280)get_cursor_positionmultiplies bydpi_scale→ physical (2560)main.rsdivides bydpi_scale→ virtual (1280)- Sampling methods use virtual coordinates
This works but adds unnecessary arithmetic. Consider having
get_cursor_positionreturn both physical and virtual coordinates, or returning virtual and converting to physical only where needed (Line 195).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
electron-app/magnifier/rust-sampler/src/main.rselectron-app/magnifier/rust-sampler/src/sampler/linux.rselectron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rselectron-app/magnifier/rust-sampler/src/sampler/windows.rselectron-app/magnifier/rust-sampler/src/types.rs
✅ Files skipped from review due to trivial changes (2)
- electron-app/magnifier/rust-sampler/src/sampler/wayland_portal.rs
- electron-app/magnifier/rust-sampler/src/types.rs
🧰 Additional context used
🧬 Code graph analysis (2)
electron-app/magnifier/rust-sampler/src/main.rs (2)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
new(17-36)electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
new(16-22)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
new(16-22)
⏰ 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 (8)
electron-app/magnifier/rust-sampler/src/sampler/linux.rs (1)
255-256: LGTM!Whitespace-only change with no functional impact.
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (5)
11-14: LGTM!Making
dpi_scalepublic enables the main loop to access it for coordinate conversion. This is acceptable, though a getter method would be more encapsulated if the API surface grows.
48-73: LGTM!The removal of DPI conversion from
sample_pixelis correct. For a DPI-unaware process, bothGetCursorPosand the screen DC operate in the same virtualized coordinate space, so no conversion is needed here. The comments clearly document this behavior.
75-92: Verify the coordinate conversion direction.The multiplication by
dpi_scaleconverts virtual → physical coordinates. This is correct for Electron's per-monitor DPI-aware window positioning. However, this creates a dependency:main.rsmust convert back to virtual coordinates before calling sampling methods.Consider documenting this contract in the
PixelSamplertrait or method documentation to prevent future regressions.
96-106: LGTM!The
sample_gridmethod correctly uses virtualized coordinates directly, consistent with the updatedsample_pixelbehavior. The comments clearly document the coordinate space expectations.
216-250: LGTM!The fallback method is consistent with the main
sample_gridimplementation, using coordinates directly without DPI conversion.electron-app/magnifier/rust-sampler/src/main.rs (2)
113-119: LGTM!The function signature update to accept
dpi_scaleis straightforward.
194-202: LGTM!The
PixelDatacorrectly usesphysical_cursorfor the cursor field, which Electron (per-monitor DPI aware) requires for accurate window positioning.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.