GNOME.org
 

[easytag] Refactor et_picture_new() to be more complete

commit 0019f3dc197585dc61bc34743b57b9ffff1bd9a9
Author: David King <amigadave amigadave com>
Date:   Sun Dec 28 11:33:24 2014 +0000

    Refactor et_picture_new() to be more complete
    
    Make et_picture_new() both create a new EtPicture instance and
    initialize all the fields. Copy the description string and ref the image
    data GBytes. Adapt existing users (and tests) to the new interface.

 src/picture.c         |   58 ++++++++++++++++++++++++------------
 src/picture.h         |    4 +-
 src/tag_area.c        |   22 +++++++++----
 src/tags/flac_tag.c   |   31 +++++++++----------
 src/tags/id3v24_tag.c |   39 +++++++++++++++---------
 src/tags/mp4_tag.cc   |   10 ++++--
 src/tags/ogg_tag.c    |   79 ++++++++++++++++++++++++++++---------------------
 tests/test-picture.c  |   42 +++++++++++---------------
 8 files changed, 164 insertions(+), 121 deletions(-)
---
diff --git a/src/picture.c b/src/picture.c
index cc3a233..a9f53cd 100644
--- a/src/picture.c
+++ b/src/picture.c
@@ -279,10 +279,40 @@ et_picture_format_info (const EtPicture *pic,
     return r;
 }
 
+/*
+ * et_picture_new:
+ * @type: the image type
+ * @description: a text description
+ * @width: image width
+ * @height image height
+ * @bytes: image data
+ *
+ * Create a new #EtPicture instance, copying the string and adding a reference
+ * to the image data.
+ *
+ * Returns: a new #EtPicture, or %NULL on failure
+ */
 EtPicture *
-et_picture_new (void)
+et_picture_new (EtPictureType type,
+                const gchar *description,
+                guint width,
+                guint height,
+                GBytes *bytes)
 {
-    EtPicture *pic = g_slice_new0 (EtPicture);
+    EtPicture *pic;
+
+    g_return_val_if_fail (description != NULL, NULL);
+    g_return_val_if_fail (bytes != NULL, NULL);
+
+    pic = g_slice_new (EtPicture);
+
+    pic->type = type;
+    pic->description = g_strdup (description);
+    pic->width = width;
+    pic->height = height;
+    pic->bytes = g_bytes_ref (bytes);
+    pic->next = NULL;
+
     return pic;
 }
 
@@ -293,17 +323,8 @@ et_picture_copy_single (const EtPicture *pic)
 
     g_return_val_if_fail (pic != NULL, NULL);
 
-    pic2 = et_picture_new ();
-    pic2->type = pic->type;
-    pic2->width  = pic->width;
-    pic2->height = pic->height;
-
-    if (pic->description)
-    {
-        pic2->description = g_strdup (pic->description);
-    }
-
-    pic2->bytes = g_bytes_ref (pic->bytes);
+    pic2 = et_picture_new (pic->type, pic->description, pic->width,
+                           pic->height, pic->bytes);
 
     return pic2;
 }
@@ -349,9 +370,9 @@ et_picture_free (EtPicture *pic)
  *
  * Load an image from the supplied @file.
  *
- * Returns: an image on success, %NULL otherwise
+ * Returns: image data on success, %NULL otherwise
  */
-EtPicture *
+GBytes *
 et_picture_load_file_data (GFile *file, GError **error)
 {
     gsize size;
@@ -404,7 +425,7 @@ et_picture_load_file_data (GFile *file, GError **error)
     else
     {
         /* Image loaded. */
-        EtPicture *pic;
+        GBytes *bytes;
         gpointer data;
         gsize data_size;
 
@@ -419,15 +440,14 @@ et_picture_load_file_data (GFile *file, GError **error)
 
         g_assert (error == NULL || *error == NULL);
 
-        pic = et_picture_new ();
         data = g_memory_output_stream_steal_data (G_MEMORY_OUTPUT_STREAM (ostream));
         data_size = g_memory_output_stream_get_data_size (G_MEMORY_OUTPUT_STREAM (ostream));
-        pic->bytes = g_bytes_new_take (data, data_size);
+        bytes = g_bytes_new_take (data, data_size);
         /* TODO: Use g_memory_output_stream_steal_as_bytes(). */
 
         g_object_unref (ostream);
         g_assert (error == NULL || *error == NULL);
-        return pic;
+        return bytes;
     }
 }
 
diff --git a/src/picture.h b/src/picture.h
index 4b0285b..b775e54 100644
--- a/src/picture.h
+++ b/src/picture.h
@@ -75,7 +75,7 @@ typedef enum
 } Picture_Format;
 
 GType et_picture_get_type (void);
-EtPicture * et_picture_new (void);
+EtPicture * et_picture_new (EtPictureType type, const gchar *description, guint width, guint height, GBytes 
*bytes);
 EtPicture * et_picture_copy_single (const EtPicture *pic);
 EtPicture * et_picture_copy_all (const EtPicture *pic);
 void et_picture_free (EtPicture *pic);
@@ -84,7 +84,7 @@ const gchar   *Picture_Mime_Type_String (Picture_Format format);
 const gchar * Picture_Type_String (EtPictureType type);
 gchar * et_picture_format_info (const EtPicture *pic, ET_Tag_Type tag_type);
 
-EtPicture * et_picture_load_file_data (GFile *file, GError **error);
+GBytes * et_picture_load_file_data (GFile *file, GError **error);
 gboolean et_picture_save_file_data (const EtPicture *pic, GFile *file, GError **error);
 
 EtPictureType et_picture_type_from_filename (const gchar *filename_utf8);
diff --git a/src/tag_area.c b/src/tag_area.c
index 6cc3626..7a743c7 100644
--- a/src/tag_area.c
+++ b/src/tag_area.c
@@ -1357,7 +1357,7 @@ static void
 load_picture_from_file (GFile *file,
                         EtTagArea *self)
 {
-    EtPicture *pic;
+    GBytes *bytes;
     const gchar *filename_utf8;
     GFileInfo *info;
     GError *error = NULL;
@@ -1373,9 +1373,9 @@ load_picture_from_file (GFile *file,
     }
 
     filename_utf8 = g_file_info_get_display_name (info);
-    pic = et_picture_load_file_data (file, &error);
+    bytes = et_picture_load_file_data (file, &error);
 
-    if (!pic)
+    if (!bytes)
     {
         GtkWidget *msgdialog;
 
@@ -1404,12 +1404,17 @@ load_picture_from_file (GFile *file,
 
     if (filename_utf8)
     {
+        EtPicture *pic;
+        EtPictureType type;
+        const gchar *description;
+
         // Behaviour following the tag type...
         switch (ETCore->ETFileDisplayed->ETFileDescription->TagType)
         {
             // Only one picture supported for MP4
             case MP4_TAG:
-                pic->type = ET_PICTURE_TYPE_FRONT_COVER;
+                description = "";
+                type = ET_PICTURE_TYPE_FRONT_COVER;
                 break;
 
             // Other tag types
@@ -1419,16 +1424,16 @@ load_picture_from_file (GFile *file,
             case APE_TAG:
             case FLAC_TAG:
             case WAVPACK_TAG:
-                pic->description = g_strdup (filename_utf8);
+                description = filename_utf8;
 
                 if (g_settings_get_boolean (MainSettings,
                                             "tag-image-type-automatic"))
                 {
-                    pic->type = et_picture_type_from_filename (pic->description);
+                    type = et_picture_type_from_filename (pic->description);
                 }
                 else
                 {
-                    pic->type = ET_PICTURE_TYPE_FRONT_COVER;
+                    type = ET_PICTURE_TYPE_FRONT_COVER;
                 }
                 break;
 
@@ -1436,11 +1441,14 @@ load_picture_from_file (GFile *file,
                 g_assert_not_reached ();
         }
 
+        pic = et_picture_new (type, description, 0, 0, bytes);
+
         PictureEntry_Update (self, pic, TRUE);
 
         et_picture_free (pic);
     }
 
+    g_bytes_unref (bytes);
     g_object_unref (info);
 }
 
diff --git a/src/tags/flac_tag.c b/src/tags/flac_tag.c
index e07fbc9..f4a42f3 100644
--- a/src/tags/flac_tag.c
+++ b/src/tags/flac_tag.c
@@ -661,33 +661,30 @@ flac_tag_read_file_tag (GFile *file,
             //
             case FLAC__METADATA_TYPE_PICTURE: 
             {
-                
-                /***********
-                 * Picture *
-                 ***********/
+                /* Picture. */
                 FLAC__StreamMetadata_Picture *p;
+                GBytes *bytes;
                 EtPicture *pic;
             
-                // Get picture data from block
+                /* Get picture data from block. */
                 p = &block->data.picture;
+
+                bytes = g_bytes_new (p->data, p->data_length);
             
-                pic = et_picture_new ();
+                pic = et_picture_new (p->type, (const gchar *)p->description,
+                                      0, 0, bytes);
+                g_bytes_unref (bytes);
+
                 if (!prev_pic)
+                {
                     FileTag->picture = pic;
+                }
                 else
+                {
                     prev_pic->next = pic;
-                prev_pic = pic;
-
-                pic->bytes = g_bytes_new (p->data, p->data_length);
-                pic->type = p->type;
-                pic->description = g_strdup((gchar *)p->description);
-                // Not necessary: will be calculated later
-                //pic->height = p->height;
-                //pic->width  = p->width;
-
-                //g_print("Image type : %s\n",FLAC__StreamMetadata_Picture_TypeString[p->type]);
-                //g_print("Mime    type : %s\n",p->mime_type);
+                }
 
+                prev_pic = pic;
                 break;
             }
                 
diff --git a/src/tags/id3v24_tag.c b/src/tags/id3v24_tag.c
index a855f13..4a42dda 100644
--- a/src/tags/id3v24_tag.c
+++ b/src/tags/id3v24_tag.c
@@ -446,15 +446,11 @@ id3tag_read_file_tag (GFile *gfile,
      ******************/
     for (i = 0; (frame = id3_tag_findframe(tag, "APIC", i)); i++)
     {
+        GBytes *bytes = NULL;
+        EtPictureType type = ET_PICTURE_TYPE_FRONT_COVER;
+        gchar *description;
         EtPicture *pic;
 
-        pic = et_picture_new ();
-        if (!prev_pic)
-            FileTag->picture = pic;
-        else
-            prev_pic->next = pic;
-        prev_pic = pic;
-
         /* Picture file data. */
         for (j = 0; (field = id3_frame_field(frame, j)); j++)
         {
@@ -469,26 +465,41 @@ id3tag_read_file_tag (GFile *gfile,
 
                         if (data)
                         {
-                            if (pic->bytes)
+                            if (bytes)
                             {
-                                g_bytes_unref (pic->bytes);
+                                g_bytes_unref (bytes);
                             }
 
-                            pic->bytes = g_bytes_new (data, size);
+                            bytes = g_bytes_new (data, size);
                         }
                     }
                     break;
                 case ID3_FIELD_TYPE_INT8:
-                    /* TODO: Verify that the type matches with EtPictureType. */
-                    pic->type = id3_field_getint(field);
+                    type = id3_field_getint (field);
                     break;
                 default:
                     break;
             }
         }
 
-        // Picture description
-        update |= libid3tag_Get_Frame_Str(frame, EASYTAG_ID3_FIELD_STRING, &pic->description);
+        /* Picture description. */
+        update |= libid3tag_Get_Frame_Str (frame, EASYTAG_ID3_FIELD_STRING,
+                                           &description);
+
+        pic = et_picture_new (type, description, 0, 0, bytes);
+        g_bytes_unref (bytes);
+        g_free (description);
+
+        if (!prev_pic)
+        {
+            FileTag->picture = pic;
+        }
+        else
+        {
+            prev_pic->next = pic;
+        }
+
+        prev_pic = pic;
     }
 
     /**********************
diff --git a/src/tags/mp4_tag.cc b/src/tags/mp4_tag.cc
index 382e745..32eaf20 100644
--- a/src/tags/mp4_tag.cc
+++ b/src/tags/mp4_tag.cc
@@ -209,11 +209,13 @@ mp4tag_read_file_tag (GFile *file,
         const TagLib::MP4::CoverArtList covers = cover.toCoverArtList ();
         const TagLib::MP4::CoverArt &art = covers.front ();
 
-        FileTag->picture = et_picture_new ();
-
         /* TODO: Use g_bytes_new_with_free_func()? */
-        FileTag->picture->bytes = g_bytes_new (art.data ().data (),
-                                               art.data ().size ());
+        GBytes *bytes = g_bytes_new (art.data ().data (), art.data ().size ());
+
+        /* MP4 does not support image types, nor descriptions. */
+        FileTag->picture = et_picture_new (ET_PICTURE_TYPE_FRONT_COVER, "", 0,
+                                           0, bytes);
+        g_bytes_unref (bytes);
     }
     else
     {
diff --git a/src/tags/ogg_tag.c b/src/tags/ogg_tag.c
index 3a48834..815f6ce 100644
--- a/src/tags/ogg_tag.c
+++ b/src/tags/ogg_tag.c
@@ -484,38 +484,43 @@ et_add_file_tags_from_vorbis_comments (vorbis_comment *vc,
         EtPicture *pic;
         guchar *data;
         gsize data_size;
+        GBytes *bytes;
+        EtPictureType type;
+        const gchar *description;
             
         /* Force marking the file as modified, so that the deprecated cover art
          * field in converted in a METADATA_PICTURE_BLOCK field. */
         FileTag->saved = FALSE;
 
-        pic = et_picture_new ();
-
-        if (!prev_pic)
-            FileTag->picture = pic;
-        else
-            prev_pic->next = pic;
-        prev_pic = pic;
-
         /* Decode picture data. */
         data = g_base64_decode (string, &data_size);
-        pic->bytes = g_bytes_new_take (data, data_size);
+        bytes = g_bytes_new_take (data, data_size);
 
-        if ( (string = vorbis_comment_query(vc,"COVERARTTYPE",field_num-1)) != NULL )
+        if ((string = vorbis_comment_query (vc, "COVERARTTYPE", field_num - 1))
+            != NULL)
         {
-            pic->type = atoi(string);
+            type = atoi (string);
         }
 
-        if ( (string = vorbis_comment_query(vc,"COVERARTDESCRIPTION",field_num-1)) != NULL )
+        if ((string = vorbis_comment_query (vc, "COVERARTDESCRIPTION",
+                                            field_num - 1)) != NULL)
         {
-            pic->description = g_strdup(string);
+            description = string;
         }
 
-        //if ((string = vorbis_comment_query(vc,"COVERTARTMIME",field_num-1)) != NULL )
-        //{
-        //    pic->description = g_strdup(string);
-        //}
+        pic = et_picture_new (type, description, 0, 0, bytes);
+        g_bytes_unref (bytes);
+
+        if (!prev_pic)
+        {
+            FileTag->picture = pic;
+        }
+        else
+        {
+            prev_pic->next = pic;
+        }
 
+        prev_pic = pic;
     }
 
     /* METADATA_BLOCK_PICTURE tag used for picture information */
@@ -527,28 +532,18 @@ et_add_file_tags_from_vorbis_comments (vorbis_comment *vc,
         gsize bytes_pos, mimelen, desclen;
         guchar *decoded_ustr;
         GBytes *bytes;
+        EtPictureType type;
+        gchar *description;
+        GBytes *pic_bytes;
         gsize decoded_size;
         gsize data_size;
 
-        pic = et_picture_new ();
-
-        if (!prev_pic)
-        {
-            FileTag->picture = pic;
-        }
-        else
-        {
-            prev_pic->next = pic;
-        }
-
-        prev_pic = pic;
-
         /* Decode picture data. */
         decoded_ustr = g_base64_decode (string, &decoded_size);
         bytes = g_bytes_new_take (decoded_ustr, decoded_size);
 
         /* Reading picture type. */
-        pic->type = read_guint32_from_byte (decoded_ustr, 0);
+        type = read_guint32_from_byte (decoded_ustr, 0);
         bytes_pos = 4;
 
         /* Reading MIME data. */
@@ -559,8 +554,8 @@ et_add_file_tags_from_vorbis_comments (vorbis_comment *vc,
         desclen = read_guint32_from_byte (decoded_ustr, bytes_pos);
         bytes_pos += 4;
 
-        pic->description = g_strndup ((const gchar *)&decoded_ustr[bytes_pos],
-                                      desclen);
+        description = g_strndup ((const gchar *)&decoded_ustr[bytes_pos],
+                                 desclen);
 
         bytes_pos += desclen + 16;
 
@@ -569,7 +564,23 @@ et_add_file_tags_from_vorbis_comments (vorbis_comment *vc,
         bytes_pos += 4;
 
         /* Read only the image data into a new GBytes. */
-        pic->bytes = g_bytes_new_from_bytes (bytes, bytes_pos, data_size);
+        pic_bytes = g_bytes_new_from_bytes (bytes, bytes_pos, data_size);
+
+        pic = et_picture_new (type, description, 0, 0, pic_bytes);
+
+        g_free (description);
+        g_bytes_unref (pic_bytes);
+
+        if (!prev_pic)
+        {
+            FileTag->picture = pic;
+        }
+        else
+        {
+            prev_pic->next = pic;
+        }
+
+        prev_pic = pic;
 
         /* pic->bytes still holds a ref on the decoded data. */
         g_bytes_unref (bytes);
diff --git a/tests/test-picture.c b/tests/test-picture.c
index 81ecb29..8948a97 100644
--- a/tests/test-picture.c
+++ b/tests/test-picture.c
@@ -23,6 +23,7 @@
 static void
 picture_copy (void)
 {
+    GBytes *bytes;
     EtPicture *pic1;
     EtPicture *pic2;
     EtPicture *pic3;
@@ -32,13 +33,10 @@ picture_copy (void)
     const EtPicture *pic3_copy;
     EtPicture *pic4_copy;
 
-    pic1 = et_picture_new ();
-
-    pic1->type = ET_PICTURE_TYPE_LEAFLET_PAGE;
-    pic1->width = 640;
-    pic1->height = 480;
-    pic1->description = g_strdup ("foobar.png");
-    pic1->bytes = g_bytes_new_static ("foobar", 6);
+    bytes = g_bytes_new_static ("foobar", 6);
+    pic1 = et_picture_new (ET_PICTURE_TYPE_LEAFLET_PAGE, "foobar.png", 640,
+                           480, bytes);
+    g_bytes_unref (bytes);
 
     pic2 = et_picture_copy_all (pic1);
 
@@ -51,13 +49,10 @@ picture_copy (void)
     g_assert (pic2->bytes == pic1->bytes);
     g_assert (pic2->next == NULL);
 
-    pic3 = et_picture_new ();
-
-    pic3->type = ET_PICTURE_TYPE_ILLUSTRATION;
-    pic3->width = 320;
-    pic3->height = 240;
-    pic3->description = g_strdup ("bash.jpg");
-    pic3->bytes = g_bytes_new_static ("foo", 3);
+    bytes = g_bytes_new_static ("foo", 3);
+    pic3 = et_picture_new (ET_PICTURE_TYPE_ILLUSTRATION, "bash.jpg", 320, 240,
+                           bytes);
+    g_bytes_unref (bytes);
 
     pic1->next = pic2;
     pic2->next = pic3;
@@ -85,13 +80,9 @@ picture_copy (void)
     g_assert_cmpstr (pic3_copy->description, ==, "bash.jpg");
     g_assert_cmpint (g_bytes_get_size (pic3_copy->bytes), ==, 3);
 
-    pic4 = et_picture_new ();
-
-    pic4->type = ET_PICTURE_TYPE_MEDIA;
-    pic4->width = 800;
-    pic4->height = 600;
-    pic4->description = g_strdup ("baz.gif");
-    pic4->bytes = g_bytes_new_static ("foobarbaz", 9);
+    bytes = g_bytes_new_static ("foobarbaz", 9);
+    pic4 = et_picture_new (ET_PICTURE_TYPE_MEDIA, "baz.gif", 800, 600, bytes);
+    g_bytes_unref (bytes);
 
     pic4_copy = g_boxed_copy (ET_TYPE_PICTURE, pic4);
 
@@ -174,11 +165,14 @@ picture_format_from_data (void)
 
     for (i = 0; i < G_N_ELEMENTS (pictures); i++)
     {
+        GBytes *bytes;
         EtPicture *pic;
 
-        pic = et_picture_new ();
-        pic->bytes = g_bytes_new_static (pictures[i].data,
-                                         strlen (pictures[i].data) + 1);
+        bytes = g_bytes_new_static (pictures[i].data,
+                                    strlen (pictures[i].data) + 1);
+        pic = et_picture_new (ET_PICTURE_TYPE_FRONT_COVER, "", 0, 0, bytes);
+        g_bytes_unref (bytes);
+
         g_assert_cmpint (pictures[i].format, ==,
                          Picture_Format_From_Data (pic));
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]