From 9a21f555beab9039975152a1d05e3fc36bc01fb0 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Mon, 7 Apr 2014 20:34:48 -0700 Subject: [PATCH] MediaFile.update() no longer writes files You now call update() followed by save(). This is mainly because the implementation no longer performs lazy updates, so the need to pair tag setting and writing together was diminished. Benefits in cleanliness: - No need to duplicate the id3v2 option. - Exception handling can be more precise. - More closely resembles dict.update(). --- beets/library.py | 8 +++++--- beets/mediafile.py | 29 +++++++++++++---------------- test/test_mediafile.py | 6 ++++++ 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/beets/library.py b/beets/library.py index 7d02c32d1..f9285cabb 100644 --- a/beets/library.py +++ b/beets/library.py @@ -408,7 +408,7 @@ class Item(LibModel): except (OSError, IOError) as exc: raise ReadError(read_path, exc) - for key in list(self._media_fields): + for key in self._media_fields: value = getattr(mediafile, key) if isinstance(value, (int, long)): # Filter values wider than 64 bits (in signed representation). @@ -427,7 +427,8 @@ class Item(LibModel): def write(self, path=None): """Write the item's metadata to a media file. - Updates the mediafile with properties from itself. + All fields in `_media_fields` are written to disk according to + the values on this object. Can raise either a `ReadError` or a `WriteError`. """ @@ -442,8 +443,9 @@ class Item(LibModel): plugins.send('write', item=self, path=path) + mediafile.update(self) try: - mediafile.update(self, id3v23=beets.config['id3v23'].get(bool)) + mediafile.save(id3v23=beets.config['id3v23'].get(bool)) except (OSError, IOError, MutagenError) as exc: raise WriteError(self.path, exc) diff --git a/beets/mediafile.py b/beets/mediafile.py index c5e5efefa..c25fd3c86 100644 --- a/beets/mediafile.py +++ b/beets/mediafile.py @@ -1258,9 +1258,13 @@ class MediaFile(object): for tag in self.mgfile.keys(): del self.mgfile[tag] + + # Convenient access to the set of available fields. + @classmethod def fields(cls): - """Yield the names of all properties that are MediaFields. + """Get the names of all writable properties that reflect + metadata tags (i.e., those that are `MediaField`s). """ for property, descriptor in cls.__dict__.items(): if isinstance(descriptor, MediaField): @@ -1268,13 +1272,13 @@ class MediaFile(object): @classmethod def readable_fields(cls): - """Yield the elements of ``fields()`` and all additional - properties retrieved from the file + """Get all metadata fields: the writable ones from `fields` and + also other audio properties. """ for property in cls.fields(): yield property - for property in ['length', 'samplerate', 'bitdepth', 'bitrate', - 'channels', 'format']: + for property in ('length', 'samplerate', 'bitdepth', 'bitrate', + 'channels', 'format'): yield property @classmethod @@ -1282,9 +1286,7 @@ class MediaFile(object): """Add a field to store custom tags. ``name`` is the name of the property the field is accessed - through. It must not already exist for the class. If the name - coincides with the name of a property of ``Item`` it will be set - from the item in ``item.write()``. + through. It must not already exist on this class. ``descriptor`` must be an instance of ``MediaField``. """ @@ -1296,21 +1298,16 @@ class MediaFile(object): u'property "{0}" already exists on MediaField'.format(name)) setattr(cls, name, descriptor) - def update(self, dict, id3v23=False): - """Update tags from the dictionary and write them to the file. + def update(self, dict): + """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``. The changes are then written to the - disk. - - By default, MP3 files are saved with ID3v2.4 tags. You can use - the older ID3v2.3 standard by specifying the `id3v23` option. + updates the ``MediaFile``. """ for field in self.fields(): if field in dict: setattr(self, field, dict[field]) - self.save(id3v23) # Field definitions. diff --git a/test/test_mediafile.py b/test/test_mediafile.py index 6f86f6c88..5548e364a 100644 --- a/test/test_mediafile.py +++ b/test/test_mediafile.py @@ -194,6 +194,7 @@ class LazySaveTestMixin(object): self.assertEqual(os.stat(mediafile.path).st_mtime, mtime) mediafile.update({'title': mediafile.title}) + mediafile.save() self.assertEqual(os.stat(mediafile.path).st_mtime, mtime) @unittest.skip('not yet implemented') @@ -213,6 +214,7 @@ class LazySaveTestMixin(object): self.assertEqual(os.stat(mediafile.path).st_mtime, mtime) mediafile.update({'title': mediafile.title, 'album': 'another'}) + mediafile.save() self.assertNotEqual(os.stat(mediafile.path).st_mtime, mtime) def _set_past_mtime(self, path): @@ -297,6 +299,7 @@ class ExtendedFieldTestMixin(object): mediafile = self._mediafile_fixture('empty') mediafile.update({'initialkey': 'F#'}) + mediafile.save() item = Item.from_path(mediafile.path) self.assertEqual(item['initialkey'], 'F#') @@ -441,6 +444,7 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, tags = self._generate_tags() mediafile.update(tags) + mediafile.save() mediafile = MediaFile(mediafile.path) self.assertTags(mediafile, tags) @@ -466,8 +470,10 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin, tags = self._generate_tags() mediafile.update(tags) + mediafile.save() # Make sure the tags are already set when writing a second time mediafile.update(tags) + mediafile.save() mediafile = MediaFile(mediafile.path) self.assertTags(mediafile, tags)