Fix AppShots external target selection - #1384
Conversation
5c0d51a to
863ddf6
Compare
9245b34 to
b0a24ea
Compare
ilysenko
left a comment
There was a problem hiding this comment.
Thanks for completing the native Wayland picker work and interactive Hyprland validation. Before this can merge, please address the remaining capture-safety issues:
- Establish that the selected workspace is actually rendered before capturing, and revalidate the selected window’s identity, focus, visibility, and bounds before accepting the acquired pixels.
- Serialize capture transactions or add explicit ownership and cancellation so concurrent requests cannot race global compositor focus and attach pixels from another target.
- Reuse the repository’s Hyprland session/instance resolution for picker execution and focus/restore when the Electron environment lacks
HYPRLAND_INSTANCE_SIGNATURE. - Require a valid ChatGPT return target before activating another workspace, and fail closed unless restoration is successfully verified before delivering the attachment.
- Make picker invocation binary-aware. Preview-only config arguments must not be passed to legacy or custom pickers, and the documented Windows-first behavior must match each supported picker.
Please add regression coverage for these failure and concurrency paths.
Verify the updated PR against CONTRIBUTING.md, run the relevant tests locally before pushing the update, and wait for every required CI check to pass. Run the complete updated base-to-head diff through your own code-review model, fix every blocker it finds, rerun the review, and repeat until the model reports no remaining blockers before requesting maintainer re-review.
Open the Hyprland share picker on its window page so users explicitly choose an external window after the composer takes focus. Activate the selected window before capturing the visible workspace, wait for stable current bounds, and restore ChatGPT even when capture fails. Preserve the user picker configuration through a temporary derived override, keep the EWMH stacking fallback for X11, and fail closed when no safe target is available. Docs: document picker selection, focus transition, and unsupported-backend behavior.
Collect window pixels and accessibility data concurrently, restore ChatGPT before delivering the capture to the renderer, and report failed capture stages with a stable Linux reason. Document the window-only picker contract and cover the new ordering and concurrency behavior. Docs: document capture ordering and unsupported picker result types.
Force GTK and Qt Wayland backends for the Hyprland picker so launcher-level X11 compatibility settings cannot demote its layer-shell overlay into a tiled client. Add regression coverage for inherited backend variables and document the runtime boundary.\n\nDocs: linux-features/appshots/README.md
9d8c962 to
eccfb7c
Compare
|
Thanks for the detailed review. I addressed the requested capture-safety changes in
Regression coverage now includes the failure, drift, restoration, and concurrency paths. The branch is rebased, the complete base-to-head diff was reviewed again with no remaining blockers, local validation passed, and all required CI checks are green. Re-requesting review. |
mise ~/.config/mise/config.toml tools: gh@2.98.0
Summary
Fixes #1383.
Opening the composer attachment menu focuses ChatGPT Community before AppShots queries the Linux window backend. An explicit picker is required, but selection alone is insufficient on Hyprland: ordinary screenshot CLIs capture the visible workspace, so cropping that image with bounds from another workspace can attach the wrong pixels.
This disabled-by-default
appshotsfeature now:hyprland-preview-share-picker;$XDG_RUNTIME_DIR/hyprwhen Electron did not inheritHYPRLAND_INSTANCE_SIGNATURE;It prefers
hyprland-preview-share-picker, falls back tohyprland-share-picker, and supportsCODEX_LINUX_APPSHOT_PICKERas an explicit executable override.User-visible behavior
On Hyprland, the composer offers a generic
Attach window...entry. Activating it opens the preview selector directly on Windows even when its persistent config defaults to Outputs or Region. Selecting a window on another workspace briefly displays that workspace while AppShots captures it, then restores ChatGPT Community before the attachment is delivered.The preview selector is a centered native Wayland layer-shell overlay, not a normal tiled client. Output and Region selections remain intentionally fail-closed because the current AppShots contract accepts only a Window target.
Scope and compatibility
Validation
Passed on the rebased branch:
An isolated official-bundle build with only
appshotsenabled applied exactly its three descriptors against officialchatgpt 26.825.32147 amd64(SHA-256986d38b690dd0310933ce61175b09c27434001f4e114332bb0f7b6ffdc3ca406).The updated regression coverage includes binary-aware picker arguments, temporary config cleanup, missing inherited Hyprland instance resolution, invalid return targets, target identity/focus/visibility/bounds drift, pre- and post-capture validation, verified restoration before delivery, restoration failure, concurrent pixel/accessibility acquisition, and serialized capture requests.
Interactive Hyprland validation
Previously confirmed on Omarchy / Hyprland / native Wayland:
Interactive X11 remains untested; synthetic coverage protects the EWMH fallback.
Review status
The branch is rebased onto current
main. I reviewed the complete updated base-to-head diff after the changes above and found no remaining blocker. Local validation and all required GitHub checks are green. The complete updated base-to-head diff was reviewed again before requesting maintainer re-review.Checklist
CONTRIBUTING.mdand kept the change inside the optional feature.