Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Victor Toso]

Hi,

On Sun, Feb 18, 2018 at 11:44:38PM +0100, Jakub Janku wrote:
> >> +typedef struct {
> >> +GMainLoop*loop;
> >> +GtkSelectionData *sel_data;
> >> +} DataRetrieval;
> >> +
> >> +struct VDAgentClipboards {
> >> +struct udscs_connection*conn;
> >> +
> >> +GtkClipboard   *clipboard[SELECTION_COUNT];
> >> +guint   owner[SELECTION_COUNT];
> >> +
> >> +GList  *data_retrievals[SELECTION_COUNT];
> >> +GList  *data_requests[SELECTION_COUNT];
> >
> > I think we could use something more clear instead of
> > 'data_retrievals' and 'data_requests' but I'm terrible with
> > names myself so I suggest 'pending_set_request' and
> > 'pending_get_requests' or just 'set_requests' and
> > 'get_requests'.
> 
> I agree, the names are far from ideal and rather confusing, but
> 'set_requests' and 'get_requests' aren't making it more clear
> either imho - "get request" could stand for a request from the
> client as well as a request from an app on the vdagent side.
> What about 'request_from_client' and 'request_from_app'?

Works for me :)

> > A comment would help too, like /* Client -> Guest */ and vice
> > versa.
> 
> Definitely, I should have added some comments.
> >
> >> +gpointer   *last_targets_req[SELECTION_COUNT];
> >> +
> >> +GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];
> >
> > Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?
> >
> > e.g.
> > - c->data_requests[sel] = ...
> > + c->selection[id].data_request = ...
> >
> Makes sense probably. It might also be handy to typedef something like
> DataSelection and use
>   DataSelection *sel = >selection[sel_id];
>   sel->data_requests = ...
> where the struct members are accessed frequently.

It was more like a possible suggestion then a request but I like
it as we would access the `selection[]` array only once to get
the right struct instead of accessing the array for each field.

> >> +};
> >> +
> >> +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)
> >> +{
> >> +int i;
> >> +gchar *name = gdk_atom_name(atom);
> >> +for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> >> +if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
> >> +break;
> >> +g_free(name);
> >> +return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
> >> +: VD_AGENT_CLIPBOARD_NONE;
> >> +}
> >> +
> >> +/* gtk_clipboard_request_(, callback, user_data) cannot be cancelled. 
> >> Instead,
> >> +   gpointer *ref = get_weak_ref(VDAgentClipboards *) is passed to the 
> >> callback.
> >> +   The callback returns if free_weak_ref(ref) == NULL, which can be done
> >> +   by setting *ref = NULL. This substitutes cancellation of the request.
> >> + */
> >> +static gpointer *get_weak_ref(gpointer data)
> >> +{
> >> +gpointer *ref = g_new(gpointer, 1);
> >> +*ref = data;
> >> +return ref;
> >> +}
> >
> > So, this is similar to GWeakRef but we are not using a GObject at
> > all.
> >
> > I might be missing something but why not an actual structure for
> > the pending request? I think it would be easier to understand and
> > track what is going.
> >
> > At least we should have some functions with the clear meaning
> > around this, like weak_ref_cancel(gpointer ref) { /* check and
> > set to NULL */ } ... but I do think something around GCancellable
> > would be nicer or even bool is_cancelled; in a custom struct.
> >
> The get_weak_ref() is only used with callbacks since we need to pass a
> reference to VDAgentClipboards. So I'm not sure GCancellable would
> help us here.
> The struct would need to contain pointer to VDAgentClipboards as well
> as the bool is_cancelled, so using something like GWeakRef seemed to
> be the easiest solution, although creating a struct might be nicer...

Right, maybe some helper functions would be enough. For instance:

 |  /* if there's a pending request for clipboard targets, cancel it */
 |  if (c->last_targets_req[sel])
 |  *(c->last_targets_req[sel]) = NULL;

Should use some 

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Jakub Janku]

Hi Victor,

sorry for the delayed reply.

On Mon, Feb 12, 2018 at 2:11 PM, Victor Toso  wrote:
> Hi,
>
> On Sun, Jan 21, 2018 at 09:03:14PM +0100, Jakub Janků wrote:
>> From: Jakub Janků 
>>
>> Place the code that handles clipboard
>> into a separate file - clipboard.c
>> ---
>>  Makefile.am |   2 +
>>  src/vdagent/clipboard.c | 401 
>> 
>>  src/vdagent/clipboard.h |  42 +
>>  src/vdagent/vdagent.c   |  31 +++-
>>  4 files changed, 471 insertions(+), 5 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..88550c6 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>>   $(common_sources)   \
>>   src/vdagent/audio.c \
>>   src/vdagent/audio.h \
>> + src/vdagent/clipboard.c \
>> + src/vdagent/clipboard.h \
>>   src/vdagent/file-xfers.c\
>>   src/vdagent/file-xfers.h\
>>   src/vdagent/x11-priv.h  \
>> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
>> new file mode 100644
>> index 000..657a6b4
>> --- /dev/null
>> +++ b/src/vdagent/clipboard.c
>> @@ -0,0 +1,401 @@
>> +/*  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 .
>> +*/
>> +
>> +#ifdef HAVE_CONFIG_H
>> +# include 
>> +#endif
>> +
>> +#include 
>> +#include 
>> +
>> +#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_from_clip(clipboard) \
>> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
>> "vdagent-selection"))
>> +
>> +enum {
>> +OWNER_NONE,
>> +OWNER_GUEST,
>> +OWNER_CLIENT
>> +};
>> +
>> +typedef struct {
>> +GMainLoop*loop;
>> +GtkSelectionData *sel_data;
>> +} DataRetrieval;
>> +
>> +struct VDAgentClipboards {
>> +struct udscs_connection*conn;
>> +
>> +GtkClipboard   *clipboard[SELECTION_COUNT];
>> +guint   owner[SELECTION_COUNT];
>> +
>> +GList  *data_retrievals[SELECTION_COUNT];
>> +GList  *data_requests[SELECTION_COUNT];
>
> I think we could use something more clear instead of
> 'data_retrievals' and 'data_requests' but I'm terrible with
> names myself so I suggest 'pending_set_request' and
> 'pending_get_requests' or just 'set_requests' and 'get_requests'.

I agree, the names are far from ideal and rather confusing, but
'set_requests' and 'get_requests' aren't making it more clear either
imho - "get request" could stand for a request from the client as well
as a request from an app on the vdagent side. What about
'request_from_client' and 'request_from_app'?
>
> A comment would help too, like /* Client -> Guest */ and vice
> versa.

Definitely, I should have added some comments.
>
>> +gpointer   *last_targets_req[SELECTION_COUNT];
>> +
>> +GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];
>
> Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?
>
> e.g.
> - c->data_requests[sel] = ...
> + c->selection[id].data_request = ...
>
Makes sense probably. It might also be handy to typedef something like
DataSelection and use
  DataSelection *sel = >selection[sel_id];
  sel->data_requests = ...
where the struct members are accessed frequently.

>> +};
>> +
>> +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"},
>> +

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Victor Toso]

Hi,

On Sun, Jan 21, 2018 at 09:03:14PM +0100, Jakub Janků wrote:
> From: Jakub Janků 
> 
> Place the code that handles clipboard
> into a separate file - clipboard.c
> ---
>  Makefile.am |   2 +
>  src/vdagent/clipboard.c | 401 
> 
>  src/vdagent/clipboard.h |  42 +
>  src/vdagent/vdagent.c   |  31 +++-
>  4 files changed, 471 insertions(+), 5 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..88550c6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>   $(common_sources)   \
>   src/vdagent/audio.c \
>   src/vdagent/audio.h \
> + src/vdagent/clipboard.c \
> + src/vdagent/clipboard.h \
>   src/vdagent/file-xfers.c\
>   src/vdagent/file-xfers.h\
>   src/vdagent/x11-priv.h  \
> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> new file mode 100644
> index 000..657a6b4
> --- /dev/null
> +++ b/src/vdagent/clipboard.c
> @@ -0,0 +1,401 @@
> +/*  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 .
> +*/
> +
> +#ifdef HAVE_CONFIG_H
> +# include 
> +#endif
> +
> +#include 
> +#include 
> +
> +#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_from_clip(clipboard) \
> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
> "vdagent-selection"))
> +
> +enum {
> +OWNER_NONE,
> +OWNER_GUEST,
> +OWNER_CLIENT
> +};
> +
> +typedef struct {
> +GMainLoop*loop;
> +GtkSelectionData *sel_data;
> +} DataRetrieval;
> +
> +struct VDAgentClipboards {
> +struct udscs_connection*conn;
> +
> +GtkClipboard   *clipboard[SELECTION_COUNT];
> +guint   owner[SELECTION_COUNT];
> +
> +GList  *data_retrievals[SELECTION_COUNT];
> +GList  *data_requests[SELECTION_COUNT];

I think we could use something more clear instead of
'data_retrievals' and 'data_requests' but I'm terrible with
names myself so I suggest 'pending_set_request' and
'pending_get_requests' or just 'set_requests' and 'get_requests'.

A comment would help too, like /* Client -> Guest */ and vice
versa.

> +gpointer   *last_targets_req[SELECTION_COUNT];
> +
> +GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];

Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?

e.g.
- c->data_requests[sel] = ...
+ c->selection[id].data_request = ...

> +};
> +
> +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)
> +{
> +int i;
> +gchar *name = gdk_atom_name(atom);
> +for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> +if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
> +break;
> +g_free(name);
> +return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
> +: VD_AGENT_CLIPBOARD_NONE;
> +}
> +
> +/* gtk_clipboard_request_(, 

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Christophe de Dinechin]

> On 7 Feb 2018, at 19:39, Jakub Janků  wrote:
> 
> Hi Christophe,
> 
> On Wed, Feb 7, 2018 at 5:12 PM, Christophe de Dinechin
>  wrote:
>> Hi Jakub,
>> 
>> 
>> I have not looked at everything, but here are a few quick notes just from 
>> glancing…
>> 
>>> On 21 Jan 2018, at 21:03, Jakub Janků  wrote:
>>> 
>>> From: Jakub Janků 
>>> 
>>> Place the code that handles clipboard
>>> into a separate file - clipboard.c
>>> ---
>>> Makefile.am |   2 +
>>> src/vdagent/clipboard.c | 401 
>>> 
>>> src/vdagent/clipboard.h |  42 +
>>> src/vdagent/vdagent.c   |  31 +++-
>>> 4 files changed, 471 insertions(+), 5 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..88550c6 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>>>  $(common_sources)   \
>>>  src/vdagent/audio.c \
>>>  src/vdagent/audio.h \
>>> + src/vdagent/clipboard.c \
>>> + src/vdagent/clipboard.h \
>>>  src/vdagent/file-xfers.c\
>>>  src/vdagent/file-xfers.h\
>>>  src/vdagent/x11-priv.h  \
>>> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
>>> new file mode 100644
>>> index 000..657a6b4
>>> --- /dev/null
>>> +++ b/src/vdagent/clipboard.c
>>> @@ -0,0 +1,401 @@
>>> +/*  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 .
>>> +*/
>>> +
>>> +#ifdef HAVE_CONFIG_H
>>> +# include 
>>> +#endif
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +#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_from_clip(clipboard) \
>>> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
>>> "vdagent-selection"))
>>> +
>>> +enum {
>>> +OWNER_NONE,
>>> +OWNER_GUEST,
>>> +OWNER_CLIENT
>>> +};
>>> +
>>> +typedef struct {
>>> +GMainLoop*loop;
>>> +GtkSelectionData *sel_data;
>>> +} DataRetrieval;
>>> +
>>> +struct VDAgentClipboards {
>> 
>> That looks C++-y. Any reason to not use a typedef struct here? Does this 
>> compile in strict C mode?
>> 
> 
> The typedef is included in clipboard.h since VDAgentClipboards is also
> used in vdagent.c
> 
>>> +struct udscs_connection*conn;
>>> +
>>> +GtkClipboard   *clipboard[SELECTION_COUNT];
>>> +guint   owner[SELECTION_COUNT];
>>> +
>>> +GList  *data_retrievals[SELECTION_COUNT];
>>> +GList  *data_requests[SELECTION_COUNT];
>>> +gpointer   *last_targets_req[SELECTION_COUNT];
>>> +
>>> +GdkAtom targets[SELECTION_COUNT][TYPE_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)
>>> +{
>>> +int i;
>>> +gchar *name = gdk_atom_name(atom);
>>> +for (i = 

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Jakub Janků]

Hi Christophe,

On Wed, Feb 7, 2018 at 5:12 PM, Christophe de Dinechin
 wrote:
> Hi Jakub,
>
>
> I have not looked at everything, but here are a few quick notes just from 
> glancing…
>
>> On 21 Jan 2018, at 21:03, Jakub Janků  wrote:
>>
>> From: Jakub Janků 
>>
>> Place the code that handles clipboard
>> into a separate file - clipboard.c
>> ---
>> Makefile.am |   2 +
>> src/vdagent/clipboard.c | 401 
>> 
>> src/vdagent/clipboard.h |  42 +
>> src/vdagent/vdagent.c   |  31 +++-
>> 4 files changed, 471 insertions(+), 5 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..88550c6 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>>   $(common_sources)   \
>>   src/vdagent/audio.c \
>>   src/vdagent/audio.h \
>> + src/vdagent/clipboard.c \
>> + src/vdagent/clipboard.h \
>>   src/vdagent/file-xfers.c\
>>   src/vdagent/file-xfers.h\
>>   src/vdagent/x11-priv.h  \
>> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
>> new file mode 100644
>> index 000..657a6b4
>> --- /dev/null
>> +++ b/src/vdagent/clipboard.c
>> @@ -0,0 +1,401 @@
>> +/*  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 .
>> +*/
>> +
>> +#ifdef HAVE_CONFIG_H
>> +# include 
>> +#endif
>> +
>> +#include 
>> +#include 
>> +
>> +#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_from_clip(clipboard) \
>> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
>> "vdagent-selection"))
>> +
>> +enum {
>> +OWNER_NONE,
>> +OWNER_GUEST,
>> +OWNER_CLIENT
>> +};
>> +
>> +typedef struct {
>> +GMainLoop*loop;
>> +GtkSelectionData *sel_data;
>> +} DataRetrieval;
>> +
>> +struct VDAgentClipboards {
>
> That looks C++-y. Any reason to not use a typedef struct here? Does this 
> compile in strict C mode?
>

The typedef is included in clipboard.h since VDAgentClipboards is also
used in vdagent.c

>> +struct udscs_connection*conn;
>> +
>> +GtkClipboard   *clipboard[SELECTION_COUNT];
>> +guint   owner[SELECTION_COUNT];
>> +
>> +GList  *data_retrievals[SELECTION_COUNT];
>> +GList  *data_requests[SELECTION_COUNT];
>> +gpointer   *last_targets_req[SELECTION_COUNT];
>> +
>> +GdkAtom targets[SELECTION_COUNT][TYPE_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)
>> +{
>> +int i;
>> +gchar *name = gdk_atom_name(atom);
>> +for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
>> +if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
>> +break;
>> +g_free(name);
>> +return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
>> +

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Christophe de Dinechin]

Hi Jakub,


I have not looked at everything, but here are a few quick notes just from 
glancing…

> On 21 Jan 2018, at 21:03, Jakub Janků  wrote:
> 
> From: Jakub Janků 
> 
> Place the code that handles clipboard
> into a separate file - clipboard.c
> ---
> Makefile.am |   2 +
> src/vdagent/clipboard.c | 401 
> src/vdagent/clipboard.h |  42 +
> src/vdagent/vdagent.c   |  31 +++-
> 4 files changed, 471 insertions(+), 5 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..88550c6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>   $(common_sources)   \
>   src/vdagent/audio.c \
>   src/vdagent/audio.h \
> + src/vdagent/clipboard.c \
> + src/vdagent/clipboard.h \
>   src/vdagent/file-xfers.c\
>   src/vdagent/file-xfers.h\
>   src/vdagent/x11-priv.h  \
> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> new file mode 100644
> index 000..657a6b4
> --- /dev/null
> +++ b/src/vdagent/clipboard.c
> @@ -0,0 +1,401 @@
> +/*  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 .
> +*/
> +
> +#ifdef HAVE_CONFIG_H
> +# include 
> +#endif
> +
> +#include 
> +#include 
> +
> +#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_from_clip(clipboard) \
> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
> "vdagent-selection"))
> +
> +enum {
> +OWNER_NONE,
> +OWNER_GUEST,
> +OWNER_CLIENT
> +};
> +
> +typedef struct {
> +GMainLoop*loop;
> +GtkSelectionData *sel_data;
> +} DataRetrieval;
> +
> +struct VDAgentClipboards {

That looks C++-y. Any reason to not use a typedef struct here? Does this 
compile in strict C mode?

> +struct udscs_connection*conn;
> +
> +GtkClipboard   *clipboard[SELECTION_COUNT];
> +guint   owner[SELECTION_COUNT];
> +
> +GList  *data_retrievals[SELECTION_COUNT];
> +GList  *data_requests[SELECTION_COUNT];
> +gpointer   *last_targets_req[SELECTION_COUNT];
> +
> +GdkAtom targets[SELECTION_COUNT][TYPE_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)
> +{
> +int i;
> +gchar *name = gdk_atom_name(atom);
> +for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> +if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
> +break;
> +g_free(name);
> +return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
> +: VD_AGENT_CLIPBOARD_NONE;
> +}

Maybe a minor style point, but I’d find it more readable with the test not 
repeated:

static guint get_type_from_atom(GdkAtom atom)
{
int i;
gchar *name = gdk_atom_name(atom);
for (i = 0; i < G_N_ELEMENTS(atom2agent); i++) {
if (!g_ascii_strcasecmp(name,