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 780138 - Add a provider for Todoist
Add a provider for Todoist
Status: RESOLVED FIXED
Product: gnome-online-accounts
Classification: Core
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME Online Accounts maintainer(s)
GNOME Online Accounts maintainer(s)
Depends on:
Blocks: 775854
 
 
Reported: 2017-03-16 12:04 UTC by Matthias Clasen
Modified: 2018-10-03 10:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ListProvider: Add ListProvider method to Goa (12.86 KB, patch)
2017-06-03 15:33 UTC, Rohit Kaushik
needs-work Details | Review
daemon: Add ListProviders method (14.73 KB, patch)
2017-06-06 05:09 UTC, Rohit Kaushik
none Details | Review
backendenums: Move GoaProviderFeatures enum to goaenum.h (5.35 KB, patch)
2017-06-07 17:08 UTC, Rohit Kaushik
none Details | Review
daemon: Add ListProviders method (15.30 KB, patch)
2017-06-07 18:17 UTC, Rohit Kaushik
none Details | Review
Add Todoist Support (21.56 KB, patch)
2017-06-11 18:42 UTC, Ekta Nandwani
none Details | Review
Add Todoist Support (21.56 KB, patch)
2017-06-12 12:42 UTC, Ekta Nandwani
none Details | Review
Add Todoist Support (21.55 KB, patch)
2017-06-12 17:51 UTC, Ekta Nandwani
none Details | Review
Add Todoist Support (21.56 KB, patch)
2017-06-13 19:03 UTC, Ekta Nandwani
committed Details | Review
daemon: Add AddAccountForProvider method (9.86 KB, patch)
2017-06-14 14:30 UTC, Rohit Kaushik
none Details | Review
daemon: Add AddAccountForProvider method (9.86 KB, patch)
2017-06-14 14:34 UTC, Rohit Kaushik
none Details | Review
daemon: Add AddAccountForProvider method (13.74 KB, patch)
2017-06-15 15:26 UTC, Rohit Kaushik
none Details | Review
Introduce org.gnome.OnlineAccounts.Todo (7.89 KB, patch)
2017-06-16 17:10 UTC, Debarshi Ray
committed Details | Review
Add Todoist (17.72 KB, patch)
2017-06-16 17:11 UTC, Debarshi Ray
committed Details | Review
daemon: Add AddAccountForProvider method (15.18 KB, patch)
2017-06-16 18:54 UTC, Rohit Kaushik
none Details | Review
gnome-control-center: Hide Todoist from providers_listbox (1.26 KB, patch)
2017-06-17 19:02 UTC, Ekta Nandwani
none Details | Review
gnome-online-accounts patch to implement "hidden" property (4.44 KB, patch)
2017-06-17 19:07 UTC, Ekta Nandwani
none Details | Review
backendenums: Move GoaProviderFeatures enum to goaenum.h (5.44 KB, patch)
2017-08-18 10:40 UTC, Rohit Kaushik
none Details | Review
daemon: Add ListProviders method (15.34 KB, patch)
2017-08-18 10:41 UTC, Rohit Kaushik
none Details | Review
daemon: Add AddAccountForProvider method (15.20 KB, patch)
2017-08-18 10:42 UTC, Rohit Kaushik
none Details | Review
todoist: Change scope to add 'project:delete' (826 bytes, patch)
2017-08-20 08:01 UTC, Ekta Nandwani
none Details | Review

Description Matthias Clasen 2017-03-16 12:04:04 UTC
Both gnome-todo and recipes want to use Todoist as a way to sync lists to mobile devices. We should share a provider for it in gnome-online-accounts.

Todoist is using oauth2, so this should be a fairly standard thing.
Comment 1 Allan Day 2017-03-21 11:56:49 UTC
What about when Todo and Recipes aren't installed? Wouldn't you end up with a Todoist online account that is essentially useless?
Comment 2 Rohit Kaushik 2017-06-03 15:33:21 UTC
Created attachment 353115 [details] [review]
ListProvider: Add ListProvider method to Goa

This patch adds ListProviders method and is the first step towards
GoaPortal. This can be used by apps that require some provider to
be present e.g GNOME To Do which will use Todoist Account.
If present, these apps can call AddAccount on the portal.
Comment 3 Georges Basile Stavracas Neto 2017-06-05 12:07:01 UTC
Review of attachment 353115 [details] [review]:

Needs some works. The commit title should be "daemon: Add ListProviders method".

::: data/dbus-interfaces.xml
@@ +410,3 @@
+    <!--
+        ListProviders:
+        @out_providers: A GVaraint of type "(a(sssu))" consisting of all the providers.

You should describe what each value stands for. Like:

s → Provider unique identifier
s → Provider name
...

::: src/daemon/goadaemon.c
@@ +87,3 @@
 static void goa_daemon_check_credentials (GoaDaemon *self);
 static void goa_daemon_reload_configuration (GoaDaemon *self);
+static GVariant* build_variant (GList *providers);

Rename to "build_providers_variant" & jump line below.

@@ +1296,3 @@
+{
+  GList *l;
+  GVariant *params[100];

- Rename this to "providers".
- This should not be a hardcoded number.
- You can use a GPtrArray here, and not care about the size.

@@ +1299,3 @@
+  GVariant *array[1];
+  l = NULL;
+  guint i =0;

Wrong style. Don't mix variable declarations and assignments.

@@ +1325,3 @@
+  array[0] = g_variant_new_array ("(sssu)", params, i);
+
+  return g_variant_new_tuple (array, 1);

Why are you returning a tuple here? Shouldn't you return "g_variant_new_array ()"?

@@ +1365,3 @@
+    g_error_free (data.error);
+    goto out;
+  }

Wrong style. GOA uses GNU C Coding Style.

::: src/goa/goaclient.c
@@ +506,3 @@
+      provider_info->name = g_variant_dup_string (name, &sz);
+      provider_info->icon = g_variant_dup_string (icon, &sz);
+      out_providers = g_list_append (out_providers, provider_info);

Use g_list_prepend() and, after the loop, call g_list_revert().

@@ +535,3 @@
+                     cancellable,
+                     (GAsyncReadyCallback) callback,
+                     user_data);

I belive gdbus-codegen provides a facility for this. Something like "goa_manager_call_list_providers()".

@@ +545,3 @@
+ * Finishes an operation started with goa_client_list_providers().
+ *
+ * Returns: A #GList or %NULL if @error is set. Free with

Returns: (transfer full):

@@ +550,3 @@
+GList*
+goa_client_list_providers_finish (GAsyncResult *res,
+                                  GError       *error)

This should be "GError **error"

@@ +559,3 @@
+  out_providers = g_dbus_proxy_call_finish (proxy,
+                                            res,
+                                            &error);

Gdbus-codegen should provide a "goa_manager_call_list_providers_finish()" too. This should be "error" only.

@@ +564,3 @@
+    return NULL;
+
+  return parse_variant_to_provider_info (out_providers);;

Double ;

@@ +575,3 @@
+ * infos currently supported by GOA.
+ *
+ * Returns: A #GList or %NULL if @error is set. Free with

Returns: (transfer full):

@@ +581,3 @@
+goa_client_list_providers_sync (GoaClient     *self,
+                                GCancellable  *cancellable,
+                                GError        *error)

Should be "GError **error"

@@ +591,3 @@
+                                          -1,
+                                          cancellable,
+                                          &error);

Again, you should use "goa_manager_call_list_providers_sync()" provided by gdbus-codegen

::: src/goa/goaclient.h
@@ +55,1 @@
+typedef struct GoaProviderInfo

Should be just "typedef struct"

@@ +58,3 @@
+  gchar *id;
+  gchar *icon;
+}GoaProviderInfo;

Space after }
Comment 4 Rohit Kaushik 2017-06-06 05:09:51 UTC
Created attachment 353235 [details] [review]
daemon: Add ListProviders method

This patch adds ListProviders method and is the first step towards
GoaPortal. This can be used by apps that require some provider to
be present e.g GNOME To Do which will use Todoist Account.
If present, these apps can call AddAccount on the portal.
Comment 5 Georges Basile Stavracas Neto 2017-06-06 12:08:44 UTC
Review of attachment 353235 [details] [review]:

More comments. I think you should add a patch that moves GoaProviderFeatures to libgoa as well, so you can reuse this enum.

::: src/daemon/goadaemon.c
@@ +1318,3 @@
+      features = goa_provider_get_provider_features (provider);
+
+      g_ptr_array_add (params, g_variant_new (G_VARIANT_TYPE ("(sssu)"), type, name, icon, features));

Put this g_variant_new() in a "variant" variable, and call "g_ptr_array_add (params, variant)"

@@ +1324,3 @@
+    }
+
+  array[0] = g_variant_new_array ("(sssu)", params, params->len);

Free the GPtrArray after using it.

@@ +1349,3 @@
+{
+  GoaDaemon *self = GOA_DAEMON (user_data);
+  ListProvidersData data = {0,};

Nitpick: should be "{ 0, };" (with spaces)

::: src/goa/goaclient.c
@@ +490,3 @@
+      GVariant *name;
+      GVariant *icon;
+      GVariant *features;

Use g_autoptr on these variants.

@@ +493,3 @@
+      gsize sz;
+
+      provider_info = g_slice_new0 (GoaProviderInfo);

Even though g_slice_new0() has a few performance advantages, I think it'll be better if you just use g_new0() so consumers of this API can just free with g_free().

@@ +509,3 @@
+  out_providers = g_list_reverse (out_providers);
+
+  return out_providers;

Before returning, you should unref() the GVariant *var, otherwise it'll leak.

@@ +524,3 @@
+goa_client_list_providers (GCancellable        *cancellable,
+                           GAsyncReadyCallback  callback,
+                           gpointer             user_data)

You must receive a GoaClient *self as one of the parameters. 'user_data' can be anything.

@@ +526,3 @@
+                           gpointer             user_data)
+{
+  GoaClient *self = GOA_CLIENT (user_data);

Please don't. You should receive 'self' as a param.

On a side note, it is important for libraries to check the params before. Add this:

g_return_if_fail (GOA_IS_CLIENT (self));

@@ +542,3 @@
+ *
+ * Returns:(transfer full) A #GList or %NULL if @error is set. Free with
+ * g_list_free() when done with it.

Mention that the GoaProviderInfo elements should be freed with g_free()

@@ +551,3 @@
+  GoaManager *proxy;
+
+  proxy = GOA_MANAGER (g_async_result_get_source_object (res));

Before this, you should the following checks using g_return_val_if_fail():

- G_IS_ASYNC_RESULT (res)
- !error || !*error

@@ +559,3 @@
+
+  if (error)
+    return NULL;

This should be "if (error && *error)"

@@ +567,3 @@
+ * goa_client_list_providers_sync:
+ * @cancellable: (allow-none): A #GCancellable or %NULL.
+ * @error: (allow-none): Return location for error or %NULL.

This doc comment is missing the "GoaClient *self" param.

@@ +573,3 @@
+ *
+ * Returns:(transfer full) A #GList or %NULL if @error is set. Free with
+ * g_list_free() when done with it.

Mention that the GoaProviderInfo elements should be freed with g_free()

@@ +580,3 @@
+                                GError       **error)
+{
+  GVariant *out_providers;

After declaring this, you should the following checks using g_return_val_if_fail():

- GOA_IS_CLIENT (self)
- !error || !*error

@@ +588,3 @@
+
+  if (error)
+    return NULL;

This should be "if (error && *error)"

::: src/goa/goaclient.h
@@ +58,3 @@
+  gchar *id;
+  gchar *icon;
+} GoaProviderInfo;

What about the GoaProviderFeatures that is received through DBus?
Comment 6 Rohit Kaushik 2017-06-07 17:08:39 UTC
Created attachment 353350 [details] [review]
backendenums: Move GoaProviderFeatures enum to goaenum.h

Since GoaProviderFeatures will be used by GoaProviderInfo which
is returned by libgoa for ListProviders Method, it is necessary
to move GoaProviderFeatures to libgoa.
Comment 7 Rohit Kaushik 2017-06-07 18:17:39 UTC
Created attachment 353354 [details] [review]
daemon: Add ListProviders method

This patch adds ListProviders method and is the first step towards
GoaPortal. This can be used by apps that require some provider to
be present e.g GNOME To Do which will use Todoist Account.
If present, these apps can call AddAccount on the portal.
Comment 8 Georges Basile Stavracas Neto 2017-06-08 20:04:38 UTC
Review of attachment 353354 [details] [review]:

It looks good enough now. From now on, it's on Debarshi's hands.
Comment 9 Georges Basile Stavracas Neto 2017-06-08 20:10:53 UTC
Review of attachment 353350 [details] [review]:

Looks good enough too.
Comment 10 Ekta Nandwani 2017-06-11 18:42:05 UTC
Created attachment 353574 [details] [review]
Add Todoist Support
Comment 11 Matthias Clasen 2017-06-12 10:25:41 UTC
Review of attachment 353574 [details] [review]:

::: configure.ac
@@ +376,3 @@
+# TODOIST
+AC_DEFINE(GOA_TODOIST_NAME, ["todoist"], [ProviderType and extension point name])
+AC_ARG_ENABLE([todoist], [AS_HELP_STRING([--enable-todosit],

typo: this should probably be --enable-todoist

::: data/dbus-interfaces.xml
@@ +263,3 @@
+
+         If %TRUE, the account will not expose any
+        #org.gnome.OnlineAccounts.ReadLater interface. If the account does not

This should be org.gnome.OnlineAccounts.TodoList, I assume

::: src/goabackend/goatodoistprovider.c
@@ +61,3 @@
+                   GoaObject   *object)
+{
+  return g_strdup (_("Todoist"));

Can this be meaningfully translated ? Its a name...
Comment 12 Ekta Nandwani 2017-06-12 12:42:01 UTC
Created attachment 353600 [details] [review]
Add Todoist Support
Comment 13 Matthias Clasen 2017-06-12 14:19:04 UTC
Review of attachment 353600 [details] [review]:

::: data/dbus-interfaces.xml
@@ +266,3 @@
+         provide read-later-like capabilities, this property does nothing.
+
+         Note that the #org.gnomeOnlineAccounts.ReadLater interface is added or

One more ReadLater here...
Comment 14 Ekta Nandwani 2017-06-12 17:51:42 UTC
Created attachment 353618 [details] [review]
Add Todoist Support

Apologies for all those copy paste errors.
Comment 15 Debarshi Ray 2017-06-12 19:24:16 UTC
Review of attachment 353618 [details] [review]:

Thanks for the revised patch, Ekta! This already looks quite good.

Could you please update the README with the developer documentation? Is it possible to have multiple owners of the Todoist API key? Some other comments below:

::: configure.ac
@@ +374,3 @@
 fi
 
+# TODOIST

Nitpick: Todoist, not TODOIST. Would be nice to retain some kind of alphabetical ordering. Facebook and Pocket are a bit out of place. Maybe put it between ownCloud and Windows Live?

@@ +379,3 @@
+                                              [Enable Todoist provider])],
+                                              [],
+                                              [enable_todoist=yes])

It might be better to keep it disabled by default while we work out the rest of the details: enable_todoist=no. You can build it locally with --enable-todoist.

::: data/dbus-interfaces.xml
@@ +259,3 @@
     <property name="ReadLaterDisabled" type="b" access="readwrite"/>
 
+    <!-- TodoListDisabled:

Did you consider "Todo" instead of "TodoList"?

The rule of thumb is to match the interface name with that of the application and/or a generic service name that will be the switch's label in the Online Accounts panel. Since we already have gnome-todo, and we are likely to label the switch "To Do", it looks like a better fit. It's also shorter, so less typing.

@@ +260,3 @@
 
+    <!-- TodoListDisabled:
+         @since: 3.12.0

3.26.0, not 3.12.0

@@ +263,3 @@
+
+         If %TRUE, the account will not expose any
+        #org.gnome.OnlineAccounts.TodoList interface. If the account does not

Nitpick: the alignment is off.

@@ +804,3 @@
   <!--
+      org.gnome.OnlineAccounts.TodoList:
+      @since: 3.12.0

Ditto.

::: src/goabackend/goaprovider.c
@@ +174,3 @@
     .blurb = N_("_Maps"),
   },
+    {

Nitpick: alignment.

@@ +177,3 @@
+    .feature = GOA_PROVIDER_FEATURE_TODO_LIST,
+    .property = "todo-list-disabled",
+    .blurb = N_("_TodoList"),

This string is going to be the label for the switch in the panel. Also see above. So it cannot be in camel case. Secondly, we can't place the underscore before the 'T' because it is already taken by 'printers' above. So, maybe 'o'? ie. "T_o Do"?

::: src/goabackend/goatodoistprovider.c
@@ +1,2 @@
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
+/*

The copyright notice is absent. See https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/goamediaserverprovider.c#n3

@@ +31,3 @@
+struct _GoaTodoistProvider
+{
+  /*< private >*/

This comment shouldn't be necessary anymore.

@@ +61,3 @@
+                   GoaObject   *object)
+{
+  return g_strdup ("Todoist");

Should be marked for translation: _("Todoist")

@@ +86,3 @@
+get_scope (GoaOAuth2Provider *oauth2_provider)
+{
+  return "data:read,data:delete";

Umm... don't you want to add tasks too?

@@ +115,3 @@
+static const gchar *
+get_authentication_cookie (GoaOAuth2Provider *oauth2_provider)
+{

This shouldn't be needed anymore.

@@ +128,3 @@
+  gchar *state;
+  gchar *uri;
+  state = g_uuid_string_random ();

The glib-2.0 requirement should be bumped to 2.52 in configure.ac.

::: src/goabackend/goatodoistprovider.h
@@ +29,3 @@
+#define GOA_TYPE_TODOIST_PROVIDER   (goa_todoist_provider_get_type ())
+#define GOA_TODOIST_PROVIDER(o)     (G_TYPE_CHECK_INSTANCE_CAST ((o), GOA_TYPE_TODOIST_PROVIDER, GoaTodoistProvider))
+#define GOA_IS_TODOIST_PROVIDER(o)  (G_TYPE_CHECK_INSTANCE_TYPE ((o), GOA_TYPE_TODOIST_PROVIDER))

G_DECLARE_FINAL_TYPE would have reduced some typing for you. :) See: https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/goafacebookprovider.h
Comment 16 Matthias Clasen 2017-06-12 19:40:00 UTC
Review of attachment 353618 [details] [review]:

::: src/goabackend/goatodoistprovider.c
@@ +61,3 @@
+                   GoaObject   *object)
+{
+  return g_strdup ("Todoist");

I questioned that earlier - can you really meaningfully translate a name like that ?
Comment 17 Debarshi Ray 2017-06-12 21:29:05 UTC
(In reply to Matthias Clasen from comment #16)
> Review of attachment 353618 [details] [review] [review]:
> 
> ::: src/goabackend/goatodoistprovider.c
> @@ +61,3 @@
> +                   GoaObject   *object)
> +{
> +  return g_strdup ("Todoist");
> 
> I questioned that earlier - can you really meaningfully translate a name
> like that ?

While most languages retain the English name in the Latin script, some do translate them. Some are mere transliterations of the original, but I can't read Persian or Arabic, so I don't know if that's all they do.

A real concern might be a language where the English name sounds like an insult or something. *shrug*

My original intent was to not get in the way of the translation teams.
Comment 18 Debarshi Ray 2017-06-12 21:37:07 UTC
Review of attachment 353618 [details] [review]:

::: src/goabackend/goatodoistprovider.c
@@ +158,3 @@
+
+  id = webkit_dom_element_get_id (WEBKIT_DOM_ELEMENT (element));
+  if (g_strcmp0 (id, "email") != 0)

I am quite amazed that you managed to figure this one out!

@@ +189,3 @@
+  proxy = rest_proxy_new ("https://todoist.com/API/v7/sync", FALSE);
+  call = rest_proxy_new_call (proxy);
+  rest_proxy_call_set_method (call, "GET");

I cannot find a mention of this call in the documentation. https://developer.todoist.com/#user doesn't seem to have it. How did you dig it up? Just curious.
Comment 19 Ekta Nandwani 2017-06-12 23:57:50 UTC
(In reply to Debarshi Ray from comment #18)
> Review of attachment 353618 [details] [review] [review]:
> 
> ::: src/goabackend/goatodoistprovider.c
> @@ +158,3 @@
> +
> +  id = webkit_dom_element_get_id (WEBKIT_DOM_ELEMENT (element));
> +  if (g_strcmp0 (id, "email") != 0)
> 
> I am quite amazed that you managed to figure this one out!
 
Thanks rishi! :) F12 works like a charm.

> @@ +189,3 @@
> +  proxy = rest_proxy_new ("https://todoist.com/API/v7/sync", FALSE);
> +  call = rest_proxy_new_call (proxy);
> +  rest_proxy_call_set_method (call, "GET");
> 
> I cannot find a mention of this call in the documentation.
> https://developer.todoist.com/#user doesn't seem to have it. How did you dig
> it up? Just curious.

And here : https://developer.todoist.com/#read-resources
Comment 20 Debarshi Ray 2017-06-13 07:57:28 UTC
(In reply to Ekta Nandwani from comment #19)
> (In reply to Debarshi Ray from comment #18)
> > Review of attachment 353618 [details] [review] [review] [review]:
> > 
> > ::: src/goabackend/goatodoistprovider.c
> > @@ +158,3 @@
> > +
> > +  id = webkit_dom_element_get_id (WEBKIT_DOM_ELEMENT (element));
> > +  if (g_strcmp0 (id, "email") != 0)
> > 
> > I am quite amazed that you managed to figure this one out!
>  
> Thanks rishi! :) F12 works like a charm.

You still need --enable-inspector for that, don't you?
Comment 21 Ekta Nandwani 2017-06-13 11:00:55 UTC
(In reply to Debarshi Ray from comment #20)
> (In reply to Ekta Nandwani from comment #19)
> > (In reply to Debarshi Ray from comment #18)
> > > Review of attachment 353618 [details] [review] [review] [review] [review]:
> > > 
> > > ::: src/goabackend/goatodoistprovider.c
> > > @@ +158,3 @@
> > > +
> > > +  id = webkit_dom_element_get_id (WEBKIT_DOM_ELEMENT (element));
> > > +  if (g_strcmp0 (id, "email") != 0)
> > > 
> > > I am quite amazed that you managed to figure this one out!
> >  
> > Thanks rishi! :) F12 works like a charm.
> 
> You still need --enable-inspector for that, don't you?

Before I started writing a provider, I played with curl and the description for build_authorization_uri() said "Builds the URI that can be opened in a web browser (or embedded web browser widget) to start authenticating an user." so I opened it in chrome and found the identity_node.That's how I did it.
Comment 22 Ekta Nandwani 2017-06-13 19:03:29 UTC
Created attachment 353701 [details] [review]
Add Todoist Support

Revised patch with changes in the review.
Comment 23 Matthias Clasen 2017-06-13 19:06:52 UTC
Review of attachment 353701 [details] [review]:

Looks good to me now
Comment 24 Ekta Nandwani 2017-06-14 01:28:39 UTC
(In reply to Debarshi Ray from comment #15)
> Review of attachment 353618 [details] [review] [review]:
> 
> Thanks for the revised patch, Ekta! This already looks quite good.
> 
> Could you please update the README with the developer documentation? Is it
> possible to have multiple owners of the Todoist API key? Some other comments
> below:
> 

The documentation has no sign of multiple ownership of API key.

> ::: configure.ac
> @@ +374,3 @@
>  fi
>  
> +# TODOIST
> 
> Nitpick: Todoist, not TODOIST. Would be nice to retain some kind of
> alphabetical ordering. Facebook and Pocket are a bit out of place. Maybe put
> it between ownCloud and Windows Live?
> 
> @@ +379,3 @@
> +                                              [Enable Todoist provider])],
> +                                              [],
> +                                              [enable_todoist=yes])
> 
> It might be better to keep it disabled by default while we work out the rest
> of the details: enable_todoist=no. You can build it locally with
> --enable-todoist.
> 
> ::: data/dbus-interfaces.xml
> @@ +259,3 @@
>      <property name="ReadLaterDisabled" type="b" access="readwrite"/>
>  
> +    <!-- TodoListDisabled:
> 
> Did you consider "Todo" instead of "TodoList"?
> 
> The rule of thumb is to match the interface name with that of the
> application and/or a generic service name that will be the switch's label in
> the Online Accounts panel. Since we already have gnome-todo, and we are
> likely to label the switch "To Do", it looks like a better fit. It's also
> shorter, so less typing.
> 
> @@ +260,3 @@
>  
> +    <!-- TodoListDisabled:
> +         @since: 3.12.0
> 
> 3.26.0, not 3.12.0
> 
> @@ +263,3 @@
> +
> +         If %TRUE, the account will not expose any
> +        #org.gnome.OnlineAccounts.TodoList interface. If the account does
> not
> 
> Nitpick: the alignment is off.
> 
> @@ +804,3 @@
>    <!--
> +      org.gnome.OnlineAccounts.TodoList:
> +      @since: 3.12.0
> 
> Ditto.
> 
> ::: src/goabackend/goaprovider.c
> @@ +174,3 @@
>      .blurb = N_("_Maps"),
>    },
> +    {
> 
> Nitpick: alignment.
> 
> @@ +177,3 @@
> +    .feature = GOA_PROVIDER_FEATURE_TODO_LIST,
> +    .property = "todo-list-disabled",
> +    .blurb = N_("_TodoList"),
> 
> This string is going to be the label for the switch in the panel. Also see
> above. So it cannot be in camel case. Secondly, we can't place the
> underscore before the 'T' because it is already taken by 'printers' above.
> So, maybe 'o'? ie. "T_o Do"?
> 
> ::: src/goabackend/goatodoistprovider.c
> @@ +1,2 @@
> +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
> +/*
> 
> The copyright notice is absent. See
> https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/
> goamediaserverprovider.c#n3
> 
> @@ +31,3 @@
> +struct _GoaTodoistProvider
> +{
> +  /*< private >*/
> 
> This comment shouldn't be necessary anymore.
> 
> @@ +61,3 @@
> +                   GoaObject   *object)
> +{
> +  return g_strdup ("Todoist");
> 
> Should be marked for translation: _("Todoist")
> 
> @@ +86,3 @@
> +get_scope (GoaOAuth2Provider *oauth2_provider)
> +{
> +  return "data:read,data:delete";
> 
> Umm... don't you want to add tasks too?
> 
> @@ +115,3 @@
> +static const gchar *
> +get_authentication_cookie (GoaOAuth2Provider *oauth2_provider)
> +{
> 
> This shouldn't be needed anymore.
> 
> @@ +128,3 @@
> +  gchar *state;
> +  gchar *uri;
> +  state = g_uuid_string_random ();
> 
> The glib-2.0 requirement should be bumped to 2.52 in configure.ac.
> 
> ::: src/goabackend/goatodoistprovider.h
> @@ +29,3 @@
> +#define GOA_TYPE_TODOIST_PROVIDER   (goa_todoist_provider_get_type ())
> +#define GOA_TODOIST_PROVIDER(o)     (G_TYPE_CHECK_INSTANCE_CAST ((o),
> GOA_TYPE_TODOIST_PROVIDER, GoaTodoistProvider))
> +#define GOA_IS_TODOIST_PROVIDER(o)  (G_TYPE_CHECK_INSTANCE_TYPE ((o),
> GOA_TYPE_TODOIST_PROVIDER))
> 
> G_DECLARE_FINAL_TYPE would have reduced some typing for you. :) See:
> https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/
> goafacebookprovider.h
Comment 25 Rohit Kaushik 2017-06-14 14:30:34 UTC
Created attachment 353744 [details] [review]
daemon: Add AddAccountForProvider method

This patch implements a DBus method, that spawns up
Control panel along with the add account dialog for
the provider_type sent as argument.
Comment 26 Rohit Kaushik 2017-06-14 14:34:50 UTC
Created attachment 353745 [details] [review]
daemon: Add AddAccountForProvider method

This patch implements a DBus method, that spawns up
Control panel along with the add account dialog for
the provider_type sent as argument.
Comment 27 Matthias Clasen 2017-06-14 20:11:58 UTC
Review of attachment 353745 [details] [review]:

::: src/daemon/goadaemon.c
@@ +1351,3 @@
+    {
+      g_warning ("Couldn't open Online Accounts panel");
+      return;

This should probably be returned as an error ?

@@ +1361,3 @@
+                          NULL,
+                          NULL);
+

You are leaking the return value of g_dbus_proxy_call_sync_here(), and you are not handling errors.

@@ +1373,3 @@
+  spawn_goa_with_args ("add", provider_type);
+
+  return TRUE; /* invocation was handled */

If the invocation was handled, somebody should have called
g_dbus_method_invocation_return_value (invocation, NULL) - I don't see that happen here.
Comment 28 Rohit Kaushik 2017-06-15 15:26:38 UTC
Created attachment 353836 [details] [review]
daemon: Add AddAccountForProvider method

This patch implements a DBus method, that spawns up
Control panel along with the add account dialog for
the provider_type sent as argument.
Comment 29 Debarshi Ray 2017-06-16 17:09:26 UTC
Review of attachment 353701 [details] [review]:

::: configure.ac
@@ +86,3 @@
 #
 
+PKG_CHECK_MODULES(GLIB, [glib-2.52 gio-2.0 gio-unix-2.0 >= 2.44])

It should be glib-2.0, but >= 2.52.

::: src/goabackend/goatodoistprovider.c
@@ +115,3 @@
+  gchar *state;
+  gchar *uri;
+  state = g_uuid_string_random ();

We are leaking 'state'.

@@ +179,3 @@
+  rest_proxy_call_add_param (call, "token", access_token);
+  rest_proxy_call_add_param (call, "sync_token", "*");
+  rest_proxy_call_add_param (call, "resource_types", "[\"all\"]");

Since we are only interested in the 'user' object, how about only asking for it instead of 'all'?
Comment 30 Debarshi Ray 2017-06-16 17:10:19 UTC
Created attachment 353905 [details] [review]
Introduce org.gnome.OnlineAccounts.Todo
Comment 31 Debarshi Ray 2017-06-16 17:11:12 UTC
Created attachment 353906 [details] [review]
Add Todoist

I took the liberty to fix those little things and add some documentation. Thanks for working on this.
Comment 32 Rohit Kaushik 2017-06-16 18:54:48 UTC
Created attachment 353918 [details] [review]
daemon: Add AddAccountForProvider method

This patch implements a DBus method, that spawns up
Control panel along with the add account dialog for
the provider_type sent as argument.
Comment 33 Ekta Nandwani 2017-06-17 19:02:43 UTC
Created attachment 353963 [details] [review]
gnome-control-center: Hide Todoist from providers_listbox

Use "hidden" property from online-accounts to hide the provider if its account isn't present
Comment 34 Ekta Nandwani 2017-06-17 19:07:54 UTC
Created attachment 353964 [details] [review]
gnome-online-accounts patch to implement "hidden" property

If this property is set, it is used to hide the provider(Todoist in this case) from control panel if the account for the same isn't present.
Comment 35 Rohit Kaushik 2017-08-18 10:40:49 UTC
Created attachment 357879 [details] [review]
backendenums: Move GoaProviderFeatures enum to goaenum.h

Since GoaProviderFeatures will be used by GoaProviderInfo which
is returned by libgoa for ListProviders Method, it is necessary
to move GoaProviderFeatures to libgoa.
Comment 36 Rohit Kaushik 2017-08-18 10:41:35 UTC
Created attachment 357880 [details] [review]
daemon: Add ListProviders method

This patch adds ListProviders method and is the first step towards
GoaPortal. This can be used by apps that require some provider to
be present e.g GNOME To Do which will use Todoist Account.
If present, these apps can call AddAccount on the portal.
Comment 37 Rohit Kaushik 2017-08-18 10:42:03 UTC
Created attachment 357881 [details] [review]
daemon: Add AddAccountForProvider method

This patch implements a DBus method, that spawns up
Control panel along with the add account dialog for
the provider_type sent as argument.
Comment 38 Rohit Kaushik 2017-08-18 10:45:12 UTC
I have updated my patches so now they don't cause any merge conflicts.
Comment 39 Ekta Nandwani 2017-08-20 08:01:32 UTC
Created attachment 358006 [details] [review]
todoist: Change scope to add 'project:delete'
Comment 40 Debarshi Ray 2018-10-03 10:08:22 UTC
We already added the Todoist provider some time ago; and since then decided to carry the Todoist integration in the applications themselves.

Closing.