Hi Jakub,
On Mon, Feb 26, 2018 at 08:01:28PM +0100, Jakub Janků wrote:
> From: Jakub Janků <jjanku@xxxxxxxxxx>
>
> Add clipboard handling that uses GTK+ instead of Xlib.
> Place the code into new files: clipboard.c, clipboard.h
>
> Use the old Xlib implementation by default.
> Add configure option --with-gtk-clipboard.
> Guard the code by WITH_GTK_CLIPBOARD variable -
> compile vdagent either with GTK+ impl or with the Xlib one.
> ---
> Makefile.am | 6 +
> configure.ac | 11 ++
> src/vdagent/clipboard.c | 430 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/vdagent/clipboard.h | 42 +++++
> src/vdagent/vdagent.c | 53 +++++-
> src/vdagent/x11-priv.h | 20 +++
> src/vdagent/x11.c | 30 +++-
> src/vdagent/x11.h | 6 +
> 8 files changed, 590 insertions(+), 8 deletions(-)
> create mode 100644 src/vdagent/clipboard.c
> create mode 100644 src/vdagent/clipboard.h
>
> diff --git a/Makefile.am b/Makefile.am
> index c4bd3dd..1d06928 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -85,6 +85,12 @@ src_spice_vdagentd_SOURCES += src/vdagentd/dummy-session-info.c
> endif
> endif
>
> +if WITH_GTK_CLIPBOARD
> +src_spice_vdagent_SOURCES += \
> + src/vdagent/clipboard.c \
> + src/vdagent/clipboard.h
> +endif
> +
Not exactly what I had in mind. The clipboad.[ch] is only
handling gtk code and the decision between gtk/x11 is on
vdagent.c by using #ifdef WITH_GTK_CLIPBOARD.
The only thing that vdagent.c has that clipboard.c doesn't in
order to handle the x11 clipboard is the x11 handler and "x11.h"
include for vdagent_x11_clipboard_*()
I think we could have one patch to simply introduce
clipboard.[ch] with vdagent_clipboards_*() functions that should
call the vdagent_x11_*() ones; and then a second patch that
introduces the gtk related code for clipboard, including
--with-gtk-clipboard. What do you think?
More comments in-line.
> xdgautostartdir = $(sysconfdir)/xdg/autostart
> xdgautostart_DATA = $(top_srcdir)/data/spice-vdagent.desktop
>
> diff --git a/configure.ac b/configure.ac
> index 1eb17a9..9a85870 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -75,6 +75,15 @@ if test "x$init_systemd" = "xyes"; then
> fi
> fi
>
> +AC_ARG_WITH([gtk-clipboard],
> + [AS_HELP_STRING([--with-gtk-clipboard], [Handle clipboard using GTK+ instead of Xlib])],
> + [],
> + [with_gtk_clipboard="no"])
Maybe we could have default set to "auto". Packagers are going to
notice the extra dependency + release notes and could chose
between enabling/disabling this.
> +if test "x$with_gtk_clipboard" = "xyes"; then
> + AC_DEFINE([WITH_GTK_CLIPBOARD], [1], [If defined, vdagent will handle clipboard using GTK+])
> +fi
> +AM_CONDITIONAL([WITH_GTK_CLIPBOARD], [test "x$with_gtk_clipboard" = "xyes"])
> +
> AC_ARG_ENABLE([pciaccess],
> [AS_HELP_STRING([--enable-pciaccess], [Enable libpciaccess use for auto generation of Xinerama xorg.conf (default: yes)])],
> [enable_pciaccess="$enableval"],
> @@ -210,6 +219,8 @@ AC_MSG_NOTICE([
> install systemd service: ${init_systemd}
> udevdir: ${udevdir}
>
> + GTK+ clipboard: ${with_gtk_clipboard}
> +
> Now type 'make' to build $PACKAGE
>
> ])
> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> new file mode 100644
> index 0000000..5ca1d57
> --- /dev/null
> +++ b/src/vdagent/clipboard.c
> @@ -0,0 +1,430 @@
> +/* clipboard.c - vdagent clipboard handling code
> +
> + Copyright 2017 Red Hat, Inc.
> +
> + 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 3 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/>.
> +*/
> +
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> +#include <gtk/gtk.h>
> +#include <syslog.h>
> +
> +#include "vdagentd-proto.h"
> +#include "spice/vd_agent.h"
> +#include "udscs.h"
> +#include "clipboard.h"
> +
> +/* 2 selections supported - _SELECTION_CLIPBOARD = 0, _SELECTION_PRIMARY = 1 */
> +#define SELECTION_COUNT (VD_AGENT_CLIPBOARD_SELECTION_PRIMARY + 1)
> +#define TYPE_COUNT (VD_AGENT_CLIPBOARD_IMAGE_JPG + 1)
> +
> +#define sel_id_from_clip(clipboard) \
> + GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), "vdagent-selection-id"))
> +
> +enum {
> + OWNER_NONE,
> + OWNER_GUEST,
> + OWNER_CLIENT
> +};
> +
> +typedef struct {
> + GMainLoop *loop;
> + GtkSelectionData *sel_data;
> +} AppRequest;
> +
> +typedef struct {
> + GtkClipboard *clipboard;
> + guint owner;
> +
> + GList *requests_from_apps; /* VDAgent --> Client */
> + GList *requests_from_client; /* Client --> VDAgent */
> + gpointer *last_targets_req;
> +
> + GdkAtom targets[TYPE_COUNT];
> +} Selection;
> +
> +struct VDAgentClipboards {
> + struct udscs_connection *conn;
> + Selection selections[SELECTION_COUNT];
> +};
> +
> +static const struct {
> + guint type;
> + const gchar *atom_name;
> +} atom2agent[] = {
> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "UTF8_STRING"},
> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain;charset=utf-8"},
> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "STRING"},
> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "TEXT"},
> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain"},
> + {VD_AGENT_CLIPBOARD_IMAGE_PNG, "image/png"},
> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/bmp"},
> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-bmp"},
> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-MS-bmp"},
> + {VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-win-bitmap"},
> + {VD_AGENT_CLIPBOARD_IMAGE_TIFF,"image/tiff"},
> + {VD_AGENT_CLIPBOARD_IMAGE_JPG, "image/jpeg"},
> +};
> +
> +static guint get_type_from_atom(GdkAtom atom)
> +{
> + gchar *name = gdk_atom_name(atom);
> + for (int i = 0; i < G_N_ELEMENTS(atom2agent); i++) {
I see that not code is not c89 complaint but nowhere else we
create variables in the for. I'd move this `int i` outside just
to keep code similar.
> + if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name)) {
> + g_free(name);
> + return atom2agent[i].type;
> + }
> + }
> + g_free(name);
> + return VD_AGENT_CLIPBOARD_NONE;
> +}
> +
> +/* gtk_clipboard_request_(, callback, user_data) cannot be cancelled.
> + Instead, gpointer *ref = new_request_ref() is passed to the callback.
> + If request_cancel(ref) is called,
> + callback using return_if_request_cancelled() returns.
> + This mechanism enables cancellation of the request
> + as well as passing VDAgentClipboards reference to the desired callback.
> + */
> +static gpointer *new_request_ref(gpointer data)
> +{
> + gpointer *ref = g_new(gpointer, 1);
> + *ref = data;
> + return ref;
> +}
> +
> +static gpointer free_request_ref(gpointer *ref)
> +{
> + gpointer data = *ref;
> + g_free(ref);
> + return data;
> +}
> +
> +#define request_cancel(ref) *((gpointer *)ref) = NULL
Better to check if ref is NULL, that way you can call
request_cancel() without having to check in the code;
> +
> +#define return_if_request_cancelled(ref) G_STMT_START { \
> + g_return_if_fail(ref != NULL); \
> + if (*((gpointer *)ref) == NULL) \
> + return; \
> +} G_STMT_END
IMHO, functions are fine for this.
A suggestion about naming is to use request_ref as prefix, e.g:
- request_ref_new(), request_ref_free(), request_ref_cancel() and
request_ref_is_cancelled()
> +
> +static void clipboard_new_owner(VDAgentClipboards *c, guint sel_id, guint new_owner)
> +{
> + Selection *sel = &c->selections[sel_id];
> + GList *l;
> + /* let the other apps know no data is coming */
> + for (l = sel->requests_from_apps; l != NULL; l= l->next) {
> + AppRequest *req = l->data;
> + g_main_loop_quit(req->loop);
> + }
> + g_clear_pointer(&sel->requests_from_apps, g_list_free);
> +
> + /* respond to pending client's data requests */
> + for (l = sel->requests_from_client; l != NULL; l = l->next) {
> + request_cancel(l->data);
> + if (c->conn)
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_DATA,
> + sel_id, VD_AGENT_CLIPBOARD_NONE, NULL, 0);
> + }
> + g_clear_pointer(&sel->requests_from_client, g_list_free);
> +
> + sel->owner = new_owner;
> +}
> +
> +static void clipboard_targets_received_cb(GtkClipboard *clipboard,
> + GdkAtom *atoms,
> + gint n_atoms,
> + gpointer user_data)
> +{
> + return_if_request_cancelled(user_data);
> + VDAgentClipboards *c = free_request_ref(user_data);
> + Selection *sel;
> + guint32 types[G_N_ELEMENTS(atom2agent)];
> + guint sel_id, type, n_types, a;
> +
> + sel_id = sel_id_from_clip(clipboard);
> + sel = &c->selections[sel_id];
> + sel->last_targets_req = NULL;
> +
> + if (atoms == NULL)
> + return;
> +
> + for (type = 0; type < TYPE_COUNT; type++)
> + sel->targets[type] = GDK_NONE;
> +
> + n_types = 0;
> + for (a = 0; a < n_atoms; a++) {
> + type = get_type_from_atom(atoms[a]);
> + if (type == VD_AGENT_CLIPBOARD_NONE || sel->targets[type] != GDK_NONE)
> + continue;
> +
> + sel->targets[type] = atoms[a];
> + types[n_types] = type;
> + n_types++;
> + }
> +
> + if (n_types == 0) {
> + syslog(LOG_WARNING, "%s: sel_id=%u: no target supported", __func__, sel_id);
> + return;
> + }
> +
> + clipboard_new_owner(c, sel_id, OWNER_GUEST);
> +
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_GRAB, sel_id, 0,
> + (guint8 *)types, n_types * sizeof(guint32));
> +}
> +
> +static void clipboard_owner_change_cb(GtkClipboard *clipboard,
> + GdkEventOwnerChange *event,
> + gpointer user_data)
> +{
> + VDAgentClipboards *c = user_data;
> + guint sel_id = sel_id_from_clip(clipboard);
> + Selection *sel = &c->selections[sel_id];
> +
> + /* if the event was caused by gtk_clipboard_set_with_data(), ignore it */
> + if (sel->owner == OWNER_CLIENT)
> + return;
> +
> + if (sel->owner == OWNER_GUEST) {
> + clipboard_new_owner(c, sel_id, OWNER_NONE);
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0, NULL, 0);
> + }
> +
> + if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER)
> + return;
> +
> + /* if there's a pending request for clipboard targets, cancel it */
> + if (sel->last_targets_req)
> + request_cancel(sel->last_targets_req);
> +
> + sel->last_targets_req = new_request_ref(c);
> + gtk_clipboard_request_targets(clipboard, clipboard_targets_received_cb,
> + sel->last_targets_req);
> +}
> +
> +static void clipboard_contents_received_cb(GtkClipboard *clipboard,
> + GtkSelectionData *sel_data,
> + gpointer user_data)
> +{
> + return_if_request_cancelled(user_data);
> + VDAgentClipboards *c = free_request_ref(user_data);
> + guint sel_id, type, target;
> +
> + sel_id = sel_id_from_clip(clipboard);
> + c->selections[sel_id].requests_from_client =
> + g_list_remove(c->selections[sel_id].requests_from_client, user_data);
> +
> + type = get_type_from_atom(gtk_selection_data_get_data_type(sel_data));
> + target = get_type_from_atom(gtk_selection_data_get_target(sel_data));
> +
> + if (type == target) {
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_DATA, sel_id, type,
> + gtk_selection_data_get_data(sel_data),
> + gtk_selection_data_get_length(sel_data));
> + } else {
> + syslog(LOG_WARNING, "%s: sel_id=%u: expected type %u, recieved %u, "
> + "skipping", __func__, sel_id, target, type);
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_DATA, sel_id,
> + VD_AGENT_CLIPBOARD_NONE, NULL, 0);
> + }
> +}
> +
> +static void clipboard_get_cb(GtkClipboard *clipboard,
> + GtkSelectionData *sel_data,
> + guint info,
> + gpointer user_data)
> +{
> + AppRequest req;
> + VDAgentClipboards *c = user_data;
> + guint sel_id, type;
> +
> + sel_id = sel_id_from_clip(clipboard);
> + g_return_if_fail(c->selections[sel_id].owner == OWNER_CLIENT);
> +
> + type = get_type_from_atom(gtk_selection_data_get_target(sel_data));
> + g_return_if_fail(type != VD_AGENT_CLIPBOARD_NONE);
> +
> + req.sel_data = sel_data;
> + req.loop = g_main_loop_new(NULL, FALSE);
> + c->selections[sel_id].requests_from_apps =
> + g_list_prepend(c->selections[sel_id].requests_from_apps, &req);
> +
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_REQUEST, sel_id, type, NULL, 0);
> +
> +G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> + gdk_threads_leave();
> + g_main_loop_run(req.loop);
> + gdk_threads_enter();
> +G_GNUC_END_IGNORE_DEPRECATIONS
> +
> + g_main_loop_unref(req.loop);
> +}
> +
> +static void clipboard_clear_cb(GtkClipboard *clipboard, gpointer user_data)
> +{
> + VDAgentClipboards *c = user_data;
> + clipboard_new_owner(c, sel_id_from_clip(clipboard), OWNER_NONE);
> +}
> +
> +void vdagent_clipboard_grab(VDAgentClipboards *c, guint sel_id,
> + guint32 *types, guint n_types)
> +{
> + GtkTargetEntry targets[G_N_ELEMENTS(atom2agent)];
> + guint n_targets, i, t;
> +
> + g_return_if_fail(sel_id < SELECTION_COUNT);
> +
> + n_targets = 0;
> + for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> + for (t = 0; t < n_types; t++)
> + if (atom2agent[i].type == types[t]) {
> + targets[n_targets].target = (gchar *)atom2agent[i].atom_name;
> + n_targets++;
> + break;
> + }
> +
> + if (n_targets == 0) {
> + syslog(LOG_WARNING, "%s: sel_id=%u: no type supported", __func__, sel_id);
> + return;
> + }
> +
> + if (gtk_clipboard_set_with_data(c->selections[sel_id].clipboard,
> + targets, n_targets,
> + clipboard_get_cb, clipboard_clear_cb, c))
> + clipboard_new_owner(c, sel_id, OWNER_CLIENT);
> + else {
> + syslog(LOG_ERR, "%s: sel_id=%u: clipboard grab failed", __func__, sel_id);
> + clipboard_new_owner(c, sel_id, OWNER_NONE);
> + }
> +}
> +
> +void vdagent_clipboard_data(VDAgentClipboards *c, guint sel_id,
> + guint type, const guchar *data, guint size)
> +{
> + g_return_if_fail(sel_id < SELECTION_COUNT);
> + Selection *sel = &c->selections[sel_id];
> + AppRequest *req;
> + GList *l;
> +
> + for (l = sel->requests_from_apps; l != NULL; l = l->next) {
> + req = l->data;
> + if (get_type_from_atom(gtk_selection_data_get_target(req->sel_data)) == type)
> + break;
> + }
> + if (l == NULL) {
> + syslog(LOG_WARNING, "%s: sel_id=%u: no corresponding request found for "
> + "type=%u, skipping", __func__, sel_id, type);
> + return;
> + }
> + sel->requests_from_apps = g_list_delete_link(sel->requests_from_apps, l);
> +
> + gtk_selection_data_set(req->sel_data,
> + gtk_selection_data_get_target(req->sel_data),
> + 8, data, size);
> +
> + g_main_loop_quit(req->loop);
> +}
> +
> +void vdagent_clipboard_release(VDAgentClipboards *c, guint sel_id)
> +{
> + g_return_if_fail(sel_id < SELECTION_COUNT);
> + if (c->selections[sel_id].owner != OWNER_CLIENT)
> + return;
> +
> + clipboard_new_owner(c, sel_id, OWNER_NONE);
> + gtk_clipboard_clear(c->selections[sel_id].clipboard);
> +}
> +
> +void vdagent_clipboards_release_all(VDAgentClipboards *c)
> +{
> + guint sel_id, owner;
> +
> + for (sel_id = 0; sel_id < SELECTION_COUNT; sel_id++) {
> + owner = c->selections[sel_id].owner;
> + clipboard_new_owner(c, sel_id, OWNER_NONE);
> + if (owner == OWNER_CLIENT)
> + gtk_clipboard_clear(c->selections[sel_id].clipboard);
> + else if (owner == OWNER_GUEST && c->conn)
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0, NULL, 0);
> + }
> +}
> +
> +void vdagent_clipboard_request(VDAgentClipboards *c, guint sel_id, guint type)
> +{
> + Selection *sel;
> +
> + if (sel_id >= SELECTION_COUNT)
> + goto err;
> + sel = &c->selections[sel_id];
> + if (sel->owner != OWNER_GUEST) {
> + syslog(LOG_WARNING, "%s: sel_id=%d: received request "
> + "while not owning clipboard", __func__, sel_id);
> + goto err;
> + }
> + if (type >= TYPE_COUNT || sel->targets[type] == GDK_NONE) {
> + syslog(LOG_WARNING, "%s: sel_id=%d: unadvertised data type requested",
> + __func__, sel_id);
> + goto err;
> + }
> +
> + gpointer *ref = new_request_ref(c);
> + sel->requests_from_client = g_list_prepend(sel->requests_from_client, ref);
> + gtk_clipboard_request_contents(sel->clipboard, sel->targets[type],
> + clipboard_contents_received_cb, ref);
> + return;
> +err:
> + udscs_write(c->conn, VDAGENTD_CLIPBOARD_DATA, sel_id,
> + VD_AGENT_CLIPBOARD_NONE, NULL, 0);
> +}
> +
> +VDAgentClipboards *vdagent_clipboards_init(struct udscs_connection *conn)
> +{
> + const GdkAtom sel_atom[SELECTION_COUNT] = {
> + GDK_SELECTION_CLIPBOARD, /* VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD */
> + GDK_SELECTION_PRIMARY, /* VD_AGENT_CLIPBOARD_SELECTION_PRIMARY */
> + };
> +
> + VDAgentClipboards *c;
> + c = g_new0(VDAgentClipboards, 1);
> + c->conn = conn;
> +
> + for (guint sel_id = 0; sel_id < SELECTION_COUNT; sel_id++) {
> + GtkClipboard *clipboard = gtk_clipboard_get(sel_atom[sel_id]);
> + c->selections[sel_id].clipboard = clipboard;
> + /* enables the use of sel_id_from_clipboard(clipboard) macro */
> + g_object_set_data(G_OBJECT(clipboard), "vdagent-selection-id",
> + GUINT_TO_POINTER(sel_id));
> + g_signal_connect(G_OBJECT(clipboard), "owner-change",
> + G_CALLBACK(clipboard_owner_change_cb), c);
> + }
> +
> + return c;
> +}
> +
> +void vdagent_clipboards_finalize(VDAgentClipboards *c,
> + struct udscs_connection *conn)
I don't follow the need of *conn here.
Would be nice to move 'handling' struct udscs_connection
somewhere else even more if we need to be sure that we have a
valid *conn.
I see that Marc-André come up with spice_vdagent_write() [0] for
that which simplify things a bit...
[0] https://github.com/elmarco/vdagent-gtk/blob/master/src/vdagent/vdagent-clipboard.c#L128
... but this just a minor comment, should be fine to refactor
this later on.
> +{
> + for (guint sel_id = 0; sel_id < SELECTION_COUNT; sel_id++)
> + g_signal_handlers_disconnect_by_func(c->selections[sel_id].clipboard,
> + G_CALLBACK(clipboard_owner_change_cb), c);
> +
> + c->conn = conn;
> + vdagent_clipboards_release_all(c);
> +
> + g_free(c);
> +}
> diff --git a/src/vdagent/clipboard.h b/src/vdagent/clipboard.h
> new file mode 100644
> index 0000000..54b8aef
> --- /dev/null
> +++ b/src/vdagent/clipboard.h
> @@ -0,0 +1,42 @@
> +/* clipboard.h - vdagent clipboard handling header
> +
> + Copyright 2017 Red Hat, Inc.
> +
> + 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 3 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/>.
> +*/
> +
> +#ifndef __VDAGENT_CLIPBOARD_H
> +#define __VDAGENT_CLIPBOARD_H
> +
> +#include "udscs.h"
> +
> +typedef struct VDAgentClipboards VDAgentClipboards;
> +
> +VDAgentClipboards *vdagent_clipboards_init(struct udscs_connection *conn);
> +void vdagent_clipboards_finalize(VDAgentClipboards *c,
> + struct udscs_connection *conn);
> +
> +void vdagent_clipboard_request(VDAgentClipboards *c, guint sel_id, guint type);
> +
> +void vdagent_clipboard_release(VDAgentClipboards *c, guint sel_id);
> +
> +void vdagent_clipboards_release_all(VDAgentClipboards *c);
> +
> +void vdagent_clipboard_data(VDAgentClipboards *c, guint sel_id,
> + guint type, const guchar *data, guint size);
> +
> +void vdagent_clipboard_grab(VDAgentClipboards *c, guint sel_id,
> + guint32 *types, guint n_types);
> +
> +#endif
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index d86ee25..7dd7da6 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -44,8 +44,14 @@
> #include "audio.h"
> #include "x11.h"
> #include "file-xfers.h"
> +#ifdef WITH_GTK_CLIPBOARD
> +# include "clipboard.h"
> +#endif
>
> typedef struct VDAgent {
> +#ifdef WITH_GTK_CLIPBOARD
> + VDAgentClipboards *clipboards;
> +#endif
> struct vdagent_x11 *x11;
> struct vdagent_file_xfers *xfers;
> struct udscs_connection *conn;
> @@ -154,6 +160,19 @@ static gboolean vdagent_finalize_file_xfer(VDAgent *agent)
> return TRUE;
> }
>
> +static void vdagent_quit_loop(VDAgent *agent)
> +{
> +#ifdef WITH_GTK_CLIPBOARD
> + /* other GMainLoop(s) might be running, quit them before agent->loop */
> + if (agent->clipboards) {
> + vdagent_clipboards_finalize(agent->clipboards, agent->conn);
> + agent->clipboards = NULL;
If agent->conn is not needed, maybe you can use
g_clear_pointer();
> + }
> +#endif
> + if (agent->loop)
> + g_main_loop_quit(agent->loop);
> +}
> +
> static void daemon_read_complete(struct udscs_connection **connp,
> struct udscs_message_header *header, uint8_t *data)
> {
> @@ -163,6 +182,22 @@ static void daemon_read_complete(struct udscs_connection **connp,
> case VDAGENTD_MONITORS_CONFIG:
> vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig *)data, 0);
> break;
> +#ifdef WITH_GTK_CLIPBOARD
> + case VDAGENTD_CLIPBOARD_REQUEST:
> + vdagent_clipboard_request(agent->clipboards, header->arg1, header->arg2);
> + break;
> + case VDAGENTD_CLIPBOARD_GRAB:
> + vdagent_clipboard_grab(agent->clipboards, header->arg1,
> + (guint32 *)data, header->size / sizeof(guint32));
> + break;
> + case VDAGENTD_CLIPBOARD_DATA:
> + vdagent_clipboard_data(agent->clipboards, header->arg1, header->arg2,
> + data, header->size);
> + break;
> + case VDAGENTD_CLIPBOARD_RELEASE:
> + vdagent_clipboard_release(agent->clipboards, header->arg1);
> + break;
> +#else
> case VDAGENTD_CLIPBOARD_REQUEST:
> vdagent_x11_clipboard_request(agent->x11, header->arg1, header->arg2);
> break;
> @@ -177,11 +212,12 @@ static void daemon_read_complete(struct udscs_connection **connp,
> case VDAGENTD_CLIPBOARD_RELEASE:
> vdagent_x11_clipboard_release(agent->x11, header->arg1);
> break;
> +#endif
> case VDAGENTD_VERSION:
> if (strcmp((char *)data, VERSION) != 0) {
> syslog(LOG_INFO, "vdagentd version mismatch: got %s expected %s",
> data, VERSION);
> - g_main_loop_quit(agent->loop);
> + vdagent_quit_loop(agent);
> version_mismatch = 1;
> }
> break;
> @@ -228,7 +264,11 @@ static void daemon_read_complete(struct udscs_connection **connp,
> }
> break;
> case VDAGENTD_CLIENT_DISCONNECTED:
> +#ifdef WITH_GTK_CLIPBOARD
> + vdagent_clipboards_release_all(agent->clipboards);
> +#else
> vdagent_x11_client_disconnected(agent->x11);
> +#endif
> if (vdagent_finalize_file_xfer(agent)) {
> vdagent_init_file_xfer(agent);
> }
> @@ -243,8 +283,7 @@ static void daemon_disconnect_cb(struct udscs_connection *conn)
> {
> VDAgent *agent = udscs_get_user_data(conn);
> agent->conn = NULL;
> - if (agent->loop)
> - g_main_loop_quit(agent->loop);
> + vdagent_quit_loop(agent);
> }
>
> /* When we daemonize, it is useful to have the main process
> @@ -316,7 +355,7 @@ gboolean vdagent_signal_handler(gpointer user_data)
> {
> VDAgent *agent = user_data;
> quit = TRUE;
> - g_main_loop_quit(agent->loop);
> + vdagent_quit_loop(agent);
> return G_SOURCE_REMOVE;
> }
>
> @@ -375,6 +414,10 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
> if (!vdagent_init_file_xfer(agent))
> syslog(LOG_WARNING, "File transfer is disabled");
>
> +#ifdef WITH_GTK_CLIPBOARD
> + agent->clipboards = vdagent_clipboards_init(agent->conn);
> +#endif
> +
> if (parent_socket != -1) {
> if (write(parent_socket, "OK", 2) != 2)
> syslog(LOG_WARNING, "Parent already gone.");
> @@ -385,7 +428,7 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
> return G_SOURCE_REMOVE;
>
> err_init:
> - g_main_loop_quit(agent->loop);
> + vdagent_quit_loop(agent);
> quit = TRUE;
> return G_SOURCE_REMOVE;
> }
> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
> index 3776098..48463ec 100644
> --- a/src/vdagent/x11-priv.h
> +++ b/src/vdagent/x11-priv.h
> @@ -1,6 +1,10 @@
> #ifndef VDAGENT_X11_PRIV
> #define VDAGENT_X11_PRIV
>
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> #include <stdint.h>
> #include <stdio.h>
>
> @@ -8,6 +12,7 @@
>
> #include <X11/extensions/Xrandr.h>
>
> +#ifndef WITH_GTK_CLIPBOARD
> /* Macros to print a message to the logfile prefixed by the selection */
> #define SELPRINTF(format, ...) \
> syslog(LOG_ERR, "%s: " format, \
> @@ -20,11 +25,13 @@
> vdagent_x11_sel_to_str(selection), ##__VA_ARGS__); \
> } \
> } while (0)
> +#endif
>
> #define MAX_SCREENS 16
> /* Same as qxl_dev.h client_monitors_config.heads count */
> #define MONITOR_SIZE_COUNT 64
>
> +#ifndef WITH_GTK_CLIPBOARD
> enum { owner_none, owner_guest, owner_client };
>
> /* X11 terminology is confusing a selection request is a request from an
> @@ -57,12 +64,14 @@ struct clipboard_format_info {
> Atom atoms[16];
> int atom_count;
> };
> +#endif
>
> struct monitor_size {
> int width;
> int height;
> };
>
> +#ifndef WITH_GTK_CLIPBOARD
> static const struct clipboard_format_tmpl clipboard_format_templates[] = {
> { VD_AGENT_CLIPBOARD_UTF8_TEXT, { "UTF8_STRING", "text/plain;charset=UTF-8",
> "text/plain;charset=utf-8", "STRING", NULL }, },
> @@ -74,27 +83,37 @@ static const struct clipboard_format_tmpl clipboard_format_templates[] = {
> };
>
> #define clipboard_format_count (sizeof(clipboard_format_templates)/sizeof(clipboard_format_templates[0]))
> +#endif
>
> struct vdagent_x11 {
> +#ifndef WITH_GTK_CLIPBOARD
> struct clipboard_format_info clipboard_formats[clipboard_format_count];
> +#endif
> Display *display;
> +#ifndef WITH_GTK_CLIPBOARD
> Atom clipboard_atom;
> Atom clipboard_primary_atom;
> Atom targets_atom;
> Atom incr_atom;
> Atom multiple_atom;
> Atom timestamp_atom;
> +#endif
> Window root_window[MAX_SCREENS];
> +#ifndef WITH_GTK_CLIPBOARD
> Window selection_window;
> +#endif
> struct udscs_connection *vdagentd;
> int debug;
> int fd;
> int screen_count;
> int width[MAX_SCREENS];
> int height[MAX_SCREENS];
> +#ifndef WITH_GTK_CLIPBOARD
> int has_xfixes;
> int xfixes_event_base;
> +#endif
> int xrandr_event_base;
> +#ifndef WITH_GTK_CLIPBOARD
> int max_prop_size;
> int expected_targets_notifies[256];
> int clipboard_owner[256];
> @@ -113,6 +132,7 @@ struct vdagent_x11 {
> uint32_t selection_req_data_pos;
> uint32_t selection_req_data_size;
> Atom selection_req_atom;
> +#endif
It should be fine to move thins around to make it a group of
#ifndef
> /* resolution change state */
> struct {
> XRRScreenResources *res;
> diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> index 9700847..f1a30e2 100644
> --- a/src/vdagent/x11.c
> +++ b/src/vdagent/x11.c
> @@ -31,6 +31,10 @@
> Calling XPending when-ever we return to the mainloop also ensures any
> pending writes are flushed. */
>
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> #include <glib.h>
> #include <gdk/gdk.h>
> #ifdef GDK_WINDOWING_X11
> @@ -53,6 +57,7 @@
> int (*vdagent_x11_prev_error_handler)(Display *, XErrorEvent *);
> int vdagent_x11_caught_error;
>
> +#ifndef WITH_GTK_CLIPBOARD
> static void vdagent_x11_handle_selection_notify(struct vdagent_x11 *x11,
> XEvent *event, int incr);
> static void vdagent_x11_handle_selection_request(struct vdagent_x11 *x11);
> @@ -77,6 +82,7 @@ static const char *vdagent_x11_sel_to_str(uint8_t selection) {
> return "unknown";
> }
> }
> +#endif
>
> static int vdagent_x11_debug_error_handler(
> Display *display, XErrorEvent *error)
> @@ -84,6 +90,7 @@ static int vdagent_x11_debug_error_handler(
> abort();
> }
>
> +#ifndef WITH_GTK_CLIPBOARD
> /* With the clipboard we're sometimes dealing with Properties on another apps
> Window. which can go away at any time. */
> static int vdagent_x11_ignore_bad_window_handler(
> @@ -94,6 +101,7 @@ static int vdagent_x11_ignore_bad_window_handler(
>
> return vdagent_x11_prev_error_handler(display, error);
> }
> +#endif
>
> void vdagent_x11_set_error_handler(struct vdagent_x11 *x11,
> int (*handler)(Display *, XErrorEvent *))
> @@ -131,7 +139,11 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
> {
> struct vdagent_x11 *x11;
> XWindowAttributes attrib;
> +#ifdef WITH_GTK_CLIPBOARD
> + int i;
> +#else
> int i, j, major, minor;
> +#endif
> const gchar *net_wm_name;
>
> x11 = calloc(1, sizeof(*x11));
> @@ -167,6 +179,7 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
> for (i = 0; i < x11->screen_count; i++)
> x11->root_window[i] = RootWindow(x11->display, i);
> x11->fd = ConnectionNumber(x11->display);
> +#ifndef WITH_GTK_CLIPBOARD
> x11->clipboard_atom = XInternAtom(x11->display, "CLIPBOARD", False);
> x11->clipboard_primary_atom = XInternAtom(x11->display, "PRIMARY", False);
> x11->targets_atom = XInternAtom(x11->display, "TARGETS", False);
> @@ -189,9 +202,11 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
> 0, 0, 1, 1, 0, 0, 0);
> if (x11->debug)
> syslog(LOG_DEBUG, "Selection window: %u", (int)x11->selection_window);
> +#endif
>
> vdagent_x11_randr_init(x11);
>
> +#ifndef WITH_GTK_CLIPBOARD
> if (XFixesQueryExtension(x11->display, &x11->xfixes_event_base, &i) &&
> XFixesQueryVersion(x11->display, &major, &minor) && major >= 1) {
> x11->has_xfixes = 1;
> @@ -217,6 +232,7 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
> /* Be a good X11 citizen and maximize the amount of data we send at once */
> if (x11->max_prop_size > 262144)
> x11->max_prop_size = 262144;
> +#endif
>
> for (i = 0; i < x11->screen_count; i++) {
> /* Catch resolution changes */
> @@ -249,17 +265,17 @@ struct vdagent_x11 *vdagent_x11_create(struct udscs_connection *vdagentd,
>
> void vdagent_x11_destroy(struct vdagent_x11 *x11, int vdagentd_disconnected)
> {
> - uint8_t sel;
> -
> if (!x11)
> return;
>
> if (vdagentd_disconnected)
> x11->vdagentd = NULL;
>
> - for (sel = 0; sel < VD_AGENT_CLIPBOARD_SELECTION_SECONDARY; ++sel) {
> +#ifndef WITH_GTK_CLIPBOARD
> + for (uint8_t sel = 0; sel < VD_AGENT_CLIPBOARD_SELECTION_SECONDARY; ++sel) {
IMO, in this case should be fine to declare uint8_t sel; just
below the ifndef
> vdagent_x11_set_clipboard_owner(x11, sel, owner_none);
> }
> +#endif
>
> XCloseDisplay(x11->display);
> free(x11->randr.failed_conf);
> @@ -271,6 +287,7 @@ int vdagent_x11_get_fd(struct vdagent_x11 *x11)
> return x11->fd;
> }
>
> +#ifndef WITH_GTK_CLIPBOARD
> static void vdagent_x11_next_selection_request(struct vdagent_x11 *x11)
> {
> struct vdagent_x11_selection_request *selection_request;
> @@ -406,10 +423,12 @@ static int vdagent_x11_get_clipboard_selection(struct vdagent_x11 *x11,
>
> return 0;
> }
> +#endif
>
> static void vdagent_x11_handle_event(struct vdagent_x11 *x11, XEvent event)
> {
> int i, handled = 0;
> +#ifndef WITH_GTK_CLIPBOARD
> uint8_t selection;
>
> if (event.type == x11->xfixes_event_base) {
> @@ -455,6 +474,7 @@ static void vdagent_x11_handle_event(struct vdagent_x11 *x11, XEvent event)
> x11->expected_targets_notifies[selection]++;
> return;
> }
> +#endif
>
> if (vdagent_x11_randr_handle_event(x11, event))
> return;
> @@ -475,6 +495,7 @@ static void vdagent_x11_handle_event(struct vdagent_x11 *x11, XEvent event)
> /* These are uninteresting */
> handled = 1;
> break;
> +#ifndef WITH_GTK_CLIPBOARD
> case SelectionNotify:
> if (event.xselection.target == x11->targets_atom)
> vdagent_x11_handle_targets_notify(x11, &event);
> @@ -534,6 +555,7 @@ static void vdagent_x11_handle_event(struct vdagent_x11 *x11, XEvent event)
> req->next = new_req;
> break;
> }
> +#endif
> }
> if (!handled && x11->debug)
> syslog(LOG_DEBUG, "unhandled x11 event, type %d, window %d",
> @@ -550,6 +572,7 @@ void vdagent_x11_do_read(struct vdagent_x11 *x11)
> }
> }
>
> +#ifndef WITH_GTK_CLIPBOARD
> static const char *vdagent_x11_get_atom_name(struct vdagent_x11 *x11, Atom a)
> {
> if (a == None)
> @@ -1284,6 +1307,7 @@ void vdagent_x11_client_disconnected(struct vdagent_x11 *x11)
> vdagent_x11_clipboard_release(x11, sel);
> }
> }
> +#endif
>
> /* Function used to determine the default location to save file-xfers,
> xdg desktop dir or xdg download dir. We err on the safe side and use a
> diff --git a/src/vdagent/x11.h b/src/vdagent/x11.h
> index a8ceb08..c001fac 100644
> --- a/src/vdagent/x11.h
> +++ b/src/vdagent/x11.h
> @@ -22,6 +22,10 @@
> #ifndef __VDAGENT_X11_H
> #define __VDAGENT_X11_H
>
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +
> #include <stdio.h>
> #include <spice/vd_agent.h>
> #include "udscs.h"
> @@ -38,6 +42,7 @@ void vdagent_x11_do_read(struct vdagent_x11 *x11);
> void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
> VDAgentMonitorsConfig *mon_config, int fallback);
>
> +#ifndef WITH_GTK_CLIPBOARD
> void vdagent_x11_clipboard_grab(struct vdagent_x11 *x11, uint8_t selection,
> uint32_t *types, uint32_t type_count);
> void vdagent_x11_clipboard_request(struct vdagent_x11 *x11,
> @@ -47,6 +52,7 @@ void vdagent_x11_clipboard_data(struct vdagent_x11 *x11, uint8_t selection,
> void vdagent_x11_clipboard_release(struct vdagent_x11 *x11, uint8_t selection);
>
> void vdagent_x11_client_disconnected(struct vdagent_x11 *x11);
> +#endif
Another way is to have x11-clipboard.[ch] and not build this code
there but I don't think you should bother with that.
Didn't test this time, just build + review.
Cheers,
toso
Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel