[Spice-devel] [PATCH] protocol: RFC: add clipboard selection capability

[Hans de Goede]

Hi,

Thanks for working on this. I was recently doing some copy pasting
between a virtual machine and a client and I really missed the
primary selection.

On 02/18/2011 11:57 PM, Marc-André Lureau wrote:
> From: Marc-André Lureau<marcandre.lureau at redhat.com>
>
> Add a new capability .
>

Sounds + looks good.

> When both client and servers have this capability, the
> VDAgentClipboard* messages MUST be prepended with a uint8_t indicating
> which clipboard selection to operate.

I'm not sure I like this part, I would rather see new structs to use
when this capability is present:

For example a new VDAgentSelectionGrab to use instead of VDAgentClipboardGrab
when both sides have the capability, which looks like this:

typedef struct SPICE_ATTR_PACKED VDAgentSelectionGrab {
     uint32_t selection;
     uint32_t types[0];
} VDAgentClipboardGrab;

Note I made the selection a uint32_t on purpose, this way the following
uint32_t's in the struct will not get unaligned by the packing.

I realize this comes down to the same thing in the end, but I think having
separate structs rather then doing prepending is more clear, and more
importantly is more consistent with how we handle all the other message
types.

I took a look at your linux vdagent implementation for this, and it looks
good. Except that you seem to do the prepending with the selection
unconditionally, iow even when the other side does not have the
VD_AGENT_CAP_CLIPBOARD_SELECTION cap.

Regards,

Hans


>
> A few clipboard selection are defined according to X11/Gtk scheme:
>
> - VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD: the default clipboard,
>    implemented by most OS to deal with explicit Copy/Paste operations.
>
> - VD_AGENT_CLIPBOARD_SELECTION_PRIMARY: the PRIMARY clipboard, used
>    for mouse selections.
>
> - VD_AGENT_CLIPBOARD_SELECTION_SECONDARY: the SECONDARY clipboard.
>
> See also:
> http://www.freedesktop.org/wiki/Specifications/ClipboardsWiki
> ---
>   spice/vd_agent.h |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index 43d0e7c..e1ae3bc 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -153,9 +153,16 @@ enum {
>       VD_AGENT_CAP_CLIPBOARD,
>       VD_AGENT_CAP_DISPLAY_CONFIG,
>       VD_AGENT_CAP_CLIPBOARD_BY_DEMAND,
> +    VD_AGENT_CAP_CLIPBOARD_SELECTION,
>       VD_AGENT_END_CAP,
>   };
>
> +enum {
> +    VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD = 0,
> +    VD_AGENT_CLIPBOARD_SELECTION_PRIMARY,
> +    VD_AGENT_CLIPBOARD_SELECTION_SECONDARY,
> +};
> +
>   typedef struct SPICE_ATTR_PACKED VDAgentAnnounceCapabilities {
>       uint32_t  request;
>       uint32_t caps[0];