After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 588449 - DnD doesn't work on GDK/Quartz
DnD doesn't work on GDK/Quartz
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: X11
unspecified
Other Mac OS
: Normal major
: ---
Assigned To: gtk-bugs
gtk-bugs
: 501588 588643 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-07-13 14:17 UTC by Paul Davis
Modified: 2011-09-11 17:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for DnD on GDK/GTK/Quartz (11.33 KB, patch)
2009-07-13 15:06 UTC, Paul Davis
none Details | Review
updated patch; applies cleanly against master (9.93 KB, patch)
2009-09-25 14:44 UTC, Kristian Rietveld
committed Details | Review

Description Paul Davis 2009-07-13 14:17:20 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.
Comment 1 Paul Davis 2009-07-13 15:06:33 UTC
Created attachment 138332 [details] [review]
patch for DnD on GDK/GTK/Quartz
Comment 2 Matthias Clasen 2009-07-20 04:43:39 UTC
+  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.
Comment 3 Paul Davis 2009-07-20 12:08:06 UTC
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.
Comment 4 Kristian Rietveld 2009-09-22 20:34:19 UTC
Looking into this now.
Comment 5 Kristian Rietveld 2009-09-24 19:53:30 UTC
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.
Comment 6 Kristian Rietveld 2009-09-25 14:44:08 UTC
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?
Comment 7 Paul Davis 2009-09-25 15:59:05 UTC
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 :)
Comment 8 Kristian Rietveld 2009-09-25 16:26:32 UTC
*** Bug 588643 has been marked as a duplicate of this bug. ***
Comment 9 Kristian Rietveld 2009-09-25 16:30:37 UTC
*** Bug 501588 has been marked as a duplicate of this bug. ***
Comment 10 Kristian Rietveld 2009-09-30 13:04:48 UTC
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 :)