GNOME Bugzilla – Bug 589545
*_download_document(): let the client decide where to download the file
Last modified: 2009-07-27 22:50:51 UTC
Please describe the problem: The client can't choose the exact file where to download a document. It should be possible. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 139108 [details] [review] First buggy patch.
Comment on attachment 139108 [details] [review] First buggy patch. Here is the backtrace
+ Trace 216595
I can't review the patch with so much unrelated stuff in it. Please remove things like test_upload_metadata_get_entry (GDataService *service) from the patch and attach an updated version with just the download changes.
Created attachment 139118 [details] [review] Cleaned up patch.
While reviewing the patch, it struck me that there's no need for the replace_file_if_exists parameter. We should make the function overwrite whatever file already exists (if any), and have the calling code check for existence of the file beforehand. The calling code would've had to do this before anyway, so that it could present a "Replace file?" dialogue to the user, in order to set the replace_file_if_exists parameter. If the user specifies a directory as destination_file, we should still overwrite existing files; just make sure that the filename that gdata_documents_*_download_document() uses is well documented. For this reason, I'm not reviewing the main changes to _gdata_documents_entry_download_document() until you make those changes (or provide a good reason why you shouldn't). :-) > - * @destination_directory: the directory into which the file should be downloaded > - * @file_extension: the extension with which to save the downloaded file > + * @destination_file: the GFile into which the presentation file should be saved > + * @extension: the file extension (is used in case @destination_file is a folder) You haven't renamed @file_extension to @extension in the function declaration, so don't change it in the documentation. You're also missing a "#" before "GFile". > - /* TODO: async version */ > + /* TODO: async version */ Watch out for introducing extraneous whitespace. > - g_return_val_if_fail (G_IS_FILE (destination_directory), NULL); > - g_return_val_if_fail (file_extension != NULL, NULL); > + g_return_val_if_fail (G_IS_FILE (destination_file), NULL); Why remove the NULL check for file_extension? > + * If @destination_file is a directory, then the file will be downloaded in this directory with the #GDataEntry:title with > + * the apropriate extension as name. > + * > + * If there is an error getting the document, a %GDATA_SERVICE_ERROR_WITH_QUERY error will be returned. That second paragraph already exists in the documentation for that function, and again for gdata_documents_text_download_document().
Created attachment 139264 [details] [review] Patch with the last review conrections implementations.
Created attachment 139304 [details] [review] New patch with the gboolean replace_if_extists. Hi, Sorry I have thought about the replace_if_exist variable and according to this: http://library.gnome.org/devel/gio/unstable/GFile.html#g-file-query-exists, I should let this boolean. >We should make the function overwrite >whatever file already exists (if any), and have the calling code check for >existence of the file beforehand. The calling code would've had to do this >before anyway, so that it could present a "Replace file?" dialogue to the user, >in order to set the replace_file_if_exists parameter. If the user specifies a >directory as destination_file, we should still overwrite existing files; just >make sure that the filename that gdata_documents_*_download_document() uses is >well documented. Actually the calling code should do this after as an G_IO_ERROR_EXISTS error handling then. So I send the new patch now hopping that you agree with me ;).
Some comments about the patch: > - * @destination_directory: the directory into which the file should be downloaded > + * @destination_file: the #GFile into which the presentation file should be saved A bit too much copy-pasting going on here; "presentation" isn't correct. Fixed. > + document_title = gdata_entry_get_title (GDATA_ENTRY (self)); > + gchar *filename = g_strdup_printf ("%s.%s", document_title, file_extension); It's old-fashioned, I know, but no inline declarations (of "filename" in this case) please. > + g_object_unref (destination_file); > + destination_file = new_destination_file; You shouldn't be unreffing the input to the function under any circumstances. That completely breaks the assumed memory management scheme with the public API. Removed. commit 88159d6a2c37cf3edd1b80398a020968d6728c39 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Jul 27 23:42:34 2009 +0100 Bug 589545 – *_download_document(): let the client decide where to download Patch from Thibault Saunier <saunierthibault@gmail.com> to change the semantics of the *_download_document() functions. Closes: bgo#589545 gdata/gdata-private.h | 2 +- gdata/services/documents/gdata-documents-entry.c | 60 +++++++++++--------- .../documents/gdata-documents-presentation.c | 13 ++-- .../documents/gdata-documents-presentation.h | 2 +- .../documents/gdata-documents-spreadsheet.c | 16 +++-- .../documents/gdata-documents-spreadsheet.h | 2 +- gdata/services/documents/gdata-documents-text.c | 13 ++-- gdata/services/documents/gdata-documents-text.h | 2 +- gdata/tests/documents.c | 57 +++++++++++++++---- 9 files changed, 105 insertions(+), 62 deletions(-)