Created attachment 285358 [details] [review]
main-view: Handle cairo_surface_t icons during DnD

A GdkPixbuf was being assumed, but the model can now hold cairo_surface_t
objects too, so check for the model GType before creating the DnD icon.

Review of attachment 285358 [details] [review]:

::: libgd/gd-main-view.c
@@ +857,3 @@
+        icon = GDK_PIXBUF (data);
+      else if (column_gtype == CAIRO_GOBJECT_TYPE_SURFACE)
+        icon = gdk_pixbuf_get_from_surface (data, 0, 0,

A nice cleanup would be to always use gtk_drag_set_icon_surface(), but for that you would need to change also gd_main_view_get_counter_icon() which is probably a little bit of work - so it's fine if you want to leave it this way.

You should call cairo_surface_destroy() on the surface in this case though, or we'll leak a surface reference.

(In reply to comment #2)
> Review of attachment 285358 [details] [review]:
> 
> ::: libgd/gd-main-view.c
> @@ +857,3 @@
> +        icon = GDK_PIXBUF (data);
> +      else if (column_gtype == CAIRO_GOBJECT_TYPE_SURFACE)
> +        icon = gdk_pixbuf_get_from_surface (data, 0, 0,
> 
> A nice cleanup would be to always use gtk_drag_set_icon_surface(), but for that
> you would need to change also gd_main_view_get_counter_icon() which is probably
> a little bit of work - so it's fine if you want to leave it this way.

I did that patch, one thing I don't like much of it is that in multiple selection, you end up creating a cairo surface twice, one as a copy of the treemodel data, and another with the emblem composed. Using a copy is a must if you want to do cairo_surface_set_device_offset() so the drag icon appears displaced and you don't modify the treemodel/cellrenderer surface too.

Optimizing that to just create a single copy in the process with GdkPixbuf in between was making it growingly unreadable, and copying a few tenths of pixels across image once during DnD isn't much of a performance concern, so I kept it as it is.

> 
> You should call cairo_surface_destroy() on the surface in this case though, or
> we'll leak a surface reference.

Oops, I indeed was, the new patch shouldn't leak.

Created attachment 285407 [details] [review]
main-view: Use cairo surfaces as DnD icon

Now all DnD icons are dealt with as cairo_surface_t, if pixbufs are used
in the data model, a cairo surface is created out of it first.

Created attachment 285408 [details] [review]
main-view: Use cairo surfaces as DnD icon

Now all DnD icons are dealt with as cairo_surface_t, if pixbufs are used
in the data model, a cairo surface is created out of it first.

Review of attachment 285408 [details] [review]:

::: libgd/gd-main-view.c
@@ +838,3 @@
+
+static cairo_surface_t *
+                                       cairo_image_surface_get_height (surface));

You should use gdk_cairo_surface_create_from_pixbuf() here instead of a custom function

@@ +916,2 @@
         {
+          cairo_surface_write_to_png (surface, "/home/carlos/foo.png");

Whoops :-)

(In reply to comment #6)
> Review of attachment 285408 [details] [review]:
> 
> ::: libgd/gd-main-view.c
> @@ +838,3 @@
> +
> +static cairo_surface_t *
> +                                       cairo_image_surface_get_height
> (surface));
> 
> You should use gdk_cairo_surface_create_from_pixbuf() here instead of a custom
> function

I somehow missed that one, useful indeed.

> 
> @@ +916,2 @@
>          {
> +          cairo_surface_write_to_png (surface, "/home/carlos/foo.png");
> 
> Whoops :-)

Doh! Never mistake cairo_format_t with cairo_content_t, and never leave the traces :)

Created attachment 285413 [details] [review]
main-view: Use cairo surfaces as DnD icon

Now all DnD icons are dealt with as cairo_surface_t, if pixbufs are used
in the data model, a cairo surface is created out of it first.

Review of attachment 285413 [details] [review]:

Thanks, Carlos!

Attachment 285413 [details] pushed as 04b2480 - main-view: Use cairo surfaces as DnD icon