GNOME Bugzilla – Bug 762318
Don't overwrite exported photos
Last modified: 2016-03-11 16:56:14 UTC
Exporting the same photo will always overwrite the previously exported version. Instead, we should save them separately (e.g, adding a suffix).
Created attachment 321667 [details] [review] Allow multiple exports of same file
Review of attachment 321667 [details] [review]: ::: src/photos-utils.c @@ +979,3 @@ + + file = g_file_get_child (dir, unique_name); + if (g_file_query_exists (file, NULL)) g_file_query_exists does blocking I/O, so we would have to use an async version. Probably using g_file_query_info_async directly. The bigger problem is that this is racy. File I/O is generally racy because multiple processes can create files at the same time. I would suggest trying to create the file using g_file_create_async without doing any checks. If it fails with G_IO_ERROR_EXISTS, then increment the counter and try again, and again, etc.. I can imagine that creating an async convenience wrapper around g_file_create_async can be helpful. (The g_file_query_exists documentation has some text on the race issue too.)
Created attachment 321850 [details] [review] Allow multiple exports of same file Addressing raised points.
Review of attachment 321850 [details] [review]: Thanks, this looks quite good. Some comments: ::: src/photos-base-item.c @@ +1219,3 @@ if (error != NULL) { + if (error->code != G_IO_ERROR_EXISTS) We should use: g_error_matches (error, G_IO_ERROR, G_IO_ERROR_EXISTS) ... instead of a direct equality check because a different error code from a different domain (ie. a different library/subsystem) can have the same integer value as G_IO_ERROR_EXISTS. @@ +1228,3 @@ + unique_file = g_file_get_child (data->dir, data->name); + /* Try again with a new name */ + photos_base_item_create_file_async (unique_file, g_object_ref (task)); I wonder if we should restrict the number of attempts to reasonably high number like G_MAXUINT? Otherwise, a malicious program can get us into an infinite loop by filling the HDD with foo.png, foo(1).png ... foo(n).png, our counter will overflow and what not. Some might even interpret that as being vulnerable to a DoS attack. Also see below. ::: src/photos-utils.c @@ +926,3 @@ +photos_utils_filename_get_unique_name (GFile *file) +{ + gint count = 0; guint, maybe? :) @@ +957,3 @@ + g_free (basename); + + return unique_name; While this code is correct, I have a feeling that it can be simplified a bit. When photos_base_item_create_file_async is called for the first time, we can split basename into name-without-extension, the extension and a counter set to 0. These three can be put into a temporary struct to be carried around. Then whenever we get G_IO_ERROR_EXISTS, we increment the counter and use g_strdup_printf to assemble the parts and try again. This can largely simplify the string parsing logic.
Created attachment 321953 [details] [review] Allow multiple exports of same file New version.
Review of attachment 321953 [details] [review]: ::: src/photos-base-item.c @@ +1316,3 @@ + g_object_ref (task)); + + g_object_unref (task); I was hoping to have clearly separated wrapper around g_file_create_async that can be easily copy pasted elsewhere (possibly in gnome-photos itself or in other modules) and be a drop-in replacement for g_file_create_async. For example: void photos_utils_file_create_async (GFile *file, GFileCreateFlags flags, gint io_priority, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); GFileOutputStream *photos_utils_file_create_finish (GFile *file, GAsyncResult *res, GError **error);
Created attachment 322853 [details] [review] Allow multiple exports of same file New version with suggested fixes.
Review of attachment 322853 [details] [review]: Thanks for splitting the repeated file creation code into a standalone method. Some other comments: ::: src/photos-base-item.c @@ +1180,3 @@ GCancellable *cancellable; GError *error = NULL; + GFile *file; It should continue to be NULL initialized. Otherwise, if gdk_pixbuf_save_to_stream_finish failed, then it will have some garbage value while finishing the function. ::: src/photos-utils.c @@ +992,3 @@ + free_resources: + g_free (filename); + g_clear_object (&unique_file); We are leaking stream. We need a: g_clear_object (&stream); @@ +1394,3 @@ + data->basename = photos_utils_filename_strip_extension (filename); + data->ext = g_strdup (photos_utils_filename_get_extension_offset (filename)); + data->count = photos_utils_filename_get_copy_number (data->basename); Can't we just start from 0? We don't show the contents of the export folder in the application. Therefore, we are never going to show or further export any of the copies that we are creating. Secondly, if we have an original that is "foo (1).jpg", it will export it as "foo (1)(2).jpg" because the initial parsing will set count to 1. Instead, we should export it as "foo (1).jpg" and further copies as "foo (1)(1).jpg" and so on. Or am I missing something?
(In reply to Debarshi Ray from comment #9) > Review of attachment 322853 [details] [review] [review]: > > Thanks for splitting the repeated file creation code into a standalone > method. Some other comments: > > ::: src/photos-base-item.c > @@ +1180,3 @@ > GCancellable *cancellable; > GError *error = NULL; > + GFile *file; > > It should continue to be NULL initialized. Otherwise, if > gdk_pixbuf_save_to_stream_finish failed, then it will have some garbage > value while finishing the function. True, I haven't thought of that. > ::: src/photos-utils.c > @@ +992,3 @@ > + free_resources: > + g_free (filename); > + g_clear_object (&unique_file); > > We are leaking stream. We need a: > g_clear_object (&stream); We are not. Note that I pass g_object_unref to return_pointer and the ref'ed stream there is unref'ed in base_item_create_file_async_finish. I could not pass the object_unref and do it directly in the function if you think it's more clear that way. > @@ +1394,3 @@ > + data->basename = photos_utils_filename_strip_extension (filename); > + data->ext = g_strdup (photos_utils_filename_get_extension_offset > (filename)); > + data->count = photos_utils_filename_get_copy_number (data->basename); > > Can't we just start from 0? > > We don't show the contents of the export folder in the application. > Therefore, we are never going to show or further export any of the copies > that we are creating. > > Secondly, if we have an original that is "foo (1).jpg", it will export it as > "foo (1)(2).jpg" because the initial parsing will set count to 1. Instead, > we should export it as "foo (1).jpg" and further copies as "foo (1)(1).jpg" > and so on. > > Or am I missing something? Hmm, I need to review this part. My idea was to export it as "foo (2).jpg" (that's not what the code is doing and that's my mistake). I guess it can be confusing if there are both "foo.jpg" and "foo(1).jpg". If I export a file as "foo(2).png", was it exported from foo or foo(1)? I agree that "foo (1)(1).jpg" makes more sense.
Created attachment 323276 [details] [review] Allow multiple exports of same file Fixing raised points.
(In reply to Rafael Fonseca from comment #10) > > ::: src/photos-utils.c > > @@ +992,3 @@ > > + free_resources: > > + g_free (filename); > > + g_clear_object (&unique_file); > > > > We are leaking stream. We need a: > > g_clear_object (&stream); > > We are not. Note that I pass g_object_unref to return_pointer and the ref'ed > stream there is unref'ed in base_item_create_file_async_finish. I see that you fixed it, so I guess you understood the problem. :) In any case, the issue is that even if you pass g_object_unref to g_task_return_pointer, once you have called g_task_propagate_pointer (through the finish method), the g_object_unref is not called. This is because "propagate" functions transfer ownership. In this case, it transfers ownership to the g_task_propagate_pointer caller (ie. the caller of the finish method). Therefore, we will end up leaking one reference.
Review of attachment 323276 [details] [review]: Thanks for the new patch, Rafael. Looks very good apart from a few minor niggles. ::: src/photos-base-item.c @@ +1221,3 @@ data = (PhotosBaseItemSaveData *) g_task_get_task_data (task); + g_clear_object (&data->filename); Typo alert: data->filename is gchar *, so g_clear_object is not ideal. :) In practice it works because data->filename is NULL here. It would be better to assert NULL here. That way we would know that none of the previous steps are misusing this member in unexpected ways. Whereas, a "g_clear_object" will mask that. ::: src/photos-utils.c @@ +70,3 @@ +typedef struct _PhotosUtilsSaveData PhotosUtilsSaveData; +struct _PhotosUtilsSaveData { s/Save/Create/ :) Since this file is a kitchen sink with random things in it, might be better to call it PhotosUtilsFileCreateData to protect against future additions of other "create" routines. @@ +73,3 @@ + GFile *dir; + gchar *basename; + gchar *ext; Nitpick: extension would be slightly better. @@ +1002,3 @@ +{ + GTask *task = G_TASK (res); + PhotosUtilsSaveData *data = g_task_get_task_data (task); Better not to call a g_task_* method before the g_task_is_valid assertion below. @@ +1009,3 @@ + g_return_val_if_fail (data != NULL, NULL); + + *filename = photos_utils_save_data_get_filename (data); Would be good if had a filename != NULL check, because, by convention, one can pass NULL to ignore an 'out' variable. I know we are not going to ignore it, but it is a little nicety that helps with future use-cases. ::: src/photos-utils.h @@ +132,3 @@ + GAsyncResult *res, + GError **error, + gchar **filename); Nitpick: (a) the convention is to put GError ** as the last parameter, (b) since this is a GFile API, it would be nicer to return the unique file as a GFile **.
Created attachment 323726 [details] [review] base-item, utils: Handle name collisions while exporting Tweaked the above issues, reformatted the commit message to be within 72 characters and pushed.