GNOME Bugzilla – Bug 658722
Drag and Drop sometimes stops working
Last modified: 2018-05-02 15:13:03 UTC
I've found that occasionally drag and drop will just stop working. This is difficult to demonstrate, but if you run tests/testdnd and repeatedly drag outside the window, you will every once in a while get it to fail, meaning that the "Drag" box stays highlighted, but if you try to drag, no drag icon appears. I've traced this to problem with failing to NULL the gdk_quartz_drag_source_context; in the present code this is the sole responsibility of GtkQuartzNSWindow. If gdk_quartz_drag_source_context isn't NULL, gdk_quartz_window_drag_begin() assumes that there's been a timing issue with quartz and that a gtk_drag_begin_idle() has already been queued but hasn't started running yet and returns NULL, which causes gtk_drag_begin_interal() to quit instead of queuing up another one. I wasn't able to actually get this to happen (i.e., I wasn't able to get quartz to think that it needed to start another drag when one was already queued or running.)
Created attachment 196218 [details] [review] Implement gdk_quartz_drag_abort and gdk_quartz_drag_drop First, rather than assuming that there's already an event queued up if _gdk_quartz_drag_source_context isn't NULL, assume that it just didn't get cleaned up the last time it ran and abort it. This naturally requires implementing gdk_quartz_drag_abort(), so remove the code from [GdkQuartzNSWindow draggedImage:endedAt:operation:] and move it to gdkdnd_quartz.c as static void gdk_quartz_drag_end(). Implement both gdk_quartz_drag_drop() and gdk_quartz_drag_abort() by calling gdk_quartz_drag_end(). Next, try to get rid of the memory cycle between gtk_drag_source_info.context and _gdk_quartz_drag_source_context (which carries the GtkQuartzDragSourceInfo struct as qdata). Replace gtk_drag_source_clear_info() by using a g_object_set_qdata_full() for context in gtk_drag_get_source_context, calling gtk_drag_source_info_destroy() as the destructor. This eliminates the need to queue a cleanup idle event. I use g_object_run_dispose() on _gtk_quartz_drag_source_context to force the deletion of the info stored as qdata, which in turn unrefs the info->context pointer. Ordinarily this gets fired off from draggedImage:endedAt:operation:, meaning that the special dragging CFRunLoop is complete and NSEvents are again flowing, so queuing a cleanup event isn't necessary. The advantage is that it can also be run from gdk_drag_abort, so if Gdk thinks there's a drag but CF doesn't all of the memory still gets cleaned up.
Review of attachment 196218 [details] [review]: I tend to agree with the general idea of this patch, but I am not very happy with the use of g_object_run_dispose(). I need to make up my mind about that before accepting this patch for commit, perhaps there's a different way to solve things. ::: gdk/quartz/gdkdnd-quartz.c @@ +46,3 @@ + g_warning ("Drag begun with existing context; aborting the preexisting drag"); + gdk_drag_abort (_gdk_quartz_drag_source_context, + (guint32)g_get_real_time()); Need a space before (). @@ +96,3 @@ + event->dnd.context = context; + + gdk_event_set_device (event, gdk_drag_context_get_device(context)); Need a space before (context). @@ +102,3 @@ + /* Remove any idles which might be depending on this context + if (g_idle_remove_by_data ((gpointer)context)) + g_warning ("Deleted context had pending idle event"); */ Now that the installation of the idle handler has been removed, I assume these two lines and the above comment can be removed? @@ +103,3 @@ + if (g_idle_remove_by_data ((gpointer)context)) + g_warning ("Deleted context had pending idle event"); */ + g_object_run_dispose (G_OBJECT (_gdk_quartz_drag_source_context)); I am a bit wary of using g_object_run_dispose(). If this is only for the sake of clearing the qdata, is there another way to accomplish this? I would have suggested g_object_set_qdata(context, quark, NULL), but in this file we do not have access to the right quark (which is a good thing, because we should actually not be clearing qdata installed by another file). Ideally qdata is gotten rid of in the GTK+ part before this function is called. Though I am not yet sure whether that is possible in this case. ::: gtk/gtkdnd-quartz.c @@ +291,3 @@ + + g_free (info); + info = NULL; info = NULL is of no use since info is a local variable; so this line can be removed.
Sorry for the three copies, I thought my browser was not responding.
(In reply to comment #2) > Review of attachment 196218 [details] [review]: > > I tend to agree with the general idea of this patch, but I am not very happy > with the use of g_object_run_dispose(). I need to make up my mind about that > before accepting this patch for commit, perhaps there's a different way to > solve things. > > ::: gdk/quartz/gdkdnd-quartz.c > @@ +46,3 @@ > + g_warning ("Drag begun with existing context; aborting the preexisting > drag"); > + gdk_drag_abort (_gdk_quartz_drag_source_context, > + (guint32)g_get_real_time()); > > Need a space before (). > > @@ +96,3 @@ > + event->dnd.context = context; > + > + gdk_event_set_device (event, gdk_drag_context_get_device(context)); > > Need a space before (context). > > @@ +102,3 @@ > + /* Remove any idles which might be depending on this context > + if (g_idle_remove_by_data ((gpointer)context)) > + g_warning ("Deleted context had pending idle event"); */ > > Now that the installation of the idle handler has been removed, I assume these > two lines and the above comment can be removed? No, there is still the startup idle handler, which might still be pending because there hasn't been a gap in the nsevent stream that allowed it to run. That's the condition the original version of gdk_window_quartz_drag_begin() was returning NULL for. > > @@ +103,3 @@ > + if (g_idle_remove_by_data ((gpointer)context)) > + g_warning ("Deleted context had pending idle event"); */ > + g_object_run_dispose (G_OBJECT (_gdk_quartz_drag_source_context)); > > I am a bit wary of using g_object_run_dispose(). If this is only for the sake > of clearing the qdata, is there another way to accomplish this? I would have > suggested g_object_set_qdata(context, quark, NULL), but in this file we do not > have access to the right quark (which is a good thing, because we should > actually not be clearing qdata installed by another file). > > Ideally qdata is gotten rid of in the GTK+ part before this function is called. Having the Gtk+ part queuing the idle event is the problem that this patch is trying to solve. It's a reference cycle: The info has a context pointer, and hanging the info as a qdata on the context creates a reference cycle. run_dispose() is the documented way of breaking reference cycles, deleting the qdata correctly unrefs the info, and we do indeed want to destroy the object, so what's your concern?
Created attachment 196824 [details] [review] Updated Patch with Kristian's typo corrections Fixes the two missing-spaces-before-function-paren typos.
Created attachment 196827 [details] [review] Updated Patch with Kristian's typo corrections This time after *saving* the corrections.
(In reply to comment #6) > > @@ +102,3 @@ > > + /* Remove any idles which might be depending on this context > > + if (g_idle_remove_by_data ((gpointer)context)) > > + g_warning ("Deleted context had pending idle event"); */ > > > > Now that the installation of the idle handler has been removed, I assume these > > two lines and the above comment can be removed? > > No, there is still the startup idle handler, which might still be pending > because there hasn't been a gap in the nsevent stream that allowed it to run. > That's the condition the original version of gdk_window_quartz_drag_begin() was > returning NULL for. I was asking because both lines are commented out.
(In reply to comment #8) > Created an attachment (id=196827) [details] [review] > Updated Patch with Kristian's typo corrections > > This time after *saving* the corrections. This patch seems incomplete -- gtk_drag_source_info_destroy() is now removed instead of being moved up?
Created attachment 201766 [details] [review] Updated patch to master of 20Nov2011 Sigh. Must have been a merge failure. Here is a new patch, rebased against today's master (including your changes to gtkdnd-quartz.c).
*** Bug 682889 has been marked as a duplicate of this bug. ***
The patch doesn't apply cleanly (gtk+-3.10.7), but without patching (hunk by hunk), drag and drop fails miserably on Mac OS X Lion. Tested by first trying <https://wiki.gnome.org/GnomeLove/DragNDropTutorial>, which led to assertion failure as follows: Gdk:ERROR:gdkdnd-quartz.c:40:_gdk_quartz_window_drag_begin: assertion failed: (_gdk_quartz_drag_source_context == NULL) Patching made the system at least run, though the following warnings are still posted: Gdk-WARNING **: Deleted context had pending idle event
Review of attachment 201766 [details] [review]: ::: gdk/quartz/gdkdnd-quartz.c @@ +53,3 @@ NULL); _gdk_quartz_drag_source_context->is_source = TRUE; + _gdk_quartz_drag_source_context->source_window = window; In particular, if anything else, can this be commited? Without this, it is easy to get critical crashes while using DND.
Actually, it needs to be reffed also
Created attachment 284598 [details] [review] quartz-dnd: make sure to set source window This makes sure that source_window is set in the context when beginning a drag. This in turn prevents crashes for example when a default dnd icon is set when beginning a drag.
Review of attachment 284598 [details] [review]: makes sense, and matches what the other backends do
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/369.