Re: long standing dnd bug fixes

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj gtk org> writes:

> hi owen,
> 
> i got annoyed enough by my dnd-bug workaround code in beast to actually
> sat down and fix the mroe serious bugs in gtkdnd.c.
> the patch to gtk_drag_find_widget() is appended, it adresses:
> 1) dnd code messing up if the widget tree changes during motion (happens
>    if a target-indicator widget is added/removed under the drag icon)

This part looks reasonable. (Thought I remembered doing this,
but if I did, it's long gone.)

> 2) wrong drop position calculations within scrolled windows that have
>    non-0 scroll offsets (and probably other cases where parent->window !=
>    gtk_widget_get_parent_window (child)).

I'm really confused by this:

 - My code looks correct
 - I don't understand your code.
 - A simple test case (sticking testdnd in a scrolled window)
   works fine with the old code.

There is a small bug in the current code if there is a window
widget with it's window not at allocation->x,y (back when
buttons were window widets, they did this when there was
a border width, may be other widgets do the same.)

> +     GdkWindow *pwindow = gtk_widget_get_parent_window (widget);
> +      /* translate offset relative to widget's parent window */
>        while (window != widget->parent->window)
>  	{
> -	  gint tx, ty, twidth, theight;
> -	  gdk_window_get_size (window, &twidth, &theight);
> +	  wlist = g_slist_prepend (wlist, window);
> +	  window = gdk_window_get_parent (window);
> +	}
> +      for (slist = wlist; slist; slist = slist->next)
> +	{
> +	  window = slist->data;
> +	  gdk_window_get_position (window, &tx, &ty);
> +	  x_offset += tx;
> +	  y_offset += ty;
> +	}

So, x_offset/y_offset are the total offset between
widget->parent->window and ->window.

> +      g_slist_free (wlist);
> +      wlist = NULL;

What's the point of the list - gdk_window_get_position()
isn't going to change the heirarchy, and order doesn't
matter for addition....

At this point, coordinates of new_allocation are relative
to pwindow.

+      window = widget->window;
> +      while (window != pwindow)
> +	{
> +	  wlist = g_slist_prepend (wlist, window);
> +	  window = gdk_window_get_parent (window);
> +	}
> +      for (slist = wlist; slist; slist = slist->next)
> +	{
> +	  window = slist->data;

This is a bit of a silly list. pwindow is either widget->window
or gdk_window_get_parent (window)... So, you could just
write if (window != pwindow) {} 

> +	  gdk_window_get_position (window, &tx, &ty);
> +	  gdk_window_get_size (window, &twidth, &theight);
> +	  x_offset += tx;
> +	  y_offset += ty;

Wait, we've already added in this position in the previous
loop...

> +	  if (tx > new_allocation.x)
>  	    {
> -	      new_allocation.width += new_allocation.x;
> +	      new_allocation.width -= tx - new_allocation.x;
>  	      new_allocation.x = 0;
> +	      new_allocation.width = MIN (new_allocation.width, twidth);
> +	    }
> +	  else
> +	    {
> +	      new_allocation.x -= tx;
> +	      new_allocation.width = MIN (new_allocation.width, twidth - new_allocation.x);
>  	    }
> -	  if (new_allocation.y < 0)
> +	  if (ty > new_allocation.y)
>  	    {
> -	      new_allocation.height += new_allocation.y;
> +	      new_allocation.height -= ty - new_allocation.y;
>  	      new_allocation.y = 0;
> +	      new_allocation.height = MIN (new_allocation.height, theight);
>  	    }
> +	  else
> +	    {
> +	      new_allocation.y -= ty;
> +	      new_allocation.height = MIN (new_allocation.height, theight - new_allocation.y);
> +	    }

OK, so this part seems to translate correctly from coordinates
relative to pwindow to coordinates relative to window.

But, we've lost the clipping to windows between widget->parent->window and 
pwindow, which means you may be able to drop on child widgets which are not 
visible.
>
> +  new_data = *data;
> +  new_data.x -= x_offset;
> +  new_data.y -= y_offset;
> +  new_data.found = FALSE;
> +  new_data.toplevel = FALSE;
> +  
>    if (data->toplevel ||
> -      ((data->x >= new_allocation.x) && (data->y >= new_allocation.y) &&
> -       (data->x < new_allocation.x + new_allocation.width) && 
> -       (data->y < new_allocation.y + new_allocation.height)))
> +      ((new_data.x >= new_allocation.x) && (new_data.y >= new_allocation.y) &&
> +       (new_data.x < new_allocation.x + new_allocation.width) && 
> +       (new_data.y < new_allocation.y + new_allocation.height)))

In the old code we were comparing coordinates relative to
widget->parent->window, here we are comparing coordinates relative to
widget->window. It doesn't matter one way or the other.

> @@ -1306,16 +1353,14 @@ gtk_drag_find_widget (GtkWidget       *w
>  	{
>  	  data->found = data->callback (widget,
>  					data->context,
> -					data->x - new_allocation.x,
> -					data->y - new_allocation.y,
> +					new_data.x - new_allocation.x,
> +					new_data.y - new_allocation.y,
>  					data->time);

Again, here the difference is just in what coordinate system we
are working in.

If you could come up with a modification to testdnd that shows
the coordinate problems, I'd appreciate it. I'll check in
some fixes now that add some comments and fixes the problem
with window widgets with borders.

> btw, i suspect my new (like your old) code to still be broken for
> handleboxes, since the position calculation relies on parent->window
> and widget->window to form a chain via gdk_window_get_parent().

I'll do a quick fix to fix the infinite loop. I think the long term
fix is to make GtkHandleBox gtk_widget_reparent() it's child widget
into a GtkWindow. Anything else just violates all sorts of GTK+
invariants.

Regards,
                                        Owen