Conversation
* move to suggests (similar to the optional dependency of the Arch PKGBUILD) Signed-off-by: Thomas Reifenberger <thomas.reifenberger@tngtech.com>
There was a problem hiding this comment.
Pull request overview
Updates packaging and clipboard integration to remove a hard dependency on xclip, allowing xsel/xclip/wl-clipboard to be optional and used as available.
Changes:
- Linux/X11 clipboard fallback now tries
xselfirst, thenxclip, with an updated error message when neither is available. - Arch packaging metadata updated to prefer
xselwithxclipas a fallback. - Debian package metadata updated to drop
xclipfromDependsand add clipboard tools underSuggests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
please.sh |
Updates X11 clipboard handling to prefer xsel and improves missing-tool messaging. |
PKGBUILD |
Adjusts optdepends to list xsel preferred and xclip as fallback. |
.github/workflows/create-release.yml |
Updates Debian control metadata to move clipboard tools to Suggests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ;; | ||
| Linux*) | ||
| if [ "$XDG_SESSION_TYPE" == "wayland" ]; then | ||
| echo -n "${command}" | wl-copy --primary |
There was a problem hiding this comment.
On Wayland, this calls wl-copy unconditionally. Since wl-clipboard is now an optional dependency (Suggests/optdepends), this can fail with a plain "command not found". Consider mirroring the X11 logic by checking for wl-copy (and optionally wl-paste) with command -v and emitting a clear error message when missing.
| echo -n "${command}" | wl-copy --primary | |
| if command -v wl-copy &> /dev/null; then | |
| echo -n "${command}" | wl-copy --primary | |
| else | |
| echo "wl-copy is not installed. Please install wl-clipboard to copy to the clipboard on Wayland. Exiting." | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
This seems like an obvious improvement to me. If nobody objects, I'll incorporate this change next week.
| echo "Neither xsel nor xclip is installed. Exiting." | ||
| exit 1 |
There was a problem hiding this comment.
This error message is written to stdout. For an error path that exits non-zero, it should go to stderr (e.g., redirect the echo) so users can pipe/parse normal output reliably.
There was a problem hiding this comment.
Sounds reasonable. If nobody objects, I'll incorporate this change next week.
|
@tom-mi, can you have a look at the changes suggested by Copilot? |
| echo -n "${command}" | wl-copy --primary | ||
| else | ||
| if command -v xclip &> /dev/null; then | ||
| if command -v xsel &> /dev/null; then |
There was a problem hiding this comment.
What I'm not sure about here is that this changes the default command that is used. Previously, we always used xclip, and didn't support xsel at all, and now we are preferring xsel.
This means that for some users who previously used xclip, but also have xsel installed, the behavior would now change. In terms of keeping behavior consistent, I would prefer xclip as the first option. But I'm not knowledgeable enough about the Unix ecosystem of clipboard managers to judge the differences.
@tom-mi did you have a specific reason for putting xsel first?
There was a problem hiding this comment.
Well my reasoning was that as xclip is banned on certain machines in our organization, xsel might be the better choice altogether. But I don't have a strong opinion. I'll update the PR accordingly to keep the default behaviour
Signed-off-by: Thomas Reifenberger <thomas.reifenberger@tngtech.com>
tom-mi
force-pushed
the
feat-drop-xclip-dependency
branch
from
fb21779 to
94577cf
Compare
As discussed in #67
Works locally installing the .deb in Debian 13 (I did not test it on Arch, but the package is also called xsel in Arch, so I don't expect any surprises)
Note
Low Risk
Low risk: packaging metadata and a small Linux clipboard fallback change; main command-generation logic is untouched.
Overview
Drops the hard Debian dependency on
xclipby moving clipboard tools to suggested/optional dependencies (Suggests: xsel, xclip, wl-clipboard) and updating the ArchPKGBUILDoptdependsto preferxselwithxclipas fallback.Updates
please.shclipboard handling on Linux/X11 to tryxselfirst, thenxclip, and improves the error message when neither is installed.Reviewed by Cursor Bugbot for commit fb21779. Bugbot is set up for automated code reviews on this repo. Configure here.