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

[Arnon Gilboa]

Hans de Goede wrote:
> Hi,
>
> On a second review I've found some more issues with the X11 client
> copy paste code, note some of this were already present before
> the patch in question.
>
> 1) You're using the XA_PRIMARY selection, this is the one which
>    gets set as soon as you select anything with the mouse cursor,
>    no need to press ctrl-c or ctrl-x to set it, so it gets changed
>    all the time! Note this is also the one which usually gets pasted
>    with the middle mouse button. I believe that rather then the
>    XA_PRIMARY you want the selection identified by the clipboard
>    atom, which uses the normal ctrl-c + ctrl-v copy / paste behavior
>
Right, this way the behavior will be similar to windows guest/client.
I think XA_PRIMARY is basically used only for text, right?
So we simply need to replace XA_PRIMARY with the clipboard atom or 
anything else?

> 2) In the SelectionNotify event case in root_win_proc the following
>    check is made:
>         if (type != event.xselection.target) {
>
>    But type can be the INCR atom as well and this is valid too, as the
>    code acknowledges by checking for it later on:
>         if (type == incr_atom) {
>
>    Which will never be reached as the above check will fail in this
>    case.
>
>    Also when the type is INCR, there is no reason to do the second
>    request with the indicated size, as the resulting data (which is an
>    32 bit integer indicating the minimum size of the data which will
>    be send) will get thrown away anyways.
>
will be fixed
> 3) In the none INCR case the property is never deleted, but it should
>    be once we're done reading it, see:
>    http://tronche.com/gui/x/icccm/sec-2.html#s-2.4
>
>    At the end of the paragraph, I believe we can do this by simply
>    setting the delete flag to true when the second XGetWindowProperty
>    call is made.
>
will be fixed
> 4) If we get a SelectionRequest with a type which we do not support, we
>    should send back a SelectionNotify event with the property set to none
>    to indicate that we cannot fulfill the request.
>
>    Like wise when Platform::request_clipboard_notification fails, or the
>    client receives a SelectionNotify with a property of None in response
>    to the Platform::request_clipboard_notification() the client should
>    send a message to the agent that it could not get the desired data
>    (so that under Linux the agent can generate a SelectionNotify with a
>     property of None to let the requesting app inside the guest know
>     we cannot give it the type it is asking for).
>
>    I suggest we add a VD_AGENT_CLIPBOARD_NONE for this to the below enum,
> enum {
>     VD_AGENT_CLIPBOARD_UTF8_TEXT = 1,
>     VD_AGENT_CLIPBOARD_BITMAP,
> };
>
>    We could make this have value 0.
>
will be fixed
> 5) When receiving a SelectionRequest event, we MUST always accept the
>    TARGETS and MULTIPLE atoms as targets, see:
>    http://tronche.com/gui/x/icccm/sec-2.html#s-2.6.2
>
>    The handling of neither of this is really easy, for TARGETS we should
>    ask the agent what types (VD_AGENT_CLIPBOARD_*) the current clipboard
>    contents can be converted too, and then report that back. This
>    requires some vd agent protocol enhancements.
>
>    The handling of MULTIPLE is no fun either, as that will require
>    multiple round trips to the agent, and we need to keep track
>    where we are in the array of requests.
>
seems like complicating things...i'll give it a look and we'll discuss it
> Regards,
>
> Hans
Thanks for the feedback,
Arnon