GNOME Bugzilla – Bug 588449
DnD doesn't work on GDK/Quartz
Last modified: 2011-09-11 17:05:56 UTC
Attached will be/is a patch to make DnD work properly with GDK/Quartz. The basic problem is that under Quartz, DnD is handled "oppositely" compared to X11 with respect to the event loop. In Quartz DnD occurs in a recursive event loop, so the control flow "drops into" an inner event loop that is known to be specifically for DnD. On X11, DnD events are just delivered within the main event loop. This breaks a key assumption in several bits of DnD related code within GTK (maybe GDK too). We've been using this patch within Ardour for more than a year now, and I regret that I don't fully recall all of the discussion I had with Richard Hult that led up to the design. The basic idea is to defer the real drag start (initiated by NSWindow:dragImage) to an idle handler. This avoids nested event loops, and allows the rest of the GTK DnD design to function normally. There are no noticeable performance hits, and the the overall code impact is low. There are still elements of DnD that are not implemented, but this allows us to drag things around within Ardour, and to DnD files etc. from Finder into Ardour, without any obvious problems.
Created attachment 138332 [details] [review] patch for DnD on GDK/GTK/Quartz
+ info->source_widget = g_object_ref (widget); info->widget = g_object_ref (widget); This seems slightly odd, is there ever a case where info->widget != info->source_widget ? Apart from that, since the code touches only quartz-specific parts, I can't really say more than: if it works, sounds good.
Any time you DnD between widgets, source_widget != widget once you are in the target. If you check the code, you will see that info->widget gets reset in several places once DnD leaves the source. If I got this wrong, please let me know. In the X11 code, it appears that info->widget can be set to NULL without any loss of information. On Quartz, we need info->source_widget if only to clean up at the end of the drag (e.g. to emit drag-data-delete). From what I could see, the X11 code uses X methods to accomplish this.
Looking into this now.
Comment on attachment 138332 [details] [review] patch for DnD on GDK/GTK/Quartz My knowledge on X11 DnD isn't so great. This patch really makes sense to me in general after briefly reading through the Cocoa DnD docs (haven't used that part myself yet). So I am with Matthias here really, "if works, then great" ;). I am sure we'll manage to further improve it along the way, this will serve as a good starting point. There's one chunk I didn't really get though: > /************************************************************* > * gtk_drag_highlight_expose: > * Callback for expose_event for highlighted widgets. >@@ -857,6 +888,8 @@ > gtk_drag_get_data (widget, context, target, time); > } > >+ /* leave a note for the source-side context about the action chosen */ >+ > g_signal_emit_by_name (widget, "drag-drop", > context, x, y, time, &retval); Should this comment really be here? Because there's a likewise comment further down, that actually *does* make sense: >@@ -510,6 +536,10 @@ > > - (void)draggingEnded:(id <NSDraggingInfo>)sender > { >+ /* leave a note for the source about what action was taken */ >+ if (_gdk_quartz_drag_source_context && current_context) >+ _gdk_quartz_drag_source_context->action = current_context->action; >+ Apart from that I only have some coding style nitpicks. Not sure if you have commit access to git master. If you want me to commit the patch, I will gladly fix those nitpicks up for you. Also, I will actually try out the patch on my Mac tomorrow.
Created attachment 144004 [details] [review] updated patch; applies cleanly against master Here's an updated patch that applies cleanly against git master. The original diff was corrupted. I have also removed the definition of _gtk_quartz_create_image_from_drawable as this function was not used in the patch. I have to say that it works beautifully. Let's commit this soon :) Will you commit or do you want me to handle that for you?
i have commit access but i'm not set up with a current git tree and currently am still a git neophyte, so i'd be entirely happy for you to commit it. re: the comment, i think you are right that the first instance is a remnant and should be removed. while reviewing the updated patch, i realized that the design here is bound to the assumption that there can only be one drag in progress. not good for multitouch but i think that GTK/GDK has much deeper issues with that for now :)
*** Bug 588643 has been marked as a duplicate of this bug. ***
*** Bug 501588 has been marked as a duplicate of this bug. ***
Oops, didn't see you commented here. I am committing it now then :) (In reply to comment #7) > i have commit access but i'm not set up with a current git tree and currently > am still a git neophyte, so i'd be entirely happy for you to commit it. re: the > comment, i think you are right that the first instance is a remnant and should > be removed. > > while reviewing the updated patch, i realized that the design here is bound to > the assumption that there can only be one drag in progress. not good for > multitouch but i think that GTK/GDK has much deeper issues with that for now :) Yes, I don't think this is a problem for now. Carlos Garnacho has been working on XInput2 and multitouch on the X side. I think he recently uploaded his branch to the GNOME repository. At some point, we should be looking into this and add Mac support I guess :)