The search button should be insensitive too, I suppose.

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

Created attachment 270948 [details] [review]
Work in progress

Please review this and let me know what you think of it. This is not yet complete. I am having trouble handling the case when the app starts. Since, the toolbar is loaded even before images are retrieved and there is no change in mode between the app starting and the images getting loaded the relevant buttons remain inactive even though they shouldn't.

Review of attachment 270948 [details] [review]:

Thanks for working on this Saurav!

::: src/photos-embed.c
@@ +605,3 @@
     photos_embed_prepare_for_preview (self);
+
+  PhotosViewContainer *container = PHOTOS_VIEW_CONTAINER (photos_embed_get_view_container_from_mode (self, mode));

Don't define variables in the middle of a block. They should go to the top. See the rest of the code for examples. I would prefer if you made it 'GtkWidget *view_container' for the sake of consistency with the rest of the file.

This will crash when you try to move to the preview because it does not have view_container associated with it. I would suggest that you rearrange the if-else-if block above to accommodate this. eg., if (mode == PREVIEW) {} else { if (mode === COLLECTIONS) {} ... set_view_model ()}

@@ +608,3 @@
+
+  photos_main_toolbar_set_view_model (PHOTOS_MAIN_TOOLBAR (self->priv->toolbar),
+                                      PHOTOS_VIEW_MODEL   (photos_view_container_get_model (container)));

There is a (hidden?) problem with this. Both PhotosEmbed and PhotosMainToolbar react to the 'window-mode-changed' signal. The order in which the signal handlers are called is undefined. So we need to make sure that it works, irrespective of the order.

::: src/photos-main-toolbar.c
@@ +835,3 @@
 }
+
+void photos_main_toolbar_set_view_model (PhotosMainToolbar *self, PhotosViewModel *model)

Function definitions are separated by two newlines, not one; and the return type should be on a line of its own.

@@ +845,3 @@
+
+    if (!g_strcmp0 ("Search", tooltip) || !g_strcmp0 ("Select Items", tooltip))
+    {

Don't use the tooltip for this. Those strings are marked for translation, so there is no guarantee that they are going to be "Search" or "Select Items" on every system.

Add 2 members -- search_button and selection_button the MainToolbarPrivate struct, and use them for this. Remember to destroy and set them to NULL in photos_main_toolbar_clear_state_data.

@@ +850,3 @@
+      else
+        gtk_widget_set_sensitive (GTK_WIDGET (iter->data), TRUE);
+    }

Please use a "> 0" comparison instead of "!". As a convention we only use it for gboolean types, while in this case we get a gint. Also, as a pathological case if it returns a negative value then it will count as true.

Created attachment 271108 [details] [review]
Changes the patch as per the suggestions

Thanks for the review.

I have made the mentioned changes to the patch. I have also added a signal handler to item_mngr in photos-embed.c to handle the following cases
1) If there are no photos, the buttons are insensitive. If a photo is added while the app in open, it automatically gets added into the 'Recent' view. However, since there is no "window-mode-changed" involved, the buttons will continue to remain insensitive while they shouldn't. Adding signal handler to item_mngr will call back photos_main_toolbar_set_view_model () to check the model again and update accordingly.

2) Similar case as (1) except this time the user deletes the photos resulting in no photos remaining in 'Recent' view.

Please let me know what you think. Thanks.

Review of attachment 271108 [details] [review]:

Thanks for the patch Saurav! This is much better. A few comments below.

::: src/photos-embed.c
@@ +607,3 @@
+      model = photos_view_container_get_model (PHOTOS_VIEW_CONTAINER (view_container));
+      photos_main_toolbar_set_view_model (PHOTOS_MAIN_TOOLBAR (self->priv->toolbar),
+                                          PHOTOS_VIEW_MODEL   (model));

This could have been on the previous line. You are allowed to go beyond 80 characters here. See README.

@@ +655,3 @@
 static void
+photos_embed_item_number_changed_cb (PhotosEmbed *self)
+{

Define 'PhotosEmbedPrivate *priv = self->priv;' here and use it in the rest of the function.

@@ +774,3 @@
+                           G_CALLBACK (photos_embed_item_number_changed_cb),
+                           self,
+                           G_CONNECT_SWAPPED);

I appreciate your attention to detail here, but unfortunately this won't quite work. Remember that the ItemManager is a global singleton. Even if an item is removed from a view, it might be in ItemManager because some other view needs it. eg., I started with an empty favorites view, marked one item as favorite, saw it appear in the favorites view. The buttons behaved as expected. However when I unstar the item in the favorites view, the view becomes empty but the buttons remain sensitive.

So what you should do is connect to the row-deleted and row-inserted signals of each ViewModel separately.

::: src/photos-main-toolbar.c
@@ +839,3 @@
+void
+photos_main_toolbar_set_view_model (PhotosMainToolbar *self, PhotosViewModel *model)
+{

Define 'PhotosMainToolbarPrivate *priv = self->priv;' here and use it in the rest of the function.

@@ +849,3 @@
+      gtk_widget_set_sensitive (self->priv->search_button, TRUE);
+      gtk_widget_set_sensitive (self->priv->selection_button, TRUE);
+    }

I would suggest checking whether priv->search_button and priv->selection_button are non-NULL before doing this. I know that it is OK in its current form, but I can easily imagine someone making a mistake a few commits down the line that changes the logic and breaks the assumption.

Maybe you could keep the value of (gtk_tree_model_iter_n_children (GTK_TREE_MODEL (model), NULL) == 0) in a gboolean, and then check each button and set its sensitivity depending on the value of the gboolean.

Created attachment 271242 [details] [review]
Made the changes

Thanks for the review.

I have incorporated the changes mentioned.

Review of attachment 271242 [details] [review]:

Thanks for the patch, Saurav. Almost there.

The commit message could be improved. I think the summary should read something like "Selection and search should be disabled when there are no photos", which describes the overall objective of the patch. "Add photos_main_toolbar_set_view_model" does not say anything about why this patch is needed and what it is doing. Secondly, the text in the body is not really useful. It would have made sense in a GNU-style ChangeLog that was common in the days of CVS, where you had to list each and every change in the patch. These days we have Git, and it is very easy to see the mundane details with a 'git show'. The body is meant to elaborate on the summary if somethings are worth mentioning or are not obvious enough from the code.

Other than that, please do claim copyright on the two *.c files that you touched. See the top of photos-embed.c for an example.

::: src/photos-embed.c
@@ +588,3 @@
+  PhotosEmbed  *self = PHOTOS_EMBED (user_data);
+  GtkWidget    *view_container;
+  GtkListStore *model;

We don't align local variables like this in the rest of the code because it is hard to keep the alignment.

@@ +658,3 @@
+  GtkWidget          *view_container;
+  GtkListStore       *model;
+  PhotosWindowMode    mode;

Ditto.

::: src/photos-main-toolbar.c
@@ +55,3 @@
   GtkWidget *selection_menu;
   GtkWidget *toolbar;
+  GtkWidget *search_button;

I think we won't need search_button, after all. See below. We will still need selection_button, though.

@@ +843,3 @@
+  gboolean zero_children;
+
+  zero_children = (gtk_tree_model_iter_n_children (GTK_TREE_MODEL (model), NULL) == 0);

I think this is better:
g_simple_action_set_enabled (G_SIMPLE_ACTION (priv->search), !zero_children);

Instead of dealing with search_button, because it will also disable Ctrl+F from opening the searchbar.

@@ +850,3 @@
+        gtk_widget_set_sensitive (priv->search_button, FALSE);
+      if (priv->selection_button != NULL)
+        gtk_widget_set_sensitive (priv->selection_button, FALSE);

Just do 'gtk_widget_set_sensitive (priv->selection_button, !zero_children);'. It is simpler.

Created attachment 271556 [details] [review]
Reworked the patch

Thanks for the review.

I have added copyrights, changed the alignment, removed search_button, modified the commit message and also reworked photos_main_toolbar_set_view_model ().

Please let me know what you think of this patch.

Review of attachment 271556 [details] [review]:

Almost there.

::: src/photos-main-toolbar.c
@@ +840,3 @@
+
+  g_simple_action_set_enabled (G_SIMPLE_ACTION (priv->search), !zero_children);
+  gtk_widget_set_sensitive (priv->selection_button, !zero_children);

You lost the NULL check for priv->selection_button. We still need it because we are creating and destroying it during the lifetime of the toolbar, unlike priv->search.

Created attachment 271580 [details] [review]
Selection and search should be disabled when there are no photos

I restored the NULL check and tweaked the commit message a bit and committed it.

Thanks for your work on this Saurav!

Excellent. Thanks Saurav.

(In reply to comment #11)
> Created an attachment (id=271580) [details] [review]
> Selection and search should be disabled when there are no photos
> 
> I restored the NULL check and tweaked the commit message a bit and committed
> it.

Extremely sorry about that NULL check. I don't know how I missed that. 
Thanks for bearing with me throughout the process. Learnt quite a lot.