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

[Hans de Goede]

Hi,

On 09/26/2010 11:28 AM, Arnon Gilboa wrote:

<snip>

>>
>> 1) I see no need for platform.h to contain a duplicate
>> enum with vd_agent.h we should simply use the
>> VD_AGENT_CLIPBOARD_* types in the platform
>> code too, this way we can get rid of this enum,
>> the ClipboardType type, the clipboard_types
>> array and the RedClient::get_platform_clipboard_type
>> and RedClient::get_agent_clipboard_type methods.
>>
>> Moreover removing this will hopefully also ease
>> the headache I'm getting when reviewing this and
>> trying to figure out how the type gets transformed
>> from one into the other as calls are made from
>> one object to the next, if all conversions are done
>> when needed, etc.
> the purpose was keeping platform & agent types separated, but you seem to be right :)
> any objections that platform.cpp will now #include vd_agent.h?
>>

No, including vd_agent.h from platform code seems fine to me.

>> 2) I believe we should not add bitmap support until we've a
>> platform independent way of doing this. Adding features prematurely
>> only leads to troubles down the road later (see the defunct before
>> even really used old clipboard capability flag for example).
> will be removed unless/until i find a plat-indep way.

Ok, thanks. I do plan on looking into how to support this under X11
using something platform neutral as format to send between the agent
and client. Say a png file or some such, how well would png work for
windows ? Otherwise we can just do a simple rgb24 format with a small
header specifying width, height and pitch.

>>> + if (event.type == XFixesSelectionNotify + xfixes_event_base) {
>>> + XFixesSelectionNotifyEvent* selection_event = (XFixesSelectionNotifyEvent *)&event;
>>> + if (selection_event->subtype != XFixesSetSelectionOwnerNotify) {
>>> + // FIXME: for some reason the XFixesSelectionWindowDestroyNotify/ClientCloseNotify
>>> + // events which can help for sending CLIPBOARD_RELEASE to the agent are not received
>>
>> This FIXME seems to me to be caused by you only checking for XFixesSelectionNotify in the
>> outer if of this nested if.
> but aren't these 3 subtypes of XFixesSelectionNotify ?

Ah yes, you're right, no idea why this is not working then.

>>
>>> + LOG_INFO("Unsupported selection event %u", selection_event->subtype);
>>> + return;
>>> + }
>>> + LOG_INFO("XFixesSetSelectionOwnerNotify %u", clipboard_changer);
>>> + if (clipboard_changer) {
>>> + clipboard_changer = false;
>>> + return;
>>> + }
>>> + // FIXME: use actual type
>>> + clipboard_listener->on_clipboard_change(Platform::CLIPBOARD_UTF8_TEXT);
>>> + return;
>>> + }
>>> switch (event.type) {
>>> case SelectionRequest: {
>>> - //FIXME: support multi-chunk
>>> Lock lock(clipboard_lock);
>>> - if (clipboard_data_size == 0) {
>>> - return;
>>> + XSelectionRequestEvent* selection_request = (XSelectionRequestEvent*)&event;
>>> + uint32_t type = Platform::get_clipboard_type(selection_request->target);
>>> + if (!type) {
>>> + LOG_INFO("Unsupported selection type %u", selection_request->target);
>>> + break;
>>> + }
>>> + if (clipboard_data_size> 0) {
>>> + send_selection_notify(selection_request->target);
>>> + break;
>>> }
>>
>> This seems wrong, if an app requests a selection it should get the current data from
>> the agent, not whatever happens to still be in the buffer from when a previous app
>> requested it but happened to close / crash before consuming the selection. Also the
>> target for the old data could be different (once we support multiple targets).
> So if we paste the same x MB again & again, you believe we should request them from the agent each time although we know there was no change?
> I cannot see the problematic issue with the previous crashes/closed app which requested the data, but I agree we need to check the target is the same and convert/request if required.

Hi, upon a second review of the code I agree :)

This second review has found a long list of other issues though
(not all stemming from this patch). I'm composing a second mail
with all these issues listed.


Regards,

Hans