feat(screenshot): drag region with ctrl - #3157
Conversation
697dbf5 to
05053c5
Compare
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return math.Max(minX, math.Min(maxX-width, x)), | ||
| math.Max(minY, math.Min(maxY-height, y)) |
There was a problem hiding this comment.
Clamping to the exclusive output edge overshoots by one device pixel, because selection coords here are inclusive.
Moving the region flush right/bottom sets maxX exactly to output.x + logicalW, so selectionExtent() computes x2 = floor(logicalW*scaleX) + 1 = screenBuf.Width + 1. Then ext.within(ext.surface) is false, so even on a single output finishSelection() takes the selectionSpansOutputs() branch: the capture is 1px wider than the screen with a transparent column (nothing blits it), the size label reads e.g. 1921x..., and result.Output is cleared so "last region" can no longer replay it.
Clamp so the inclusive far edge stays inside the output, e.g. cap at maxX - width - 1/scaleX (one device pixel, same convention applyPreSelection uses for x2) instead of maxX - width.
Claude reviewOne off-by-one in the move clamp; the rest of the ctrl-drag flow looks sound.
|
05053c5 to
591e658
Compare
|
/claude review |
| r.selection.anchorX += deltaX | ||
| r.selection.currentX += deltaX | ||
| r.selection.anchorY += deltaY | ||
| r.selection.currentY += deltaY |
There was a problem hiding this comment.
Moving the region leaves r.selection.surface pointing at the output it was created on, and the clamp uses the union of all outputs, so on multi-monitor the region can end up entirely on a different output while selection.surface still names the old one.
Trace with A x=0,1920x1080 and B x=1920,1920x1080, selection on A at 100..299: ctrl-drag to global 2000..2199 (allowed, clamp cap is 3840-width-epsilon). selectionExtent() still uses A, so x2=2200 > 1920 and within(A) is false → finishSelection() takes the selectionSpansOutputs() branch even though the region sits wholly inside B. Consequences:
r.result.Outputis set to"", so "last region" can no longer replay the capture;capturedRegion.Outputreports A's name for pixels that came from B;- if the outputs have different scales the composite is built on A's grid, so a 200x200 logical region on a 2x B is captured at 200x200 instead of 400x400 (half resolution).
After applying the delta, re-home the selection: pick the surface that now contains the moved rect (fall back to the current one if none does) and assign it to r.selection.surface before returning, so a fully-contained move goes back through the single-output path.
Claude reviewThe earlier off-by-one is fixed by the
|
|
/claude review |
| epsilonY = float64(surface.logicalH) / float64(surface.screenBuf.Height) | ||
| } | ||
| } | ||
| return math.Max(minX, math.Min(maxX-width-epsilonX, x)), |
There was a problem hiding this comment.
The clamp uses the bounding box of all outputs, which on any non-uniform layout contains dead space no output covers, so a region can be dragged off-screen into it.
Two 1080p/1440p monitors side by side (A x=0,1920x1080, B x=1920,2560x1440): union maxY is 1440, so a 500px-tall region on A grabbed near its top can be dragged to maxY = 1440-eps — its bottom ~360 logical px sit below A's bottom edge over nothing. rehomeSelectionSurface() finds no containing output (A fails on maxY, B on minX) so the surface stays A, within(A) is false, and finishSelection() takes the selectionSpansOutputs() branch: only A intersects, so the bottom rows of the composite are never blitted and stay at composite.Clear() (transparent), while result.Output is cleared. During the drag it also looks fine — selectionRenderBounds() clamps to srcBuf.Height-1, so the drawn box stops at A's edge and the user never sees the part that's hanging off.
Clamping against the rect of the output under the pointer (r.activeSurface, falling back to the union when nil) instead of the union keeps the region on real screen area and still lets it follow the pointer across a boundary.
Claude reviewThe rehome fix resolves the previous multi-monitor finding; one clamp issue remains.
|
|
/claude review |
| epsilonY = float64(boundsSurface.logicalH) / float64(boundsSurface.screenBuf.Height) | ||
| } | ||
| } | ||
| return math.Max(minX, math.Min(maxX-width-epsilonX, x)), |
There was a problem hiding this comment.
The clamp range inverts when the region is larger than the bounds output, so an oversized axis snaps to the output origin and stops following the pointer.
With a cross-output selection (press on A, drag into B — allowed, finishSelectionAcrossOutputs() handles it), say width = 3000 and bounds A x=0,1920 wide: maxX-width-epsilonX = -1081, so math.Max(minX, math.Min(-1081, x)) always returns 0. On the first motion of a ctrl-drag the region jumps left to A's origin and then refuses to move horizontally for the rest of the drag; the same happens vertically for a region taller than the active output (e.g. a 1200px-tall region while the pointer's output is 1080p), and there the overhang can land in dead space again — no output intersects those rows, so the composite keeps the transparent Clear() rows and result.Output is emptied.
Guard each axis before clamping, e.g. only apply the clamp when maxX-width-epsilonX >= minX (falling back to the union bounds, or leaving that axis alone) instead of clamping into an empty range.
Claude reviewThe active-output clamp fixes the dead-space case for normal-sized regions; one inverted-range edge remains.
|
|
/claude review |
Claude reviewNo issues found. The union fallback resolves the previous inverted-clamp finding. |
2 participants
Add
Ctrl + left-dragsupport for moving the current screenshot region.The selection keeps its size and cursor offset while being moved, and is clamped to the available output bounds.