feat: allow WGC capture path to attempt minimized UWP windows (remove IsIconic early-return)#1736
Conversation
…ows in cua-driver-rs
|
@mvanhorn is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWGC module removes the early check that rejected minimized windows, allowing them to proceed through standard capture and frame polling with timeout handling. Documentation updated accordingly. New Windows-only integration tests validate UWP app screenshot capture for both normal and minimized window states. ChangesWGC Minimized Window Capture Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (1)
libs/cua-driver/rust/crates/platform-windows/tests/capture_uwp_test.rs (1)
20-27: ⚡ Quick winUse launched PID for window matching instead of title text.
At Line 20 and Line 26, the test ignores the PID returned from
launch_uwpand matches by"calculator"title. That is locale-dependent and can accidentally pick a pre-existing Calculator window. Filtering by the launched PID will make this test much more deterministic.🤖 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 `@libs/cua-driver/rust/crates/platform-windows/tests/capture_uwp_test.rs` around lines 20 - 27, The test currently ignores the PID returned from launch_uwp::launch_uwp(CALCULATOR_AUMID, "") and searches windows by title; change it to capture the returned _pid (e.g., let pid = launch_uwp::launch_uwp(...)?;) and use that PID to find the window from platform_windows::win32::windows::list_windows(None) by comparing the window's process id field (e.g., window.process_id or window.pid) to the launched pid instead of matching title text so the loop deterministically selects the launched UWP process.
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/tests/capture_uwp_test.rs`:
- Around line 59-79: In minimized_calculator_wgc_attempts_composited_capture,
don't unwrap the wgc::screenshot_window_via_wgc result with result?; instead
match on result from wgc::screenshot_window_via_wgc(window.hwnd) and accept
either the successful tuple (pixels, w, h) which you then validate as before, or
the expected structured timeout error returned by the wgc module; implement the
match using either matches!(err, wgc::...::Timeout) or an
is_timeout()/is_timeout_variant() helper on the error type so the test passes
when WGC legitimately times out while still failing for other unexpected errors.
---
Nitpick comments:
In `@libs/cua-driver/rust/crates/platform-windows/tests/capture_uwp_test.rs`:
- Around line 20-27: The test currently ignores the PID returned from
launch_uwp::launch_uwp(CALCULATOR_AUMID, "") and searches windows by title;
change it to capture the returned _pid (e.g., let pid =
launch_uwp::launch_uwp(...)?;) and use that PID to find the window from
platform_windows::win32::windows::list_windows(None) by comparing the window's
process id field (e.g., window.process_id or window.pid) to the launched pid
instead of matching title text so the loop deterministically selects the
launched UWP process.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e914f668-123a-46e4-a583-86a7a4ce5c70
📒 Files selected for processing (2)
libs/cua-driver/rust/crates/platform-windows/src/wgc.rslibs/cua-driver/rust/crates/platform-windows/tests/capture_uwp_test.rs
| fn minimized_calculator_wgc_attempts_composited_capture() -> Result<()> { | ||
| let window = launch_calculator_and_wait_for_window()?; | ||
| let hwnd = HWND(window.hwnd as *mut _); | ||
|
|
||
| unsafe { | ||
| let _ = ShowWindow(hwnd, SW_MINIMIZE); | ||
| } | ||
| sleep(Duration::from_millis(500)); | ||
|
|
||
| let result = wgc::screenshot_window_via_wgc(window.hwnd); | ||
|
|
||
| unsafe { | ||
| let _ = ShowWindow(hwnd, SW_RESTORE); | ||
| } | ||
|
|
||
| let (pixels, w, h) = result?; | ||
| assert!( | ||
| w > 120 && h > 30, | ||
| "expected real Calculator UI dimensions from minimized WGC capture, got {w}x{h}" | ||
| ); | ||
| assert_eq!(pixels.len(), w as usize * h as usize * 4); |
There was a problem hiding this comment.
Minimized WGC test should accept structured timeout as a valid outcome.
At Line 74, result? makes this test fail on systems where minimized/cloaked windows legitimately produce no frame within timeout (which this PR explicitly documents as possible). The test should assert either: (a) successful composited capture with valid dimensions, or (b) the expected structured timeout error.
Proposed adjustment
- let (pixels, w, h) = result?;
- assert!(
- w > 120 && h > 30,
- "expected real Calculator UI dimensions from minimized WGC capture, got {w}x{h}"
- );
- assert_eq!(pixels.len(), w as usize * h as usize * 4);
+ match result {
+ Ok((pixels, w, h)) => {
+ assert!(
+ w > 120 && h > 30,
+ "expected real Calculator UI dimensions from minimized WGC capture, got {w}x{h}"
+ );
+ assert_eq!(pixels.len(), w as usize * h as usize * 4);
+ }
+ Err(e) => {
+ let msg = e.to_string();
+ assert!(
+ msg.contains("no frame within 1500 ms"),
+ "unexpected minimized WGC failure: {e:#}"
+ );
+ }
+ }📝 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.
| fn minimized_calculator_wgc_attempts_composited_capture() -> Result<()> { | |
| let window = launch_calculator_and_wait_for_window()?; | |
| let hwnd = HWND(window.hwnd as *mut _); | |
| unsafe { | |
| let _ = ShowWindow(hwnd, SW_MINIMIZE); | |
| } | |
| sleep(Duration::from_millis(500)); | |
| let result = wgc::screenshot_window_via_wgc(window.hwnd); | |
| unsafe { | |
| let _ = ShowWindow(hwnd, SW_RESTORE); | |
| } | |
| let (pixels, w, h) = result?; | |
| assert!( | |
| w > 120 && h > 30, | |
| "expected real Calculator UI dimensions from minimized WGC capture, got {w}x{h}" | |
| ); | |
| assert_eq!(pixels.len(), w as usize * h as usize * 4); | |
| fn minimized_calculator_wgc_attempts_composited_capture() -> Result<()> { | |
| let window = launch_calculator_and_wait_for_window()?; | |
| let hwnd = HWND(window.hwnd as *mut _); | |
| unsafe { | |
| let _ = ShowWindow(hwnd, SW_MINIMIZE); | |
| } | |
| sleep(Duration::from_millis(500)); | |
| let result = wgc::screenshot_window_via_wgc(window.hwnd); | |
| unsafe { | |
| let _ = ShowWindow(hwnd, SW_RESTORE); | |
| } | |
| match result { | |
| Ok((pixels, w, h)) => { | |
| assert!( | |
| w > 120 && h > 30, | |
| "expected real Calculator UI dimensions from minimized WGC capture, got {w}x{h}" | |
| ); | |
| assert_eq!(pixels.len(), w as usize * h as usize * 4); | |
| } | |
| Err(e) => { | |
| let msg = e.to_string(); | |
| assert!( | |
| msg.contains("no frame within 1500 ms"), | |
| "unexpected minimized WGC failure: {e:#}" | |
| ); | |
| } | |
| } | |
| } |
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/tests/capture_uwp_test.rs`
around lines 59 - 79, In minimized_calculator_wgc_attempts_composited_capture,
don't unwrap the wgc::screenshot_window_via_wgc result with result?; instead
match on result from wgc::screenshot_window_via_wgc(window.hwnd) and accept
either the successful tuple (pixels, w, h) which you then validate as before, or
the expected structured timeout error returned by the wgc module; implement the
match using either matches!(err, wgc::...::Timeout) or an
is_timeout()/is_timeout_variant() helper on the error type so the test passes
when WGC legitimately times out while still failing for other unexpected errors.
mvanhorn
changed the title
Removing the ignored integration test that referenced symbols (win32::windows::WindowInfo, capture::png_dimensions_pub) which don't exist on this branch. It would not have compiled if not for the #[ignore]. The IsIconic guard removal can stand on its own; once the actual WGC path lands we can add a test that compiles.
1 participant
Summary
feat: Windows.Graphics.Capture path for background-collapsed UWP windows in cua-driver-rs
Closes #1600
AI was used for assistance.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests