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