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

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

 



Hi,

On Tue, Feb 27, 2018 at 10:22:49AM +0100, Jakub Janku wrote:
> Hi Victor,
> 
> On Tue, Feb 27, 2018 at 9:02 AM, Victor Toso <victortoso@xxxxxxxxxx> wrote:
> > 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.

Yes

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

IMHO, clipboard.[ch] should handle clipboard code as much as
possible; considering that this change is not too big, I think it
is worth the effort.

e.g: if we add wayland/windows backend besides x11/gtk, the code
to be changed would be clipboard.[ch] instead of vdagent.c. Of
course, I really hope that gtk would be enough for x11/wayland
and even windows if we get this to be mingw buildable.

> 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?

Probably auto would set to --with-gtk-clipboard to yes as the
only dependency is gtk which we already depend on... but in case
of issues, one could switch off, at least in the next release.

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

> >> +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); 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?

Ah, right. Yes, a boolean might make it clear on why you are
changing the pointer.

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

Yep

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

if agent->conn was not needed, you could perhaps:
    g_clear_pointer(&agent->clipboards, vdagent_clipboards_finalize);

But I understand that it is needed so you can ignore my comment
;)

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

Yes. Nobody has manifested in regard to keeping it so far so I
too think it should be fine to deprecated it in this release and
likely remove it short after.

        toso

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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
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]