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

[Hans de Goede]

Hi,

On 09/26/2010 10:15 AM, Arnon Gilboa wrote:
> 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.
>

Right, support for the old way can be completely removed. But since it was shipped (short of) we should
use a new capability to check for the new way of handling the clipboard, and make both sided only do clipboard
stuff when the other side reports VD_AGENT_CAP_CLIPBOARD_BY_DEMAND. I like that as a CAP name btw :)

Regards,

Hans

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