[PATCH 3/8] winex11.drv: implement DoDragDrop on top of XDND

[Damjan Jovanovic]

On Wed, Jul 15, 2015 at 8:34 PM, Sebastian Lackner
<sebastian at fds-team.de> wrote:
> On 04.07.2015 15:36, Damjan Jovanovic wrote:
>> Implements DoDragDrop on top of XDND. While some of DoDragDrop() and
>> supporting functions are copied from ole32, they were modified to be
>> non-blocking and event-driven.
>>
>> The clipboard is patched to allow searching its clipboard formats for
>> X11 atoms, export data in X11 format, and forward selection requests
>> for the XdndSelection selection to XDND.
>>
>> We support XDND version 3 to 5 (although we still need XdndProxy
>> support for version 4). XdndPosition messages are rate-limited to at
>> most 1 unreplied message and at least every 50 milliseconds apart, so
>> that we don't flood slow X11 clients like the spec warns.
>>
>> Damjan Jovanovic
>> ---
>>  dlls/winex11.drv/clipboard.c |  50 +++
>>  dlls/winex11.drv/event.c     |   2 +
>>  dlls/winex11.drv/x11drv.h    |   7 +
>>  dlls/winex11.drv/xdnd.c      | 829 ++++++++++++++++++++++++++++++++++++++++++-
>>  4 files changed, 887 insertions(+), 1 deletion(-)
>
> Thanks for working on this. Sorry for the delayed reply, I've been a bit busy
> during the last days. Not sure if you have already started improving the
> patches, but I'll just comment on this version.

Thank you for your comments.

> First of all, regarding patch 2, applying such a patch separately doesn't make
> much sense. Either the design is accepted (then it can be part of patch 3), or
> it needs additional work, but then its questionable if the function can be used
> without further modifications.

On the other hand people are always harping on about patches needing
to be split up, so I'll keep try 2 like that ;-).

> At first I was planning to give line by line feedback on the patch, but I think
> you are more interested in general feedback about the design. One of the main
> disadvantages I see in the current approach is that there is a lot of code
> duplication in winex11 and ole32. It will get even worse as soon as someone
> starts implementing the same for winemac or other future backends. I am aware
> that Ken Thomases basically asked for such a solution, but I don't think its the
> right approach. There are basically two ways to solve it:

> * Either the code in winex11 has to be stripped down a lot. A more minimalistic
> solution has better chances to be accepted, and as soon as winemac contains a
> similar implementation, the code in ole32 could be removed. When we need a huge
> statemachine though, its better to avoid duplication.

The idea was also that since on Mac the conversion won't be 100%
accurate (unlike on Windows, the drag source can't drop at a specific
time, cancel the drop at a specific time, or continue the drop even if
ESC is pressed), other platforms could be even less accurate (eg. on
Android you can't hold down modifier keys while dragging), and even on
X11 you can't really right-drag (though some hacks with XdndActionAsk
are possible), we could maintain a generic perfectly working Wine to
Wine ole32 drag and drop that can be selectively activated through a
registry setting or winecfg, in place of native drag and drag.

Also Alexandre accepted my original drop into Wine using XDND code as
one single large patch, so maybe minimalism isn't always necessary
;-).

> * Alternatively, instead of duplicating code, the winex11/winemac backend could
> provide a function pointer table or separate exports, with callbacks for things
> which can be done natively. Ole32 could use these callbacks to register the
> format, query if it dragging was finished, and so on. To avoid polling, the
> driver could also post window messages to the "TrackerWindow".

My first patch went along those lines, and it got rejected. XDND
cannot be cleanly implemented the way that first patch tried, that is,
by doing non-blocking XDND messaging in the blocking IDropTarget
callbacks because (1) the long timeouts possible in X11 will cause
those callbacks to block for a long time, something this patch avoids,
and, even worse, (2) the IDropTarget callbacks that will do XDND
messaging must pump the UI event loop *again* to receive
XClientMessageEvent responses. Also posting messages to the
"TrackerWindow" may work on X11, but Mac is blocking.

> If you need any help, feel free to ask. Also feel free to show proof-of-concept
> patches even when they are not finished yet, for example via IRC or wine-devel.

Thank you.

> As a side note, it looks like various parts of your patch lack proper locking,
> but thats not your fault, its already missing in the existing winex11 code. I'll
> see if I can clean up the existing code a bit to simplify the integration of
> such a new feature.

I am not sure what locking you're referring to?

> Best regards,
> Sebastian

Thank you
Damjan