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
_______________________________________________
Spice-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/spice-devel
