Re: [Spice-devel] [PATCH vdagent 1/2] vdagent-x11: remove clipboard handling

[Jakub Janku]

Hi,

On Mon, Feb 12, 2018 at 10:40 AM, Victor Toso  wrote:
> Hi,
>
> Follow up after the comment in the cover letter.
>
> On Sun, Jan 21, 2018 at 09:03:13PM +0100, Jakub Janků wrote:
>> From: Jakub Janků 
>>
>> The code will be replaced by GTK+ in the following patch.
>> ---
>>  src/vdagent/vdagent.c  |7 -
>>  src/vdagent/x11-priv.h |   91 
>>  src/vdagent/x11.c  | 1074 
>> +---
>>  src/vdagent/x11.h  |   10 -
>>  4 files changed, 1 insertion(+), 1181 deletions(-)
>>
>> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
>> index d86ee25..92ffcf3 100644
>> --- a/src/vdagent/vdagent.c
>> +++ b/src/vdagent/vdagent.c
>> @@ -164,18 +164,12 @@ static void daemon_read_complete(struct 
>> udscs_connection **connp,
>>  vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig 
>> *)data, 0);
>>  break;
>>  case VDAGENTD_CLIPBOARD_REQUEST:
>> -vdagent_x11_clipboard_request(agent->x11, header->arg1, 
>> header->arg2);
>>  break;
>>  case VDAGENTD_CLIPBOARD_GRAB:
>> -vdagent_x11_clipboard_grab(agent->x11, header->arg1, (uint32_t 
>> *)data,
>> -   header->size / sizeof(uint32_t));
>>  break;
>>  case VDAGENTD_CLIPBOARD_DATA:
>> -vdagent_x11_clipboard_data(agent->x11, header->arg1, header->arg2,
>> -   data, header->size);
>>  break;
>>  case VDAGENTD_CLIPBOARD_RELEASE:
>> -vdagent_x11_clipboard_release(agent->x11, header->arg1);
>>  break;
>>  case VDAGENTD_VERSION:
>>  if (strcmp((char *)data, VERSION) != 0) {
>> @@ -228,7 +222,6 @@ static void daemon_read_complete(struct udscs_connection 
>> **connp,
>>  }
>>  break;
>>  case VDAGENTD_CLIENT_DISCONNECTED:
>> -vdagent_x11_client_disconnected(agent->x11);
>
> So, instead of removing the vdagent_x11 calls, you could
> introduce the clipboard.[ch] and use the renamed calls here that
> will call the x11 code for now.

Seems reasonable (including the proposal in cover letter comment),
I'll change it with v2.

  Jakub
>
> Reviewed-by: Victor Toso 
>
> toso
>
>>  if (vdagent_finalize_file_xfer(agent)) {
>>  vdagent_init_file_xfer(agent);
>>  }
>> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
>> index 3776098..627e0ed 100644
>> --- a/src/vdagent/x11-priv.h
>> +++ b/src/vdagent/x11-priv.h
>> @@ -1,118 +1,27 @@
>>  #ifndef VDAGENT_X11_PRIV
>>  #define VDAGENT_X11_PRIV
>>
>> -#include 
>> -#include 
>> -
>> -#include 
>> -
>>  #include 
>>
>> -/* Macros to print a message to the logfile prefixed by the selection */
>> -#define SELPRINTF(format, ...) \
>> -syslog(LOG_ERR, "%s: " format, \
>> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__)
>> -
>> -#define VSELPRINTF(format, ...) \
>> -do { \
>> -if (x11->debug) { \
>> -syslog(LOG_DEBUG, "%s: " format, \
>> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__); \
>> -} \
>> -} while (0)
>> -
>>  #define MAX_SCREENS 16
>>  /* Same as qxl_dev.h client_monitors_config.heads count */
>>  #define MONITOR_SIZE_COUNT 64
>>
>> -enum { owner_none, owner_guest, owner_client };
>> -
>> -/* X11 terminology is confusing a selection request is a request from an
>> -   app to get clipboard data from us, so iow from the spice client through
>> -   the vdagent channel. We handle these one at a time and queue any which
>> -   come in while we are still handling the current one. */
>> -struct vdagent_x11_selection_request {
>> -XEvent event;
>> -uint8_t selection;
>> -struct vdagent_x11_selection_request *next;
>> -};
>> -
>> -/* A conversion request is X11 speak for asking another app to give its
>> -   clipboard data to us, we do these on behalf of the spice client to copy
>> -   data from the guest to the client. Like selection requests we process
>> -   these one at a time. */
>> -struct vdagent_x11_conversion_request {
>> -Atom target;
>> -uint8_t selection;
>> -struct vdagent_x11_conversion_request *next;
>> -};
>> -
>> -struct clipboard_format_tmpl {
>> -uint32_t type;
>> -const char *atom_names[16];
>> -};
>> -
>> -struct clipboard_format_info {
>> -uint32_t type;
>> -Atom atoms[16];
>> -int atom_count;
>> -};
>> -
>>  struct monitor_size {
>>  int width;
>>  int height;
>>  };
>>
>> -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 }, },
>> -{ VD_AGENT_CLIPBOARD_IMAGE_PNG, { "image/png", NULL }, },
>> -{ VD_AGENT_CLIPBOARD_IMAGE_BMP, { "image/bmp", "image/x-bmp",
>> -  "image/x-MS-bmp", "image/x-win-bitmap", NULL }, },
>> -{ 

Re: [Spice-devel] [PATCH vdagent 1/2] vdagent-x11: remove clipboard handling

[Victor Toso]

Hi,

Follow up after the comment in the cover letter.

On Sun, Jan 21, 2018 at 09:03:13PM +0100, Jakub Janků wrote:
> From: Jakub Janků 
> 
> The code will be replaced by GTK+ in the following patch.
> ---
>  src/vdagent/vdagent.c  |7 -
>  src/vdagent/x11-priv.h |   91 
>  src/vdagent/x11.c  | 1074 
> +---
>  src/vdagent/x11.h  |   10 -
>  4 files changed, 1 insertion(+), 1181 deletions(-)
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index d86ee25..92ffcf3 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -164,18 +164,12 @@ static void daemon_read_complete(struct 
> udscs_connection **connp,
>  vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig 
> *)data, 0);
>  break;
>  case VDAGENTD_CLIPBOARD_REQUEST:
> -vdagent_x11_clipboard_request(agent->x11, header->arg1, 
> header->arg2);
>  break;
>  case VDAGENTD_CLIPBOARD_GRAB:
> -vdagent_x11_clipboard_grab(agent->x11, header->arg1, (uint32_t 
> *)data,
> -   header->size / sizeof(uint32_t));
>  break;
>  case VDAGENTD_CLIPBOARD_DATA:
> -vdagent_x11_clipboard_data(agent->x11, header->arg1, header->arg2,
> -   data, header->size);
>  break;
>  case VDAGENTD_CLIPBOARD_RELEASE:
> -vdagent_x11_clipboard_release(agent->x11, header->arg1);
>  break;
>  case VDAGENTD_VERSION:
>  if (strcmp((char *)data, VERSION) != 0) {
> @@ -228,7 +222,6 @@ static void daemon_read_complete(struct udscs_connection 
> **connp,
>  }
>  break;
>  case VDAGENTD_CLIENT_DISCONNECTED:
> -vdagent_x11_client_disconnected(agent->x11);

So, instead of removing the vdagent_x11 calls, you could
introduce the clipboard.[ch] and use the renamed calls here that
will call the x11 code for now.

Reviewed-by: Victor Toso 

toso

>  if (vdagent_finalize_file_xfer(agent)) {
>  vdagent_init_file_xfer(agent);
>  }
> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
> index 3776098..627e0ed 100644
> --- a/src/vdagent/x11-priv.h
> +++ b/src/vdagent/x11-priv.h
> @@ -1,118 +1,27 @@
>  #ifndef VDAGENT_X11_PRIV
>  #define VDAGENT_X11_PRIV
>  
> -#include 
> -#include 
> -
> -#include 
> -
>  #include 
>  
> -/* Macros to print a message to the logfile prefixed by the selection */
> -#define SELPRINTF(format, ...) \
> -syslog(LOG_ERR, "%s: " format, \
> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__)
> -
> -#define VSELPRINTF(format, ...) \
> -do { \
> -if (x11->debug) { \
> -syslog(LOG_DEBUG, "%s: " format, \
> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__); \
> -} \
> -} while (0)
> -
>  #define MAX_SCREENS 16
>  /* Same as qxl_dev.h client_monitors_config.heads count */
>  #define MONITOR_SIZE_COUNT 64
>  
> -enum { owner_none, owner_guest, owner_client };
> -
> -/* X11 terminology is confusing a selection request is a request from an
> -   app to get clipboard data from us, so iow from the spice client through
> -   the vdagent channel. We handle these one at a time and queue any which
> -   come in while we are still handling the current one. */
> -struct vdagent_x11_selection_request {
> -XEvent event;
> -uint8_t selection;
> -struct vdagent_x11_selection_request *next;
> -};
> -
> -/* A conversion request is X11 speak for asking another app to give its
> -   clipboard data to us, we do these on behalf of the spice client to copy
> -   data from the guest to the client. Like selection requests we process
> -   these one at a time. */
> -struct vdagent_x11_conversion_request {
> -Atom target;
> -uint8_t selection;
> -struct vdagent_x11_conversion_request *next;
> -};
> -
> -struct clipboard_format_tmpl {
> -uint32_t type;
> -const char *atom_names[16];
> -};
> -
> -struct clipboard_format_info {
> -uint32_t type;
> -Atom atoms[16];
> -int atom_count;
> -};
> -
>  struct monitor_size {
>  int width;
>  int height;
>  };
>  
> -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 }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_PNG, { "image/png", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_BMP, { "image/bmp", "image/x-bmp",
> -  "image/x-MS-bmp", "image/x-win-bitmap", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_TIFF, { "image/tiff", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_JPG, { "image/jpeg", NULL }, },
> -};
> -
> -#define clipboard_format_count 
> (sizeof(clipboard_format_templates)/sizeof(clipboard_format_templates[0]))
> -
>  struct vdagent_x11 {
> -struct clipboard_format_info