Created attachment 349339 [details] [review]
view-container: Display titles for collections

Review of attachment 349339 [details] [review]:

Thanks for the patch, Umang.

::: src/photos-view-container.c
@@ +146,3 @@
+
+      if  (self->mode == PHOTOS_WINDOW_MODE_COLLECTIONS)
+        gd_main_box_set_show_primary_text (GD_MAIN_BOX (self->view), TRUE);

This is a bit more involved. Doing just this means that collections will have a title in "Albums", but not when searching; and that individual images will also have titles when browsing the contents of a collection.

What we need to do is:

(a) Ensure that PhotosBaseItems have a primary-text only for collections. See photos_base_item_main_box_item_get_primary_text

(b) Enable show-primary-text for all the GdMainBoxes.

(c) Fix the theming and layout of GdMainIconBoxChild. I expect some roughness because the show-primary-text=TRUE code paths were not excercised so far.

Created attachment 349517 [details] [review]
Display titles in album grid

Fixes (a) and partially (b)

As in (b), when we try to enable primary-text for all GdMainBoxes , there's a empty space left on image thumbs (overview mode). That's because a GtkBox is added inside the grid (refer to libgd patch attached next).

Created attachment 349518 [details] [review]
GdMainIconBoxChild: Fix layout of primary and secondary text

Add a GtkBox which will contain primary and secondary text, into the Grid.

Designers might step in for more visual fixes (border-width of GtkBox or padding?)

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

Review of attachment 349517 [details] [review]:

::: src/photos-base-item.c
@@ +699,3 @@
 
+  priv = photos_base_item_get_instance_private (self);
+  g_return_val_if_fail (priv->collection, NULL);

We also need to change the PROP_PRIMARY_TEXT case in photos_base_item_get_property. It can no longer return priv->name directly. It needs to return the value from this method, instead.

Secondly, g_return_* are assertions to be used to guard against programming errors, which should never happen. While I agree that want to show the title only for collections, it should still be possible to access GdMainBoxItem:primary-text for non-collections without hitting an assertion. We cannot assume that not showing the title means that the property will never be accessed for any other purpose.

Created attachment 357504 [details] [review]
base-item, view-container: Display names only for collections

I took the liberty to fix it up.

Review of attachment 349518 [details] [review]:

::: libgd/gd-main-icon-box-child.c
@@ +206,3 @@
+      GtkWidget *box;
+
+      box = gtk_box_new (GTK_ORIENTATION_VERTICAL, 0);

Umm... I don't understand this. In comment 3 you seemed to imply that the presence of a GtkBox was the cause of extra padding along the bottom edge, but now you are actually adding a GtkBox.

I think we should simply hide the GtkLabels if there is no label to shown in them.

Created attachment 357506 [details] [review]
main-icon-box-child: Show the labels only if there's some text

Something like this might be better. What do you think?

> Created attachment 357506 [details] [review] [review]
> main-icon-box-child: Show the labels only if there's some text
> 
> Something like this might be better. What do you think?

This looks good.

Also, doesn't it make sense to bind "show-primary-text" property to the "visible" property of the primary_label?

(In reply to Umang Jain from comment #10)
> Also, doesn't it make sense to bind "show-primary-text" property to the
> "visible" property of the primary_label?

The problem is that of scalability. The GtkList/FlowBox widgets create as many row/grid widgets as there are items. So, if there are a thousand items to be displayed, it will create thousand of them. Having so many widgets is already a problem - it stretches GTK+ to its limit, hits memory limits, etc..

Hence, it is better not to have unused widgets lying around. That's why we re-create the entire widget without the GtkLabels whenever show-primary-text changes. Recreating the layout like that has a cost, but show-primary-text is not expected to change 20 times a second, so it shouldn't be a disaster.

In fact, you have now given me a better idea. Instead of hiding the GtkLabel when the primary-text becomes NULL or "", I think we should simply destroy the GtkLabel. Just like we do for show-primary-text.

Do you want to work on it, and modify my patch in that direction?

Created attachment 357878 [details] [review]
base-item, view-container: Display names only for collections