GNOME Bugzilla – Bug 587073
Add Google Documents service
Last modified: 2009-07-17 18:08:23 UTC
Add a Google Documents service. Thibault has got a branch to do just that: http://github.com/thiblahute/libgdata/tree/master This bug is about reviewing that branch.
Review of gdata-documents-entry.c: > GDataEntryClass *entry_class = GDATA_ENTRY_CLASS (klass); Unused variable. > GDataDocumentsEntry::last_viewed You should use hyphens (rather than underscores) in property names *and* documentation. > GDataDocumentsEntry:last_modified_by This can be a g_param_spec_object, pointing to a GDATA_TYPE_AUTHOR object. This is due to the changes I made recently, converting things like GDataAuthor to full GObjects. > GDataDocumentsEntry* > gdata_documents_entry_new(const gchar *id) Please use spaces before the asterisks in return types (e.g. use "GDataDocumentsEntry *" instead) and use spaces before the opening bracket of function definitions and function calls. > g_return_val_if_fail (GDATA_IS_DOCUMENTS_ENTRY (parsable), FALSE); > g_return_val_if_fail (doc != NULL, FALSE); > g_return_val_if_fail (node != NULL, FALSE); There's no need for this; the inputs to parse_xml are guaranteed to be correct. (I realise this is wrong in other parts of the code, but that doesn't mean it should be wrong here too.) > if (strcmp ( (char*) _writers_can_invite, (char*) "true") == 0) It's better to use xmlStrcmp with the strings returned by libxml2, since it behaves slightly better with xmlChars. Just do the same as you've done with the xmlStrcmp()s for determining which element you're parsing. > g_free (_writers_can_invite); You should use xmlFree to free memory allocated by libxml2, or crashes can happen. > document_id = g_strsplit ((gchar *)document_id, ":", 2)[1]; Use "(gchar*) document_id" instead for casts. This code could crash if document_id is, for example, NULL. It's also leaking the memory allocated by xmlNodeListGetString and some of the memory allocated by g_strsplit. > } else if (xmlStrcmp (node->name, (xmlChar*) "feedLink") == 0){ The parsing code for <feedLink>s could be better. Since I changed GDataLink to being a full GObject, you can use the following: GDataLink *link = GDATA_LINK (_gdata_parsable_new_from_xml_node (GDATA_TYPE_LINK, "feedLink", doc, node, NULL, error)); if (link == NULL) return FALSE; Then add the link to the entry as before. Also, you should put a space before the opening brace of the if-block (e.g. ") == 0) {"). > } else if (xmlStrcmp (node->name, (xmlChar*) "lastModifiedBy") == 0){ As with <feedLink>s, you should be able to parse the <lastModifiedBy> element more easily, treating it as an "author" kind. See how gdata-entry.c parses <author> elements. > g_object_unref (priv->last_modified_by); This should be done in the class' "dispose" function (which doesn't currently exist). This isn't strictly necessary, but is good style; the dispose function can be called multiple times to break reference loops between objects, which wouldn't be possible if objects just unreffed things in their finalize function (which is only ever called once). > static void > get_xml (GDataEntry *entry, GString *xml_string) This parameter list is outdated and is causing gcc to give me warnings. It should use GDataParsable *parsable instead of the entry. Same for get_namespaces. > title = g_markup_escape_text (gdata_entry_get_title (entry), -1); > g_string_append_printf (xml_string, "<title type='text'>%s</title>", title); > g_free (title); There's no need for this; that XML is outputted by GDataEntry. You should remove this code and the stuff below it which outputs <category> elements, and instead simply call the parent class' get_xml method at the top of the function: /* Chain up to the parent class */ GDATA_PARSABLE_CLASS (gdata_documents_entry_parent_class)->get_xml (parsable, xml_string); > /*TODO check it after writing get_xml*/ The get_namespaces function looks fine. :-) It should only list namespaces which are used in the get_xml function. > g_return_if_fail ( GDATA_IS_DOCUMENTS_ENTRY ( self )); > /*g_return_if_fail (edited == NULL); TODO*/ Don't put spaces after the opening bracket, or before the closing bracket, of a function call. The second g_return_if_fail would be a good idea here. > self->priv->path = gdata_link_get_title (((GDataLink*) element->data)); You should be g_strdup()ing the link title here, or you'll end up freeing memory which doesn't belong to you. See my comment at the end about this function as a whole. > self->priv->path = g_strconcat (self->priv->path, gdata_link_get_title (((GDataLink*) element->data))); You're missing the NULL sentinel at the end of the g_strconcat call. > * @path: a new document_id (or NULL?) "path" here should be "document_id", but I'm not really reviewing documentation at the moment. Just a note to say that "NULL" should be written as "%NULL" in gtk-doc comments so it gets formatted correctly when the documentation is rendered. > g_object_notify (G_OBJECT (self), "resource-id"); You need to decide whether it's going to be called "document-id" or "resource-id" and use one name for this property (along with its getters and setters) consistently. I'd suggest "document-id". > * @writers_can_invite: %TRUE if writers can invite other personns to write on the document or %false otherwise Use "%FALSE" instead of "false" or "%false". > //g_object_notify (G_OBJECT (self), "writers-can-invite"); Any reason why this is commented out? > void > gdata_documents_entry_set_last_modified_by (GDataDocumentsEntry *self, GDataAuthor *last_modified_by) I don't think this function should exist; the property isn't writeable, and it's not the sort of thing you should be able to set as a client. > GDataFeed * > gdata_documents_entry_get_access_rules (GDataDocumentsEntry *self) I don't think there's any need for this (or *_set_access_rules). Clients can quite easily query each entry's ACL as and when necessary. Unless there's something I'm not considering? > * @replace_file_if_exist: %TRUE if you want to replace the file if it exists, %FALSE otherwise. If it's set to %False > * a G_IO_ERROR_EXISTS will be set Move the explanation about the error to the body of the function's documentation. The documentation for each parameter is meant to be fairly brief. > * Downloads and returns the documents descibed here. If the documents doesn't exist the download document will be an HTML file containing the error explanation. Is there any way we could detect when the document doesn't exist, and return an error, rather than returning as if nothing's wrong? Perhaps by looking at the HTTP status or content type of the returned document? > * Return value: the document's data, or %NULL; free with g_free() GFiles should be unreffed with g_object_unref, rather than freed. > GFile * > gdata_documents_entry_download_document (GDataDocumentsEntry *self, GDataService *service, gchar **content_type, gchar *link, gchar *destination_folder,\ > gchar *file_extension, gboolean replace_file_if_exist, GCancellable *cancellable, GError **error) There's no need for the backslash in the middle of the parameter list. Couldn't the link be retrieved from the GDataDocumentsEntry inside the function, rather than requiring the client to work out the download link themselves? Otherwise, gchar *link should become const gchar *link. The destination file should be passed in as GFile (since GFiles can refer to paths which don't actually exist), so gchar *destination_folder, gchar *file_extension can be replaced by GFile *destination_file. > g_set_error_literal (error, GDATA_SERVICE_ERROR, GDATA_SERVICE_ERROR_AUTHENTICATION_REQUIRED, > _("You must be authenticated to query documents.")); Don't forget to document this in the function's documentation. > } > else These should be on the same line. > g_signal_connect (message, "got-chunk", (GCallback) _on_chunk_signal, file_stream); It would be better if _on_chunk_signal was called got_chunk_cb ("cb" stands for "callback") — this just makes the codebase more consistent. This function should be static, and should be placed just above gdata_documents_entry_download_document. Its declaration should be removed from gdata-documents-entry.h. > if (status == SOUP_STATUS_NONE) { > g_object_unref (message); > return NULL; > } Don't forget to unref/free things before returning. Here, you're leaking file_stream and destination_file. Finally, I don't understand the purpose of the GDataDocumentsEntry:path property and its getter/setter. Could you explain it please?
Review of gdata-documents-feed.c: > * Copyright (C) Thibault Saunier <saunierthibault@gmail.com You should include the year you worked on the file, and you missed off the closing angle bracket from your e-mail address. The same problems are present in gdata-documents-entry.c and gdata-documents-feed.h, amongst other files. > struct _GDataDocumentsFeedPrivate { > /**/; > }; This can be removed from the class if it's not going to be used. > GObjectClass *gobject_class = G_OBJECT_CLASS (klass); > GDataParsableClass *parsable_class = GDATA_PARSABLE_CLASS (klass); > GDataFeed *feed_class = GDATA_FEED_CLASS (klass); gobject_class and feed_class are unused. > /*Nothing to be here*/ It's the law that this comment should be witty. Please rewrite it. > GDataDocumentsFeed* > _gdata_documents_feed_new_from_xml (GType feed_type, const gchar *xml, gint length, GType entry_type,\ > GDataQueryProgressCallback progress_callback, gpointer progress_user_data, GError **error) This function is now unnecessary, since GDataFeed was redesigned to extend GDataParsable, and you've set GDataService->feed_type in gdata-documents-service.c. That means the _gdata_feed_parse_data_new stuff is also now unnecessary. :-) > data = _gdata_feed_parse_data_new(entry_type, progress_callback, progress_user_data); Missing a space before the opening bracket of the function call. You could also probably take out the newlines from between those three lines of code, since they're quite closely related. > g_return_val_if_fail (GDATA_IS_FEED (parsable), FALSE); > g_return_val_if_fail (doc != NULL, FALSE); > g_return_val_if_fail (node != NULL, FALSE); As with GDataDocumentsEntry's get_xml function, these checks are unnecessary. > GDataAuthor *author; Unused. > if (is_spreadsheet_entry (doc, node)) > { > entry = GDATA_DOCUMENTS_SPREADSHEET (_gdata_parsable_new_from_xml_node (GDATA_TYPE_DOCUMENTS_SPREADSHEET, "entry", doc, node, NULL, error)); > > } You don't need the curly brackets for this if-block. The parsable should be cast to GDataEntry, rather than GDataDocumentsSpreadsheet. Remove the space after the opening bracket in the if statement for text entries, below. > gchar *document_type; Unused. > xmlNodePtr entry_node; Use xmlNode *entry_node instead; I find it's easier to follow than xmlNodePtr. > g_return_val_if_fail (node != NULL, FALSE); There's no need for this; you can control all the input to this function, and node is guaranteed not to be NULL in parse_xml anyway. > if ( strcmp ((gchar*) entry_node->name, "category") == 0){ Use xmlStrcmp. Remove the space after the opening bracket of the if statement. Insert a space before the opening curly bracket of the if-block. > const gchar *label= (const gchar*) (xmlGetProp (entry_node, (xmlChar*) "label")); This should not be const; xmlGetProp returns allocated memory, which should be freed with xmlFree before the function returns or the loop next iterates. There should be a space before the equals sign. > if (strcmp (label, "spreadsheet") == 0){ > return TRUE; > } There's no need to make this if statement a block. Remove the curly brackets. All of the above problems apply equally to is_text_entry, is_presentation_entry and is_folder_entry. In gdata-documents-feed.h, the line with the FIXME can be removed completely.
Review of gdata-documents-folder.c: > struct _GDataDocumentsFolderPrivate > { > /*TODO*/ > }; Doesn't look like there will be any properties or private data for this class, so this can be removed. > GObjectClass *gobject_class = G_OBJECT_CLASS (klass); > GDataEntryClass *entry_class = GDATA_ENTRY_CLASS (klass); > GDataParsableClass *parsable_class = GDATA_PARSABLE_CLASS (klass); > GDataDocumentsEntryClass *documents_entry_class = GDATA_DOCUMENTS_ENTRY_CLASS (klass); > > gobject_class->finalize = gdata_documents_folder_finalize; All this can be removed except parsable_class. > static void > gdata_documents_folder (GDataDocumentsFolder *self) Can be removed. > GDataDocumentsFolder* > gdata_documents_folder_new(const gchar *id) Needs a space before the asterisk in the return type, and a space before the opening bracket for the parameter list. > static gboolean > parse_xml (GDataParsable *parsable, xmlDoc *doc, xmlNode *node, gpointer user_data, GError **error) This can be removed if there's nothing specific to GDataDocumentsFolder to parse. > static void > gdata_documents_folder_init (GDataDocumentsFolder *self) Needs a witty comment! > static void > gdata_documents_folder_finalize (GObject *object) Can be removed. > static void > get_xml (GDataEntry *entry, GString *xml_string) GDataEntry *entry should be GDataParsable *parsable to fit in with the recent conversion of GDataEntry to extend GDataParsable. > gchar *document_id = gdata_documents_entry_get_document_id (GDATA_DOCUMENTS_ENTRY (entry)); document_id should be declared at the top of the function, to be C90-compliant. It should also be const (as should the return type of gdata_documents_entry_get_document_id, which I missed before), and should not be freed at the end of get_xml. > /*#include <gdata/gdata-gdata.h>*/ This can be removed from gdata-documents-folder.h.
gdata-documents-headers.h should probably be removed. Users of the library will either include everything with #include <gdata/gdata.h>, or include files individually. Review of gdata-documents-presentation.c: Most of the comments about gdata-documents-folder.c apply here too (e.g. private struct can be removed, unused variables need removing and parse_xml can be removed). > GFile * > gdata_documents_presentation_download_document (GDataDocumentsEntry *self, GDataDocumentsService *service, gchar **content_type, > gchar *export_format, gchar *destination_folder, gboolean replace_file_if_exist, GCancellable *cancellable, GError **error) I see now why you had gdata_documents_entry_download_document take a link parameter. In that case, gdata_documents_entry_download_document should be made private (its declaration put in gdata-private.h), and should take GFile *destination_directory, along with the current const gchar *file_extension. Sorry for the confusion in my first comment. I hadn't seen this code at that point. :-) export_format should be an enum, which gdata_documents_presentation_download_document converts to a string-based file extension for gdata_documents_entry_download_document. > link_href = g_string_new ("http://docs.google.com/feeds/download/presentations/Export?exportFormat="); > g_string_append_printf (link_href, "%s&docID=%s", export_format, document_id); There's no need to use a GString here; just use g_strdup_printf and free with g_free.
Review of gdata-documents-spreadsheet.c: Most of the comments about gdata-documents-folder.c apply here too (e.g. private struct can be removed, unused variables need removing and parse_xml can be removed). > static gchar *get_extension_from_fmcmd (gchar *fmcmd); Should return a const gchar *. > static gchar *fmcmd_extension_map[12*sizeof(void*)][sizeof(gchar*)] = {{"4","xls"}, {"5", "cvs"}, {"12", "pdf"}, {"13", "ods"}, {"23", "tsv"}, {"102", "html"}}; It would be easier to read this if each of the first-level array elements were on a new line, and tab indented. I'd ideally define it as follows: typedef enum { GDATA_DOCUMENTS_SPREADSHEET_XLS = 4, GDATA_DOCUMENTS_SPREADSHEET_CVS = 5, ... } GDataDocumentsSpreadsheetFormat; typedef struct { GDataDocumentsSpreadsheetFormat format; const gchar *extension; } ExtensionMapping; static ExtensionMapping extension_map[] = { { GDATA_DOCUMENTS_SPREADSHEET_XLS, "xls" }, { GDATA_DOCUMENTS_SPREADSHEET_CVS, "cvs" }, ... }; The need for GDataDocumentsSpreadsheetFormat is something I'll cover below. It should be put in gdata-documents-spreadsheet.h, while the rest should be in gdata-documents-spreadsheet.c as it is currently. > enum { > PROP_KEY > }; Is there any plan for this to be used? As far as I can tell from the protocol documentation, it's just what the document ID is called for spreadsheets (in which case, there's no need to duplicate GDataDocumentsEntry:document-id in GDataDocumentsSpreadsheet). > GFile * > gdata_documents_spreadsheet_download_document (GDataDocumentsEntry *self, GDataDocumentsService *service, gchar **content_type, gchar *gid,\ > gchar *fmcmd, gchar *destination_folder, gboolean replace_file_if_exist, GCancellable *cancellable, GError **error) The gid parameter should be a gint, and should have a more descriptive name, such as "sheet_number". The fmcmd parameter should be a GDataDocumentsSpreadsheetFormat, since giving people who use the library a choice of download formats from an enum is more intuitive than getting them to input a string, then having to validate it and reject invalid input. It should also have a more descriptive name, such as "download_format". > g_string_append_printf (link_href, "%s&fmcmd=%s", document_id, fmcmd); According to the documentation (http://code.google.com/apis/documents/docs/2.0/reference.html#ExportParameters), you can just use the exportFormat parameter, as with text documents and presentations. Is that incorrect? If it is correct, then the extension map can be removed. > g_string_free (link_href, FALSE); The second argument here should be TRUE, since you want to free the string data as well. > static gchar * > get_extension_from_fmcmd (gchar *fmcmd) If this is still needed (see comment above about exportFormat download parameter), it should be moved above the download_document function so there's no need for a forward declaration.
Review of gdata-documents-text.c: All of the comments about gdata-documents-presentation.c apply here. The only other thing to comment upon is in gdata-documents-text.h: > GFile *gdata_documents_text_download_document (GDataDocumentsEntry *self, GDataDocumentsService *service, gchar **content_type,\ > gchar *export_format, gchar *destination_folder, gboolean replace_file_if_exist,\ > GCancellable *cancellable, GError **error); There's no need for the backslashes, and the first content on each new line should be lined up with the first "G" of the "GDataDocumentsEntry *self", rather than being arbitrarily indented. The function should be declared with the G_GNUC_WARN_UNUSED_RESULT attribute, since it returns allocated data. All the other *_download_document functions should be declared with this attribute too; something I missed before.
Review of gdata-documents-query.c: > g_object_class_install_property (gobject_class, PROP_EMAILS, > g_param_spec_string ("emails", > "Emails", "Specifies the emails of the persons collaborating on the document concerned the querry sperated by '%2C', or %NULL if it is unset.", > "NULL", > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); This should be removed and replaced by API for the "writer" and "reader" query parameters separately. It's OK if GDataDocumentsQuery doesn't implement support for the "reader" parameter at the moment. Furthermore, comma-separated string lists should not be taken in; a gdata_documents_query_add_writer function should exist to add an e-mail address to an internal GList or GPtrArray. This list/array would then be iterated through to build the real comma-separated list of e-mail addresses in get_query_uri. Related functions like gdata_documents_query_remove_writer and gdata_documents_query_get_writers would also have to be added. GDataDocumentsQuery:emails could then be removed (and needn't be replaced, since exposing lists as properties is not brilliant). Could you also leave a TODO comment in the file about adding support for any query parameters which are missing (such as "reader", "opened-min", "opened-max", etc.)? > g_string_append_printf (query_uri, "/folder\\%3A=%s", priv->folder_id); You probably want to use %%3A, rather than \\%3A. You should also consider what to do if (params_started == TRUE) when you append this string. > g_string_append_printf (query_uri, "?title=", priv->title); You shouldn't append the "?" here — APPEND_SEP does that for you. > if (priv->exact_title = TRUE){ Space needed before the opening curly bracket. > gboolean > gdata_documents_query_get_show_folder (GDataDocumentsQuery *self) Should be called gdata_documents_query_get_show_folders, to fit in with the property name. The setter's name should also be changed. > void > gdata_documents_query_set_exact_title (GDataDocumentsQuery *self, gboolean exact_title) It might be better to remove this function and just add a gboolean exact_title parameter to the gdata_documents_query_set_title function.
Review of gdata-documents-service.c: > * Copyright (C) Philip Withnall 2009 <philip@tecnocode.co.uk> I didn't realise I'd written this code. ;-) > * Fore more details about the spreadsheet downloads handling, see the <ulink type="http" url:"http://groups.google.com/group/Google-Docs-Data-APIs/browse_thread/thread/bfc50e94e303a29a?pli=1" Should be "url=" here, since you're writing DocBook markup. > g_object_class_install_property (gobject_class, PROP_SPREADSHEET_SERVICE, > g_param_spec_pointer ("spreadsheet-service", > "Spreadsheet service", "Another service for spreadsheets.", > G_PARAM_READWRITE)); This should be a g_param_spec_object, and should not be writeable. > if (priv->spreadsheet_service != NULL) > G_OBJECT_CLASS (gdata_documents_service_parent_class)->finalize (G_OBJECT (priv->spreadsheet_service)); You shouldn't need to do this. Just ensure you unref priv->spreadsheet_service in GDataDocumentsService's dispose function. > static void > gdata_documents_service_set_property (GObject *object, guint property_id, const GValue *value, GParamSpec *pspec) GDataDocumentsService shouldn't have any writeable properties, so this function can be removed. > GDataDocumentsFeed* > gdata_documents_service_query_documents (GDataDocumentsService *self, GDataDocumentsQuery *query, gboolean set_access_rules,\ > GCancellable *cancellable, GDataQueryProgressCallback progress_callback, > gpointer progress_user_data, GError **error) I don't see the need for set_access_rules. Surely the program using libgdata can call gdata_access_handler_get_rules as and when necessary on each entry in the returned feed? Again, there's no need for the backslash in the parameter list, and there should be a space before the asterisk in the return type. > GDataService *spreadsheet_service = g_object_new (GDATA_TYPE_SERVICE, "client-id", gdata_service_get_client_id (GDATA_SERVICE (service)), NULL); You should also copy across the proxy-uri property, and it would be a good idea to write a comment in notify_authenticated_cb explaining why all this is necessary. > #define BOUNDARY_STRING "END_OF_PART" It would be better to choose a boundary string which is less likely to appear in the file you're uploading. Some variation on the boundary string used in gdata-service.c would be fine. This is another place where witty comments are highly appropriate. > if ( folder != NULL){ > g_return_val_if_fail (GDATA_IS_DOCUMENTS_FOLDER (folder), NULL); The spacing's all wrong on the if statement, and that g_return_val_if_fail would be more appropriate at the top of the function: g_return_val_if_fail (folder == NULL || GDATA_IS_DOCUMENTS_FOLDER (folder), NULL); > g_return_val_if_fail ( folder_id != NULL, NULL); g_assert would probably be more appropriate here. > upload_uri = "http://docs.google.com/feeds/folders/private/full/folder\%3A"; > upload_uri = g_strconcat (upload_uri, folder_id, NULL); Using g_strdup_printf would be easier here. > upload_uri = "http://docs.google.com/feeds/documents/private/full"; You should g_strdup the constant string here, since the other assignment to upload_uri is of an allocated string. This means you can g_free (upload_uri) before returning from gdata_documents_service_upload_document without crashing. > if (klass->append_query_headers != NULL) > klass->append_query_headers ( GDATA_SERVICE (self), message); Should this not use the spreadsheet service when uploading spreadsheets? > /*Corrects a bug on spreadsheets mime types handling*/ Could you elaborate more on what the bug is, where it is, and provide a link (or something) to allow people to check if it's been fixed in the future, please? > second_chunk_header = g_strdup_printf ("\n--" BOUNDARY_STRING "\nContent-Type: %s\n\n", "application/x-vnd.oasis.opendocument.spreadsheet"); It would be cleaner to just set content_type to "application/x-vnd.oasis.opendocument.spreadsheet" in this case, and print content_type into second_chunk_header in all cases. This function as a whole is quite leaky, too. You should go through and ensure all the allocated memory is freed (with the notable exception of upload_data, which is inherited by the SoupMessage). Take extra care to ensure things are freed before the function returns with an error, for example. > void > gdata_documents_service_update_document (GDataDocumentsService *self, GDataDocumentsEntry *document, GFile *document_file,\ > gboolean metadata, GCancellable *cancellable, GError **error) Is there any way to merge this with gdata_documents_service_upload_document? They look very similar. > document = GDATA_DOCUMENTS_ENTRY (_gdata_entry_new_from_xml (G_OBJECT_TYPE (document), message->response_body->data, message->response_body->length, error)); That won't work, because you're just changing the value of the pointer you've been passed. Regardless, the function should return the updated document, and leave the one it was originally passed untouched. > void > gdata_documents_service_move_document_to_folder (GDataDocumentsService *self, GDataDocumentsEntry *document, GDataDocumentsEntry *folder, > GCancellable *cancellable, GError **error) This should return the updated document, instead of trying to modify the one passed to it. folder should be a GDataDocumentsFolder. > /* TODO check it*/ > upload_uri = (gdata_link_get_uri ((GDataLink*) gdata_entry_look_up_link (GDATA_ENTRY (folder), "self"))); The return value of gdata_entry_look_up_link should be checked before being passed to gdata_link_get_uri. > if (klass->append_query_headers != NULL) > klass->append_query_headers ( GDATA_SERVICE (self), message); Shouldn't this be spreadsheet_service when moving spreadsheets? > entry_xml = gdata_entry_get_xml (GDATA_ENTRY (document)); This needs freeing with g_free once you've set the request of the SoupMessage. > void > gdata_documents_service_remove_document_from_folder (GDataDocumentsService *self, GDataDocumentsEntry *document, GDataDocumentsEntry *folder, > GCancellable *cancellable, GError **error) This should also return the updated document, and folder should be a GDataDocumentsFolder. > upload_uri = gdata_link_get_uri (((GDataLink*) gdata_entry_look_up_link (GDATA_ENTRY (folder), "self"))); > upload_uri = g_strconcat (upload_uri, "/document\%3A%"); > upload_uri = g_strconcat (upload_uri, document_id); See the comment above about checking the link. You should probably use g_strdup_printf here for clarity, and replace \%3A with %%3A. > if (klass->append_query_headers != NULL) > klass->append_query_headers ( GDATA_SERVICE (self), message); Use spreadsheet_service here as well, when appropriate? > GDataService * > gdata_documents_service_get_spreadsheet_service(GDataDocumentsService *self) This should be made private, and its declaration moved to gdata-private.h.
That's everything new reviewed, apart from the test suite. I'll take a look at that, and anything which still isn't working, once these changes have been implemented.
A small response to your questions: "path" here should be "document_id", but I'm not really reviewing documentationat the moment. Just a note to say that "NULL" should be written as "%NULL" in gtk-doc comments so it gets formatted correctly when the documentation is rendered. -> Actually I think it's mor easy for clients to get easily the "path" of the document as a string. That's why I added this property even if it's not in the documentation. I asked you ones what you think about that, but I think you didn't received the message, and forgot to ask again. === > title = g_markup_escape_text (gdata_entry_get_title (entry), -1); > g_string_append_printf (xml_string, "<title type='text'>%s</title>", title); > g_free (title); There's no need for this; that XML is outputted by GDataEntry. You should remove this code and the stuff below it which outputs <category> elements, and instead simply call the parent class' get_xml method at the top of the function: /* Chain up to the parent class */ GDATA_PARSABLE_CLASS (gdata_documents_entry_parent_class)->get_xml (parsable, xml_string); ->It doesn't work for moving a document to a folder, I think there are to many informations and I can't get it working. What do you think I should do about that? Let as before? == Is there any way we could detect when the document doesn't exist, and return an error, rather than returning as if nothing's wrong? Perhaps by looking at the HTTP status or content type of the returned document? -> Actually the server does as if there where no problem. I spent some time thinking about that, but didn't figure out what to do to get something smarter. === Couldn't the link be retrieved from the GDataDocumentsEntry inside the function, rather than requiring the client to work out the download link themselves? Otherwise, gchar *link should become const gchar *link. ->mmm, I just understood that this is a private function, I moved it as needed. The functions to call are actually: gdata_documents_presentation_download_document... (You saw it afterward;) ) === The destination file should be passed in as GFile (since GFiles can refer to paths which don't actually exist), so gchar *destination_folder, gchar *file_extension can be replaced by GFile *destination_file. ->that's actually what I was trying to do, but no way to make it working. I actually haven't understood how works GFile and since I should have get the path from the GFile, and then create a new document with the new path. Do you think it's really import to do so? === > g_object_class_install_property (gobject_class, PROP_EMAILS, > g_param_spec_string ("emails", > "Emails", "Specifies the emails of the persons collaborating on the document concerned the querry sperated by '%2C', or %NULL if it is unset.", > "NULL", > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); This should be removed and replaced by API for the "writer" and "reader" query parameters separately. It's OK if GDataDocumentsQuery doesn't implement support for the "reader" parameter at the moment. Furthermore, comma-separated string lists should not be taken in; a gdata_documents_query_add_writer function should exist to add an e-mail address to an internal GList or GPtrArray. This list/array would then be iterated through to build the real comma-separated list of e-mail addresses in get_query_uri. Related functions like gdata_documents_query_remove_writer and gdata_documents_query_get_writers would also have to be added. GDataDocumentsQuery:emails could then be removed (and needn't be replaced, since exposing lists as properties is not brilliant). Could you also leave a TODO comment in the file about adding support for any query parameters which are missing (such as "reader", "opened-min", "opened-max", etc.)? -> I added reader and collaborators. I don't understand why you never add the properties with g_param_spec_pointer for GList, I added it, but could you please explain me a bit about that? I had a look on the documentation and I think there is no missing parameters. === > void > gdata_documents_query_set_exact_title (GDataDocumentsQuery *self, gboolean exact_title) It might be better to remove this function and just add a gboolean exact_title parameter to the gdata_documents_query_set_title function. ->How should I handle it here: case PROP_TITLE: gdata_documents_query_set_title (self, g_value_get_string (value)); break; === You should also copy across the proxy-uri property, and it would be a good idea to write a comment in notify_authenticated_cb explaining why all this is necessary. -> I added a notify_proxy_uri_cb to handle it. === Is there any way to merge gdata_documents_service_update_document with gdata_documents_service_upload_document? They look very similar. ->well, this is not possible since we can't know if we want to update or upload a document when the document entry is NULL. I am creating *_upload_document and *_update_document but both using the same _update_upload_document private (to file) function === > void > gdata_documents_service_remove_document_from_folder (GDataDocumentsService *self, GDataDocumentsEntry *document, GDataDocumentsEntry *folder, > GCancellable *cancellable, GError **error) This should also return the updated document, and folder should be a GDataDocumentsFolder. ->I know it should be a GDataDocumentsFolder, but I can't make it working, I include thde gdata-documents-folder.h file but anyway I get this erro message: In file included from ../../../gdata/services/documents/gdata-documents-entry.h:82, from gdata-documents-folder.h:27, from gdata-documents-folder.c:38: ../../../gdata/services/documents/gdata-documents-service.h:76: error: expected declaration specifiers or ‘...’ before ‘GDataDocumentsFolder’ ../../../gdata/services/documents/gdata-documents-service.h:80: error: expected declaration specifiers or ‘...’ before ‘GDataDocumentsFolder’ ../../../gdata/services/documents/gdata-documents-service.h:82: error: expected declaration specifiers or ‘...’ before ‘GDataDocumentsFolder’ I don't understand why, how can I fix it? === > if (klass->append_query_headers != NULL) > klass->append_query_headers ( GDATA_SERVICE (self), message); Shouldn't this be spreadsheet_service when moving spreadsheets? ->I don't know the documentation is very unclear about that. Actually it doesn't work using it! === > upload_uri = gdata_link_get_uri (((GDataLink*) gdata_entry_look_up_link (GDATA_ENTRY (folder), "self"))); > upload_uri = g_strconcat (upload_uri, "/document\%3A%"); > upload_uri = g_strconcat (upload_uri, document_id); See the comment above about checking the link. You should probably use g_strdup_printf here for clarity, and replace \%3A with %%3A. ->It doesn't work with %%3A, the message = soup_message_new (SOUP_METHOD_POST, uri); returns NULL. I updated the git repo.
(In reply to comment #10) > A small response to your questions: > > "path" here should be "document_id", but I'm not really reviewing > documentationat the moment. Just a note to say that "NULL" should be written as > "%NULL" in > gtk-doc comments so it gets formatted correctly when the documentation is > rendered. > > -> Actually I think it's mor easy for clients to get easily the "path" of the > document as a string. That's why I added this property even if it's not in the > documentation. I asked you ones what you think about that, but I think you > didn't received the message, and forgot to ask again. I was actually referring to the usage of "@path" instead of "@document_id" in the documentation for gdata_documents_entry_set_document_id. > > title = g_markup_escape_text (gdata_entry_get_title (entry), -1); > > g_string_append_printf (xml_string, "<title type='text'>%s</title>", title); > > g_free (title); > > There's no need for this; that XML is outputted by GDataEntry. You should > remove this code and the stuff below it which outputs <category> elements, and > instead simply call the parent class' get_xml method at the top of the > function: > > /* Chain up to the parent class */ > GDATA_PARSABLE_CLASS (gdata_documents_entry_parent_class)->get_xml (parsable, > xml_string); > > ->It doesn't work for moving a document to a folder, I think there are to many > informations and I can't get it working. What do you think I should do about > that? Let as before? I don't understand why it would work with your code, but not the code in GDataEntry. Compare the XML produced by the two pieces of code and see where it differs. > Is there any way we could detect when the document doesn't exist, and return an > error, rather than returning as if nothing's wrong? Perhaps by looking at the > HTTP status or content type of the returned document? > > -> Actually the server does as if there where no problem. I spent some time > thinking about that, but didn't figure out what to do to get something smarter. In the little bit of testing I've done with a modified version of gdata_documents_presentation_download_document and the /documents/download/download_all_documents test, the server seems to always return an error 500 status code when it can't find the specified ID. That's already caught by the check at gdata-documents-entry.c:704. > The destination file should be passed in as GFile (since GFiles can refer to > paths which don't actually exist), so gchar > *destination_folder, gchar > *file_extension can be replaced by GFile *destination_file. > > ->that's actually what I was trying to do, but no way to make it working. I > actually haven't understood how works GFile and since I should have get the > path from the GFile, and then create a new document with the new path. Do you > think it's really import to do so? Instead of the current code: document_title = gdata_entry_get_title (GDATA_ENTRY (self)); path = g_string_new (destination_folder); g_string_append_printf (path, "/%s.%s", document_title, file_extension); destination_file = g_file_new_for_path (path->str); g_string_free (path, FALSE); Use this: gchar *filename; document_title = gdata_entry_get_title (GDATA_ENTRY (self)); filename = g_strdup_printf ("%s.%s", document_title, file_extension); destination_file = g_file_get_child (destination_directory, filename); g_free (filename); You can then use destination_file as you are currently, then return it. > > g_object_class_install_property (gobject_class, PROP_EMAILS, > > g_param_spec_string ("emails", > > "Emails", "Specifies > the emails of the persons collaborating on the document concerned the querry > sperated by '%2C', or %NULL if it is unset.", > > "NULL", > > G_PARAM_READWRITE | > G_PARAM_STATIC_STRINGS)); > > This should be removed and replaced by API for the "writer" and "reader" query > parameters separately. It's OK if GDataDocumentsQuery doesn't implement support > for the "reader" parameter at the moment. > > Furthermore, comma-separated string lists should not be taken in; a > gdata_documents_query_add_writer function should exist to add an e-mail address > to an internal GList or GPtrArray. This list/array would then be iterated > through to build the real comma-separated list of e-mail addresses in > get_query_uri. > > Related functions like gdata_documents_query_remove_writer and > gdata_documents_query_get_writers would also have to be added. > GDataDocumentsQuery:emails could then be removed (and needn't be replaced, > since exposing lists as properties is not brilliant). > > Could you also leave a TODO comment in the file about adding support for any > query parameters which are missing (such as "reader", "opened-min", > "opened-max", etc.)? > > > -> I added reader and collaborators. I don't understand why you never add the > properties with g_param_spec_pointer for GList, > I added it, but could you please explain me a bit about that? > I had a look on the documentation and I think there is no missing parameters. I don't generally add properties for GLists, since I want the option to change the GList to something else in future (e.g. a GHashTable, though that wouldn't apply in this case). Exposing the GList as a property stops that from being done without silently breaking the API, which is bad. I can see "owner", "orderby", "opened-min" and "opened-max" properties in the documentation (http://code.google.com/apis/documents/docs/2.0/reference.html#Parameters) which I can't see in GDataDocumentsQuery. > > void > > gdata_documents_query_set_exact_title (GDataDocumentsQuery *self, gboolean exact_title) > > It might be better to remove this function and just add a gboolean > exact_title > parameter to the gdata_documents_query_set_title function. > > ->How should I handle it here: > case PROP_TITLE: > gdata_documents_query_set_title (self, > g_value_get_string (value)); > break; Just leave it with its current value. People who use the properties API, rather than the getters/setters, will just have to live with it. Perhaps you could make a note about it in the documentation for the :title property, though. > Is there any way to merge gdata_documents_service_update_document with > gdata_documents_service_upload_document? > They look very similar. > > ->well, this is not possible since we can't know if we want to update or upload > a document when the document entry is NULL. > I am creating *_upload_document and *_update_document but both using the same > _update_upload_document private (to file) function Putting the common code into a private function is what I meant. Sorry for not being clearer. You should rename the private function to something more manageable (and more obviously private) than gdata_documents_service_upload_update_document. Perhaps simply upload_update_document? Also, make sure it's static. > > void > > gdata_documents_service_remove_document_from_folder (GDataDocumentsService *self, GDataDocumentsEntry *document, GDataDocumentsEntry *folder, > > GCancellable *cancellable, GError **error) > > This should also return the updated document, and folder should be a > GDataDocumentsFolder. > > ->I know it should be a GDataDocumentsFolder, but I can't make it working, I > include thde gdata-documents-folder.h file but anyway I get this erro message: > In file included from > ../../../gdata/services/documents/gdata-documents-entry.h:82, > from gdata-documents-folder.h:27, > from gdata-documents-folder.c:38: > ../../../gdata/services/documents/gdata-documents-service.h:76: error: > expected declaration specifiers or ‘...’ before ‘GDataDocumentsFolder’ > ../../../gdata/services/documents/gdata-documents-service.h:80: error: > expected declaration specifiers or ‘...’ before ‘GDataDocumentsFolder’ > ../../../gdata/services/documents/gdata-documents-service.h:82: error: > expected declaration specifiers or ‘...’ before ‘GDataDocumentsFolder’ > > > I don't understand why, how can I fix it? The problem is given away by the "included from" information in the error. gdata-documents-folder.h includes gdata-documents-entry.h before it defines GDataDocumentsFolder. gdata-documents-entry.h goes on to include gdata-documents-service.h, which then can't find the definition of GDataDocumentsFolder, because it's been included before the symbol was defined. To fix it, it's probably best to remove the #include of gdata-documents-service.h from gdata-documents-entry.h, since it's not being used. As a side note, why do gdata_documents_entry_[set|get]_access_rules still exist? Please remove them or give a reason for them to be kept. :-) > > if (klass->append_query_headers != NULL) > > klass->append_query_headers ( GDATA_SERVICE (self), message); > > Shouldn't this be spreadsheet_service when moving spreadsheets? > > ->I don't know the documentation is very unclear about that. Actually it > doesn't work using it! Fair enough. > > upload_uri = gdata_link_get_uri (((GDataLink*) gdata_entry_look_up_link (GDATA_ENTRY (folder), "self"))); > > upload_uri = g_strconcat (upload_uri, "/document\%3A%"); > > upload_uri = g_strconcat (upload_uri, document_id); > > See the comment above about checking the link. You should probably use > g_strdup_printf here for clarity, and replace \%3A with %%3A. > > > ->It doesn't work with %%3A, the > message = soup_message_new (SOUP_METHOD_POST, uri); > returns NULL. How odd. I see you've taken a different approach anyway, which looks fine.
I rebased my work and I always get a "No <root/author> node" error if I dont comment: klass = GDATA_PARSABLE_GET_CLASS (parsable); if (klass->parse_xml == NULL) return FALSE; /*if (xmlStrcmp (node->name, (xmlChar*) klass->element_name) != 0 || (node->ns != NULL && xmlStrcmp (node->ns->prefix, (xmlChar*) klass->element_namespace) != 0)) { /*No <entry> element (required) xmlFreeDoc (doc); gdata_parser_error_required_element_missing (klass->element_name, "root", error); return NULL; }*/ I don't really understand your new code, but I think there is a mistake here. I updated the SVN, and now I get a "ERROR:documents.c:273:test_add_remove_file_from_folder: assertion failed (error == NULL): Error code 500 when inserting an entry: Internal Server Error (gdata-service-error-quark, 5) " on to move out documents. But actually it worked a few times and I can't get it working more. I check the documentation and the bug reports and nowhere I could see this bug. And since errorr 500 doesn't mean anything I think this error is comin from my code and not from the server.
(In reply to comment #12) > I rebased my work and I always get a "No <root/author> node" error if I dont > comment: > > klass = GDATA_PARSABLE_GET_CLASS (parsable); > if (klass->parse_xml == NULL) > return FALSE; > > /*if (xmlStrcmp (node->name, (xmlChar*) klass->element_name) != 0 || > (node->ns != NULL && xmlStrcmp (node->ns->prefix, (xmlChar*) > klass->element_namespace) != 0)) { > /*No <entry> element (required) > xmlFreeDoc (doc); > gdata_parser_error_required_element_missing (klass->element_name, "root", > error); > return NULL; > }*/ > > > I don't really understand your new code, but I think there is a mistake here. That code is there to check that gdata_parsable_new_from_xml is being called on the right XML node, and is necessary. Obviously, some parsing code somewhere is wrong. What test case does this fail on? > I updated the SVN, and now I get a > "ERROR:documents.c:273:test_add_remove_file_from_folder: assertion failed > (error == NULL): Error code 500 when inserting an entry: Internal Server Error > (gdata-service-error-quark, 5) > " on to move out documents. But actually it worked a few times and I can't get > it working more. I check the documentation and the bug reports and nowhere I > could see this bug. And since errorr 500 doesn't mean anything I think this > error is comin from my code and not from the server. If you look at the error response in Wireshark, you should find a more useful error message in the response body. It should explain why the server is rejecting your request.
I forgot to mention that before the branch can be merged, you need to change the test suite so that it doesn't reference files in your home directory. Those files need to be freely-licenced and added to git, then used with relative paths from the test cases.
I changed the suite and added the files to the git repo.
commit 4ee6c030ecdc75086d6317fd0e2e8fa8ae454903 Author: Thibault Saunier <saunierthibault@gmail.com> Date: Fri Jul 17 19:04:15 2009 +0100 Bug 587073 – Add Google Documents service Added support for Google Documents, from work done by Thibault Saunier <saunierthibault@gmail.com>. Closes bgo#587073 and bgo#588282. HACKING | 2 + configure.in | 3 +- docs/reference/gdata-docs.xml | 13 +- docs/reference/gdata-sections.txt | 188 +++++ gdata/Makefile.am | 1 + gdata/atom/gdata-link.c | 6 +- gdata/gdata-entry.c | 35 + gdata/gdata-entry.h | 1 + gdata/gdata-feed.c | 100 ++- gdata/gdata-parsable.c | 7 +- gdata/gdata-private.h | 13 + gdata/gdata.h | 10 + gdata/gdata.symbols | 48 ++ gdata/services/Makefile.am | 2 +- gdata/services/documents/gdata-documents-entry.c | 633 ++++++++++++++++ gdata/services/documents/gdata-documents-entry.h | 81 ++ gdata/services/documents/gdata-documents-feed.c | 119 +++ gdata/services/documents/gdata-documents-feed.h | 67 ++ gdata/services/documents/gdata-documents-folder.c | 88 +++ gdata/services/documents/gdata-documents-folder.h | 69 ++ .../documents/gdata-documents-presentation.c | 151 ++++ .../documents/gdata-documents-presentation.h | 96 +++ gdata/services/documents/gdata-documents-query.c | 563 ++++++++++++++ gdata/services/documents/gdata-documents-query.h | 86 +++ gdata/services/documents/gdata-documents-service.c | 767 ++++++++++++++++++++ gdata/services/documents/gdata-documents-service.h | 105 +++ .../documents/gdata-documents-spreadsheet.c | 172 +++++ .../documents/gdata-documents-spreadsheet.h | 99 +++ gdata/services/documents/gdata-documents-text.c | 154 ++++ gdata/services/documents/gdata-documents-text.h | 103 +++ gdata/services/youtube/gdata-youtube-video.c | 2 + gdata/tests/Makefile.am | 3 + gdata/tests/common.h | 1 + gdata/tests/documents.c | 588 +++++++++++++++ gdata/tests/picasaweb.c | 1 - po/fr.po | 4 + 37 files changed, 4418 insertions(+), 43 deletions(-)