Re: [PATCH 3/8] winex11.drv: implement DoDragDrop on top of XDND
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. 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. 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. * 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". 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. 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. Best regards, Sebastian
On Wed, Jul 15, 2015 at 8:34 PM, Sebastian Lackner <sebastian(a)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
participants (2)
-
Damjan Jovanovic -
Sebastian Lackner