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

[Arnon Gilboa]

So far, the clipboard support was disabled in the agent side, so I see 
no real need for supporting the two different clipboard flows and 
complicating the behavior in both sides. I suggest removing the support 
for the "old clipboard" (VD_AGENT_CAP_CLIPBOARD, which was unusable 
anyway), and using something like VD_AGENT_CAP_CLIPBOARD_BY_DEMAND for 
the new capability & its conditional behavior in both sides.

Hans de Goede wrote:
> 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