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

[Hans de Goede]

Hi,

On 09/27/2010 04:51 PM, Arnon Gilboa wrote:
>
> 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?
>

AFAIK, yes replacing XA_PRIMARY with the clipboard is all that is needed.

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

Ok.

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

Ok.

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

Ok.

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

I've been thinking more about esp. in combination with 6) from my follow on
mail. I believe that we simply need to throw received SelectionRequest's
into a queue, this will fix 6). If we then add a flag if we need to
send a SelectionNotify event or not, we can use the same queue to handle
MULTIPLE by simply emulation MULTIPLE as multiple SelectionRequest-s
which we put on the queue one by one (which is exactly the way MULTIPLE
is supposed to work, we then just need the flag to only send
one SelectionNotify event after the last SelectionRequest, instead of
one per SelectionRequest which we do when we really receive multiple
SelectionRequest-s

Regards,

Hans




>> Regards,
>>
>> Hans
> Thanks for the feedback,
> Arnon