I would be interested in storing the recipes inside a git repository. Would it be possible to design it so that multiple connectors can be added ?

As a stopgap measure, I've implemented share-by-email by directly calling out to evolution.

this is an outreachy project idea: https://wiki.gnome.org/Apps/Recipes/Contributing/Sharing

It appears this function calls specifically the Evolution email client. If the user does not have evolution then this does not work. Also setting an alias so that the call to evolution calls some other client dees not work either.the code should be rewritten so that the default email application is called.

You don't say which function you are talking about.

In any case:

$ git grep evolution

doesn't find any hits, so we are certainly not hardcoding evolution in git master.

Created attachment 357025 [details] [review]
Creates a dialog for sharing ingredeints to todoist

Created attachment 357026 [details] [review]
Send ingredients to todoist

Created attachment 357027 [details] [review]
Fix infinite loop introduced in previous commit

Review of attachment 357025 [details] [review]:

There's a typo in the commit message: ingredients. It would be good to explain a bit more what the dialog does, and also to mention that this commit introduces a new dependency, on goa.

::: src/gr-ingredients-exporter.c
@@ +92,3 @@
+		gtk_stack_set_visible_child_name (GTK_STACK (exporter->dialog_stack), "providers_box");
+		gtk_stack_set_visible_child_name (GTK_STACK (exporter->header_start_stack), "back");
+		gtk_header_bar_set_title (GTK_HEADER_BAR (exporter->header), "Add Account");

The string needs to be marked for translation: _("Add Account")

@@ +95,3 @@
+	}
+	else
+	// if (gtk_stack_get_visible_child (GTK_STACK (exporter->dialog_stack)) == exporter->providers_box)

if this condition is not needed after all, the commented-out line should be removed

@@ +100,3 @@
+		gtk_stack_set_visible_child (GTK_STACK (exporter->dialog_stack), exporter->accounts_box);
+		gtk_stack_set_visible_child_name (GTK_STACK (exporter->header_start_stack), "cancel_button");
+		gtk_header_bar_set_title (GTK_HEADER_BAR (exporter->header), "Export Ingredients");

The string needs to be marked for translation: _("Export Ingredient")

@@ +102,3 @@
+		gtk_header_bar_set_title (GTK_HEADER_BAR (exporter->header), "Export Ingredients");
+	}
+	//gtk_widget_set_visible (exporter->accounts_box, FALSE);

Same here: if not needed, remove

@@ +152,3 @@
+	    GoaAccount *account;
+	    GError *error;
+

Some indentation trouble here. And i would prefer to move the variable declaration to the beginning of the function (even though the compiler accepts this)

@@ +157,3 @@
+	    if (!client)
+	    {
+	    		g_error ("Could not create GoaClient: %s", error->message);

You should use g_warning here - g_error will abort the program, which is a bit harsh.

@@ +188,3 @@
+				        g_free (access_token);
+		      			}
+		      			g_print ("\tClientId: %s\n\tClientSecret: %s\n",

All the debug spew here should be removed, or alternatively, converted to g_debug(), which makes it not be printed by default, but still available if you run recipes with --verbose.

::: src/gr-recipe-exporter.c
@@ +18,3 @@
+     * You should have received a copy of the GNU General Public License
+     * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+     */

Somehow this comment got indented. No need for that...

@@ +85,3 @@
 gr_recipe_exporter_class_init (GrRecipeExporterClass *klass)
+{       
+        g_print("gr_recipe_exporter_class_init\n");

Debug spew should be removed thoughout this file.

::: src/gr-shopping-page.c
@@ +777,3 @@
+//         g_list_free_full (recipes, g_object_unref);
+//         g_list_free_full (items, item_free);
+// }

This function needs to be called if you click on the share by email item in the export dialog, I think ?

@@ +787,3 @@
+
+                window = gtk_widget_get_ancestor (GTK_WIDGET (page), GTK_TYPE_APPLICATION_WINDOW);
+                page->exporter = gr_ingredients_exporter_new(GTK_WINDOW (window));

Where does this object get freed ? You probably need to update finalize to free it if it is not NULL.

::: src/ingredients-exporter-dialog.ui
@@ +151,3 @@
+                  <child>
+                    <object class="GtkLabel" id="add_service">
+                      <property name="label">&lt;a href=&quot;&quot;&gt;Add service&lt;/a&gt;</property>

This is not ideal. Not only does it requires ugly escaping, but it also means that "Add service" is not translated. I would suggest to set this label in the code instead.

Review of attachment 357026 [details] [review]:

::: src/gr-ingredients-exporter.c
@@ +109,3 @@
+	 gr_shopping_page_populate (GR_SHOPPING_PAGE(shopping_page));
+	 exporter->ingredients = get_ingredients (GR_SHOPPING_PAGE(shopping_page));
+	g_print("inside get_ingredients_list\n");

Some missing spaces before ( here. And some debug spew

@@ +116,3 @@
 {
+		if (gtk_stack_get_visible_child (GTK_STACK (exporter->dialog_stack)) == exporter->accounts_box)
+		{

Our coding style generally puts the { on the end of the previous line.

@@ +173,3 @@
+		sync_token = json_object_get_string_member (object, "sync_token");
+		g_print("%s",sync_token);
+		exporter->sync_token;

This looks like it should be

exporter->sync_token = sync_token;

who is responsible for freeing this string ? We probably need to do it in finalize ? And do we need to worry about overwriting an existing sync_token here that we would need to free first ?

@@ +202,3 @@
+
+	    json_object_set_string_member (project, "type", "project_add");
+	    json_object_set_string_member (project, "args", "{\"name\": \"Project4\"}");

Project4 looks dubious as a hardcoded string.

@@ +287,3 @@
+	  	}
+
+  		accounts = goa_client_get_accounts (client);

How is the memory management here ? does goa_client_get_accounts returns a list that needs to be freed ? Do the individual objects in the list need to be unreffed ?

@@ +298,3 @@
+			if (g_strcmp0 (provider_type, "Todoist") == 0)
+		    {	
+		    	exporter->account_object = GOA_OBJECT(l->data);

If not, we probably need to ref this object here to ensure it stays alive ?

@@ +305,3 @@
+		    	return FALSE;
+		    	
+  		}

Quite a bit of messy formatting here.

@@ +331,3 @@
+    	g_print ("export the list here\n");
+    }
+    exporter->access_token = access_token;

Same question as for the sync_token: where is it freed ?

@@ +362,3 @@
+
+	    {
+	      g_clear_error (&error);

The error handling here needs some work - if the call fails, it probably doesn't make sense to continue with the rest of this function ?

@@ +377,3 @@
+		GList *l;
+		JsonArray *projects;
+		RestProxyCall *call2;

I would prefer variable declarations up top. Also, indentation trouble

@@ +394,3 @@
+		if (!json_parser_load_from_data (parser, payload, payload_length, &parse_error))
+		{
+		    g_clear_error (&parse_error);

Same here - we probably need to return here ?

@@ +399,3 @@
+		object = json_node_dup_object (json_parser_get_root (parser));
+		if (!object)
+			g_print("No Data found");

...and here

@@ +416,3 @@
+		    name = json_object_get_string_member (object, "name");
+		    g_print("%s" , name);
+		    if (strcmp (name, "Shopping List from Recipes") == 0)

is this a user-visible string in todoist ? then it should probably be translated.

::: src/gr-ingredients-exporter.h
@@ +23,3 @@
 #include <gtk/gtk.h>
 #include "gr-recipe.h"
+#include "gr-shopping-list-printer.h"

Is the include needed here, or is it enough to have it in the .c file ?

::: src/gr-shopping-page.h
@@ +33,3 @@
 GtkWidget      *gr_shopping_page_new      (void);
 
+GList * get_ingredients (GrShoppingPage *page);

This data should really be obtained from the recipe store, where we persist the shopping list.

Review of attachment 357027 [details] [review]:

I think  would make most sense to squash the three patches together, since each following patch contains fixes for things I complained about in the previous one...

::: src/gr-ingredients-exporter.c
@@ +113,3 @@
+	    	s = g_string_new ("");
+	    	g_string_append_printf (s, "%s %s", item->amount, item->name);
+	   	}

This for loop looks like it does nothing but leak memory - it should be removed

Created attachment 357183 [details] [review]
revised patch

Review of attachment 357183 [details] [review]:

As a general comment, I prefer spaces over tabs. Maybe you can convince your editor to expand tabs ? That should also help with getting indentation to be consistent: 8-space indent.

::: src/gr-ingredients-exporter.c
@@ +68,3 @@
+static void
+gr_ingredients_exporter_finalize (GObject *object)
+{

I think we need to free some things here, like  the access_token and the ingredients list, and maybe the account_object too

@@ +130,3 @@
+static void
+export_shopping_list_callback (RestProxyCall *call, GError *error, 
+							   GObject *obj, GrIngredientsExporter *exporter)

the parameter list looks a bit disorderly... would be nicer if it is lined up like this:

static void
export_shopping_list_callback (RestProxyCall         *call,
                               GError                *error, 
                               GObject               *obj,
                               GrIngredientsExporter *exporter)

@@ +146,3 @@
+		if (status_code != 200) {
+			  	g_warning("Couldn't export shopping list");
+			    goto out;

a bad indentation and a missing space before ( here

I wonder: what is the role of the GError that is passed into this callback - does it contain error information that we should be printing with the warning here ?

@@ +160,3 @@
+
+		if (!object) {
+		  		g_warning("Export returned empty json");

I think there needs to be a goto out; here as well ?

@@ +177,3 @@
+	    RestProxy *proxy;
+		RestProxyCall *call;
+		GError *error;

Indentation is a bit messy in this function. We want to have a consistent 8-space indent

@@ +237,3 @@
+cancel_export (GrIngredientsExporter *exporter)
+{
+		g_print ("cancel_export\n");

This should be fleshed out, I guess.

@@ +255,3 @@
+	    GError *error;
+
+	    client = goa_client_new_sync (NULL, &error);

This object is currently leaked. You should change the variable declaration to

g_autoptr GoaClient client = NULL;

to avoid that

@@ +258,3 @@
+	    
+	    if (!client) {
+	    	g_error ("Could not create GoaClient: %s", error->message);

Please use g_warning instead of g_error - g_error will abort the program

@@ +273,3 @@
+					    return TRUE;
+				}	
+  		}

We are leaking memory here. The docs for goa_client_get_accounts say:

 * Returns: (transfer full) (element-type GoaObject): A list of
 * #GoaObject instances that must be freed with g_list_free() after
 * each element has been freed with g_object_unref().

@@ +290,3 @@
+	    		g_warning ("Access token not found!");
+	    }
+	    exporter->access_token = access_token;

Can this be called more than once ? If so, we need to check if access_token has already been set, and free the old first

@@ +319,3 @@
+		project_add_commands = g_string_new ("");
+		uuid = g_uuid_string_random ();
+		temp_id = g_uuid_string_random ();

These strings look like they will be leaked. Change the variable declarations to g_autofree char *uuid = NULL; and same for temp_id

@@ +321,3 @@
+		temp_id = g_uuid_string_random ();
+		proxy = rest_proxy_new (TODOIST_URL, FALSE);
+		call = rest_proxy_new_call (proxy);

These two objects look like they will be leaked. Change the variable declarations to g_autoptr RestProxy proxy = NULL (if librest supports these g_auto annotations). Otherwise, you'll have to g_object_unref them when exiting the function scope.
Same for call.

@@ +334,3 @@
+
+		g_string_append_printf (project_add_commands, "[{\"type\": \"project_add\", \"temp_id\":\"%s\",\"uuid\":\"%s\", "
+        	                    "\"args\":{\"name\":\"Shopping List from Recipes\"}}]",

Is the name visible in the todoist ui ? if yes, the string should probably be translated

@@ +349,3 @@
+		}
+
+		parser = json_parser_new ();

The parser looks like it will get leaked as well - change the variable declaration to g_autoptr JsonParser parser = NULL; if json-glib supports g_auto annotations; otherwise, g_object_unref at the end

@@ +355,3 @@
+		if (!json_parser_load_from_data (parser, payload, payload_length, &parse_error)) {
+			    g_clear_error (&parse_error);
+			    g_warning("Couldnt load payload");

If we can't load data, I guess we should have some goto out; or so here to skip over the code below ?

@@ +361,3 @@
+		
+		if (!object)
+				g_warning("No Data found");

same here

@@ +379,3 @@
+			    if (strcmp (name, "Shopping List from Recipes") == 0) {    	
+				      	id = json_object_get_double_member (object, "id");
+					  	(exporter->project_id) = (glong) id;

odd () on the left-hand side here

@@ +403,3 @@
+
+
+	    proxy = rest_proxy_new (TODOIST_URL, FALSE);

As above: add g_autoptr to the variable declaration

@@ +417,3 @@
+
+	    if (!rest_proxy_call_sync (call, &error)) {
+	      	g_clear_error (&error);

Should there be a goto out; style early exit here ? no point in continuing the rest of the function if the call fails, I guess ?

@@ +420,3 @@
+	    }
+
+	    g_object_unref(proxy);

Ah no. here you manually unref the proxy. I think g_autoptr is a slightly nicer, but at least you don't leak.

@@ +424,3 @@
+		parse_error = NULL;
+		status_code = rest_proxy_call_get_status_code (call);
+		parser = json_parser_new ();

The parser object will get leaked - g_autoptr needed again.
Also, might make sense to move the parser creation below the status_code check.

@@ +427,3 @@
+
+		if (status_code != 200) {
+				g_warning ("Couldn't export shopping list");

goto out; or something ?

@@ +434,3 @@
+
+		if (!json_parser_load_from_data (parser, payload, payload_length, &parse_error)) {
+		    	g_clear_error (&parse_error);

goto out ?

@@ +438,3 @@
+
+		object = json_node_dup_object (json_parser_get_root (parser));
+		if (!object)

goto out ?

@@ +443,3 @@
+		projects = json_object_get_array_member (object, "projects");
+
+		lists = json_array_get_elements (projects);

You are leaking the list here. The docs for json_array_get_elements says:

 * Return value: (element-type JsonNode) (transfer container): a #GList
 *   containing the elements of the array. The contents of the list are
 *   owned by the array and should never be modified or freed. Use
 *   g_list_free() on the returned list when done using it

So you need to free the list (but not the list elements)

@@ +457,3 @@
+				      	id = json_object_get_double_member (object, "id");
+				      	(exporter->project_id) = (glong) id;
+				      	goto export;

Once you add a g_list_free() call after the loop, this goto will be problematic because it will jump over that

@@ +464,3 @@
+				add_project_id (exporter);
+
+		sync_token = json_object_get_string_member (object, "sync_token");	

json_object_get_string_member returns a const char * that points at the contents of object. I'm a bit worried that the pointer will be dangling once you unref the object. It is probably better to strdup here. You'll have to change the sync_token member to a char * and free it in finalize.

@@ +480,3 @@
+				get_access_token (exporter);
+				if (!exporter->project_id) {
+						get_project_id (exporter);

I think the name of the function, get_project_id is a bit confusing, if it is the one you call to actually export the list. Should it maybe be called export_shopping_list_to_todoist ?

@@ +488,3 @@
+				shopping_page = gr_shopping_page_new ();
+				gr_shopping_page_populate (GR_SHOPPING_PAGE (shopping_page));
+				share_list (GR_SHOPPING_PAGE (shopping_page));

Eeks. We should not be greating a new GrShoppingPage object here. The share_list function should be changed to get its list directly from the recipe store

::: src/gr-recipe-exporter.c
@@ +22,1 @@
 #include "config.h"

No relevant changes in gr-recipe-exporter.c - this should just be dropped from the commit

::: src/gr-shopping-page.c
@@ +25,3 @@
 #include <glib/gi18n.h>
 #include <gtk/gtk.h>
+#include <goa/goa.h>

I don't think you need the goa.h include here. It is only used in gr-ingredients-exporter.c

@@ +648,1 @@
 get_ingredients (GrShoppingPage *page)

I really don't want to make this function public and call it from elsewhere. I think the easiest way to straighten this out is to call get_ingredients before you create the exporter object, and pass the ingredients when you call export().
See for example how we do this for printing the list.

@@ +754,3 @@
 }
 
+void share_list (GrShoppingPage *page)

I think it would make more sense to move the entire share_list function over to gr-recipe-exporter.c, and then adjust it over there

@@ +757,3 @@
 {
+        // GtkWidget *test = gtk_dialog_new();
+        // gtk_widget_show (test);

Leftovers, remove

@@ +787,3 @@
+                page->exporter = gr_ingredients_exporter_new(GTK_WINDOW (window));
+        }
+        gr_ingredients_exporter_export (page->exporter);

So this would become:

items = get_ingredients (page);
gr_ingredients_exporter_export (page->exporter, items);

Makes sense ?

the todoist support was merged