feat: add xsel fallback; implement program_exists via PATH search; - #5
Conversation
…cument xsel in README
There was a problem hiding this comment.
Pull Request Overview
This PR adds xsel as a fallback clipboard utility for Linux systems and implements a proper PATH-based program existence check. The changes enhance clipboard functionality robustness by supporting multiple clipboard utilities and improving the detection mechanism.
- Added
program_exists()function that searches PATH directories for executable programs - Enhanced Linux clipboard support with xsel as an alternative to xclip
- Updated documentation to reflect the new xsel support
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/utils.c | Implements program_exists() function and refactors clipboard logic to support xclip, xsel, and wl-copy with proper fallback handling |
| src/recap.h | Adds function declaration for program_exists() |
| README.md | Updates documentation to mention xsel as an alternative X11 clipboard utility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| result = system(command); | ||
| if (result != 0) { | ||
| fprintf(stderr, "Warning: 'wl-copy' command failed. Is 'wl-clipboard' installed?\n"); | ||
| if (!getenv("WAYLAND_DISPLAY")) { |
There was a problem hiding this comment.
The logic is inverted. This condition should check if (getenv("WAYLAND_DISPLAY")) to handle Wayland environments, or the X11 and Wayland blocks should be swapped. Currently, X11 clipboard utilities are being used when WAYLAND_DISPLAY is NOT set, which is backwards.
| char* dir = strtok_r(paths, ":", &saveptr); | ||
| while (dir) { | ||
| char candidate[MAX_PATH_SIZE]; | ||
| snprintf(candidate, sizeof(candidate), "%s/%s", dir, name); |
There was a problem hiding this comment.
Potential buffer overflow if the combined path length exceeds MAX_PATH_SIZE. Consider checking the length of dir and name before concatenation, or use a safer approach like asprintf() for dynamic allocation.
| snprintf(candidate, sizeof(candidate), "%s/%s", dir, name); | |
| int n = snprintf(candidate, sizeof(candidate), "%s/%s", dir, name); | |
| if (n < 0 || (size_t)n >= sizeof(candidate)) { | |
| // Path too long, skip this entry | |
| dir = strtok_r(NULL, ":", &saveptr); | |
| continue; | |
| } |
| else if (program_exists("wl-copy")) { | ||
| snprintf(command, sizeof(command), "cat \"%s\" | wl-copy", filepath); | ||
| result = system(command); | ||
| if (result != 0) { | ||
| fprintf(stderr, "Warning: 'wl-copy' command failed.\n"); | ||
| } | ||
| } | ||
| else { | ||
| fprintf(stderr, "Error: No clipboard utility found. Install 'xclip', 'xsel', or 'wl-clipboard'.\n"); |
There was a problem hiding this comment.
In the X11 block (when WAYLAND_DISPLAY is not set), checking for wl-copy as a fallback is illogical. Wayland utilities should not be used in X11 environments. Consider removing this check or moving it to the appropriate context.
Some recap v2.1 features: