After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772278 - Add todoist integration
Add todoist integration
Status: RESOLVED OBSOLETE
Product: gnome-todo
Classification: Other
Component: Plugins
3.20.x
Other Linux
: Normal enhancement
: ---
Assigned To: GNOME To Do maintainer(s)
GNOME To Do maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-10-01 06:21 UTC by Khurshid Alam
Modified: 2020-11-25 16:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
todoist-plugin: stub out (42.25 KB, patch)
2017-05-30 18:14 UTC, Rohit Kaushik
none Details | Review
todoist-plugin: stub out (44.58 KB, patch)
2017-06-02 12:49 UTC, Rohit Kaushik
none Details | Review
todoist-plugin: stub out (45.48 KB, patch)
2017-06-03 18:03 UTC, Rohit Kaushik
none Details | Review
todoist-plugin: stub out (46.47 KB, patch)
2017-06-08 14:40 UTC, Rohit Kaushik
none Details | Review
todoist-plugin: stub out (42.93 KB, patch)
2017-06-08 17:40 UTC, Rohit Kaushik
committed Details | Review
todoist-preferences-panel: Hide add account button if Todoist not in GOA (1.73 KB, patch)
2017-06-09 13:14 UTC, Rohit Kaushik
needs-work Details | Review
todoist-preferences-panel: fix signal connect for add_button (1.57 KB, patch)
2017-06-14 19:18 UTC, Rohit Kaushik
none Details | Review
provider-todoist: Add function converting color index to GdkRGBA and opposite (3.35 KB, patch)
2017-06-15 18:51 UTC, Rohit Kaushik
needs-work Details | Review
todoist: Add auxiliary methods for color conversion (3.33 KB, patch)
2017-06-18 19:27 UTC, Rohit Kaushik
none Details | Review
todoist: Add auxiliary methods for color conversion (3.16 KB, patch)
2017-06-18 19:31 UTC, Rohit Kaushik
none Details | Review
todoist-preferences-panel: fix signal connect for add_button (1.57 KB, patch)
2017-06-18 19:41 UTC, Rohit Kaushik
none Details | Review
todoist-preferences-panel: fix signal connect for add_button (1.56 KB, patch)
2017-06-18 19:46 UTC, Rohit Kaushik
committed Details | Review
todoist: Add auxiliary methods for color conversion (3.17 KB, patch)
2017-06-19 20:52 UTC, Rohit Kaushik
none Details | Review
todoist: Show accounts page only if todoist account is present (2.25 KB, patch)
2017-06-19 20:54 UTC, Rohit Kaushik
none Details | Review
todoist: Add auxiliary methods for color conversion (3.09 KB, patch)
2017-06-19 21:02 UTC, Rohit Kaushik
committed Details | Review
todoist: Show accounts page only if todoist account is present (2.77 KB, patch)
2017-06-20 19:42 UTC, Rohit Kaushik
none Details | Review
todoist: fix some issues in preference panel (2.18 KB, patch)
2017-06-20 19:43 UTC, Rohit Kaushik
none Details | Review
todoist: fix some issues in preference panel (2.16 KB, patch)
2017-06-22 03:40 UTC, Rohit Kaushik
committed Details | Review
todoist: Show accounts page only if todoist account is present (3.39 KB, patch)
2017-06-22 05:41 UTC, Rohit Kaushik
none Details | Review
todoist: Show accounts page only if todoist account is present (3.39 KB, patch)
2017-06-23 06:41 UTC, Georges Basile Stavracas Neto
committed Details | Review
todoist: add provider for inital todoist-accounts (1.86 KB, patch)
2017-06-28 12:26 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (10.61 KB, patch)
2017-06-28 12:26 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (10.75 KB, patch)
2017-06-28 15:43 UTC, Rohit Kaushik
none Details | Review
todoist: add provider for inital todoist-accounts (2.12 KB, patch)
2017-07-02 19:20 UTC, Rohit Kaushik
committed Details | Review
todoist: implement loading of tasks and lists (11.33 KB, patch)
2017-07-03 17:49 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (11.50 KB, patch)
2017-07-04 14:43 UTC, Rohit Kaushik
none Details | Review
todoist: implement a generic post method (4.83 KB, patch)
2017-07-04 14:44 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (12.08 KB, patch)
2017-07-05 15:53 UTC, Rohit Kaushik
none Details | Review
todoist: implement a generic post method (4.58 KB, patch)
2017-07-05 17:55 UTC, Rohit Kaushik
none Details | Review
todoist: implement a generic post method (4.63 KB, patch)
2017-07-05 18:31 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (11.59 KB, patch)
2017-07-06 05:58 UTC, Rohit Kaushik
none Details | Review
todoist: Implement todoist tasks/lists sync every 1hour (6.40 KB, patch)
2017-07-10 19:37 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (13.20 KB, patch)
2017-07-12 16:06 UTC, Rohit Kaushik
none Details | Review
todoist: implement a generic post method (7.20 KB, patch)
2017-07-12 16:07 UTC, Rohit Kaushik
none Details | Review
todoist: save local changes to todoist (7.64 KB, patch)
2017-07-12 16:07 UTC, Rohit Kaushik
none Details | Review
todoist: save local changes to todoist (7.88 KB, patch)
2017-07-12 18:02 UTC, Rohit Kaushik
none Details | Review
todoist: move access_token to provider struct (10.14 KB, patch)
2017-07-13 19:05 UTC, Rohit Kaushik
none Details | Review
todoist: implement loading of tasks and lists (13.21 KB, patch)
2017-07-13 21:04 UTC, Georges Basile Stavracas Neto
committed Details | Review
todoist: implement a generic post method (6.51 KB, patch)
2017-07-13 21:05 UTC, Georges Basile Stavracas Neto
committed Details | Review
todoist: save local changes to todoist (8.89 KB, patch)
2017-07-13 21:06 UTC, Georges Basile Stavracas Neto
committed Details | Review
todoist: move access_token to provider struct (10.56 KB, patch)
2017-07-13 21:06 UTC, Georges Basile Stavracas Neto
committed Details | Review
todoist: implement task and list creation (11.54 KB, patch)
2017-07-24 13:38 UTC, Rohit Kaushik
committed Details | Review
todoist: Implement queueing of post requests (11.94 KB, patch)
2017-08-13 16:53 UTC, Rohit Kaushik
none Details | Review
todoist: Implement queueing of post requests (11.53 KB, patch)
2017-08-16 13:44 UTC, Rohit Kaushik
none Details | Review
todoist: Implement todoist tasks/lists sync every 1hour (5.16 KB, patch)
2017-08-17 20:58 UTC, Rohit Kaushik
none Details | Review
todoist-preferences-panel: add parameters to spawn_goa (1.09 KB, patch)
2017-08-19 17:13 UTC, Rohit Kaushik
committed Details | Review
todoist: Implement queueing of post requests (12.47 KB, patch)
2017-08-29 15:09 UTC, Rohit Kaushik
none Details | Review
todoist: Implement queueing of post requests (12.48 KB, patch)
2017-08-30 12:10 UTC, Rohit Kaushik
committed Details | Review
todoist: Implement todoist tasks/lists sync every 1hour (18.84 KB, patch)
2017-08-30 12:35 UTC, Rohit Kaushik
none Details | Review
todoist: Implement todoist tasks/lists sync every 1hour (5.62 KB, patch)
2017-08-30 12:44 UTC, Rohit Kaushik
none Details | Review
todoist: hold todo until request queue is empty (2.06 KB, patch)
2017-09-05 16:35 UTC, Rohit Kaushik
committed Details | Review

Description Khurshid Alam 2016-10-01 06:21:59 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.
Comment 1 Georges Basile Stavracas Neto 2017-04-20 12:07:10 UTC
This will be implemented in the form of a plugin in the near future.
Comment 2 Rohit Kaushik 2017-05-30 18:14:16 UTC
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
Comment 3 Georges Basile Stavracas Neto 2017-06-01 14:33:31 UTC
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.
Comment 4 Rohit Kaushik 2017-06-02 12:49:02 UTC
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
Comment 5 Rohit Kaushik 2017-06-03 18:03:51 UTC
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
Comment 6 Georges Basile Stavracas Neto 2017-06-04 13:31:00 UTC
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.
Comment 7 Rohit Kaushik 2017-06-08 14:40:42 UTC
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
Comment 8 Georges Basile Stavracas Neto 2017-06-08 16:30:04 UTC
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)
Comment 9 Rohit Kaushik 2017-06-08 17:40:16 UTC
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
Comment 10 Georges Basile Stavracas Neto 2017-06-08 19:06:50 UTC
Review of attachment 353415 [details] [review]:

A big patch that is looking pretty good now. Thanks!
Comment 11 Georges Basile Stavracas Neto 2017-06-08 19:07:57 UTC
Comment on attachment 353415 [details] [review]
todoist-plugin: stub out

Attachment 353415 [details] pushed as cf778f8 - todoist-plugin: stub out
Comment 12 Rohit Kaushik 2017-06-09 13:14:09 UTC
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.
Comment 13 Rohit Kaushik 2017-06-14 19:18:45 UTC
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.
Comment 14 Rohit Kaushik 2017-06-15 18:51:10 UTC
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.
Comment 15 Khurshid Alam 2017-06-17 10:03:29 UTC
Will it be possible to sync only particular task lists  of todoist (i.e Projects) from preference?
Comment 16 Georges Basile Stavracas Neto 2017-06-18 17:39:04 UTC
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?
Comment 17 Georges Basile Stavracas Neto 2017-06-18 17:50:26 UTC
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.
Comment 18 Georges Basile Stavracas Neto 2017-06-18 17:52:38 UTC
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
Comment 19 Rohit Kaushik 2017-06-18 19:27:13 UTC
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.
Comment 20 Rohit Kaushik 2017-06-18 19:31:40 UTC
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.
Comment 21 Rohit Kaushik 2017-06-18 19:41:52 UTC
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.
Comment 22 Rohit Kaushik 2017-06-18 19:46:11 UTC
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.
Comment 23 Georges Basile Stavracas Neto 2017-06-19 20:49:38 UTC
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
Comment 24 Georges Basile Stavracas Neto 2017-06-19 20:49:58 UTC
Review of attachment 354005 [details] [review]:

Looks good!
Comment 25 Georges Basile Stavracas Neto 2017-06-19 20:51:39 UTC
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
Comment 26 Rohit Kaushik 2017-06-19 20:52:03 UTC
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.
Comment 27 Rohit Kaushik 2017-06-19 20:54:12 UTC
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.
Comment 28 Rohit Kaushik 2017-06-19 21:02:54 UTC
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.
Comment 29 Georges Basile Stavracas Neto 2017-06-19 21:22:56 UTC
Review of attachment 354058 [details] [review]:

Looks good!
Comment 30 Georges Basile Stavracas Neto 2017-06-19 21:25:19 UTC
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
Comment 31 Rohit Kaushik 2017-06-20 19:42:40 UTC
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.
Comment 32 Rohit Kaushik 2017-06-20 19:43:13 UTC
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.
Comment 33 Georges Basile Stavracas Neto 2017-06-22 03:21:59 UTC
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();"
Comment 34 Georges Basile Stavracas Neto 2017-06-22 03:26:46 UTC
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
Comment 35 Rohit Kaushik 2017-06-22 03:40:20 UTC
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.
Comment 36 Rohit Kaushik 2017-06-22 05:41:17 UTC
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.
Comment 37 Georges Basile Stavracas Neto 2017-06-23 06:35:56 UTC
Review of attachment 354213 [details] [review]:

Looks good!
Comment 38 Georges Basile Stavracas Neto 2017-06-23 06:40:42 UTC
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.
Comment 39 Georges Basile Stavracas Neto 2017-06-23 06:41:10 UTC
Created attachment 354295 [details] [review]
todoist: Show accounts page only if todoist account is present

Fix these comments
Comment 40 Georges Basile Stavracas Neto 2017-06-23 06:50:26 UTC
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
Comment 41 Rohit Kaushik 2017-06-28 12:26:06 UTC
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.
Comment 42 Rohit Kaushik 2017-06-28 12:26:36 UTC
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
Comment 43 Rohit Kaushik 2017-06-28 15:43:35 UTC
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
Comment 44 Georges Basile Stavracas Neto 2017-06-29 21:58:59 UTC
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
Comment 45 Georges Basile Stavracas Neto 2017-06-29 22:12:17 UTC
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.
Comment 46 Rohit Kaushik 2017-07-02 19:20:42 UTC
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.
Comment 47 Rohit Kaushik 2017-07-03 17:49:51 UTC
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
Comment 48 Georges Basile Stavracas Neto 2017-07-04 13:13:06 UTC
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.
Comment 49 Georges Basile Stavracas Neto 2017-07-04 13:24:00 UTC
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)
Comment 50 Rohit Kaushik 2017-07-04 14:43:57 UTC
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
Comment 51 Rohit Kaushik 2017-07-04 14:44:22 UTC
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.
Comment 52 Georges Basile Stavracas Neto 2017-07-05 13:02:42 UTC
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.
Comment 53 Georges Basile Stavracas Neto 2017-07-05 13:05:17 UTC
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.
Comment 54 Rohit Kaushik 2017-07-05 15:53:11 UTC
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
Comment 55 Rohit Kaushik 2017-07-05 17:55:43 UTC
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.
Comment 56 Georges Basile Stavracas Neto 2017-07-05 18:00:18 UTC
Review of attachment 354934 [details] [review]:

Looks good!
Comment 57 Georges Basile Stavracas Neto 2017-07-05 18:02:20 UTC
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
Comment 58 Rohit Kaushik 2017-07-05 18:31:22 UTC
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.
Comment 59 Georges Basile Stavracas Neto 2017-07-05 18:35:42 UTC
Review of attachment 354955 [details] [review]:

Looks god
Comment 60 Georges Basile Stavracas Neto 2017-07-06 01:12:29 UTC
Applying these patches leads to dozens of compile warnings.

Here: https://paste.gnome.org/poscrhxqr (available for 1 year from now)
Comment 61 Rohit Kaushik 2017-07-06 05:58:15 UTC
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
Comment 62 Rohit Kaushik 2017-07-06 06:04:47 UTC
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.
Comment 63 Rohit Kaushik 2017-07-10 19:37:58 UTC
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.
Comment 64 Rohit Kaushik 2017-07-12 16:06:35 UTC
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
Comment 65 Rohit Kaushik 2017-07-12 16:07:06 UTC
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.
Comment 66 Rohit Kaushik 2017-07-12 16:07:30 UTC
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.
Comment 67 Rohit Kaushik 2017-07-12 18:02:17 UTC
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.
Comment 68 Rohit Kaushik 2017-07-13 19:05:30 UTC
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.
Comment 69 Georges Basile Stavracas Neto 2017-07-13 20:23:41 UTC
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
Comment 70 Georges Basile Stavracas Neto 2017-07-13 20:26:20 UTC
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)
Comment 71 Georges Basile Stavracas Neto 2017-07-13 21:04:59 UTC
Created attachment 355553 [details] [review]
todoist: implement loading of tasks and lists

Add the fixes.
Comment 72 Georges Basile Stavracas Neto 2017-07-13 21:05:49 UTC
Created attachment 355554 [details] [review]
todoist: implement a generic post method
Comment 73 Georges Basile Stavracas Neto 2017-07-13 21:06:09 UTC
Created attachment 355555 [details] [review]
todoist: save local changes to todoist
Comment 74 Georges Basile Stavracas Neto 2017-07-13 21:06:51 UTC
Created attachment 355556 [details] [review]
todoist: move access_token to provider struct
Comment 75 Georges Basile Stavracas Neto 2017-07-14 02:27:19 UTC
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
Comment 76 Rohit Kaushik 2017-07-24 13:38:57 UTC
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.
Comment 77 Georges Basile Stavracas Neto 2017-08-09 04:55:15 UTC
Review of attachment 355294 [details] [review]:

Does not apply anymore.
Comment 78 Georges Basile Stavracas Neto 2017-08-09 04:56:37 UTC
Comment on attachment 356298 [details] [review]
todoist: implement task and list creation

Attachment 356298 [details] pushed as c0e17a1 - todoist: implement task and list creation
Comment 79 Rohit Kaushik 2017-08-13 16:53:07 UTC
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.
Comment 80 Rohit Kaushik 2017-08-16 13:44:59 UTC
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.
Comment 81 Rohit Kaushik 2017-08-17 20:58:12 UTC
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.
Comment 82 Rohit Kaushik 2017-08-19 17:13:48 UTC
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.
Comment 83 Georges Basile Stavracas Neto 2017-08-25 13:30:48 UTC
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)
Comment 84 Georges Basile Stavracas Neto 2017-08-25 13:33:12 UTC
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()
Comment 85 Georges Basile Stavracas Neto 2017-08-25 13:34:15 UTC
Review of attachment 357985 [details] [review]:

LGTM
Comment 86 Georges Basile Stavracas Neto 2017-08-25 13:37:13 UTC
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
Comment 87 Rohit Kaushik 2017-08-29 15:09:06 UTC
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.
Comment 88 Rohit Kaushik 2017-08-30 12:10:32 UTC
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.
Comment 89 Rohit Kaushik 2017-08-30 12:35:29 UTC
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.
Comment 90 Rohit Kaushik 2017-08-30 12:44:25 UTC
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.
Comment 91 Rohit Kaushik 2017-09-05 16:35:21 UTC
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 92 Georges Basile Stavracas Neto 2017-10-11 02:56:21 UTC
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 93 Georges Basile Stavracas Neto 2017-10-11 03:04:52 UTC
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
Comment 94 André Klapper 2020-11-25 16:21:18 UTC
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).