Continuing from #control-center in GIMPNet:

<hadess> i blame rishi's "hi dpi" work ;)

Instead, I think it has got to do with GTK_ICON_SIZE_DND and GtkCellRendererPixbuf because it doesn't matter if you use the HiDpi patches or not. Investigating ...

(In reply to comment #1)
> Continuing from #control-center in GIMPNet:
> 
> <hadess> i blame rishi's "hi dpi" work ;)
> 
> Instead, I think it has got to do with GTK_ICON_SIZE_DND and
> GtkCellRendererPixbuf because it doesn't matter if you use the HiDpi patches or
> not. Investigating ...

Not a regression then, that's good.

The GtkCellRendererPixbuf:stock-size is now causing the pixbufs to be scaled. That was not the case before.

We were setting stock-size to DND (ie. 32 pixels) so that emblems like "changes throughout the day" are rendered at 16 pixels, but that did not affect the size of the thumbnails. See bug 682123

This gtk+ commit is causing the tiny thumbnails:

commit f5d7e54d33ce72ba1cdcb2d4cd75ef61e946504a
Author: Matthias Clasen <mclasen@redhat.com>
Date:   Fri Jun 20 18:34:06 2014 -0400

    GtkCellRendererPixbuf: Set force_scale_pixbuf
    
    The recent icon theme scaling changes make the code more
    sensitive to mis-sized icons (e.g. application icons in
    the app chooser). A single row whose size gets blown out
    of proportion by a big icon is never wanted in a list.
    We can avoid this situation by telling GtkIconHelper to
    force-scale the pixbuf to the requested size.

However, the fix for bug 682123 has also been broken.

Created attachment 279601 [details] [review]
background: Composite the emblems ourselves

Created attachment 279602 [details] [review]
background: Clean up uses of GIcon

Created attachment 279603 [details] [review]
background: Composite the emblems ourselves

Fix typo in commit message.

Review of attachment 279601 [details] [review]:

Looks good otherwise.

::: panels/background/cc-background-item.c
@@ +101,3 @@
+        retval = g_object_ref (pixbuf);
+
+        icon = g_themed_icon_new ("slideshow-emblem");

You should cache this at the class level instead, so that we only need to get the slideshow-emblem pixbuf once.

Review of attachment 279602 [details] [review]:

That's a bit gratuitous, as a GdkPixbuf is a GIcon, but sure.

Review of attachment 279603 [details] [review]:

::: panels/background/cc-background-item.c
@@ +101,3 @@
+        retval = g_object_ref (pixbuf);
+
+        icon = g_themed_icon_new ("slideshow-emblem");

Ditto the previous review, the emblem pixbuf should be gotten once at the class level.

(In reply to comment #8)
> Review of attachment 279601 [details] [review]:
> 
> Looks good otherwise.
> 
> ::: panels/background/cc-background-item.c
> @@ +101,3 @@
> +        retval = g_object_ref (pixbuf);
> +
> +        icon = g_themed_icon_new ("slideshow-emblem");
> 
> You should cache this at the class level instead, so that we only need to get
> the slideshow-emblem pixbuf once.

Good idea. I have now used a static global to cache the pixbuf.

Created attachment 279745 [details] [review]
background: Composite the emblems ourselves

Review of attachment 279745 [details] [review]:

::: panels/background/cc-background-item.c
@@ +83,3 @@
 G_DEFINE_TYPE (CcBackgroundItem, cc_background_item, G_TYPE_OBJECT)
 
+static GdkPixbuf *slideshow_emblem = NULL;

Use a class-wide variable, not a static one, please.

Make the CcBackgroundItem struct private (in the C file), add a GdkPixbuf *member which you'd set in class_init(), and free in class_finalize.

(In reply to comment #13)
> Review of attachment 279745 [details] [review]:
> 
> ::: panels/background/cc-background-item.c
> @@ +83,3 @@
>  G_DEFINE_TYPE (CcBackgroundItem, cc_background_item, G_TYPE_OBJECT)
> 
> +static GdkPixbuf *slideshow_emblem = NULL;
> 
> Use a class-wide variable, not a static one, please.
> 
> Make the CcBackgroundItem struct private (in the C file), add a GdkPixbuf
> *member which you'd set in class_init(), and free in class_finalize.

You mean, add a member to CcBackgroundItemClass, right?  Each instance of the type will have a separate CcBackgroundItem struct.

Isn't it so that class_finalize is only used for dynamic types since static types are never destroyed? Declaring a cc_background_panel_class_finalize resulted in a 'defined but not used' warning, and a cursory reading of the GLib sources seems to indicate this? Or am I mistaken?

*** Bug 732845 has been marked as a duplicate of this bug. ***

Review of attachment 279745 [details] [review]:

::: panels/background/bg-source.c
@@ +158,3 @@
   priv = self->priv = SOURCE_PRIVATE (self);
 
+  priv->store = gtk_list_store_new (3, GDK_TYPE_PIXBUF, G_TYPE_OBJECT, G_TYPE_STRING);

We need to use Cairo surfaces instead of GdkPixbufs. Otherwise on HiDpi displays this will get scaled up to cover 512 application pixels instead of 256. This is because we hit this code path in ensure_surface_from_pixbuf: https://git.gnome.org/browse/gtk+/tree/gtk/gtkiconhelper.c#n583

It would have worked if 'scale' got set to 2 instead of 1, but such is life.

Created attachment 280123 [details] [review]
background: Composite the emblems ourselves

Using Cairo surfaces.

Created attachment 280723 [details] [review]
background: Composite the emblems ourselves

We were assuming that setting stock-size would affect the emblems in
GEmblemedIcons, but not the icons themselves. This is a bit weird.
GtkCellRendererPixbuf:gicon is meant to work with
GtkCellRendererPixbuf:stock-size, and this was only working so far
because GTK_ICON_LOOKUP_FORCE_SIZE was not being used when loading
the icon.

Lets composite the emblems ourselves so that we can use
GtkCellRendererPixbuf:pixbuf, which is unaffected by stock-size.

Also, as the GTK+ icon helpers will double the size of the preview
icons, don't bother doubling the default size of the icons.

This last patch is a modification of attachment 279745 [details] [review]. I fixed the double-doubling when using hi-dpi, letting the GTK+ icon code handle it, and fixed the memory management of the emblem icon.

You were correct about the class finalize only being used for dynamic types.

Review of attachment 280723 [details] [review]:

Unfortunately, the last patch was more correct than this one.

::: panels/background/bg-source.c
@@ -62,3 @@
-      priv->thumbnail_height *= scale_factor;
-      priv->thumbnail_width *= scale_factor;
-    }

This is wrong. We really should be creating pixbufs that have double the number of pixels. A pixbuf that has 512 pixels will look much better on a HiDpi display than a pixbuf that has 256 pixels but has been scaled up, even though both will appear of have the same size. In the latter case, there won't be any difference in the picture quality from a LoDpi display, which defeats the point of having a HiDpi display to begin with.

@@ +147,3 @@
   priv = self->priv = SOURCE_PRIVATE (self);
 
+  priv->store = gtk_list_store_new (3, GDK_TYPE_PIXBUF, G_TYPE_OBJECT, G_TYPE_STRING);

... We should be using a Cairo surface here because unlike a GdkPixbuf it knows about the scale factor.

Incidentally this is also what gnome-documents and gnome-photos does.

(In reply to comment #20)
> Review of attachment 280723 [details] [review]:
> 
> Unfortunately, the last patch was more correct than this one.
> 
> ::: panels/background/bg-source.c
> @@ -62,3 @@
> -      priv->thumbnail_height *= scale_factor;
> -      priv->thumbnail_width *= scale_factor;
> -    }
> 
> This is wrong. We really should be creating pixbufs that have double the number
> of pixels. A pixbuf that has 512 pixels will look much better on a HiDpi
> display than a pixbuf that has 256 pixels but has been scaled up, even though
> both will appear of have the same size. In the latter case, there won't be any
> difference in the picture quality from a LoDpi display, which defeats the point
> of having a HiDpi display to begin with.

Right.

> @@ +147,3 @@
>    priv = self->priv = SOURCE_PRIVATE (self);
> 
> +  priv->store = gtk_list_store_new (3, GDK_TYPE_PIXBUF, G_TYPE_OBJECT,
> G_TYPE_STRING);
> 
> ... We should be using a Cairo surface here because unlike a GdkPixbuf it knows
> about the scale factor.
> 
> Incidentally this is also what gnome-documents and gnome-photos does.

OK, I'll wait on your version of the last patch with the memory management for the emblem fixed.

Created attachment 280743 [details] [review]
background: Composite the emblems ourselves

Keep track of the references to the cached slideshow_emblem, and use a weak pointer to set it to NULL when the last reference is dropped, as done by Bastien in his last patch.

I slightly modified it so that each CcBackgroundItem explicitly keeps track of its reference to the cache. Otherwise if an item is destroyed before it had any reference to the cache, or if cc_background_item_get_frame_thumbnail was called multiple times on an item, then we would have a memory error.

Review of attachment 280743 [details] [review]:

"Let's"

Looks fine to commit other than that.

::: panels/background/bg-colors-source.c
@@ +79,3 @@
       GIcon *pixbuf;
+      cairo_surface_t *surface;
+      gint scale_factor;

Use int.

::: panels/background/bg-pictures-source.c
@@ +153,3 @@
   GtkListStore *store;
+  cairo_surface_t *surface = NULL;
+  gint scale_factor;

int.

::: panels/background/cc-background-item.c
@@ +128,3 @@
+        }
+
+        eh = gdk_pixbuf_get_height (slideshow_emblem);

What's the point of setting the item->priv->slideshow_emblem struct member if you're not going to use it?

@@ +135,3 @@
+        y = h - eh;
+
+        gdk_pixbuf_composite (slideshow_emblem, pixbuf, x, y, ew, eh, x, y, 1.0, 1.0, GDK_INTERP_BILINEAR, 255);

Ditto.

(In reply to comment #23)
> Review of attachment 280743 [details] [review]:
> 
> "Let's"

Ok.

> Looks fine to commit other than that.
> 
> ::: panels/background/bg-colors-source.c
> @@ +79,3 @@
>        GIcon *pixbuf;
> +      cairo_surface_t *surface;
> +      gint scale_factor;
> 
> Use int.

Ok.

> ::: panels/background/bg-pictures-source.c
> @@ +153,3 @@
>    GtkListStore *store;
> +  cairo_surface_t *surface = NULL;
> +  gint scale_factor;
> 
> int.

Ok.

> ::: panels/background/cc-background-item.c
> @@ +128,3 @@
> +        }
> +
> +        eh = gdk_pixbuf_get_height (slideshow_emblem);
> 
> What's the point of setting the item->priv->slideshow_emblem struct member if
> you're not going to use it?

priv->slideshow_emblem keeps track of an item's reference to the crash. Otherwise if an item is destroyed before it had any reference to the cache, or if cc_background_item_get_frame_thumbnail was called multiple times on an item, then we would have a memory error.

(In reply to comment #24)
> priv->slideshow_emblem keeps track of an item's reference to the crash.

s/crash/cache/

Created attachment 280800 [details] [review]
background: Composite the emblems ourselves

Created attachment 280803 [details] [review]
background: Composite the emblems ourselves

(In reply to comment #24)
> > What's the point of setting the item->priv->slideshow_emblem struct member if
> > you're not going to use it?
> 
> priv->slideshow_emblem keeps track of an item's reference to the crash.
> Otherwise if an item is destroyed before it had any reference to the cache, or
> if cc_background_item_get_frame_thumbnail was called multiple times on an item,
> then we would have a memory error.

Right, that'd be why it was done in _init() in my version of the patch. Looks fine either way.

(In reply to comment #28)
> (In reply to comment #24)
> > > What's the point of setting the item->priv->slideshow_emblem struct member if
> > > you're not going to use it?
> > 
> > priv->slideshow_emblem keeps track of an item's reference to the crash.
> > Otherwise if an item is destroyed before it had any reference to the cache, or
> > if cc_background_item_get_frame_thumbnail was called multiple times on an item,
> > then we would have a memory error.
> 
> Right, that'd be why it was done in _init() in my version of the patch. Looks
> fine either way.

Oh, yes, I had misread your patch. Sorry about that.

Now that I tried to put it in _init I think that it is not as robust as using a private struct member. eg., if gtk_icon_info_load_icon failed for the first item, but worked for the second, then we will get the reference count wrong. However I don't know if that is a realistic possibility. Maybe, in the middle of an update?

Your version looks fine to me.

Created attachment 283234 [details] [review]
background: Clean up uses of GIcon

Rebased against current master.