Re: [PATCH vdagent v2 1/1] vdagent: add GTK+ clipboard handling

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

 



Hi Victor,

On Tue, Feb 27, 2018 at 9:02 AM, Victor Toso <victortoso@xxxxxxxxxx> wrote:
> 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?

The vdagent_x11_*() calls require struct *vdagent_x11 as argument, so
we would need to store it in the VDAgentClipboards struct.
It seemed more clear to me to have clipboard.* files with gtk code
only and don't bother with x11.h and WITH_GTK_CLIPBOARD here.
But I'm completely fine with your approach too.
>
> 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.
>
OK. But the behavior should stay the same - auto = use Xlib, right?

>> +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.

Sure, this occurs at multiple places in clipboard.c, I'll fix it.
>
>> +        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()
>
OK
>> +
>> +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.

It's similar to
void vdagent_x11_destroy(struct vdagent_x11 *x11, int vdagentd_disconnected);
If the connection is destroyed, daemon_disconnect_cb() in vdagent.c is
called. Subsequently, vdagent_clipboards_finalize() is called too.
Maybe passing boolean like vdagentd_disconnected would make it more clear?
>
> 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.

Agreed.
>
> 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.
>
This doesn't really affect only clipboard.c. vdagent_x11 and
vdagent_file_xfers structs also keep a udscs_connection reference. I
would postpone the refactor for now.

>> +{
>> +    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();
>
Not sure what you mean here. agent->conn is set to NULL in
daemon_disconnect_cb(), if the connection was destroyed.
>> +    }
>> +#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.
>
Since the Xlib clipboard code will be removed (hopefully) soon anyway,
I would prefer not to manipulate with it much.

> Didn't test this time, just build + review.
>
> Cheers,
>         toso
>
> Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
>

Cheers,
Jakub
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]