Repository navigation
feat(desktop-gpui): resize a picked window to a ratio or size, and focus it to record it alone - #3454
richiemcilroy wants to merge 14 commits into
Conversation
…cus it to record it alone
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
hey @greptileai, please re-review the PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
# Conflicts: # apps/desktop-gpui/src/picker_probe.rs
… pin the size menu's window, ignore stale moves, restore maximized windows
# Conflicts: # apps/desktop-gpui/src/picker_probe.rs
|
hey @greptileai, please re-review the PR |
…he smoke's test window outright
|
hey @greptileai, please re-review the PR |
…gion, and spawn the restore
|
hey @greptileai, please re-review the PR |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| } | ||
|
|
||
| pub fn dismiss_target_overlays(cx: &mut App) { | ||
| restore_arranged_windows(cx); |
There was a problem hiding this comment.
Reopening Cap bypasses restoration of arranged windows
Medium Severity
The unchanged show_main_window_after_capture_pause path calls close_target_overlays directly. After resizing or focusing a window, choosing Open Cap from the tray therefore closes the picker without calling this restore helper. The next TargetSelect::arm clears the saved originals, permanently leaving the other application's window rearranged despite no recording starting. Route non-recording picker exits through restoration, including the main-window reveal path.
| this.select.update(cx, |select, cx| select.pin_hovered(cx)); | ||
| this.window_menu = None; | ||
| let picked = this.select.update(cx, |select, cx| select.pin_hovered(cx)); | ||
| if picked && this.select.read(cx).focus_window { |
There was a problem hiding this comment.
Saved Focus does not centre preselected windows
Medium Severity
With Focused window already saved, selecting an off-centre window from the main dropdown seeds pinned_window and displays Focus as enabled, but never centres the window. The existing focus_selected_window helper only raises it, and clicking that same window in the overlay makes pin_hovered return false, bypassing this arrangement call again. Opening the size menu also pins a window without applying saved Focus. Apply the Focus arrangement when these selection paths initially pin a window, rather than only when an overlay click changes its ID.
|
hey @greptileai, please re-review the PR |
| // The user shaped the window to record it: it stays where it is. | ||
| TargetSelect::global(cx).update(cx, |select, _| { | ||
| select.take_arranged(); | ||
| }); |
There was a problem hiding this comment.
Starting a recording does not wait for pending window moves. If the user chooses a size and starts while the target app is busy, the default ScreenRegion capture can read the old bounds before SetWindowPos finishes. The window then moves outside the fixed recording crop.
Wait for queued moves to finish before starting capture.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/app_windows.rs
Line: 2449-2452
Comment:
**Recording can use old bounds**
Starting a recording does not wait for pending window moves. If the user chooses a size and starts while the target app is busy, the default `ScreenRegion` capture can read the old bounds before `SetWindowPos` finishes. The window then moves outside the fixed recording crop.
Wait for queued moves to finish before starting capture.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| pub fn dismiss_target_overlays(cx: &mut App) { | ||
| restore_arranged_windows(cx); |
There was a problem hiding this comment.
Window restoration only runs through dismiss_target_overlays, but other user actions close the picker directly. Choosing Open Cap from the tray calls show_main_window, which closes the overlays without restoring moved windows. The next picker opening clears arranged, losing the original positions.
Restore moved windows on non-recording close paths too.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/app_windows.rs
Line: 2173-2174
Comment:
**Some exits leave windows changed**
Window restoration only runs through `dismiss_target_overlays`, but other user actions close the picker directly. Choosing Open Cap from the tray calls `show_main_window`, which closes the overlays without restoring moved windows. The next picker opening clears `arranged`, losing the original positions.
Restore moved windows on non-recording close paths too.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| overlay | ||
| .update(cx, |view, _, cx| view.set_focus(false, cx)) | ||
| .ok(); | ||
| sleep(cx, 600).await; | ||
|
|
||
| cx.update(app_windows::dismiss_target_overlays); |
There was a problem hiding this comment.
The smoke check turns Focus off before dismissing the picker. set_focus(false) already restores the original window, so restored_ok can pass even if dismissal restoration is completely broken.
Dismiss while the window is still moved, then check its original bounds. Test turning Focus off separately.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/picker_probe.rs
Line: 199-204
Comment:
**Smoke check misses broken cancellation**
The smoke check turns Focus off before dismissing the picker. `set_focus(false)` already restores the original window, so `restored_ok` can pass even if dismissal restoration is completely broken.
Dismiss while the window is still moved, then check its original bounds. Test turning Focus off separately.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Stacked on #3452.
Problem
In the window picker, the picked window's size was plain text, and the only way to change the shape of what gets recorded was to resize the window by hand. A window recording also captured a region of the screen. #3452 adds the option to record the window by itself, but nothing in the app could turn it on.
Change
foreign_windowdoes the platform work:AXPosition/AXSize, matched to the window through_AXUIElementGetWindow), which Cap already holds the permission for.SetWindowPos, adding back DWM's invisible resize borders and restoring a maximized window first. The maximized window'sWINDOWPLACEMENTis saved, so putting it back maximizes it again with its old restore size._NET_MOVERESIZE_WINDOWand_NET_ACTIVE_WINDOW.SetWindowPoswaits on the target app. So a cancel's restore always lands after a move the user started first. The highlight then animates to wherever the app actually put the window, since apps clamp to their own minimum sizes or cell grids. A move that finishes after the user has picked another window, or closed the picker, leaves the new choice alone.The new overlay strings are literals, like the rest of the overlay on this base. #3423 localizes the overlay, and they move into its catalogue when this rebases onto it. The settings row and options are localized in all seven catalogues.
Test
Unit tests (
window_arrange, andforeign_window's ordering of queued jobs):i18n: the i18n surface tests pin the new keys, and
cap-i18n-buildpasses.Real app:
Picker smoke (real app)now drives a real window on macOS, Windows and Linux X11 through the overlay's own menu and toggle. It picks the window it opened (TextEdit, Notepad or xterm) outright, so the runner's cursor position doesn't matter:Screenshots of each step are uploaded.
Wait to merge until recording waits for pending window moves.
Findings
Fix with agent prompt
Summary
Adds window size and ratio choices, a shared Focus toggle, platform window movement, and a saved capture-style setting.
These issues remain in the inspected higher stack revisions, including #3456.
Reviews (6) · Last reviewed commit: "fix(desktop-gpui): colour the window siz..." · Reviewed by Greptile