GNOME Bugzilla – Bug 772278
Add todoist integration
Last modified: 2020-11-25 16:21:18 UTC
Integration with todoist would be nice. It seems some work on that started but now stalled. https://github.com/GNOME/gnome-todo/tree/wip/gbsneto/todoist-plugin It would be nice if anyone picks up from that.
This will be implemented in the form of a plugin in the near future.
Created attachment 352896 [details] [review] todoist-plugin: stub out This patch includes the basic skeleton along with certain signal and functions that will be required for todoist integration. Many function are not implemented since we don't have a Todoist Goa Account. Classes added: 1) TodoistPreferencesPanel 2) TodoistProvider 3) TodoistPlugin
Review of attachment 352896 [details] [review]: This commit does not add the necessary Meson files. A few other comments below. The commit message is good, and the patch is going in the right direction =) ::: plugins/todoist/gtd-plugin-todoist.c @@ +156,3 @@ + l = NULL; + + child = gtk_container_get_children (GTK_CONTAINER (self->preferences)); Don't do this. Instead, you should load the GoaClient here in GtdPluginTodoist, and then call "gtd_todoist_preferences_panel_set_client (prefs, client);". ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +168,3 @@ + "row-activated", + G_CALLBACK (account_row_clicked_cb), + self); This is wrong. You should connect to the listbox only once, in the 'preferences.ui' file. @@ +254,3 @@ + + G_OBJECT_CLASS (gtd_todoist_preferences_panel_parent_class)->finalize (object); +} Unref the GoaClient here. @@ +270,3 @@ + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + } +} You can entirely remove the 'switch' statement and the 'self' var, and only have a "G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);" here. @@ +286,3 @@ + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + } +} You can entirely remove the 'switch' statement and the 'self' var, and only have a "G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);" here. @@ +363,3 @@ + + /* And the preferences panel */ + builder = gtk_builder_new_from_resource ("/org/gnome/todo/ui/todoist/preferences.ui"); You should use a template class, and call gtk_widget_class_set_template_from_resource() in class_init(). ::: plugins/todoist/ui/preferences.ui @@ +3,3 @@ +<interface> + <requires lib="gtk+" version="3.16"/> + <object class="GtkBox" id="accounts_box"> This should be a template class.
Created attachment 353077 [details] [review] todoist-plugin: stub out This patch includes the basic skeleton along with certain signal and functions that will be required for todoist integration. Many function are not implemented since we don't have a Todoist Goa Account. Classes added: 1) TodoistPreferencesPanel 2) TodoistProvider 3) TodoistPlugin
Created attachment 353118 [details] [review] todoist-plugin: stub out This patch includes the basic skeleton along with certain signal and functions that will be required for todoist integration. Many function are not implemented since we don't have a Todoist Goa Account. Classes added: 1) TodoistPreferencesPanel 2) TodoistProvider 3) TodoistPlugin
Review of attachment 353118 [details] [review]: A few comments below. This is starting to take shape! ::: plugins/todoist/ui/preferences.ui @@ +3,3 @@ +<interface> + <requires lib="gtk+" version="3.16"/> + <template class="GtdTodoistPreferencesPanel" parent="GtkBox"> This class should be a GtkStack, and there should be 2 pages: 1. "no-accounts" page, with an icon, a label and a "Add Todoist Account" button 2. "accounts" page, with the current widgets. @@ +9,3 @@ + <property name="valign">center</property> + <property name="margin_left">24</property> + <property name="margin_right">24</property> Never use "margin_left" and "margin_right", always use "margin_start" and "margin_end" @@ +14,3 @@ + <property name="hexpand">True</property> + <property name="vexpand">True</property> + <property name="border_width">24</property> You already set the margins, remove this. @@ +21,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="pixel_size">62</property> Perhaps 64? Icons should be always be 8x8, 16x16, 24x24, 32x32, 64x64, 128x128, 256x256 or 512x512. @@ +34,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">Todoist Accounts Being Used</property> This should be "Available Todoist Accounts:" @@ +48,3 @@ + <property name="shadow_type">in</property> + <property name="min_content_width">300</property> + <property name="min_content_height">200</property> Instead of setting the minimum width and height, I suggest you set "max-content-height" to 300.
Created attachment 353386 [details] [review] todoist-plugin: stub out This patch includes the basic skeleton along with certain signal and functions that will be required for todoist integration. Many function are not implemented since we don't have a Todoist Goa Account. Classes added: 1) TodoistPreferencesPanel 2) TodoistProvider 3) TodoistPlugin
Review of attachment 353386 [details] [review]: Mostly nitpicks, but a couple of big comments too. ::: plugins/todoist/gtd-plugin-todoist.c @@ +157,3 @@ + + gtd_todoist_preferences_panel_set_client (GTD_TODOIST_PREFERENCES_PANEL (self->preferences), client); + Nitpick: remove empty line before end of function. @@ +161,3 @@ + +static void +setup (GtdPluginTodoist *self) You can remove this function and put it's code inside "gtd_plugin_todoist_init()" directly. ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +36,3 @@ +}; + +G_DEFINE_TYPE_WITH_PRIVATE (GtdTodoistPreferencesPanel, gtd_todoist_preferences_panel, GTK_TYPE_STACK) This is a final class. Adding a Private field is not necessary. @@ +42,3 @@ + ACCOUNT_ADDED, + ACCOUNT_REMOVED, + ACCOUNT_CHANGED, You don't need any of these signals. GtdPluginTodoist has access to the GoaClient, and must connect directly to the client. @@ +155,3 @@ + row = gtk_list_box_row_new (); + + g_object_set_data_full (G_OBJECT (row), "goa-object", g_object_ref(object), g_object_unref); Missing space before parenthesis. @@ +206,3 @@ + GtdTodoistPreferencesPanel *self) +{ + g_signal_emit (self, signals[ACCOUNT_CHANGED], 0, object); You should check if the account is a Todoist account before emiting this signal. @@ +346,3 @@ + + gtk_stack_set_visible_child (GTK_STACK (self), self->priv->empty_page); + label = gtk_label_new ("No todoist accounts configured"); - This label should be translatable. - When writing in English, use English capitalization rules (i.e. "Todoist" not "todoist"). - "account" not "accounts" ::: plugins/todoist/ui/preferences.ui @@ +6,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="transition_type">slide-left</property> transition should be "crossfade" @@ +60,3 @@ + </object> + <packing> + <property name="name">page0</property> name should be "accounts" @@ +61,3 @@ + <packing> + <property name="name">page0</property> + <property name="title" translatable="yes">page0</property> Remove the title (we don't expose it in the UI) @@ +75,3 @@ + <property name="spacing">12</property> + <child> + <object class="GtkImage"> Add the "dim-label" style class to this search icon. @@ +79,3 @@ + <property name="can_focus">False</property> + <property name="pixel_size">96</property> + <property name="icon_name">edit-find-symbolic.symbolic</property> The icon name is just "edit-find-symbolic" @@ +80,3 @@ + <property name="pixel_size">96</property> + <property name="icon_name">edit-find-symbolic.symbolic</property> + <property name="icon_size">6</property> Remove this "icon_size" property @@ +86,3 @@ + <property name="fill">True</property> + <property name="position">0</property> + </packing> You can remove all these <packing>...</packing> from GtkBox children. Glade adds them automatically, but they're not needed @@ +95,3 @@ + <attributes> + <attribute name="weight" value="bold"/> + <attribute name="foreground" value="#000000000000"/> Replace this "foreground" attribute with <attribute name="scale" value="1.44" /> @@ +120,3 @@ + </object> + <packing> + <property name="name">page1</property> name should be "empty" @@ +121,3 @@ + <packing> + <property name="name">page1</property> + <property name="title" translatable="yes">page1</property> Remove the title (we don't expose it in the UI)
Created attachment 353415 [details] [review] todoist-plugin: stub out This patch includes the basic skeleton along with certain signal and functions that will be required for todoist integration. Many function are not implemented since we don't have a Todoist Goa Account. Classes added: 1) TodoistPreferencesPanel 2) TodoistProvider 3) TodoistPlugin
Review of attachment 353415 [details] [review]: A big patch that is looking pretty good now. Thanks!
Comment on attachment 353415 [details] [review] todoist-plugin: stub out Attachment 353415 [details] pushed as cf778f8 - todoist-plugin: stub out
Created attachment 353461 [details] [review] todoist-preferences-panel: Hide add account button if Todoist not in GOA This patch hides the add account button is there is no provider named Todoist in GOA. Since add account button will only be required if we can actually add a Todoist account.
Created attachment 353765 [details] [review] todoist-preferences-panel: fix signal connect for add_button Since, add_account_button_clicked only need the data, that is GtdTodoistPreferencesPanel rather than instance add_button, it should be g_signal_connect_swapped. Currently with just g_signal_connect the callback function will not work properly since data won't be correctly passed.
Created attachment 353856 [details] [review] provider-todoist: Add function converting color index to GdkRGBA and opposite Since Todoist uses an integer mapped to hex code for list color, but To Do uses GdkRGBA we need function that will do the necessary conversion from one to other. This patch add following method: 1)convert_color_code: This converts todoist supported int code to To Do supported GdkRGBA color. 2)get_color_code_index: This returns index for To Do supported GdkRGBA color which maps correctly to todoist color codes (hex). 3)optimized_eucledian_color_distance :Required by 2nd method to find To Do color closed to todoist predifined color codes.
Will it be possible to sync only particular task lists of todoist (i.e Projects) from preference?
Review of attachment 353765 [details] [review]: One question ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +108,3 @@ { + g_return_if_fail (GOA_IS_CLIENT (self->client)); + Why are you removing the 'spawn_goa_with_args()' call?
Review of attachment 353856 [details] [review]: The patch looks good, expect for a few comments. The commit message can have a few fixes too: - Always add a space after '1)', '2)', etc. - In English, the space comes ~after~ :, not before. - The commit title is a little too long. ::: plugins/todoist/gtd-provider-todoist.c @@ +57,3 @@ + "#dc4fad", "#ac193d", "#d24726", "#82ba00", "#03b3b2", "#008299", + "#5db2ff", "#0072c6", "#000000", "#777777"}; + Nit: One color per line, please. Like static const gchar *colormap[] = { ... , ... , ... }; @@ +117,3 @@ + gdouble red_mean_level; + + red_mean_level = (color1->red + color2->red) / 2; I think there is a confusion here. GdkRGBA fields are doubles that range between [0, 1]. Below, you didive by 256 as if the fields are integers ranging between [0, 255]. Is that correct? @@ +123,3 @@ + + return ((2 + (red_mean_level / 256))*red_diff*red_diff + 4*green_diff*green_diff + + (2 + ((255 - red_mean_level) / 256))*blue_diff*blue_diff); Nit: You can make it much more readable if you write one color per line. Like return (red_diff * red_diff * (2 + red_mean_level) + green_diff * green_diff * 4 + blue_diff * blue_diff * (1 - red_mean_level) + 2); @@ +142,3 @@ + gint min_color_diff; + gint n_color_codes; + gint i; Can any of those variables assume a negative number? Those who are always positive (like i and 'min_color_diff') should be guint. @@ +144,3 @@ + gint i; + + nearest_color_index = -1; Start with G_MAXUINT @@ +160,3 @@ + min_color_diff = distance; + nearest_color_index = i; + } If you use G_MAXUINT, you won't need this check.
Review of attachment 353461 [details] [review]: There is a better approach: - Make the empty page the first page by default - When any Todoist account is added, switch to the lists page - When the last Todoist account is removed, switch back to the empty page
Created attachment 354002 [details] [review] todoist: Add auxiliary methods for color conversion Since Todoist uses an integer mapped to hex code for list color, but To Do uses GdkRGBA we need function that will do the necessary conversion from one to other. This patch add following method: 1)convert_color_code: This converts todoist supported int code to To Do supported GdkRGBA color. 2)get_color_code_index: This returns index for To Do supported GdkRGBA color which maps correctly to todoist color codes (hex). 3)optimized_eucledian_color_distance: Required by 2nd method to find To Do color closed to todoist predifined color codes.
Created attachment 354003 [details] [review] todoist: Add auxiliary methods for color conversion Since Todoist uses an integer mapped to hex code for list color, but To Do uses GdkRGBA we need function that will do the necessary conversion from one to other. This patch add following method: 1)convert_color_code: This converts todoist supported int code to To Do supported GdkRGBA color. 2)get_color_code_index: This returns index for To Do supported GdkRGBA color which maps correctly to todoist color codes (hex). 3)optimized_eucledian_color_distance: Required by 2nd method to find To Do color closed to todoist predifined color codes.
Created attachment 354004 [details] [review] todoist-preferences-panel: fix signal connect for add_button Since, add_account_button_clicked only need the data, that is GtdTodoistPreferencesPanel rather than instance add_button, it should be g_signal_connect_swapped. Currently with just g_signal_connect the callback function will not work properly since data won't be correctly passed.
Created attachment 354005 [details] [review] todoist-preferences-panel: fix signal connect for add_button Since, add_account_button_clicked only need the data, that is GtdTodoistPreferencesPanel rather than instance add_button, it should be g_signal_connect_swapped. Currently with just g_signal_connect the callback function will not work properly since data won't be correctly passed.
Review of attachment 354003 [details] [review]: One last comment. ::: plugins/todoist/gtd-provider-todoist.c @@ +169,3 @@ + n_color_codes = sizeof (colormap) / sizeof (colormap [0]); + + for (i = 0; i < n_color_codes; i++) Use G_N_ELEMENTS() and remove the 'n_color_codes' var
Review of attachment 354005 [details] [review]: Looks good!
Comment on attachment 354005 [details] [review] todoist-preferences-panel: fix signal connect for add_button Attachment 354005 [details] pushed as a761b22 - todoist-preferences-panel: fix signal connect for add_button
Created attachment 354056 [details] [review] todoist: Add auxiliary methods for color conversion Since Todoist uses an integer mapped to hex code for list color, but To Do uses GdkRGBA we need function that will do the necessary conversion from one to other. This patch add following method: 1)convert_color_code: This converts todoist supported int code to To Do supported GdkRGBA color. 2)get_color_code_index: This returns index for To Do supported GdkRGBA color which maps correctly to todoist color codes (hex). 3)optimized_eucledian_color_distance: Required by 2nd method to find To Do color closed to todoist predifined color codes.
Created attachment 354057 [details] [review] todoist: Show accounts page only if todoist account is present This patch enable empty page as default intial page and switches the stack child to accounts page if any todoist account was added. Incase, an account was removed, we need to check if no account is present after this removal and switch to empty page, if true.
Created attachment 354058 [details] [review] todoist: Add auxiliary methods for color conversion Since Todoist uses an integer mapped to hex code for list color, but To Do uses GdkRGBA we need function that will do the necessary conversion from one to other. This patch add following method: 1)convert_color_code: This converts todoist supported int code to To Do supported GdkRGBA color. 2)get_color_code_index: This returns index for To Do supported GdkRGBA color which maps correctly to todoist color codes (hex). 3)optimized_eucledian_color_distance: Required by 2nd method to find To Do color closed to todoist predifined color codes.
Review of attachment 354058 [details] [review]: Looks good!
Review of attachment 354057 [details] [review]: Some comments. ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +157,3 @@ + */ + if (!gtk_container_get_children (GTK_CONTAINER (self->accounts_listbox))) + gtk_stack_set_visible_child (GTK_STACK (self), self->accounts_page); This is a memory leak. The GList returned by gtk_container_get_children() must be freed. @@ +200,3 @@ + */ + if (!gtk_container_get_children (GTK_CONTAINER (self->accounts_listbox))) + gtk_stack_set_visible_child (GTK_STACK (self), self->empty_page); Ditto
Created attachment 354114 [details] [review] todoist: Show accounts page only if todoist account is present This patch enable empty page as default intial page and switches the stack child to accounts page if any todoist account was added. Incase, an account was removed, we need to check if no account is present after this removal and switch to empty page, if true.
Created attachment 354115 [details] [review] todoist: fix some issues in preference panel This patch fixes: 1) wrongly used key while getting goa-object associated to accounts_listbox row. 2) load inital goa-accounts after goa-client is created. That is call on_account_added on every account after calling goa_client_get_accounts for first time. 3) free the accounts list returned from goa_client_get_ accounts which earlier was leaking.
Review of attachment 354114 [details] [review]: One more comment below ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +212,3 @@ + gtk_stack_set_visible_child (GTK_STACK (self), self->empty_page); + + g_list_free (child); This looks like a double free? Also, since you're doing the same check here and above, you can factor this in a "guint count_available_todoist_accounts()" Then you can just "...set_visible (count_available_...() > 0) show(); else hide();"
Review of attachment 354115 [details] [review]: One minor nitpick ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +234,3 @@ + { + on_goa_account_added (self->client, l->data, self); + } Single line for does not need {}s
Created attachment 354213 [details] [review] todoist: fix some issues in preference panel This patch fixes: 1) wrongly used key while getting goa-object associated to accounts_listbox row. 2) load inital goa-accounts after goa-client is created. That is call on_account_added on every account after calling goa_client_get_accounts for first time. 3) free the accounts list returned from goa_client_get_ accounts which earlier was leaking.
Created attachment 354214 [details] [review] todoist: Show accounts page only if todoist account is present This patch enable empty page as default intial page and switches the stack child to accounts page if any todoist account was added. Incase, an account was removed, we need to check if no account is present after this removal and switch to empty page, if true.
Review of attachment 354213 [details] [review]: Looks good!
Review of attachment 354214 [details] [review]: ::: plugins/todoist/gtd-todoist-preferences-panel.c @@ +159,3 @@ + /* If List Box was empty before this addition, the preferences + * panel should change to accounts_page. + */ Wrong comment style @@ +165,3 @@ gtk_list_box_insert (GTK_LIST_BOX (self->accounts_listbox), GTK_WIDGET (row), -1); + + g_list_free (child); You don't need to get the children. If you're adding a new row, you can just unconditionally change to accounts_page. @@ +206,3 @@ + /* Check if ListBox becomes empty after this removal. + * If true change to empty_page of preference panel. + */ Wrong comment style.
Created attachment 354295 [details] [review] todoist: Show accounts page only if todoist account is present Fix these comments
Attachment 354213 [details] pushed as ba89700 - todoist: fix some issues in preference panel Attachment 354295 [details] pushed as 3bfc0a5 - todoist: Show accounts page only if todoist account is present
Created attachment 354615 [details] [review] todoist: add provider for inital todoist-accounts This patch calls gtd_plugin_todoist_account_added on the accounts that were already present, once goa_client is finished loading. After calling account_added, every account is associated with a provider and todoist provider is initalised. This is done to make sure to load those accounts which were already added before the goa_client was loaded.
Created attachment 354616 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
Created attachment 354627 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
Review of attachment 354615 [details] [review]: Some comments below. ::: plugins/todoist/gtd-plugin-todoist.c @@ +191,3 @@ + + account = goa_object_get_account (l->data); + provider_name = goa_account_get_provider_name (account); Would be better to use the provider type, right? @@ +194,3 @@ + + if (g_strcmp0 (provider_name, "Todoist") != 0) + continue; This leaks the GoaAccount
Review of attachment 354627 [details] [review]: Some comments below. ::: plugins/todoist/gtd-provider-todoist.c @@ +176,3 @@ + + gtd_manager_emit_error_message (gtd_manager_get_default (), + _("Error making a sync call to Todoist"), This is not very clear... perhaps "Error synchronizing with Todoist". @@ +218,3 @@ + + lists = NULL; + l = NULL; You don't need to initialize them here. @@ +262,3 @@ + + lists = NULL; + l = NULL; You don't need to initialize them here. @@ +351,3 @@ + rest_proxy_call_add_param (call, "resource_types", "[\"all\"]"); + + if (!rest_proxy_call_sync (call, &error)) Use the asynchronous version. This can potentially take many seconds.
Created attachment 354801 [details] [review] todoist: add provider for inital todoist-accounts This patch calls gtd_plugin_todoist_account_added on the accounts that were already present, once goa_client is finished loading. After calling account_added, every account is associated with a provider and todoist provider is initalised. This is done to make sure to load those accounts which were already added before the goa_client was loaded.
Created attachment 354848 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
Review of attachment 354801 [details] [review]: Looks good with the comment below. ::: plugins/todoist/gtd-plugin-todoist.c @@ +179,3 @@ + + accounts = NULL; + l = NULL; No need to initialize it here, since these vars will be always initialized anyway.
Review of attachment 354848 [details] [review]: Some minor comments below. Almost there! ::: plugins/todoist/gtd-provider-todoist.c @@ +176,3 @@ + + gtd_manager_emit_error_message (gtd_manager_get_default (), + _("Error making a sync call to Todoist"), Bad message. Better would be "Error loading Todoist tasks". @@ +299,3 @@ + project_id = json_object_get_int_member (object, "project_id"); + + list = g_hash_table_lookup (self->lists, GUINT_TO_POINTER(project_id)); Nitpick: style issue (missing space before parenthesis) @@ +354,3 @@ + { + gtd_manager_emit_error_message (gtd_manager_get_default (), + _("Wrong status code recieved. Request to Todoist not performend correctly."), This would be a bad message. Perhaps "Error loading Todoist tasks" with a detailed description "Bad status code (%d) received. Please check your connection." @@ +506,3 @@ + account = goa_object_get_account (self->account_object); + identity = goa_account_dup_identity (account); + self->description = g_strconcat ("Todoist: ", identity , NULL); This will be better as g_strdup_printf (_("Todoist: %s"), identity)
Created attachment 354875 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
Created attachment 354876 [details] [review] todoist: implement a generic post method Since methods such as gtd_provider_todoist_create_task etc. will make a post request to todoist endpoint, it is necessary to have a generic post function otherwise a lot of code will be repeated. This patch implements: 1) Implements a generic post method that takes Json Object as params. 2) Make use of the generic post method in the function gtd_provider_todoist_sync_call which is use to sync tasks and lists from todoist.
Review of attachment 354875 [details] [review]: Some other comments below. We're getting close now. ::: configure.ac @@ +82,3 @@ + libpeas-1.0 >= 1.17 + rest-0.7 + json-glib-1.0) You need to add these dependencies to plugins/todoist/meson.build too. ::: plugins/todoist/gtd-provider-todoist.c @@ +172,3 @@ + g_warning ("%s: %s: %s", + G_STRFUNC, + _("Error making a sync call to Todoist"), Don't need to be translatable. @@ +218,3 @@ + + lists = NULL; + l = NULL; No need to initialize them here (they are always initialized below) @@ +388,3 @@ + +static void +gtd_provider_todoist_sync_call (GtdProviderTodoist *self) A better name would be "synchronize_call()" → without the gtd_provider_todoist_ prefix. In general, I'm trying to avoid prefixing static functions nowadays. @@ +505,2 @@ static void +gtd_provider_todoist_set_description (GtdProviderTodoist *self) A better name would be "update_description()" @@ +510,3 @@ + + account = goa_object_get_account (self->account_object); + identity = goa_account_dup_identity (account); You can use "get_identity()" and avoid a memory allocation cycle.
Review of attachment 354876 [details] [review]: Small comments now. ::: plugins/todoist/gtd-provider-todoist.c @@ +26,3 @@ #include <glib/gi18n.h> +#define TODOIST_URL "https://todoist.com/API/v7/sync" This should be in the previous patch. @@ +461,1 @@ + post (params, (RestProxyCallAsyncCallback) gtd_provider_todoist_sync_cb, self); Do you really have to cast here? If the function has the right signature, this is not needed.
Created attachment 354934 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac and todoist/meson.build which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
Created attachment 354945 [details] [review] todoist: implement a generic post method Since methods such as gtd_provider_todoist_create_task etc. will make a post request to todoist endpoint, it is necessary to have a generic post function otherwise a lot of code will be repeated. This patch implements: 1) Implements a generic post method that takes Json Object as params. 2) Make use of the generic post method in the function gtd_provider_todoist_sync_call which is use to sync tasks and lists from todoist.
Review of attachment 354934 [details] [review]: Looks good!
Review of attachment 354945 [details] [review]: Two minor nitpicks. ::: plugins/todoist/gtd-provider-todoist.c @@ +331,3 @@ +{ + RestProxy *proxy; + RestProxyCall *call; Style issue here. The variables should not be aligned. @@ +342,3 @@ + + rest_proxy_call_set_method (call, "POST"); + rest_proxy_call_add_header (call, "content-type", "application/x-www-form-urlencoded"); Nitpick: one argument per line
Created attachment 354955 [details] [review] todoist: implement a generic post method Since methods such as gtd_provider_todoist_create_task etc. will make a post request to todoist endpoint, it is necessary to have a generic post function otherwise a lot of code will be repeated. This patch implements: 1) Implements a generic post method that takes Json Object as params. 2) Make use of the generic post method in the function gtd_provider_todoist_sync_call which is use to sync tasks and lists from todoist.
Review of attachment 354955 [details] [review]: Looks god
Applying these patches leads to dozens of compile warnings. Here: https://paste.gnome.org/poscrhxqr (available for 1 year from now)
Created attachment 355005 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac and todoist/meson.build which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
I have fixed the patch. Most of the warning was because i was checking with json_object_has_member which can be removed, since a valid sync call is guaranteed to have the members. Also, only the first patch introduce these warnings. Weird thing is I was not getting the warning while building through jhbuild.
Created attachment 355294 [details] [review] todoist: Implement todoist tasks/lists sync every 1hour This patch modifies: 1) parse_array_to_task 2) parse_array_to_task and adds a GSource Callback (synchronize_call) to be called every hour so that tasks and lists are in sync with todoist. The functions are modified so that they can be reused and any further use does not loads the task again but if task was already loaded it just updates the changes locally.
Created attachment 355438 [details] [review] todoist: implement loading of tasks and lists This patch implement methods to make a request to todoist and parse the response to load tasks and tasklists. The loading of tasks/lists is not synchronized and it happens only at the time todoist plugin is loaded. This patch does: 1) Allow loading of tasks and lists from users todoist at the time todoist plugin is loaded. 2) It also adds necessary library in configure.ac and todoist/meson.build which are required for making a POST call, and parsing the JSON respone. eg. JSON-glib, librest
Created attachment 355439 [details] [review] todoist: implement a generic post method Since methods such as gtd_provider_todoist_create_task etc. will make a post request to todoist endpoint, it is necessary to have a generic post function otherwise a lot of code will be repeated. This patch implements: 1) Implements a generic post method that takes Json Object as params. 2) Make use of the generic post method in the function gtd_provider_todoist_sync_call which is use to sync tasks and lists from todoist.
Created attachment 355440 [details] [review] todoist: save local changes to todoist This patch implements: 1) gtd_provider_todoist_remove_task_list 2) gtd_provider_todoist_update_task_list 3) gtd_provider_todoist_remove_task 4) gtd_provider_todoist_update_task With this, todoist tasks/lists can be modified from To Do and the changes gets saved on Todoist.
Created attachment 355449 [details] [review] todoist: save local changes to todoist This patch implements: 1) gtd_provider_todoist_remove_task_list 2) gtd_provider_todoist_update_task_list 3) gtd_provider_todoist_remove_task 4) gtd_provider_todoist_update_task With this, todoist tasks/lists can be modified from To Do and the changes gets saved on Todoist.
Created attachment 355548 [details] [review] todoist: move access_token to provider struct This patch stores the Todoist Account access token in Provider structure. Since access_token is to be sent with every http post request, it is redundant to fetch it again and again. Hence this patch proposes to store it just once at the start. Also a function to emit access_token related errors has been added.
Review of attachment 355438 [details] [review]: Only one issue, looks good otherwise ::: plugins/todoist/gtd-provider-todoist.c @@ +257,3 @@ + struct tm due_dt; + + res = strptime (due_date, "%a %d %b %Y %T %z", &due_dt); 'res' is unused, and 'due_dt' is accused to be potentially uninitalized
Review of attachment 355439 [details] [review]: One issue ::: plugins/todoist/gtd-provider-todoist.c @@ +398,3 @@ + +static void +post_generic_cb (RestProxyCall *call, This is unused (must be introduced in the next patch)
Created attachment 355553 [details] [review] todoist: implement loading of tasks and lists Add the fixes.
Created attachment 355554 [details] [review] todoist: implement a generic post method
Created attachment 355555 [details] [review] todoist: save local changes to todoist
Created attachment 355556 [details] [review] todoist: move access_token to provider struct
Attachment 354058 [details] pushed as 1db51e8 - todoist: Add auxiliary methods for color conversion Attachment 355553 [details] pushed as 9f11c1c - todoist: implement loading of tasks and lists Attachment 355554 [details] pushed as 4c243b8 - todoist: implement a generic post method Attachment 355555 [details] pushed as d811fed - todoist: save local changes to todoist Attachment 355556 [details] pushed as 253edc9 - todoist: move access_token to provider struct
Created attachment 356298 [details] [review] todoist: implement task and list creation This patch implements: 1) gtd_provider_todoist_create_task 2) gtd_provider_todoist_create_list and modifies code i.e addition of a struct for post callback data and implement mapping temp id. With this patch To Do can now create todoist task and list and save this to Todoist account.
Review of attachment 355294 [details] [review]: Does not apply anymore.
Comment on attachment 356298 [details] [review] todoist: implement task and list creation Attachment 356298 [details] pushed as c0e17a1 - todoist: implement task and list creation
Created attachment 357517 [details] [review] todoist: Implement queueing of post requests This patch implements a queue to which post request is added. The requests are dispatched every 6s, and is removed from queue only if request was a success. In case of error, it waits for 1 minute to dispatch the requests again.
Created attachment 357734 [details] [review] todoist: Implement queueing of post requests This patch implements a queue to which post request is added. The requests are dispatched every 6s, and is removed from queue only if request was a success. In case of error, it waits for 1 minute to dispatch the requests again.
Created attachment 357838 [details] [review] todoist: Implement todoist tasks/lists sync every 1hour This patch modifies: 1) parse_array_to_task 2) parse_array_to_task and adds a GSource Callback (synchronize_call) to be called every hour so that tasks and lists are in sync with todoist. The functions are modified so that they can be reused and any further use does not loads the task again but if task was already loaded it just updates the changes locally.
Created attachment 357985 [details] [review] todoist-preferences-panel: add parameters to spawn_goa Since todoist is hidden in GOA, on add account button click todoist panel should open and not goa accounts panel. This panel fixes this by passing parameters action and arg that is add and todoist to spawn_goa_ with_args function.
Review of attachment 357734 [details] [review]: Some comments below. ::: plugins/todoist/gtd-provider-todoist.c @@ +50,3 @@ + /* timeout ids */ + guint wait_id; + guint dispatch_id; You can use only one timeout id here. There's no need to split 'wait' and 'dispatch'. @@ +686,3 @@ + n_requests = g_queue_get_length (self->queue); + + for (i = 0; i < n_requests; i++) You can't use a forloop when dealing with asynchronous functions. You have to fire the first item, and only continue inside the callback. @@ +779,3 @@ + + /* Push post request data to queue */ + g_queue_push_head (self->queue, data); You should add an "push_post_request()" function that: 1. adds 'data' to the queue 2. if the queue is not being consumed, start consuming it @@ +850,3 @@ + + /* Push post request data to queue */ + g_queue_push_head (self->queue, data); Should use push_post_request() here @@ +894,3 @@ + + /* Push post request data to queue */ + g_queue_push_head (self->queue, data); And here too @@ +947,3 @@ + + /* Push post request data to queue */ + g_queue_push_head (self->queue, data); And here too @@ +998,3 @@ + + /* Push post request data to queue */ + g_queue_push_head (self->queue, data); And here too @@ +1042,3 @@ + + /* Push post request data to queue */ + g_queue_push_head (self->queue, data); ... @@ +1236,3 @@ + + /* Timeout id for dispatch requests */ + self->dispatch_id = g_timeout_add (6000, (GSourceFunc) dispatch_requests, self); Wrong. You should only start the timeout when there's something queued (see the comment about push_post_request() before)
Review of attachment 357838 [details] [review]: Minor comments below. ::: plugins/todoist/gtd-provider-todoist.c @@ +687,3 @@ { emit_access_token_error (); + return FALSE; Better use G_SOURCE_CONTINUE/G_SOURCE_STOP @@ +700,3 @@ json_object_unref (params); + + return TRUE; Better use G_SOURCE_CONTINUE/G_SOURCE_STOP @@ +1274,3 @@ + /* synchronize call */ + g_timeout_add_seconds (3600, (GSourceFunc) synchronize_call, self); You need to store this source id and remove it on finalize()
Review of attachment 357985 [details] [review]: LGTM
Comment on attachment 357985 [details] [review] todoist-preferences-panel: add parameters to spawn_goa Attachment 357985 [details] pushed as 48af9ed - todoist-preferences-panel: add parameters to spawn_goa
Created attachment 358690 [details] [review] todoist: Implement queueing of post requests This patch implements queuing of requests and disptaching it properly to avoid conflicts and time limits. It adds :- 1) dispatch_requests - dispatches the first request from queue. If there are more request they are dispatched in post callback. 2) push_post_request - When user changes any todoist task/lists, this push the request to queue and also adds a timeout of 15 seconds for dispatching the requests. A queue is added to provider class which holds post requests. With this patch, the problem of reaching a limit is fixed since on error the request is not removed from queue and hence is not lost.
Created attachment 358754 [details] [review] todoist: Implement queueing of post requests This patch implements queuing of requests and disptaching it properly to avoid conflicts and time limits. It adds :- 1) dispatch_requests - dispatches the first request from queue. If there are more request they are dispatched in post callback. 2) push_post_request - When user changes any todoist task/lists, this push the request to queue and also adds a timeout of 15 seconds for dispatching the requests. A queue is added to provider class which holds post requests. With this patch, the problem of reaching a limit is fixed since on error the request is not removed from queue and hence is not lost.
Created attachment 358755 [details] [review] todoist: Implement todoist tasks/lists sync every 1hour This patch modifies: 1) parse_array_to_task 2) parse_array_to_list and adds a GSource Callback(synchronize_call) to be called every hour so that tasks and lists are in sync with todoist. The functions are modified so that any tasks/lists which was created prior to this sync is updated and not created again. We check for presence of same id task or list in hash_table and update the that tasks if present else create new task or list.
Created attachment 358756 [details] [review] todoist: Implement todoist tasks/lists sync every 1hour This patch modifies: 1) parse_array_to_task 2) parse_array_to_list and adds a GSource Callback(synchronize_call) to be called every hour so that tasks and lists are in sync with todoist. The functions are modified so that any tasks/lists which was created prior to this sync is updated and not created again. We check for presence of same id task or list in hash_table and update the that tasks if present else create new task or list.
Created attachment 359211 [details] [review] todoist: hold todo until request queue is empty This patch introduces a hold-state which should be active when todoist request queue is not empty. User can accidently close the app when there are requests yet to be dispatched, this can cause lose of request. With this patch todo continues to run in background until all the post requests are dispatched.
Comment on attachment 358754 [details] [review] todoist: Implement queueing of post requests Attachment 358754 [details] pushed as c25a0f0 - todoist: Implement queueing of post requests
Comment on attachment 359211 [details] [review] todoist: hold todo until request queue is empty Attachment 359211 [details] pushed as 10e854e - todoist: hold todo until request queue is empty
bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all older bug reports and feature requests in GNOME Bugzilla which have not seen updates for a while. If you still use gnome-todo and if you still see this bug / want this feature in a recent and currently supported version, then please feel free to report it at https://gitlab.gnome.org/GNOME/gnome-todo/-/issues/ by following the guidelines at https://wiki.gnome.org/Community/GettingInTouch/BugReportingGuidelines Thank you for creating this report and we are sorry it could not be implemented so far (volunteer workforce and time is limited).