Merge pull request #333 from exaile/tags

Fix all the tags bugs

tests/conftest.py

@@ -32,8 +32,10 @@ def exaile_test_cleanup():
 #
 
 
-TrackData = collections.namedtuple(
-                'TrackData', ['ext', 'filename', 'uri', 'size', 'writeable'])
+TrackData = collections.namedtuple( 'TrackData',[
+    'ext', 'filename', 'uri', 'size', 'writeable',
+    'has_cover', 'has_tags'
+])
 
 
 def _fname(ext):
@@ -44,18 +46,18 @@ def _fname(ext):
     return ext, local_path, Gio.File.new_for_path(local_path).get_uri()
 
 _all_tracks = [
-    TrackData(*_fname('aac'),  size=3639,  writeable=True),
-    TrackData(*_fname('aiff'), size=16472, writeable=False),
-    TrackData(*_fname('au'),   size=16425, writeable=False),
-    TrackData(*_fname('flac'), size=17453, writeable=True),
-    TrackData(*_fname('mp3'),  size=4692,  writeable=True),
-    TrackData(*_fname('mp4'),  size=2968,  writeable=True),
-    TrackData(*_fname('mpc'),  size=6650,  writeable=True),
-    TrackData(*_fname('ogg'),  size=13002, writeable=True),
-    TrackData(*_fname('spx'),  size=1000,  writeable=True),
-    TrackData(*_fname('wav'),  size=46124, writeable=False),
-    TrackData(*_fname('wma'),  size=3864,  writeable=True),
-    TrackData(*_fname('wv'),   size=32293, writeable=True),
+    TrackData(*_fname('aac'),  size=9404,  writeable=True, has_cover=True, has_tags=True),
+    TrackData(*_fname('aiff'), size=16472, writeable=False, has_cover=False, has_tags=False),
+    TrackData(*_fname('au'),   size=16425, writeable=False, has_cover=False, has_tags=False),
+    TrackData(*_fname('flac'), size=20668, writeable=True, has_cover=True, has_tags=True),
+    TrackData(*_fname('mp3'),  size=7495,  writeable=True, has_cover=True, has_tags=True),
+    TrackData(*_fname('mp4'),  size=7763,  writeable=True, has_cover=True, has_tags=True),
+    TrackData(*_fname('mpc'),  size=6650,  writeable=True, has_cover=False, has_tags=True),
+    TrackData(*_fname('ogg'),  size=17303, writeable=True, has_cover=True, has_tags=True),
+    TrackData(*_fname('spx'),  size=1000,  writeable=True, has_cover=False, has_tags=True),
+    TrackData(*_fname('wav'),  size=46124, writeable=False, has_cover=False, has_tags=False),
+    TrackData(*_fname('wma'),  size=4929,  writeable=True, has_cover=False, has_tags=True),
+    TrackData(*_fname('wv'),   size=32293, writeable=True, has_cover=False, has_tags=True),
 ]
 
 
@@ -74,24 +76,22 @@ def writeable_track(request):
     return request.param
 
 
-@pytest.yield_fixture(params=_all_tracks)
-def test_track_fp(request):
-    t = request.param
-    with tempfile.NamedTemporaryFile(suffix='.' + t.ext) as tfp:
-        with open(t.filename, 'rb') as fp:
+@pytest.yield_fixture()
+def test_track_fp(test_track):
+    with tempfile.NamedTemporaryFile(suffix='.' + test_track.ext) as tfp:
+        with open(test_track.filename, 'rb') as fp:
             shutil.copyfileobj(fp, tfp)
 
         tfp.flush()
 
         yield tfp
 
 
-@pytest.yield_fixture(params=_writeable_tracks)
-def writeable_track_name(request):
+@pytest.yield_fixture()
+def writeable_track_name(writeable_track):
     '''Fixture that returns names of temporary copies of writeable tracks'''
-    t = request.param
-    with tempfile.NamedTemporaryFile(suffix='.' + t.ext) as tfp:
-        with open(t.filename, 'rb') as fp:
+    with tempfile.NamedTemporaryFile(suffix='.' + writeable_track.ext) as tfp:
+        with open(writeable_track.filename, 'rb') as fp:
             shutil.copyfileobj(fp, tfp)
 
         tfp.flush()

tests/xl/trax/test_track.py

@@ -11,6 +11,7 @@
 from mox3 import mox
 import pytest
 
+from xl.metadata import CoverImage
 import xl.trax.track as track
 import xl.settings as settings
 
@@ -84,6 +85,27 @@ def setup(self):
     def teardown(self):
         self.mox.UnsetStubs()
 
+    def verify_tags_exist(self, tr, test_track, deleted=None):
+        internal_tags = {'__length', '__modified', '__basedir', '__basename', '__loc'}
+        if test_track.ext not in ['aac', 'spx']:
+            internal_tags.add('__bitrate')
+
+        disk_tags = {'album', 'tracknumber', 'artist', 'title'}
+        normal_tags = internal_tags | disk_tags
+
+        if test_track.has_cover:
+            disk_tags.add('cover')
+
+        if deleted:
+            normal_tags.discard(deleted)
+            disk_tags.remove(deleted)
+
+        # check non-disk tag list
+        assert set(tr.list_tags()) == normal_tags
+
+        # check disk tag list
+        assert set(tr.list_tags_disk()) == disk_tags
+
     ## Creation
     def test_flyweight(self, test_track):
         """There can only be one object based on a url in args"""
@@ -188,33 +210,91 @@ def test_write_tags_no_perms(self, test_track_fp):
         tr.set_tag_raw('artist', random_str())
         assert not tr.write_tags()
 
-    def test_write_tag(self, writeable_track_name):
+    def test_write_tag(self, writeable_track, writeable_track_name):
 
         artist = random_str()
         tr = track.Track(writeable_track_name)
         tr.set_tag_raw('artist', artist)
-        tr.write_tags()
+        assert tr.write_tags() is not False
+        assert tr.get_tag_raw('artist') == [artist]
 
+        # if tag was changed, ensure it gets overridden by reading tags from file
         tr.set_tag_raw('artist', None)
         assert tr.get_tag_raw('artist') == None
 
-        tr.read_tags()
-
+        assert tr.read_tags() is not False
         assert tr.get_tag_raw('artist') == [artist]
+
+        self.verify_tags_exist(tr, writeable_track)
 
-    def test_delete_tag(self, writeable_track_name):
+    def test_delete_tag(self, writeable_track, writeable_track_name):
 
         artist = random_str()
         tr = track.Track(writeable_track_name)
         assert tr.get_tag_raw('artist') is not None
 
         tr.set_tag_raw('artist', None)
-        tr.write_tags()
+        assert tr.write_tags() is not False
+        assert tr.get_tag_raw('artist') == None
 
+        # if tag was deleted, ensure it gets overridden by reading tags from file
         tr.set_tag_raw('artist', artist)
-        tr.read_tags()
+        assert tr.get_tag_raw('artist') == [artist]
+
+        assert tr.read_tags() is not False
         assert tr.get_tag_raw('artist') == None
-
+
+        self.verify_tags_exist(tr, writeable_track, deleted='artist')
+
+    def test_delete_missing_tag(self, writeable_track, writeable_track_name):
+        tr = track.Track(writeable_track_name)
+        assert tr.get_tag_raw('genre') is None
+        tr.set_tag_raw('genre', None)
+        assert tr.get_tag_raw('genre') is None
+
+        self.verify_tags_exist(tr, writeable_track)
+
+        # this actually writes it to disk
+        assert tr.write_tags() is not False
+
+        # make sure another read works
+        assert tr.read_tags() is not False
+
+        self.verify_tags_exist(tr, writeable_track)
+
+
+    def test_write_delete_cover(self, writeable_track, writeable_track_name):
+        if not writeable_track.has_cover:
+            return
+
+        if writeable_track.ext in ['aac', 'mp4']:
+            from mutagen.mp4 import MP4Cover
+            newcover = CoverImage(None, None, 'image/jpeg', MP4Cover(random_str()))
+        else:
+            newcover = CoverImage(3, 'cover', 'image/jpeg', bytes(random_str()))
+
+        tr = track.Track(writeable_track_name)
+        assert tr.get_tag_raw('cover') is None
+        assert tr.get_tag_disk('cover') is not None
+
+        # Can we delete a cover?
+        tr.set_tag_disk('cover', None)
+        assert tr.write_tags() is not False
+
+        self.verify_tags_exist(tr, writeable_track, deleted='cover')
+
+        # Can we write a new cover?
+        tr.set_tag_disk('cover', newcover)
+        assert tr.get_tag_raw('cover') is None
+        assert tr.get_tag_disk('cover') == [newcover]
+
+        # reading the tags shouldn't change anything, since we're reading from disk
+        assert tr.read_tags() is not False
+        assert tr.get_tag_raw('cover') is None
+        assert tr.get_tag_disk('cover') == [newcover]
+
+        self.verify_tags_exist(tr, writeable_track)
+
     def test_write_tag_invalid_format(self):
         tr = track.Track('/tmp/foo.foo')
         assert tr.write_tags() == False
@@ -514,23 +594,31 @@ def test_get_search_tag_bitrate(self):
         tr.set_tag_raw('__bitrate', 48000)
         assert tr.get_tag_search('__bitrate') == '__bitrate=="48k" __bitrate=="48000"'
 
-    def test_get_disk_tag(self, test_tracks):
-        td = test_tracks.get('.mp3')
-        tr = track.Track(td.filename)
-        assert tr.get_tag_disk('artist') == [u'Delerium']
-
     def test_get_disk_tag_invalid_format(self):
         tr = track.Track('/tmp/foo.bah')
         assert tr.get_tag_disk('artist') == None
 
-    def test_list_disk_tag(self, test_tracks):
-        td = test_tracks.get('.ogg')
-        tr = track.Track(td.filename)
-        assert set(tr.list_tags_disk()) == \
-                        {'album', 'tracknumber', 'artist', 'title'}
-
     def test_list_disk_tag_invalid_format(self):
         tr_name = '/tmp/foo.foo'
         tr = track.Track(tr_name)
         assert tr.list_tags_disk() == None
 
+    def test_read_real_tracks(self, test_track):
+        if not test_track.has_tags:
+            return
+
+        tr = track.Track(test_track.filename)
+
+        # raw tags
+        assert tr.get_tag_raw('album') == [u'Chimera']
+        assert tr.get_tag_raw('artist') == [u'Delerium']
+        assert tr.get_tag_raw('title') == [u'Truly']
+        assert tr.get_tag_raw('tracknumber') in [[u'5'], [u'5/0']]
+
+        # disk tags should be the same
+        assert tr.get_tag_disk('album') == [u'Chimera']
+        assert tr.get_tag_disk('artist') == [u'Delerium']
+        assert tr.get_tag_disk('title') == [u'Truly']
+        assert tr.get_tag_disk('tracknumber') in [[u'5'], [u'5/0']]
+
+        self.verify_tags_exist(tr, test_track)

xl/metadata/__init__.py

@@ -29,7 +29,7 @@
 import sys
 from gi.repository import Gio
 
-from xl.metadata._base import BaseFormat, NotWritable, NotReadable
+from xl.metadata._base import BaseFormat, CoverImage, NotWritable, NotReadable
 import urlparse
 
 from xl.metadata import (ape, asf, flac, mka, mod, mp3, mp4, mpc, ogg, sid, speex,

xl/metadata/_apev2.py

@@ -26,11 +26,11 @@
 
 
 
-from xl.metadata._base import BaseFormat
+from xl.metadata._base import CaseInsensitveBaseFormat
 from mutagen import apev2
 
 
-class ApeFormat(BaseFormat):
+class ApeFormat(CaseInsensitveBaseFormat):
     MutagenType = apev2.APEv2
     # TODO: this mapping is incomplete
     tag_mapping = {