Merge pull request #1944 from zas/file_update_rework

File.update() rework & testing

picard/file.py

@@ -652,25 +652,35 @@ class File(QtCore.QObject, Item):
     def is_saved(self):
         return self.similarity == 1.0 and self.state == File.NORMAL
 
+    def _tags_to_update(self, ignored_tags):
+        for name in set(self.metadata) | set(self.orig_metadata):
+            if name.startswith('~'):
+                continue
+            if not self.supports_tag(name):
+                continue
+            if name in ignored_tags:
+                continue
+            yield name
+
     def update(self, signal=True):
-        metadata = self.metadata
-        names = set(metadata) | set(self.orig_metadata)
         config = get_config()
         clear_existing_tags = config.setting["clear_existing_tags"]
-        ignored_tags = config.setting["compare_ignore_tags"]
-        for name in names:
-            if (not name.startswith('~') and self.supports_tag(name)
-                and name not in ignored_tags):
-                new_values = metadata.getall(name)
-                if not (new_values or clear_existing_tags
-                        or name in metadata.deleted_tags):
-                    continue
-                orig_values = self.orig_metadata.getall(name)
-                if orig_values != new_values:
-                    self.similarity = self.orig_metadata.compare(metadata, ignored_tags)
-                    if self.state == File.NORMAL:
-                        self.state = File.CHANGED
-                    break
+        ignored_tags = set(config.setting["compare_ignore_tags"])
+
+        for name in self._tags_to_update(ignored_tags):
+            new_values = self.metadata.getall(name)
+            if not (
+                new_values
+                or clear_existing_tags
+                or name in self.metadata.deleted_tags
+            ):
+                continue
+            orig_values = self.orig_metadata.getall(name)
+            if orig_values != new_values:
+                self.similarity = self.orig_metadata.compare(self.metadata, ignored_tags)
+                if self.state == File.NORMAL:
+                    self.state = File.CHANGED
+                break
         else:
             if (self.metadata.images
                     and self.orig_metadata.images != self.metadata.images):

test/test_file.py

@@ -23,10 +23,12 @@
 
 import os
 import re
+from types import GeneratorType
 import unittest
 from unittest.mock import MagicMock
 
 from test.picardtestcase import PicardTestCase
+from test.test_coverart_image import create_image
 
 from picard import config
 from picard.const.sys import (
@@ -402,3 +404,191 @@ class FileAdditionalFilesPatternsTest(PicardTestCase):
             (re.compile('(?s:.*\\.jpg)\\Z', re.IGNORECASE), False),
         }
         self.assertEqual(File._compile_move_additional_files_pattern(pattern), expected)
+
+
+class FileUpdateTest(PicardTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.file = File('/somepath/somefile.mp3')
+        self.INVALIDSIMVAL = 666
+        self.file.similarity = self.INVALIDSIMVAL  # to check if changed or not
+        self.file.supports_tag = lambda x: False if x.startswith('unsupported') else True
+        self.set_config_values({
+            'clear_existing_tags': False,
+            'compare_ignore_tags': [],
+            'enabled_plugins': [],
+        })
+
+    def test_same_image(self):
+        image = create_image(b'a')
+        self.file.metadata.images = [image]
+        self.file.orig_metadata.images = [image]
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)  # it should be modified
+        self.assertEqual(self.file.state, File.NORMAL)
+
+    def test_same_image_pending(self):
+        image = create_image(b'a')
+        self.file.metadata.images = [image]
+        self.file.orig_metadata.images = [image]
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.PENDING)
+
+    def test_same_image_changed_state(self):
+        image = create_image(b'a')
+        self.file.metadata.images = [image]
+        self.file.orig_metadata.images = [image]
+        self.file.state = File.CHANGED
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.NORMAL)
+
+    def test_changed_image(self):
+        old_image = create_image(b'a')
+        new_image = create_image(b'b')
+        self.file.metadata.images = [new_image]
+        self.file.orig_metadata.images = [old_image]
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, self.INVALIDSIMVAL)  # it shouldbn't be modified
+        self.assertEqual(self.file.state, File.CHANGED)
+
+    def test_signal(self):
+        #  just for coverage
+        self.file.update(signal=True)
+        self.assertEqual(self.file.metadata, Metadata())
+        self.assertEqual(self.file.orig_metadata, Metadata())
+
+    def test_tags_to_update(self):
+        self.file.orig_metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+            'ignoreme_old': 'a',
+            '~ignoreme_old': 'b',
+            'unsupported_old': 'c',
+        })
+        self.file.metadata = Metadata({
+            'artist': 'someartist',
+            'ignoreme_new': 'd',
+            '~ignoreme_new': 'e',
+            'unsupported_new': 'f',
+        })
+
+        ignore_tags = {'ignoreme_old', 'ignoreme_new'}
+
+        expected = {'album', 'title', 'artist'}
+        result = self.file._tags_to_update(ignore_tags)
+        self.assertIsInstance(result, GeneratorType)
+        self.assertEqual(set(result), expected)
+
+    def test_unchanged_metadata(self):
+        self.file.orig_metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+        })
+        self.file.metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+        })
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.NORMAL)
+
+    def test_changed_metadata(self):
+        self.file.orig_metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+        })
+        self.file.metadata = Metadata({
+            'album': 'somealbum2',
+            'title': 'sometitle2',
+        })
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertLess(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.CHANGED)
+
+    def test_changed_metadata_pending(self):
+        self.file.orig_metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+        })
+        self.file.metadata = Metadata({
+            'album': 'somealbum2',
+            'title': 'sometitle2',
+        })
+
+        self.file.update(signal=False)
+        self.assertLess(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.PENDING)  # it shouldn't be modified
+
+    def test_clear_existing(self):
+        self.file.orig_metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+        })
+        self.file.metadata = Metadata()
+        self.file.state = File.NORMAL
+
+        config.setting["clear_existing_tags"] = True
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 0.0)
+        self.assertEqual(self.file.state, File.CHANGED)
+
+    def test_no_new_metadata(self):
+        self.file.orig_metadata = Metadata({
+            'album': 'somealbum',
+            'title': 'sometitle',
+        })
+        self.file.metadata = Metadata()
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.NORMAL)
+
+    def test_tilde_tag(self):
+        self.file.orig_metadata = Metadata()
+        self.file.metadata = Metadata({
+            '~tag': 'value'
+        })
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.NORMAL)
+
+    def test_ignored_tag(self):
+        self.file.orig_metadata = Metadata()
+        self.file.metadata = Metadata({
+            'tag': 'value'
+        })
+        self.file.state = File.NORMAL
+
+        config.setting["compare_ignore_tags"] = ['tag']
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.NORMAL)
+
+    def test_unsupported_tag(self):
+        self.file.orig_metadata = Metadata()
+        self.file.metadata = Metadata({
+            'unsupported': 'value'
+        })
+        self.file.state = File.NORMAL
+
+        self.file.update(signal=False)
+        self.assertEqual(self.file.similarity, 1.0)
+        self.assertEqual(self.file.state, File.NORMAL)