Repository: beetbox/beets
Issue: Non-determinism of MediaFile.art and embedart plugin

@mried work on #1328 brought to light the fact that the `extractart` plugin relies on `MediaFile.art` which is as "legacy album art field", the "first image on a file". However this seems non-deterministic (sometime returns a 2nd+ image) especially on python 2.6 (sigh).

Moreover `extractart` works with this field, while it could work with `images`, extracting all arts from a file (`MediaFile.images`).

The questions raised are:
- how does MediaFile determines the `art` property?
- should the `art` property be kept? It seems used by the plugins `scrub` and `embedart`
- shouldn't `embedart` manage all art files?


Changed files:
test/test_embedart.py, beets/mediafile.py, test/test_mediafile.py

diff --git a/beets/mediafile.py b/beets/mediafile.py
index 6c5d0a2c2..22899dee0 100644
--- a/beets/mediafile.py
+++ b/beets/mediafile.py
@@ -1246,18 +1246,31 @@ class DateItemField(MediaField):
 
 class CoverArtField(MediaField):
     """A descriptor that provides access to the *raw image data* for the
-    first image on a file. This is used for backwards compatibility: the
+    cover image on a file. This is used for backwards compatibility: the
     full `ImageListField` provides richer `Image` objects.
+
+    When there are multiple images we try to pick the most likely to be a front
+    cover.
     """
     def __init__(self):
         pass
 
     def __get__(self, mediafile, _):
-        try:
-            return mediafile.images[0].data
-        except IndexError:
+        candidates = mediafile.images
+        if candidates:
+            return self.guess_cover_image(candidates).data
+        else:
             return None
 
+    @staticmethod
+    def guess_cover_image(candidates):
+        if len(candidates) == 1:
+            return candidates[0]
+        try:
+            return next(c for c in candidates if c.type == ImageType.front)
+        except StopIteration:
+            return candidates[0]
+
     def __set__(self, mediafile, data):
         if data:
             mediafile.images = [Image(data=data)]
diff --git a/test/test_embedart.py b/test/test_embedart.py
index 4a4f157ae..729f5853d 100644
--- a/test/test_embedart.py
+++ b/test/test_embedart.py
@@ -124,11 +124,8 @@ class EmbedartCliTest(_common.TestCase, TestHelper):
 
         self.run_command('extractart', '-n', 'extracted')
 
-        path = os.path.join(albumpath.decode('utf-8'), 'extracted.')
-        found = any(map(lambda ext: os.path.exists(path + ext),
-                        ['png', 'jpeg', 'jpg']))
-
-        self.assertTrue(found, 'Extracted image was not found!')
+        self.assertExists(os.path.join(albumpath.decode('utf-8'),
+                                       'extracted.png'))
 
 
 class EmbedartTest(unittest.TestCase):
diff --git a/test/test_mediafile.py b/test/test_mediafile.py
index 8b23d1cee..cd149e7e4 100644
--- a/test/test_mediafile.py
+++ b/test/test_mediafile.py
@@ -28,7 +28,7 @@ from test import _common
 from test._common import unittest
 from beets.mediafile import MediaFile, MediaField, Image, \\
     MP3DescStorageStyle, StorageStyle, MP4StorageStyle, \\
-    ASFStorageStyle, ImageType
+    ASFStorageStyle, ImageType, CoverArtField
 from beets.library import Item
 from beets.plugins import BeetsPlugin
 
@@ -161,6 +161,13 @@ class ImageStructureTestMixin(ArtTestMixin):
         mediafile = MediaFile(mediafile.path)
         self.assertEqual(len(mediafile.images), 0)
 
+    def test_guess_cover(self):
+        mediafile = self._mediafile_fixture('image')
+        self.assertEqual(len(mediafile.images), 2)
+        cover = CoverArtField.guess_cover_image(mediafile.images)
+        self.assertEqual(cover.desc, 'album cover')
+        self.assertEqual(mediafile.art, cover.data)
+
     def assertExtendedImageAttributes(self, image, **kwargs):
         """Ignore extended image attributes in the base tests.
         """
@@ -662,7 +669,7 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin,
                 errors.append('Tag %s does not exist' % key)
             else:
                 if value2 != value:
-                    errors.append('Tag %s: %s != %s' % (key, value2, value))
+                    errors.append('Tag %s: %r != %r' % (key, value2, value))
         if any(errors):
             errors = ['Tags did not match'] + errors
             self.fail('\\n  '.join(errors))
@@ -758,6 +765,10 @@ class MP4Test(ReadWriteTestBase, PartialTestMixin,
         with self.assertRaises(ValueError):
             mediafile.images = [Image(data=self.tiff_data)]
 
+    def test_guess_cover(self):
+        # There is no metadata associated with images, we pick one at random
+        pass
+
 
 class AlacTest(ReadWriteTestBase, unittest.TestCase):
     extension = 'alac.m4a'