qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: BALATON Zoltan
Subject: Re: [PATCH RFC v1 1/1] ui/sdl2: clipboard sharing implementation for SDL
Date: Tue, 14 Nov 2023 13:28:07 +0100 (CET)

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?

Regards,
BALATON Zoltan

+    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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]