Closed wisp3rwind closed 5 years ago
Looking good. To test this you can add the following test case to ExternalConvertTest
def test_convert_write_tags(self):
item = self.add_track(myexternal='true', format='mp4', title=u'TITLE')
# We "convert" by copying the file. Setting the title simulates
# a badly behaved converter
mediafile_converted = MediaFile(syspath(item.path))
mediafile_converted.title = u'WRONG'
mediafile_converted.save()
self.runcli('alt', 'update', 'myexternal')
item.load()
alt_mediafile = MediaFile(syspath(self.get_path(item)))
self.assertEqual(alt_mediafile.title, u'TITLE')
Thanks for the snippet, that is quite clever! I committed this as-is (and rebased & force pushed to resolve the merge conflict in the changelog).
This matches the behaviour of the convert plugin, see its source. Without this
write
call, tags in the new file can easily be be incomplete/missing: Even if the encoding program does write tags, it might not map them between formats in the way that beets would.This was the reason behind some recent "corruption" of my alternative collection: Newly added files didn't contain any tags, not even title or artist. I did not try to check what triggered this (I hadn't changed any of beets' configuration), but I would suspect that some new release of ffmpeg changed its behaviour to not write tags anymore.
I guess it would be nice to have a test, but I'm not sure how to set it up. In some way,
ExternalConvert
would have to be mocked in order to copy the file and delete tags.