I would suggest to avoid duplicates that the list should case insensitive. Why? Because in English nationalities start with uppercase but in many other languages don't, so we may end with a  "thai"  and a  "Thai" cuisines that are actually the same.

Ekta will be looking into this

Created attachment 348007 [details] [review]
Offer existing cuisines in edit page

Some initial feedback:

My compiler complains:

  CC       gnome_recipes-gr-edit-page.o
gr-edit-page.c: In function ‘populate_cuisine_combo’:
gr-edit-page.c:395:58: error: passing argument 2 of ‘gr_cuisine_get_data’ from incompatible pointer type [-Werror=incompatible-pointer-types]
                 gr_cuisine_get_data (duplicate_names[i], &title[i], NULL, NULL);

Which hints at some memory management issues. I'll comment on that in the detailed review. After adding a cast there, I can make it build and it works somewhat, but I see a recipe like my Donauwellen show "German" as cuisine in the details view, and "American" in the edit view. Clearly, something is going wrong there...

Review of attachment 348007 [details] [review]:

::: src/gr-edit-page.c
@@ +388,3 @@
+        gtk_combo_box_text_remove_all (GTK_COMBO_BOX_TEXT (page->cuisine_combo));
+        names = gr_recipe_store_get_all_cuisines (store, &length);
+        duplicate_names = g_strdupv (names);

You are leaking the memory that is returned by g_strdupv() here. duplicate_names probably needs to be declared like this:

g_auto(GStrv) duplicate_names = NULL;

That will instruct gcc to call g_strfreev automatically for you when the leaving the scope of the function.

@@ +389,3 @@
+        names = gr_recipe_store_get_all_cuisines (store, &length);
+        duplicate_names = g_strdupv (names);
+        title = g_strdupv (names);

You are also leaking some things in the title array. g_strdupv() will duplicate each name, and then in the loop below, you overwrite title[i] with the string that gets returned by gr_cuisine_get_data, leaking the duplicated name that title[i] was pointing to before.

To get the memory management right here, you'll probably have to declare title like this:

g_autofree char **title = NULL

and allocate it like this:

title = g_new (char *, length + 1);  (+ 1 is for the terminating NULL)

The reason for using g_autofree instead of g_auto(GStrv) here is that gr_cuisine_get_data returns a const char * for the title, so you are not supposed to free it.

@@ +397,3 @@
+
+        g_qsort_with_data (title, length, sizeof (char *), compare_strings, NULL);
+

Here is where things go wrong wrt to the American / German example: You are sorting the title array, but the duplicate_names array is not kept in sync. After this calls, duplicate_names[i] and titles[i] will no longer 'match'.

::: src/gr-recipe-store.c
@@ +1193,3 @@
+                        g_hash_table_insert (cuisines,
+                                             (gpointer) (gr_recipe_get_cuisine (recipe)),
+                                             g_utf8_casefold (gr_recipe_get_cuisine (recipe), -1));

Here you are casefolding the value. I wonder if we don't want to use the case-folded string as the key, so we get treat "canadian" and Canadian" as the same value ? But then we would return the case-folded strings and loose their original case-ness. Complicated... In any case, if we are not case-folding the keys, there is no point in doing it for the values - nobody is looking at those anyway, and we're leaking the memory that g_utf8_casefold allocated for them.

Created attachment 348272 [details] [review]
Offer existing cuisines in edit page

Review of attachment 348272 [details] [review]:

I would split this into two patches: The first one just adds the new api to GrRecipeStore, the second one uses it in GrEditPage. The first patch needs to also add the new function to gr-recipe-store.h

::: src/gr-edit-page.c
@@ +367,3 @@
+        GArray *name;
+        GArray *duplicate_name;
+} Cuisine;

Looking at this patch, this struct is indeed not useful here. It is also not quite what I had in mind when we started discussing it as a solution to the 'parallel arrays in sorting' problem. It would be more like this:

typedef struct {
  char *name;     // what we store in the database
  char *sort_key; // what we sort it as
  char *title;    // what we display in the ui
} Cuisine;

static void clear_cuisine (gpointer data)
{
  Cuisine c = data;
  g_free (c->name);
  g_free (c->sort_key);
  g_free (c->title);
}

static int compare_cuisines (gconstpointer a, gconstpointer b)
{
  const Cuisine *ca = a;
  const Cuisine *cb = b;

  return strcmp (ca->sort_key, cb->sort_key);
}

cuisines = g_array_new (FALSE, TRUE, sizeof (Cuisine));
g_array_set_clear_func (cuisines, clear_cuisine);

Since we are not doing sorting here at the moment, we should just drop the struct.

@@ +2249,3 @@
                 g_set_object (&page->recipe, recipe);
         }
+        populate_cuisine_combo(page);

Please add a space before ( here

Created attachment 348297 [details] [review]
gr-recipe-store api to store and get all the cuisines

Created attachment 348298 [details] [review]
Populate cuisine combobox with all the cuisines

Review of attachment 348297 [details] [review]:

Looks good, anyway. Feel free to push it with or without these cosmetic fixes.

::: src/gr-recipe-store.c
@@ +1181,3 @@
+char **
+gr_recipe_store_get_all_cuisines (GrRecipeStore *self,
+                                  guint *length )

you could line up the parameters nicely here, like we do elsewhere:

        (GrRecipeStore *self,
         guint         *length)

@@ +1194,3 @@
+                        g_hash_table_insert (cuisines,
+                                             (gpointer) (gr_recipe_get_cuisine (recipe)),
+                                             (gpointer) (gr_recipe_get_cuisine (recipe)));

A shorter version of this is g_hash_table_add (cuisines, (gpointer) gr_recipe_get_cuisine (recipe));

Review of attachment 348298 [details] [review]:

::: src/gr-edit-page.c
@@ +389,1 @@
         }

I think you need to free the array somehow. But I also wonder about the use of the array here.
You put the names strings in there without duplicating them, and then g_strstrip modifies them in place, which possibly changes strings that are coming out of the store. I think you need to make a copy here at some point, probably when putting them into the array. You could do without the array by just working on a local copy of names[i] inside the loop, and free it when you're done:

  g_autofree char *temp_cuisine_name = g_strstrip (g_strdup (names[i]));
  ...

Created attachment 348897 [details] [review]
Populate cuisine combobox with all the cuisines

Attachment 348897 [details] pushed as 22248bf - Populate cuisine combobox with all the cuisines