Repository navigation
fix(scap-targets): keep partially visible X11 windows selectable - #2906
yashranaway wants to merge 2 commits into
Conversation
|
@yashranaway is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
@greptileai review |
| }; | ||
|
|
||
| #[test] | ||
| #[ignore = "requires an isolated X11 server, run with xvfb-run"] |
There was a problem hiding this comment.
The new X11 test is marked #[ignore], but no CI job runs it with --ignored. The existing Xvfb job runs only cap-gpui tests, so a change that breaks window selection can pass CI without exercising this test.
Add a Linux CI step that runs xvfb-run -a cargo test --locked -p scap-targets --test linux_targets -- --ignored.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/scap-targets/tests/linux_targets.rs
Line: 14
Comment:
**CI skips the regression test**
The new X11 test is marked `#[ignore]`, but no CI job runs it with `--ignored`. The existing Xvfb job runs only `cap-gpui` tests, so a change that breaks window selection can pass CI without exercising this test.
Add a Linux CI step that runs `xvfb-run -a cargo test --locked -p scap-targets --test linux_targets -- --ignored`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
fair, fixed in 09a22a2. the test now starts its own Xvfb, isn't ignored anymore, and runs in the existing linux desktop regression step. checked it fails with the old window selection and passes with the fix.
|
Thanks @yashranaway, the fix looks right to us. Greptile's 4/5 is about coverage: the new X11 test in |
on linux, a window was matched to whichever display contained its centre point. move a window so its centre is off screen, or sitting in the gap between two monitors, and it dropped out of the window picker even though most of it was still visible.
now a window goes to the display it overlaps the most, and only disappears once none of it is on any display. the xvfb test moves a real window half off screen and checks it's still listed, plus unit tests for monitor gaps and two displays.
note: #2816 adds a second copy of the centre point check in its x11 snapshot parser, so whichever of the two lands second should switch that path to this overlap helper too.
The PR appears safe to merge, with a non-blocking gap in CI coverage.
Findings
Fix with agent prompt
Summary
Changes Linux window selection to use the display with the largest visible overlap instead of the display containing the window center.
Reviews (1) · Last reviewed commit: "fix(scap-targets): keep partially visibl..." · Reviewed by Greptile