Repository navigation
feat(desktop-gpui): area mode lock, window snapping, click to fit and clearer handles - #3515
Conversation
…nto feat/gpui-area-mode-ux
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptileai review |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| if crop.width < 2. || crop.height < 2. { | ||
| self.crop = None; | ||
| if matches!(drag, AreaDrag::Draw { .. }) { | ||
| self.fit_area_to_window_at((f32::from(position.x), f32::from(position.y)), cx); |
There was a problem hiding this comment.
Snapping turns no-drag clicks into tiny selections
Medium Severity
The unchanged click/drag test in area_mouse_up runs after area_mouse_move snaps the release point. With Free ratio and a window at (200,150) sized 400×300, pressing and releasing at (404,304) snaps to its center (400,300), producing a 4×4 crop despite no pointer movement. This bypasses click-to-fit, leaves an invalid recording selection, and immediately captures a 4×4 screenshot in screenshot mode. Classify clicks from unsnapped pointer movement before applying drag snapping.
| self.area_locked = true; | ||
| self.drag = None; | ||
| self.snap_guides.clear(); | ||
| store_locked_area(Some((&self.display_id, crop))); |
There was a problem hiding this comment.
Display-local lock flags can erase another display's saved lock
Medium Severity
On two displays, lock an area on A, then draw and lock an area on B. The stored screenId changes to B, but A's overlay remains locally locked because no shared-state synchronization updates area_locked. Unlocking or resetting A then unconditionally writes locked=false, silently deleting B's remembered lock while B still displays its Locked badge. Synchronize lock ownership across overlays and allow an unlock/reset to clear storage only when that display owns the stored lock.
| .await; | ||
| let fitted = this | ||
| .update(cx, |this, cx| { | ||
| let window = window.filter(|window| window.display_id == display)?; |
There was a problem hiding this comment.
Spanning windows are rejected on their other display
Medium Severity
When a window straddles two monitors, HoveredWindow::from_window assigns one display through Window::display()—the largest intersection on X11, or the first intersecting display on macOS. Clicking its visible portion on the other monitor therefore fails this filter and does nothing. load_area_windows applies the same filter, so that portion also provides no snap targets. Convert window bounds into the current overlay's coordinate system and intersect them with its display instead of rejecting windows by their single assigned display ID.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if crop.width < 2. || crop.height < 2. { | ||
| self.crop = None; | ||
| if matches!(drag, AreaDrag::Draw { .. }) { | ||
| self.fit_area_to_window_at((f32::from(position.x), f32::from(position.y)), cx); |
There was a problem hiding this comment.
A stationary click near snap lines on both axes can skip click-to-fit. On a 1920 × 1080 display, clicking at (957, 537) snaps the release to (960, 540), leaving a 3 × 3 crop. Neither dimension passes the < 2 check, so the picker shows an invalid selection instead of fitting the window. Detect clicks from the pointer's movement before snapping.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 4633-4636
Comment:
**Clicks become tiny selections**
A stationary click near snap lines on both axes can skip click-to-fit. On a 1920 × 1080 display, clicking at (957, 537) snaps the release to (960, 540), leaving a 3 × 3 crop. Neither dimension passes the `< 2` check, so the picker shows an invalid selection instead of fitting the window. Detect clicks from the pointer's movement before snapping.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let (locked_display, bounds) = stored_locked_area()?; | ||
| (locked_display == display.to_string()) | ||
| .then(|| bounds.clamped(display_size)) | ||
| .filter(|bounds| bounds.is_valid_for(AREA_MIN_SIZE)) |
There was a problem hiding this comment.
locked_area_for always requires 150 × 150, but toggle_area_lock accepts screenshot areas as small as one pixel. A user can resize a screenshot selection below 150 points and lock it, yet the next screenshot picker silently drops that saved area. Use the current mode's minimum when restoring, just as the lock button does.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 813
Comment:
**Small screenshot locks disappear**
`locked_area_for` always requires 150 × 150, but `toggle_area_lock` accepts screenshot areas as small as one pixel. A user can resize a screenshot selection below 150 points and lock it, yet the next screenshot picker silently drops that saved area. Use the current mode's minimum when restoring, just as the lock button does.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let window = cx | ||
| .background_executor() | ||
| .spawn(async move { | ||
| crate::devices::topmost_foreign_window_at_cursor() | ||
| .as_ref() | ||
| .and_then(HoveredWindow::from_window) |
There was a problem hiding this comment.
Click-to-fit reads the cursor position later on a background task instead of finding the window at the supplied click point. If the user moves onto another window before that task runs, the picker fits the other window. Pass the captured click position to the window lookup instead of sampling the cursor again.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 3773-3778
Comment:
**The wrong window gets selected**
Click-to-fit reads the cursor position later on a background task instead of finding the window at the supplied click point. If the user moves onto another window before that task runs, the picker fits the other window. Pass the captured click position to the window lookup instead of sampling the cursor again.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| cx.update(|cx| { | ||
| overlay | ||
| .update(cx, |view, _, cx| view.probe_offset_area(base, 0., cx)) | ||
| .ok(); | ||
| }); | ||
| sleep(cx, 300).await; | ||
| drag(ProbeDrag::Press, grab, cx); | ||
| probe_frames::begin(); |
There was a problem hiding this comment.
drag_frames resets the crop but leaves area_locked unchanged. If the probe opens on a display with a saved lock, area_mouse_down returns without starting a drag, so every measured move is a no-op. Unlock the probe's temporary selection and check that the press started the expected drag before reporting performance.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/picker_probe.rs
Line: 716-723
Comment:
**Saved locks stop probe drags**
`drag_frames` resets the crop but leaves `area_locked` unchanged. If the probe opens on a display with a saved lock, `area_mouse_down` returns without starting a drag, so every measured move is a no-op. Unlock the probe's temporary selection and check that the press started the expected drag before reporting performance.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| .detach(); | ||
| } | ||
|
|
||
| /// A click with no drag fits the area to the window under it. |
There was a problem hiding this comment.
The new comments repeatedly describe what the adjacent code already says. AGENTS.md requires comments to capture non-obvious context and explicitly bans comments that merely narrate code. Remove these descriptions or keep only reasoning that is not clear from the code. The comments above toggle_area_lock and area_hover_at have the same problem. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 3768
Comment:
**Comments repeat the code**
The new comments repeatedly describe what the adjacent code already says. `AGENTS.md` requires comments to capture non-obvious context and explicitly bans comments that merely narrate code. Remove these descriptions or keep only reasoning that is not clear from the code. The comments above `toggle_area_lock` and `area_hover_at` have the same problem. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| drag(ProbeDrag::Release, grab, cx); | ||
| let mut gaps: Vec<f64> = frames | ||
| .windows(2) | ||
| .map(|pair| (pair[1] - pair[0]).as_secs_f64() * 1000.) |
There was a problem hiding this comment.
The new frame-gap calculation subtracts Instant values directly. AGENTS.md explicitly requires saturating time subtraction. Use pair[1].saturating_duration_since(pair[0]) here. This repository requirement must be satisfied before merging.
| .map(|pair| (pair[1] - pair[0]).as_secs_f64() * 1000.) | |
| .map(|pair| pair[1].saturating_duration_since(pair[0]).as_secs_f64() * 1000.) |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/picker_probe.rs
Line: 742
Comment:
**Time subtraction breaks repository rules**
The new frame-gap calculation subtracts `Instant` values directly. `AGENTS.md` explicitly requires saturating time subtraction. Use `pair[1].saturating_duration_since(pair[0])` here. This repository requirement must be satisfied before merging.
```suggestion
.map(|pair| pair[1].saturating_duration_since(pair[0]).as_secs_f64() * 1000.)
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…nto feat/gpui-area-mode-ux
…clicked, keep small screenshot locks
|
@greptileai addressed in cc1dd52:
A click-fit in screenshot mode now waits for the button instead of capturing straight away. Please re-review. |
| if this.area_key(&event.keystroke, window, cx) { | ||
| cx.stop_propagation(); | ||
| return; |
There was a problem hiding this comment.
With a valid area selected, pressing Return while the recording-mode menu is open calls area_key and starts recording in the current mode. The key handler checks the other open menus but skips mode_menu, so the user can start recording instead of finishing their mode choice. Handle mode_menu before calling area_key.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 1854-1856
Comment:
**Return starts recording too soon**
With a valid area selected, pressing Return while the recording-mode menu is open calls `area_key` and starts recording in the current mode. The key handler checks the other open menus but skips `mode_menu`, so the user can start recording instead of finishing their mode choice. Handle `mode_menu` before calling `area_key`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
@greptileai fixed in 6c05c6a: the area keys are skipped while the mode menu is open. Please re-review. |
|
@greptileai the remaining descriptive comments are gone in 9ad696a (only the ones giving a reason stay); please re-review. |
| if key == "escape" | ||
| && let Some(drag) = self.drag.take() | ||
| { | ||
| self.crop = match drag { | ||
| AreaDrag::Draw { .. } => None, | ||
| AreaDrag::Move { start, .. } | AreaDrag::Resize { start, .. } => Some(start), | ||
| }; |
There was a problem hiding this comment.
On macOS, Escape during a drag closes the picker instead of restoring the area. The registered global Escape shortcut calls dismiss_target_overlays, bypassing this new area_key branch. Route the global shortcut through drag cancellation first, and dismiss only when there is no drag to cancel.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 3788-3794
Comment:
**Escape closes the picker**
On macOS, Escape during a drag closes the picker instead of restoring the area. The registered global Escape shortcut calls `dismiss_target_overlays`, bypassing this new `area_key` branch. Route the global shortcut through drag cancellation first, and dismiss only when there is no drag to cancel.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| self.crop = match drag { | ||
| AreaDrag::Draw { .. } => None, | ||
| AreaDrag::Move { start, .. } | AreaDrag::Resize { start, .. } => Some(start), |
There was a problem hiding this comment.
Pressing outside an existing selection replaces crop and starts AreaDrag::Draw. Cancelling that draw with Escape always restores None, so the user loses their earlier selection and must draw it again. Save the previous crop when drawing starts and restore it when the user cancels.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 3791-3793
Comment:
**Cancelling loses the old area**
Pressing outside an existing selection replaces `crop` and starts `AreaDrag::Draw`. Cancelling that draw with Escape always restores `None`, so the user loses their earlier selection and must draw it again. Save the previous crop when drawing starts and restore it when the user cancels.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if self.area_locked { | ||
| self.area_locked = false; | ||
| store_locked_area(None); | ||
| } else { | ||
| let min = area_min_size(self.select.read(cx).recording_mode); | ||
| let Some(crop) = self.crop.filter(|crop| crop.is_valid_for(min)) else { | ||
| return; | ||
| }; | ||
| self.area_locked = true; | ||
| self.drag = None; | ||
| self.snap_guides.clear(); | ||
| store_locked_area(Some((&self.display_id, crop))); |
There was a problem hiding this comment.
Locking an area on display B replaces the saved lock but leaves display A's area_locked unchanged. A still shows “Locked”; unlocking or resetting A then clears B's saved lock. Clear the old display's lock when ownership changes, and only let the owning display clear the saved lock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 3692-3703
Comment:
**Display locks disagree**
Locking an area on display B replaces the saved lock but leaves display A's `area_locked` unchanged. A still shows “Locked”; unlocking or resetting A then clears B's saved lock. Clear the old display's lock when ownership changes, and only let the owning display clear the saved lock.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…lds the area lock
|
@greptileai fixed:
The full desktop test suite passes (1548). Please re-review. |
…, fit and move strings
|
@greptileai merged main (which localized the picker) in a9cda4d: the area hint conflict is resolved and the ten new area strings (click to fit, drag to move, locked badge, lock and unlock hints) are in every catalog, with their surface test pairs. Full desktop suite passes (1563). Please re-review. |
| impl Render for OverlayWindow { | ||
| fn render(&mut self, window: &mut Window, cx: &mut Context<Self>) -> impl IntoElement { | ||
| #[cfg(debug_assertions)] | ||
| probe_frames::rendered(); |
There was a problem hiding this comment.
probe_frames counts renders from every overlay, but drag_frames reports them as the dragged overlay's frame rate. With multiple displays, microphone updates can repaint the other overlays during the sample. Those renders inflate fps and mix unrelated timestamps into the frame gaps. Record frames only for the overlay being measured.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/target_overlay.rs
Line: 1757
Comment:
**Other displays inflate probe results**
`probe_frames` counts renders from every overlay, but `drag_frames` reports them as the dragged overlay's frame rate. With multiple displays, microphone updates can repaint the other overlays during the sample. Those renders inflate `fps` and mix unrelated timestamps into the frame gaps. Record frames only for the overlay being measured.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
@greptileai fixed in 391f438: probe frames are recorded only for the dragged display's overlay. Please re-review. |
Makes the area picker much easier to use. Stacked on #3511 (it uses that PR's tooltip placement, which keeps hints off the Start button).
Lock and remember
locked,screenId,bounds).Snapping
Faster to aim
Clearer bounds and handles
Camera preview
Probe
Test plan
cargo testfor the overlay, camera window and area camera tests (52 pass), including new tests for window and work-area snapping, single-axis edge snapping, snap rect filtering and the toolbar fitTried by hand in a local build on macOS: lock, snapping, click to fit, handles, camera toolbar
Picker probe on the Mac mini (1080p, 60 Hz, release build with debug assertions), dragging through the real handlers:
Footprint 201 MB after warmup, 185 MB after 30 s more dragging (no growth). Window arrangement checks pass.
CI
The PR appears safe to merge; the probe now excludes frames from other displays.Summary
The PR adds saved area locks, window snapping, click-to-fit, keyboard controls, clearer handles, and camera controls that fit narrow previews.
Reviews (7) · Last reviewed commit: "fix(desktop-gpui): count only the dragge..." · Reviewed by Greptile