feat(backend): capture regions spanning multiple outputs - #223
Conversation
Reviewer's GuideImplement multi-output global region capture by compositing per-output images, while preserving the existing single-output path and wiring interactive selection to use the new compositor logic instead of rejecting spanning regions. Sequence diagram for multi-output global region capturesequenceDiagram
participant Caller
participant Wayland as capture_global_region
participant Outputs as list_outputs
participant Resolver as resolve_global_region_in
participant OutputCap as capture_output_region_logical
participant PerOutput as capture_output
participant Composer as crop_frozen_global_region
Caller->>Wayland: capture_global_region(requested, cursor)
Wayland->>Outputs: list_outputs()
Outputs-->>Wayland: outputs
Wayland->>Resolver: resolve_global_region_in(outputs, requested)
Resolver-->>Wayland: (output_name, local)
Wayland->>Wayland: compute fits
alt fits in single output
Wayland->>OutputCap: capture_output_region_logical(Some(output_name), local, cursor)
OutputCap-->>Wayland: CapturedImage
Wayland-->>Caller: CapturedImage
else spanning region
loop intersecting outputs
Wayland->>PerOutput: capture_output(Some(name), cursor)
PerOutput-->>Wayland: CapturedOutput
end
Wayland->>Composer: crop_frozen_global_region(captured, outputs, requested)
Composer-->>Wayland: CapturedImage
Wayland-->>Caller: CapturedImage
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
capture_global_regionlogic for checking whether a region fits within a single output duplicates the bounds calculation used elsewhere; consider extracting a small helper (e.g.,region_fits_output(output, local)) to keep this logic centralized and less error-prone. - In
crop_frozen_global_region, you perform multipleoutputs.iter().find(|output| output.name == capture.name)lookups inside loops; caching aHashMap<String, &OutputInfo>upfront would simplify the code and avoid repeated linear searches. - Error messages for non-intersecting regions are now emitted in several places (
capture_global_region,crop_frozen_global_region); it may be clearer to unify the wording and ensure they are emitted consistently from a single pathway.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `capture_global_region` logic for checking whether a region fits within a single output duplicates the bounds calculation used elsewhere; consider extracting a small helper (e.g., `region_fits_output(output, local)`) to keep this logic centralized and less error-prone.
- In `crop_frozen_global_region`, you perform multiple `outputs.iter().find(|output| output.name == capture.name)` lookups inside loops; caching a `HashMap<String, &OutputInfo>` upfront would simplify the code and avoid repeated linear searches.
- Error messages for non-intersecting regions are now emitted in several places (`capture_global_region`, `crop_frozen_global_region`); it may be clearer to unify the wording and ensure they are emitted consistently from a single pathway.
## Individual Comments
### Comment 1
<location path="dms-screenshot-rs/src/main.rs" line_range="153-154" />
<code_context>
+ .iter()
.find(|output| output.name == output_name)
.ok_or_else(|| format!("output disappeared during selection: {output_name}"))?;
+ let (output_x, output_y) = output.position.unwrap_or((0, 0));
let local = Rect {
- x: rect.x - output.position.map(|position| position.0).unwrap_or(0),
- y: rect.y - output.position.map(|position| position.1).unwrap_or(0),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Defaulting missing output positions to (0,0) can silently mask configuration issues.
Using `unwrap_or((0, 0))` causes outputs with no position to be treated as if they were at the origin, affecting region fitting and compositing. Since multi-output compositing relies on correct global coordinates, consider returning an explicit error when `position` is `None` (like `crop_frozen_global_region` does) instead of defaulting to (0,0), so misconfigurations are detected rather than silently hidden.
Suggested implementation:
```rust
let outputs = wayland::list_outputs()?;
let output = outputs
.iter()
.find(|output| output.name == output_name)
.ok_or_else(|| format!("output disappeared during selection: {output_name}"))?;
let (output_x, output_y) = output
.position
.ok_or_else(|| format!("output has no position configured: {output_name}"))?;
let local = Rect {
x: rect.x - output_x,
y: rect.y - output_y,
width: rect.width,
height: rect.height,
};
let logical_width = (output.width as f64 / output.scale.max(1.0)).round() as i32;
let logical_height = (output.height as f64 / output.scale.max(1.0)).round() as i32;
```
If this function's return type is not already `Result<_, String>` (or otherwise compatible with the `format!`-produced error), you may need to adjust the error type or mapping to fit your existing error handling conventions. For consistency with `crop_frozen_global_region`, consider using the same error type and message style that function uses for missing positions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Global-logical regions that intersect more than one output are now composited into a single image instead of being rejected or clipped: - crop_frozen_global_region: composite per-output captures for a global region; result scale follows the output containing the region's center, mixed-DPI pieces resampled into the canvas - capture_global_region: single-output regions keep the existing compositor-side region capture; only spanning regions capture the intersecting outputs - interactive flow: 'region crosses output boundaries' replaced by compositing the frozen captures, so the result matches what was selected
cd02672 to
7b0e9f0
Compare
|
Addressed the review — pushed 1. Duplicated fits check → helper (adopted). Extracted 2. Repeated name lookups → HashMap (skipped). The 3. Unified error pathway (partially adopted). The wording was already identical in all spots ("region does not intersect a Wayland output"). The real structural wart was in 4. Re-verified after the refactor: single-output and spanning (both center placements) captures are 0-pixel-diff against the all-outputs composite, stale-region error unchanged, and |
Problem
On multi-output setups, region selections expressed in global coordinates that span more than one output are rejected:
region crosses output boundaries— a drag that starts on one output and extends past its edge produces a spanning rect (the pointer grab is retained past the surface edge), which the capture then refuses.last): resolved to the single output containing the region's center; a spanning region is clipped to that output, and regions whose center has no output fail outright. Saved regions can end up spanning after a monitor layout change.Fix
Composite the intersecting outputs into a single image:
crop_frozen_global_region(wayland.rs): pure composition over per-output captures. Each intersecting output contributes its slice; the result scale follows the output containing the region's center; pieces from outputs at a different scale are resampled into the canvas (Triangle filter). Outputs without position info are skipped (they cannot be placed in the global workspace); no intersection at all returns a clean error.capture_global_region: single-output regions keep the existing compositor-side region capture path (zero behavior change). Only spanning regions capture the intersecting outputs — no extra screencopy sessions in the common case.region crosses output boundariesrejection is replaced by composing the frozen captures, so the result matches exactly what the user saw while selecting.Verification
full,all,list: unchanged.cargo clippy --all-targets --all-features -- -D warnings(the CI configuration added in4d4084f): clean.Notes
capture_all).Summary by Sourcery
Support global region capture across output boundaries while preserving existing single-output behavior.
New Features:
Bug Fixes:
Enhancements:
Related issues
Closes #222 — the non-focused-output failure (
region is outside the frozen capture) reported there is already fixed onmainby70dd4b8(per-output frozen captures); this PR completes the multi-output region story by handling selections that span several outputs.