After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 725399 - Create loading and saving of pictures Asynchronous in picture.c
Create loading and saving of pictures Asynchronous in picture.c
Status: RESOLVED OBSOLETE
Product: easytag
Classification: Other
Component: general
master
Other All
: Normal normal
: 2.1
Assigned To: EasyTAG maintainer(s)
EasyTAG maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-02-28 13:53 UTC by Abhinav
Modified: 2021-05-26 09:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
This patch will make loading of pictures asynchronous in picture.c (9.51 KB, patch)
2014-02-28 14:01 UTC, Abhinav
none Details | Review
This patch will make loading and saving of pictures asynchronous in picture.c (12.85 KB, patch)
2014-03-04 08:08 UTC, Abhinav
needs-work Details | Review
This patch will make loading and saving of pictures asynchronous in picture.c (12.99 KB, patch)
2014-03-05 15:38 UTC, Abhinav
needs-work Details | Review
This patch will make loading of pictures asynchronous in picture.c (11.85 KB, patch)
2014-03-06 10:02 UTC, Abhinav
reviewed Details | Review
Rebased changes according to previous patch. (2.37 KB, patch)
2014-03-06 10:31 UTC, Abhinav
committed Details | Review

Description Abhinav 2014-02-28 13:53:11 UTC
Loading and saving of pictures should be made asynchronous. This would include modifying code in picture.c
Comment 1 Abhinav 2014-02-28 14:01:20 UTC
Created attachment 270568 [details] [review]
This patch will make loading of pictures asynchronous in picture.c
Comment 2 Abhinav 2014-03-04 08:08:25 UTC
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.
Comment 3 David King 2014-03-05 14:50:21 UTC
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.
Comment 4 Abhinav 2014-03-05 15:38:51 UTC
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.
Comment 5 David King 2014-03-05 23:43:58 UTC
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.
Comment 6 Abhinav 2014-03-06 10:02:11 UTC
Created attachment 271079 [details] [review]
This patch will make loading of pictures asynchronous in picture.c
Comment 7 David King 2014-03-06 10:08:43 UTC
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!
Comment 8 Abhinav 2014-03-06 10:31:20 UTC
Created attachment 271081 [details] [review]
Rebased changes according to previous patch.
Comment 9 David King 2014-03-06 11:08:23 UTC
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.
Comment 10 David King 2014-04-26 09:01:47 UTC
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
Comment 11 André Klapper 2021-05-26 09:51:47 UTC
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.