File 0001-Fix-deletion-of-comment-performer-and-lyri... of Package picard

Overview Repositories Revisions Requests Users Attributes Meta

File 0001-Fix-deletion-of-comment-performer-and-lyrics-tags.patch of Package picard (Revision 6d2e829ca47511b43f3a0049b827560e)

Currently displaying revision 6d2e829ca47511b43f3a0049b827560e , Show latest

From 57c8fe209afe0e5cb5cdacf3e67145192ef483dc Mon Sep 17 00:00:00 2001
From: Philipp Wolfer <ph.wolfer@gmail.com>
Date: Tue, 3 Mar 2020 17:09:57 +0100
Subject: [PATCH] PICARD-1783: Fix deletion of comment, performer and lyrics
 tags

This adds tests for deleting comment, performer and lyrics tags and fixes various issues with this in ID3, MP4, Vorbis and APEv2.
---
 picard/formats/apev2.py  | 21 ++++++++++--------
 picard/formats/id3.py    |  2 +-
 picard/formats/mp4.py    |  8 +++----
 picard/formats/vorbis.py | 16 +++++++++-----
 test/formats/common.py   | 48 +++++++++++++++++++++++++++++++++++-----
 test/formats/test_mp4.py | 14 +++++++++++-
 6 files changed, 84 insertions(+), 25 deletions(-)

diff --git a/picard/formats/apev2.py b/picard/formats/apev2.py
index 9d5047684..9b1bec03b 100644
--- a/picard/formats/apev2.py
+++ b/picard/formats/apev2.py
@@ -169,7 +169,7 @@ def _load(self, filename):
                         if len(disc) > 1:
                             metadata["totaldiscs"] = disc[1]
                             value = disc[0]
-                    elif name == 'performer' or name == 'comment':
+                    elif name in ('performer', 'comment'):
                         if value.endswith(')'):
                             start = value.rfind(' (')
                             if start > 0:
@@ -236,14 +236,17 @@ def _remove_deleted_tags(self, metadata, tags):
         """Remove the tags from the file that were deleted in the UI"""
         for tag in metadata.deleted_tags:
             real_name = self._get_tag_name(tag)
-            if (real_name in ('Lyrics', 'Comment', 'Performer')
-                and ':' in tag and not tag.endswith(':')):
-                tag_type = re.compile(r"$%s$$" % tag.split(':', 1)[1])
-                existing_tags = tags.get(real_name)
-                if existing_tags:
-                    for item in existing_tags:
-                        if tag_type.search(item):
-                            tags.get(real_name).remove(item)
+            if real_name in ('Lyrics', 'Comment', 'Performer'):
+                parts = tag.split(':', 1)
+                if len(parts) == 2:
+                    tag_type_regex = r"$%s$$" % parts[1]
+                else:
+                    tag_type_regex = r"[^)]$"
+                existing_tags = tags.get(real_name, [])
+                for item in existing_tags:
+                    if re.search(tag_type_regex, item):
+                        existing_tags.remove(item)
+                tags[real_name] = existing_tags
             elif tag in ('totaltracks', 'totaldiscs'):
                 tagstr = real_name.lower() + 'number'
                 if tagstr in metadata:
diff --git a/picard/formats/id3.py b/picard/formats/id3.py
index ccba1193d..3940d056c 100644
--- a/picard/formats/id3.py
+++ b/picard/formats/id3.py
@@ -547,7 +547,7 @@ def _remove_deleted_tags(self, metadata, tags):
                             for people in frame.people:
                                 if people[0] == role:
                                     frame.people.remove(people)
-                elif name.startswith('comment:'):
+                elif name.startswith('comment:') or name == 'comment':
                     (lang, desc) = parse_comment_tag(name)
                     if desc.lower()[:4] != 'itun':
                         for key, frame in list(tags.items()):
diff --git a/picard/formats/mp4.py b/picard/formats/mp4.py
index d459c3a60..e37bb7017 100644
--- a/picard/formats/mp4.py
+++ b/picard/formats/mp4.py
@@ -280,8 +280,7 @@ def _save(self, filename, metadata):
                 tags[name] = values
             elif name == "musicip_fingerprint":
                 tags["----:com.apple.iTunes:fingerprint"] = [b"MusicMagic Fingerprint%s" % v.encode('ascii') for v in values]
-            elif self.supports_tag(name) and name not in ('tracknumber',
-                    'totaltracks', 'discnumber', 'totaldiscs'):
+            elif self.supports_tag(name) and name not in self.__other_supported_tags:
                 values = [v.encode("utf-8") for v in values]
                 name = self.__casemap.get(name, name)
                 tags['----:com.apple.iTunes:' + name] = values
@@ -351,8 +350,9 @@ def _get_tag_name(self, name):
             return "trkn"
         elif name in ("discnumber", "totaldiscs"):
             return "disk"
-        else:
-            return None
+        elif self.supports_tag(name) and name not in self.__other_supported_tags:
+            name = self.__casemap.get(name, name)
+            return '----:com.apple.iTunes:' + name
 
     def _info(self, metadata, file):
         super()._info(metadata, file)
diff --git a/picard/formats/vorbis.py b/picard/formats/vorbis.py
index d37c73521..4e9d33a0e 100644
--- a/picard/formats/vorbis.py
+++ b/picard/formats/vorbis.py
@@ -4,7 +4,7 @@
 #
 # Copyright (C) 2006-2008, 2012 Lukáš Lalinský
 # Copyright (C) 2008 Hendrik van Antwerpen
-# Copyright (C) 2008-2010, 2014-2015, 2018-2019 Philipp Wolfer
+# Copyright (C) 2008-2010, 2014-2015, 2018-2020 Philipp Wolfer
 # Copyright (C) 2012-2013 Michael Wiencek
 # Copyright (C) 2012-2014 Wieland Hoffmann
 # Copyright (C) 2013 Calvin Walton
@@ -316,10 +316,16 @@ def _remove_deleted_tags(self, metadata, tags):
             real_name = self._get_tag_name(tag)
             if real_name and real_name in tags:
                 if real_name in ('performer', 'comment'):
-                    tag_type = r"$%s$" % tag.split(':', 1)[1]
-                    for item in tags.get(real_name):
-                        if re.search(tag_type, item):
-                            tags.get(real_name).remove(item)
+                    parts = tag.split(':', 1)
+                    if len(parts) == 2:
+                        tag_type_regex = r"$%s$$" % parts[1]
+                    else:
+                        tag_type_regex = r"[^)]$"
+                    existing_tags = tags.get(real_name)
+                    for item in existing_tags:
+                        if re.search(tag_type_regex, item):
+                            existing_tags.remove(item)
+                    tags[real_name] = existing_tags
                 else:
                     if tag in ('totaldiscs', 'totaltracks'):
                         # both tag and real_name are to be deleted in this case
diff --git a/test/formats/common.py b/test/formats/common.py
index ac830fd87..0974576e6 100644
--- a/test/formats/common.py
+++ b/test/formats/common.py
@@ -149,6 +149,7 @@ def save_raw(filename, tags):
     'originaldate': '1980-01-20',
     'originalyear': '1980',
     'originalfilename': 'Foo',
+    'performer': 'Foo',
     'performer:guest vocal': 'Foo',
     'podcast': '1',
     'podcasturl': 'Foo',
@@ -330,14 +331,51 @@ def test_delete_simple_tags(self):
                 self.assertNotIn('~rating', new_metadata.keys())
 
         @skipUnlessTestfile
-        def test_delete_lyrics_tags(self):
-            for key in ('lyrics', 'lyrics:'):
-                metadata = Metadata(self.tags)
+        def test_delete_tags_with_empty_description(self):
+            for key in ('lyrics', 'lyrics:', 'comment', 'comment:', 'performer', 'performer:'):
+                name = key.rstrip(':')
+                name_with_description = name + ':foo'
+                if not self.format.supports_tag(name):
+                    continue
+                metadata = Metadata()
+                metadata[name] = 'bar'
+                metadata[name_with_description] = 'other'
                 original_metadata = save_and_load_metadata(self.filename, metadata)
-                self.assertIn('lyrics', original_metadata)
+                self.assertIn(name, original_metadata)
+                del metadata[key]
+                new_metadata = save_and_load_metadata(self.filename, metadata)
+                self.assertNotIn(name, new_metadata)
+                # Ensure the names with description did not get deleted
+                if name_with_description in original_metadata:
+                    self.assertIn(name_with_description, new_metadata)
+
+        @skipUnlessTestfile
+        def test_delete_tags_with_description(self):
+            for key in ('comment:foo', 'comment:de:foo', 'performer:foo', 'lyrics:foo'):
+                if not self.format.supports_tag(key):
+                    continue
+                prefix = key.split(':')[0]
+                metadata = Metadata()
+                metadata[key] = 'bar'
+                metadata[prefix] = '(foo) bar'
+                original_metadata = save_and_load_metadata(self.filename, metadata)
+                if not key in original_metadata and prefix in original_metadata:
+                    continue  # Skip if the type did not support saving this kind of tag
+                del metadata[key]
+                new_metadata = save_and_load_metadata(self.filename, metadata)
+                self.assertNotIn(key, new_metadata)
+                self.assertEqual('(foo) bar', new_metadata[prefix])
+
+        @skipUnlessTestfile
+        def test_delete_nonexistant_tags(self):
+            for key in ('title', 'foo', 'comment:foo', 'comment:de:foo', 'performer:foo', 'lyrics:foo'):
+                if not self.format.supports_tag(key):
+                    continue
+                metadata = Metadata()
+                save_metadata(self.filename, metadata)
                 del metadata[key]
                 new_metadata = save_and_load_metadata(self.filename, metadata)
-                self.assertNotIn('lyrics', new_metadata)
+                self.assertNotIn(key, new_metadata)
 
         @skipUnlessTestfile
         def test_delete_non_existant_tags(self):
diff --git a/test/formats/test_mp4.py b/test/formats/test_mp4.py
index a34cccb17..116237c44 100644
--- a/test/formats/test_mp4.py
+++ b/test/formats/test_mp4.py
@@ -2,7 +2,7 @@
 #
 # Picard, the next-generation MusicBrainz tagger
 #
-# Copyright (C) 2019 Philipp Wolfer
+# Copyright (C) 2019-2020 Philipp Wolfer
 #
 # This program is free software; you can redistribute it and/or
 # modify it under the terms of the GNU General Public License
@@ -24,11 +24,13 @@
 import mutagen
 
 from picard.formats import ext_to_format
+from picard.metadata import Metadata
 
 from .common import (
     CommonTests,
     load_metadata,
     load_raw,
+    save_and_load_metadata,
     save_metadata,
     save_raw,
     skipUnlessTestfile,
@@ -93,6 +95,16 @@ def test_ci_tags_preserve_case(self):
                 self.assertEqual(1, len(raw_metadata['----:com.apple.iTunes:' + name]))
                 self.assertNotIn('----:com.apple.iTunes:' + name.upper(), raw_metadata)
 
+        @skipUnlessTestfile
+        def test_delete_freeform_tags(self):
+            metadata = Metadata()
+            metadata['foo'] = 'bar'
+            original_metadata = save_and_load_metadata(self.filename, metadata)
+            self.assertEqual('bar', original_metadata['foo'])
+            del metadata['foo']
+            new_metadata = save_and_load_metadata(self.filename, metadata)
+            self.assertNotIn('foo', new_metadata)
+
 
 class M4ATest(CommonMP4Tests.MP4TestCase):
     testfile = 'test.m4a'

Places

File 0001-Fix-deletion-of-comment-performer-and-lyrics-tags.patch of Package picard (Revision 6d2e829ca47511b43f3a0049b827560e)

Places