[Spice-devel] [PATCH] vd_agent: support clipboard/selection-owner model

[Hans de Goede]

Hi,

See comments inline.

On 09/21/2010 07:16 PM, Arnon Gilboa wrote:
> -enable the clipboard support
> -support the GRAB/REQUEST/DATA/RELEASE verbs in both ways.
> -pasting clipboard data is now "only-by-demand" from both sides (client and agent), whose behavior is symmetric.
> -client and agent don't read or send the contents of the clipboard unnecessarily (e.g. copy, internal paste, repeating paste, focus change)
> -bonus (no cost): support image cut&  paste, currently only with win client
> ---
>   vdagent/vdagent.cpp |  323 ++++++++++++++++++++++++++++++++++++++++-----------
>   1 files changed, 253 insertions(+), 70 deletions(-)
>
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 8ef1a68..5ff595f 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -20,12 +20,21 @@
>   #include "display_setting.h"
>   #include<lmcons.h>
>
> -//#define CLIPBOARD_ENABLED
> -
>   #define VD_AGENT_LOG_PATH       TEXT("%svdagent.log")
>   #define VD_AGENT_WINCLASS_NAME  TEXT("VDAGENT")
>   #define VD_INPUT_INTERVAL_MS    20
>   #define VD_TIMER_ID             1
> +#define VD_CLIPBOARD_TIMEOUT_MS 10000
> +
> +typedef struct VDClipboardFormat {
> +    uint32_t format;
> +    uint32_t type;
> +} VDClipboardFormat;
> +
> +VDClipboardFormat supported_clipboard_formats[] = {
> +    {CF_UNICODETEXT, VD_AGENT_CLIPBOARD_UTF8_TEXT},
> +    {CF_DIB, VD_AGENT_CLIPBOARD_BITMAP},

I've no idea what exactly the contents of a DIB buffer are, but
using a windows specific format for image buffers seems very wrong to
me. We should use something standard here (say png) and use that.

<snip>

> @@ -562,9 +609,7 @@ bool VDAgent::send_announce_capabilities(bool request)
>       VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
>       VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG);
>       VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_REPLY);
> -#ifdef CLIPBOARD_ENABLED
>       VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_CLIPBOARD);
> -#endif
>       VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_DISPLAY_CONFIG);
>       vd_printf("sending capabilities:");
>       for (uint32_t i = 0 ; i<  caps_size; ++i) {

Given that the 0.6.0 spice client sets VD_AGENT_CAP_CLIPBOARD, but only
supports the old way of dealing with the clipboard, I believe that we
should introduce a new capability for the new way of dealing with
the clipboard and set / check for that.

<snip>

> -//FIXME: currently supports text only
> +bool VDAgent::write_message(uint32_t type, uint32_t size = 0, void* data = NULL)
> +{
> +    VDPipeMessage* pipe_msg;
> +    VDAgentMessage* msg;
> +
> +    pipe_msg = (VDPipeMessage*)write_lock(VD_MESSAGE_HEADER_SIZE + size);
> +    if (!pipe_msg) {
> +        return false;
> +    }
> +    pipe_msg->type = VD_AGENT_COMMAND;
> +    pipe_msg->opaque = VDP_CLIENT_PORT;
> +    pipe_msg->size = sizeof(VDAgentMessage) + size;
> +    msg = (VDAgentMessage*)pipe_msg->data;
> +    msg->protocol = VD_AGENT_PROTOCOL;
> +    msg->type = type;
> +    msg->opaque = 0;
> +    msg->size = size;
> +    if (size&&  data) {
> +        memcpy(msg->data, data, size);
> +    }
> +    write_unlock(VD_MESSAGE_HEADER_SIZE + size);
> +    if (!_pending_write) {
> +        write_completion(0, 0,&_pipe_state.write.overlap);
> +    }
> +    return true;
> +}
> +
>   bool VDAgent::on_clipboard_change()
>   {
> -    UINT format = CF_UNICODETEXT;
> +    uint32_t type = 0;
> +
> +    for (VDClipboardFormat* iter = supported_clipboard_formats; iter->format&&  !type; iter++) {
> +        if (IsClipboardFormatAvailable(iter->format)) {
> +            type = iter->type;
> +        }
> +    }
> +    if (!type) {
> +        vd_printf("Unsupported clipboard format");
> +        return false;
> +    }
> +    VDAgentClipboardGrab grab = {type};
> +    return write_message(VD_AGENT_CLIPBOARD_GRAB, sizeof(grab),&grab);
> +}

Sending of this message should be conditional on the (new) clipboard capability.

> +// In delayed rendering, Windows requires us to SetClipboardData before we return from
> +// handling WM_RENDERFORMAT. Therefore, we try our best by sending CLIPBOARD_REQUEST to the
> +// agent, while waiting alertably for a while (hoping for good) for receiving CLIPBOARD data
> +// or CLIPBOARD_RELEASE from the agent, which both will signal clipboard_event.
> +bool VDAgent::on_clipboard_render(UINT format)
> +{
> +    uint32_t type = get_clipboard_type(format);
> +
> +    if (!type) {
> +        vd_printf("Unsupported clipboard format %u", format);
> +        return false;
> +    }
> +    VDAgentClipboardRequest request = {type};

Sending of this message should be conditional on the (new) clipboard capability. Note
I may have missed a few other places where the same goes.

Regards,

Hans