Re: [PATCH RFC v1 1/1] ui/sdl2: clipboard sharing implementation for SDL

[Marc-André Lureau]

Hi

On Wed, Nov 8, 2023 at 7:56 PM Kamay Xutax <admin@xutaxkamay.com> wrote:
>
> Signed-off-by: Kamay Xutax <admin@xutaxkamay.com>
> ---
>  include/ui/sdl2.h   |   5 ++
>  meson.build         |   1 +
>  ui/meson.build      |   1 +
>  ui/sdl2-clipboard.c | 147 ++++++++++++++++++++++++++++++++++++++++++++
>  ui/sdl2.c           |   8 +++
>  5 files changed, 162 insertions(+)
>  create mode 100644 ui/sdl2-clipboard.c
>
> diff --git a/include/ui/sdl2.h b/include/ui/sdl2.h
> index e3acc7c82a..120fe6f856 100644
> --- a/include/ui/sdl2.h
> +++ b/include/ui/sdl2.h
> @@ -21,6 +21,7 @@
>  # include <SDL_image.h>
>  #endif
>
> +#include "ui/clipboard.h"
>  #include "ui/kbd-state.h"
>  #ifdef CONFIG_OPENGL
>  # include "ui/egl-helpers.h"
> @@ -51,6 +52,7 @@ struct sdl2_console {
>      bool y0_top;
>      bool scanout_mode;
>  #endif
> +    QemuClipboardPeer cbpeer;
>  };
>
>  void sdl2_window_create(struct sdl2_console *scon);
> @@ -70,6 +72,9 @@ void sdl2_2d_redraw(struct sdl2_console *scon);
>  bool sdl2_2d_check_format(DisplayChangeListener *dcl,
>                            pixman_format_code_t format);
>
> +void sdl2_clipboard_handle_request(struct sdl2_console *scon);
> +void sdl2_clipboard_init(struct sdl2_console *scon);
> +
>  void sdl2_gl_update(DisplayChangeListener *dcl,
>                      int x, int y, int w, int h);
>  void sdl2_gl_switch(DisplayChangeListener *dcl,
> diff --git a/meson.build b/meson.build
> index 4848930680..1358d14b2e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2157,6 +2157,7 @@ config_host_data.set('CONFIG_RDMA', rdma.found())
>  config_host_data.set('CONFIG_RELOCATABLE', get_option('relocatable'))
>  config_host_data.set('CONFIG_SAFESTACK', get_option('safe_stack'))
>  config_host_data.set('CONFIG_SDL', sdl.found())
> +config_host_data.set('CONFIG_SDL_CLIPBOARD', sdl.found())

'gtk_clipboard' option is there because it has some issues with the
glib loop - see https://gitlab.com/qemu-project/qemu/-/issues/1150.

Apparently this code could have similar kind of issues, since it's
reentering the main loop too.  it might be worth to have a similar
option and disclaimer...


>  config_host_data.set('CONFIG_SDL_IMAGE', sdl_image.found())
>  config_host_data.set('CONFIG_SECCOMP', seccomp.found())
>  if seccomp.found()
> diff --git a/ui/meson.build b/ui/meson.build
> index 0ccb3387ee..0cadd1a18f 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -125,6 +125,7 @@ if sdl.found()
>    sdl_ss = ss.source_set()
>    sdl_ss.add(sdl, sdl_image, pixman, glib, files(
>      'sdl2-2d.c',
> +    'sdl2-clipboard.c',
>      'sdl2-input.c',
>      'sdl2.c',
>    ))
> diff --git a/ui/sdl2-clipboard.c b/ui/sdl2-clipboard.c
> new file mode 100644
> index 0000000000..405bb9ea8b
> --- /dev/null
> +++ b/ui/sdl2-clipboard.c
> @@ -0,0 +1,147 @@
> +/*
> + * SDL UI -- clipboard support
> + *
> + * Copyright (C) 2023 Kamay Xutax <admin@xutaxkamay.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "ui/sdl2.h"
> +
> +static void sdl2_clipboard_update(struct sdl2_console *scon,
> +                                  QemuClipboardInfo *info)
> +{
> +    bool self_update = info->owner == &scon->cbpeer;
> +    char *text;
> +    size_t text_size;
> +
> +    /*
> +     * In case of a self update,
> +     * set again the text in SDL
> +     *
> +     * This is a workaround for hosts that have clipboard history
> +     * or when they're copying again something,
> +     * so that SDL can accept a new request from the host
> +     * and make a new SDL_CLIPBOARDUPDATE event
> +     */
> +
> +    if (self_update) {
> +        text = SDL_GetClipboardText();
> +        SDL_SetClipboardText(text);
> +        SDL_free(text);
> +        return;
> +    }

Isn't this basically doing the work of a clipboard manager? it takes
the current clipboard data and makes qemu the new owner. It looks like
it could also run in a loop quite easily if it fights with a manager.

> +
> +    if (!info->types[QEMU_CLIPBOARD_TYPE_TEXT].available) {
> +        return;
> +    }
> +
> +    info = qemu_clipboard_info_ref(info);
> +    qemu_clipboard_request(info, QEMU_CLIPBOARD_TYPE_TEXT);
> +
> +    while (info == qemu_clipboard_info(info->selection) &&
> +           info->types[QEMU_CLIPBOARD_TYPE_TEXT].available &&
> +           info->types[QEMU_CLIPBOARD_TYPE_TEXT].data == NULL) {
> +        main_loop_wait(false);

Reentering the loop, that's annoying... same as gtk-clipboard.c..

 Have you tried to defer the handling of the update? That will add
extra state & logic though.

> +    }
> +
> +    /* clipboard info changed while waiting for data */
> +    if (info != qemu_clipboard_info(info->selection)) {
> +        qemu_clipboard_info_unref(info);
> +        return;
> +    }
> +
> +    /* text is not null terminated in cb info, so we need to copy it */
> +    text_size = info->types[QEMU_CLIPBOARD_TYPE_TEXT].size;

hmm, I wonder why.. isn't it something we could fix from
qemu_clipboard_set_data() callers?

(gtk_selection_data_set_text() doc says it should be \0 terminated,
although it seems not required when len is given).

I am not sure if the spice/vdagent ensures \0 terminated strings, but
the qemu code could adjust it as necessary.


> +    if (!text_size) {
> +        qemu_clipboard_info_unref(info);
> +        return;
> +    }
> +
> +    text = malloc(text_size + 1);

We use g_malloc() and g_free() (even better with g_autofree).

> +
> +    if (!text) {

Then, no need to check for NULL results (it aborts on OOM).

> +        qemu_clipboard_info_unref(info);
> +        return;
> +    }
> +
> +    text[text_size] = 0;
> +    memcpy(text, info->types[QEMU_CLIPBOARD_TYPE_TEXT].data, text_size);
> +    /* unref as soon we copied the text */
> +    qemu_clipboard_info_unref(info);
> +    SDL_SetClipboardText(text);
> +
> +    free(text);
> +}
> +
> +static void sdl2_clipboard_notify(Notifier *notifier,
> +                                  void *data)
> +{
> +    QemuClipboardNotify *notify = data;
> +    struct sdl2_console *scon =
> +        container_of(notifier, struct sdl2_console, cbpeer.notifier);
> +
> +    switch (notify->type) {
> +    case QEMU_CLIPBOARD_UPDATE_INFO:
> +        sdl2_clipboard_update(scon, notify->info);
> +        break;
> +    case QEMU_CLIPBOARD_RESET_SERIAL:
> +        break;
> +    }
> +}
> +
> +static void sdl2_clipboard_request(QemuClipboardInfo *info,
> +                                   QemuClipboardType type)
> +{
> +    struct sdl2_console *scon =
> +        container_of(info->owner, struct sdl2_console, cbpeer);
> +    char *text;
> +
> +    switch (type) {
> +    case QEMU_CLIPBOARD_TYPE_TEXT:
> +        if (!SDL_HasClipboardText()) {
> +            return;
> +        }
> +
> +        text = SDL_GetClipboardText();
> +        qemu_clipboard_set_data(&scon->cbpeer, info, type,
> +                                strlen(text), text, true);

strlen() + 1  to have \0 then? (other backends would need similar fix)

> +
> +        SDL_free(text);
> +        break;
> +    default:
> +        return;
> +    }
> +}
> +
> +void sdl2_clipboard_handle_request(struct sdl2_console *scon)
> +{
> +    g_autoptr(QemuClipboardInfo) info =
> +        qemu_clipboard_info_new(&scon->cbpeer,
> +                                QEMU_CLIPBOARD_SELECTION_CLIPBOARD);
> +
> +    sdl2_clipboard_request(info, QEMU_CLIPBOARD_TYPE_TEXT);
> +}
> +
> +void sdl2_clipboard_init(struct sdl2_console *scon)
> +{
> +    scon->cbpeer.name = "sdl2";
> +    scon->cbpeer.notifier.notify = sdl2_clipboard_notify;
> +    /* requests will be handled from the SDL event loop */
> +    qemu_clipboard_peer_register(&scon->cbpeer);
> +}
> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index fbfdb64e90..d674add7c5 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -702,6 +702,11 @@ void sdl2_poll_events(struct sdl2_console *scon)
>          case SDL_WINDOWEVENT:
>              handle_windowevent(ev);
>              break;
> +#if defined(CONFIG_SDL_CLIPBOARD)
> +        case SDL_CLIPBOARDUPDATE:
> +            sdl2_clipboard_handle_request(scon);
> +            break;
> +#endif
>          default:
>              break;
>          }
> @@ -922,6 +927,9 @@ static void sdl2_display_init(DisplayState *ds, 
> DisplayOptions *o)
>              qemu_console_set_window_id(con, info.info.x11.window);
>  #endif
>          }
> +#endif
> +#if defined(CONFIG_SDL_CLIPBOARD)
> +        sdl2_clipboard_init(&sdl2_console[i]);
>  #endif
>      }
>
> --
> 2.41.0
>
>


-- 
Marc-André Lureau