diff --git a/beets/mediafile.py b/beets/mediafile.py index 29cb4f396..a1a020fc6 100644 --- a/beets/mediafile.py +++ b/beets/mediafile.py @@ -98,10 +98,11 @@ def _safe_cast(out_type, val): returned. out_type should be bool, int, or unicode; otherwise, the value is just passed through. """ + if val is None: + return None + if out_type == int: - if val is None: - return 0 - elif isinstance(val, int) or isinstance(val, float): + if isinstance(val, int) or isinstance(val, float): # Just a number. return int(val) else: @@ -116,30 +117,22 @@ def _safe_cast(out_type, val): return int(val) elif out_type == bool: - if val is None: + try: + # Should work for strings, bools, ints: + return bool(int(val)) + except ValueError: return False - else: - try: - # Should work for strings, bools, ints: - return bool(int(val)) - except ValueError: - return False elif out_type == unicode: - if val is None: - return u'' + if isinstance(val, str): + return val.decode('utf8', 'ignore') + elif isinstance(val, unicode): + return val else: - if isinstance(val, str): - return val.decode('utf8', 'ignore') - elif isinstance(val, unicode): - return val - else: - return unicode(val) + return unicode(val) elif out_type == float: - if val is None: - return 0.0 - elif isinstance(val, int) or isinstance(val, float): + if isinstance(val, int) or isinstance(val, float): return float(val) else: if not isinstance(val, basestring): @@ -342,8 +335,9 @@ class StorageStyle(object): describe more sophisticated translations or format-specific access strategies. - MediaFile uses a StorageStyle via two methods: ``get()`` and - ``set()``. It passes a Mutagen file object to each. + MediaFile uses a StorageStyle via three methods: ``get()``, + ``set()``, and ``delete()``. It passes a Mutagen file object to + each. Internally, the StorageStyle implements ``get()`` and ``set()`` using two steps that may be overridden by subtypes. To get a value, @@ -447,6 +441,12 @@ class StorageStyle(object): return value + def delete(self, mutagen_file): + """Remove the tag from the file. + """ + if self.key in mutagen_file: + del mutagen_file[self.key] + class ListStorageStyle(StorageStyle): """Abstract storage style that provides access to lists. @@ -510,9 +510,7 @@ class SoundCheckStorageStyleMixin(object): """ def get(self, mutagen_file): data = self.fetch(mutagen_file) - if data is None: - return 0 - else: + if data is not None: return _sc_decode(data)[self.index] def set(self, mutagen_file, value): @@ -563,7 +561,13 @@ class MP4TupleStorageStyle(MP4StorageStyle): return list(items) + [0] * (packing_length - len(items)) def get(self, mutagen_file): - return super(MP4TupleStorageStyle, self).get(mutagen_file)[self.index] + value = super(MP4TupleStorageStyle, self).get(mutagen_file)[self.index] + if value == 0: + # The values are always present and saved as integers. So we + # assume that "0" indicates it is not set. + return None + else: + return value def set(self, mutagen_file, value): if value is None: @@ -572,6 +576,12 @@ class MP4TupleStorageStyle(MP4StorageStyle): items[self.index] = int(value) self.store(mutagen_file, items) + def delete(self, mutagen_file): + if self.index == 0: + super(MP4TupleStorageStyle, self).delete(mutagen_file) + else: + self.set(mutagen_file, None) + class MP4ListStorageStyle(ListStorageStyle, MP4StorageStyle): pass @@ -725,6 +735,14 @@ class MP3DescStorageStyle(MP3StorageStyle): except IndexError: return None + def delete(self, mutagen_file): + frame = None + for frame in mutagen_file.tags.getall(self.key): + if frame.desc.lower() == self.description.lower(): + break + if frame is not None: + del mutagen_file[frame.HashKey] + class MP3SlashPackStorageStyle(MP3StorageStyle): """Store value as part of pair that is serialized as a slash- @@ -735,23 +753,32 @@ class MP3SlashPackStorageStyle(MP3StorageStyle): self.pack_pos = pack_pos def _fetch_unpacked(self, mutagen_file): - data = self.fetch(mutagen_file) or '' - items = unicode(data).split('/') + data = self.fetch(mutagen_file) + if data: + items = unicode(data).split('/') + else: + items = [] packing_length = 2 return list(items) + [None] * (packing_length - len(items)) def get(self, mutagen_file): - return self._fetch_unpacked(mutagen_file)[self.pack_pos] or 0 + return self._fetch_unpacked(mutagen_file)[self.pack_pos] def set(self, mutagen_file, value): items = self._fetch_unpacked(mutagen_file) items[self.pack_pos] = value if items[0] is None: - items[0] = 0 + items[0] = '' if items[1] is None: items.pop() # Do not store last value self.store(mutagen_file, '/'.join(map(unicode, items))) + def delete(self, mutagen_file): + if self.pack_pos == 0: + super(MP3SlashPackStorageStyle, self).delete(mutagen_file) + else: + self.set(mutagen_file, None) + class MP3ImageStorageStyle(ListStorageStyle, MP3StorageStyle): """Converts between APIC frames and ``Image`` instances. @@ -944,6 +971,10 @@ class MediaField(object): for style in self.styles(mediafile.mgfile): style.set(mediafile.mgfile, value) + def __delete__(self, mediafile): + for style in self.styles(mediafile.mgfile): + style.delete(mediafile.mgfile) + def _none_value(self): """Get an appropriate "null" value for this field's type. This is used internally when setting the field to None. @@ -1006,18 +1037,25 @@ class DateField(MediaField): def __get__(self, mediafile, owner=None): year, month, day = self._get_date_tuple(mediafile) + if not year: + return None try: return datetime.date( - year or datetime.MINYEAR, + year, month or 1, day or 1 ) except ValueError: # Out of range values. - return datetime.date.min + return None def __set__(self, mediafile, date): self._set_date_tuple(mediafile, date.year, date.month, date.day) + def __delete__(self, mediafile): + super(DateField, self).__delete__(mediafile) + if hasattr(self, '_year_field'): + self._year_field.__delete__(mediafile) + def _get_date_tuple(self, mediafile): """Get a 3-item sequence representing the date consisting of a year, month, and day number. Each number is either an integer or @@ -1025,8 +1063,11 @@ class DateField(MediaField): """ # Get the underlying data and split on hyphens. datestring = super(DateField, self).__get__(mediafile, None) - datestring = re.sub(r'[Tt ].*$', '', unicode(datestring)) - items = unicode(datestring).split('-') + if isinstance(datestring, basestring): + datestring = re.sub(r'[Tt ].*$', '', unicode(datestring)) + items = unicode(datestring).split('-') + else: + items = [] # Ensure that we have exactly 3 components, possibly by # truncating or padding. @@ -1039,14 +1080,22 @@ class DateField(MediaField): items[0] = self._year_field.__get__(mediafile) # Convert each component to an integer if possible. - return [_safe_cast(int, item) for item in items] + items_ = [] + for item in items: + try: + items_.append(int(item)) + except: + items_.append(None) + return items_ def _set_date_tuple(self, mediafile, year, month=None, day=None): """Set the value of the field given a year, month, and day number. Each number can be an integer or None to indicate an unset component. """ - date = [year or 0] + if year is None: + self.__delete__(mediafile) + date = [year] if month: date.append(month) if month and day: @@ -1083,6 +1132,9 @@ class DateItemField(MediaField): items[self.item_pos] = value self.date_field._set_date_tuple(mediafile, *items) + def __delete__(self, mediafile): + self.__set__(mediafile, None) + class CoverArtField(MediaField): """A descriptor that provides access to the *raw image data* for the @@ -1104,6 +1156,9 @@ class CoverArtField(MediaField): else: mediafile.images = [] + def __delete__(self, mediafile): + delattr(mediafile, 'images') + class ImageListField(MediaField): """Descriptor to access the list of images embedded in tags. @@ -1298,12 +1353,16 @@ class MediaFile(object): """Set all field values from a dictionary. For any key in `dict` that is also a field to store tags the - method retrieves the corresponding value from `dict` and - updates the `MediaFile`. + method retrieves the corresponding value from `dict` and updates + the `MediaFile`. If a key has the value `None`, the + corresponding property is deleted from the `MediaFile`. """ for field in self.fields(): if field in dict: - setattr(self, field, dict[field]) + if dict[field] is None: + delattr(self, field) + else: + setattr(self, field, dict[field]) # Field definitions. diff --git a/test/rsrc/empty.wma b/test/rsrc/empty.wma index 76ea8a235..b4874c14c 100644 Binary files a/test/rsrc/empty.wma and b/test/rsrc/empty.wma differ diff --git a/test/test_mediafile.py b/test/test_mediafile.py index 1cc6c85d6..060f7cf2c 100644 --- a/test/test_mediafile.py +++ b/test/test_mediafile.py @@ -284,7 +284,7 @@ class ExtendedFieldTestMixin(object): plugin.add_media_field('initialkey', field_extension) mediafile = self._mediafile_fixture('empty') - self.assertEqual(mediafile.initialkey, '') + self.assertIsNone(mediafile.initialkey) item = Item(path=mediafile.path, initialkey='Gb') item.write() @@ -333,8 +333,8 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, 'composer': u'the composer', 'grouping': u'the grouping', 'year': 2001, - 'month': 0, - 'day': 0, + 'month': None, + 'day': None, 'date': datetime.date(2001, 1, 1), 'track': 2, 'tracktotal': 3, @@ -351,59 +351,57 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, 'label': u'the label', } - empty_tags = { - 'title': u'', - 'artist': u'', - 'album': u'', - 'genre': u'', - 'composer': u'', - 'grouping': u'', - 'year': 0, - 'month': 0, - 'day': 0, - 'date': datetime.date.min, - 'track': 0, - 'tracktotal': 0, - 'disc': 0, - 'disctotal': 0, - 'lyrics': u'', - 'comments': u'', - 'bpm': 0, - 'comp': False, - 'mb_trackid': u'', - 'mb_albumid': u'', - 'mb_artistid':u'', - 'art': None, - 'label': u'', - - # Additional, non-iTunes fields. - 'rg_track_peak': 0.0, - 'rg_track_gain': 0.0, - 'rg_album_peak': 0.0, - 'rg_album_gain': 0.0, - 'albumartist': u'', - 'mb_albumartistid': u'', - 'artist_sort': u'', - 'albumartist_sort': u'', - 'acoustid_fingerprint': u'', - 'acoustid_id': u'', - 'mb_releasegroupid': u'', - 'asin': u'', - 'catalognum': u'', - 'disctitle': u'', - 'script': u'', - 'language': u'', - 'country': u'', - 'albumstatus': u'', - 'media': u'', - 'albumdisambig': u'', - 'artist_credit': u'', - 'albumartist_credit': u'', - 'original_year': 0, - 'original_month': 0, - 'original_day': 0, - 'original_date': datetime.date.min, - } + tag_fields = [ + 'title', + 'artist', + 'album', + 'genre', + 'composer', + 'grouping', + 'year', + 'month', + 'day', + 'date', + 'track', + 'tracktotal', + 'disc', + 'disctotal', + 'lyrics', + 'comments', + 'bpm', + 'comp', + 'mb_trackid', + 'mb_albumid', + 'mb_artistid', + 'art', + 'label', + 'rg_track_peak', + 'rg_track_gain', + 'rg_album_peak', + 'rg_album_gain', + 'albumartist', + 'mb_albumartistid', + 'artist_sort', + 'albumartist_sort', + 'acoustid_fingerprint', + 'acoustid_id', + 'mb_releasegroupid', + 'asin', + 'catalognum', + 'disctitle', + 'script', + 'language', + 'country', + 'albumstatus', + 'media', + 'albumdisambig', + 'artist_credit', + 'albumartist_credit', + 'original_year', + 'original_month', + 'original_day', + 'original_date', + ] def setUp(self): self.temp_dir = tempfile.mkdtemp() @@ -427,7 +425,8 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, def test_read_empty(self): mediafile = self._mediafile_fixture('empty') - self.assertTags(mediafile, self.empty_tags) + for field in self.tag_fields: + self.assertIsNone(getattr(mediafile, field)) def test_write_empty(self): mediafile = self._mediafile_fixture('empty') @@ -508,8 +507,8 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, mediafile = MediaFile(mediafile.path) self.assertEqual(mediafile.year, 2001) - self.assertEqual(mediafile.month, 0) - self.assertEqual(mediafile.day, 0) + self.assertIsNone(mediafile.month) + self.assertIsNone(mediafile.day) self.assertEqual(mediafile.date, datetime.date(2001,1,1)) def test_write_dates(self): @@ -528,22 +527,6 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, self.assertEqual(mediafile.original_day, 30) self.assertEqual(mediafile.original_date, datetime.date(1999,12,30)) - def test_read_write_float_none(self): - mediafile = self._mediafile_fixture('full') - mediafile.rg_track_gain = None - mediafile.rg_track_peak = None - mediafile.original_year = None - mediafile.original_month = None - mediafile.original_day = None - mediafile.save() - - mediafile = MediaFile(mediafile.path) - self.assertEqual(mediafile.rg_track_gain, 0) - self.assertEqual(mediafile.rg_track_peak, 0) - self.assertEqual(mediafile.original_year, 0) - self.assertEqual(mediafile.original_month, 0) - self.assertEqual(mediafile.original_day, 0) - def test_write_packed(self): mediafile = self._mediafile_fixture('empty') @@ -563,22 +546,82 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, self.assertEqual(mediafile.disctotal, 5) mediafile.track = 10 - mediafile.tracktotal = None + delattr(mediafile, 'tracktotal') mediafile.disc = 10 - mediafile.disctotal = None + delattr(mediafile, 'disctotal') mediafile.save() mediafile = MediaFile(mediafile.path) self.assertEqual(mediafile.track, 10) - self.assertEqual(mediafile.tracktotal, 0) + self.assertEqual(mediafile.tracktotal, None) self.assertEqual(mediafile.disc, 10) - self.assertEqual(mediafile.disctotal, 0) + self.assertEqual(mediafile.disctotal, None) def test_unparseable_date(self): mediafile = self._mediafile_fixture('unparseable') - self.assertEqual(mediafile.year, 0) - self.assertEqual(mediafile.date, datetime.date.min) + self.assertIsNone(mediafile.date) + self.assertIsNone(mediafile.year) + self.assertIsNone(mediafile.month) + self.assertIsNone(mediafile.day) + + def test_delete_tag(self): + mediafile = self._mediafile_fixture('full') + + keys = self.full_initial_tags.keys() + for key in set(keys) - set(['art', 'month', 'day']): + self.assertIsNotNone(getattr(mediafile, key)) + for key in keys: + delattr(mediafile, key) + + mediafile.save() + mediafile = MediaFile(mediafile.path) + + for key in keys: + self.assertIsNone(getattr(mediafile, key)) + + def test_delete_packed_total(self): + mediafile = self._mediafile_fixture('full') + + delattr(mediafile, 'tracktotal') + delattr(mediafile, 'disctotal') + + mediafile.save() + mediafile = MediaFile(mediafile.path) + self.assertEqual(mediafile.track, self.full_initial_tags['track']) + self.assertEqual(mediafile.disc, self.full_initial_tags['disc']) + + def test_delete_partial_date(self): + mediafile = self._mediafile_fixture('empty') + + mediafile.date = datetime.date(2001, 12, 3) + mediafile.save() + mediafile = MediaFile(mediafile.path) + self.assertIsNotNone(mediafile.date) + self.assertIsNotNone(mediafile.year) + self.assertIsNotNone(mediafile.month) + self.assertIsNotNone(mediafile.day) + + delattr(mediafile, 'month') + mediafile.save() + mediafile = MediaFile(mediafile.path) + self.assertIsNotNone(mediafile.date) + self.assertIsNotNone(mediafile.year) + self.assertIsNone(mediafile.month) + self.assertIsNone(mediafile.day) + + def test_delete_year(self): + mediafile = self._mediafile_fixture('full') + + self.assertIsNotNone(mediafile.date) + self.assertIsNotNone(mediafile.year) + + delattr(mediafile, 'year') + mediafile.save() + mediafile = MediaFile(mediafile.path) + self.assertIsNone(mediafile.date) + self.assertIsNone(mediafile.year) + def assertTags(self, mediafile, tags): errors = [] @@ -606,20 +649,19 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, """Return dictionary of tags, mapping tag names to values. """ tags = {} - if base is None: - base = self.empty_tags - for key, value in base.items(): - if key == 'art': - tags[key] = self.jpg_data - elif isinstance(value, unicode): - tags[key] = 'value\u2010%s' % key - elif isinstance(value, int): - tags[key] = 1 - elif isinstance(value, float): + for key in self.tag_fields: + if key.startswith('rg_'): + # ReplayGain is float tags[key] = 1.0 - elif isinstance(value, bool): - tags[key] = True + else: + tags[key] = 'value\u2010%s' % key + + for key in ['disc', 'disctotal', 'track', 'tracktotal', 'bpm']: + tags[key] = 1 + + tags['art'] = self.jpg_data + tags['comp'] = True date = datetime.date(2001, 4, 3) tags['date'] = date @@ -646,9 +688,9 @@ class PartialTestMixin(object): def test_read_track_without_total(self): mediafile = self._mediafile_fixture('partial') self.assertEqual(mediafile.track, 2) - self.assertEqual(mediafile.tracktotal, 0) + self.assertIsNone(mediafile.tracktotal) self.assertEqual(mediafile.disc, 4) - self.assertEqual(mediafile.disctotal, 0) + self.assertIsNone(mediafile.disctotal) class MP3Test(ReadWriteTestBase, PartialTestMixin, @@ -817,7 +859,7 @@ class MediaFieldTest(unittest.TestCase): self.assertTrue(hasattr(mediafile, field)) def test_known_fields(self): - fields = ReadWriteTestBase.empty_tags.keys() + fields = list(ReadWriteTestBase.tag_fields) fields.extend(('encoder', 'images', 'genres', 'albumtype')) self.assertItemsEqual(MediaFile.fields(), fields) diff --git a/test/test_mediafile_edge.py b/test/test_mediafile_edge.py index f87eaf132..03017430e 100644 --- a/test/test_mediafile_edge.py +++ b/test/test_mediafile_edge.py @@ -30,7 +30,7 @@ class EdgeTest(unittest.TestCase): emptylist = beets.mediafile.MediaFile( os.path.join(_common.RSRC, 'emptylist.mp3')) genre = emptylist.genre - self.assertEqual(genre, '') + self.assertEqual(genre, None) def test_release_time_with_space(self): # Ensures that release times delimited by spaces are ignored. @@ -215,7 +215,7 @@ class TypeTest(unittest.TestCase): def test_set_year_to_none(self): self.mf.year = None - self.assertEqual(self.mf.year, 0) + self.assertIsNone(self.mf.year) def test_set_track_to_none(self): self.mf.track = None