GNOME Bugzilla – Bug 657539
Add support for Google Tasks
Last modified: 2014-09-21 16:42:57 UTC
The page for google task api is here: http://code.google.com/apis/tasks/v1/using.html A request for the api (in Evolution) is here: https://bugzilla.gnome.org/show_bug.cgi?id=652132
This is going to be fun, because the Google Tasks API is only available in JSON, and libgdata (currently) only handles XML-based APIs. That's no reason not to do it, however. Hopefully it can be done for 0.12.
(And also, the API hasn't graduated from Labs yet, so whatever we add in libgdata will have to be API-unstable and protected by a LIBGDATA_I_KNOW_UNSTABLE_APIS_KILL_KITTENS #define.)
The page for the Google task API http://code.google.com/apis/tasks/v1/using.html now redirects to https://developers.google.com/google-apps/tasks/firstapp , and I can't find the words "lab" or "beta" there, so I guess this wouldn't kill any kittens anymore?
(In reply to comment #3) > The page for the Google task API > http://code.google.com/apis/tasks/v1/using.html now redirects to > https://developers.google.com/google-apps/tasks/firstapp , and I can't find the > words "lab" or "beta" there, so I guess this wouldn't kill any kittens anymore? I can't see anything mentioning the API being stable now, but it definitely looks like it's graduated. I guess it should be safe for libgdata to implement without killing kittens now. :-) Patches welcome!
Phase 1 complete: adding support for JSON. Thanks to Pēteris! commit 9771af32dac2a78f67b2ff76f9cea35677e8b99f Author: Peteris Krisjanis <pecisk@gmail.com> Date: Mon Jul 8 18:29:17 2013 +0300 core: Add support for JSON to GDataParsable, GDataEntry and GDataFeed This also includes initial code for detection of the Content-Type of receive messages, and parsing JSON or XML depending on that. This breaks ABI (but not API), and adds a dependency on json-glib ≥ 0.15. Complete unit tests are included. Further work is expected for integrating JSON support into GDataService, ready for use with the Tasks service. This work is originally by Pēteris Krišjānis <pecisk@gmail.com>, with additions by Philip Withnall <philip@tecnocode.co.uk>. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657539 Makefile.am | 4 +- README | 1 + configure.ac | 3 +- docs/reference/gdata-sections.txt | 2 + gdata/GData-0.0.metadata | 3 +- gdata/gdata-entry.c | 98 +++++++++++++ gdata/gdata-feed.c | 77 ++++++++++ gdata/gdata-parsable.c | 292 ++++++++++++++++++++++++++++++++++++++ gdata/gdata-parsable.h | 15 +- gdata/gdata-parser.c | 243 +++++++++++++++++++++++++++++++ gdata/gdata-parser.h | 10 ++ gdata/gdata-private.h | 8 ++ gdata/gdata-service.c | 23 ++- gdata/gdata.symbols | 2 + gdata/tests/common.c | 161 +++++++++++++++++++++ gdata/tests/common.h | 11 +- gdata/tests/general.c | 173 +++++++++++++++++++++- 17 files changed, 1115 insertions(+), 11 deletions(-)
Created attachment 255579 [details] [review] Adding get_content_type virtual function to GDataParsable and use it in GDataService functions. core: Adding get_content_type virtual function to GDataParsable and use it in GDataService functions. Adding get_content_type virtual function to GDataParsable to allow insert/update/delete_entry functions in GDataService to distinguish between JSON and XML based GDataParsables. GDataParsable subclass who uses JSON format data define their virtual function and return const gchar* string application/json. Otherwise GDataParsable own defined virtual function will return application/atom+xml. For insert/update_entry it allows to choose between two different calls to build xml or json message, and also in return parse returned data from online service correctly. For delete_entry it allows to choose different link from links list, as for json it is stored in different place. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657539
Created attachment 255594 [details] [review] Initial commit of Google Tasks service support From 4f38afb68d89b8ae7840fc3be4c3655c53e74c93 Mon Sep 17 00:00:00 2001 From: Peteris Krisjanis <pecisk@gmail.com> Date: Mon, 23 Sep 2013 23:51:59 +0300 Subject: [PATCH] tasks: Initial commit of Google Tasks service support. Adds GDataTasks classes to libgdata - GDataTasksService, GDataTasksQuery, GDataTasksTasklist and GDataTasksTask. This is addition to API. Changed also Makefile.am and gdata/gdata.symbols to add public API calls. Closes: https://bugzilla.gnome.org/show_bug.cgi?id=657539 --- Makefile.am | 15 +- gdata/gdata.symbols | 20 + gdata/services/tasks/gdata-tasks-query.c | 622 ++++++++++++++++++++++ gdata/services/tasks/gdata-tasks-query.h | 81 +++ gdata/services/tasks/gdata-tasks-service.c | 787 ++++++++++++++++++++++++++++ gdata/services/tasks/gdata-tasks-service.h | 109 ++++ gdata/services/tasks/gdata-tasks-task.c | 645 +++++++++++++++++++++++ gdata/services/tasks/gdata-tasks-task.h | 83 +++ gdata/services/tasks/gdata-tasks-tasklist.c | 125 +++++ gdata/services/tasks/gdata-tasks-tasklist.h | 63 +++ 10 files changed, 2549 insertions(+), 1 deletion(-) create mode 100644 gdata/services/tasks/gdata-tasks-query.c create mode 100644 gdata/services/tasks/gdata-tasks-query.h create mode 100644 gdata/services/tasks/gdata-tasks-service.c create mode 100644 gdata/services/tasks/gdata-tasks-service.h create mode 100644 gdata/services/tasks/gdata-tasks-task.c create mode 100644 gdata/services/tasks/gdata-tasks-task.h create mode 100644 gdata/services/tasks/gdata-tasks-tasklist.c create mode 100644 gdata/services/tasks/gdata-tasks-tasklist.h
Created attachment 255930 [details] [review] Initial commit of Google Tasks service support Updated patch for initial commit, removed whitespaces and redundant functions I decided we actually don't need.
Review of attachment 255579 [details] [review]: This looks good, but doesn’t compile on its own due to some missing variable declarations. There are also a couple of tiny whitespace problems. Please produce an updated version of the patch, then it should be ready to commit. Thanks! ::: gdata/gdata-service.c @@ +1369,2 @@ /* Append the data */ + klass = GDATA_PARSABLE_GET_CLASS (entry); You haven’t defined the ‘klass’ variable. @@ +1563,2 @@ /* Append the data */ + klass = GDATA_PARSABLE_GET_CLASS (entry); ‘klass’ isn’t defined in this function either. @@ +1601,3 @@ + updated_entry = GDATA_ENTRY (gdata_parsable_new_from_json (G_OBJECT_TYPE (entry), message->response_body->data, message->response_body->length, + error)); + } else { The body of this if-block isn’t indented correctly. @@ +1604,2 @@ updated_entry = GDATA_ENTRY (gdata_parsable_new_from_xml (G_OBJECT_TYPE (entry), message->response_body->data, message->response_body->length, error)); And here. @@ +1757,2 @@ /* Get the edit URI. We have to fix it to always use HTTPS as YouTube videos appear to incorrectly return a HTTP URI as their edit URI. */ + klass = GDATA_PARSABLE_GET_CLASS (entry); ‘klass’ isn’t defined in this function either.
Review of attachment 255930 [details] [review]: Looks like this is missing most of the files!
Created attachment 255980 [details] [review] Initial commit of Google Tasks service support Updated second patch with all files :)
Created attachment 255986 [details] [review] core: Adding get_content_type virtual function to GDataParsable and use it in GDataService functions. Fixing missed variables and formatting for first patch.
Comment on attachment 255986 [details] [review] core: Adding get_content_type virtual function to GDataParsable and use it in GDataService functions. Committed with some minor changes to fix some variable aliasing, and to wrap the commit message at 78 characters. commit da3aa429b5fba38b57aec40ecb37ab340cce90a5 Author: Peteris Krisjanis <pecisk@gmail.com> Date: Sat Sep 28 16:53:49 2013 +0300 core: Add get_content_type virtual function to GDataParsable Adding get_content_type virtual function to GDataParsable to allow insert/update/delete_entry functions in GDataService to distinguish between JSON and XML based GDataParsables. GDataParsable subclass who uses JSON format data define their virtual function and return const gchar* string application/json. Otherwise GDataParsable own defined virtual function will return application/atom+xml. For insert/update_entry it allows to choose between two different calls to build xml or json message, and also in return parse returned data from online service correctly. For delete_entry it allows to choose different link from links list, as for json it is stored in different place. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657539 gdata/gdata-parsable.c | 7 +++++++ gdata/gdata-parsable.h | 3 +++ gdata/gdata-service.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------ 3 files changed, 75 insertions(+), 30 deletions(-)
Review of attachment 255980 [details] [review]: Looking good. A number of minor comments. I’m being nitpicky, but I want the code to be really clean before it gets committed. That makes it easier for people to read and maintain in the future. Two bigger comments: 1. Have you been building with --enable-introspection passed to ./configure? The introspection tools printed a warning when I tried compiling with your patch, which is something you should’ve caught and fixed — every warning printed by the introspection tools means a function isn’t introspectable, which is bad. 2. Similarly, have you been building with --enable-gtk-doc? gtk-doc printed the following warnings for me: /opt/gnome3/source/libgdata/gdata/services/tasks/gdata-tasks-task.c:31: warning: Section gdata-tasks-task is not defined in the gdata-sections.txt file. /opt/gnome3/source/libgdata/gdata/services/tasks/gdata-tasks-service.c:32: warning: Section gdata-tasks-service is not defined in the gdata-sections.txt file. /opt/gnome3/source/libgdata/gdata/services/tasks/gdata-tasks-tasklist.c:31: warning: Section gdata-tasks-tasklist is not defined in the gdata-sections.txt file. ./gdata-unused.txt:1: warning: 89 unused declarations.They should be added to gdata-sections.txt in the appropriate place. which show that none of the new documentation is being used because the new classes haven’t been added to the gdata-sections.txt file. That needs doing! :-) Apart from that, this is looking good. Thanks! ::: Makefile.am @@ -298,2 @@ $(gdata_youtube_headers) \ gdata/services/youtube/gdata-youtube-enums.h You’re missing a backslash on the end of the youtube-enums line, so the tasks files aren’t getting included in the gdata_sources variable at all, and hence aren’t getting built. ::: gdata/gdata.symbols @@ +981,3 @@ +gdata_tasks_service_update_task_async +gdata_tasks_service_update_tasklist +gdata_tasks_service_update_tasklist_async This is missing entries for the functions for GDataTasksQuery, GDataTasksTask and GDataTasksTasklist. ::: gdata/services/tasks/gdata-tasks-query.c @@ +2,3 @@ +/* + * GData Client + * Copyright (C) Philip Withnall 2009–2010 <philip@tecnocode.co.uk> You need to put yourself in the copyright header. I don’t have copyright on this because I didn’t write it! (Same for the other files.) @@ +88,3 @@ + "Max task completion date", "Upper bound for a task's completion date to filter by.", + -1, G_MAXINT64, -1, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); Indentation’s too great here (and in the properties below). @@ +142,3 @@ + "Show completed tasks?", "Indicated whatever completed tasks are returned in the result.", + FALSE, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); This property should be named ‘show-completed’ rather than ‘is-show-completed’, since the latter doesn’t make sense grammatically. (The idea of the ‘is-’ prefix is to clarify property names like ‘hidden’ or ‘deleted’, and isn’t a strict rule.) @@ +155,3 @@ + "Show deleted tasks?", "Indicated whatever deleted tasks are returned in the result.", + FALSE, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); Same here. @@ +160,3 @@ + * GDataTasksQuery:is-show-hidden: + * + * Flag indicating whether hidden tasks are returned in the result. Optional. The default is False. Please use ‘%FALSE’ instead of ‘False’, since gtk-doc formats the former nicely. Same in the other documentation comments in this file. @@ +263,3 @@ + #define APPEND_SEP g_string_append_c (query_uri, (*params_started == FALSE) ? '?' : '&'); *params_started = TRUE; + + if (gdata_query_get_max_results (GDATA_QUERY(self)) > 0) { Missing space before the ‘GDATA_QUERY’ cast. @@ +265,3 @@ + if (gdata_query_get_max_results (GDATA_QUERY(self)) > 0) { + APPEND_SEP + g_string_append_printf (query_uri, "maxResults=%u", gdata_query_get_max_results (GDATA_QUERY(self))); Missing space before the ‘GDATA_QUERY’ cast. @@ +273,3 @@ + APPEND_SEP + g_string_append (query_uri, "updatedMin="); + updated_min = gdata_parser_int64_to_iso8601 (gdata_query_get_updated_min (GDATA_QUERY(self))); Missing space before the ‘GDATA_QUERY’ cast. @@ +341,3 @@ + /* We don't chain up with parent class get_query_uri because it uses + * GData protocol parameters and they aren't compatible with newest API family + */ It would be a good idea to add “#undef APPEND_SEP” at the bottom of the function so we don’t leak the helper macro. (I realise the original GDataQuery code doesn’t do this, because it’s old and ugly, but we can do it here.) @@ +514,3 @@ + + self->priv->due_min = due_min; + g_object_notify (G_OBJECT (self), "d-min"); s/d-min/due-min/ ::: gdata/services/tasks/gdata-tasks-query.h @@ +41,3 @@ + * GDataTasksQuery: + * + * All the fields in the #GDataTasksQuery structure are private and should never be accessed directly. This should have a ‘Since: UNRELEASED’ line. (Same for GDataTasksQueryClass and the other object/class structs in other files.) @@ +62,3 @@ +GDataTasksQuery *gdata_tasks_query_new (const gchar *q) G_GNUC_WARN_UNUSED_RESULT G_GNUC_MALLOC; + +gint64 gdata_tasks_query_get_completed_max (GDataTasksQuery *self); This should be G_GNUC_PURE. @@ +64,3 @@ +gint64 gdata_tasks_query_get_completed_max (GDataTasksQuery *self); +void gdata_tasks_query_set_completed_max (GDataTasksQuery *self, gint64 completed_max); +gint64 gdata_tasks_query_get_completed_min (GDataTasksQuery *self); And this. @@ +66,3 @@ +gint64 gdata_tasks_query_get_completed_min (GDataTasksQuery *self); +void gdata_tasks_query_set_completed_min (GDataTasksQuery *self, gint64 completed_min); +gint64 gdata_tasks_query_get_due_max (GDataTasksQuery *self); And this. @@ +68,3 @@ +gint64 gdata_tasks_query_get_due_max (GDataTasksQuery *self); +void gdata_tasks_query_set_due_max (GDataTasksQuery *self, gint64 due_max); +gint64 gdata_tasks_query_get_due_min (GDataTasksQuery *self); And this. @@ +79,3 @@ +G_END_DECLS + +#endif /* !GDATA_CALENDAR_QUERY_H */ s/CALENDAR/TASKS/ ::: gdata/services/tasks/gdata-tasks-service.c @@ +30,3 @@ + * online documentation</ulink>. + * + */ This documentation comment should have a ‘Since: UNRELEASED’. @@ +35,3 @@ +#include <glib.h> +#include <glib/gi18n-lib.h> +#include <libsoup/soup.h> This #include is unnecessary. @@ +48,3 @@ + +static GList *get_authorization_domains (void); +// needs to find proper service name to use for tasks Is this still to do? @@ +50,3 @@ +// needs to find proper service name to use for tasks +_GDATA_DEFINE_AUTHORIZATION_DOMAIN (tasks, "tasks", "https://www.googleapis.com/auth/tasks") +G_DEFINE_TYPE_WITH_CODE (GDataTasksService, gdata_tasks_service, GDATA_TYPE_SERVICE, G_IMPLEMENT_INTERFACE (GDATA_TYPE_BATCHABLE, NULL)) Does the API support batch operations? It doesn’t look to me like it does; if so, it shouldn’t implement the GDataBatchable interface. @@ +131,3 @@ +GDataFeed * +gdata_tasks_service_query_all_tasklists (GDataTasksService *self, GDataQuery *query, GCancellable *cancellable, + GDataQueryProgressCallback progress_callback, gpointer progress_user_data, GError **error) Indentation problem here. @@ +149,3 @@ + } + + request_uri = g_strconcat (_gdata_service_get_scheme (), "://www.googleapis.com/tasks/v1/users/@me/lists", "?key=AIzaSyBu-tk69L2jm7MhgY9fIxLMB-IYKWQNsS8", NULL); Is this key required when using OAuth 2? I can’t find much about it in the documentation. Could it be causing the authentication problems? In any case, if it is staying in the code, the value of the key should be a #define to make it more maintainable. @@ +182,3 @@ + GDataQueryProgressCallback progress_callback, gpointer progress_user_data, + GDestroyNotify destroy_progress_user_data, + GAsyncReadyCallback callback, gpointer user_data) Indentation problems. @@ +203,3 @@ + } + + request_uri = g_strconcat (_gdata_service_get_scheme (), "://www.googleapis.com/tasks/v1/users/@me/lists", NULL); If the request in gdata_tasks_service_query_all_tasklists() has a ‘key’ parameter, why doesn’t this? @@ +229,3 @@ +GDataFeed * +gdata_tasks_service_query_tasks (GDataTasksService *self, GDataTasksTasklist *tasklist, GDataQuery *query, GCancellable *cancellable, + GDataQueryProgressCallback progress_callback, gpointer progress_user_data, GError **error) Indentation problem. @@ +254,3 @@ + progress_callback, progress_user_data, error); + g_free (request_uri); + return feed; It would make the code a little clearer to put a blank line before the ‘return’. (I know, I’m being really nitpicky. It makes the code more pleasant to read!) @@ +284,3 @@ + GDataQueryProgressCallback progress_callback, gpointer progress_user_data, + GDestroyNotify destroy_progress_user_data, + GAsyncReadyCallback callback, gpointer user_data) Indentation problems. @@ +359,3 @@ + * + * Inserts @task by uploading it to the online tasks service into tasklist @tasklist. @self and @task are both reffed when this function is called, so can safely be + * unreffed after this function returns. You should also mention that it’s safe to unref @tasklist after the function returns. @@ +371,3 @@ +void +gdata_tasks_service_insert_task_async (GDataTasksService *self, GDataTasksTask *task, GDataTasksTasklist *tasklist, GCancellable *cancellable, + GAsyncReadyCallback callback, gpointer user_data) Indentation problem. @@ +382,3 @@ + request_uri = g_strconcat (_gdata_service_get_scheme (), "://www.googleapis.com/tasks/v1/lists/", gdata_entry_get_id (GDATA_ENTRY (tasklist)), "/tasks", NULL); + gdata_service_insert_entry_async (GDATA_SERVICE (self), get_tasks_authorization_domain (), request_uri, GDATA_ENTRY (task), cancellable, + callback, user_data); Indentation problem. @@ +548,3 @@ + * unreffed after this function returns. + * + * @callback should call gdata_service_insert_entry_finish() to finish deleting tasklist and to check for possible s/gdata_service_insert_entry_finish()/gdata_service_delete_entry_finish()/ @@ +605,3 @@ + * unreffed after this function returns. + * + * @callback should call gdata_service_insert_entry_finish() to obtain a #GDataTasksTask representing the updated task and to check for possible s/gdata_service_insert_entry_finish()/gdata_service_update_entry_finish()/ @@ +636,3 @@ + * For more details, see gdata_service_update_entry(). + * + * Return value: %TRUE on success, %FALSE otherwise This ‘Return value’ is wrong and is missing (transfer full). Were you building with --enable-introspection? The introspection tools print a warning message about this during compilation. ::: gdata/services/tasks/gdata-tasks-service.h @@ +41,3 @@ + * GDataTasksService: + * + * All the fields in the #GDataTasksService structure are private and should never be accessed directly. Needs a ‘Since: UNRELEASED’ line (and in the comment below). @@ +68,3 @@ +void gdata_tasks_service_query_all_tasklists_async (GDataTasksService *self, GDataQuery *query, GCancellable *cancellable, + GDataQueryProgressCallback progress_callback, gpointer progress_user_data, + GDestroyNotify destroy_progress_user_data, GAsyncReadyCallback callback, gpointer user_data); Indentation problems on the wrapped lines (and on the lines below). @@ +86,3 @@ + GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); +gboolean gdata_tasks_service_delete_task (GDataTasksService *self, GDataTasksTask *task, + GCancellable *cancellable, GError **error) G_GNUC_MALLOC G_GNUC_WARN_UNUSED_RESULT; This shouldn’t be G_GNUC_MALLOC G_GNUC_WARN_UNUSED_RESULT because it’s returning a gboolean, not a newly-allocated object. @@ +90,3 @@ + GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); +gboolean gdata_tasks_service_delete_tasklist (GDataTasksService *self, GDataTasksTasklist *tasklist, + GCancellable *cancellable, GError **error) G_GNUC_MALLOC G_GNUC_WARN_UNUSED_RESULT; Same here. @@ +101,3 @@ +void gdata_tasks_service_update_tasklist_async (GDataTasksService *self, GDataTasksTasklist *tasklist, + GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); +#endif /* !GDATA_CALENDAR_SERVICE_H */ s/CALENDAR/TASKS/ Also needs a blank line before the ‘#endif’ to space things out nicely. :-) ::: gdata/services/tasks/gdata-tasks-task.c @@ +29,3 @@ + * online documentation</ulink>. + * + */ Needs a ‘Since: UNRELEASED’ line. @@ +34,3 @@ +#include <glib.h> +#include <glib/gi18n-lib.h> +#include <libxml/parser.h> This #include is unnecessary. @@ +127,3 @@ + * GDataTasksTask:notes: + * + * Notes describing the task Might want to add another sentence here clarifying that “This is where the description of what needs to be done in the task is stored.”. @@ +161,3 @@ + "Due date of the task", "Due date of the task.", + -1, G_MAXINT64, -1, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); Indentation problems. @@ +167,3 @@ + * + * Completion date of the task (as a RFC 3339 timestamp). + * This field is omitted if the task has not been completed. s/omitted/<code class="literal">-1</code>/ @@ +180,3 @@ + * GDataTasksTask:is-deleted: + * + * Flag indicating whether the task has been deleted. The default if False. s/default if False/default is %FALSE/ @@ +188,3 @@ + "Deleted?", "Indicated whatever task is deleted.", + FALSE, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); I’m not sure this property should be writeable. The reference documentation doesn’t say it’s a read-only property, but it looks like it should be. Have you tried making an update query after explicitly changing the value of this property on a GDataTasksTask instance? I expect it’ll be read-only, and only set by calling the ‘delete’ method. If it is read-only, the public setter for the property should be removed. @@ +193,3 @@ + * GDataTasksTask:is-hidden: + * + * Flag indicating whether completed tasks are returned in the result. Optional. The default is True. This documentation comment doesn’t match the property (or its default value) at all. Copy–paste error? @@ +210,3 @@ + self->priv->due = -1; + self->priv->completed = -1; + /* FIXME make sure we don't have to initialise anything else */ This FIXME needs removing. Doesn’t look like anything else needs to be initialised. All the other members are automatically initialised to NULL/FALSE/0. @@ +221,3 @@ + + return object; +} This constructor doesn’t do anything so can be removed. @@ +229,3 @@ + /* Chain up to the parent class */ + G_OBJECT_CLASS (gdata_tasks_task_parent_class)->dispose (object); +} This dispose method doesn’t do anything so can be removed. @@ +305,3 @@ + case PROP_PARENT: + case PROP_POSITION: + case PROP_HIDDEN: It would be useful to add a comment on a line between ‘case PROP_HIDDEN:’ and ‘default:’ saying: /* Read-only. */ @@ +326,3 @@ + gdata_parser_int64_time_from_json_member (reader, "completed", P_DEFAULT, &(self->priv->completed), &success, error) == TRUE || + gdata_parser_boolean_from_json_member (reader, "deleted", P_DEFAULT, &(self->priv->deleted), &success, error) == TRUE || + gdata_parser_boolean_from_json_member (reader, "hidden", P_DEFAULT, &(self->priv->hidden), &success, error) == TRUE) { Indentation problems. @@ +384,3 @@ + json_builder_set_member_name (builder, "deleted"); + json_builder_add_boolean_value (builder, FALSE); + } If GDataTasksTask:deleted is read-only, this needs to be removed. Otherwise, the whitespace needs fixing: it needs a space after ‘if’ and the ‘else’ needs to be on the same line as both the braces (‘} else {’). @@ +388,3 @@ + +static const gchar * +get_content_type (void) { The opening brace should be on a new line. @@ +394,3 @@ +/** + * gdata_tasks_task_new: + * @id: (allow-none): the event's ID, or %NULL s/event/task/ @@ +414,3 @@ + * Gets the #GDataTasksTask:parent property. + * + * Return value: the parent of the task, or %NULL This needs (allow-none). @@ +431,3 @@ + * Gets the #GDataTasksTask:position property. + * + * Return value: the position of the task, or %NULL This needs (allow-none). @@ +448,3 @@ + * Gets the #GDataTasksTask:notes property. + * + * Return value: notes of the task, or %NULL This needs (allow-none). @@ +476,3 @@ + + g_free (self->priv->notes); + self->priv->notes = g_strdup (notes); Technically, you should perform the g_strdup() before calling g_free() on the old value. This fixes the case where notes == self->priv->notes; as it stands, the code would free both the old and new value, then try to g_strdup() the freed value, which produces an undefined result. This is really unlikely to happen (how often do you call gdata_tasks_task_set_notes(foo, gdata_tasks_task_get_notes(foo))?), but the code should be correct anyway. :-) @@ +486,3 @@ + * Gets the #GDataTasksTask:status property. + * + * Return value: the status of the task, or %NULL This needs (allow-none). @@ +514,3 @@ + + g_free (self->priv->status); + self->priv->status = g_strdup (status); Same as above. @@ +578,3 @@ + * @completed: completion time of the task, or <code class="literal">-1</code> + * + * Sets the #GDataTasksTask:completed property of the #GDataTasksTask to the new completion time of the task, @due. s/due/completed/ @@ +600,3 @@ + * Gets the #GDataTasksTask:is-deleted property. + * + * Return value: %TRUE if event is deleted, %FALSE otherwise s/event/task/ @@ +634,3 @@ + * Gets the #GDataTasksTask:is-hidden property. + * + * Return value: %TRUE if event is hidden, %FALSE otherwise s/event/task/ ::: gdata/services/tasks/gdata-tasks-task.h @@ +41,3 @@ + * GDataTasksTask: + * + * All the fields in the #GDataTasksTask structure are private and should never be accessed directly. This needs a ‘Since: UNRELEASED’ (and the comment below). @@ +63,3 @@ + +const gchar *gdata_tasks_task_get_parent (GDataTasksTask *self) G_GNUC_PURE; +void gdata_tasks_task_set_parent (GDataTasksTask *self, const gchar *parent); gdata_tasks_task_set_parent() doesn’t exist. A few other functions listed here don’t exist either. @@ +70,3 @@ +const gchar *gdata_tasks_task_get_status (GDataTasksTask *self) G_GNUC_PURE; +void gdata_tasks_task_set_status (GDataTasksTask *self, const gchar *status); +gint64 gdata_tasks_task_get_due (GDataTasksTask *self); This should be G_GNUC_PURE. @@ +72,3 @@ +gint64 gdata_tasks_task_get_due (GDataTasksTask *self); +void gdata_tasks_task_set_due (GDataTasksTask *self, gint64 due); +gint64 gdata_tasks_task_get_completed (GDataTasksTask *self); This should be G_GNUC_PURE. ::: gdata/services/tasks/gdata-tasks-tasklist.c @@ +29,3 @@ + * online documentation</ulink>. + * + **/ This needs a ‘Since: UNRELEASED’. Also, use ‘*/’ rather than ‘**/’. :-) @@ +34,3 @@ +#include <glib.h> +#include <glib/gi18n-lib.h> +#include <libxml/parser.h> This #include is unnecessary. @@ +43,3 @@ +#include "gdata-parser.h" +#include "gdata-types.h" +#include "gdata-access-handler.h" This #include is unnecessary. Some of the ones above it may also be unnecessary. gdata-service.h, for example. @@ +82,3 @@ + + return object; +} This constructor is empty and hence can be removed. @@ +89,3 @@ + /* Chain up to the parent class */ + G_OBJECT_CLASS (gdata_tasks_tasklist_parent_class)->finalize (object); +} This finaliser is empty and hence can be removed. @@ +96,3 @@ + /* well, there's nothing specific to parse from tasklist entries, all goes upstream */ + return GDATA_PARSABLE_CLASS (gdata_tasks_tasklist_parent_class)->parse_json (parsable, reader, user_data, error); +} This parser is empty and hence can be removed. @@ +103,3 @@ + /* Chain up to the parent class */ + GDATA_PARSABLE_CLASS (gdata_tasks_tasklist_parent_class)->get_json (parsable, builder); +} This getter is empty and hence can be removed. @@ +105,3 @@ +} + +static const gchar * get_content_type (void) { The type and opening brace should be on separate lines from the function name and parameters. @@ +118,3 @@ + * + * Since: UNRELEASED + **/ Use ‘*/’ instead of ‘**/’. ::: gdata/services/tasks/gdata-tasks-tasklist.h @@ +39,3 @@ + * GDataTasksTasklist: + * + * All the fields in the #GDataTasksTasklist structure are private and should never be accessed directly. This should have a ‘Since: UNRELEASED’ (and the comment below).
Created attachment 257099 [details] [review] Initial commit of Google Tasks service support Cleanup and fixes following review. Several side notes for reference: * API key in gdata_tasks_service_query_all_taskslists wasn't required, removed; * is_deleted property for task object is writable too, verified by testing;
Created attachment 257100 [details] [review] Add helper function to GDataEntry to add workaround to Google ISO 8601/RFC 3339 bug. This is additional patch needed to fix 'updated' date/time correctly for putting them on Google Tasks API, because Google currently doesn't support ISO 8601/RFC 3339 times without miliseconds.
Review of attachment 257100 [details] [review]: ::: gdata/gdata-entry.c @@ +637,3 @@ } +static gchar* Missing space before the ‘*’. This whole function needs a ‘FIXME’ comment saying why it’s necessary and linking to a bug in Google’s bug tracker asking them to fix their support for ISO 8601. @@ +638,3 @@ +static gchar* +iso8601_to_google (gchar* datetime_string) { Please use ‘gchar *datetime_string’. @@ +639,3 @@ +static gchar* +iso8601_to_google (gchar* datetime_string) { + return g_strjoinv (".000001+00:00", g_strsplit ((const gchar*) datetime_string, "Z", -1)); g_strsplit() returns a newly-allocated array, so you need to free it with g_strfreev() to avoid a memory leak.
Review of attachment 257099 [details] [review]: Looking a lot better. This is only a quick review; I still need to get a chance to compile and run it. ::: docs/reference/gdata-docs.xml @@ +184,3 @@ + <xi:include href="xml/gdata-tasks-tasklist.xml"/> + <xi:include href="xml/gdata-tasks-task.xml"/> + </chapter> This chunk needs to be before the closing ‘</part>’ above. ::: gdata/services/tasks/gdata-tasks-query.c @@ +539,3 @@ + * Gets the #GDataTasksQuery:is-show-completed property. + * + * Return value: the is-show-completed property These property names are outdated in the documentation. @@ +555,3 @@ + * @show_completed: %TRUE to show completed tasks, %FALSE otherwise + * + * Sets the #GDataTasksQuery:is-show-completed property of the #GDataTasksQuery. This property name is outdated in the documentation. ::: gdata/services/tasks/gdata-tasks-query.h @@ +79,3 @@ +void gdata_tasks_query_set_is_show_deleted (GDataTasksQuery *self, gboolean show_deleted); +gboolean gdata_tasks_query_is_show_hidden (GDataTasksQuery *self) G_GNUC_PURE; +void gdata_tasks_query_set_is_show_hidden (GDataTasksQuery *self, gboolean show_hidden); As well as renaming the properties, you need to rename the getters and setters: gdata_tasks_query_is_show_completed() → gdata_tasks_query_show_completed(), etc. ::: gdata/services/tasks/gdata-tasks-service.c @@ +358,3 @@ + * + * Inserts @task by uploading it to the online tasks service into tasklist @tasklist. @self and @task are both reffed when this function is called, so can safely be + * unreffed after this function returns. Still need to mention it’s safe to unref @tasklist after the function returns. ::: gdata/services/tasks/gdata-tasks-service.h @@ +90,3 @@ + GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); +gboolean gdata_tasks_service_delete_task (GDataTasksService *self, GDataTasksTask *task, + GCancellable *cancellable, GError **error) G_GNUC_PURE; This shouldn’t be G_GNUC_PURE either, as it can give a different return value each time it’s executed. G_GNUC_PURE should only be used for getters. @@ +94,3 @@ + GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); +gboolean gdata_tasks_service_delete_tasklist (GDataTasksService *self, GDataTasksTasklist *tasklist, + GCancellable *cancellable, GError **error) G_GNUC_PURE; This too. ::: gdata/services/tasks/gdata-tasks-task.c @@ +177,3 @@ + * GDataTasksTask:is-deleted: + * + * Flag indicating whether the task has been deleted. The default if %FALSE. ‘default if %FALSE’ still needs changing to ‘default is %FALSE’. ::: gdata/services/tasks/gdata-tasks-tasklist.c @@ +70,3 @@ + * Creates a new #GDataTasksTasklist with the given ID and default properties. + * + * Return value: a new #GDataTasksTasklist; unref with g_object_unref() This needs ‘(transfer full)’.
Review of attachment 257100 [details] [review]: This also makes test /entry/parse_json in general.c fail. You need to make the following change: diff --git a/gdata/tests/general.c b/gdata/tests/general.c index 6d8ec5b..8a19563 100644 --- a/gdata/tests/general.c +++ b/gdata/tests/general.c @@ -576,7 +576,7 @@ test_entry_parse_json (void) "{" "\"title\":\"A title\"," "\"id\":\"some-id\"," - "\"updated\":\"2009-01-25T14:07:37Z\"," + "\"updated\":\"2009-01-25T14:07:37.000001+00:00\"," "\"etag\":\"some-etag\"," "\"selfLink\":\"http://example.com/\"," "\"kind\":\"kind#kind\","
OK, having compiled it, I think that once the points from comment #17, comment #18 and comment #19 have been addressed (i.e. one more patch iteration!) I will go ahead and merge it. :-) Then we need to write some tests.
I just tidied up a few things and committed the two patches. Very little needed to be done to the Tasks service, but I ended up reworking the ISO 8601/RFC 3339 patch somewhat. Take a look and see what you think. :-) Now we just need to write some test cases for the new service. commit 76ea0e7dc549e4abfbab6caca87fa834245a3b88 Author: Peteris Krisjanis <pecisk@gmail.com> Date: Sat Oct 12 16:17:17 2013 +0300 core: Add helper function to work around Google RFC 3339 incompatibility In Google Tasks API it isn't supported to provide date/time in RFC 3339/ISO 8601 format without providing time in miliseconds. As gdata_parser_int64_to_iso8601() returns a properly formatted string, helper function replaces last character 'Z' with '.000001+00:00' to fit Google’s supported format. https://bugzilla.gnome.org/show_bug.cgi?id=657539 gdata/gdata-entry.c | 2 +- gdata/gdata-parser.c | 17 +++++++++++++++++ gdata/gdata-parser.h | 1 + gdata/services/tasks/gdata-tasks-task.c | 9 ++------- gdata/tests/general.c | 2 +- 5 files changed, 22 insertions(+), 9 deletions(-) commit 1e32273ff60b20da66a5334b911b2346d2af017a Author: Peteris Krisjanis <pecisk@gmail.com> Date: Sat Oct 12 15:49:13 2013 +0300 tasks: Add Google Tasks service support Adds GDataTasks classes to libgdata: • GDataTasksService • GDataTasksQuery • GDataTasksTasklist • GDataTasksTask This includes full documentation, but no test cases. No new dependencies have been added to libgdata, as all the necessary ones were added with the core JSON work. https://bugzilla.gnome.org/show_bug.cgi?id=657539 Makefile.am | 18 ++- docs/reference/gdata-docs.xml | 8 ++ docs/reference/gdata-sections.txt | 114 ++++++++++++++++++ gdata/gdata.h | 6 + gdata/gdata.symbols | 51 ++++++++ gdata/services/tasks/gdata-tasks-query.c | 638 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdata/services/tasks/gdata-tasks-query.h | 85 ++++++++++++++ gdata/services/tasks/gdata-tasks-service.c | 681 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdata/services/tasks/gdata-tasks-service.h | 108 +++++++++++++++++ gdata/services/tasks/gdata-tasks-task.c | 635 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdata/services/tasks/gdata-tasks-task.h | 84 +++++++++++++ gdata/services/tasks/gdata-tasks-tasklist.c | 80 +++++++++++++ gdata/services/tasks/gdata-tasks-tasklist.h | 68 +++++++++++ po/POTFILES.in | 1 + 14 files changed, 2575 insertions(+), 2 deletions(-)
(In reply to comment #21) > Now we just need to write some test cases for the new service. The eds backend is done now, kind of "real-life test case". Maybe you can close this, unless you want some more automated tests.
(In reply to comment #22) > (In reply to comment #21) > > Now we just need to write some test cases for the new service. > > The eds backend is done now, kind of "real-life test case". Maybe you can close > this, unless you want some more automated tests. Nice! I would like some automated tests for the Tasks service in libgdata, so I’ll leave this open. Goodness knows when I’m going to get time to implement them.
Implemented a few. Still need to add networked tests. commit 65745626ffc893a06d2d27c562effed4b683002b Author: Philip Withnall <philip@tecnocode.co.uk> Date: Sun Aug 31 00:59:12 2014 +0100 tests: Add unit tests for the Google Tasks service These are non-interactive and non-network tests, since I don’t currently have time to get networked tests working for Google Tasks — they require OAuth 2 support to land first (bug #646285). By themselves, these non-network tests give 64% line coverage and 74% function coverage of gdata/services/tasks/. gdata/tests/Makefile.am | 4 + gdata/tests/tasks.c | 712 +++++++++++++++++++++++++ gdata/tests/traces/tasks/authentication | 58 ++ gdata/tests/traces/tasks/global-authentication | 58 ++ gdata/tests/traces/tasks/tasklist-insert | 43 ++ 5 files changed, 875 insertions(+)
Fixed! commit 5d6f40293c1127a3879d32bd7eef4b518cb797e7 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Wed Sep 17 23:52:48 2014 +0100 tests: Add networked tests for the Google Tasks service This brings test coverage of the gdata/services/tasks/ directory up to 84% of lines and 90% of functions, which is good enough. https://bugzilla.gnome.org/show_bug.cgi?id=657539 gdata/tests/Makefile.am | 23 +++ gdata/tests/tasks.c | 748 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- gdata/tests/traces/tasks/authentication | 67 +++---- gdata/tests/traces/tasks/global-authentication | 67 +++---- gdata/tests/traces/tasks/setup-delete-task | 80 +++++++++ gdata/tests/traces/tasks/setup-delete-tasklist | 39 +++++ gdata/tests/traces/tasks/setup-insert-task | 39 +++++ gdata/tests/traces/tasks/setup-list-task | 162 +++++++++++++++++ gdata/tests/traces/tasks/setup-list-tasklist | 117 +++++++++++++ gdata/tests/traces/tasks/setup-update-task | 80 +++++++++ gdata/tests/traces/tasks/setup-update-tasklist | 39 +++++ gdata/tests/traces/tasks/task-delete | 23 +++ gdata/tests/traces/tasks/task-insert | 41 +++++ gdata/tests/traces/tasks/task-list | 63 +++++++ gdata/tests/traces/tasks/task-update | 42 +++++ gdata/tests/traces/tasks/tasklist-delete | 23 +++ gdata/tests/traces/tasks/tasklist-insert | 42 ++--- gdata/tests/traces/tasks/tasklist-insert-unauthorised | 44 +++++ gdata/tests/traces/tasks/tasklist-list | 61 +++++++ gdata/tests/traces/tasks/tasklist-update | 40 +++++ gdata/tests/traces/tasks/teardown-delete-task | 23 +++ gdata/tests/traces/tasks/teardown-insert-task | 46 +++++ gdata/tests/traces/tasks/teardown-insert-tasklist | 23 +++ gdata/tests/traces/tasks/teardown-list-task | 92 ++++++++++ gdata/tests/traces/tasks/teardown-list-tasklist | 69 ++++++++ gdata/tests/traces/tasks/teardown-update-task | 46 +++++ gdata/tests/traces/tasks/teardown-update-tasklist | 23 +++ 27 files changed, 2035 insertions(+), 127 deletions(-)