GNOME Bugzilla – Bug 780138
Add a provider for Todoist
Last modified: 2018-10-03 10:08:22 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.
What about when Todo and Recipes aren't installed? Wouldn't you end up with a Todoist online account that is essentially useless?
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.
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 }
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.
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?
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.
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.
Review of attachment 353354 [details] [review]: It looks good enough now. From now on, it's on Debarshi's hands.
Review of attachment 353350 [details] [review]: Looks good enough too.
Created attachment 353574 [details] [review] Add Todoist Support
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...
Created attachment 353600 [details] [review] Add Todoist Support
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...
Created attachment 353618 [details] [review] Add Todoist Support Apologies for all those copy paste errors.
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
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 ?
(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.
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.
(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
(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?
(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.
Created attachment 353701 [details] [review] Add Todoist Support Revised patch with changes in the review.
Review of attachment 353701 [details] [review]: Looks good to me now
(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
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.
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.
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.
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.
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'?
Created attachment 353905 [details] [review] Introduce org.gnome.OnlineAccounts.Todo
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.
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.
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
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.
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.
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.
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.
I have updated my patches so now they don't cause any merge conflicts.
Created attachment 358006 [details] [review] todoist: Change scope to add 'project:delete'
We already added the Todoist provider some time ago; and since then decided to carry the Todoist integration in the applications themselves. Closing.