Cloudef / bemenu

Dynamic menu library and client program inspired by dmenu
GNU General Public License v3.0
1.16k stars 90 forks source link

C-y to paste: Issues with falling back to `xclip` #363

Closed vain closed 6 days ago

vain commented 8 months ago

The code currently does this:

FILE *clipboard;
clipboard = popen("wl-paste -t text/plain", "r");
if (!clipboard) {
    clipboard = popen("xclip -t text/plain -out", "r");
}
if (!clipboard) {
    break;
}

The intention here is probably to try to execute wl-paste and, if that doesn't work, fall back to xclip.

I don't think that's the correct way to do this. popen() will only return NULL if fork() or pipe() fails, but that doesn't tell you anything about the exec() call, nor if the child exited cleanly. The latter is the crucial information. For example, if both wl-paste and xclip are installed and you run bemenu on X11, wl-paste will properly exec() but eventually exit with an exit code of 1 because it can't talk to a Wayland compositor.

What we would need to to is: Open the pipe, try to read from it, and only use the result if the child exited with 0. If it didn't, try another paste program.

I'd suggest doing something like this:

char *paster[] = {"wl-paste -t text/plain", "xclip -t text/plain -out"};
char *pasted_buf = NULL;
size_t pasted_len = 0, pasted_fill = 0;
for (size_t paster_i = 0; paster_i < 2; paster_i++) {
    FILE *clipboard = popen(paster[paster_i], "r");
    if (clipboard == NULL)
        continue;

    char *line = NULL;
    size_t len = 0;
    ssize_t nread;

    while ((nread = getline(&line, &len, clipboard)) != -1) {
        while (pasted_fill + (size_t)nread >= pasted_len) {
            pasted_len += BUFSIZ;
            pasted_buf = realloc(pasted_buf, pasted_len);
            if (pasted_buf == NULL)
                abort();
        }

        memmove(pasted_buf + pasted_fill, line, (size_t)nread);
        pasted_fill += (size_t)nread;
    }

    free(line);

    if (pclose(clipboard) == 0) {
        for (size_t i = 0; i < pasted_fill; i++) {
            size_t width;
            menu->cursor += bm_unicode_insert(&menu->filter, &menu->filter_size, menu->cursor, pasted_buf[i], &width);
            menu->curses_cursor += width;
        }
        free(pasted_buf);
        break;
    } else {
        free(pasted_buf);
        pasted_buf = NULL;
        pasted_len = pasted_fill = 0;
    }
}

Note: I'm pretty tired and wrote this piece of code rather quickly without much testing. If you agree that this is the way to go, then I'd be happy to turn this into a proper pull request. 🙂

merrittlj commented 8 months ago

Thanks for adding code to this issue, it helps with understanding your idea. This approach looks good to me, I would reccomend turning it into a pull request. Code wise, I would add some sort of error-handling if the child didn't exit with 0, even something such as a print to the console to understand why it didn't use "x" paster. However, this code should likely be reviewed in the pull request, than here.

vain commented 6 days ago

Closing, this has been fixed in the meantime.