Re: Comments on 95362 (tree dnd) patch

[Owen Taylor <otaylor redhat com>]

On Sat, 2003-08-16 at 21:55, Kristian Rietveld wrote:

> > ===
> > +      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?
> 
> No, I can't. set_drag_dest_row() will try to create a row reference for
> path "0" at some point. The row reference constructor checks if the row
> actually exists in the model (by trying to get the iter using the path),
> and because the model is empty this will fail. So the row reference will
> not be created.

Hmm. Seems fairly logical to me to extend GtkRowReference to allow 
one-past-the-end. It is useful any time you want to refer to an
insertion position in the tree. However, I guess I can see where
that is going to be difficult to implement.

Note, however, that accessing the flag in various places in the
code indicates an API problem, since
gtk_tree_view_set/get_drag_dest_row() are public API functions.

Maybe you should pass the empty row to gtk_tree_view_set_drag_dest_row()
and let that take care of setting the flag?

> > ===
> > +      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 
> 
> No, this path will be stored in a row reference too, so the row must
> exist in the model.

Well, then this has the same problem as path_down_mode -
it's logically associated with the GtkDragContext not the view,
so I think it needs to be stored on the context, not the 
view.

One possibility is that you could make get/set_dest_row() do
the "path-off-the-end" / flag conversion.

> > ===
> >    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. 
> 
> It does get hit, because we don't store a path for the empty view drop.

Something seems a bit fishy here ... the empty_view_drop flag
goes along with gtk_tree_view_set/get_dest_row() - it is a flag for
the *drag* operation. But here we are in the land of 
set/get_dest_row() - the *drop* operation. 

Two tiny comments from browsing the path.

===
+static void
+dest_row_free (gpointer data)
+{
+  DestRow *dr = (DestRow *)data;
+
+  if (!dr)
+    return;
==

This isn't right - free functions never get NULL passed to them.

====
   if (TRUE /* FIXME if the location droppable predicate */)
     {
+      can_drop = TRUE;
+    }
+  else
+    {
+      /* can't drop here */
+      remove_open_timeout (tree_view);
+
+      gtk_tree_view_set_drag_dest_row (GTK_TREE_VIEW (widget),
+                                       NULL,
+                                       GTK_TREE_VIEW_DROP_BEFORE);
+    }
+
+out:
+  if (can_drop)
+    {
====

I think it would read better if you moved the else {} to 
the if (can_drop) 

Regards,
					Owen