GNOME Bugzilla – Bug 725399
Create loading and saving of pictures Asynchronous in picture.c
Last modified: 2021-05-26 09:51:47 UTC
Loading and saving of pictures should be made asynchronous. This would include modifying code in picture.c
Created attachment 270568 [details] [review] This patch will make loading of pictures asynchronous in picture.c
Created attachment 270875 [details] [review] This patch will make loading and saving of pictures asynchronous in picture.c This patch applies for saving of pictures also. Also, few problems of previous patch is also corrected.
Review of attachment 270875 [details] [review]: Looks fine in general. ::: src/picture.c @@ +134,3 @@ + if (!et_picture_save_file_data (data->pic, data->file, &error)) + { + Log_Print (LOG_ERROR, _("Image file not saved: %s"), If this is running in a different thread, can you safely call Log_Print(), which will call gtk_*() functions? @@ +243,3 @@ + GCancellable *cancellable) +{ + gchar **uri_list = g_async_result_get_user_data ((GAsyncResult*)res); Use "G_ASYNC_RESULT (res)" instead of the cast. @@ +282,3 @@ + et_picture_load_file_thread_func, + G_PRIORITY_DEFAULT, NULL); + gtk_widget_set_sensitive (MainWindow, FALSE); You should call this before starting the thread.
Created attachment 270999 [details] [review] This patch will make loading and saving of pictures asynchronous in picture.c About Log_Print, yeah you are right. So, I transferred Log_Print to callback. All other changes are done.
Review of attachment 270999 [details] [review]: ::: src/picture.c @@ +64,3 @@ +static void +et_picture_load_file (GFile *file, gpointer user_data); +static void Because of the way you ordered the functions, there is no need to add any prototypes here, so you can remove them. @@ +247,3 @@ + gchar **uri_list = g_async_result_get_user_data (G_ASYNC_RESULT (res)); + gchar **uri = uri_list; + while (*uri && strlen(*uri)) Instead of: if (*strv && strlen(*strv)) it is better to do: if (*strv && *(*strv)) This is done quite often in EasyTAG to check for an empty string, and eventually it should all be replaced by an inline function which does the right thing. @@ +440,3 @@ { + GSimpleAsyncResult *simple; + ErrorDialogData *data = g_malloc (sizeof (ErrorDialogData)); As you know the type of the struct when you free it, you should use g_slice_new() here, and g_slice_free() when freeing. @@ +442,3 @@ + ErrorDialogData *data = g_malloc (sizeof (ErrorDialogData)); + data->error = error; + data->filename = filename_utf8; This assignment relies on the string remaining in memory, which is only the case because the GFileInfo is leaked when returning from this block. You should fix both problems. @@ +585,1 @@ // Save the directory selected for initialize next time You should keep the empty line that existed before the comment. @@ +928,3 @@ GFile *file; + GSimpleAsyncResult *res; + PictureSaveData *data = g_malloc (sizeof (PictureSaveData)); Also use g_slice_new() here.
Created attachment 271079 [details] [review] This patch will make loading of pictures asynchronous in picture.c
Comment on attachment 271079 [details] [review] This patch will make loading of pictures asynchronous in picture.c Oops, I just pushed your previous patch to master. Can you rebase your changes in this patch and create another one on top. Thanks, and sorry!
Created attachment 271081 [details] [review] Rebased changes according to previous patch.
Comment on attachment 271081 [details] [review] Rebased changes according to previous patch. Thanks for the updated patch, I pushed it with slight modifications as f432ebd26c9bf5fc060eecb0c90635ca4acd3abd.
There were a large number of stability problems introduced by the asynchronous image processing, so I reverted the changes for now. If adding asynchronous processing in future, the code which runs in a thread must be very careful not to call any GTK+ functions. https://bugzilla.redhat.com/show_bug.cgi?id=1088022 https://bugzilla.redhat.com/show_bug.cgi?id=1091577
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/easytag/-/issues/ Thank you for your understanding and your help.