Created attachment 270264 [details] [review]
allow enter to activate first search item

Review of attachment 270264 [details] [review]:

Thanks for the patch, Pranav. Great job! It looks good, apart from a few style issues. What happens if the user hits enter when the search entry is empty?

::: src/photos-embed.c
@@ +611,3 @@
+photos_embed_activate_result (PhotosEmbed *self)
+{
+  GtkTreeModel *model = GTK_TREE_MODEL(photos_view_container_get_model(PHOTOS_VIEW_CONTAINER(self->priv->search)));

We try to avoid such long statements. Please use the pattern followed elsewhere in the code:
 - the first variable to be defined is "PhotosEmbedPrivate *priv = self->priv;", and then use priv->... instead of self->priv in the rest of the code
 - don't put function calls in the definition

Also, put a space before opening parentheses.

@@ +613,3 @@
+  GtkTreeModel *model = GTK_TREE_MODEL(photos_view_container_get_model(PHOTOS_VIEW_CONTAINER(self->priv->search)));
+  GtkTreeIter iter;
+  gchar *value;

Please insert a newline between the variable definitions and the rest of the function.

@@ +614,3 @@
+  GtkTreeIter iter;
+  gchar *value;
+  if( gtk_tree_model_get_iter_first(model, &iter)) {

We follow the GNU coding style. So the opening brace should be on the next line, and we put the space before the opening parenthesis, not after it. See the rest of the code for examples.

@@ +726,3 @@
 
+  PhotosMainToolbar *toolbar = photos_embed_get_main_toolbar (self);
+  PhotosSearchbar *searchbar = photos_main_toolbar_get_searchbar (toolbar);

For consistency with rest of the code, define variables only at the beginning of a block. Secondly, you can just use priv->toolbar instead of using photos_embed_get_main_toolbar. Third, can't we move this chunk higher up in the function where priv->toolbar is created?

::: src/photos-overview-searchbar.c
@@ +410,3 @@
                                                         G_PARAM_CONSTRUCT_ONLY | G_PARAM_WRITABLE));
+
+  g_signal_new("activate-result",

Please put a space before the opening parenthesis.

@@ +418,3 @@
+               g_cclosure_marshal_VOID__VOID,
+               G_TYPE_NONE,
+               0);

This signal should be part of PhotosSearchbar, not PhotosOverviewSearchbar.

Also, please use the same pattern that is used elsewhere in the code for adding signals:
 - add a function pointer in the class structure and use G_STRUCT_OFFSET (...)
 - use a enums and a signals array

See photos-base-item.[ch] for an example.

::: src/photos-searchbar.c
@@ +140,3 @@
+  else if (event->keyval == GDK_KEY_Return)
+    {
+      g_signal_emit_by_name (self, "activate-result");

Use g_signal_emit for consistency with the rest of the code.

Created attachment 270291 [details] [review]
allow enter to activate first search item

> What happens if the user hits enter when the search entry is empty?

Hitting enter on an empty search box still opens the first item that is being shown in the View container.


Patch fixed as per Comment 2

Review of attachment 270291 [details] [review]:

Great job! This is much better, apart from some cosmetic issues that I have pointed out below.

However, I have some concerns about the "hitting enter in an empty search entry" case. In my testing it works if you were in the overview or "photos" mode. However, say, if you were in albums or favorites, then it picks the wrong item on hitting enter because the first item in priv->search is not the same as the first item in priv->collections or priv->favorites.

One way to solve it would be to pick the view container corresponding to the current mode instead of hard coding priv->search. We already have some code in 'photos_embed_item_load' that does this. I would suggest splitting that code out into a separate 'photos_embed_get_view_container_from_mode' function and using it.

::: src/photos-embed.c
@@ +2,3 @@
  * Photos - access, organize and share your photos on GNOME
  * Copyright © 2012, 2013, 2014 Red Hat, Inc.
+ * Copyright © 2014 Pranav Kant

Put it above "Red Hat" so that it is in alphabetical order.

@@ +620,3 @@
+
+  priv = self->priv;
+  store = photos_view_container_get_model (PHOTOS_VIEW_CONTAINER(priv->search));

Missing space before opening parenthesis.

@@ +621,3 @@
+  priv = self->priv;
+  store = photos_view_container_get_model (PHOTOS_VIEW_CONTAINER(priv->search));
+  model = GTK_TREE_MODEL (store);

No need for a separate 'model' variable. Just do the cast when calling gtk_tree_model. This is a common pattern in GObject using C code where we cast the instance to the particular parent class whose method we are calling.

@@ +627,3 @@
+      gtk_tree_model_get (model, &iter, PHOTOS_VIEW_MODEL_URN, &value, -1);
+      obj = photos_base_manager_get_object_by_id (self->priv->item_mngr, value);
+      photos_base_manager_set_active_object (self->priv->item_mngr, obj);

Use "priv->..." instead of "self->priv->..."

@@ +632,1 @@
 

Function definitions should be separated by 2 newlines.

@@ +638,3 @@
   PhotosSearchContextState *state;
   gboolean querying;
+  PhotosSearchbar *searchbar;

Put this above "PhotosSearchContextState *state;" so that things are in alphabetical order.

::: src/photos-main-toolbar.c
@@ +827,3 @@
   photos_header_bar_set_stack (PHOTOS_HEADER_BAR (self->priv->toolbar), stack);
 }
+

Function definitions should be separated by 2 newlines.

::: src/photos-main-toolbar.h
@@ +28,2 @@
 #include <gtk/gtk.h>
+#include "photos-searchbar.h"

Library headers and private headers are separated by a newline. Put a newline above this line.

::: src/photos-searchbar.c
@@ +2,3 @@
  * Photos - access, organize and share your photos on GNOME
  * Copyright © 2014 Red Hat, Inc.
+ * Copyright © 2014 Pranav Kant

Put it above "Red Hat" so that it is in alphabetical order.

@@ +51,3 @@
+};
+
+static guint signals[LAST_SIGNAL] = { 0 };

Put another newline below this. We use 2 newlines to separate function definitions, components of GObject class definition boiler plate, etc..

Created attachment 270362 [details] [review]
allow enter to activate first search item

Review of attachment 270362 [details] [review]:

::: src/photos-embed.c
@@ +162,3 @@
+photos_embed_set_view_container (PhotosEmbedPrivate *priv,
+                                 PhotosWindowMode mode,
+                                 GtkWidget **view_container)

It should be photos_embed_get_view_container, and you do not pass PhotosEmbedPrivate to it. You only pass 'PhotosEmbed *self' See the rest of the code for examples.

@@ +643,1 @@
 

Function definitions should be separated by 2 newlines, not 1.

Created attachment 270390 [details] [review]
allow enter to activate first search item

Review of attachment 270390 [details] [review]:

::: src/photos-embed.c
@@ +640,3 @@
+  if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (store), &iter))
+    {
+      gtk_tree_model_get (GTK_TREE_MODEL (store), &iter, PHOTOS_VIEW_MODEL_URN, &value, -1);

Forgot to mention before, but you need to g_free value.

Created attachment 270402 [details] [review]
searchbar: Allow enter to activate first search item

I fixed the previous patch and committed this.

Thanks for you work, Pranav.