[Spice-devel] [PATCH] client: support clipboard/selection-owner model
Arnon Gilboa
agilboa at redhat.com
Sun Sep 26 02:28:13 PDT 2010
Hans de Goede wrote:
> Hi,
>
> See comments inline.
>
> On 09/22/2010 02:16 PM, Arnon Gilboa wrote:
>> -support the GRAB/REQUEST/DATA/RELEASE verbs in both ways
>> -pasting clipboard data is now "only-by-demand" from both sides
>> (client and agent), whose behavior is symmetric
>> -client and agent don't read or send the contents of the clipboard
>> unnecessarily (e.g. copy, internal paste, repeating paste, focus change)
>> -set client as clipboard listener instead of application
>> -add atexit(cleanup) in win platform
>>
>> linux:
>> -enable USE_XRANDR_1_2 and support clipboard in MultyMonScreen
>> -send utf8 with no null termination, remove ++size
>> -add xfixes in configure.ac& Makefile.am
>>
>> windows:
>> -bonus: support image cut& paste, currently only on windows
>> ---
>> client/application.cpp | 6 -
>> client/application.h | 2 -
>> client/platform.h | 29 ++++-
>> client/red_client.cpp | 205 +++++++++++++++++++--------
>> client/red_client.h | 34 ++++-
>> client/windows/platform.cpp | 240 +++++++++++++++++++++++++-------
>> client/x11/Makefile.am | 1 +
>> client/x11/platform.cpp | 326
>> +++++++++++++++++++++++++++----------------
>> configure.ac | 3 +-
>> 9 files changed, 597 insertions(+), 249 deletions(-)
>>
>> diff --git a/client/application.cpp b/client/application.cpp
>> index 490cd8c..a13d5e8 100644
>> --- a/client/application.cpp
>> +++ b/client/application.cpp
>> @@ -1325,12 +1325,6 @@ void Application::on_app_activated()
>> {
>> _active = true;
>> _key_handler->on_focus_in();
>> - Platform::set_clipboard_listener(this);
>> -}
>> -
>> -void Application::on_clipboard_change()
>> -{
>> - _client.on_clipboard_change();
>> }
>>
>> void Application::on_app_deactivated()
>> diff --git a/client/application.h b/client/application.h
>> index dde37fc..36ae86e 100644
>> --- a/client/application.h
>> +++ b/client/application.h
>> @@ -141,7 +141,6 @@ typedef std::list<GUIBarrier*> GUIBarriers;
>> class Application : public ProcessLoop,
>> public Platform::EventListener,
>> public Platform::DisplayModeListener,
>> - public Platform::ClipboardListener,
>> public CommandTarget {
>> public:
>>
>> @@ -191,7 +190,6 @@ public:
>> virtual void on_app_deactivated();
>> virtual void on_monitors_change();
>> virtual void on_display_mode_change();
>> - virtual void on_clipboard_change();
>> void on_connected();
>> void on_disconnected(int spice_error_code);
>> void on_disconnecting();
>> diff --git a/client/platform.h b/client/platform.h
>> index 6288380..e68ba8f 100644
>> --- a/client/platform.h
>> +++ b/client/platform.h
>> @@ -123,11 +123,32 @@ public:
>>
>> enum {
>> CLIPBOARD_UTF8_TEXT = 1,
>> + CLIPBOARD_BITMAP
>> };
>>
>
> 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?
>
> 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.
>
>> + typedef struct ClipboardFormat {
>> + uint32_t format;
>> + uint32_t type;
>> + } ClipboardFormat;
>> +
>> + static ClipboardFormat clipboard_formats[];
>> +
>> + static uint32_t get_clipboard_type(uint32_t format) {
>> + ClipboardFormat* iter;
>> + for (iter = clipboard_formats; iter->type&& iter->format !=
>> format; iter++);
>> + return iter->type;
>> + }
>> +
>> + static uint32_t get_clipboard_format(uint32_t type) {
>> + ClipboardFormat* iter;
>> + for (iter = clipboard_formats; iter->format&& iter->type !=
>> type; iter++);
>> + return iter->format;
>> + }
>> +
>
> Pretty much the same as above, I see no reason for anything outside the
> platform specific to need the mapping between the VD_AGENT_CLIPBOARD_*
> types and platform specific formats. The lower this mapping is handled
> the cleaner and easier to read the remaining code is. pushing this down
> will even make the platform specific code much clearer as a direct
> conversion
> there is a lot easier to follow when reading the code then having to
> trace the flow through some unneeded abstraction layer.
again, i tend to agree & will fix it unless there are any objections...
>
> Last, but not least, this puts the assumption in the platform independent
> code that all platform clipboards formats are stored as an uint32_t,
> which
> seems wrong to me. For example I'm pretty sure some platform use mime
> types
> stores as strings.
let's discuss this one in the irc
>
>> + static bool set_clipboard_owner(uint32_t type);
>> static bool set_clipboard_data(uint32_t type, const uint8_t*
>> data, int32_t size);
>> - static bool get_clipboard_data(uint32_t type, uint8_t* data,
>> int32_t size);
>> - static int32_t get_clipboard_data_size(uint32_t type);
>> + static bool request_clipboard_notification(uint32_t type);
>> + static void release_clipboard();
>> };
>>
>> class Platform::EventListener {
>> @@ -141,7 +162,9 @@ public:
>> class Platform::ClipboardListener {
>> public:
>> virtual ~ClipboardListener() {}
>> - virtual void on_clipboard_change() = 0;
>> + virtual void on_clipboard_change(uint32_t type) = 0;
>> + virtual void on_clipboard_render(uint32_t type) = 0;
>> + virtual void on_clipboard_notify(uint32_t type, uint8_t* data,
>> int32_t size) = 0;
>
> These names seem a bit windows centric, esp. the render name, why not
> simply name them after the vdagent messages. so change -> grab and
> render -> request ?
will be renamed
>
>> };
>>
>> class Platform::RecordClient {
>> diff --git a/client/red_client.cpp b/client/red_client.cpp
>> index 79f5e6d..ba1cb89 100644
>> --- a/client/red_client.cpp
>> +++ b/client/red_client.cpp
>> @@ -81,9 +81,28 @@ uint32_t default_agent_caps[] = {
>> (1<< VD_AGENT_CAP_REPLY)
>> };
>>
>> -void ClipboardEvent::response(AbstractProcessLoop& events_loop)
>> +typedef struct ClipboardType {
>> + uint32_t agent_type;
>> + uint32_t platform_type;
>> +} ClipboardType;
>> +
>> +ClipboardType clipboard_types[] = {
>> + {VD_AGENT_CLIPBOARD_UTF8_TEXT, Platform::CLIPBOARD_UTF8_TEXT},
>> + {VD_AGENT_CLIPBOARD_BITMAP, Platform::CLIPBOARD_BITMAP},
>> + {0, 0}};
>> +
>
> As said above this should all be removed IMHO.
right
>
>> +void ClipboardGrabEvent::response(AbstractProcessLoop& events_loop)
>> +{
>> + VDAgentClipboardGrab grab = {_type};
>> +
>> static_cast<RedClient*>(events_loop.get_owner())->send_agent_clipboard_message(
>>
>> + VD_AGENT_CLIPBOARD_GRAB, sizeof(grab),&grab);
>> +}
>> +
>> +void ClipboardRequestEvent::response(AbstractProcessLoop& events_loop)
>> {
>> -
>> static_cast<RedClient*>(events_loop.get_owner())->send_agent_clipboard();
>>
>> + VDAgentClipboardRequest request = {_type};
>> +
>> static_cast<RedClient*>(events_loop.get_owner())->send_agent_clipboard_message(
>>
>> + VD_AGENT_CLIPBOARD_REQUEST, sizeof(request),&request);
>> }
>>
>> Migrate::Migrate(RedClient& client)
>> @@ -339,6 +358,7 @@ RedClient::RedClient(Application& application)
>> , _migrate (*this)
>> , _glz_window (0, _glz_debug)
>> {
>> + Platform::set_clipboard_listener(this);
>> MainChannelLoop* message_loop =
>> static_cast<MainChannelLoop*>(get_message_handler());
>> uint32_t default_caps_size = SPICE_N_ELEMENTS(default_agent_caps);
>>
>> @@ -542,6 +562,7 @@ bool RedClient::abort_channels()
>> bool RedClient::abort()
>> {
>> if (!_aborting) {
>> + Platform::set_clipboard_listener(NULL);
>> Lock lock(_sync_lock);
>> _aborting = true;
>> _sync_condition.notify_all();
>
> Not related to this patch, but I assume the
> Lock lock(_sync_lock); is some sort of locking mechanism to protect
> the abort handler against concurrent access. If that is the case,
> the locking is done too late as if this code is re-entrant then the test
> and set of the _aborting flag needs to be protected by a lock as well.
seems like a bug, put a note
>
>> @@ -784,16 +805,6 @@ void RedClient::on_display_mode_change()
>> #endif
>> }
>>
>> -uint32_t get_agent_clipboard_type(uint32_t type)
>> -{
>> - switch (type) {
>> - case Platform::CLIPBOARD_UTF8_TEXT:
>> - return VD_AGENT_CLIPBOARD_UTF8_TEXT;
>> - default:
>> - return 0;
>> - }
>> -}
>> -
>> void RedClient::do_send_agent_clipboard()
>> {
>> uint32_t size;
>> @@ -802,8 +813,7 @@ void RedClient::do_send_agent_clipboard()
>> (size = MIN(VD_AGENT_MAX_DATA_SIZE,
>> _agent_out_msg_size - _agent_out_msg_pos))) {
>> Message* message = new Message(SPICE_MSGC_MAIN_AGENT_DATA);
>> - void* data =
>> - spice_marshaller_reserve_space(message->marshaller(), size);
>> + void* data =
>> spice_marshaller_reserve_space(message->marshaller(), size);
>> memcpy(data, (uint8_t*)_agent_out_msg + _agent_out_msg_pos,
>> size);
>> _agent_tokens--;
>> post_message(message);
>> @@ -817,14 +827,39 @@ void RedClient::do_send_agent_clipboard()
>> }
>> }
>>
>> -//FIXME: currently supports text only; better name - poll_clipboard?
>> -void RedClient::send_agent_clipboard()
>> +void RedClient::send_agent_clipboard_message(uint32_t message_type,
>> uint32_t size, void* data)
>> +{
>> + Message* message = new Message(SPICE_MSGC_MAIN_AGENT_DATA);
>> + VDAgentMessage* msg = (VDAgentMessage*)
>> + spice_marshaller_reserve_space(message->marshaller(),
>> sizeof(VDAgentMessage) + size);
>> + msg->protocol = VD_AGENT_PROTOCOL;
>> + msg->type = message_type;
>> + msg->opaque = 0;
>> + msg->size = size;
>> + if (size&& data) {
>> + memcpy(msg->data, data, size);
>> + }
>> + ASSERT(_agent_tokens)
>> + _agent_tokens--;
>> + post_message(message);
>> +}
>> +
>
> ## begin these ###
>
>> +void RedClient::on_clipboard_change(uint32_t type)
>> {
>> - //FIXME: check connected - assert on disconnect
>> - uint32_t clip_type = Platform::CLIPBOARD_UTF8_TEXT;
>> - int32_t clip_size = Platform::get_clipboard_data_size(clip_type);
>> + AutoRef<ClipboardGrabEvent> event(new ClipboardGrabEvent(type));
>> + get_process_loop().push_event(*event);
>> +}
>> +
>> +void RedClient::on_clipboard_render(uint32_t type)
>> +{
>> + AutoRef<ClipboardRequestEvent> event(new
>> ClipboardRequestEvent(type));
>> + get_process_loop().push_event(*event);
>> +}
>>
>
> Notice the naming issue here again clipboard_change ->
> ClipboardGrabEvent,
> clipboard_render -> ClipboardRequestEvent.
right!
>
>> - if (!clip_size || !_agent_connected) {
>> +void RedClient::on_clipboard_notify(uint32_t type, uint8_t* data,
>> int32_t size)
>> +{
>> + ASSERT(size&& data);
>> + if (!_agent_connected) {
>> return;
>> }
>> if (_agent_out_msg) {
>> @@ -832,32 +867,20 @@ void RedClient::send_agent_clipboard()
>> return;
>> }
>> _agent_out_msg_pos = 0;
>> - _agent_out_msg_size = sizeof(VDAgentMessage) +
>> sizeof(VDAgentClipboard) + clip_size;
>> + _agent_out_msg_size = sizeof(VDAgentMessage) +
>> sizeof(VDAgentClipboard) + size;
>> _agent_out_msg = (VDAgentMessage*)new
>> uint8_t[_agent_out_msg_size];
>> _agent_out_msg->protocol = VD_AGENT_PROTOCOL;
>> _agent_out_msg->type = VD_AGENT_CLIPBOARD;
>> _agent_out_msg->opaque = 0;
>> - _agent_out_msg->size = sizeof(VDAgentClipboard) + clip_size;
>> + _agent_out_msg->size = sizeof(VDAgentClipboard) + size;
>> VDAgentClipboard* clipboard =
>> (VDAgentClipboard*)_agent_out_msg->data;
>> - clipboard->type = get_agent_clipboard_type(clip_type);
>> - if (!Platform::get_clipboard_data(clip_type, clipboard->data,
>> clip_size)) {
>> - delete[] (uint8_t *)_agent_out_msg;
>> - _agent_out_msg = NULL;
>> - _agent_out_msg_size = 0;
>> - return;
>> - }
>> -
>> + clipboard->type = get_agent_clipboard_type(type);
>> + memcpy(clipboard->data, data, size);
>> if (_agent_tokens) {
>> do_send_agent_clipboard();
>> }
>> }
>>
>> -void RedClient::on_clipboard_change()
>> -{
>> - AutoRef<ClipboardEvent> event(new ClipboardEvent());
>> - get_process_loop().push_event(*event);
>> -}
>> -
>> void RedClient::set_mouse_mode(uint32_t supported_modes, uint32_t
>> current_mode)
>> {
>> if (current_mode != _mouse_mode) {
>
> ### start "these functions"
>
>> @@ -886,13 +909,62 @@ void RedClient::set_mouse_mode(uint32_t
>> supported_modes, uint32_t current_mode)
>>
>> void RedClient::on_agent_clipboard(VDAgentClipboard* clipboard,
>> uint32_t size)
>> {
>> - switch (clipboard->type) {
>> - case VD_AGENT_CLIPBOARD_UTF8_TEXT:
>> - Platform::set_clipboard_data(Platform::CLIPBOARD_UTF8_TEXT,
>> clipboard->data, size);
>> - break;
>> - default:
>> - THROW("unexpected vdagent clipboard data type");
>> + uint32_t type = get_platform_clipboard_type(clipboard->type);
>> +
>> + if (!type) {
>> + LOG_INFO("Unsupported clipboard type %u", clipboard->type);
>> + return;
>> }
>> + Platform::set_clipboard_data(type, clipboard->data, size);
>> +}
>> +
>> +void RedClient::on_agent_clipboard_grab(VDAgentClipboardGrab*
>> clipboard_grab)
>> +{
>> + uint32_t type = get_platform_clipboard_type(clipboard_grab->type);
>> +
>> + if (!type) {
>> + LOG_INFO("Unsupported clipboard type %u",
>> clipboard_grab->type);
>> + return;
>> + }
>> + Platform::set_clipboard_owner(type);
>> +}
>> +
>> +void RedClient::on_agent_clipboard_request(VDAgentClipboardRequest*
>> clipboard_request)
>> +{
>> + uint32_t type =
>> get_platform_clipboard_type(clipboard_request->type);
>> +
>> + if (!type) {
>> + LOG_INFO("Unsupported clipboard type %u",
>> clipboard_request->type);
>> + return;
>> + }
>> + if (!Platform::request_clipboard_notification(type)) {
>> + send_agent_clipboard_message(VD_AGENT_CLIPBOARD_RELEASE);
>> + }
>> +}
>> +
>> +void RedClient::on_agent_clipboard_release()
>> +{
>> + Platform::release_clipboard();
>> +}
>> +
>
> It seems to me that "these" functions are a but superfluous, making it
> harder
> to follow the call chain (esp once get_platform_clipboard_type is no
> more).
> I think it would be better to just directly call the relevant
> Platform::*clipboard* functions directly from the
> dispatch_agent_message()
> switch case, this removes a nice number of lines and makes the call chain
> easier to follow.
>
right!
>> +uint32_t RedClient::get_platform_clipboard_type(uint32_t agent_type)
>> +{
>> + for (ClipboardType* iter = clipboard_types; iter->agent_type&&
>> iter->platform_type; iter++) {
>> + if (iter->agent_type == agent_type) {
>> + return iter->platform_type;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +uint32_t RedClient::get_agent_clipboard_type(uint32_t platform_type)
>> +{
>> + for (ClipboardType* iter = clipboard_types; iter->agent_type&&
>> iter->platform_type; iter++) {
>> + if (iter->platform_type == platform_type) {
>> + return iter->agent_type;
>> + }
>> + }
>> + return 0;
>> }
>>
>
> As discussed before I believe these are unnecessary.
:)
>
>> void RedClient::handle_init(RedPeer::InMessage* message)
>> @@ -1055,25 +1127,7 @@ void
>> RedClient::handle_agent_data(RedPeer::InMessage* message)
>> }
>> if (_agent_msg_pos == sizeof(VDAgentMessage) +
>> _agent_msg->size) {
>> DBG(0, "agent msg end");
>> - switch (_agent_msg->type) {
>> - case VD_AGENT_ANNOUNCE_CAPABILITIES: {
>> - on_agent_announce_capabilities(
>> - (VDAgentAnnounceCapabilities*)_agent_msg_data,
>> - _agent_msg->size);
>> - break;
>> - }
>> - case VD_AGENT_REPLY: {
>> - on_agent_reply((VDAgentReply*)_agent_msg_data);
>> - break;
>> - }
>> - case VD_AGENT_CLIPBOARD: {
>> - on_agent_clipboard((VDAgentClipboard*)_agent_msg_data,
>> - _agent_msg->size -
>> sizeof(VDAgentClipboard));
>> - break;
>> - }
>> - default:
>> - DBG(0, "Unsupported message type %u size %u",
>> _agent_msg->type, _agent_msg->size);
>> - }
>> + dispatch_agent_message(_agent_msg, _agent_msg_data);
>> delete[] _agent_msg_data;
>> _agent_msg_data = NULL;
>> _agent_msg_pos = 0;
>> @@ -1081,6 +1135,35 @@ void
>> RedClient::handle_agent_data(RedPeer::InMessage* message)
>> }
>> }
>>
>> +void RedClient::dispatch_agent_message(VDAgentMessage* msg, void* data)
>> +{
>> + switch (msg->type) {
>> + case VD_AGENT_ANNOUNCE_CAPABILITIES: {
>> +
>> on_agent_announce_capabilities((VDAgentAnnounceCapabilities*)data,
>> msg->size);
>> + break;
>> + }
>> + case VD_AGENT_REPLY: {
>> + on_agent_reply((VDAgentReply*)data);
>> + break;
>> + }
>> + case VD_AGENT_CLIPBOARD: {
>> + on_agent_clipboard((VDAgentClipboard*)data, msg->size -
>> sizeof(VDAgentClipboard));
>> + break;
>> + }
>> + case VD_AGENT_CLIPBOARD_GRAB:
>> + on_agent_clipboard_grab((VDAgentClipboardGrab*)data);
>> + break;
>> + case VD_AGENT_CLIPBOARD_REQUEST:
>> + on_agent_clipboard_request((VDAgentClipboardRequest*)data);
>> + break;
>> + case VD_AGENT_CLIPBOARD_RELEASE:
>> + on_agent_clipboard_release();
>> + break;
>> + default:
>> + DBG(0, "Unsupported message type %u size %u", msg->type,
>> msg->size);
>> + }
>> +}
>> +
>> void RedClient::handle_agent_tokens(RedPeer::InMessage* message)
>> {
>> SpiceMsgMainAgentTokens *token = (SpiceMsgMainAgentTokens
>> *)message->data();
>> diff --git a/client/red_client.h b/client/red_client.h
>> index ba8b4ee..ea9c735 100644
>> --- a/client/red_client.h
>> +++ b/client/red_client.h
>> @@ -144,16 +144,31 @@ public:
>> uint32_t _color_depth;
>> };
>>
>> -class ClipboardEvent : public Event {
>> +class ClipboardGrabEvent : public Event {
>> public:
>> + ClipboardGrabEvent(uint32_t type) : _type (type) {}
>> virtual void response(AbstractProcessLoop& events_loop);
>> +
>> +private:
>> + uint32_t _type;
>> };
>>
>> -class RedClient: public RedChannel {
>> +class ClipboardRequestEvent : public Event {
>> +public:
>> + ClipboardRequestEvent(uint32_t type) : _type (type) {}
>> + virtual void response(AbstractProcessLoop& events_loop);
>> +
>> +private:
>> + uint32_t _type;
>> +};
>> +
>> +class RedClient: public RedChannel,
>> + public Platform::ClipboardListener {
>> public:
>> friend class RedChannel;
>> friend class Migrate;
>> - friend class ClipboardEvent;
>> + friend class ClipboardGrabEvent;
>> + friend class ClipboardRequestEvent;
>>
>> RedClient(Application& application);
>> ~RedClient();
>> @@ -192,7 +207,10 @@ public:
>> PixmapCache& get_pixmap_cache() {return _pixmap_cache;}
>> uint64_t get_pixmap_cache_size() { return _pixmap_cache_size;}
>> void on_display_mode_change();
>> - void on_clipboard_change();
>> + void on_clipboard_change(uint32_t type);
>> + void on_clipboard_render(uint32_t type);
>> + void on_clipboard_notify(uint32_t type, uint8_t* data, int32_t
>> size);
>> +
>> void for_each_channel(ForEachChannelFunc& func);
>> void on_mouse_capture_trigger(RedScreen& screen);
>>
>> @@ -228,13 +246,19 @@ private:
>> void handle_agent_data(RedPeer::InMessage* message);
>> void handle_agent_tokens(RedPeer::InMessage* message);
>> void handle_migrate_switch_host(RedPeer::InMessage* message);
>> + void dispatch_agent_message(VDAgentMessage* msg, void* data);
>>
>> void on_agent_reply(VDAgentReply* reply);
>> void
>> on_agent_announce_capabilities(VDAgentAnnounceCapabilities* caps,
>> uint32_t msg_size);
>> void on_agent_clipboard(VDAgentClipboard* clipboard, uint32_t
>> size);
>> - void send_agent_clipboard();
>> + void on_agent_clipboard_grab(VDAgentClipboardGrab* clipboard_grab);
>> + void on_agent_clipboard_request(VDAgentClipboardRequest*
>> clipboard_request);
>> + void on_agent_clipboard_release();
>> void do_send_agent_clipboard();
>> + void send_agent_clipboard_message(uint32_t message_type,
>> uint32_t size = 0, void* data = NULL);
>> + uint32_t get_platform_clipboard_type(uint32_t agent_type);
>> + uint32_t get_agent_clipboard_type(uint32_t platform_type);
>>
>> ChannelFactory* find_factory(uint32_t type);
>> void create_channel(uint32_t type, uint32_t id);
>> diff --git a/client/windows/platform.cpp b/client/windows/platform.cpp
>> index ae49d02..096cd66 100644
>> --- a/client/windows/platform.cpp
>> +++ b/client/windows/platform.cpp
>> @@ -45,9 +45,35 @@ public:
>>
>> static DefaultEventListener default_event_listener;
>> static Platform::EventListener* event_listener
>> =&default_event_listener;
>> -static HWND paltform_win;
>> +static HWND paltform_win = NULL;
>
> Typo s/paltform_win/platform_win/g
'll correct
>
>> static ProcessLoop* main_loop = NULL;
>>
>> +class DefaultClipboardListener: public Platform::ClipboardListener {
>> +public:
>> + virtual void on_clipboard_change(uint32_t type) {}
>> + virtual void on_clipboard_render(uint32_t type) {}
>> + virtual void on_clipboard_notify(uint32_t type, uint8_t* data,
>> int32_t size) {}
>> +};
>> +
>> +static DefaultClipboardListener default_clipboard_listener;
>> +static Platform::ClipboardListener* clipboard_listener
>> =&default_clipboard_listener;
>> +
>> +// The next window in the clipboard viewer chain, which is refered
>> in all clipboard events.
>> +static HWND next_clipboard_viewer_win = NULL;
>> +static HANDLE clipboard_event = NULL;
>> +
>> +// clipboard_changer says whether the client was the last one to
>> change cliboard, for loop
>> +// prevention. It's initialized to true so we ignore the first
>> clipboard change event which
>> +// happens right when we call SetClipboardViewer().
>> +static bool clipboard_changer = true;
>> +
>> +static const int CLIPBOARD_TIMEOUT_MS = 10000;
>> +
>> +Platform::ClipboardFormat Platform::clipboard_formats[] = {
>> + {CF_UNICODETEXT, Platform::CLIPBOARD_UTF8_TEXT},
>> + {CF_DIB, Platform::CLIPBOARD_BITMAP},
>> + {0, 0}};
>> +
>> static const unsigned long MODAL_LOOP_TIMER_ID = 1;
>> static const int MODAL_LOOP_DEFAULT_TIMEOUT = 100;
>> static bool modal_loop_active = false;
>> @@ -59,6 +85,19 @@ void Platform::send_quit_request()
>> main_loop->quit(0);
>> }
>>
>> +static uint32_t get_available_clipboard_type()
>> +{
>> + uint32_t type = 0;
>> +
>> + for (Platform::ClipboardFormat* iter = Platform::clipboard_formats;
>> + iter->format&& !type;
>> iter++) {
>> + if (IsClipboardFormatAvailable(iter->format)) {
>> + type = iter->type;
>> + }
>> + }
>> + return type;
>> +}
>> +
>> static LRESULT CALLBACK PlatformWinProc(HWND hWnd, UINT message,
>> WPARAM wParam, LPARAM lParam)
>> {
>> switch (message) {
>> @@ -82,6 +121,44 @@ static LRESULT CALLBACK PlatformWinProc(HWND
>> hWnd, UINT message, WPARAM wParam,
>> case WM_DISPLAYCHANGE:
>> event_listener->on_monitors_change();
>> break;
>> + case WM_CHANGECBCHAIN:
>> + if (next_clipboard_viewer_win == (HWND)wParam) {
>> + next_clipboard_viewer_win = (HWND)lParam;
>> + } else if (next_clipboard_viewer_win) {
>> + SendMessage(next_clipboard_viewer_win, message, wParam,
>> lParam);
>> + }
>> + break;
>> + case WM_DRAWCLIPBOARD:
>> + if (!clipboard_changer) {
>> + uint32_t type = get_available_clipboard_type();
>> + if (type) {
>> + clipboard_listener->on_clipboard_change(type);
>> + } else {
>> + LOG_INFO("Unsupported clipboard format");
>> + }
>> + } else {
>> + clipboard_changer = false;
>> + }
>> + if (next_clipboard_viewer_win) {
>> + SendMessage(next_clipboard_viewer_win, message, wParam,
>> lParam);
>> + }
>> + break;
>> + case WM_RENDERFORMAT: {
>> + // In delayed rendering, Windows requires us to
>> SetClipboardData before we return from
>> + // handling WM_RENDERFORMAT. Therefore, we try our best by
>> sending CLIPBOARD_REQUEST to the
>> + // agent, while waiting alertably for a while (hoping for
>> good) for receiving CLIPBOARD data
>> + // or CLIPBOARD_RELEASE from the agent, which both will
>> signal clipboard_event.
>> + uint32_t type = Platform::get_clipboard_type(wParam);
>> + if (!type) {
>> + LOG_INFO("Unsupported clipboard format %u", wParam);
>> + break;
>> + }
>> + clipboard_listener->on_clipboard_render(type);
>> + DWORD start_tick = GetTickCount();
>> + while (WaitForSingleObjectEx(clipboard_event, 1000, TRUE) !=
>> WAIT_OBJECT_0&&
>> + GetTickCount()< start_tick + CLIPBOARD_TIMEOUT_MS);
>> + break;
>> + }
>> default:
>> return DefWindowProc(hWnd, message, wParam, lParam);
>> }
>> @@ -92,7 +169,6 @@ static void create_message_wind()
>> {
>> WNDCLASSEX wclass;
>> ATOM class_atom;
>> - HWND window;
>>
>> const LPCWSTR class_name = L"spicec_platform_wclass";
>>
>> @@ -113,11 +189,16 @@ static void create_message_wind()
>> THROW("register class failed");
>> }
>>
>> - if (!(window = CreateWindow(class_name, L"", 0, 0, 0, 0, 0,
>> NULL, NULL, instance, NULL))) {
>> + if (!(paltform_win = CreateWindow(class_name, L"", 0, 0, 0, 0,
>> 0, NULL, NULL, instance, NULL))) {
>> THROW("create message window failed");
>> }
>>
>> - paltform_win = window;
>> + if (!(next_clipboard_viewer_win =
>> SetClipboardViewer(paltform_win))&& GetLastError()) {
>> + THROW("set clipboard viewer failed");
>> + }
>> + if (!(clipboard_event = CreateEvent(NULL, FALSE, FALSE, NULL))) {
>> + THROW("create clipboard event failed");
>> + }
>> }
>>
>> NamedPipe::ListenerRef NamedPipe::create(const char *name,
>> ListenerInterface& listener_interface)
>> @@ -460,9 +541,16 @@ void Platform::path_append(std::string& path,
>> const std::string& partial_path)
>> path += partial_path;
>> }
>>
>> +static void cleanup()
>> +{
>> + ChangeClipboardChain(paltform_win, next_clipboard_viewer_win);
>> + CloseHandle(clipboard_event);
>> +}
>> +
>> void Platform::init()
>> {
>> create_message_wind();
>> + atexit(cleanup);
>> }
>>
>> void Platform::set_process_loop(ProcessLoop& main_process_loop)
>> @@ -749,62 +837,101 @@ void WinPlatform::exit_modal_loop()
>> modal_loop_active = false;
>> }
>>
>> -void Platform::set_clipboard_listener(ClipboardListener* listener)
>> +bool Platform::set_clipboard_owner(uint32_t type)
>> {
>> - //FIXME: call only on change, use statics
>> - listener->on_clipboard_change();
>> + uint32_t format = get_clipboard_format(type);
>> +
>> + if (!format) {
>> + LOG_INFO("Unsupported clipboard type %u", type);
>> + return false;
>> + }
>> + if (!OpenClipboard(paltform_win)) {
>> + return false;
>> + }
>> + clipboard_changer = true;
>> + EmptyClipboard();
>> + SetClipboardData(format, NULL);
>> + CloseClipboard();
>> + return true;
>> }
>>
>> -UINT get_format(uint32_t type)
>> +void Platform::set_clipboard_listener(ClipboardListener* listener)
>> {
>> - switch (type) {
>> - case Platform::CLIPBOARD_UTF8_TEXT:
>> - return CF_UNICODETEXT;
>> - default:
>> - return 0;
>> - }
>> + clipboard_listener = listener ? listener
>> :&default_clipboard_listener;
>> }
>>
>> bool Platform::set_clipboard_data(uint32_t type, const uint8_t*
>> data, int32_t size)
>> {
>> - UINT format = get_format(type);
>> HGLOBAL clip_data;
>> LPVOID clip_buf;
>> int clip_size;
>> - bool ret;
>> -
>> - //LOG_INFO("type %u size %d %s", type, size, data);
>> - if (!format || !OpenClipboard(paltform_win)) {
>> + int clip_len;
>> + UINT format;
>> + bool ret = false;
>> +
>> + // Get the required clipboard size
>> + switch (format = get_clipboard_format(type)) {
>
> No need to do a switch case on the format here, you might
> as well do it directly on the type given that there is a
> 1:1 mapping.
right
>
>> + case CF_UNICODETEXT:
>> + // Received utf8 string is not null-terminated
>> + if (!(clip_len = MultiByteToWideChar(CP_UTF8, 0,
>> (LPCSTR)data, size, NULL, 0))) {
>> + return false;
>> + }
>> + clip_len++;
>> + clip_size = clip_len * sizeof(WCHAR);
>> + break;
>> + case CF_DIB:
>> + clip_size = size;
>> + break;
>> + default:
>> + LOG_INFO("Unsupported clipboard type %u", type);
>> return false;
>> }
>> - clip_size = MultiByteToWideChar(CP_UTF8, 0, (LPCSTR)data, size,
>> NULL, 0);
>> - if (!clip_size || !(clip_data = GlobalAlloc(GMEM_DDESHARE,
>> clip_size * sizeof(WCHAR)))) {
>> - CloseClipboard();
>> +
>> + // Allocate and lock clipboard memory
>> + if (!(clip_data = GlobalAlloc(GMEM_DDESHARE, clip_size))) {
>> return false;
>> }
>> if (!(clip_buf = GlobalLock(clip_data))) {
>> GlobalFree(clip_data);
>> - CloseClipboard();
>> return false;
>> }
>> - ret = !!MultiByteToWideChar(CP_UTF8, 0, (LPCSTR)data, size,
>> (LPWSTR)clip_buf, clip_size);
>> +
>> + // Translate data and set clipboard content
>> + switch (format) {
>> + case CF_UNICODETEXT:
>> + ret = !!MultiByteToWideChar(CP_UTF8, 0, (LPCSTR)data, size,
>> (LPWSTR)clip_buf, clip_len);
>> + ((LPWSTR)clip_buf)[clip_len - 1] = L'\0';
>> + break;
>> + case CF_DIB:
>> + memcpy(clip_buf, data, size);
>> + ret = true;
>> + break;
>> + }
>> GlobalUnlock(clip_data);
>> - if (ret) {
>> - EmptyClipboard();
>> - ret = !!SetClipboardData(format, clip_data);
>> + if (!ret) {
>> + return false;
>> + }
>> + if (SetClipboardData(format, clip_data)) {
>> + SetEvent(clipboard_event);
>> + return true;
>> + }
>> + // We retry clipboard open-empty-set-close only when there is a
>> timeout in WM_RENDERFORMAT
>> + if (!OpenClipboard(paltform_win)) {
>> + return false;
>> }
>> + EmptyClipboard();
>> + ret = !!SetClipboardData(format, clip_data);
>> CloseClipboard();
>> return ret;
>> }
>>
>> -bool Platform::get_clipboard_data(uint32_t type, uint8_t* data,
>> int32_t size)
>> +bool Platform::request_clipboard_notification(uint32_t type)
>> {
>> - UINT format = get_format(type);
>> + UINT format = get_clipboard_format(type);
>> HANDLE clip_data;
>> LPVOID clip_buf;
>> - bool ret;
>> + bool ret = false;
>>
>> - LOG_INFO("type %u size %d", type, size);
>> if (!format || !IsClipboardFormatAvailable(format) ||
>> !OpenClipboard(paltform_win)) {
>> return false;
>> }
>> @@ -812,32 +939,45 @@ bool Platform::get_clipboard_data(uint32_t
>> type, uint8_t* data, int32_t size)
>> CloseClipboard();
>> return false;
>> }
>> - ret = !!WideCharToMultiByte(CP_UTF8, 0, (LPCWSTR)clip_buf, -1,
>> (LPSTR)data, size, NULL, NULL);
>> - GlobalUnlock(clip_data);
>> - CloseClipboard();
>> - return ret;
>> -}
>> -
>> -int32_t Platform::get_clipboard_data_size(uint32_t type)
>> -{
>> - UINT format = get_format(type);
>> - HANDLE clip_data;
>> - LPVOID clip_buf;
>> - int clip_size;
>>
>> - if (!format || !IsClipboardFormatAvailable(format) ||
>> !OpenClipboard(paltform_win)) {
>> - return 0;
>> + switch (format) {
>> + case CF_UNICODETEXT: {
>> + size_t len = wcslen((wchar_t*)clip_buf);
>> + int utf8_size = WideCharToMultiByte(CP_UTF8, 0,
>> (LPCWSTR)clip_buf, len, NULL, 0, NULL, NULL);
>> + if (!utf8_size) {
>> + break;
>> + }
>> + uint8_t* utf8_data = new uint8_t[utf8_size];
>> + if (WideCharToMultiByte(CP_UTF8, 0, (LPCWSTR)clip_buf, len,
>> (LPSTR)utf8_data, utf8_size,
>> + NULL, NULL)) {
>> + clipboard_listener->on_clipboard_notify(type, utf8_data,
>> utf8_size);
>> + ret = true;
>> + }
>> + delete[] (uint8_t *)utf8_data;
>> + break;
>> }
>> - if (!(clip_data = GetClipboardData(format)) || !(clip_buf =
>> GlobalLock(clip_data))) {
>> - CloseClipboard();
>> - return 0;
>> + case CF_DIB: {
>> + size_t clip_size = GlobalSize(clip_data);
>> + if (!clip_size) {
>> + break;
>> + }
>> + clipboard_listener->on_clipboard_notify(type,
>> (uint8_t*)clip_buf, clip_size);
>> + ret = true;
>> + break;
>> + }
>> + default:
>> + LOG_INFO("Unsupported clipboard type %u", type);
>> }
>> - clip_size = WideCharToMultiByte(CP_UTF8, 0, (LPCWSTR)clip_buf,
>> -1, NULL, 0, NULL, NULL);
>> +
>> GlobalUnlock(clip_data);
>> CloseClipboard();
>> - return clip_size;
>> + return ret;
>> }
>>
>> +void Platform::release_clipboard()
>> +{
>> + SetEvent(clipboard_event);
>> +}
>>
>> static bool has_console = false;
>>
>> diff --git a/client/x11/Makefile.am b/client/x11/Makefile.am
>> index 101f6dd..2a83473 100644
>> --- a/client/x11/Makefile.am
>> +++ b/client/x11/Makefile.am
>> @@ -22,6 +22,7 @@ INCLUDES = \
>> $(CELT051_CFLAGS) \
>> $(SSL_CFLAGS) \
>> $(XRANDR_CFLAGS) \
>> + $(XFIXES_LIBS) \
>> $(MISC_X_CFLAGS) \
>> $(CEGUI_CFLAGS) \
>> $(WARN_CFLAGS) \
>> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
>> index cc1502b..625af2a 100644
>> --- a/client/x11/platform.cpp
>> +++ b/client/x11/platform.cpp
>> @@ -28,6 +28,7 @@
>> #include<X11/extensions/XKB.h>
>> #include<X11/extensions/Xrender.h>
>> #include<X11/extensions/XShm.h>
>> +#include<X11/extensions/Xfixes.h>
>> #include<unistd.h>
>> #include<sys/socket.h>
>> #include<sys/epoll.h>
>> @@ -64,7 +65,7 @@
>> //#define X_DEBUG_SYNC(display) XSync(display, False)
>> #define X_DEBUG_SYNC(display)
>> #ifdef HAVE_XRANDR12
>> -//#define USE_XRANDR_1_2
>> +#define USE_XRANDR_1_2
>> #endif
>>
>> static Display* x_display = NULL;
>> @@ -94,24 +95,33 @@ static int xrandr_minor = 0;
>>
>> static bool using_xrender_0_5 = false;
>>
>> +static bool using_xfixes_1_0 = false;
>> +
>> +static int xfixes_event_base;
>> +static int xfixes_error_base;
>> +
>> static unsigned int caps_lock_mask = 0;
>> static unsigned int num_lock_mask = 0;
>>
>> //FIXME: nicify
>> static uint8_t* clipboard_data = NULL;
>> +static int32_t clipboard_data_type = 0;
>> static int32_t clipboard_data_size = 0;
>> static int32_t clipboard_data_space = 0;
>> +static int32_t clipboard_request_type = 0;
>> +static bool clipboard_changer = false;
>> +static XEvent clipboard_event;
>> static Mutex clipboard_lock;
>> static Atom clipboard_prop;
>> static Atom incr_atom;
>> static Atom utf8_atom;
>> -//static Atom clipboard_type_utf8;
>> -#ifdef USE_XRANDR_1_2
>> -static bool clipboard_inited = false;
>> -#endif
>> static Bool handle_x_error = false;
>> static int x_error_code;
>>
>> +Platform::ClipboardFormat Platform::clipboard_formats[] = {
>> + {0, 0},
>> + {0, 0}};
>> +
>> class DefaultEventListener: public Platform::EventListener {
>> public:
>> virtual void on_app_activated() {}
>> @@ -132,7 +142,9 @@ static Platform::DisplayModeListener*
>> display_mode_listener =&default_display_m
>>
>> class DefaultClipboardListener: public Platform::ClipboardListener {
>> public:
>> - void on_clipboard_change() {}
>> + void on_clipboard_change(uint32_t type) {}
>> + void on_clipboard_render(uint32_t type) {}
>> + void on_clipboard_notify(uint32_t type, uint8_t* data, int32_t
>> size) {}
>> };
>>
>> static DefaultClipboardListener default_clipboard_listener;
>> @@ -763,6 +775,10 @@ static void intern_clipboard_atoms()
>> clipboard_prop = XInternAtom(x_display, "CLIPBOARD", False);
>> incr_atom = XInternAtom(x_display, "INCR", False);
>> utf8_atom = XInternAtom(x_display, "UTF8_STRING", False);
>> +
>> + Platform::clipboard_formats[0].format = utf8_atom;
>> + Platform::clipboard_formats[0].type =
>> Platform::CLIPBOARD_UTF8_TEXT;
>> +
>> interned = true;
>> }
>>
>> @@ -778,13 +794,19 @@ DynamicScreen::DynamicScreen(Display* display,
>> int screen, int& next_mon_id)
>> platform_win = XCreateSimpleWindow(display, RootWindow(display,
>> screen), 0, 0, 1, 1, 0, 0, 0);
>> XSelectInput(display, platform_win, StructureNotifyMask);
>> XRRSelectInput(display, platform_win, RRScreenChangeNotifyMask);
>> + if (using_xfixes_1_0) {
>> + XFixesSelectSelectionInput(display, platform_win, XA_PRIMARY,
>> + XFixesSetSelectionOwnerNotifyMask |
>> +
>> XFixesSelectionWindowDestroyNotifyMask |
>> +
>> XFixesSelectionClientCloseNotifyMask);
>> + }
>>
>> Monitor::self_monitors_change++;
>> process_monitor_configure_events(platform_win);
>> Monitor::self_monitors_change--;
>>
>> - XPlatform::set_win_proc(platform_win, root_win_proc);
>> intern_clipboard_atoms();
>> + XPlatform::set_win_proc(platform_win, root_win_proc);
>> X_DEBUG_SYNC(display);
>> }
>>
>> @@ -873,7 +895,7 @@ bool DynamicScreen::set_screen_size(int size_index)
>> Monitor::self_monitors_change++;
>> /*what status*/
>> XRRSetScreenConfig(get_display(), config, root_window,
>> size_index, rotation, CurrentTime);
>> - process_monitor_configure_events(root_window);
>> + process_monitor_configure_events(platform_win);
>> Monitor::self_monitors_change--;
>> XRRFreeScreenConfigInfo(config);
>> X_DEBUG_SYNC(get_display());
>> @@ -1045,26 +1067,24 @@ MultyMonScreen::MultyMonScreen(Display*
>> display, int screen, int& next_mon_id)
>> throw;
>> }
>>
>> - XSelectInput(display, root_window, StructureNotifyMask);
>> + platform_win = XCreateSimpleWindow(display, RootWindow(display,
>> screen), 0, 0, 1, 1, 0, 0, 0);
>> + XSelectInput(display, platform_win, StructureNotifyMask);
>
> Not directly related to this patch, but why the creation and use
> of a platform win rather then directly use the root window?
I cannot guarantee on this one, but as I remember it was impossible to
get the selection events on the root_window for some reason (do u have
any idea?). Anyway, I will check it again.
>
>> X_DEBUG_SYNC(get_display());
>> - XRRSelectInput(display, root_window, RRScreenChangeNotifyMask);
>> + XRRSelectInput(display, platform_win, RRScreenChangeNotifyMask);
>> X_DEBUG_SYNC(get_display());
>> - intern_clipboard_atoms();
>> - XPlatform::set_win_proc(root_window, root_win_proc);
>> + if (using_xfixes_1_0) {
>> + XFixesSelectSelectionInput(display, platform_win, XA_PRIMARY,
>> + XFixesSetSelectionOwnerNotifyMask |
>> +
>> XFixesSelectionWindowDestroyNotifyMask |
>> +
>> XFixesSelectionClientCloseNotifyMask);
>> + }
>>
>> XMonitor::inc_change_ref();
>> - process_monitor_configure_events(root_window);
>> + process_monitor_configure_events(platform_win);
>> XMonitor::dec_change_ref();
>>
>> - X_DEBUG_SYNC(get_display());
>> - //
>> - //platform_win = XCreateSimpleWindow(display,
>> RootWindow(display, screen), 0, 0, 1, 1, 0, 0, 0);
>> - //XSelectInput(display, platform_win, StructureNotifyMask);
>> - //XRRSelectInput(display, platform_win, RRScreenChangeNotifyMask);
>> - //XPlatform::set_win_proc(platform_win, root_win_proc);
>> - //clipboard_prop = XInternAtom(x_display, "CLIPBOARD", False);
>> - //
>> - clipboard_inited = true;
>> + intern_clipboard_atoms();
>> + XPlatform::set_win_proc(platform_win, root_win_proc);
>> X_DEBUG_SYNC(get_display());
>> }
>>
>> @@ -1172,7 +1192,7 @@ void MultyMonScreen::restore()
>> (*iter)->revert();
>> }
>> enable();
>> - process_monitor_configure_events(root_window);
>> + process_monitor_configure_events(platform_win);
>> XMonitor::dec_change_ref();
>> X_DEBUG_SYNC(get_display());
>> }
>> @@ -1734,8 +1754,7 @@ bool
>> MultyMonScreen::set_monitor_mode(XMonitor& monitor, const
>> XRRModeInfo& mode
>> monitor.set_mode(mode_info);
>> set_size(screen_width, screen_height);
>> enable();
>> - Window root_window = RootWindow(get_display(), get_screen());
>> - process_monitor_configure_events(root_window);
>> + process_monitor_configure_events(platform_win);
>> XMonitor::dec_change_ref();
>> X_DEBUG_SYNC(get_display());
>> return true;
>> @@ -2193,7 +2212,9 @@ static void
>> ensure_clipboard_data_space(uint32_t size)
>>
>> static void realloc_clipboard_data_space(uint32_t size)
>> {
>> - if (size<= clipboard_data_space) return;
>> + if (size<= clipboard_data_space) {
>> + return;
>> + }
>> uint32_t old_alloc = clipboard_data_space;
>> clipboard_data_space = size;
>> uint8_t *newbuf = new uint8_t[clipboard_data_space];
>> @@ -2211,49 +2232,58 @@ static void update_clipboard(unsigned long
>> size, uint8_t* data)
>> clipboard_data_size = size;
>> }
>>
>> -
>> -/* NOTE: Function taken from xsel, original name get_append_property
>> - *
>> - * Get a window clipboard property and append its data to the
>> clipboard_data
>> - *
>> - * Returns true if more data is available for receipt.
>> - *
>> - * Returns false if no data is availab, or on error.
>> - */
>> -static bool
>> -get_append_clipboard_data (XSelectionEvent* xsel)
>> -{
>> - Atom target;
>> - int format;
>> - unsigned long bytesafter, length;
>> - unsigned char * value;
>> -
>> - XGetWindowProperty (x_display, xsel->requestor, clipboard_prop,
>> - 0L, 1000000, True, (Atom)AnyPropertyType,
>> -&target,&format,&length,&bytesafter,&value);
>> -
>> - if (target != utf8_atom) {
>> - LOG_INFO ("%s: target %d not UTF8", __func__, target);
>> - // realloc clipboard_data to 0?
>> - return false;
>> - } else if (length == 0) {
>> - /* A length of 0 indicates the end of the transfer */
>> - LOG_INFO ("Got zero length property; end of INCR transfer");
>> - return false;
>> - } else if (format == 8) {
>> +// Function based on xsel get_append_property()
>> +// Get a window clipboard property and append its data to the
>> clipboard_data
>> +// Returns true if more data is available for receipt. false if no
>> data is available, or on error.
>> +static bool get_append_clipboard_data(XSelectionEvent* xsel)
>> +{
>> + Atom target;
>> + int format;
>> + unsigned long bytesafter, length;
>> + unsigned char* value;
>> +
>> + XGetWindowProperty(x_display, xsel->requestor, clipboard_prop,
>> 0L, 1000000, True,
>> +
>> xsel->target,&target,&format,&length,&bytesafter,&value);
>> + if (target != xsel->target) {
>> + LOG_INFO("target %d != %u requested", target, xsel->target);
>> + clipboard_data_size = 0;
>> + return false;
>> + } else if (length == 0) {
>> + // A length of 0 indicates the end of the transfer
>> + LOG_INFO("Got zero length property; end of INCR transfer");
>> + return false;
>> + } else if (format != 8) {
>> + LOG_INFO("Retrieved non-8-bit data");
>> + clipboard_data_size = 0;
>> + return false;
>> + }
>> if (clipboard_data_size + length> clipboard_data_space) {
>> - realloc_clipboard_data_space(clipboard_data_size + length);
>> + realloc_clipboard_data_space(clipboard_data_size + length);
>> }
>> - strncpy ((char*)clipboard_data + clipboard_data_size,
>> (char*)value, length);
>> + strncpy((char*)clipboard_data + clipboard_data_size,
>> (char*)value, length);
>
> Why strncpy and not memcpy? given that we've a length memcpy seems better
> suited for this.
bug
>
>> clipboard_data_size += length;
>> - LOG_INFO ("Appended %d bytes to buffer\n", length);
>> - } else {
>> - LOG_WARN ("Retrieved non-8-bit data\n");
>> - }
>> -
>> - return true;
>> + LOG_INFO("Appended %d bytes to buffer", length);
>> + return true;
>> }
>>
>> +// FIXME: use INCR for large data transfers
>> +static void send_selection_notify(Atom type)
>> +{
>> + Window requestor_win = clipboard_event.xselectionrequest.requestor;
>> + Atom prop = clipboard_event.xselectionrequest.property;
>> + XChangeProperty(x_display, requestor_win, prop, type, 8,
>> PropModeReplace,
>> + (unsigned char *)clipboard_data,
>> clipboard_data_size);
>> + XEvent res;
>> + res.xselection.property = prop;
>> + res.xselection.type = SelectionNotify;
>> + res.xselection.display = clipboard_event.xselectionrequest.display;
>> + res.xselection.requestor = requestor_win;
>> + res.xselection.selection =
>> clipboard_event.xselectionrequest.selection;
>> + res.xselection.target = clipboard_event.xselectionrequest.target;
>> + res.xselection.time = clipboard_event.xselectionrequest.time;
>> + XSendEvent(x_display, requestor_win, 0, 0,&res);
>> + XFlush(x_display);
>> +}
>>
>> static void root_win_proc(XEvent& event)
>> {
>> @@ -2281,28 +2311,38 @@ static void root_win_proc(XEvent& event)
>> event_listener->on_monitors_change();
>> return;
>> }
>> -
>> + 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 ?
>
>> + 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.
>
>> - Window requestor_win = event.xselectionrequest.requestor;
>> - Atom prop = event.xselectionrequest.property;
>> - XChangeProperty(x_display, requestor_win, prop, utf8_atom,
>> 8, PropModeReplace,
>> - (unsigned char *)clipboard_data,
>> clipboard_data_size);
>> - XEvent res;
>> - res.xselection.property = prop;
>> - res.xselection.type = SelectionNotify;
>> - res.xselection.display = event.xselectionrequest.display;
>> - res.xselection.requestor = requestor_win;
>> - res.xselection.selection = event.xselectionrequest.selection;
>> - res.xselection.target = event.xselectionrequest.target;
>> - res.xselection.time = event.xselectionrequest.time;
>> - XSendEvent(x_display, requestor_win, 0, 0,&res);
>> - XFlush(x_display);
>> + clipboard_event = event;
>> + clipboard_listener->on_clipboard_render(type);
>> break;
>> }
>> case SelectionClear: {
>> @@ -2317,17 +2357,26 @@ static void root_win_proc(XEvent& event)
>> unsigned long size;
>> unsigned long dummy;
>> unsigned char *data;
>> +
>> XGetWindowProperty(x_display, platform_win, clipboard_prop,
>> 0, 0, False,
>> -
>> AnyPropertyType,&type,&format,&len,&size,&data);
>> +
>> event.xselection.target,&type,&format,&len,&size,&data);
>> if (size == 0) {
>> + LOG_INFO("XGetWindowProperty(size) failed");
>> + break;
>> + }
>> + if (type != event.xselection.target) {
>> + LOG_INFO("type %d != %u requested", type,
>> event.xselection.target);
>> break;
>> }
>> if (XGetWindowProperty(x_display, platform_win,
>> clipboard_prop, 0, size,
>> - False, AnyPropertyType,&type,&format,&len,&dummy,&data)
>> != Success) {
>> - LOG_INFO("XGetWindowProperty failed");
>> + False,
>> event.xselection.target,&type,&format,&len,&dummy,&data) != Success) {
>> + LOG_INFO("XGetWindowProperty(data) failed");
>> + break;
>> + }
>> + if (type != event.xselection.target) {
>> + LOG_INFO("type %d != %u requested", type,
>> event.xselection.target);
>> break;
>> }
>> - LOG_INFO("data: %s len: %u", data, len);
>> {
>> Lock lock(clipboard_lock);
>> clipboard_data_size = 0;
>> @@ -2336,7 +2385,7 @@ static void root_win_proc(XEvent& event)
>> Window requestor_win = event.xselection.requestor;
>> Atom prop = event.xselection.property; // is this
>> always "CLIPBOARD"?
>> // According to ICCCM spec 2.7.2 INCR Properties, and
>> xsel reference
>> - XSelectInput (x_display, requestor_win,
>> PropertyChangeMask);
>> + XSelectInput(x_display, requestor_win, PropertyChangeMask);
>> XDeleteProperty(x_display, requestor_win, prop);
>> waiting_for_property_notify = true;
>> {
>> @@ -2347,31 +2396,32 @@ static void root_win_proc(XEvent& event)
>> }
>> {
>> Lock lock(clipboard_lock);
>> - update_clipboard(++size, data);
>> + update_clipboard(size, data);
>> }
>> +
>> clipboard_listener->on_clipboard_notify(clipboard_request_type,
>> clipboard_data,
>> + clipboard_data_size);
>> + clipboard_request_type = 0;
>> XFree(data);
>> - clipboard_listener->on_clipboard_change();
>> break;
>> }
>> - case PropertyNotify:
>> - if (!waiting_for_property_notify) {
>> + case PropertyNotify: {
>> + if (!waiting_for_property_notify || event.xproperty.state !=
>> PropertyNewValue) {
>> break;
>> }
>> + bool finished_incr = false;
>> {
>> - if (event.xproperty.state != PropertyNewValue) break;
>> - bool finished_incr = false;
>> - {
>> - Lock lock(clipboard_lock);
>> - finished_incr =
>> !get_append_clipboard_data(&event.xselection);
>> - }
>> - if (finished_incr) {
>> - waiting_for_property_notify = false;
>> - XDeleteProperty(x_display, event.xselection.requestor,
>> - clipboard_prop);
>> - clipboard_listener->on_clipboard_change();
>> - }
>> + Lock lock(clipboard_lock);
>> + finished_incr =
>> !get_append_clipboard_data(&event.xselection);
>> + }
>> + if (finished_incr) {
>> + waiting_for_property_notify = false;
>> + XDeleteProperty(x_display, event.xselection.requestor,
>> clipboard_prop);
>> +
>> clipboard_listener->on_clipboard_notify(clipboard_request_type,
>> clipboard_data,
>> +
>> clipboard_data_size);
>> + clipboard_request_type = 0;
>> }
>> break;
>> + }
>> default:
>> return;
>> }
>> @@ -2463,6 +2513,15 @@ static void init_xrender()
>> XRenderQueryVersion(x_display,&major,&minor)&& (major> 0
>> || minor>= 5);
>> }
>>
>> +static void init_xfixes()
>> +{
>> + int major;
>> + int minor;
>> +
>> + using_xfixes_1_0 =
>> XFixesQueryExtension(x_display,&xfixes_event_base,&xfixes_error_base)&&
>> + XFixesQueryVersion(x_display,&major,&minor)&& major>= 1;
>> +}
>> +
>> unsigned int get_modifier_mask(KeySym modifier)
>> {
>> int mask = 0;
>> @@ -2673,6 +2732,7 @@ void Platform::init()
>> init_kbd();
>> init_xrandr();
>> init_xrender();
>> + init_xfixes();
>> init_XIM();
>>
>> struct sigaction act;
>> @@ -3009,22 +3069,35 @@ LocalCursor* Platform::create_default_cursor()
>> return new XDefaultCursor();
>> }
>>
>> -void Platform::set_clipboard_listener(ClipboardListener* listener)
>> +bool Platform::set_clipboard_owner(uint32_t type)
>> {
>> - //FIXME: XA_CLIPBOARD(x_display)
>> - if (XGetSelectionOwner(x_display, XA_PRIMARY) == None) {
>> - return;
>> + Lock lock(clipboard_lock);
>> + uint32_t format = get_clipboard_format(type);
>> +
>> + if (!format) {
>> + LOG_INFO("Unsupported clipboard type %u", type);
>> + return false;
>> }
>> - clipboard_listener = listener;
>> - XConvertSelection(x_display, XA_PRIMARY, utf8_atom, clipboard_prop,
>> - platform_win, CurrentTime);
>> + clipboard_changer = true;
>> + clipboard_data_size = 0;
>> + XSetSelectionOwner(x_display, XA_PRIMARY, platform_win,
>> CurrentTime);
>> + return true;
>> +}
>> +
>> +void Platform::set_clipboard_listener(ClipboardListener* listener)
>> +{
>> + clipboard_listener = listener ? listener
>> :&default_clipboard_listener;
>> }
>>
>> bool Platform::set_clipboard_data(uint32_t type, const uint8_t*
>> data, int32_t size)
>> {
>> Lock lock(clipboard_lock);
>> + uint32_t format = get_clipboard_format(type);
>>
>> - LOG_INFO("type %u size %u data %s", type, size, data);
>> + if (!format) {
>> + LOG_INFO("Unsupported clipboard type %u", type);
>> + return false;
>> + }
>> if (size> clipboard_data_space) {
>> delete clipboard_data;
>> clipboard_data = new uint8_t[size];
>> @@ -3032,22 +3105,33 @@ bool Platform::set_clipboard_data(uint32_t
>> type, const uint8_t* data, int32_t si
>> }
>> memcpy(clipboard_data, data, size);
>> clipboard_data_size = size;
>> - //FIXME: XA_CLIPBOARD(x_display)
>> - XSetSelectionOwner(x_display, XA_PRIMARY, platform_win,
>> CurrentTime);
>> - LOG_INFO("XSetSelectionOwner");
>> + clipboard_data_type = type;
>> + send_selection_notify(format);
>> return true;
>> }
>>
>> -bool Platform::get_clipboard_data(uint32_t type, uint8_t* data,
>> int32_t size)
>> +bool Platform::request_clipboard_notification(uint32_t type)
>> {
>> - //FIXME: check type
>> - memcpy(data, clipboard_data, size);
>> + uint32_t format = get_clipboard_format(type);
>> +
>> + if (!format) {
>> + LOG_INFO("Unsupported clipboard type %u", type);
>> + return false;
>> + }
>> + if (XGetSelectionOwner(x_display, XA_PRIMARY) == None) {
>> + LOG_INFO("No owner for the selection");
>> + return false;
>> + }
>> + if (clipboard_request_type) {
>> + LOG_INFO("XConvertSelection request is already pending");
>> + return false;
>> + }
>> + clipboard_request_type = type;
>> + XConvertSelection(x_display, XA_PRIMARY, format, clipboard_prop,
>> platform_win, CurrentTime);
>> return true;
>> }
>>
>> -int32_t Platform::get_clipboard_data_size(uint32_t type)
>> +void Platform::release_clipboard()
>> {
>> - //FIXME: check type
>> - return clipboard_data_size;
>> + XSetSelectionOwner(x_display, XA_PRIMARY, None, CurrentTime);
>> }
>> -
>> diff --git a/configure.ac b/configure.ac
>> index 3369dde..77fdf6e 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -189,9 +189,10 @@ AC_SUBST(GL_LIBS)
>> SPICE_NONPKGCONFIG_LIBS+=" $GL_LIBS"
>>
>> PKG_CHECK_MODULES(XRANDR, xrandr)
>> +PKG_CHECK_MODULES(XFIXES, xfixes)
>> AC_SUBST(XRANDR_CFLAGS)
>> AC_SUBST(XRANDR_LIBS)
>> -SPICE_REQUIRES+=" xrandr"
>> +SPICE_REQUIRES+=" xrandr xfixes"
>>
>> PKG_CHECK_MODULES(XRANDR12,
>> xrandr>= 1.2,
>
>
> Regards,
>
> Hans
More information about the Spice-devel
mailing list