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 794845 - magnatune: Add cover art support
magnatune: Add cover art support
Status: RESOLVED FIXED
Product: grilo
Classification: Other
Component: plugins
unspecified
Other All
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2018-03-30 17:33 UTC by Jean Felder
Modified: 2018-03-31 19:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
magnatune: Add cover art support (4.97 KB, patch)
2018-03-30 17:33 UTC, Jean Felder
none Details | Review
magnatune: Remove trailing spaces (1.77 KB, patch)
2018-03-31 12:13 UTC, Jean Felder
committed Details | Review
magnatune: Add cover art support (3.78 KB, patch)
2018-03-31 12:13 UTC, Jean Felder
committed Details | Review

Description Jean Felder 2018-03-30 17:33:03 UTC
It follows the specifications defined at:
http://magnatune.com/info/sqlite-normalized
Comment 1 Jean Felder 2018-03-30 17:33:08 UTC
Created attachment 370350 [details] [review]
magnatune: Add cover art support

It follows the specifications defined at:
http://magnatune.com/info/sqlite-normalized
Comment 2 Bastien Nocera 2018-03-30 23:28:47 UTC
Victor should be the one reviewing this.
Comment 3 Victor Toso 2018-03-31 11:06:43 UTC
Review of attachment 370350 [details] [review]:

Thanks for the patch! Just small comments on the code but the change itself looks good.

::: src/magnatune/grl-magnatune.c
@@ +117,3 @@
+
+#define COVER_ART_SIZES 50, 75, 100, 160, 200, 300, 600, 1400
+#define COVER_ART_LENGTH 8

You might move that cover_sizes[] declaration here instead of these two defines. To get the length of static array you can use G_N_ELEMENTS() on it.

@@ +352,3 @@
   gchar *content = NULL;
   gsize length = 0;
+  gboolean ret = FALSE;

Trailing spaces should be done in an extra patch.

@@ +387,3 @@
 {
   GrlNetWc *wc = NULL;
+

ditto

@@ +512,3 @@
   gchar *old_crc = NULL;
   gsize length = 0;
+  gboolean ret = FALSE;

ditto

@@ +613,3 @@
             gint duration,
+            const gchar *url_to_mp3,
+	    GPtrArray *url_to_covers)

No tabs, space only please.

@@ +653,3 @@
+  gchar *encoded_album;
+  int cover_sizes[] = {COVER_ART_SIZES};
+  gchar *cover;

You can move 'cover' to inside the for scope

@@ +672,3 @@
+  for (i = 0; i < COVER_ART_LENGTH; i++) {
+    cover = g_strdup_printf("%s/%s/%s/%s_%d.%s", URL_SONG_COVER, encoded_artist,
+			    encoded_album, "cover", cover_sizes[i], "jpg");

Maybe an extra define here would be useful, such as:

URL_SONG_COVER_FORMAT URL_SONG_COVER "/%s/%s/cover_%d.jpg"

@@ +707,3 @@
 }
 
+static GList*

ditto

@@ +733,3 @@
   }
 
+  while ((ret = sqlite3_step(sql_stmt)) == SQLITE_BUSY);

ditto
Comment 4 Jean Felder 2018-03-31 12:13:07 UTC
Created attachment 370376 [details] [review]
magnatune: Remove trailing spaces
Comment 5 Jean Felder 2018-03-31 12:13:17 UTC
Created attachment 370377 [details] [review]
magnatune: Add cover art support

It follows the specifications defined at:
http://magnatune.com/info/sqlite-normalized
Comment 6 Victor Toso 2018-03-31 19:41:25 UTC
Review of attachment 370376 [details] [review]:

Pushed as de694c68862e86fa6ff2ed3c0d2dd4e765afce18
Comment 7 Victor Toso 2018-03-31 19:43:05 UTC
Review of attachment 370377 [details] [review]:

Moved the cover_art[] array up (global) and removed the #define COVER_ART_SIZES
Pushed as 81076c6da54599acd814e2ce628fb6e2208cbed5

Many thanks!