Re: drag and drop additions

[Owen Taylor <otaylor redhat com>]

Brett Hall <swizin@rain.org> writes:

> Attached is a patch implementing the additions to drag and drop that I
> mentioned in my earlier message.  Specifically I added four functions to
> gtkdnd.h/.c:

> void     gtk_drag_dest_set_with_list   (GtkWidget        *widget,
> 					GtkDestDefaults   flags,
> 					GtkTargetList    *targets,
> 					GdkDragAction     actions);
> gboolean gtk_drag_dest_get_info        (GtkWidget        *widget,
> 					GtkDestDefaults  *flags,
> 					GtkTargetList   **targets,

As a trivial point, I'd call this argument **target_list, instead of **targets.

> 					GdkDragAction    *actions);
> void     gtk_drag_source_set_with_list (GtkWidget        *widget,
> 					GdkModifierType   start_button_mask,
> 					GtkTargetList    *targets,
> 					GdkDragAction     actions);
> gboolean gtk_drag_source_get_info      (GtkWidget        *widget,
> 					GdkModifierType  *start_button_mask,
> 					GtkTargetList   **targets,
>					GdkDragAction    *actions);
>
> The gtk_drag_*_set_with_list work just like the existing gtk_drag_*_set
> functions except that they take a GtkTargetList argument instead of an
> array of GtkTargetEntry's.  
> The gtk_drag_*_get_info functions return the
> drag and drop info in the pointer arguments for the specified widget,
> the return value will be TRUE when the widget is a drag destination (for
> the dest function) or a source (for the source function) and
> FALSE otherwise.  Note that the caller is responsible for freeing the
> returned target list with gtk_target_list_unref.

I think the right approach here is for the _get_info() functions
to simply return a pointer to the GtkTargetList object. Modifying
it will work fine currently, and I don't mind guaranteeing that
for the future, and if the caller wants to save the reference, they
can refcount it themself.

> I also added two functions to gtkselection.h/.c:
> GtkTargetList *gtk_target_list_new_from_list       (GtkTargetList
> *targets);
> void           gtk_target_list_add_list (GtkTargetList        *list,
>        GtkTargetList* targets);
> 
> The first creates a copy of the given target list and the second adds
> targets from the given list (second argument) to another list (the first
> argument).

The copy function, if necessary, should be called gtk_target_list_copy(). 
(You may find it less necessary with the above suggested change to
the GtkDND API).

I don't think the add_list() function is necessary. It's pretty trivial
to implement, and not that common. If you don't want the user to have
iterate through the list themselves, then the preferred addition would
be:

 typedef void (*GtkTargetFunc) (GtkTargetPair *pair, gpointer data);
 gtk_target_list_foreach (GtkTargetList *list, GtkTargetFunc func, gpointer data);

I'm not sure that this is that valuable - the list member of TargetList is
public, reasonably intuitive, and I see no immediate reason to make
it private.

> Finally I modified testdnd (in the gtk/src directory) to test this new
> functionality.

Now to detailed commentary about the implementation. (Note for future, we'd
prefer to get unified diffs with diff -u instead of context diffs.)

[...]

+ void 
+ gtk_drag_dest_set_with_list   (GtkWidget            *widget,
+ 			  GtkDestDefaults       flags,
+ 			  GtkTargetList			*targets,
+ 			  GdkDragAction         actions)
+ {
+   GtkDragDestSite *site;
+   
+   g_return_if_fail (widget != NULL);
+ 
+   /* HACK, do this in the destroy */
+   site = gtk_object_get_data (GTK_OBJECT (widget), "gtk-drag-dest");
+   if (site)
+     gtk_signal_disconnect_by_data (GTK_OBJECT (widget), site);
+ 
+   if (GTK_WIDGET_REALIZED (widget))
+     gtk_drag_dest_realized (widget);
+ 
+   gtk_signal_connect (GTK_OBJECT (widget), "realize",
+ 		      GTK_SIGNAL_FUNC (gtk_drag_dest_realized), NULL);
+ 
+   site = g_new (GtkDragDestSite, 1);
+ 
+   site->flags = flags;
+   site->have_drag = FALSE;
+   if (targets)
+     site->target_list = gtk_target_list_new_from_list (targets);

Ref here, don't copy. 

+   else
+     site->target_list = NULL;
+ 
+   site->actions = actions;
+   site->do_proxy = FALSE;
+ 
+   gtk_object_set_data_full (GTK_OBJECT (widget), "gtk-drag-dest",
+ 			    site, gtk_drag_dest_site_destroy);
+ }

It general, it would be better not to cut-and-paste the code around
like this - the right way to do it is to make gtk_drag_dest_set() a
wrapper around your new gtk_drag_dest_set_with_list(). Note that this
code actually has been reorganized a bit in CVS as well - so 
gtk_drag_dest_set() is already calling a gtk_drag_dest_set_internal(),
so taht will affect things some.

+ gboolean 
+ gtk_drag_dest_get_info (GtkWidget*	widget,
+ 				GtkDestDefaults		*flags,
+ 				GtkTargetList		**targets,
+ 				GdkDragAction 		*actions)
+ 
+ {
+     GtkDragDestSite* site;
+     
+     g_return_val_if_fail(widget != NULL, FALSE);
+     g_return_val_if_fail(flags != NULL, FALSE);
+     g_return_val_if_fail(targets != NULL, FALSE);
+     g_return_val_if_fail(actions != NULL, FALSE);

The GTK+ standard is to allow all out arguments to be NULL and ignore
them if they are NULL . This turns out to be an enormous convenience.
+     
+     site = gtk_object_get_data(GTK_OBJECT(widget), "gtk-drag-dest");
+     if (site == NULL)
+         return FALSE;

2 space indents.
     
+     *flags = site->flags;
+     *actions = site->actions;
+     *targets = gtk_target_list_new_from_list(site->target_list);

see comment above.

+     return TRUE;
+ }
+ 

+ void 
+ gtk_drag_source_set_with_list  (GtkWidget            *widget,
+ 			   GdkModifierType       start_button_mask,
+ 			   GtkTargetList		 *targets,
+ 			   GdkDragAction         actions)
+ 
+ {
    [...]

Again, gtk_drag_source_set() needs to be a wrapper around this.

+   if (targets)
+     site->target_list = gtk_target_list_new_from_list (targets);

Again, ref, instead of copy.

+   else
+     site->target_list = NULL;
+ 
+   site->actions = actions;
+ 
+ }
+ 

+ gboolean
+ gtk_drag_source_get_info (GtkWidget* widget,
+ 				   GdkModifierType       *start_button_mask,
+ 				   GtkTargetList		 **targets,
+ 				   GdkDragAction         *actions)
+ {
+     GtkDragSourceSite* site;
+     
+     g_return_val_if_fail(widget != NULL, FALSE);
+     g_return_val_if_fail(start_button_mask != NULL, FALSE);
+     g_return_val_if_fail(targets != NULL, FALSE);
+     g_return_val_if_fail(actions != NULL, FALSE);

Same comments as dest_get_info().
+     
+     site = gtk_object_get_data(GTK_OBJECT(widget), "gtk-site-data");
+     if (site == NULL)
+         return FALSE;
+     
+     *start_button_mask = site->start_button_mask;
+     *actions = site->actions;
+     *targets = gtk_target_list_new_from_list(site->target_list);

see above.
diff -cr gtk+-1.2.8.orig/gtk/gtkselection.c gtk+-1.2.8/gtk/gtkselection.c
*** gtk+-1.2.8.orig/gtk/gtkselection.c	Tue Oct 26 15:20:16 1999
--- gtk+-1.2.8/gtk/gtkselection.c	Tue Jun  6 18:44:52 2000
***************
*** 171,176 ****
--- 171,190 ----
    return result;
  }
  
+ GtkTargetList *
+ gtk_target_list_new_from_list (GtkTargetList *targets)
+ {

This function, if kept, should simply have:

    g_return_val_if_fail (targets != NULL, NULL);

Allowing a NULL targets in gtk_target_list_copy() doesn't really
make sense to me. 

+   GtkTargetList *result = g_new (GtkTargetList, 1);
+   result->list = NULL;
+   result->ref_count = 1;
+ 
+   if (targets)
+     gtk_target_list_add_list (result, targets);
+   
+   return result;
+ }
+ 
+ 

diff -cr gtk+-1.2.8.orig/gtk/testdnd.c gtk+-1.2.8/gtk/testdnd.c
*** gtk+-1.2.8.orig/gtk/testdnd.c	Tue Apr  6 18:15:35 1999
--- gtk+-1.2.8/gtk/testdnd.c	Tue Jun  6 18:44:53 2000
***************
*** 287,292 ****
--- 287,398 ----
  
  static guint n_targets = sizeof(target_table) / sizeof(target_table[0]);
  
+ void
+ print_dest_flags (GtkDestDefaults flags)
+ {
+ 	g_print("Destination Flags: ");
+ 	if(flags & GTK_DEST_DEFAULT_MOTION)
+ 		g_print("MOTION ");
+ 	if(flags & GTK_DEST_DEFAULT_HIGHLIGHT)
+ 		g_print("HIGHLIGHT ");
+ 	if(flags & GTK_DEST_DEFAULT_DROP)
+ 		g_print("DROP ");
+ 	g_print("\n");
+ }

Indentation.

In general this looks good. With the changes I mentioned above, it should
be ready to apply.

Thanks for the patch,
                                             Owen