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

[Marc-André Lureau]

Hi

On Tue, Nov 14, 2023 at 4:27 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 14 Nov 2023, Marc-André Lureau wrote:
> > 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.
>
> What if copied text contains \0?
>

I don't think that's a valid utf8 string then:

@QEMU_CLIPBOARD_TYPE_TEXT: text/plain; charset=utf-8

or could it be?

-- 
Marc-André Lureau