GNOME Bugzilla – Bug 598748
*_download_document prevents non-overwriting saving into directories
Last modified: 2010-01-19 15:23:23 UTC
Logic in gdata_documents_entry_download_document() appears to be like this | if (dest exists) | if (replace_file_if_exists == TRUE) | if (dest is directory) | output = new child destination in directory | else | output = dest | else | error out | else | output = dest This way, you won't be able to save into a directory if you don't set replace_file_if_exists. However, client code might reasonably want to not overwrite files in the directory, but still just pass a directory. But, if the target (i.e. the directory) exists and we don't set that flag, we error out :( PicasaWeb has the following logic, first determine what the true destination will be, and then using an appropriate API to handle overwriting in case of existence. | if (target_dest exists) | if (target_dest is a directory) | output = new child destination | if (output not set) | output = target_dest | | if (replace_file_if_exists) | g_file_replace (output) | else | g_file_create (output) (Note: I would have like to do something like this | if (target_dest exists and target_dest is directory) | output = new child destination | else | output = target_dest | g_file_create (output, replace_file_if_exists ? | G_FILE_CREATE_REPLACE_DESTINATION : G_FILE_CREATE_NONE, ...) But apparently, G_FILE_CREATE_REPLACE_DESTINATION doesn't do what I thought it did (it seems to be ignored in g_file_create () (whose docs say it will never overwrite) and in g_file_replace () (which seems to replace regardless)), and I cannot test the directory without first getting the GFileInfo which I can't do if it doesn't exist first. Sigh! Perhaps the code in gdata_documents_entry_download_common () and the bulk of gdata_picasaweb_file_download_content () are similar enough that they should be merged somewhere? I just realised that I have to refactor the Picasa one to also handle the thumbnails!
(In reply to comment #0) *snip* > But apparently, G_FILE_CREATE_REPLACE_DESTINATION doesn't do what I thought it > did (it seems to be ignored in g_file_create () (whose docs say it will never > overwrite) and in g_file_replace () (which seems to replace regardless)), and I > cannot test the directory without first getting the GFileInfo which I can't do > if it doesn't exist first. Sigh! Good catch. Patch (with test cases!) welcome. :-) > Perhaps the code in gdata_documents_entry_download_common () and the bulk of > gdata_picasaweb_file_download_content () are similar enough that they should be > merged somewhere? I just realised that I have to refactor the Picasa one to > also handle the thumbnails! If you can see a decent way to do that when you write gdata_picasaweb_file_download_content(), feel free. I would suggest that a private method of GDataDownloadStream is probably the best place to put such logic. Perhaps _gdata_download_stream_find_destination()?
Indeed. I do have a patch for this languishing. I will clean it double-check it tomorrow and attach.
Created attachment 151221 [details] [review] Patch altering _gdata_documents_entry_download_document This makes _gdata_documents_entry_download_document () behave a bit better, and actually, almost exactly like gdata_media_content_download (). Some of the variables have been changed as well to be more consistent between the two, in case we want to merge them. Not quite the "tomorrow" I had typed before. Need to be better able at anticipating availability :|
Review of attachment 151221 [details] [review]: Assuming the documents test suite still passes, please go ahead and commit. Thanks.
Patch pushed. The test suite still passes, and the test documents downloaded to /tmp/ seem alright. commit 06ca3389099f3dee7b68a49ca5d18d39208d32fe Author: Richard Schwarting <rschwart@src.gnome.org> Date: Tue Jan 12 12:44:58 2010 -0500 Bug 598748 — *_download_document prevents non-overwriting saving into directories Make _gdata_documents_entry_download_document rely on _gdata_download_stream_find_destination to allow saving into directories without the overwrite flag. Closes: bgo#598748
Seems to be much more logical, thanks for the patch Richard!