[Spice-devel] [PATCH spice 14/17] spicec-x11: handle multiple types per grab

[Hans de Goede]

Hi,

On 10/04/2010 07:24 PM, Arnon Gilboa wrote:
> comments below
>
> Hans de Goede wrote:
>> And also handle many x11 targets (ie utf8 variants) to a single agent
>> type mapping.
>> ---
>> client/x11/platform.cpp | 154 ++++++++++++++++++++++++++++++-----------------
>> 1 files changed, 99 insertions(+), 55 deletions(-)
>>
>> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
>> index 82660a4..90b086f 100644
>> --- a/client/x11/platform.cpp
>> +++ b/client/x11/platform.cpp

<snip snip>

>> @@ -2302,15 +2314,31 @@ static void print_targets(const char *action, Atom *atoms, int c)
>> static void send_targets(XEvent& request_event)
>> {
>> XEvent res;
>> - /* FIXME add MULTIPLE */
>> - /* FIXME add (and support) all 3 utf8 atom variations (see linux agent) */
>> - Atom targets[2] = { targets_atom, utf8_atom };
>> + /* Warning the size of this needs to be increased each time we add support
>> + for a new type, or the atom count of an existing type changes */
>> + Atom targets[4] = { targets_atom, };
> it's a bit fishy

I know, a better solution is welcome.

>> + int i, j, target_count = 1;
>> +
>> + for (i = 0; i < clipboard_type_count; i++) {
>> + switch (clipboard_agent_types[i]) {
>> + case VD_AGENT_CLIPBOARD_UTF8_TEXT:
>> + for (j = 0; j < utf8_atom_count; j++) {
>> + targets[target_count] = utf8_atoms[j];
>> + target_count++;
>> + }
>> + break;
>> + /* TODO Add support for more types here */
>> + }
>> + }
>>
>> Window requestor_win = request_event.xselectionrequest.requestor;
>> Atom prop = request_event.xselectionrequest.property;
>> + if (prop == None)
>> + prop = request_event.xselectionrequest.target;
>> +
>> XChangeProperty(x_display, requestor_win, prop, XA_ATOM, 32,
>> PropModeReplace, (unsigned char *)&targets,
>> - sizeof(targets)/sizeof(Atom));
>> + target_count);
>>
>> res.xselection.property = prop;
>> res.xselection.type = SelectionNotify;
>> @@ -2321,7 +2349,7 @@ static void send_targets(XEvent& request_event)
>> res.xselection.time = request_event.xselectionrequest.time;
>> XSendEvent(x_display, requestor_win, 0, 0, &res);
>> XFlush(x_display);
>> - print_targets("sent", targets, sizeof(targets)/sizeof(Atom));
>> + print_targets("sent", targets, target_count);
>> }
>>
>> static int get_selection(XEvent &event, Atom type, Atom prop, int format,
>> @@ -2434,6 +2462,18 @@ static void get_selection_free(unsigned char *data, bool incr)
>> XFree(data);
>> }
>>
>> +static Atom atom_lists_overlap(Atom *atoms1, Atom *atoms2, int l1, int l2)
>> +{
>> + int i, j;
>> +
>> + for (i = 0; i < l1; i++)
>> + for (j = 0; j < l2; j++)
>> + if (atoms1[i] == atoms2[j])
>> + return atoms1[i];
>> +
>> + return 0;
>> +}
>> +
> why not support overlap (send grab msg) of more than one atom?

If there is any atom overlap (so 1 or more) for a list of atoms
which maps to a single vdagent type (such as utf8_atoms) with the
list of atoms reported by the owner we report the type in the
grab message. We only need the first one which overlaps, as we
need to pick one to send with XConvertSelection when the
other side (the agent) requests the vdagent type in question.

>> static void handle_targets_notify(XEvent& event, bool incr)
>> {
>> int len;
>> @@ -2460,13 +2500,13 @@ static void handle_targets_notify(XEvent& event, bool incr)
>> /* bytes -> atoms */
>> len /= sizeof(Atom);
>> print_targets("received", atoms, len);
>> -#if 0 /* FIXME support multiple types */
>> +
>> clipboard_type_count = 0;
>> - atom = atom_lists_overlap(utf8_atoms, atoms, utf8_atom_count, len);
>> + Atom atom = atom_lists_overlap(utf8_atoms, atoms, utf8_atom_count, len);
>> if (atom) {
>> clipboard_agent_types[clipboard_type_count] =
>> VD_AGENT_CLIPBOARD_UTF8_TEXT;
>> - clipboard_x11_targets[x11->clipboard_type_count] = atom;
>> + clipboard_x11_targets[clipboard_type_count] = atom;
>> clipboard_type_count++;
>> }
>>
>> @@ -2475,10 +2515,7 @@ static void handle_targets_notify(XEvent& event, bool incr)
>> if (clipboard_type_count)
>> clipboard_listener->on_clipboard_grab(clipboard_agent_types,
>> clipboard_type_count);
>> -#else
>> - uint32_t type = VD_AGENT_CLIPBOARD_UTF8_TEXT;
>> - clipboard_listener->on_clipboard_grab(&type, 1);
>> -#endif
>> +
>> get_selection_free((unsigned char *)atoms, incr);
>> }
>>
>> @@ -3258,13 +3295,24 @@ LocalCursor* Platform::create_default_cursor()
>> bool Platform::on_clipboard_grab(uint32_t *types, uint32_t type_count)
>> {
>> Lock lock(clipboard_lock);
>> - /* FIXME use all types rather then just the first one */
>> - uint32_t format = get_clipboard_format(types[0]);
>> + int i;
>> +
>> + clipboard_type_count = 0;
>> + for (i = 0; i < type_count; i++) {
>> + /* TODO Add support for more types here */
>> + /* Check if we support the type */
>> + if (types[i] != VD_AGENT_CLIPBOARD_UTF8_TEXT)
>> + continue;
>> +
>> + clipboard_agent_types[clipboard_type_count] = types[i];
>> + clipboard_type_count++;
>> + }
> on receiving grab, i see multiple atoms are handled

Yes, just as they are on receiving a targets list.

<snip snip>

> btw, now that both client & platform have the same on_clipnboard_x() functions (which handle the opposite directions), i'm not sure it helps readability. maybe we should add _client_/_agent_ to the names?

I've been thinking about that too. To me the difference is obvious, one direction
always is Platform::... the other is always clipboard_listener->... but feel
free to rename them.

Regards,

Hans