Windows magnifier improvements #2462
Conversation
WalkthroughAdded optimized Windows grid sampling using GDI BitBlt + GetDIBits with a pixel-by-pixel GetPixel fallback; stored virtual screen dimensions on the sampler; documentation and tests updated to reflect performance and behavior changes. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Magnifier App
participant WS as WindowsSampler
participant GDI as GDI Subsystem
App->>WS: sample_grid(center_x, center_y, grid_size)
activate WS
Note over WS: Optimized path — BitBlt + GetDIBits
WS->>GDI: CreateCompatibleDC / CreateCompatibleBitmap / SelectObject
WS->>GDI: BitBlt(memory_dc, grid_region)
alt BitBlt Success
WS->>GDI: GetDIBits(bitmap_handle) → BGRA buffer
GDI-->>WS: pixel_data
WS->>WS: Convert BGRA → Color grid
WS-->>App: Result::Ok(Vec<Vec<Color>>)
else BitBlt Failure
Note over WS: Fallback — per-pixel GetPixel
loop each pixel
WS->>GDI: GetPixel(hdc, x, y)
GDI-->>WS: pixel_value
end
WS->>WS: Build Color grid from pixel values
WS-->>App: Result::Ok(Vec<Vec<Color>>)
end
WS->>GDI: Cleanup (SelectObject restore, DeleteObject, DeleteDC)
deactivate WS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (2)
9-9: Remove unused import.
GetSystemMetricsForDpiis imported but never used in this file. Either remove it or add a TODO comment if it's intended for future HiDPI-aware sampling.-use windows::Win32::UI::HiDpi::GetSystemMetricsForDpi;
212-232: Consider reusing the default trait implementation.This fallback duplicates the logic in
PixelSampler::sample_gridfromtypes.rs. You could potentially call the default implementation via a helper or extract shared logic to reduce duplication. However, the explicit fallback is clear and maintainable as-is.electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
231-245: Clarify test scope: mock uses default implementation, not BitBlt.The comment states this verifies "the optimized implementation is being used," but
MockWindowsSampleruses the defaultPixelSampler::sample_gridtrait implementation, not the BitBlt-basedWindowsSampler::sample_grid. Consider updating the comment to reflect that this tests the grid sampling contract/interface rather than the optimization path.#[test] fn test_windows_sampler_optimized_grid_sampling() { - // Verify that the optimized implementation is being used - // This test is mostly for documentation purposes - the real test - // happens on actual Windows hardware + // Tests grid sampling interface contract using mock sampler. + // The actual BitBlt optimization is tested on real Windows hardware. let mut sampler = MockWindowsSampler::new(1920, 1080);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/PERFORMANCE.md(1 hunks)docs/PIXEL_SAMPLER.md(1 hunks)electron-app/magnifier/rust-sampler/Cargo.toml(1 hunks)electron-app/magnifier/rust-sampler/src/sampler/windows.rs(3 hunks)electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (2)
electron-app/magnifier/rust-sampler/src/types.rs (2)
sample_grid(87-105)new(12-18)electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
new(15-20)
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
new(19-39)electron-app/magnifier/rust-sampler/src/types.rs (1)
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: Test Rust Sampler (windows-latest)
- GitHub Check: Tests
🔇 Additional comments (5)
docs/PIXEL_SAMPLER.md (1)
49-50: LGTM!The documentation accurately reflects the implementation changes. The distinction between BitBlt for grid capture and GetPixel for single pixels correctly describes the new Windows sampling strategy.
docs/PERFORMANCE.md (1)
94-98: LGTM!The performance documentation clearly articulates the optimization gains from switching to BitBlt-based grid capture. The ~100x improvement claim is reasonable given the reduction from 81 GetPixel calls to a single BitBlt operation.
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
86-209: Well-structured BitBlt optimization with proper fallback.The implementation correctly:
- Creates compatible DC and bitmap for off-screen rendering
- Uses negative
biHeightfor top-down DIB orientation- Cleans up GDI resources in all code paths
- Falls back to pixel-by-pixel sampling on failure
The buffer stride calculation is correct for 32-bit bitmaps (no row padding needed).
electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (2)
269-295: Excellent pixel alignment verification test.This test properly validates that grid sampling produces consistent results with individual pixel samples, which is crucial for ensuring the optimized path (when used on real Windows) maintains pixel-perfect accuracy.
353-369: Good edge case coverage for fully out-of-bounds grids.Testing the gray fallback behavior when the entire grid is outside screen bounds ensures graceful degradation in edge cases.
| "Win32_Foundation", | ||
| "Win32_Graphics_Gdi", | ||
| "Win32_UI_WindowsAndMessaging", | ||
| "Win32_UI_HiDpi", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "GetSystemMetricsForDpi" --type rustRepository: shipshapecode/swach
Length of output: 178
🏁 Script executed:
# Also check what's imported in windows.rs
fd "windows.rs" --type f -x cat -n {}Repository: shipshapecode/swach
Length of output: 10260
🏁 Script executed:
# Check the Cargo.toml context to understand the feature
cat -n electron-app/magnifier/rust-sampler/Cargo.toml | head -50Repository: shipshapecode/swach
Length of output: 1822
🏁 Script executed:
# Search entire codebase for any usage of GetSystemMetricsForDpi
rg "GetSystemMetricsForDpi" --type rust -B 2 -A 2Repository: shipshapecode/swach
Length of output: 591
Remove unused GetSystemMetricsForDpi import and Win32_UI_HiDpi feature.
GetSystemMetricsForDpi is imported in windows.rs (line 9) but never called. The code uses GetSystemMetrics instead. Remove the unused import and the corresponding Win32_UI_HiDpi feature from Cargo.toml (line 36) to reduce compile-time dependencies, or add a TODO comment if this API is intended for future HiDPI-aware metrics support.
🤖 Prompt for AI Agents
In electron-app/magnifier/rust-sampler/Cargo.toml around line 36 and in
electron-app/magnifier/rust-sampler/src/windows.rs around line 9, the
Win32_UI_HiDpi feature is enabled in Cargo.toml but GetSystemMetricsForDpi is
imported in windows.rs and never used; remove the unused import from windows.rs
(line 9) and delete the "Win32_UI_HiDpi" feature entry from Cargo.toml (line 36)
to eliminate the unnecessary dependency, or if you plan to use HiDPI APIs later,
replace the removal with a single-line TODO comment in both files indicating
future HiDPI support and keep the feature intentionally.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (3)
13-14: Consider using screen dimensions for bounds validation.The
screen_widthandscreen_heightfields are stored but not used internally for any validation or bounds checking. While exposing them as part of the public API is reasonable, you could defensively validate that capture regions fall within screen bounds before callingBitBlt, which would provide clearer error messages than relying on GDI failures.
30-30: Consider using a logging framework instead of eprintln!.Using
eprintln!in library code writes directly to stderr without giving consumers control over logging behavior or levels. Consider using a logging facade like thelogcrate, which allows consumers to configure logging as needed.
211-229: Minor code duplication with default trait implementation.The fallback logic here is nearly identical to the default
sample_gridimplementation intypes.rs(lines 86-104). While functionally correct, consider whether the fallback could simply delegate to the default trait implementation to reduce duplication, or if there are subtle differences that justify the separate implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (2)
electron-app/magnifier/rust-sampler/src/types.rs (2)
sample_grid(87-105)new(12-18)electron-app/magnifier/rust-sampler/tests/windows_sampler_tests.rs (1)
new(15-20)
⏰ 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 (1)
electron-app/magnifier/rust-sampler/src/sampler/windows.rs (1)
86-134: Excellent optimization with proper fallback handling.The BitBlt approach is a solid optimization for batch pixel capture. The resource cleanup paths correctly handle all error cases, and the fallback to pixel-by-pixel sampling ensures robustness when BitBlt fails.
| let buffer_size = (width * height * 4) as usize; | ||
| let mut buffer: Vec<u8> = vec![0; buffer_size]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to sample_grid to verify grid_size bounds checking
rg -nP --type=rust -C3 'sample_grid\s*\(' -g '!**/target/**'Repository: shipshapecode/swach
Length of output: 32979
🏁 Script executed:
# Get context around the sample_grid call in main.rs
sed -n '140,180p' electron-app/magnifier/rust-sampler/src/main.rsRepository: shipshapecode/swach
Length of output: 1566
🏁 Script executed:
# Search for current_grid_size initialization in main.rs
rg -n 'current_grid_size' electron-app/magnifier/rust-sampler/src/main.rs -B3 -A3Repository: shipshapecode/swach
Length of output: 989
🏁 Script executed:
# Find initial_grid_size and Command definition
rg -n 'initial_grid_size\|enum Command' electron-app/magnifier/rust-sampler/src/main.rs -B2 -A2Repository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
# Search for initial_grid_size and Command in all Rust files
rg -n 'initial_grid_size' electron-app/magnifier/rust-sampler/src/ --type=rustRepository: shipshapecode/swach
Length of output: 247
🏁 Script executed:
# Find the function/struct that contains initial_grid_size
sed -n '95,105p' electron-app/magnifier/rust-sampler/src/main.rsRepository: shipshapecode/swach
Length of output: 305
🏁 Script executed:
# Find callers of run_sampling_loop
rg -n 'run_sampling_loop' electron-app/magnifier/rust-sampler/src/main.rs -B2 -A2Repository: shipshapecode/swach
Length of output: 549
🏁 Script executed:
# Find Command enum definition and validation
rg -n 'enum Command|Command::' electron-app/magnifier/rust-sampler/src/main.rs -B1 -A1Repository: shipshapecode/swach
Length of output: 1274
🏁 Script executed:
# Find Command enum definition
rg -n 'enum Command\s*{' electron-app/magnifier/rust-sampler/src/ -A5Repository: shipshapecode/swach
Length of output: 158
🏁 Script executed:
# Find Command enum definition
rg -n 'enum Command' electron-app/magnifier/rust-sampler/src/ -A8Repository: shipshapecode/swach
Length of output: 731
🏁 Script executed:
# Find where Command is parsed/deserialized from input
rg -n 'serde_json::from_str\|from_reader' electron-app/magnifier/rust-sampler/src/main.rs -B3 -A3Repository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
# Look at the stdin thread that reads commands
sed -n '35,65p' electron-app/magnifier/rust-sampler/src/main.rsRepository: shipshapecode/swach
Length of output: 1243
Add bounds validation for grid_size parameter.
The buffer allocation (width * height * 4) bytes receives grid_size directly from user input (via JSON commands at src/main.rs:51) with no validation. A caller could send an arbitrarily large grid_size (e.g., 10000+) causing hundreds of megabytes of allocation per frame. Validate that callers enforce reasonable bounds (tests use up to 21×21; suggest enforcing a maximum like 25×25 or similar).
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.