Created attachment 346380 [details] [review]
Add GdMainIconBoxIcon

Created attachment 346381 [details] [review]
main-icon-box-child: Make the icon expand horizontally

Created attachment 346382 [details] [review]
main-icon-box-child: Remove the margin-bottom from the GtkOverlay

Created attachment 346535 [details] [review]
Add GdMainIconBoxIcon

Factor out some common code in _get_preferred_height/width.

Review of attachment 346535 [details] [review]:

This approach looks reasonable to me, but I think that we should implement a very simple optimization.
Right now, as far as I can see, every time the widget is drawn with an allocation different than the original size of the surface, we create a scaled copy of the surface.
Instead, we should be caching the scaled copy and just draw it until a new size is allocated, at which point we would invalidate the cache.

::: libgd/gd-main-icon-box-icon.c
@@ +205,3 @@
+  GdMainIconBoxIcon *self = GD_MAIN_ICON_BOX_ICON (obj);
+
+  G_OBJECT_CLASS (gd_main_icon_box_icon_parent_class)->constructed (obj);

Remove this unused method

Review of attachment 346382 [details] [review]:

OK

Review of attachment 346381 [details] [review]:

Looks good.

(In reply to Cosimo Cecchi from comment #5)
> Review of attachment 346535 [details] [review] [review]:

Thanks, Cosimo!

> This approach looks reasonable to me, but I think that we should implement a
> very simple optimization.
> Right now, as far as I can see, every time the widget is drawn with an
> allocation different than the original size of the surface, we create a
> scaled copy of the surface.
> Instead, we should be caching the scaled copy and just draw it until a new
> size is allocated, at which point we would invalidate the cache.

Yeah, I am concerned about the repeated scaling too.

However, caching the scaled surface would double the memory footprint of each tile. Given that GtkFlowBox and GtkListBox don't limit the number of widgets, and assuming, say, 800 tiles with 512x512 thumbnails each, and 3 bytes per pixel, it adds up to:
  800 * 512 * 512 * 3 * 2 / (1024 * 1024) = 1200 MB

Also note that the application keeps another copy of the non-emblemed thumbnail. That increases it to 1800 MB. That's a lot, no?

Then I thought about only keeping the original dimensions and the scaled surface around. However, I was worried that repeatedly scaling a scaled buffer would degrade the quality of the thumbnails.

That's why I decided to settle for scaling on demand.

But I don't know which trade-off to pick. If you think caching the scaled surface is fine, I will be happy to implement it.

(In reply to Debarshi Ray from comment #8)
> Yeah, I am concerned about the repeated scaling too.
> 
> However, caching the scaled surface would double the memory footprint of
> each tile. Given that GtkFlowBox and GtkListBox don't limit the number of
> widgets, and assuming, say, 800 tiles with 512x512 thumbnails each, and 3
> bytes per pixel, it adds up to:
>   800 * 512 * 512 * 3 * 2 / (1024 * 1024) = 1200 MB
> 
> Also note that the application keeps another copy of the non-emblemed
> thumbnail. That increases it to 1800 MB. That's a lot, no?
> 
> Then I thought about only keeping the original dimensions and the scaled
> surface around. However, I was worried that repeatedly scaling a scaled
> buffer would degrade the quality of the thumbnails.
> 
> That's why I decided to settle for scaling on demand.
> 
> But I don't know which trade-off to pick. If you think caching the scaled
> surface is fine, I will be happy to implement it.

800 tiles are quite a large number; it feels very unlikely that there would be 800 tiles visible on the screen at a given time, so a simple optimization here would be not to keep in memory tiles that are scrolled out of the view.

But in general, yes that's a lot of memory; why do we need to keep the non-emblemed copy of the thumbnail in memory too? It feels to me that it would be more useful to keep the scaled one in memory instead.

Another way of implementing this could be avoiding to keep the original unscaled surface always in memory, and re-load it from file when needed; that way the "usual" case (scrolling around, moving from backdrop to front focus) is optimized and cached, and the "resize" case takes the performance hit of having to reload/scale pixels.

Created attachment 346616 [details] [review]
Add GdMainIconBoxIcon

Cache the zoomed surface in size_allocate and remove the empty constructed vfunc.

(In reply to Cosimo Cecchi from comment #9)

Ok, I updated the patch to cache the zoomed surface.

> 800 tiles are quite a large number; it feels very unlikely that there would
> be 800 tiles visible on the screen at a given time, so a simple optimization
> here would be not to keep in memory tiles that are scrolled out of the view.

You are right. Sadly, I don't know how the GListModel can track the visible items. It seems that it will need some co-operation from the widget. Plus, GtkFlowBox is not a GtkScrollable at this point, which makes it harder. Implementing GtkScrollable is also a necessity to optimize the widget itself.

So this doesn't look like an optimization we can do in the immediate short-term.

> But in general, yes that's a lot of memory; why do we need to keep the
> non-emblemed copy of the thumbnail in memory too? It feels to me that it
> would be more useful to keep the scaled one in memory instead.

Well, that's just the way the gnome-documents/photos thumbnailing code is at the moment. They keep a non-emblemed GdkPixbuf and an emblemed surface for each item.

We can make the applications smarter, but I am hoping to avoid too much code churn for 3.24.

> Another way of implementing this could be avoiding to keep the original
> unscaled surface always in memory, and re-load it from file when needed;
> that way the "usual" case (scrolling around, moving from backdrop to front
> focus) is optimized and cached, and the "resize" case takes the performance
> hit of having to reload/scale pixels.

Yes, you gave me an idea. :)

How about passing the GdMainBoxItem, and not the Cairo surface, to the GdMainIconBoxIcon widget? The widget can call gd_main_box_item_get_icon whenever it needs the original icon - in size_allocate, and when the icon itself changes, and it will continue to cache the zoomed icon.

Then, in due course, we can make the applications smarter to avoid keeping the original surface in memory. One option is to only keep the non-emblemed GdkPixbuf, and apply the emblems on the fly.

Yeah, I like that approach! I agree that any other larger refactor, both on GListModel and in the apps themselves, is too much for 3.24 at this point.

Created attachment 346699 [details] [review]
Add GdMainIconBoxIcon

Created attachment 346700 [details] [review]
main-icon-box-child: Make the icon expand horizontally

Review of attachment 346699 [details] [review]:

::: libgd/gd-main-icon-box-icon.c
@@ +62,3 @@
+
+  if (surface == NULL)
+    goto out;

This is always called with surface != NULL (the only caller checks beforehand).
So I would either remove the check or make it an assertion.

@@ +179,3 @@
+  GTK_WIDGET_CLASS (gd_main_icon_box_icon_parent_class)->size_allocate (widget, allocation);
+
+  g_clear_pointer (&self->surface_zoomed, (GDestroyNotify) cairo_surface_destroy);

Should you not clear this only when the new size is different?

@@ +325,3 @@
+  return g_object_new (GD_TYPE_MAIN_ICON_BOX_ICON, "item", item, NULL);
+}
+

Remove empty line

Review of attachment 346700 [details] [review]:

OK

(In reply to Cosimo Cecchi from comment #15)
> Review of attachment 346699 [details] [review] [review]:
> 
> ::: libgd/gd-main-icon-box-icon.c
> @@ +62,3 @@
> +
> +  if (surface == NULL)
> +    goto out;
> 
> This is always called with surface != NULL (the only caller checks
> beforehand). So I would either remove the check or make it an assertion.

Ok. I changed it to a g_return_val_if_fail assertion.

(I had added a NULL-check so that the function could be factored out for wider use in future.)

> 
> @@ +179,3 @@
> +  GTK_WIDGET_CLASS (gd_main_icon_box_icon_parent_class)->size_allocate
> (widget, allocation);
> +
> +  g_clear_pointer (&self->surface_zoomed, (GDestroyNotify)
> cairo_surface_destroy);
> 
> Should you not clear this only when the new size is different?

True. I wasn't sure how often size_allocate might be called without the size actually changing. So I decided to always re-create a new surface because it made the code marginally simpler.

I have changed it now.

Created attachment 346985 [details] [review]
Add GdMainIconBoxIcon

I also changed self->height_zoomed_scaled and self->width_zoomed_scaled to be local variables since we don't need them to across the entire instance.

Review of attachment 346985 [details] [review]:

Feel free to push with this change.

::: libgd/gd-main-icon-box-icon.c
@@ +85,3 @@
+  cairo_destroy (cr);
+
+ out:

Remove this label

(In reply to Cosimo Cecchi from comment #19)
> Review of attachment 346985 [details] [review] [review]:
> 
> Feel free to push with this change.
> 
> ::: libgd/gd-main-icon-box-icon.c
> @@ +85,3 @@
> +  cairo_destroy (cr);
> +
> + out:
> 
> Remove this label

Eek! I am embarrassed. Somehow I don't get some compiler warnings from the libgd submodule, which I'd otherwise get when building the application. Could be something wrong with the setup of the buildsystem.

Another example of this is commit f03788bac621c6e45a5ecfb5421d93487b57d040 :(

Created attachment 347028 [details] [review]
Add GdMainIconBoxIcon

Pushed after removing the unused label.

Thanks for all the reviews at such short notice.