Comments on 95362 (tree dnd) patch

[Owen Taylor <otaylor redhat com>]

[ Doing this by mail rather than pasting in bugzilla, because
the comments are a bit long, and it's harder to respond to
comments in bugzilla. Cc'ing gtk-devel-list to have a public
record that can be linked to from bugzilla ]

In general, the patch looks fine. Most of the comments
below aren't about correctness, but rather about cleaner/simpler
ways of doing things.

I'd consider this definitely a 2.4-only patch. First it's
just long and touching a complex bit of code.

But also, it does subtly change the behavior of GTK+ a bit
and while I consider the changes "compatible enough",
it's still better to keep 2.2.x releases as little
suprising as possible.

Yes, this leaves tree DND pretty much broken until we
get 2.4.0 out ... well, more incentive to get 2.4.0 soon. :-)

It's possible if we get this into HEAD and it works well
for a while, we can consider a backport for 2.2.4.

Regards,
					Owen

===
@@ -315,3 +315,6 @@ gtk_tree_get_row_drag_data (GtkSelection
 
   trd = (void*) selection_data->data;
 
+  if (!trd)
+    return FALSE;
+
====

Two comments. The canonical check for a failed retrieval
of selection data would be:

 if (selection_data->length < 0)
   return;

The other comment is that while I think this addition is fine,
if you hit it in testing, there probably is a bug elsewhere.

One potential problem I see is that gtk_tree_view_drag_data_received()
doesn't check selection_data->length before calling 
gtk_tree_drag_dest_row_drop_possible(), but does before
calling gtk_tree_drag_dest_drag_data_received(), which looks
wrong. If selection_data->length < 0, the drop isn't possible.

===
+      gint childs;
===

n_children looks like a better name

===
+      /* the row got dropped on empty space, let's setup a special case
*/
       remove_open_timeout (tree_view);
===

The comment here looks like it refers to just the next line,
which is confusing. As a general convention, I write it as above 
for comments that affect only one line, and write

 /* the row got dropped on empty space, let's setup a special case
  */

For larger comments, but in any case, I think moving the comment
to the top of the block would be better.
 
===
+      if (path)
+	gtk_tree_path_free (path);
===

gtk_tree_view_get_dest_row_at_pos() follow the standard

===
+      *suggested_action = context->suggested_action;
+      source_widget = gtk_drag_get_source_widget (context);
+
+      if (source_widget == widget)
+        {
+          /* Default to MOVE, unless the user has
+           * pressed ctrl or alt to affect available actions
                              ^^^
probably shift is meant (old bug copied from elsewhere)

+           */
+          if ((context->actions & GDK_ACTION_MOVE) != 0)
+            *suggested_action = GDK_ACTION_MOVE;
+        }
===

I don't think duplicating this block of code (and the more below) is
good practice.

A judicious use of goto looks like it would work here.  You could have
an out: label that set the suggested action (might need a gboolean
can_drop), called gtk_tree_view_set_drag_dest_row() and freed path.

===
+      tree_view->priv->empty_view_drop = 1;
===

Instead of using a flag for this, couldn't you just use
set_drag_dest_row([0], BEFORE) to signal a drop into an empty view?
After all, that's how you are going to return that information from
gtk_tree_view_get_drag_dest_row().

===
+  tree_view->priv->path_down_mode = 0;
===

Hmm, it seems cleaner to me to make this an extra out parameter
of get_logical_dest_row(), and then make set_dest_row() store
it along with the row-reference. The reason that the destintation
row is stored on the context rather than the tree-view is
that you could (theoretically at least) have multiple drops
in progress at once. If we didn't worry about that, we could
just store the row-reference with the tree view as well.

This is a bit of extra code - you'll need a temporary structure and 
a function to free it - but almost all of the rest of my suggestions 
involve making the patch shorter, so ... :-)

===
+      if (!gtk_tree_model_get_iter (model, &iter, path) ||
+          !gtk_tree_model_iter_next (model, &iter))
+        tree_view->priv->drop_append_mode = 1;
       else
         {
+          tree_view->priv->drop_append_mode = 0;
+          gtk_tree_path_next (path);
+        }
     }
===

Couldn't you just immediately use a path one past
the end rather than using drop_append_mode? The only place 
that the path at the end is used, when calling
gtk_tree_drag_dest_row_drop_possible(),
seems like a bug. The path passed to 
gtk_tree_drag_dest_row_drop_possible() needs to match
that passed to gtk_tree_drag_dest_drag_data_received().

===
-	    suggested_action = 0;
+            {
+              if (gtk_tree_path_get_depth (path) > 1)
+                {
+                  tree_view->priv->path_down_mode = 0;
+                  gtk_tree_path_up (path);
===

Does going up if path_down wasn't set really make sense?
It isn't consistent what you do when the data is finally
received either. It seems you should be checking 
path_down_mode rather than gtk_tree_path_get_depth (path)


===
   dest_row = get_dest_row (context);
 
   if (dest_row == NULL)
-    return;
+    {
+      if (tree_view->priv->empty_view_drop)
+        dest_row = gtk_tree_path_new_from_indices (0, -1);
+      else
+        return;
+    }
===

I don't think this code will be hit even if with your
current code; even if empty_view_drop() is set, 
get_dest_row() will be set. 

===
+              if (gtk_tree_path_get_indices (prev)[0] ==
list_store->length - 1)
+                /* drop append mode */
+		gtk_list_store_append (list_store, &dest_iter);
 	      else
+		gtk_list_store_insert_after (list_store, &dest_iter, &tmp_iter);
===

If gtk_tree_path_get_indices (prev)[0] == length -1, then isn't
appending the same as inserting after prev?

===
+              if (gtk_tree_path_get_indices
(prev)[gtk_tree_path_get_depth (prev)-1]
+                  == length - 1)
+                gtk_tree_store_append (tree_store, &dest_iter,
+                                       has_parent?&parent:NULL);
 	      else
+		gtk_tree_store_insert_after (tree_store, &dest_iter, NULL,
&tmp_iter);
===

Same observation as above; if 

 gtk_tree_path_get_indices (prev)[<last>] == length - 1

then gtk_tree_store_insert_after() and gtk_tree_store_append() should
be the same thing.